| Message ID | 1490419893-5073-3-git-send-email-oza.oza@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Mar 25, 2017 at 12:31 AM, Oza Pawandeep <oza.oza@broadcom.com> wrote: > it jumps to the parent node without examining the child node. > also with that, it throws "no dma-ranges found for node" > for pci dma-ranges. > > this patch fixes device node traversing for dma-ranges. What's the DT look like that doesn't work? dma-ranges is supposed to be a bus property, not a device's property. So looking at the parent is correct behavior generally. Rob
Hi Rob, On 27/03/17 15:34, Rob Herring wrote: > On Sat, Mar 25, 2017 at 12:31 AM, Oza Pawandeep <oza.oza@broadcom.com> wrote: >> it jumps to the parent node without examining the child node. >> also with that, it throws "no dma-ranges found for node" >> for pci dma-ranges. >> >> this patch fixes device node traversing for dma-ranges. > > What's the DT look like that doesn't work? The problem is the bodge in pci_dma_configure() where we don't have an OF node for the actual device itself, so pass in the host bridge's OF node instead. This happens to work well enough for dma-coherent, but I don't think dma-ranges was even considered at the time. As it happens I'm currently halfway through writing an experiment wherein pci_dma_configure() creates a temporary child node for the of_dma_configure() call if no other suitable alternative (e.g. some intermediate bridge node) exists. How hard are you likely to NAK that approach? ;) > dma-ranges is supposed to be a bus property, not a device's property. > So looking at the parent is correct behavior generally. Indeed, this patch as-is will break currently correct DTs (because we won't find dma-ranges on the device, so will bail before even looking at the parent as we should). Robin. > > Rob >
please find my comments inline. On Mon, Mar 27, 2017 at 8:15 PM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Rob, > > On 27/03/17 15:34, Rob Herring wrote: >> On Sat, Mar 25, 2017 at 12:31 AM, Oza Pawandeep <oza.oza@broadcom.com> wrote: >>> it jumps to the parent node without examining the child node. >>> also with that, it throws "no dma-ranges found for node" >>> for pci dma-ranges. >>> >>> this patch fixes device node traversing for dma-ranges. >> >> What's the DT look like that doesn't work? > > The problem is the bodge in pci_dma_configure() where we don't have an > OF node for the actual device itself, so pass in the host bridge's OF > node instead. This happens to work well enough for dma-coherent, but I > don't think dma-ranges was even considered at the time. > > As it happens I'm currently halfway through writing an experiment > wherein pci_dma_configure() creates a temporary child node for the > of_dma_configure() call if no other suitable alternative (e.g. some > intermediate bridge node) exists. How hard are you likely to NAK that > approach? ;) > >> dma-ranges is supposed to be a bus property, not a device's property. >> So looking at the parent is correct behavior generally. > > Indeed, this patch as-is will break currently correct DTs (because we > won't find dma-ranges on the device, so will bail before even looking at > the parent as we should). current parsing of dma-ranges assume that dma-ranges always to be found in parent node. based on that, my thinking is following: couple of options 1) instead while(1) some meaningful condition such as while(!node) the following bail out is not required. if (!ranges) break; 2) have check based on dt-property to distinguish between pci and handle dma-ranges root bridge but again these changes do not solve the entire problem for choosing right dma_mask. neither it actually correctly address root bridge pci dma-ranges. and hence I have written https://lkml.org/lkml/2017/3/27/540 my final take is: this function does not need to change, let it parse memory mapped dma-ranges as it is doing. I am more inclined to have generic pci dma-ranges and parsing. which following already addresses. https://lkml.org/lkml/2017/3/27/540 Regards, Oza.
diff --git a/drivers/of/address.c b/drivers/of/address.c index 02b2903..3293d55 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -836,9 +836,6 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz while (1) { naddr = of_n_addr_cells(node); nsize = of_n_size_cells(node); - node = of_get_next_parent(node); - if (!node) - break; ranges = of_get_property(node, "dma-ranges", &len); @@ -852,6 +849,10 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz */ if (!ranges) break; + + node = of_get_next_parent(node); + if (!node) + break; } if (!ranges) {