[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH v3 6/6] of: Add plumbing for restricted DMA pool
On Fri, Jan 15, 2021 at 2:52 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: > > On 1/14/21 1:08 AM, Claire Chang wrote: > > On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli <f.fainelli@xxxxxxxxx> > > wrote: > >> > >> On 1/5/21 7:41 PM, Claire Chang wrote: > >>> If a device is not behind an IOMMU, we look up the device node and set > >>> up the restricted DMA when the restricted-dma-pool is presented. > >>> > >>> Signed-off-by: Claire Chang <tientzu@xxxxxxxxxxxx> > >>> --- > >> > >> [snip] > >> > >>> +int of_dma_set_restricted_buffer(struct device *dev) > >>> +{ > >>> + struct device_node *node; > >>> + int count, i; > >>> + > >>> + if (!dev->of_node) > >>> + return 0; > >>> + > >>> + count = of_property_count_elems_of_size(dev->of_node, > >>> "memory-region", > >>> + sizeof(phandle)); > >> > >> You could have an early check for count < 0, along with an error > >> message, if that is deemed useful. > >> > >>> + for (i = 0; i < count; i++) { > >>> + node = of_parse_phandle(dev->of_node, "memory-region", i); > >>> + if (of_device_is_compatible(node, "restricted-dma-pool")) > >> > >> And you may want to add here an of_device_is_available(node). A platform > >> that provides the Device Tree firmware and try to support multiple > >> different SoCs may try to determine if an IOMMU is present, and if it > >> is, it could be marking the restriced-dma-pool region with a 'status = > >> "disabled"' property, or any variant of that scheme. > > > > This function is called only when there is no IOMMU present (check in > > drivers/of/device.c). I can still add of_device_is_available(node) > > here if you think it's helpful. > > I believe it is, since boot loader can have a shared Device Tree blob > skeleton and do various adaptations based on the chip (that's what we > do) and adding a status property is much simpler than insertion new > nodes are run time. > > > > >> > >>> + return of_reserved_mem_device_init_by_idx( > >>> + dev, dev->of_node, i); > >> > >> This does not seem to be supporting more than one memory region, did not > >> you want something like instead: > >> > >> ret = of_reserved_mem_device_init_by_idx(...); > >> if (ret) > >> return ret; > >> > > > > Yes. This implement only supports one restriced-dma-pool memory region > > with the assumption that there is only one memory region with the > > compatible string, restricted-dma-pool, in the dts. IIUC, it's similar > > to shared-dma-pool. > > Then if here is such a known limitation it should be both documented and > enforced here, you shouldn ot be iterating over all of the phandles that > you find, stop at the first one and issue a warning if count > 1? What I have in mind is there might be multiple memory regions, but only one is for restriced-dma-pool. Say, if you want a separated region for coherent DMA and only do streaming DMA in this restriced-dma-pool region, you can add another reserved-memory node with shared-dma-pool in dts and the current implementation will try to allocate the memory via dma_alloc_from_dev_coherent() first (see dma_alloc_attrs() in /kernel/dma/mapping.c). Or if you have vendor specific memory region, you can still set up restriced-dma-pool by adding another reserved-memory node in dts. Dose this make sense to you? I'll document this for sure. > -- > Florian
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |