Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Regards, Nicolas
[1] https://patchwork.kernel.org/patch/9650345/#20294961 [2] https://patchwork.kernel.org/patch/9650345/
---
Nicolas Saenz Julienne (11): of: address: clean-up unused variable in of_dma_get_range() of: base: introduce __of_n_*_cells_parent() of: address: use parent DT node in bus->count_cells() of: address: introduce of_translate_dma_address_parent() of: expose __of_get_dma_parent() to OF subsystem of: address: use parent OF node in of_dma_get_range() dts: arm64: layerscape: add dma-ranges property to qoric-mc node dts: arm64: layerscape: add dma-ranges property to pcie nodes of: device: remove comment in of_dma_configure() of: device: introduce of_dma_configure_parent() of: simplify of_dma_config()'s arguments
.../arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 1 + .../arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 5 + .../arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 1 + drivers/base/platform.c | 2 +- drivers/bcma/main.c | 2 +- drivers/bus/fsl-mc/fsl-mc-bus.c | 2 +- drivers/dma/qcom/hidma_mgmt.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 2 +- drivers/gpu/drm/xen/xen_drm_front.c | 2 +- drivers/gpu/host1x/bus.c | 4 +- drivers/media/platform/qcom/venus/firmware.c | 2 +- drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 +- drivers/of/address.c | 136 +++++++++--------- drivers/of/base.c | 69 +++++++-- drivers/of/device.c | 59 +++++++- drivers/of/of_private.h | 5 + drivers/pci/pci-driver.c | 3 +- drivers/xen/gntdev.c | 2 +- include/linux/of_address.h | 8 +- include/linux/of_device.h | 23 ++- 22 files changed, 223 insertions(+), 113 deletions(-)
'len' in of_dma_get_range() is used to check the 'dma-ranges' property length. After the fact, some calculations are run on the variable to be then left unused.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/address.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c index 978427a9d5e6..0f898756199d 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -922,7 +922,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz { struct device_node *node = of_node_get(np); const __be32 *ranges = NULL; - int len, naddr, nsize, pna; + int naddr, nsize, pna; int ret = 0; u64 dmaaddr;
@@ -931,6 +931,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
while (1) { struct device_node *parent; + int len;
naddr = of_n_addr_cells(node); nsize = of_n_size_cells(node); @@ -962,8 +963,6 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz goto out; }
- len /= sizeof(u32); - pna = of_n_addr_cells(node);
/* dma-ranges format:
Master PCI devices might not appear in the device tree, yet they still need to get the underlying cells properties in order to calculate the bus DMA constraints. This conflicts with of_n_*_cells() as it's designed under the assumption it'll receive a device OF node.
Create __of_n_*_cells_parent() in order to deal with this limitation. For now, it'll only be available privately to OF code.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/base.c | 44 +++++++++++++++++++++++++++++------------ drivers/of/of_private.h | 3 +++ 2 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c index 1d667eb730e1..94f83051910c 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -86,34 +86,52 @@ static bool __of_node_is_type(const struct device_node *np, const char *type) return np && match && type && !strcmp(match, type); }
-int of_n_addr_cells(struct device_node *np) +int __of_n_addr_cells_parent(struct device_node *parent) { u32 cells;
- do { - if (np->parent) - np = np->parent; - if (!of_property_read_u32(np, "#address-cells", &cells)) + while (parent) { + if (!of_property_read_u32(parent, "#address-cells", &cells)) return cells; - } while (np->parent); + + parent = parent->parent; + } + /* No #address-cells property for the root node */ return OF_ROOT_NODE_ADDR_CELLS_DEFAULT; } + +int of_n_addr_cells(struct device_node *np) +{ + if (np->parent) + np = np->parent; + + return __of_n_addr_cells_parent(np); +} EXPORT_SYMBOL(of_n_addr_cells);
-int of_n_size_cells(struct device_node *np) +int __of_n_size_cells_parent(struct device_node *parent) { u32 cells;
- do { - if (np->parent) - np = np->parent; - if (!of_property_read_u32(np, "#size-cells", &cells)) + while (parent) { + if (!of_property_read_u32(parent, "#size-cells", &cells)) return cells; - } while (np->parent); - /* No #size-cells property for the root node */ + + parent = parent->parent; + } + + /* No #address-cells property for the root node */ return OF_ROOT_NODE_SIZE_CELLS_DEFAULT; } + +int of_n_size_cells(struct device_node *np) +{ + if (np->parent) + np = np->parent; + + return __of_n_size_cells_parent(np); +} EXPORT_SYMBOL(of_n_size_cells);
#ifdef CONFIG_NUMA diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index 24786818e32e..b528304be244 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -39,6 +39,9 @@ extern struct mutex of_mutex; extern struct list_head aliases_lookup; extern struct kset *of_kset;
+int __of_n_addr_cells_parent(struct device_node *parent); +int __of_n_size_cells_parent(struct device_node *parent); + #if defined(CONFIG_OF_DYNAMIC) extern int of_property_notify(int action, struct device_node *np, struct property *prop, struct property *old_prop);
The function provides the cell sizes for a specific bus type. Instead of passing it the device DT node sitting on top of that bus we directly pass its parent which is the actual node the function will start looking from.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/address.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c index 0f898756199d..9c1e638fa8ea 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -14,6 +14,8 @@ #include <linux/slab.h> #include <linux/string.h>
+#include "of_private.h" + /* Max address size we deal with */ #define OF_MAX_ADDR_CELLS 4 #define OF_CHECK_ADDR_COUNT(na) ((na) > 0 && (na) <= OF_MAX_ADDR_CELLS) @@ -42,7 +44,7 @@ struct of_bus { const char *name; const char *addresses; int (*match)(struct device_node *parent); - void (*count_cells)(struct device_node *child, + void (*count_cells)(struct device_node *parent, int *addrc, int *sizec); u64 (*map)(__be32 *addr, const __be32 *range, int na, int ns, int pna); @@ -54,13 +56,13 @@ struct of_bus { * Default translator (generic bus) */
-static void of_bus_default_count_cells(struct device_node *dev, +static void of_bus_default_count_cells(struct device_node *parent, int *addrc, int *sizec) { if (addrc) - *addrc = of_n_addr_cells(dev); + *addrc = __of_n_addr_cells_parent(parent); if (sizec) - *sizec = of_n_size_cells(dev); + *sizec = __of_n_size_cells_parent(parent); }
static u64 of_bus_default_map(__be32 *addr, const __be32 *range, @@ -192,7 +194,7 @@ const __be32 *of_get_pci_address(struct device_node *dev, int bar_no, u64 *size, of_node_put(parent); return NULL; } - bus->count_cells(dev, &na, &ns); + bus->count_cells(parent, &na, &ns); of_node_put(parent); if (!OF_CHECK_ADDR_COUNT(na)) return NULL; @@ -592,7 +594,7 @@ static u64 __of_translate_address(struct device_node *dev, bus = of_match_bus(parent);
/* Count address cells & copy address locally */ - bus->count_cells(dev, &na, &ns); + bus->count_cells(parent, &na, &ns); if (!OF_CHECK_COUNTS(na, ns)) { pr_debug("Bad cell count for %pOF\n", dev); goto bail; @@ -634,7 +636,7 @@ static u64 __of_translate_address(struct device_node *dev,
/* Get new parent bus and counts */ pbus = of_match_bus(parent); - pbus->count_cells(dev, &pna, &pns); + pbus->count_cells(parent, &pna, &pns); if (!OF_CHECK_COUNTS(pna, pns)) { pr_err("Bad cell count for %pOF\n", dev); break; @@ -726,7 +728,7 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size, if (parent == NULL) return NULL; bus = of_match_bus(parent); - bus->count_cells(dev, &na, &ns); + bus->count_cells(parent, &na, &ns); of_node_put(parent); if (!OF_CHECK_ADDR_COUNT(na)) return NULL;
Some devices might not have a DT node of their own, but might still need to translate DMA addresses based on their bus DT node.
Update of_translate_dma_address() to only depend on the parent DT node. Rename it to of_translate_dma_address_parent(). The later will be still available as a wrapper around the new function.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/address.c | 60 ++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c index 9c1e638fa8ea..c9eb4ebcc2e9 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -570,33 +570,32 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus, * device that had registered logical PIO mapping, and the return code is * relative to that node. */ -static u64 __of_translate_address(struct device_node *dev, - struct device_node *(*get_parent)(const struct device_node *), - const __be32 *in_addr, const char *rprop, - struct device_node **host) +static u64 __of_translate_address_parent(struct device_node *parent, + struct device_node *(*get_parent) + (const struct device_node *), + const __be32 *in_addr, + const char *rprop, + struct device_node **host) { - struct device_node *parent = NULL; + struct device_node *dev = NULL; struct of_bus *bus, *pbus; __be32 addr[OF_MAX_ADDR_CELLS]; int na, ns, pna, pns; u64 result = OF_BAD_ADDR;
- pr_debug("** translation for device %pOF **\n", dev); - - /* Increase refcount at current level */ - of_node_get(dev); - *host = NULL; - /* Get parent & match bus type */ - parent = get_parent(dev); - if (parent == NULL) + + if (!parent) goto bail; + + /* Increase refcount at current level and match bus type */ + of_node_get(parent); bus = of_match_bus(parent);
/* Count address cells & copy address locally */ bus->count_cells(parent, &na, &ns); if (!OF_CHECK_COUNTS(na, ns)) { - pr_debug("Bad cell count for %pOF\n", dev); + pr_debug("Bad cell count for %pOF\n", parent); goto bail; } memcpy(addr, in_addr, na * 4); @@ -610,9 +609,8 @@ static u64 __of_translate_address(struct device_node *dev, struct logic_pio_hwaddr *iorange;
/* Switch to parent bus */ - of_node_put(dev); dev = parent; - parent = get_parent(dev); + parent = get_parent(parent);
/* If root, we have finished */ if (parent == NULL) { @@ -650,6 +648,8 @@ static u64 __of_translate_address(struct device_node *dev, break;
/* Complete the move up one level */ + of_node_put(dev); + dev = parent; na = pna; ns = pns; bus = pbus; @@ -668,8 +668,10 @@ u64 of_translate_address(struct device_node *dev, const __be32 *in_addr) struct device_node *host; u64 ret;
- ret = __of_translate_address(dev, of_get_parent, - in_addr, "ranges", &host); + pr_debug("** translation for device %pOF **\n", dev); + + ret = __of_translate_address_parent(dev->parent, of_get_parent, + in_addr, "ranges", &host); if (host) { of_node_put(host); return OF_BAD_ADDR; @@ -697,14 +699,14 @@ static struct device_node *__of_get_dma_parent(const struct device_node *np) return of_node_get(args.np); }
-u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) +static u64 of_translate_dma_address_parent(struct device_node *parent, + const __be32 *in_addr) { struct device_node *host; u64 ret;
- ret = __of_translate_address(dev, __of_get_dma_parent, - in_addr, "dma-ranges", &host); - + ret = __of_translate_address_parent(parent, __of_get_dma_parent, in_addr, + "dma-ranges", &host); if (host) { of_node_put(host); return OF_BAD_ADDR; @@ -712,6 +714,14 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
return ret; } + +u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) +{ + pr_debug("** translation for device %pOF **\n", dev); + + return of_translate_dma_address_parent(__of_get_dma_parent(dev->parent), + in_addr); +} EXPORT_SYMBOL(of_translate_dma_address);
const __be32 *of_get_address(struct device_node *dev, int index, u64 *size, @@ -759,8 +769,10 @@ static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr, unsigned long port; struct device_node *host;
- taddr = __of_translate_address(dev, of_get_parent, - in_addr, "ranges", &host); + pr_debug("** translation for device %pOF **\n", dev); + + taddr = __of_translate_address_parent(dev->parent, of_get_parent, + in_addr, "ranges", &host); if (host) { /* host-specific port access */ port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size);
The function was only available locally to of/address.c, make it available to the OF subsystem.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/address.c | 18 ------------------ drivers/of/base.c | 25 +++++++++++++++++++++++++ drivers/of/of_private.h | 2 ++ 3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c index c9eb4ebcc2e9..53666063e938 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -681,24 +681,6 @@ u64 of_translate_address(struct device_node *dev, const __be32 *in_addr) } EXPORT_SYMBOL(of_translate_address);
-static struct device_node *__of_get_dma_parent(const struct device_node *np) -{ - struct of_phandle_args args; - int ret, index; - - index = of_property_match_string(np, "interconnect-names", "dma-mem"); - if (index < 0) - return of_get_parent(np); - - ret = of_parse_phandle_with_args(np, "interconnects", - "#interconnect-cells", - index, &args); - if (ret < 0) - return of_get_parent(np); - - return of_node_get(args.np); -} - static u64 of_translate_dma_address_parent(struct device_node *parent, const __be32 *in_addr) { diff --git a/drivers/of/base.c b/drivers/of/base.c index 94f83051910c..ec161e6b5222 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -716,6 +716,31 @@ struct device_node *of_get_parent(const struct device_node *node) } EXPORT_SYMBOL(of_get_parent);
+/** + * __of_get_dma_parent - Get a node's dma parent if any + * @node: Node to get dma parent + * + * Returns a node pointer with refcount incremented, use + * of_node_put() on it when done. + */ +struct device_node *__of_get_dma_parent(const struct device_node *np) +{ + struct of_phandle_args args; + int ret, index; + + index = of_property_match_string(np, "interconnect-names", "dma-mem"); + if (index < 0) + return of_get_parent(np); + + ret = of_parse_phandle_with_args(np, "interconnects", + "#interconnect-cells", + index, &args); + if (ret < 0) + return of_get_parent(np); + + return of_node_get(args.np); +} + /** * of_get_next_parent - Iterate to a node's parent * @node: Node to get parent of diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index b528304be244..63bb16cc454c 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -42,6 +42,8 @@ extern struct kset *of_kset; int __of_n_addr_cells_parent(struct device_node *parent); int __of_n_size_cells_parent(struct device_node *parent);
+struct device_node *__of_get_dma_parent(const struct device_node *np); + #if defined(CONFIG_OF_DYNAMIC) extern int of_property_notify(int action, struct device_node *np, struct property *prop, struct property *old_prop);
Some devices don't have their own OF node, and are stuck passing their bus node. Adapt the function for this use case.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/address.c | 33 +++++++++++++++------------------ drivers/of/device.c | 3 ++- include/linux/of_address.h | 8 ++++---- 3 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c index 53666063e938..b7b864b0ac75 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -914,30 +914,21 @@ 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 *parent, u64 *dma_addr, + u64 *paddr, u64 *size) { - struct device_node *node = of_node_get(np); + struct device_node *node = of_node_get(parent); const __be32 *ranges = NULL; int naddr, nsize, pna; int ret = 0; u64 dmaaddr;
- if (!node) - return -EINVAL; - while (1) { - struct device_node *parent; + struct device_node *tmp; int len;
- naddr = of_n_addr_cells(node); - nsize = of_n_size_cells(node); - - parent = __of_get_dma_parent(node); - of_node_put(node); - - node = parent; - if (!node) - break; + naddr = __of_n_addr_cells_parent(node); + nsize = __of_n_size_cells_parent(node);
ranges = of_get_property(node, "dma-ranges", &len);
@@ -951,10 +942,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz */ if (!ranges) break; + + tmp = __of_get_dma_parent(node); + of_node_put(node); + node = tmp; + if (!node) + break; }
if (!ranges) { - pr_debug("no dma-ranges found for node(%pOF)\n", np); + pr_debug("no dma-ranges found for node(%pOF)\n", node); ret = -ENODEV; goto out; } @@ -967,10 +964,10 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz * size : nsize cells */ dmaaddr = of_read_number(ranges, naddr); - *paddr = of_translate_dma_address(np, ranges); + *paddr = of_translate_dma_address_parent(parent, ranges); if (*paddr == OF_BAD_ADDR) { pr_err("translation of DMA address(%pad) to CPU address failed node(%pOF)\n", - dma_addr, np); + dma_addr, parent); ret = -EINVAL; goto out; } diff --git a/drivers/of/device.c b/drivers/of/device.c index da8158392010..267b509df517 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -95,7 +95,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) const struct iommu_ops *iommu; u64 mask;
- ret = of_dma_get_range(np, &dma_addr, &paddr, &size); + ret = of_dma_get_range(__of_get_dma_parent(np), &dma_addr, + &paddr, &size); if (ret < 0) { /* * For legacy reasons, we have to assume some devices need diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 30e40fb6936b..aa1ce0cb59a1 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -55,8 +55,8 @@ extern int of_pci_dma_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 *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, @@ -104,8 +104,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, - u64 *paddr, u64 *size) +static inline int of_dma_get_range(struct device_node *parent, u64 *dma_addr, + u64 *paddr, u64 *size) { return -ENODEV; }
qoriq-mc's dpmacs DMA configuration is inherited from their parent node, which acts a bus in this regard. So far it maked all devices as dma-coherent but no dma-ranges recommendation is made.
The truth is that the underlying interconnect has DMA constraints, so add an empty dma-ranges in qoriq-mc's node in order for DT's DMA configuration code to get the DMA constraints from it.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 1 + arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 1 + arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 1 + 3 files changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi index c676d0771762..f0d0b6145b72 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi @@ -698,6 +698,7 @@ <0x00000000 0x08340000 0 0x40000>; /* MC control reg */ msi-parent = <&its>; iommu-map = <0 &smmu 0 0>; /* This is fixed-up by u-boot */ + dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>; diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi index 7a0be8eaa84a..fd6036b7865c 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi @@ -340,6 +340,7 @@ <0x00000000 0x08340000 0 0x40000>; /* MC control reg */ msi-parent = <&its>; iommu-map = <0 &smmu 0 0>; /* This is fixed-up by u-boot */ + dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>; diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi index 408e0ecdce6a..3735bb139cb2 100644 --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi @@ -868,6 +868,7 @@ msi-parent = <&its>; /* iommu-map property is fixed up by u-boot */ iommu-map = <0 &smmu 0 0>; + dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>;
On Tue, Sep 24, 2019 at 08:12:38PM +0200, Nicolas Saenz Julienne wrote:
qoriq-mc's dpmacs DMA configuration is inherited from their parent node, which acts a bus in this regard. So far it maked all devices as dma-coherent but no dma-ranges recommendation is made.
The truth is that the underlying interconnect has DMA constraints, so add an empty dma-ranges in qoriq-mc's node in order for DT's DMA configuration code to get the DMA constraints from it.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
Updated subject prefix as 'arm64: dts: ...', and applied the patch.
Shawn
arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 1 + arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 1 + arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 1 + 3 files changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi index c676d0771762..f0d0b6145b72 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi @@ -698,6 +698,7 @@ <0x00000000 0x08340000 0 0x40000>; /* MC control reg */ msi-parent = <&its>; iommu-map = <0 &smmu 0 0>; /* This is fixed-up by u-boot */
dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>;diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi index 7a0be8eaa84a..fd6036b7865c 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi @@ -340,6 +340,7 @@ <0x00000000 0x08340000 0 0x40000>; /* MC control reg */ msi-parent = <&its>; iommu-map = <0 &smmu 0 0>; /* This is fixed-up by u-boot */
dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>;diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi index 408e0ecdce6a..3735bb139cb2 100644 --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi @@ -868,6 +868,7 @@ msi-parent = <&its>; /* iommu-map property is fixed up by u-boot */ iommu-map = <0 &smmu 0 0>;
dma-ranges; dma-coherent; #address-cells = <3>; #size-cells = <1>;-- 2.23.0
On Mon, 2019-10-14 at 16:28 +0800, Shawn Guo wrote:
On Tue, Sep 24, 2019 at 08:12:38PM +0200, Nicolas Saenz Julienne wrote:
qoriq-mc's dpmacs DMA configuration is inherited from their parent node, which acts a bus in this regard. So far it maked all devices as dma-coherent but no dma-ranges recommendation is made.
The truth is that the underlying interconnect has DMA constraints, so add an empty dma-ranges in qoriq-mc's node in order for DT's DMA configuration code to get the DMA constraints from it.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
Updated subject prefix as 'arm64: dts: ...', and applied the patch.
Hi Shawn, these two patches are no longer needed. This series has been superseded by this patch[1] 951d48855d ('of: Make of_dma_get_range() work on bus nodes', available in linux-next) which fixed the issue directly in OF code.
Sorry for the noise.
Regards, Nicolas
On Mon, Oct 14, 2019 at 12:00:25PM +0200, Nicolas Saenz Julienne wrote:
On Mon, 2019-10-14 at 16:28 +0800, Shawn Guo wrote:
On Tue, Sep 24, 2019 at 08:12:38PM +0200, Nicolas Saenz Julienne wrote:
qoriq-mc's dpmacs DMA configuration is inherited from their parent node, which acts a bus in this regard. So far it maked all devices as dma-coherent but no dma-ranges recommendation is made.
The truth is that the underlying interconnect has DMA constraints, so add an empty dma-ranges in qoriq-mc's node in order for DT's DMA configuration code to get the DMA constraints from it.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
Updated subject prefix as 'arm64: dts: ...', and applied the patch.
Hi Shawn, these two patches are no longer needed. This series has been superseded by this patch[1] 951d48855d ('of: Make of_dma_get_range() work on bus nodes', available in linux-next) which fixed the issue directly in OF code.
Sorry for the noise.
Okay, thanks for letting me know. Dropped them.
Shawn
The bus behind the board's PCIe core has DMA addressing limitations. Add an empty 'dma-ranges' property on all PCIe bus descriptions to inform the OF core that a translation is due further down the line.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi index fd6036b7865c..2c41cfc66bb9 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi @@ -640,6 +640,7 @@ #size-cells = <2>; device_type = "pci"; dma-coherent; + dma-ranges; num-viewport = <6>; bus-range = <0x0 0xff>; msi-parent = <&its>; @@ -661,6 +662,7 @@ #size-cells = <2>; device_type = "pci"; dma-coherent; + dma-ranges; num-viewport = <6>; bus-range = <0x0 0xff>; msi-parent = <&its>; @@ -682,6 +684,7 @@ #size-cells = <2>; device_type = "pci"; dma-coherent; + dma-ranges; num-viewport = <256>; bus-range = <0x0 0xff>; msi-parent = <&its>; @@ -703,6 +706,7 @@ #size-cells = <2>; device_type = "pci"; dma-coherent; + dma-ranges; num-viewport = <6>; bus-range = <0x0 0xff>; msi-parent = <&its>;
On Tue, Sep 24, 2019 at 08:12:39PM +0200, Nicolas Saenz Julienne wrote:
The bus behind the board's PCIe core has DMA addressing limitations. Add an empty 'dma-ranges' property on all PCIe bus descriptions to inform the OF core that a translation is due further down the line.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
Applied, thanks.
It's not longer advised to use notifiers in order to setup custom DMA ops.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/of/device.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c index 267b509df517..018c52688546 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -81,10 +81,6 @@ int of_device_add(struct platform_device *ofdev) * * Try to get devices's DMA configuration from DT and update it * accordingly. - * - * If platform code needs to use its own special DMA configuration, it - * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events - * to fix up DMA configuration. */ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) {
Devices not represented in DT, placed behind a bus that autodetects them, pass of_dma_configure() their bus' DT node. This misses that bus' 'dma-ranges' property as the function's implementation expects the DT node to be one of a device.
Create of_dma_configure_parent() which takes the given DT node as the device's underlying bus and parses it accordingly.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de ---
drivers/bus/fsl-mc/fsl-mc-bus.c | 2 +- drivers/gpu/host1x/bus.c | 2 +- drivers/of/device.c | 30 ++++++++++++++++++++++++++++-- drivers/pci/pci-driver.c | 3 ++- include/linux/of_device.h | 13 ++++++++++--- 5 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c index 5c9bf2e06552..217e64119497 100644 --- a/drivers/bus/fsl-mc/fsl-mc-bus.c +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c @@ -134,7 +134,7 @@ static int fsl_mc_dma_configure(struct device *dev) while (dev_is_fsl_mc(dma_dev)) dma_dev = dma_dev->parent;
- return of_dma_configure(dev, dma_dev->of_node, 0); + return of_dma_configure_parent(dev, dma_dev->of_node, false); }
static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c index 742aa9ff21b8..2fdc04383a60 100644 --- a/drivers/gpu/host1x/bus.c +++ b/drivers/gpu/host1x/bus.c @@ -442,7 +442,7 @@ static int host1x_device_add(struct host1x *host1x, device->dev.bus = &host1x_bus_type; device->dev.parent = host1x->dev;
- of_dma_configure(&device->dev, host1x->dev->of_node, true); + of_dma_configure_parent(&device->dev, host1x->dev->of_node, true);
device->dev.dma_parms = &device->dma_parms; dma_set_max_seg_size(&device->dev, SZ_4M); diff --git a/drivers/of/device.c b/drivers/of/device.c index 018c52688546..1d0b1127d05e 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -83,6 +83,31 @@ int of_device_add(struct platform_device *ofdev) * accordingly. */ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) +{ + struct device_node *dma_parent; + int ret; + + dma_parent = __of_get_dma_parent(np); + ret = of_dma_configure_parent(dev, dma_parent, force_dma); + of_node_put(dma_parent); + + return ret; +} +EXPORT_SYMBOL_GPL(of_dma_configure); + +/** + * of_dma_configure_parent - Setup DMA configuration based on a parent bus' + * DT node + * @dev: Device to apply DMA configuration + * @parent: Parent bus device node + * @force_dma: Whether device is to be set up by of_dma_configure() even if + * DMA capability is not explicitly described by firmware. + * + * Try to get devices's DMA configuration from parent bus' DT and update it + * accordingly. + */ +int of_dma_configure_parent(struct device *dev, struct device_node *parent, + bool force_dma) { u64 dma_addr, paddr, size = 0; int ret; @@ -91,8 +116,9 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) const struct iommu_ops *iommu; u64 mask;
- ret = of_dma_get_range(__of_get_dma_parent(np), &dma_addr, - &paddr, &size); + dev_dbg(dev, "starting at %pOF\n", parent); + + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); if (ret < 0) { /* * For legacy reasons, we have to assume some devices need diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index a8124e47bf6e..25073d68a0f6 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1628,7 +1628,8 @@ static int pci_dma_configure(struct device *dev)
if (IS_ENABLED(CONFIG_OF) && bridge->parent && bridge->parent->of_node) { - ret = of_dma_configure(dev, bridge->parent->of_node, true); + ret = of_dma_configure_parent(dev, bridge->parent->of_node, + true); } else if (has_acpi_companion(bridge)) { struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 8d31e39dd564..8f319b8c9deb 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -55,9 +55,9 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); }
-int of_dma_configure(struct device *dev, - struct device_node *np, - bool force_dma); +int of_dma_configure(struct device *dev, struct devce_node *np, bool force_dma); +int of_dma_configure_parent(struct device *dev, struct device_node *parent, + bool force_dma); #else /* CONFIG_OF */
static inline int of_driver_match_device(struct device *dev, @@ -112,6 +112,13 @@ static inline int of_dma_configure(struct device *dev, { return 0; } + +static inline int of_dma_configure_parent(struct device *dev, + struct device_node *parent, + bool force_dma) +{ + return 0; +} #endif /* CONFIG_OF */
#endif /* _LINUX_OF_DEVICE_H */
The widespread use case for of_dma_config() highlights a redundant argument. Most callers provide both 'dev' and 'dev->of_node'. The rest of users use it to copy some device's DMA configuration into a device not represented in DT.
In order to simplify the common use case, and make code a little more explicit, create of_dma_config_copy() which will take another device's DT node as an argument and simplify of_dma_config() by removing one of it's redundant arguments.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
---
drivers/base/platform.c | 2 +- drivers/bcma/main.c | 2 +- drivers/dma/qcom/hidma_mgmt.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 2 +- drivers/gpu/drm/xen/xen_drm_front.c | 2 +- drivers/gpu/host1x/bus.c | 2 +- drivers/media/platform/qcom/venus/firmware.c | 2 +- drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 +- drivers/of/device.c | 30 ++++++++++++++++++-- drivers/xen/gntdev.c | 2 +- include/linux/of_device.h | 12 ++++++-- 13 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index b6c6c7d97d5b..4e252028aee9 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -1221,7 +1221,7 @@ int platform_dma_configure(struct device *dev) int ret = 0;
if (dev->of_node) { - ret = of_dma_configure(dev, dev->of_node, true); + ret = of_dma_configure(dev, true); } else if (has_acpi_companion(dev)) { attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode)); ret = acpi_dma_configure(dev, attr); diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c index 6535614a7dc1..4c7e8b35525c 100644 --- a/drivers/bcma/main.c +++ b/drivers/bcma/main.c @@ -207,7 +207,7 @@ static void bcma_of_fill_device(struct device *parent,
core->irq = bcma_of_get_irq(parent, core, 0);
- of_dma_configure(&core->dev, node, false); + of_dma_configure(&core->dev, false); }
unsigned int bcma_core_irq(struct bcma_device *core, int num) diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c index 806ca02c52d7..4ea901537902 100644 --- a/drivers/dma/qcom/hidma_mgmt.c +++ b/drivers/dma/qcom/hidma_mgmt.c @@ -388,7 +388,7 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np) goto out; } new_pdev->dev.of_node = child; - of_dma_configure(&new_pdev->dev, child, true); + of_dma_configure(&new_pdev->dev, true); /* * It is assumed that calling of_msi_configure is safe on * platforms with or without MSI support. diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 1f9c01be40d7..1375f41593ef 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -706,7 +706,7 @@ static int __init etnaviv_init(void) * device as the GPU we found. This assumes that all Vivante * GPUs in the system share the same DMA constraints. */ - of_dma_configure(&pdev->dev, np, true); + of_dma_configure_copy(&pdev->dev, np, true);
ret = platform_device_add(pdev); if (ret) { diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index 85f14feafdec..825fcdc72bd9 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -1268,7 +1268,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
gmu->dev = &pdev->dev;
- of_dma_configure(gmu->dev, node, true); + of_dma_configure(gmu->dev, true);
/* Fow now, don't do anything fancy until we get our feet under us */ gmu->idle_level = GMU_IDLE_STATE_ACTIVE; diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 4e29f4fe4a05..97dd5d31e38d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -802,7 +802,7 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, * for us, and DRM doesn't do per-device allocation either, so * we would need to fix DRM first... */ - ret = of_dma_configure(drm->dev, dev->of_node, true); + ret = of_dma_configure_copy(drm->dev, dev->of_node, true); if (ret) return ret; } else { diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c index ba1828acd8c9..fa0bfd6517fa 100644 --- a/drivers/gpu/drm/xen/xen_drm_front.c +++ b/drivers/gpu/drm/xen/xen_drm_front.c @@ -726,7 +726,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev, * node to set default DMA ops. */ dev->coherent_dma_mask = DMA_BIT_MASK(32); - ret = of_dma_configure(dev, NULL, true); + ret = of_dma_configure(dev, true); if (ret < 0) { DRM_ERROR("Cannot setup DMA ops, ret %d", ret); return ret; diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c index 2fdc04383a60..4eb531906e4a 100644 --- a/drivers/gpu/host1x/bus.c +++ b/drivers/gpu/host1x/bus.c @@ -337,7 +337,7 @@ static int host1x_device_uevent(struct device *dev,
static int host1x_dma_configure(struct device *dev) { - return of_dma_configure(dev, dev->of_node, true); + return of_dma_configure(dev, true); }
static const struct dev_pm_ops host1x_device_pm_ops = { diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index d3d1748a7ef6..82f433218d99 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -238,7 +238,7 @@ int venus_firmware_init(struct venus_core *core)
pdev->dev.of_node = np;
- ret = of_dma_configure(&pdev->dev, np, true); + ret = of_dma_configure(&pdev->dev, true); if (ret) { dev_err(core->dev, "dma configure fail\n"); goto err_unregister; diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c index b776f83e395e..ad33e7a655aa 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c @@ -1095,7 +1095,7 @@ static struct device *s5p_mfc_alloc_memdev(struct device *dev, * to be treated as valid DMA masters we need a bit of a hack to force * them to inherit the MFC node's DMA configuration. */ - of_dma_configure(child, dev->of_node, true); + of_dma_configure_copy(child, dev->of_node, true);
if (device_add(child) == 0) { ret = of_reserved_mem_device_init_by_idx(child, dev->of_node, diff --git a/drivers/of/device.c b/drivers/of/device.c index 1d0b1127d05e..66726a2499d3 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -75,19 +75,18 @@ 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 * @force_dma: Whether device is to be set up by of_dma_configure() even if * DMA capability is not explicitly described by firmware. * * Try to get devices's DMA configuration from DT and update it * accordingly. */ -int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) +int of_dma_configure(struct device *dev, bool force_dma) { struct device_node *dma_parent; int ret;
- dma_parent = __of_get_dma_parent(np); + dma_parent = __of_get_dma_parent(dev->of_node); ret = of_dma_configure_parent(dev, dma_parent, force_dma); of_node_put(dma_parent);
@@ -95,6 +94,31 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) } EXPORT_SYMBOL_GPL(of_dma_configure);
+/** + * of_dma_configure_copy - Setup DMA configuration based on another device's DT + * node + * @dev: Device to apply DMA configuration + * @np: Device node to copy the configuration from + * @force_dma: Whether device is to be set up by of_dma_configure() even if + * DMA capability is not explicitly described by firmware. + * + * Try to get devices's DMA configuration from DT and update it + * accordingly. + */ +int of_dma_configure_copy(struct device *dev, struct device_node *np, + bool force_dma) +{ + struct device_node *dma_parent; + int ret; + + dma_parent = __of_get_dma_parent(np); + ret = of_dma_configure_parent(dev, dma_parent, force_dma); + of_node_put(dma_parent); + + return ret; +} +EXPORT_SYMBOL_GPL(of_dma_configure_copy); + /** * of_dma_configure_parent - Setup DMA configuration based on a parent bus' * DT node diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index a446a7221e13..115e39f9f67a 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -632,7 +632,7 @@ static int gntdev_open(struct inode *inode, struct file *flip) * Fix this by calling of_dma_configure() with a NULL node to set * default DMA ops. */ - of_dma_configure(priv->dma_dev, NULL, true); + of_dma_configure(priv->dma_dev, true); #endif pr_debug("priv %p\n", priv);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 8f319b8c9deb..b05a86fd35a4 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -55,7 +55,9 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); }
-int of_dma_configure(struct device *dev, struct devce_node *np, bool force_dma); +int of_dma_configure(struct device *dev, bool force_dma); +int of_dma_configure_copy(struct device *dev, struct device_node *np, + bool force_dma); int of_dma_configure_parent(struct device *dev, struct device_node *parent, bool force_dma); #else /* CONFIG_OF */ @@ -107,12 +109,18 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) }
static inline int of_dma_configure(struct device *dev, - struct device_node *np, bool force_dma) { return 0; }
+static inline int of_dma_configure_copy(struct device *dev, + struct device_node *np, + bool force_dma) +{ + return 0; +} + static inline int of_dma_configure_parent(struct device *dev, struct device_node *parent, bool force_dma)
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
Rob
[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /* - * At least empty ranges has to be defined for parent node if - * DMA is supported - */ - if (!ranges) - break;
Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
Also I tested your branch both on an RPi4, with a PCI device that depends on these changes and by comparing the OF debugs logs on a Layerscape board which uses dma-ranges, dma-coherent and IOMMU. All works as expected.
Will you send this series for v5.5? Please keep me in the loop, I'll review and test the final version.
Regards, Nicolas
On 25/09/2019 15:52, Nicolas Saenz Julienne wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /*
* At least empty ranges has to be defined for parent node if* DMA is supported*/- if (!ranges)
break;Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
WRT that patch, the original intent of "force_dma" was purely to consider a device DMA-capable regardless of the presence of "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF device has always been bogus - magic paravirt devices which appear out of nowhere and expect to be treated as genuine DMA masters are a separate problem that we haven't really approached yet.
Robin.
On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote:
On 25/09/2019 15:52, Nicolas Saenz Julienne wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /*
* At least empty ranges has to be defined for parent node if* DMA is supported*/- if (!ranges)
break;Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
WRT that patch, the original intent of "force_dma" was purely to consider a device DMA-capable regardless of the presence of "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF device has always been bogus - magic paravirt devices which appear out of nowhere and expect to be treated as genuine DMA masters are a separate problem that we haven't really approached yet.
I agree it's clearly abusing the function. I have no problem with the behaviour change if it's OK with you.
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Regards, Nicolas
On Wed, Sep 25, 2019 at 10:30 AM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote:
On 25/09/2019 15:52, Nicolas Saenz Julienne wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /*
- At least empty ranges has to be defined for parent node if
- DMA is supported
- */
- if (!ranges)
break;Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
WRT that patch, the original intent of "force_dma" was purely to consider a device DMA-capable regardless of the presence of "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF device has always been bogus - magic paravirt devices which appear out of nowhere and expect to be treated as genuine DMA masters are a separate problem that we haven't really approached yet.
I agree it's clearly abusing the function. I have no problem with the behaviour change if it's OK with you.
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
Rob
On 25/09/2019 17:16, Rob Herring wrote:
On Wed, Sep 25, 2019 at 10:30 AM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote:
On 25/09/2019 15:52, Nicolas Saenz Julienne wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /*
- At least empty ranges has to be defined for parent node if
- DMA is supported
- */
- if (!ranges)
break;Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
WRT that patch, the original intent of "force_dma" was purely to consider a device DMA-capable regardless of the presence of "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF device has always been bogus - magic paravirt devices which appear out of nowhere and expect to be treated as genuine DMA masters are a separate problem that we haven't really approached yet.
I agree it's clearly abusing the function. I have no problem with the behaviour change if it's OK with you.
Thinking about it, you could probably just remove that call from the Xen DRM driver now anyway - since the dma-direct rework, we lost the ability to set dma_dummy_ops by default, and NULL ops now represent what it (presumably) wants.
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
There's also the in-between step of making of_dma_get_range() return a size based on all the dma-ranges entries rather than only the first one - otherwise, something like [1] can lead to pretty unworkable default masks. We implemented that when doing acpi_dma_get_range(), it's just that the OF counterpart never caught up.
Robin.
[1] http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=a2814af56b3486c2985a9...
On Wed, Sep 25, 2019 at 11:52 AM Robin Murphy robin.murphy@arm.com wrote:
On 25/09/2019 17:16, Rob Herring wrote:
On Wed, Sep 25, 2019 at 10:30 AM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote:
On 25/09/2019 15:52, Nicolas Saenz Julienne wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote: > Hi All, > this series tries to address one of the issues blocking us from > upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that > devices not represented in DT which sit behind a PCI bus fail to get the > bus' DMA addressing constraints. > > This is due to the fact that of_dma_configure() assumes it's receiving a > DT node representing the device being configured, as opposed to the PCIe > bridge node we currently pass. This causes the code to directly jump > into PCI's parent node when checking for 'dma-ranges' and misses > whatever was set there. > > To address this I create a new API in OF - inspired from Robin Murphys > original proposal[2] - which accepts a bus DT node as it's input in > order to configure a device's DMA constraints. The changes go deep into > of/address.c's implementation, as a device being having a DT node > assumption was pretty strong. > > On top of this work, I also cleaned up of_dma_configure() removing its > redundant arguments and creating an alternative function for the special > cases > not applicable to either the above case or the default usage. > > IMO the resulting functions are more explicit. They will probably > surface some hacky usages that can be properly fixed as I show with the > DT fixes on the Layerscape platform. > > This was also tested on a Raspberry Pi 4 with a custom PCIe driver and > on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
- /*
- At least empty ranges has to be defined for parent node if
- DMA is supported
- */
- if (!ranges)
break;Which I assumed was bound to the standard and makes things easier.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
WRT that patch, the original intent of "force_dma" was purely to consider a device DMA-capable regardless of the presence of "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF device has always been bogus - magic paravirt devices which appear out of nowhere and expect to be treated as genuine DMA masters are a separate problem that we haven't really approached yet.
I agree it's clearly abusing the function. I have no problem with the behaviour change if it's OK with you.
Thinking about it, you could probably just remove that call from the Xen DRM driver now anyway - since the dma-direct rework, we lost the ability to set dma_dummy_ops by default, and NULL ops now represent what it (presumably) wants.
Not xen_dma_ops? In any case, I'll send out a patch for the the Xen folks to comment on.
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
There's also the in-between step of making of_dma_get_range() return a size based on all the dma-ranges entries rather than only the first one
- otherwise, something like [1] can lead to pretty unworkable default
masks. We implemented that when doing acpi_dma_get_range(), it's just that the OF counterpart never caught up.
Right. I suppose we assume any holes in the ranges are addressable by the device but won't get used for other reasons (such as no memory there). However, to be correct, the range of the dma offset plus mask would need to be within the min start and max end addresses. IOW, while we need to round up (0xa_8000_0000 - 0x2c1c_0000) to the next power of 2, the 'correct' thing to do is round down.
Rob
[1] http://linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=a2814af56b3486c2985a9...
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
Sorry, I meant supporting multiple DMA offsets[1]. I think I could still make it with a single DMA mask though.
There's also the in-between step of making of_dma_get_range() return a size based on all the dma-ranges entries rather than only the first one
- otherwise, something like [1] can lead to pretty unworkable default
masks. We implemented that when doing acpi_dma_get_range(), it's just that the OF counterpart never caught up.
Right. I suppose we assume any holes in the ranges are addressable by the device but won't get used for other reasons (such as no memory there). However, to be correct, the range of the dma offset plus mask would need to be within the min start and max end addresses. IOW, while we need to round up (0xa_8000_0000 - 0x2c1c_0000) to the next power of 2, the 'correct' thing to do is round down.
IIUC I also have this issue on my list. The RPi4 PCIe block has an integration bug that only allows DMA to the lower 3GB. With dma-ranges of size 0xc000_0000 you get a 32bit DMA mask wich is not what you need. So far I faked it in the device-tree but I guess it be better to add an extra check in of_dma_configure(), decrease the mask and print some kind of warning stating that DMA addressing is suboptimal.
Regards, Nicolas
On 2019-09-26 11:44 am, Nicolas Saenz Julienne wrote:
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
Sorry, I meant supporting multiple DMA offsets[1]. I think I could still make it with a single DMA mask though.
The main problem for supporting that case in general is the disgusting carving up of the physical memory map you may have to do to guarantee that a single buffer allocation cannot ever span two windows with different offsets. I don't think we ever reached a conclusion on whether that was even achievable in practice.
There's also the in-between step of making of_dma_get_range() return a size based on all the dma-ranges entries rather than only the first one
- otherwise, something like [1] can lead to pretty unworkable default
masks. We implemented that when doing acpi_dma_get_range(), it's just that the OF counterpart never caught up.
Right. I suppose we assume any holes in the ranges are addressable by the device but won't get used for other reasons (such as no memory there). However, to be correct, the range of the dma offset plus mask would need to be within the min start and max end addresses. IOW, while we need to round up (0xa_8000_0000 - 0x2c1c_0000) to the next power of 2, the 'correct' thing to do is round down.
IIUC I also have this issue on my list. The RPi4 PCIe block has an integration bug that only allows DMA to the lower 3GB. With dma-ranges of size 0xc000_0000 you get a 32bit DMA mask wich is not what you need. So far I faked it in the device-tree but I guess it be better to add an extra check in of_dma_configure(), decrease the mask and print some kind of warning stating that DMA addressing is suboptimal.
Yeah, there's just no way for masks to describe that the device can drive all the individual bits, just not in certain combinations :(
The plan I have sketched out there is to merge dma_pfn_offset and bus_dma_mask into a "DMA range" descriptor, so we can then hang one or more of those off a device to properly cope with all these weird interconnects. Conceptually it feels pretty straightforward; I think most of the challenge is in implementing it efficiently. Plus there's the question of whether it could also subsume the dma_mask as well.
Robin.
On 9/26/2019 4:20 AM, Robin Murphy wrote:
On 2019-09-26 11:44 am, Nicolas Saenz Julienne wrote:
Robin, have you looked into supporting multiple dma-ranges? It's the next thing we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the works already.
Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet.
Sorry, I meant supporting multiple DMA offsets[1]. I think I could still make it with a single DMA mask though.
The main problem for supporting that case in general is the disgusting carving up of the physical memory map you may have to do to guarantee that a single buffer allocation cannot ever span two windows with different offsets. I don't think we ever reached a conclusion on whether that was even achievable in practice.
It is with the Broadcom STB SoCs which have between 1 and 3 memory controllers depending on the SoC, and multiple dma-ranges cells for PCIe as a consequence.
Each memory controller has a different physical address aperture in the CPU's physical address map (e.g.: MEMC0 is 0x0 - 0x3fff_ffff, MEMC1 0x4000_0000 - 0x7ffff_ffff and MEMC2 0x8000_0000 - 0xbfff_ffff, not counting the extension regions above 4GB), and while the CPU is scheduled and arbitrated the same way across all memory controllers (thus making it virtually UMA, almost) having a buffer span two memory controllers would be problematic because the memory controllers do not know how to guarantee the transaction ordering and buffer data consistency in both DRAM itself and for other memory controller clients, like PCIe.
We historically had to reserve the last 4KB of each memory controller to avoid problematic controllers like EHCI to prefetch beyond the end of a memory controller's populated memory and that also incidentally takes care of never having a buffer cross a controller boundary. Either you can allocate the entire buffer on a given memory controller, or you cannot allocate memory at all on that zone/region and another one must be found (or there is no more memory and there is a genuine OOM).
The way we reserve memory right now is based on the first patch submitted by Jim:
https://lore.kernel.org/patchwork/patch/988469/
whereby we read the memory node's "reg" property and we map the physical addresses to the memory controller configuration read from the specific registers in the CPU's Bus Interface Unit (where the memory controller apertures are architecturally defined) and then we use that to call memblock_reserve() (not part of that patch, it should be though).
On Wed, Sep 25, 2019 at 9:53 AM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote:
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Hi All, this series tries to address one of the issues blocking us from upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that devices not represented in DT which sit behind a PCI bus fail to get the bus' DMA addressing constraints.
This is due to the fact that of_dma_configure() assumes it's receiving a DT node representing the device being configured, as opposed to the PCIe bridge node we currently pass. This causes the code to directly jump into PCI's parent node when checking for 'dma-ranges' and misses whatever was set there.
To address this I create a new API in OF - inspired from Robin Murphys original proposal[2] - which accepts a bus DT node as it's input in order to configure a device's DMA constraints. The changes go deep into of/address.c's implementation, as a device being having a DT node assumption was pretty strong.
On top of this work, I also cleaned up of_dma_configure() removing its redundant arguments and creating an alternative function for the special cases not applicable to either the above case or the default usage.
IMO the resulting functions are more explicit. They will probably surface some hacky usages that can be properly fixed as I show with the DT fixes on the Layerscape platform.
This was also tested on a Raspberry Pi 4 with a custom PCIe driver and on a Seattle AMD board.
Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found.
That's good news, and yes now that I see it, some stuff on my series is overly complicated. Specially around of_translate_*().
On top of that, you removed in of_dma_get_range():
/** At least empty ranges has to be defined for parent node if* DMA is supported*/if (!ranges)break;Which I assumed was bound to the standard and makes things easier.
The standard is whatever we say it is and what exists in the wild...
Probably better for me to get the series posted for context, but the above is removed because we could be passing in the bus device/child node and checking for 'dma-ranges' rather than only the bus node. While this does mean 'dma-ranges' could be in a child node which is wrong, it simplifies the only caller of_dma_configure(). And really, there's no way to detect that error. Someone could call of_dma_configure(NULL, child, ...). Perhaps we could assert that 'ranges' is present whenever 'dma-ranges' is.
Back to the standard, I think it can be summarized as a device's immediate parent (a bus node) must contain 'dma-ranges'. All the parent nodes of the bus node should also have 'dma-ranges', but missing is treated as empty (1:1 translation). 'dma-ranges' missing in all the parent nodes is also treated as 1:1 translation and no addressing restrictions.
Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU.
I reviewed everything, I did find a minor issue, see the patch attached.
Also I tested your branch both on an RPi4, with a PCI device that depends on these changes and by comparing the OF debugs logs on a Layerscape board which uses dma-ranges, dma-coherent and IOMMU. All works as expected.
Will you send this series for v5.5? Please keep me in the loop, I'll review and test the final version.
Yes, sending it out soon.
Rob
dri-devel@lists.freedesktop.org
-
Florian Fainelli -
Nicolas Saenz Julienne -
Rob Herring -
Robin Murphy -
Shawn Guo