Re: [RFC PATCH v3 6/6] of: Add plumbing for restricted DMA pool

On 1/14/21 1:08 AM, Claire Chang wrote:

On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli [off-list ref] wrote:

quoted

On 1/5/21 7:41 PM, Claire Chang wrote:

quoted

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 <redacted>
---

[snip]

quoted

+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.

quoted

+     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.

quoted

quoted

+                     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?
-- 
Florian