| Message ID | 67d1f46172599be243405f4341665fd0ef9ab969.1490726288.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: > For PCI masters not represented in DT, we pass the OF node of their > associated host bridge to of_dma_configure(), such that they can inherit > the appropriate DMA configuration from whatever is described there. > Unfortunately, whilst this has worked for the "dma-coherent" property, > it turns out to miss the case where the host bridge node has a non-empty > "dma-ranges", since nobody is expecting the 'device' to be a bus itself. > > It transpires, though, that the de-facto interface since the prototype > change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help > re-use") is very clear-cut: either the master_np argument is redundant > with dev->of_node, or dev->of_node is NULL and master_np is the relevant > parent bus. Let's ratify that behaviour, then teach the whole > of_dma_configure() pipeline to cope with both cases properly. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > This is what I'd consider the better fix - rather than adding yet more > special cases - which will also make it simple to handle multiple > "dma-ranges" entries with minimal further disruption. The callers now > left passing dev->of_node as 'parent' are harmless, but look a bit silly > and clearly want cleaning up - I'd be partial to renaming the existing > function and having a single-argument wrapper for the 'normal' case, e.g.: > > static inline int of_dma_configure(struct device_node *dev) > { > return of_dma_configure_parent(dev, NULL); > } > > Thoughts? > > Robin. > > drivers/iommu/of_iommu.c | 7 ++++--- > drivers/of/address.c | 37 +++++++++++++++++++++++++------------ > drivers/of/device.c | 12 +++++++----- > include/linux/of_address.h | 7 ++++--- > include/linux/of_device.h | 4 ++-- > include/linux/of_iommu.h | 4 ++-- > 6 files changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 2683e9fc0dcf..35aff07bb5eb 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -138,21 +138,22 @@ static const struct iommu_ops > } > > const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np) > + struct device_node *parent) > { > struct of_phandle_args iommu_spec; > - struct device_node *np; > + struct device_node *np, *master_np; > const struct iommu_ops *ops = NULL; > int idx = 0; > > if (dev_is_pci(dev)) > - return of_pci_iommu_configure(to_pci_dev(dev), master_np); > + return of_pci_iommu_configure(to_pci_dev(dev), parent); > > /* > * We don't currently walk up the tree looking for a parent IOMMU. > * See the `Notes:' section of > * Documentation/devicetree/bindings/iommu/iommu.txt > */ > + master_np = dev->of_node ? dev->of_node : parent; > while (!of_parse_phandle_with_args(master_np, "iommus", > "#iommu-cells", idx, > &iommu_spec)) { > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903fe9d2..833bc17f5e55 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -808,6 +808,7 @@ EXPORT_SYMBOL(of_io_request_and_map); > /** > * of_dma_get_range - Get DMA range info > * @np: device node to get DMA range info > + * @parent: node of device's parent bus, if @np is NULL > * @dma_addr: pointer to store initial DMA address of DMA range > * @paddr: pointer to store initial CPU address of DMA range > * @size: pointer to store size of DMA range > @@ -822,36 +823,48 @@ EXPORT_SYMBOL(of_io_request_and_map); > * It returns -ENODEV if "dma-ranges" property was not found > * for this device in DT. > */ > -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) > +int of_dma_get_range(struct device_node *np, struct device_node *parent, > + u64 *dma_addr, u64 *paddr, u64 *size) > { > - struct device_node *node = of_node_get(np); > + struct device_node *node; > const __be32 *ranges = NULL; > int len, naddr, nsize, pna; > int ret = 0; > u64 dmaaddr; > > - if (!node) > - return -EINVAL; > - > - while (1) { > + if (np) { > + node = of_node_get(np); > naddr = of_n_addr_cells(node); > nsize = of_n_size_cells(node); > node = of_get_next_parent(node); > - if (!node) > - break; > + } else if (parent) { > + node = of_node_get(parent); > + np = parent; > + if (of_property_read_u32(node, "#address-cells", &naddr)) > + naddr = of_n_addr_cells(node); > + if (of_property_read_u32(node, "#size-cells", &nsize)) > + nsize = of_n_size_cells(node); > + } else { > + return -EINVAL; > + } > > + while (node) { > ranges = of_get_property(node, "dma-ranges", &len); > > - /* Ignore empty ranges, they imply no translation required */ > - if (ranges && len > 0) > - break; > - > /* > * At least empty ranges has to be defined for parent node if > * DMA is supported > */ > if (!ranges) > break; > + > + /* Ignore empty ranges, they imply no translation required */ > + if (len > 0) > + break; > + > + naddr = of_n_addr_cells(node); > + nsize = of_n_size_cells(node); > + node = of_get_next_parent(node); > } > > if (!ranges) { > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 9bb8518b28f2..57ec5324ed6c 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -73,7 +73,8 @@ int of_device_add(struct platform_device *ofdev) > /** > * of_dma_configure - Setup DMA configuration > * @dev: Device to apply DMA configuration > - * @np: Pointer to OF node having DMA configuration > + * @parent: OF node of device's parent bus, if @dev is not > + * represented in DT (i.e. @dev->of_node is NULL) > * > * Try to get devices's DMA configuration from DT and update it > * accordingly. > @@ -82,13 +83,14 @@ int of_device_add(struct platform_device *ofdev) > * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events > * to fix up DMA configuration. > */ > -void of_dma_configure(struct device *dev, struct device_node *np) > +void of_dma_configure(struct device *dev, struct device_node *parent) > { > u64 dma_addr, paddr, size; > int ret; > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + struct device_node *np = dev->of_node; > > /* > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > @@ -104,7 +106,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) > if (!dev->dma_mask) > dev->dma_mask = &dev->coherent_dma_mask; > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > + ret = of_dma_get_range(np, parent, &dma_addr, &paddr, &size); > if (ret < 0) { > dma_addr = offset = 0; > size = dev->coherent_dma_mask + 1; > @@ -132,11 +134,11 @@ void of_dma_configure(struct device *dev, struct device_node *np) > dev->coherent_dma_mask = DMA_BIT_MASK(ilog2(dma_addr + size)); > *dev->dma_mask = dev->coherent_dma_mask; > > - coherent = of_dma_is_coherent(np); > + coherent = of_dma_is_coherent(np ? np : parent); > dev_dbg(dev, "device is%sdma coherent\n", > coherent ? " " : " not "); > > - iommu = of_iommu_configure(dev, np); > + iommu = of_iommu_configure(dev, parent); > dev_dbg(dev, "device is%sbehind an iommu\n", > iommu ? " " : " not "); > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h > index 37864734ca50..f1a507f3ae57 100644 > --- a/include/linux/of_address.h > +++ b/include/linux/of_address.h > @@ -52,8 +52,8 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser, > extern struct of_pci_range *of_pci_range_parser_one( > struct of_pci_range_parser *parser, > struct of_pci_range *range); > -extern int of_dma_get_range(struct device_node *np, u64 *dma_addr, > - u64 *paddr, u64 *size); > +extern int of_dma_get_range(struct device_node *np, struct device_node *parent, > + u64 *dma_addr, u64 *paddr, u64 *size); > extern bool of_dma_is_coherent(struct device_node *np); > #else /* CONFIG_OF_ADDRESS */ > static inline void __iomem *of_io_request_and_map(struct device_node *device, > @@ -95,7 +95,8 @@ static inline struct of_pci_range *of_pci_range_parser_one( > return NULL; > } > > -static inline int of_dma_get_range(struct device_node *np, u64 *dma_addr, > +static inline int of_dma_get_range(struct device_node *np, > + struct device_node *parent, u64 *dma_addr, > u64 *paddr, u64 *size) > { > return -ENODEV; > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index c12dace043f3..bcd2b6fbeef3 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > return of_node_get(cpu_dev->of_node); > } > > -void of_dma_configure(struct device *dev, struct device_node *np); > +void of_dma_configure(struct device *dev, struct device_node *parent); > #else /* CONFIG_OF */ > > static inline int of_driver_match_device(struct device *dev, > @@ -103,7 +103,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > { > return NULL; > } > -static inline void of_dma_configure(struct device *dev, struct device_node *np) > +static inline void of_dma_configure(struct device *dev, struct device_node *parent) > {} > #endif /* CONFIG_OF */ > > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index 13394ac83c66..c02b62e8e6ed 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -12,7 +12,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, > size_t *size); > > extern const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np); > + struct device_node *parent); > > #else > > @@ -24,7 +24,7 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, > } > > static inline const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np) > + struct device_node *parent) > { > return NULL; > } > -- > 2.11.0.dirty > pci and memory mapped device world is different. of course if you talk from iommu perspective, all the master are same for IOMMU but the original intention of the patch is to try to solve 2 problems. please refer to https://lkml.org/lkml/2017/3/29/10 1) expose generic API for pci world clients to configure their dma-ranges. right now there is none. 2) same API can be used by IOMMU to derive dma_mask. while the current patch posted to handle dma-ranges for both memory mapped and pci devices, which I think is overdoing. besides we have different configuration of dma-ranges based on iommu is enabled or not enabled. #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 0x80000000 0x00 0x80000000 \ 0x43000000 0x08 0x00000000 0x08 0x00000000 0x08 0x00000000 \ 0x43000000 0x80 0x00000000 0x80 0x00000000 0x40 0x00000000>; Not sure if this patch will consider above dma-ranges. my suggestion is to leave existing of_dma_get_range as is, and have new function for pci world as discussed in please refer to https://lkml.org/lkml/2017/3/29/10 Regards, Oza.
On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza <oza.oza@broadcom.com> wrote: > On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> For PCI masters not represented in DT, we pass the OF node of their >> associated host bridge to of_dma_configure(), such that they can inherit >> the appropriate DMA configuration from whatever is described there. >> Unfortunately, whilst this has worked for the "dma-coherent" property, >> it turns out to miss the case where the host bridge node has a non-empty >> "dma-ranges", since nobody is expecting the 'device' to be a bus itself. >> >> It transpires, though, that the de-facto interface since the prototype >> change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help >> re-use") is very clear-cut: either the master_np argument is redundant >> with dev->of_node, or dev->of_node is NULL and master_np is the relevant >> parent bus. Let's ratify that behaviour, then teach the whole >> of_dma_configure() pipeline to cope with both cases properly. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> >> This is what I'd consider the better fix - rather than adding yet more >> special cases - which will also make it simple to handle multiple >> "dma-ranges" entries with minimal further disruption. The callers now >> left passing dev->of_node as 'parent' are harmless, but look a bit silly >> and clearly want cleaning up - I'd be partial to renaming the existing >> function and having a single-argument wrapper for the 'normal' case, e.g.: >> >> static inline int of_dma_configure(struct device_node *dev) >> { >> return of_dma_configure_parent(dev, NULL); >> } >> >> Thoughts? >> >> Robin. >> >> drivers/iommu/of_iommu.c | 7 ++++--- >> drivers/of/address.c | 37 +++++++++++++++++++++++++------------ >> drivers/of/device.c | 12 +++++++----- >> include/linux/of_address.h | 7 ++++--- >> include/linux/of_device.h | 4 ++-- >> include/linux/of_iommu.h | 4 ++-- >> 6 files changed, 44 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >> index 2683e9fc0dcf..35aff07bb5eb 100644 >> --- a/drivers/iommu/of_iommu.c >> +++ b/drivers/iommu/of_iommu.c >> @@ -138,21 +138,22 @@ static const struct iommu_ops >> } >> >> const struct iommu_ops *of_iommu_configure(struct device *dev, >> - struct device_node *master_np) >> + struct device_node *parent) >> { >> struct of_phandle_args iommu_spec; >> - struct device_node *np; >> + struct device_node *np, *master_np; >> const struct iommu_ops *ops = NULL; >> int idx = 0; >> >> if (dev_is_pci(dev)) >> - return of_pci_iommu_configure(to_pci_dev(dev), master_np); >> + return of_pci_iommu_configure(to_pci_dev(dev), parent); >> >> /* >> * We don't currently walk up the tree looking for a parent IOMMU. >> * See the `Notes:' section of >> * Documentation/devicetree/bindings/iommu/iommu.txt >> */ >> + master_np = dev->of_node ? dev->of_node : parent; >> while (!of_parse_phandle_with_args(master_np, "iommus", >> "#iommu-cells", idx, >> &iommu_spec)) { >> diff --git a/drivers/of/address.c b/drivers/of/address.c >> index 02b2903fe9d2..833bc17f5e55 100644 >> --- a/drivers/of/address.c >> +++ b/drivers/of/address.c >> @@ -808,6 +808,7 @@ EXPORT_SYMBOL(of_io_request_and_map); >> /** >> * of_dma_get_range - Get DMA range info >> * @np: device node to get DMA range info >> + * @parent: node of device's parent bus, if @np is NULL >> * @dma_addr: pointer to store initial DMA address of DMA range >> * @paddr: pointer to store initial CPU address of DMA range >> * @size: pointer to store size of DMA range >> @@ -822,36 +823,48 @@ EXPORT_SYMBOL(of_io_request_and_map); >> * It returns -ENODEV if "dma-ranges" property was not found >> * for this device in DT. >> */ >> -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) >> +int of_dma_get_range(struct device_node *np, struct device_node *parent, >> + u64 *dma_addr, u64 *paddr, u64 *size) >> { >> - struct device_node *node = of_node_get(np); >> + struct device_node *node; >> const __be32 *ranges = NULL; >> int len, naddr, nsize, pna; >> int ret = 0; >> u64 dmaaddr; >> >> - if (!node) >> - return -EINVAL; >> - >> - while (1) { >> + if (np) { >> + node = of_node_get(np); >> naddr = of_n_addr_cells(node); >> nsize = of_n_size_cells(node); >> node = of_get_next_parent(node); >> - if (!node) >> - break; >> + } else if (parent) { >> + node = of_node_get(parent); >> + np = parent; >> + if (of_property_read_u32(node, "#address-cells", &naddr)) >> + naddr = of_n_addr_cells(node); >> + if (of_property_read_u32(node, "#size-cells", &nsize)) >> + nsize = of_n_size_cells(node); >> + } else { >> + return -EINVAL; >> + } >> >> + while (node) { >> ranges = of_get_property(node, "dma-ranges", &len); >> >> - /* Ignore empty ranges, they imply no translation required */ >> - if (ranges && len > 0) >> - break; >> - >> /* >> * At least empty ranges has to be defined for parent node if >> * DMA is supported >> */ >> if (!ranges) >> break; >> + >> + /* Ignore empty ranges, they imply no translation required */ >> + if (len > 0) >> + break; >> + >> + naddr = of_n_addr_cells(node); >> + nsize = of_n_size_cells(node); >> + node = of_get_next_parent(node); >> } >> >> if (!ranges) { >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index 9bb8518b28f2..57ec5324ed6c 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -73,7 +73,8 @@ int of_device_add(struct platform_device *ofdev) >> /** >> * of_dma_configure - Setup DMA configuration >> * @dev: Device to apply DMA configuration >> - * @np: Pointer to OF node having DMA configuration >> + * @parent: OF node of device's parent bus, if @dev is not >> + * represented in DT (i.e. @dev->of_node is NULL) >> * >> * Try to get devices's DMA configuration from DT and update it >> * accordingly. >> @@ -82,13 +83,14 @@ int of_device_add(struct platform_device *ofdev) >> * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events >> * to fix up DMA configuration. >> */ >> -void of_dma_configure(struct device *dev, struct device_node *np) >> +void of_dma_configure(struct device *dev, struct device_node *parent) >> { >> u64 dma_addr, paddr, size; >> int ret; >> bool coherent; >> unsigned long offset; >> const struct iommu_ops *iommu; >> + struct device_node *np = dev->of_node; >> >> /* >> * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> @@ -104,7 +106,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> if (!dev->dma_mask) >> dev->dma_mask = &dev->coherent_dma_mask; >> >> - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); >> + ret = of_dma_get_range(np, parent, &dma_addr, &paddr, &size); >> if (ret < 0) { >> dma_addr = offset = 0; >> size = dev->coherent_dma_mask + 1; >> @@ -132,11 +134,11 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> dev->coherent_dma_mask = DMA_BIT_MASK(ilog2(dma_addr + size)); >> *dev->dma_mask = dev->coherent_dma_mask; >> >> - coherent = of_dma_is_coherent(np); >> + coherent = of_dma_is_coherent(np ? np : parent); >> dev_dbg(dev, "device is%sdma coherent\n", >> coherent ? " " : " not "); >> >> - iommu = of_iommu_configure(dev, np); >> + iommu = of_iommu_configure(dev, parent); >> dev_dbg(dev, "device is%sbehind an iommu\n", >> iommu ? " " : " not "); >> >> diff --git a/include/linux/of_address.h b/include/linux/of_address.h >> index 37864734ca50..f1a507f3ae57 100644 >> --- a/include/linux/of_address.h >> +++ b/include/linux/of_address.h >> @@ -52,8 +52,8 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser, >> extern struct of_pci_range *of_pci_range_parser_one( >> struct of_pci_range_parser *parser, >> struct of_pci_range *range); >> -extern int of_dma_get_range(struct device_node *np, u64 *dma_addr, >> - u64 *paddr, u64 *size); >> +extern int of_dma_get_range(struct device_node *np, struct device_node *parent, >> + u64 *dma_addr, u64 *paddr, u64 *size); >> extern bool of_dma_is_coherent(struct device_node *np); >> #else /* CONFIG_OF_ADDRESS */ >> static inline void __iomem *of_io_request_and_map(struct device_node *device, >> @@ -95,7 +95,8 @@ static inline struct of_pci_range *of_pci_range_parser_one( >> return NULL; >> } >> >> -static inline int of_dma_get_range(struct device_node *np, u64 *dma_addr, >> +static inline int of_dma_get_range(struct device_node *np, >> + struct device_node *parent, u64 *dma_addr, >> u64 *paddr, u64 *size) >> { >> return -ENODEV; >> diff --git a/include/linux/of_device.h b/include/linux/of_device.h >> index c12dace043f3..bcd2b6fbeef3 100644 >> --- a/include/linux/of_device.h >> +++ b/include/linux/of_device.h >> @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) >> return of_node_get(cpu_dev->of_node); >> } >> >> -void of_dma_configure(struct device *dev, struct device_node *np); >> +void of_dma_configure(struct device *dev, struct device_node *parent); >> #else /* CONFIG_OF */ >> >> static inline int of_driver_match_device(struct device *dev, >> @@ -103,7 +103,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) >> { >> return NULL; >> } >> -static inline void of_dma_configure(struct device *dev, struct device_node *np) >> +static inline void of_dma_configure(struct device *dev, struct device_node *parent) >> {} >> #endif /* CONFIG_OF */ >> >> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h >> index 13394ac83c66..c02b62e8e6ed 100644 >> --- a/include/linux/of_iommu.h >> +++ b/include/linux/of_iommu.h >> @@ -12,7 +12,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, >> size_t *size); >> >> extern const struct iommu_ops *of_iommu_configure(struct device *dev, >> - struct device_node *master_np); >> + struct device_node *parent); >> >> #else >> >> @@ -24,7 +24,7 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, >> } >> >> static inline const struct iommu_ops *of_iommu_configure(struct device *dev, >> - struct device_node *master_np) >> + struct device_node *parent) >> { >> return NULL; >> } >> -- >> 2.11.0.dirty >> > > pci and memory mapped device world is different. of course if you talk > from iommu perspective, all the master are same for IOMMU > > but the original intention of the patch is to try to solve 2 problems. > please refer to https://lkml.org/lkml/2017/3/29/10 > > 1) expose generic API for pci world clients to configure their > dma-ranges. right now there is none. > 2) same API can be used by IOMMU to derive dma_mask. > > while the current patch posted to handle dma-ranges for both memory > mapped and pci devices, which I think is overdoing. > > besides we have different configuration of dma-ranges based on iommu > is enabled or not enabled. > #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 > 0x80000000 0x00 0x80000000 \ > 0x43000000 0x08 0x00000000 0x08 > 0x00000000 0x08 0x00000000 \ > 0x43000000 0x80 0x00000000 0x80 > 0x00000000 0x40 0x00000000>; > Not sure if this patch will consider above dma-ranges. > > my suggestion is to leave existing of_dma_get_range as is, and have > new function for pci world as discussed in > please refer to https://lkml.org/lkml/2017/3/29/10 > > Regards, > Oza. also I see that, in case of multiple ranges of_dma_get_range doesnt handle that. and also it is not meant to handle. so with this patch will return wrong size and hence wrong dma_mask. having said, I think it is better to separate pci world dma-ranges function on of_pci.c for which my discussion with Rob already, (same thread) https://lkml.org/lkml/2017/3/29/10 Waiting for Rob's viewpoint on this. Regards, Oza.
On 29/03/17 06:46, Oza Oza wrote: > On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza <oza.oza@broadcom.com> wrote: >> On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>> For PCI masters not represented in DT, we pass the OF node of their >>> associated host bridge to of_dma_configure(), such that they can inherit >>> the appropriate DMA configuration from whatever is described there. >>> Unfortunately, whilst this has worked for the "dma-coherent" property, >>> it turns out to miss the case where the host bridge node has a non-empty >>> "dma-ranges", since nobody is expecting the 'device' to be a bus itself. >>> >>> It transpires, though, that the de-facto interface since the prototype >>> change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help >>> re-use") is very clear-cut: either the master_np argument is redundant >>> with dev->of_node, or dev->of_node is NULL and master_np is the relevant >>> parent bus. Let's ratify that behaviour, then teach the whole >>> of_dma_configure() pipeline to cope with both cases properly. >>> >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> [...] >> >> pci and memory mapped device world is different. ??? No they aren't. There is nothing magic about PCI. PCI *is* a memory-mapped bus. The only PCI-specific aspect here is the Linux code path which passes a host controller node into of_dma_configure() where the latter expects a node for the actual endpoint device. It managed to work for "dma-coherent", because that may appear either directly on a device node or on any of its parent buses, but "dma-ranges" is *only* valid for DT nodes representing buses, thus reveals that using a parent bus to stand in for a device isn't actually correct. I only posted that horrible hack patch to prove the point that having a child node to represent the actual device is indeed sufficient to make of_dma_configure() work correctly for PCI masters as-is (modulo the other issues). > of course if you talk >> from iommu perspective, all the master are same for IOMMU I don't understand what you mean by that. There's nothing about IOMMUs here, it's purely about parsing DT properties correctly. >> but the original intention of the patch is to try to solve 2 problems. >> please refer to https://lkml.org/lkml/2017/3/29/10 One patch should not solve two separate problems anyway. Taking a step back, there are at least 3 things that this discussion has brought up: 1) The way in which we call of_dma_configure() for PCI devices causes the "dma-ranges" property on PCI host controllers to be ignored. 2) of_dma_get_range() only considers the first entry in any "dma-ranges" property. 3) When of_dma_configure() does set a DMA mask, there is nothing on arm64 and other architectures to prevent drivers overriding that with a wider mask later. Those are 3 separate problems, to solve with 3 or more separate patches, and I have deliberately put the second and third to one side for the moment. This patch fixes problem 1. >> 1) expose generic API for pci world clients to configure their >> dma-ranges. right now there is none. >> 2) same API can be used by IOMMU to derive dma_mask. >> >> while the current patch posted to handle dma-ranges for both memory >> mapped and pci devices, which I think is overdoing. No, of_dma_configure() handles the "dma-ranges" property as it is defined in the DT spec to describe the mapping between a child bus address space and a parent bus address space. Whether those memory-mapped parent and child buses are PCI, ISA, AMBA, HyperTransport, or anything else is irrelevant other than for the encoding of the address specifiers. All this patch does is sort out the cases where we have a real device node to start at, from the cases where we don't and so start directly at the device's parent bus node instead. >> besides we have different configuration of dma-ranges based on iommu >> is enabled or not enabled. That doesn't sound right, unless you mean the firmware actually programs the host controller's AXI bridge differently for system configurations where the IOMMU is expected to be used or not? (and even then, I don't really see why it would be necessary to do that...) >> #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 >> 0x80000000 0x00 0x80000000 \ >> 0x43000000 0x08 0x00000000 0x08 >> 0x00000000 0x08 0x00000000 \ >> 0x43000000 0x80 0x00000000 0x80 >> 0x00000000 0x40 0x00000000>; >> Not sure if this patch will consider above dma-ranges. >> >> my suggestion is to leave existing of_dma_get_range as is, and have >> new function for pci world as discussed in >> please refer to https://lkml.org/lkml/2017/3/29/10 And then we keep adding new functions to do the exact same thing for every other discoverable bus type whose bridge is be described in DT? I fail to see how that is in any way better than simply fixing the existing code to work as it was intended to. of_dma_get_ranges() uses of_translate_dma_address() just the same way as existing PowerPC PCI code does, which further disproves your assertion that parsing PCI addresses is somehow special - it's really only a matter of getting the right number of address cells in order to to read a child address to give to of_translate_dma_address() in the first place. Incidentally, I now notice that the proposed of_pci_get_dma_ranges() is incomplete as it doesn't use of_translate_dma_address() or otherwise traverse upwards through the dma-ranges of any further parent buses. >> >> Regards, >> Oza. > > also I see that, in case of multiple ranges of_dma_get_range doesnt handle that. > and also it is not meant to handle. Yes, the existing code doesn't handle multiple dma-ranges entries, because nobody's had the need to implement that so far, and this patch does not change that because it's fixing a separate problem. Now, of course of_dma_get_range() *should* be capable of handling multiple entries regardless of this patch, and I'm going to write *that* patch right now (it's going to be a case of adding a handful of lines which probably won't even conflict with this one at all). If we had a bunch of different range parsing functions, we'd then have to duplicate the equivalent logic across all of them, which is clearly undesirable when it can easily be avoided altogether. Robin. > so with this patch will return wrong size and hence wrong dma_mask. > having said, I think it is better to separate pci world dma-ranges > function on of_pci.c > > for which my discussion with Rob already, (same thread) > https://lkml.org/lkml/2017/3/29/10 > Waiting for Rob's viewpoint on this. > > > Regards, > Oza. >
On Wed, Mar 29, 2017 at 11:12 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 29/03/17 06:46, Oza Oza wrote: >> On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza <oza.oza@broadcom.com> wrote: >>> On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>>> For PCI masters not represented in DT, we pass the OF node of their >>>> associated host bridge to of_dma_configure(), such that they can inherit >>>> the appropriate DMA configuration from whatever is described there. >>>> Unfortunately, whilst this has worked for the "dma-coherent" property, >>>> it turns out to miss the case where the host bridge node has a non-empty >>>> "dma-ranges", since nobody is expecting the 'device' to be a bus itself. >>>> >>>> It transpires, though, that the de-facto interface since the prototype >>>> change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help >>>> re-use") is very clear-cut: either the master_np argument is redundant >>>> with dev->of_node, or dev->of_node is NULL and master_np is the relevant >>>> parent bus. Let's ratify that behaviour, then teach the whole >>>> of_dma_configure() pipeline to cope with both cases properly. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > [...] > >>> >>> pci and memory mapped device world is different. > > ??? > > No they aren't. There is nothing magic about PCI. PCI *is* a > memory-mapped bus. > > The only PCI-specific aspect here is the Linux code path which passes a > host controller node into of_dma_configure() where the latter expects a > node for the actual endpoint device. It managed to work for > "dma-coherent", because that may appear either directly on a device node > or on any of its parent buses, but "dma-ranges" is *only* valid for DT > nodes representing buses, thus reveals that using a parent bus to stand > in for a device isn't actually correct. I only posted that horrible hack > patch to prove the point that having a child node to represent the > actual device is indeed sufficient to make of_dma_configure() work > correctly for PCI masters as-is (modulo the other issues). > >> of course if you talk >>> from iommu perspective, all the master are same for IOMMU > > I don't understand what you mean by that. There's nothing about IOMMUs > here, it's purely about parsing DT properties correctly. > >>> but the original intention of the patch is to try to solve 2 problems. >>> please refer to https://lkml.org/lkml/2017/3/29/10 > > One patch should not solve two separate problems anyway. Taking a step > back, there are at least 3 things that this discussion has brought up: > > 1) The way in which we call of_dma_configure() for PCI devices causes > the "dma-ranges" property on PCI host controllers to be ignored. > > 2) of_dma_get_range() only considers the first entry in any "dma-ranges" > property. > > 3) When of_dma_configure() does set a DMA mask, there is nothing on > arm64 and other architectures to prevent drivers overriding that with a > wider mask later. > > Those are 3 separate problems, to solve with 3 or more separate patches, > and I have deliberately put the second and third to one side for the > moment. This patch fixes problem 1. > >>> 1) expose generic API for pci world clients to configure their >>> dma-ranges. right now there is none. >>> 2) same API can be used by IOMMU to derive dma_mask. >>> >>> while the current patch posted to handle dma-ranges for both memory >>> mapped and pci devices, which I think is overdoing. > > No, of_dma_configure() handles the "dma-ranges" property as it is > defined in the DT spec to describe the mapping between a child bus > address space and a parent bus address space. Whether those > memory-mapped parent and child buses are PCI, ISA, AMBA, HyperTransport, > or anything else is irrelevant other than for the encoding of the > address specifiers. All this patch does is sort out the cases where we > have a real device node to start at, from the cases where we don't and > so start directly at the device's parent bus node instead. > >>> besides we have different configuration of dma-ranges based on iommu >>> is enabled or not enabled. > > That doesn't sound right, unless you mean the firmware actually programs > the host controller's AXI bridge differently for system configurations > where the IOMMU is expected to be used or not? (and even then, I don't > really see why it would be necessary to do that...) > >>> #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 >>> 0x80000000 0x00 0x80000000 \ >>> 0x43000000 0x08 0x00000000 0x08 >>> 0x00000000 0x08 0x00000000 \ >>> 0x43000000 0x80 0x00000000 0x80 >>> 0x00000000 0x40 0x00000000>; >>> Not sure if this patch will consider above dma-ranges. >>> >>> my suggestion is to leave existing of_dma_get_range as is, and have >>> new function for pci world as discussed in >>> please refer to https://lkml.org/lkml/2017/3/29/10 > > And then we keep adding new functions to do the exact same thing for > every other discoverable bus type whose bridge is be described in DT? I > fail to see how that is in any way better than simply fixing the > existing code to work as it was intended to. > > of_dma_get_ranges() uses of_translate_dma_address() just the same way as > existing PowerPC PCI code does, which further disproves your assertion > that parsing PCI addresses is somehow special - it's really only a > matter of getting the right number of address cells in order to to read > a child address to give to of_translate_dma_address() in the first > place. Incidentally, I now notice that the proposed > of_pci_get_dma_ranges() is incomplete as it doesn't use > of_translate_dma_address() or otherwise traverse upwards through the > dma-ranges of any further parent buses. > >>> >>> Regards, >>> Oza. >> >> also I see that, in case of multiple ranges of_dma_get_range doesnt handle that. >> and also it is not meant to handle. > > Yes, the existing code doesn't handle multiple dma-ranges entries, > because nobody's had the need to implement that so far, and this patch > does not change that because it's fixing a separate problem. > > Now, of course of_dma_get_range() *should* be capable of handling > multiple entries regardless of this patch, and I'm going to write *that* > patch right now (it's going to be a case of adding a handful of lines > which probably won't even conflict with this one at all). If we had a > bunch of different range parsing functions, we'd then have to duplicate > the equivalent logic across all of them, which is clearly undesirable > when it can easily be avoided altogether. > > Robin. > >> so with this patch will return wrong size and hence wrong dma_mask. >> having said, I think it is better to separate pci world dma-ranges >> function on of_pci.c >> >> for which my discussion with Rob already, (same thread) >> https://lkml.org/lkml/2017/3/29/10 >> Waiting for Rob's viewpoint on this. >> >> >> Regards, >> Oza. >> > In my opinion NAKing to this, because 1) some reasons already mentioned in https://lkml.org/lkml/2017/3/29/10 2) also of_dma_get_range is supposed to handle traditional dma-ranges (not pci one) 3) we need separate function for pci users such as iproc or rcar SOCs to have their dma-ranges parsed, which can be extended later for e.g. pci flags have some meanings. besides of_dma_configure is written to pass only single out parameter (..., *dma_addr, *size) handling multiple ranges here, and passing only one range out is not desirable. also you would not like to handle pci flags (the first portion of DT entry) in this function if required in future. this new function will be similar to of_pci_get_host_bridge_resources https://lkml.org/lkml/2017/3/29/10 which I am writing/improving right now.. Regards, Oza.
On Thu, Mar 30, 2017 at 8:51 AM, Oza Oza <oza.oza@broadcom.com> wrote: > On Wed, Mar 29, 2017 at 11:12 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> On 29/03/17 06:46, Oza Oza wrote: >>> On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza <oza.oza@broadcom.com> wrote: >>>> On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>>>> For PCI masters not represented in DT, we pass the OF node of their >>>>> associated host bridge to of_dma_configure(), such that they can inherit >>>>> the appropriate DMA configuration from whatever is described there. >>>>> Unfortunately, whilst this has worked for the "dma-coherent" property, >>>>> it turns out to miss the case where the host bridge node has a non-empty >>>>> "dma-ranges", since nobody is expecting the 'device' to be a bus itself. >>>>> >>>>> It transpires, though, that the de-facto interface since the prototype >>>>> change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help >>>>> re-use") is very clear-cut: either the master_np argument is redundant >>>>> with dev->of_node, or dev->of_node is NULL and master_np is the relevant >>>>> parent bus. Let's ratify that behaviour, then teach the whole >>>>> of_dma_configure() pipeline to cope with both cases properly. >>>>> >>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> >> [...] >> >>>> >>>> pci and memory mapped device world is different. >> >> ??? >> >> No they aren't. There is nothing magic about PCI. PCI *is* a >> memory-mapped bus. >> >> The only PCI-specific aspect here is the Linux code path which passes a >> host controller node into of_dma_configure() where the latter expects a >> node for the actual endpoint device. It managed to work for >> "dma-coherent", because that may appear either directly on a device node >> or on any of its parent buses, but "dma-ranges" is *only* valid for DT >> nodes representing buses, thus reveals that using a parent bus to stand >> in for a device isn't actually correct. I only posted that horrible hack >> patch to prove the point that having a child node to represent the >> actual device is indeed sufficient to make of_dma_configure() work >> correctly for PCI masters as-is (modulo the other issues). >> >>> of course if you talk >>>> from iommu perspective, all the master are same for IOMMU >> >> I don't understand what you mean by that. There's nothing about IOMMUs >> here, it's purely about parsing DT properties correctly. >> >>>> but the original intention of the patch is to try to solve 2 problems. >>>> please refer to https://lkml.org/lkml/2017/3/29/10 >> >> One patch should not solve two separate problems anyway. Taking a step >> back, there are at least 3 things that this discussion has brought up: >> >> 1) The way in which we call of_dma_configure() for PCI devices causes >> the "dma-ranges" property on PCI host controllers to be ignored. >> >> 2) of_dma_get_range() only considers the first entry in any "dma-ranges" >> property. >> >> 3) When of_dma_configure() does set a DMA mask, there is nothing on >> arm64 and other architectures to prevent drivers overriding that with a >> wider mask later. >> >> Those are 3 separate problems, to solve with 3 or more separate patches, >> and I have deliberately put the second and third to one side for the >> moment. This patch fixes problem 1. >> >>>> 1) expose generic API for pci world clients to configure their >>>> dma-ranges. right now there is none. >>>> 2) same API can be used by IOMMU to derive dma_mask. >>>> >>>> while the current patch posted to handle dma-ranges for both memory >>>> mapped and pci devices, which I think is overdoing. >> >> No, of_dma_configure() handles the "dma-ranges" property as it is >> defined in the DT spec to describe the mapping between a child bus >> address space and a parent bus address space. Whether those >> memory-mapped parent and child buses are PCI, ISA, AMBA, HyperTransport, >> or anything else is irrelevant other than for the encoding of the >> address specifiers. All this patch does is sort out the cases where we >> have a real device node to start at, from the cases where we don't and >> so start directly at the device's parent bus node instead. >> >>>> besides we have different configuration of dma-ranges based on iommu >>>> is enabled or not enabled. >> >> That doesn't sound right, unless you mean the firmware actually programs >> the host controller's AXI bridge differently for system configurations >> where the IOMMU is expected to be used or not? (and even then, I don't >> really see why it would be necessary to do that...) >> >>>> #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 >>>> 0x80000000 0x00 0x80000000 \ >>>> 0x43000000 0x08 0x00000000 0x08 >>>> 0x00000000 0x08 0x00000000 \ >>>> 0x43000000 0x80 0x00000000 0x80 >>>> 0x00000000 0x40 0x00000000>; >>>> Not sure if this patch will consider above dma-ranges. >>>> >>>> my suggestion is to leave existing of_dma_get_range as is, and have >>>> new function for pci world as discussed in >>>> please refer to https://lkml.org/lkml/2017/3/29/10 >> >> And then we keep adding new functions to do the exact same thing for >> every other discoverable bus type whose bridge is be described in DT? I >> fail to see how that is in any way better than simply fixing the >> existing code to work as it was intended to. >> >> of_dma_get_ranges() uses of_translate_dma_address() just the same way as >> existing PowerPC PCI code does, which further disproves your assertion >> that parsing PCI addresses is somehow special - it's really only a >> matter of getting the right number of address cells in order to to read >> a child address to give to of_translate_dma_address() in the first >> place. Incidentally, I now notice that the proposed >> of_pci_get_dma_ranges() is incomplete as it doesn't use >> of_translate_dma_address() or otherwise traverse upwards through the >> dma-ranges of any further parent buses. >> >>>> >>>> Regards, >>>> Oza. >>> >>> also I see that, in case of multiple ranges of_dma_get_range doesnt handle that. >>> and also it is not meant to handle. >> >> Yes, the existing code doesn't handle multiple dma-ranges entries, >> because nobody's had the need to implement that so far, and this patch >> does not change that because it's fixing a separate problem. >> >> Now, of course of_dma_get_range() *should* be capable of handling >> multiple entries regardless of this patch, and I'm going to write *that* >> patch right now (it's going to be a case of adding a handful of lines >> which probably won't even conflict with this one at all). If we had a >> bunch of different range parsing functions, we'd then have to duplicate >> the equivalent logic across all of them, which is clearly undesirable >> when it can easily be avoided altogether. >> >> Robin. >> >>> so with this patch will return wrong size and hence wrong dma_mask. >>> having said, I think it is better to separate pci world dma-ranges >>> function on of_pci.c >>> >>> for which my discussion with Rob already, (same thread) >>> https://lkml.org/lkml/2017/3/29/10 >>> Waiting for Rob's viewpoint on this. >>> >>> >>> Regards, >>> Oza. >>> >> > > In my opinion NAKing to this, because > > 1) some reasons already mentioned in https://lkml.org/lkml/2017/3/29/10 > > 2) also of_dma_get_range is supposed to handle traditional dma-ranges > (not pci one) > > 3) we need separate function for pci users such as iproc or rcar SOCs > to have their dma-ranges parsed, > which can be extended later for e.g. pci flags have some meanings. > > besides of_dma_configure is written to pass only single out parameter > (..., *dma_addr, *size) > handling multiple ranges here, and passing only one range out is not desirable. > > also you would not like to handle pci flags (the first portion of DT > entry) in this function if required in future. > > this new function will be similar to of_pci_get_host_bridge_resources > https://lkml.org/lkml/2017/3/29/10 > which I am writing/improving right now.. > > > Regards, > Oza. Hi Robin, Gentle request to have a look at following approach and patch. [RFC PATCH 2/2] of/pci: call pci specific dma-ranges instead of memory-mapped. [RFC PATCH 1/2] of/pci: implement inbound dma-ranges for PCI I have tested this on our platform, with and without iommu, and seems to work. of course it requires your "in discussion" iommu dma_mask" changes to work. which I believe you will take it ahead. so I have removed all the iommu related changes now. and this addresses purely pci world now. let me know your view on this. Regards, Oza.
On Wed, Mar 29, 2017 at 11:12 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 29/03/17 06:46, Oza Oza wrote: >> On Wed, Mar 29, 2017 at 10:23 AM, Oza Oza <oza.oza@broadcom.com> wrote: >>> On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>>> For PCI masters not represented in DT, we pass the OF node of their >>>> associated host bridge to of_dma_configure(), such that they can inherit >>>> the appropriate DMA configuration from whatever is described there. >>>> Unfortunately, whilst this has worked for the "dma-coherent" property, >>>> it turns out to miss the case where the host bridge node has a non-empty >>>> "dma-ranges", since nobody is expecting the 'device' to be a bus itself. >>>> >>>> It transpires, though, that the de-facto interface since the prototype >>>> change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help >>>> re-use") is very clear-cut: either the master_np argument is redundant >>>> with dev->of_node, or dev->of_node is NULL and master_np is the relevant >>>> parent bus. Let's ratify that behaviour, then teach the whole >>>> of_dma_configure() pipeline to cope with both cases properly. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > [...] > >>> >>> pci and memory mapped device world is different. > > ??? > > No they aren't. There is nothing magic about PCI. PCI *is* a > memory-mapped bus. > > The only PCI-specific aspect here is the Linux code path which passes a > host controller node into of_dma_configure() where the latter expects a > node for the actual endpoint device. It managed to work for > "dma-coherent", because that may appear either directly on a device node > or on any of its parent buses, but "dma-ranges" is *only* valid for DT > nodes representing buses, thus reveals that using a parent bus to stand > in for a device isn't actually correct. I only posted that horrible hack > patch to prove the point that having a child node to represent the > actual device is indeed sufficient to make of_dma_configure() work > correctly for PCI masters as-is (modulo the other issues). > >> of course if you talk >>> from iommu perspective, all the master are same for IOMMU > > I don't understand what you mean by that. There's nothing about IOMMUs > here, it's purely about parsing DT properties correctly. > >>> but the original intention of the patch is to try to solve 2 problems. >>> please refer to https://lkml.org/lkml/2017/3/29/10 > > One patch should not solve two separate problems anyway. Taking a step > back, there are at least 3 things that this discussion has brought up: > > 1) The way in which we call of_dma_configure() for PCI devices causes > the "dma-ranges" property on PCI host controllers to be ignored. > > 2) of_dma_get_range() only considers the first entry in any "dma-ranges" > property. > > 3) When of_dma_configure() does set a DMA mask, there is nothing on > arm64 and other architectures to prevent drivers overriding that with a > wider mask later. > > Those are 3 separate problems, to solve with 3 or more separate patches, > and I have deliberately put the second and third to one side for the > moment. This patch fixes problem 1. > >>> 1) expose generic API for pci world clients to configure their >>> dma-ranges. right now there is none. >>> 2) same API can be used by IOMMU to derive dma_mask. >>> >>> while the current patch posted to handle dma-ranges for both memory >>> mapped and pci devices, which I think is overdoing. > > No, of_dma_configure() handles the "dma-ranges" property as it is > defined in the DT spec to describe the mapping between a child bus > address space and a parent bus address space. Whether those > memory-mapped parent and child buses are PCI, ISA, AMBA, HyperTransport, > or anything else is irrelevant other than for the encoding of the > address specifiers. All this patch does is sort out the cases where we > have a real device node to start at, from the cases where we don't and > so start directly at the device's parent bus node instead. > >>> besides we have different configuration of dma-ranges based on iommu >>> is enabled or not enabled. > > That doesn't sound right, unless you mean the firmware actually programs > the host controller's AXI bridge differently for system configurations > where the IOMMU is expected to be used or not? (and even then, I don't > really see why it would be necessary to do that...) > >>> #define PCIE_DMA_RANGES dma-ranges = <0x43000000 0x00 0x80000000 0x00 >>> 0x80000000 0x00 0x80000000 \ >>> 0x43000000 0x08 0x00000000 0x08 >>> 0x00000000 0x08 0x00000000 \ >>> 0x43000000 0x80 0x00000000 0x80 >>> 0x00000000 0x40 0x00000000>; >>> Not sure if this patch will consider above dma-ranges. >>> >>> my suggestion is to leave existing of_dma_get_range as is, and have >>> new function for pci world as discussed in >>> please refer to https://lkml.org/lkml/2017/3/29/10 > > And then we keep adding new functions to do the exact same thing for > every other discoverable bus type whose bridge is be described in DT? I > fail to see how that is in any way better than simply fixing the > existing code to work as it was intended to. > > of_dma_get_ranges() uses of_translate_dma_address() just the same way as > existing PowerPC PCI code does, which further disproves your assertion > that parsing PCI addresses is somehow special - it's really only a > matter of getting the right number of address cells in order to to read > a child address to give to of_translate_dma_address() in the first > place. Incidentally, I now notice that the proposed > of_pci_get_dma_ranges() is incomplete as it doesn't use > of_translate_dma_address() or otherwise traverse upwards through the > dma-ranges of any further parent buses. > >>> >>> Regards, >>> Oza. >> >> also I see that, in case of multiple ranges of_dma_get_range doesnt handle that. >> and also it is not meant to handle. > > Yes, the existing code doesn't handle multiple dma-ranges entries, > because nobody's had the need to implement that so far, and this patch > does not change that because it's fixing a separate problem. > > Now, of course of_dma_get_range() *should* be capable of handling > multiple entries regardless of this patch, and I'm going to write *that* > patch right now (it's going to be a case of adding a handful of lines > which probably won't even conflict with this one at all). If we had a > bunch of different range parsing functions, we'd then have to duplicate > the equivalent logic across all of them, which is clearly undesirable > when it can easily be avoided altogether. > > Robin. > >> so with this patch will return wrong size and hence wrong dma_mask. >> having said, I think it is better to separate pci world dma-ranges >> function on of_pci.c >> >> for which my discussion with Rob already, (same thread) >> https://lkml.org/lkml/2017/3/29/10 >> Waiting for Rob's viewpoint on this. >> >> >> Regards, >> Oza. >> > first of all the patch https://lkml.org/lkml/2017/3/30/304 is not only to address the original problem of dma_mask but also to provide generic APIs to PCI RC drivers such as pcie-iproc.c or even rcar to get their dma-ranges. this API not only provides basis to retrieve the ranges but also it should have capability to handle inbound memory flags. having said that the PCI dma-ranges do differ slightly from traditional memory ranges. because it has got flags. now those flags could be IORESOURCE_CACHEABLE or anything else. our SOC will be having use of these flags in near future (although can not discuss details as of now). but in short we will need these flags to be taken care which were passed form DT. so PCIe iproc driver calls this function to get all teh information about dma-ranges. now if such function exists, other framework could also make use of it to get resources and figure out dma_mask. and in that case you really need not change of_dma_get_range. not only of_dma_get_range can hdnle PCI flags property but also it can not handle multiple ranges because finally it has only single *dma_addr, *paddr, *size out parameters. so the burdon will be on API to fix the behavior as what to return rather than having Caller to choose what to do with the ranges. coming back to my current patch currently it does not do flag handling, but in near future I will have to add, but for that, this patch https://lkml.org/lkml/2017/3/30/304 needs to bring under consideration to be ACKed. hope this explains the need of API. Regards, Oza.
On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@arm.com> wrote: > For PCI masters not represented in DT, we pass the OF node of their > associated host bridge to of_dma_configure(), such that they can inherit > the appropriate DMA configuration from whatever is described there. > Unfortunately, whilst this has worked for the "dma-coherent" property, > it turns out to miss the case where the host bridge node has a non-empty > "dma-ranges", since nobody is expecting the 'device' to be a bus itself. > > It transpires, though, that the de-facto interface since the prototype > change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help > re-use") is very clear-cut: either the master_np argument is redundant > with dev->of_node, or dev->of_node is NULL and master_np is the relevant > parent bus. Let's ratify that behaviour, then teach the whole > of_dma_configure() pipeline to cope with both cases properly. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > This is what I'd consider the better fix - rather than adding yet more > special cases - which will also make it simple to handle multiple > "dma-ranges" entries with minimal further disruption. The callers now > left passing dev->of_node as 'parent' are harmless, but look a bit silly > and clearly want cleaning up - I'd be partial to renaming the existing > function and having a single-argument wrapper for the 'normal' case, e.g.: > > static inline int of_dma_configure(struct device_node *dev) > { > return of_dma_configure_parent(dev, NULL); > } > > Thoughts? > > Robin. > > drivers/iommu/of_iommu.c | 7 ++++--- > drivers/of/address.c | 37 +++++++++++++++++++++++++------------ > drivers/of/device.c | 12 +++++++----- > include/linux/of_address.h | 7 ++++--- > include/linux/of_device.h | 4 ++-- > include/linux/of_iommu.h | 4 ++-- > 6 files changed, 44 insertions(+), 27 deletions(-) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 2683e9fc0dcf..35aff07bb5eb 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -138,21 +138,22 @@ static const struct iommu_ops > } > > const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np) > + struct device_node *parent) > { > struct of_phandle_args iommu_spec; > - struct device_node *np; > + struct device_node *np, *master_np; > const struct iommu_ops *ops = NULL; > int idx = 0; > > if (dev_is_pci(dev)) > - return of_pci_iommu_configure(to_pci_dev(dev), master_np); > + return of_pci_iommu_configure(to_pci_dev(dev), parent); > > /* > * We don't currently walk up the tree looking for a parent IOMMU. > * See the `Notes:' section of > * Documentation/devicetree/bindings/iommu/iommu.txt > */ > + master_np = dev->of_node ? dev->of_node : parent; > while (!of_parse_phandle_with_args(master_np, "iommus", > "#iommu-cells", idx, > &iommu_spec)) { > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903fe9d2..833bc17f5e55 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -808,6 +808,7 @@ EXPORT_SYMBOL(of_io_request_and_map); > /** > * of_dma_get_range - Get DMA range info > * @np: device node to get DMA range info > + * @parent: node of device's parent bus, if @np is NULL > * @dma_addr: pointer to store initial DMA address of DMA range > * @paddr: pointer to store initial CPU address of DMA range > * @size: pointer to store size of DMA range > @@ -822,36 +823,48 @@ EXPORT_SYMBOL(of_io_request_and_map); > * It returns -ENODEV if "dma-ranges" property was not found > * for this device in DT. > */ > -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) > +int of_dma_get_range(struct device_node *np, struct device_node *parent, > + u64 *dma_addr, u64 *paddr, u64 *size) > { > - struct device_node *node = of_node_get(np); > + struct device_node *node; > const __be32 *ranges = NULL; > int len, naddr, nsize, pna; > int ret = 0; > u64 dmaaddr; > > - if (!node) > - return -EINVAL; > - > - while (1) { > + if (np) { > + node = of_node_get(np); > naddr = of_n_addr_cells(node); > nsize = of_n_size_cells(node); > node = of_get_next_parent(node); > - if (!node) > - break; > + } else if (parent) { > + node = of_node_get(parent); > + np = parent; > + if (of_property_read_u32(node, "#address-cells", &naddr)) > + naddr = of_n_addr_cells(node); > + if (of_property_read_u32(node, "#size-cells", &nsize)) > + nsize = of_n_size_cells(node); > + } else { > + return -EINVAL; > + } > > + while (node) { > ranges = of_get_property(node, "dma-ranges", &len); > > - /* Ignore empty ranges, they imply no translation required */ > - if (ranges && len > 0) > - break; > - > /* > * At least empty ranges has to be defined for parent node if > * DMA is supported > */ > if (!ranges) > break; > + > + /* Ignore empty ranges, they imply no translation required */ > + if (len > 0) > + break; > + > + naddr = of_n_addr_cells(node); > + nsize = of_n_size_cells(node); > + node = of_get_next_parent(node); > } > > if (!ranges) { > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 9bb8518b28f2..57ec5324ed6c 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -73,7 +73,8 @@ int of_device_add(struct platform_device *ofdev) > /** > * of_dma_configure - Setup DMA configuration > * @dev: Device to apply DMA configuration > - * @np: Pointer to OF node having DMA configuration > + * @parent: OF node of device's parent bus, if @dev is not > + * represented in DT (i.e. @dev->of_node is NULL) > * > * Try to get devices's DMA configuration from DT and update it > * accordingly. > @@ -82,13 +83,14 @@ int of_device_add(struct platform_device *ofdev) > * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events > * to fix up DMA configuration. > */ > -void of_dma_configure(struct device *dev, struct device_node *np) > +void of_dma_configure(struct device *dev, struct device_node *parent) > { > u64 dma_addr, paddr, size; > int ret; > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + struct device_node *np = dev->of_node; > > /* > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > @@ -104,7 +106,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) > if (!dev->dma_mask) > dev->dma_mask = &dev->coherent_dma_mask; > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > + ret = of_dma_get_range(np, parent, &dma_addr, &paddr, &size); > if (ret < 0) { > dma_addr = offset = 0; > size = dev->coherent_dma_mask + 1; > @@ -132,11 +134,11 @@ void of_dma_configure(struct device *dev, struct device_node *np) > dev->coherent_dma_mask = DMA_BIT_MASK(ilog2(dma_addr + size)); > *dev->dma_mask = dev->coherent_dma_mask; > > - coherent = of_dma_is_coherent(np); > + coherent = of_dma_is_coherent(np ? np : parent); > dev_dbg(dev, "device is%sdma coherent\n", > coherent ? " " : " not "); > > - iommu = of_iommu_configure(dev, np); > + iommu = of_iommu_configure(dev, parent); > dev_dbg(dev, "device is%sbehind an iommu\n", > iommu ? " " : " not "); > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h > index 37864734ca50..f1a507f3ae57 100644 > --- a/include/linux/of_address.h > +++ b/include/linux/of_address.h > @@ -52,8 +52,8 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser, > extern struct of_pci_range *of_pci_range_parser_one( > struct of_pci_range_parser *parser, > struct of_pci_range *range); > -extern int of_dma_get_range(struct device_node *np, u64 *dma_addr, > - u64 *paddr, u64 *size); > +extern int of_dma_get_range(struct device_node *np, struct device_node *parent, > + u64 *dma_addr, u64 *paddr, u64 *size); > extern bool of_dma_is_coherent(struct device_node *np); > #else /* CONFIG_OF_ADDRESS */ > static inline void __iomem *of_io_request_and_map(struct device_node *device, > @@ -95,7 +95,8 @@ static inline struct of_pci_range *of_pci_range_parser_one( > return NULL; > } > > -static inline int of_dma_get_range(struct device_node *np, u64 *dma_addr, > +static inline int of_dma_get_range(struct device_node *np, > + struct device_node *parent, u64 *dma_addr, > u64 *paddr, u64 *size) > { > return -ENODEV; > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index c12dace043f3..bcd2b6fbeef3 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > return of_node_get(cpu_dev->of_node); > } > > -void of_dma_configure(struct device *dev, struct device_node *np); > +void of_dma_configure(struct device *dev, struct device_node *parent); > #else /* CONFIG_OF */ > > static inline int of_driver_match_device(struct device *dev, > @@ -103,7 +103,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) > { > return NULL; > } > -static inline void of_dma_configure(struct device *dev, struct device_node *np) > +static inline void of_dma_configure(struct device *dev, struct device_node *parent) > {} > #endif /* CONFIG_OF */ > > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index 13394ac83c66..c02b62e8e6ed 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -12,7 +12,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, > size_t *size); > > extern const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np); > + struct device_node *parent); > > #else > > @@ -24,7 +24,7 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, > } > > static inline const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np) > + struct device_node *parent) > { > return NULL; > } > -- > 2.11.0.dirty > Hi Robin, After some more thoughts to this, I have improved upon old patch design. please check https://lkml.org/lkml/2017/4/22/34 this patch served following purposes 1) exposes intrface to the pci host driver for thir inbound memory ranges 2) provide an interface to callers such as of_dma_get_ranges. so then the returned size get best possible (largest) dma_mask. for e.g. dma-ranges = <0x43000000 0x00 0x00 0x00 0x00 0x80 0x00>; we should get dev->coherent_dma_mask=0x7fffffffff. 3) this patch handles multiple inbound windows and dma-ranges. it is left to the caller, how it wants to use them. the new function returns the resources in a standard and unform way 4) this way the callers of of_dma_get_ranges does not need to change. and 5) leaves scope of adding PCI flag handling for inbound memory by the new function. which I feel much better approach/way than accommodating of_dma_get_ranges to handle multiple dma-ranges, addressing PCI masters. also in your patch I think, you have to make emulated parent node which is more work I suppose. again I have tested this on our SOC and it works well. Regards, Oza.
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 2683e9fc0dcf..35aff07bb5eb 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -138,21 +138,22 @@ static const struct iommu_ops } const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np) + struct device_node *parent) { struct of_phandle_args iommu_spec; - struct device_node *np; + struct device_node *np, *master_np; const struct iommu_ops *ops = NULL; int idx = 0; if (dev_is_pci(dev)) - return of_pci_iommu_configure(to_pci_dev(dev), master_np); + return of_pci_iommu_configure(to_pci_dev(dev), parent); /* * We don't currently walk up the tree looking for a parent IOMMU. * See the `Notes:' section of * Documentation/devicetree/bindings/iommu/iommu.txt */ + master_np = dev->of_node ? dev->of_node : parent; while (!of_parse_phandle_with_args(master_np, "iommus", "#iommu-cells", idx, &iommu_spec)) { diff --git a/drivers/of/address.c b/drivers/of/address.c index 02b2903fe9d2..833bc17f5e55 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -808,6 +808,7 @@ EXPORT_SYMBOL(of_io_request_and_map); /** * of_dma_get_range - Get DMA range info * @np: device node to get DMA range info + * @parent: node of device's parent bus, if @np is NULL * @dma_addr: pointer to store initial DMA address of DMA range * @paddr: pointer to store initial CPU address of DMA range * @size: pointer to store size of DMA range @@ -822,36 +823,48 @@ EXPORT_SYMBOL(of_io_request_and_map); * It returns -ENODEV if "dma-ranges" property was not found * for this device in DT. */ -int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size) +int of_dma_get_range(struct device_node *np, struct device_node *parent, + u64 *dma_addr, u64 *paddr, u64 *size) { - struct device_node *node = of_node_get(np); + struct device_node *node; const __be32 *ranges = NULL; int len, naddr, nsize, pna; int ret = 0; u64 dmaaddr; - if (!node) - return -EINVAL; - - while (1) { + if (np) { + node = of_node_get(np); naddr = of_n_addr_cells(node); nsize = of_n_size_cells(node); node = of_get_next_parent(node); - if (!node) - break; + } else if (parent) { + node = of_node_get(parent); + np = parent; + if (of_property_read_u32(node, "#address-cells", &naddr)) + naddr = of_n_addr_cells(node); + if (of_property_read_u32(node, "#size-cells", &nsize)) + nsize = of_n_size_cells(node); + } else { + return -EINVAL; + } + while (node) { ranges = of_get_property(node, "dma-ranges", &len); - /* Ignore empty ranges, they imply no translation required */ - if (ranges && len > 0) - break; - /* * At least empty ranges has to be defined for parent node if * DMA is supported */ if (!ranges) break; + + /* Ignore empty ranges, they imply no translation required */ + if (len > 0) + break; + + naddr = of_n_addr_cells(node); + nsize = of_n_size_cells(node); + node = of_get_next_parent(node); } if (!ranges) { diff --git a/drivers/of/device.c b/drivers/of/device.c index 9bb8518b28f2..57ec5324ed6c 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -73,7 +73,8 @@ int of_device_add(struct platform_device *ofdev) /** * of_dma_configure - Setup DMA configuration * @dev: Device to apply DMA configuration - * @np: Pointer to OF node having DMA configuration + * @parent: OF node of device's parent bus, if @dev is not + * represented in DT (i.e. @dev->of_node is NULL) * * Try to get devices's DMA configuration from DT and update it * accordingly. @@ -82,13 +83,14 @@ int of_device_add(struct platform_device *ofdev) * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -void of_dma_configure(struct device *dev, struct device_node *np) +void of_dma_configure(struct device *dev, struct device_node *parent) { u64 dma_addr, paddr, size; int ret; bool coherent; unsigned long offset; const struct iommu_ops *iommu; + struct device_node *np = dev->of_node; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to @@ -104,7 +106,7 @@ void of_dma_configure(struct device *dev, struct device_node *np) if (!dev->dma_mask) dev->dma_mask = &dev->coherent_dma_mask; - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); + ret = of_dma_get_range(np, parent, &dma_addr, &paddr, &size); if (ret < 0) { dma_addr = offset = 0; size = dev->coherent_dma_mask + 1; @@ -132,11 +134,11 @@ void of_dma_configure(struct device *dev, struct device_node *np) dev->coherent_dma_mask = DMA_BIT_MASK(ilog2(dma_addr + size)); *dev->dma_mask = dev->coherent_dma_mask; - coherent = of_dma_is_coherent(np); + coherent = of_dma_is_coherent(np ? np : parent); dev_dbg(dev, "device is%sdma coherent\n", coherent ? " " : " not "); - iommu = of_iommu_configure(dev, np); + iommu = of_iommu_configure(dev, parent); dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 37864734ca50..f1a507f3ae57 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -52,8 +52,8 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser, extern struct of_pci_range *of_pci_range_parser_one( struct of_pci_range_parser *parser, struct of_pci_range *range); -extern int of_dma_get_range(struct device_node *np, u64 *dma_addr, - u64 *paddr, u64 *size); +extern int of_dma_get_range(struct device_node *np, struct device_node *parent, + u64 *dma_addr, u64 *paddr, u64 *size); extern bool of_dma_is_coherent(struct device_node *np); #else /* CONFIG_OF_ADDRESS */ static inline void __iomem *of_io_request_and_map(struct device_node *device, @@ -95,7 +95,8 @@ static inline struct of_pci_range *of_pci_range_parser_one( return NULL; } -static inline int of_dma_get_range(struct device_node *np, u64 *dma_addr, +static inline int of_dma_get_range(struct device_node *np, + struct device_node *parent, u64 *dma_addr, u64 *paddr, u64 *size) { return -ENODEV; diff --git a/include/linux/of_device.h b/include/linux/of_device.h index c12dace043f3..bcd2b6fbeef3 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -55,7 +55,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); } -void of_dma_configure(struct device *dev, struct device_node *np); +void of_dma_configure(struct device *dev, struct device_node *parent); #else /* CONFIG_OF */ static inline int of_driver_match_device(struct device *dev, @@ -103,7 +103,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) { return NULL; } -static inline void of_dma_configure(struct device *dev, struct device_node *np) +static inline void of_dma_configure(struct device *dev, struct device_node *parent) {} #endif /* CONFIG_OF */ diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index 13394ac83c66..c02b62e8e6ed 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -12,7 +12,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, size_t *size); extern const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np); + struct device_node *parent); #else @@ -24,7 +24,7 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, } static inline const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np) + struct device_node *parent) { return NULL; }
For PCI masters not represented in DT, we pass the OF node of their associated host bridge to of_dma_configure(), such that they can inherit the appropriate DMA configuration from whatever is described there. Unfortunately, whilst this has worked for the "dma-coherent" property, it turns out to miss the case where the host bridge node has a non-empty "dma-ranges", since nobody is expecting the 'device' to be a bus itself. It transpires, though, that the de-facto interface since the prototype change in 1f5c69aa51f9 ("of: Move of_dma_configure() to device.c to help re-use") is very clear-cut: either the master_np argument is redundant with dev->of_node, or dev->of_node is NULL and master_np is the relevant parent bus. Let's ratify that behaviour, then teach the whole of_dma_configure() pipeline to cope with both cases properly. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- This is what I'd consider the better fix - rather than adding yet more special cases - which will also make it simple to handle multiple "dma-ranges" entries with minimal further disruption. The callers now left passing dev->of_node as 'parent' are harmless, but look a bit silly and clearly want cleaning up - I'd be partial to renaming the existing function and having a single-argument wrapper for the 'normal' case, e.g.: static inline int of_dma_configure(struct device_node *dev) { return of_dma_configure_parent(dev, NULL); } Thoughts? Robin. drivers/iommu/of_iommu.c | 7 ++++--- drivers/of/address.c | 37 +++++++++++++++++++++++++------------ drivers/of/device.c | 12 +++++++----- include/linux/of_address.h | 7 ++++--- include/linux/of_device.h | 4 ++-- include/linux/of_iommu.h | 4 ++-- 6 files changed, 44 insertions(+), 27 deletions(-)