On Wed, Mar 29, 2017 at 12:27 AM, Robin Murphy <robin.murphy@xxxxxxx> 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@xxxxxxx>
> ---
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
