Hi, We've had for quite some time to hack around in our drivers to take into account the fact that our DMA accesses are not done through the parent node, but through another bus with a different mapping than the CPU for the RAM (0 instead of 0x40000000 for most SoCs). After some discussion after the submission of a camera device suffering of the same hacks, I've decided to put together a serie that introduce a special interconnect name called "dma" that that allows to express the DMA relationship between a master and its bus, even if they are not direct parents in the DT. Let me know what you think, Maxime Changes from v2: - Rewrite commit logs still mentionning dma-parent - Removed dma-parent-cells left over in the binding example - Removed dma-parent still being mentionned in comments Changes from v1: - Change to use the now merged interconnect bindings - Move the DMA parent retrieval logic to its own function - Rebase on top of 5.0 Maxime Ripard (7): dt-bindings: interconnect: Add a dma interconnect name dt-bindings: bus: Add binding for the Allwinner MBUS controller of: address: Add parent pointer to the __of_translate_address args of: address: Add support for the parent DMA bus drm/sun4i: Rely on dma interconnect for our RAM offset clk: sunxi-ng: sun5i: Export the MBUS clock ARM: dts: sun5i: Add the MBUS controller Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt | 36 +++++- Documentation/devicetree/bindings/interconnect/interconnect.txt | 3 +- arch/arm/boot/dts/sun5i.dtsi | 13 ++- drivers/clk/sunxi-ng/ccu-sun5i.h | 4 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++- drivers/of/address.c | 49 +++++-- include/dt-bindings/clock/sun5i-ccu.h | 2 +- 7 files changed, 114 insertions(+), 21 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt base-commit: 87e87c7b0eeb3c9e08cdfe28fd540247bdf31ef5 -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The current DT bindings assume that the DMA will be performed by the devices through their parent DT node, and rely on that assumption for the address translation using dma-ranges. However, some SoCs have devices that will perform DMA through another bus, with separate address translation rules. We therefore need to express that relationship, through the special interconnect name "dma". Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- Documentation/devicetree/bindings/interconnect/interconnect.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt index 5a3c575b387a..e69fc2d992c3 100644 --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path name strings sorted in the same interconnect-names to match interconnect paths with interconnect specifier pairs. + Reserved interconnect names: + * dma: Path from the device to the main memory of the system + Example: sdhci@7864000 { -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The MBUS controller drives the MBUS that other devices in the SoC will use to perform DMA. It also has a register interface that allows to monitor and control the bandwidth and priorities for masters on that bus. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt | 36 +++++++- 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt diff --git a/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt b/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt new file mode 100644 index 000000000000..e72b7ac9e359 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt @@ -0,0 +1,36 @@ +Allwinner Memory Bus (MBUS) controller + +The MBUS controller drives the MBUS that other devices in the SoC will +use to perform DMA. It also has a register interface that allows to +monitor and control the bandwidth and priorities for masters on that +bus. + +Required properties: + - compatible: Must be one of: + - allwinner,sun5i-a13-mbus + - reg: Offset and length of the register set for the controller + - clocks: phandle to the clock driving the controller + - dma-ranges: see booting-without-of.txt + - #interconnect-cells: Must be one, with the argument being the MBUS + port ID + +Each device having to perform their DMA through the MBUS must have the +interconnects and interconnect-names properties set to the MBUS +controller and with "dma" as the interconnect name. + +Example: + +mbus: dram-controller@1c01000 { + compatible = "allwinner,sun5i-a13-mbus"; + reg = <0x01c01000 0x1000>; + clocks = <&ccu CLK_MBUS>; + dma-ranges = <0x00000000 0x40000000 0x20000000>; + #interconnect-cells = <1>; +}; + +fe0: display-frontend@1e00000 { + compatible = "allwinner,sun5i-a13-display-frontend"; + ... + interconnects = <&mbus 19>; + interconnect-names = "dma"; +}; -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The __of_translate_address function is used to translate the device tree addresses to physical addresses using the various ranges property to create the offset. However, it's shared between the CPU addresses (based on the ranges property) and the DMA addresses (based on dma-ranges). Since we're going to add support for a DMA parent node that is not the DT parent node, we need to change the logic a bit to have an optional parent node that we should use. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/of/address.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/of/address.c b/drivers/of/address.c index 2270373b30ab..4c5dc21c71ca 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -569,10 +569,10 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus, * relative to that node. */ static u64 __of_translate_address(struct device_node *dev, + struct device_node *parent, const __be32 *in_addr, const char *rprop, struct device_node **host) { - struct device_node *parent = NULL; struct of_bus *bus, *pbus; __be32 addr[OF_MAX_ADDR_CELLS]; int na, ns, pna, pns; @@ -583,11 +583,14 @@ static u64 __of_translate_address(struct device_node *dev, /* Increase refcount at current level */ of_node_get(dev); - *host = NULL; - /* Get parent & match bus type */ - parent = of_get_parent(dev); - if (parent == NULL) - goto bail; + if (!parent) { + *host = NULL; + /* Get parent & match bus type */ + parent = of_get_parent(dev); + if (parent == NULL) + goto bail; + } + bus = of_match_bus(parent); /* Count address cells & copy address locally */ @@ -665,7 +668,7 @@ u64 of_translate_address(struct device_node *dev, const __be32 *in_addr) struct device_node *host; u64 ret; - ret = __of_translate_address(dev, in_addr, "ranges", &host); + ret = __of_translate_address(dev, NULL, in_addr, "ranges", &host); if (host) { of_node_put(host); return OF_BAD_ADDR; @@ -680,7 +683,7 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) struct device_node *host; u64 ret; - ret = __of_translate_address(dev, in_addr, "dma-ranges", &host); + ret = __of_translate_address(dev, NULL, in_addr, "dma-ranges", &host); if (host) { of_node_put(host); @@ -736,7 +739,7 @@ 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, in_addr, "ranges", &host); + taddr = __of_translate_address(dev, NULL, in_addr, "ranges", &host); if (host) { /* host-specific port access */ port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size); -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Some SoCs have devices that are using a separate bus from the main bus to perform DMA. These buses might have some restrictions and/or different mapping than from the CPU side, so we'd need to express those using the usual dma-ranges, but using a different DT node than the node's parent. Now that the generic interconnect bindings are available, we can model an interconnect with the reserved name "dma" for those use-cases. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/of/address.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/drivers/of/address.c b/drivers/of/address.c index 4c5dc21c71ca..0e9d87a664f5 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -583,8 +583,8 @@ static u64 __of_translate_address(struct device_node *dev, /* Increase refcount at current level */ of_node_get(dev); + *host = NULL; if (!parent) { - *host = NULL; /* Get parent & match bus type */ parent = of_get_parent(dev); if (parent == NULL) @@ -678,12 +678,34 @@ 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(struct device_node *np) +{ + struct of_phandle_args args; + unsigned int index; + int ret; + + ret = of_property_match_string(np, "interconnect-names", "dma"); + if (ret < 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); +} + u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) { + struct device_node *parent; struct device_node *host; u64 ret; - ret = __of_translate_address(dev, NULL, in_addr, "dma-ranges", &host); + parent = __of_get_dma_parent(dev); + ret = __of_translate_address(dev, parent, in_addr, "dma-ranges", &host); + of_node_put(parent); if (host) { of_node_put(host); @@ -911,9 +933,15 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz return -EINVAL; while (1) { + struct device_node *parent; + naddr = of_n_addr_cells(node); nsize = of_n_size_cells(node); - node = of_get_next_parent(node); + + parent = __of_get_dma_parent(node); + of_node_put(node); + + node = parent; if (!node) break; -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Now that we can express our DMA topology, rely on those property instead of hardcoding an offset from the dma_addr_t which wasn't really great. We still need to add some code to deal with the old DT that would lack that property, but we move the offset to the DRM device dma_pfn_offset to be able to rely on just the dma_addr_t associated to the GEM object. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 9e9255ee59cd..1846a1b30fea 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, paddr = drm_fb_cma_get_gem_addr(fb, state, 0); DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); - /* - * backend DMA accesses DRAM directly, bypassing the system - * bus. As such, the address range is different and the buffer - * address needs to be corrected. - */ - paddr -= PHYS_OFFSET; - if (fb->format->is_yuv) return sun4i_backend_update_yuv_buffer(backend, fb, paddr); @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, dev_set_drvdata(dev, backend); spin_lock_init(&backend->frontend_lock); + if (of_find_property(dev->of_node, "interconnects", NULL)) { + /* + * This assume we have the same DMA constraints for all our the + * devices in our pipeline (all the backends, but also the + * frontends). This sounds bad, but it has always been the case + * 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); + if (ret) + return ret; + } else { + /* + * If we don't have the interconnect property, most likely + * because of an old DT, we need to set the DMA offset by hand + * on our device since the RAM mapping is at 0 for the DMA bus, + * unlike the CPU. + */ + drm->dev->dma_pfn_offset = PHYS_PFN_OFFSET; + } + backend->engine.node = dev->of_node; backend->engine.ops = &sun4i_backend_engine_ops; backend->engine.id = sun4i_backend_of_get_id(dev->of_node); -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The MBUS clock is used by the MBUS controller, so let's export it so that we can use it in our DT node. Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- drivers/clk/sunxi-ng/ccu-sun5i.h | 4 ---- include/dt-bindings/clock/sun5i-ccu.h | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/clk/sunxi-ng/ccu-sun5i.h b/drivers/clk/sunxi-ng/ccu-sun5i.h index 93a275fbd9a9..b66abd4fd0bf 100644 --- a/drivers/clk/sunxi-ng/ccu-sun5i.h +++ b/drivers/clk/sunxi-ng/ccu-sun5i.h @@ -60,10 +60,6 @@ /* The rest of the module clocks are exported */ -#define CLK_MBUS 99 - -/* And finally the IEP clock */ - #define CLK_NUMBER (CLK_IEP + 1) #endif /* _CCU_SUN5I_H_ */ diff --git a/include/dt-bindings/clock/sun5i-ccu.h b/include/dt-bindings/clock/sun5i-ccu.h index 81f34d477aeb..2e6b9ddcc24e 100644 --- a/include/dt-bindings/clock/sun5i-ccu.h +++ b/include/dt-bindings/clock/sun5i-ccu.h @@ -100,7 +100,7 @@ #define CLK_AVS 96 #define CLK_HDMI 97 #define CLK_GPU 98 - +#define CLK_MBUS 99 #define CLK_IEP 100 #endif /* _DT_BINDINGS_CLK_SUN5I_H_ */ -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The MBUS (and its associated controller) is the bus in the Allwinner SoCs that DMA devices use in the system to access the memory. Among other things (and depending on the SoC generation), it can also enforce priorities or report bandwidth usages on a per-master basis. One of the most notable thing is that instead of having the same mapping for the RAM than the CPU, it maps it at address 0, which means we'll have to do address translation thanks to the dma-ranges property. Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> --- arch/arm/boot/dts/sun5i.dtsi | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi index 5497d985c54a..a29203b7661d 100644 --- a/arch/arm/boot/dts/sun5i.dtsi +++ b/arch/arm/boot/dts/sun5i.dtsi @@ -127,6 +127,7 @@ compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; + dma-ranges; ranges; system-control@1c00000 { @@ -181,6 +182,14 @@ }; }; + mbus: dram-controller@1c01000 { + compatible = "allwinner,sun5i-a13-mbus"; + reg = <0x01c01000 0x1000>; + clocks = <&ccu CLK_MBUS>; + dma-ranges = <0x00000000 0x40000000 0x20000000>; + #interconnect-cells = <1>; + }; + dma: dma-controller@1c02000 { compatible = "allwinner,sun4i-a10-dma"; reg = <0x01c02000 0x1000>; @@ -727,6 +736,8 @@ clock-names = "ahb", "mod", "ram"; resets = <&ccu RST_DE_FE>; + interconnects = <&mbus 19>; + interconnect-names = "dma"; status = "disabled"; ports { @@ -755,6 +766,8 @@ clock-names = "ahb", "mod", "ram"; resets = <&ccu RST_DE_BE>; + interconnects = <&mbus 18>; + interconnect-names = "dma"; status = "disabled"; assigned-clocks = <&ccu CLK_DE_BE>; -- git-series 0.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/02/2019 15:02, Maxime Ripard wrote: > The __of_translate_address function is used to translate the device tree > addresses to physical addresses using the various ranges property to create > the offset. > > However, it's shared between the CPU addresses (based on the ranges > property) and the DMA addresses (based on dma-ranges). Since we're going to > add support for a DMA parent node that is not the DT parent node, we need > to change the logic a bit to have an optional parent node that we should > use. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/of/address.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 2270373b30ab..4c5dc21c71ca 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -569,10 +569,10 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus, > * relative to that node. > */ > static u64 __of_translate_address(struct device_node *dev, > + struct device_node *parent, > const __be32 *in_addr, const char *rprop, > struct device_node **host) > { > - struct device_node *parent = NULL; > struct of_bus *bus, *pbus; > __be32 addr[OF_MAX_ADDR_CELLS]; > int na, ns, pna, pns; > @@ -583,11 +583,14 @@ static u64 __of_translate_address(struct device_node *dev, > /* Increase refcount at current level */ > of_node_get(dev); > > - *host = NULL; > - /* Get parent & match bus type */ > - parent = of_get_parent(dev); > - if (parent == NULL) > - goto bail; > + if (!parent) { I would have suggested having the callers make the of_get_parent() call themselves, but having pulled up the code for a look at the whole function, there's a bigger issue at play. The parent traversal happens within the main translation loop as well, and topologies could quite feasibly exist where that makes a difference, e.g.: icc { #interconnect-cells = <0>; dma-ranges = <...>; }; subsystem { interconnects = <&icc>; interconnect-names = "dma"; intermediate-bus { dma-ranges; device { ... }; }; }; or: icc2 { #interconnect-cells = <0>; dma-ranges = <...>; }; icc1 { #interconnect-cells = <0>; dma-ranges = <...>; interconnects = <&icc2>; interconnect-names = "dma"; }; device { interconnects = <&icc1>; interconnect-names = "dma"; ... }; So I guess the answer is to switch this over to using a callback and have the callers pass of_get_parent/of_get_dma_parent in as appropriate. Robin. > + *host = NULL; > + /* Get parent & match bus type */ > + parent = of_get_parent(dev); > + if (parent == NULL) > + goto bail; > + } > + > bus = of_match_bus(parent); > > /* Count address cells & copy address locally */ > @@ -665,7 +668,7 @@ u64 of_translate_address(struct device_node *dev, const __be32 *in_addr) > struct device_node *host; > u64 ret; > > - ret = __of_translate_address(dev, in_addr, "ranges", &host); > + ret = __of_translate_address(dev, NULL, in_addr, "ranges", &host); > if (host) { > of_node_put(host); > return OF_BAD_ADDR; > @@ -680,7 +683,7 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) > struct device_node *host; > u64 ret; > > - ret = __of_translate_address(dev, in_addr, "dma-ranges", &host); > + ret = __of_translate_address(dev, NULL, in_addr, "dma-ranges", &host); > > if (host) { > of_node_put(host); > @@ -736,7 +739,7 @@ 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, in_addr, "ranges", &host); > + taddr = __of_translate_address(dev, NULL, in_addr, "ranges", &host); > if (host) { > /* host-specific port access */ > port = logic_pio_trans_hwaddr(&host->fwnode, taddr, size); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/02/2019 15:02, Maxime Ripard wrote: > Some SoCs have devices that are using a separate bus from the main bus to > perform DMA. > > These buses might have some restrictions and/or different mapping than from > the CPU side, so we'd need to express those using the usual dma-ranges, but > using a different DT node than the node's parent. > > Now that the generic interconnect bindings are available, we can model an > interconnect with the reserved name "dma" for those use-cases. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/of/address.c | 34 +++++++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 4c5dc21c71ca..0e9d87a664f5 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -583,8 +583,8 @@ static u64 __of_translate_address(struct device_node *dev, > /* Increase refcount at current level */ > of_node_get(dev); > > + *host = NULL; > if (!parent) { > - *host = NULL; > /* Get parent & match bus type */ > parent = of_get_parent(dev); > if (parent == NULL) > @@ -678,12 +678,34 @@ 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(struct device_node *np) > +{ > + struct of_phandle_args args; > + unsigned int index; > + int ret; > + > + ret = of_property_match_string(np, "interconnect-names", "dma"); > + if (ret < 0) > + return of_get_parent(np); Don't you need to propagate ret to index here? Robin. > + > + 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); > +} > + > u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr) > { > + struct device_node *parent; > struct device_node *host; > u64 ret; > > - ret = __of_translate_address(dev, NULL, in_addr, "dma-ranges", &host); > + parent = __of_get_dma_parent(dev); > + ret = __of_translate_address(dev, parent, in_addr, "dma-ranges", &host); > + of_node_put(parent); > > if (host) { > of_node_put(host); > @@ -911,9 +933,15 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz > return -EINVAL; > > while (1) { > + struct device_node *parent; > + > naddr = of_n_addr_cells(node); > nsize = of_n_size_cells(node); > - node = of_get_next_parent(node); > + > + parent = __of_get_dma_parent(node); > + of_node_put(node); > + > + node = parent; > if (!node) > break; > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/02/2019 15:02, Maxime Ripard wrote: > Now that we can express our DMA topology, rely on those property instead of > hardcoding an offset from the dma_addr_t which wasn't really great. > > We still need to add some code to deal with the old DT that would lack that > property, but we move the offset to the DRM device dma_pfn_offset to be > able to rely on just the dma_addr_t associated to the GEM object. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > index 9e9255ee59cd..1846a1b30fea 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > paddr = drm_fb_cma_get_gem_addr(fb, state, 0); > DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); > > - /* > - * backend DMA accesses DRAM directly, bypassing the system > - * bus. As such, the address range is different and the buffer > - * address needs to be corrected. > - */ > - paddr -= PHYS_OFFSET; > - > if (fb->format->is_yuv) > return sun4i_backend_update_yuv_buffer(backend, fb, paddr); > > @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > dev_set_drvdata(dev, backend); > spin_lock_init(&backend->frontend_lock); > > + if (of_find_property(dev->of_node, "interconnects", NULL)) { > + /* > + * This assume we have the same DMA constraints for all our the > + * devices in our pipeline (all the backends, but also the > + * frontends). This sounds bad, but it has always been the case > + * 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); It would be even nicer if we could ensure that drm->dev originates from a DT node which has the appropriate interconnects property itself, such that we can assume it's already configured correctly. Robin. > + if (ret) > + return ret; > + } else { > + /* > + * If we don't have the interconnect property, most likely > + * because of an old DT, we need to set the DMA offset by hand > + * on our device since the RAM mapping is at 0 for the DMA bus, > + * unlike the CPU. > + */ > + drm->dev->dma_pfn_offset = PHYS_PFN_OFFSET; > + } > + > backend->engine.node = dev->of_node; > backend->engine.ops = &sun4i_backend_engine_ops; > backend->engine.id = sun4i_backend_of_get_id(dev->of_node); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/02/2019 15:02, Maxime Ripard wrote: > The MBUS controller drives the MBUS that other devices in the SoC will > use to perform DMA. It also has a register interface that allows to > monitor and control the bandwidth and priorities for masters on that > bus. > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt | 36 +++++++- > 1 file changed, 36 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt > > diff --git a/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt b/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt > new file mode 100644 > index 000000000000..e72b7ac9e359 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/sunxi/sunxi-mbus.txt > @@ -0,0 +1,36 @@ > +Allwinner Memory Bus (MBUS) controller > + > +The MBUS controller drives the MBUS that other devices in the SoC will > +use to perform DMA. It also has a register interface that allows to > +monitor and control the bandwidth and priorities for masters on that > +bus. > + > +Required properties: > + - compatible: Must be one of: > + - allwinner,sun5i-a13-mbus > + - reg: Offset and length of the register set for the controller > + - clocks: phandle to the clock driving the controller > + - dma-ranges: see booting-without-of.txt Nit: this is a standard property in DTSpec, so it's probably better to refer to that rather than Linux-specific documentation. Robin. > + - #interconnect-cells: Must be one, with the argument being the MBUS > + port ID > + > +Each device having to perform their DMA through the MBUS must have the > +interconnects and interconnect-names properties set to the MBUS > +controller and with "dma" as the interconnect name. > + > +Example: > + > +mbus: dram-controller@1c01000 { > + compatible = "allwinner,sun5i-a13-mbus"; > + reg = <0x01c01000 0x1000>; > + clocks = <&ccu CLK_MBUS>; > + dma-ranges = <0x00000000 0x40000000 0x20000000>; > + #interconnect-cells = <1>; > +}; > + > +fe0: display-frontend@1e00000 { > + compatible = "allwinner,sun5i-a13-display-frontend"; > + ... > + interconnects = <&mbus 19>; > + interconnect-names = "dma"; > +}; > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2769 bytes --] Hi Robin, Thanks for your feedback! On Tue, Feb 12, 2019 at 06:46:40PM +0000, Robin Murphy wrote: > On 11/02/2019 15:02, Maxime Ripard wrote: > > Now that we can express our DMA topology, rely on those property instead of > > hardcoding an offset from the dma_addr_t which wasn't really great. > > > > We still need to add some code to deal with the old DT that would lack that > > property, but we move the offset to the DRM device dma_pfn_offset to be > > able to rely on just the dma_addr_t associated to the GEM object. > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > > index 9e9255ee59cd..1846a1b30fea 100644 > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > > @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > > paddr = drm_fb_cma_get_gem_addr(fb, state, 0); > > DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); > > - /* > > - * backend DMA accesses DRAM directly, bypassing the system > > - * bus. As such, the address range is different and the buffer > > - * address needs to be corrected. > > - */ > > - paddr -= PHYS_OFFSET; > > - > > if (fb->format->is_yuv) > > return sun4i_backend_update_yuv_buffer(backend, fb, paddr); > > @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > > dev_set_drvdata(dev, backend); > > spin_lock_init(&backend->frontend_lock); > > + if (of_find_property(dev->of_node, "interconnects", NULL)) { > > + /* > > + * This assume we have the same DMA constraints for all our the > > + * devices in our pipeline (all the backends, but also the > > + * frontends). This sounds bad, but it has always been the case > > + * 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); > > It would be even nicer if we could ensure that drm->dev originates from a DT > node which has the appropriate interconnects property itself, such that we > can assume it's already configured correctly. The thing is drm->dev comes from a node in the DT that is a virtual node, and therefore doesn't have any resources attached, so I'm not sure we have any other way, unfortunately. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 13/02/2019 15:41, Maxime Ripard wrote: > Hi Robin, > > Thanks for your feedback! > > On Tue, Feb 12, 2019 at 06:46:40PM +0000, Robin Murphy wrote: >> On 11/02/2019 15:02, Maxime Ripard wrote: >>> Now that we can express our DMA topology, rely on those property instead of >>> hardcoding an offset from the dma_addr_t which wasn't really great. >>> >>> We still need to add some code to deal with the old DT that would lack that >>> property, but we move the offset to the DRM device dma_pfn_offset to be >>> able to rely on just the dma_addr_t associated to the GEM object. >>> >>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>> --- >>> drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- >>> 1 file changed, 21 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c >>> index 9e9255ee59cd..1846a1b30fea 100644 >>> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c >>> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c >>> @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, >>> paddr = drm_fb_cma_get_gem_addr(fb, state, 0); >>> DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); >>> - /* >>> - * backend DMA accesses DRAM directly, bypassing the system >>> - * bus. As such, the address range is different and the buffer >>> - * address needs to be corrected. >>> - */ >>> - paddr -= PHYS_OFFSET; >>> - >>> if (fb->format->is_yuv) >>> return sun4i_backend_update_yuv_buffer(backend, fb, paddr); >>> @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, >>> dev_set_drvdata(dev, backend); >>> spin_lock_init(&backend->frontend_lock); >>> + if (of_find_property(dev->of_node, "interconnects", NULL)) { >>> + /* >>> + * This assume we have the same DMA constraints for all our the >>> + * devices in our pipeline (all the backends, but also the >>> + * frontends). This sounds bad, but it has always been the case >>> + * 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); >> >> It would be even nicer if we could ensure that drm->dev originates from a DT >> node which has the appropriate interconnects property itself, such that we >> can assume it's already configured correctly. > > The thing is drm->dev comes from a node in the DT that is a virtual > node, and therefore doesn't have any resources attached, so I'm not > sure we have any other way, unfortunately. Right, I appreciate that it may not be feasible to swizzle drm->dev for one of the 'real' component devices, but what I was also thinking was that since the virtual device node effectively represents the aggregation of the other component devices, we could just say that it also has to have its own link to the MBUS interconnect (with the ID of the pipeline entrypoint it's associated with, I guess). That ought to be enough to get dma_configure() to do the job, and in fairness is no *less* accurate a description of the hardware, even if might look a little funky to some. Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 4543 bytes --] Hi Robin, On Wed, Feb 13, 2019 at 04:40:11PM +0000, Robin Murphy wrote: > On 13/02/2019 15:41, Maxime Ripard wrote: > > Hi Robin, > > > > Thanks for your feedback! > > > > On Tue, Feb 12, 2019 at 06:46:40PM +0000, Robin Murphy wrote: > > > On 11/02/2019 15:02, Maxime Ripard wrote: > > > > Now that we can express our DMA topology, rely on those property instead of > > > > hardcoding an offset from the dma_addr_t which wasn't really great. > > > > > > > > We still need to add some code to deal with the old DT that would lack that > > > > property, but we move the offset to the DRM device dma_pfn_offset to be > > > > able to rely on just the dma_addr_t associated to the GEM object. > > > > > > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > --- > > > > drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- > > > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > > > > index 9e9255ee59cd..1846a1b30fea 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > > > > @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > > > > paddr = drm_fb_cma_get_gem_addr(fb, state, 0); > > > > DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); > > > > - /* > > > > - * backend DMA accesses DRAM directly, bypassing the system > > > > - * bus. As such, the address range is different and the buffer > > > > - * address needs to be corrected. > > > > - */ > > > > - paddr -= PHYS_OFFSET; > > > > - > > > > if (fb->format->is_yuv) > > > > return sun4i_backend_update_yuv_buffer(backend, fb, paddr); > > > > @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > > > > dev_set_drvdata(dev, backend); > > > > spin_lock_init(&backend->frontend_lock); > > > > + if (of_find_property(dev->of_node, "interconnects", NULL)) { > > > > + /* > > > > + * This assume we have the same DMA constraints for all our the > > > > + * devices in our pipeline (all the backends, but also the > > > > + * frontends). This sounds bad, but it has always been the case > > > > + * 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); > > > > > > It would be even nicer if we could ensure that drm->dev originates from a DT > > > node which has the appropriate interconnects property itself, such that we > > > can assume it's already configured correctly. > > > > The thing is drm->dev comes from a node in the DT that is a virtual > > node, and therefore doesn't have any resources attached, so I'm not > > sure we have any other way, unfortunately. > > Right, I appreciate that it may not be feasible to swizzle drm->dev for one > of the 'real' component devices, but what I was also thinking was that since > the virtual device node effectively represents the aggregation of the other > component devices, we could just say that it also has to have its own link > to the MBUS interconnect (with the ID of the pipeline entrypoint it's > associated with, I guess). That ought to be enough to get dma_configure() to > do the job, and in fairness is no *less* accurate a description of the > hardware, even if might look a little funky to some. That virtual device however can work with up to 4 devices that can perform DMA operations, all of them going through a different port of the MBUS controller that can account for DMA usage on a per-master basis. Eventually, we should support that feature as well, but the point is that from a DT point of view, there's not a single point of DMA access for that particular device but more likely 2-4 points, each with a different argument to the phandle. We could of course have a hack and use only one of them, but that would be less accurate than what we currently have. Plus, this is really due to a restriction on the DRM side, that could change in the future (even though it's unlikely). Fixing that restriction would make that property completely useless, confusing and wrong from an hardware PoV. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Maxime, On 2/11/19 17:02, Maxime Ripard wrote: > The current DT bindings assume that the DMA will be performed by the > devices through their parent DT node, and rely on that assumption for the > address translation using dma-ranges. > > However, some SoCs have devices that will perform DMA through another bus, > with separate address translation rules. We therefore need to express that > relationship, through the special interconnect name "dma". > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > Documentation/devicetree/bindings/interconnect/interconnect.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt > index 5a3c575b387a..e69fc2d992c3 100644 > --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path name strings sorted in the same > interconnect-names to match interconnect paths with interconnect > specifier pairs. > > + Reserved interconnect names: > + * dma: Path from the device to the main memory of the system Bikeshed: As it's from the device to the main memory, maybe here we can also denote this my calling the path dma-mem or dma-memory. For other paths, we are trying to mention both the source and the destination and maybe it would be good to be consistent although this is special one. Thanks, Georgi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1979 bytes --] Hi, On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: > On 2/11/19 17:02, Maxime Ripard wrote: > > The current DT bindings assume that the DMA will be performed by the > > devices through their parent DT node, and rely on that assumption for the > > address translation using dma-ranges. > > > > However, some SoCs have devices that will perform DMA through another bus, > > with separate address translation rules. We therefore need to express that > > relationship, through the special interconnect name "dma". > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > --- > > Documentation/devicetree/bindings/interconnect/interconnect.txt | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > index 5a3c575b387a..e69fc2d992c3 100644 > > --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path name strings sorted in the same > > interconnect-names to match interconnect paths with interconnect > > specifier pairs. > > > > + Reserved interconnect names: > > + * dma: Path from the device to the main memory of the system > > Bikeshed: As it's from the device to the main memory, maybe here we can > also denote this my calling the path dma-mem or dma-memory. For other > paths, we are trying to mention both the source and the destination and > maybe it would be good to be consistent although this is special one. I'm not sure I understand what you mean. You'd like two interconnect names, one called dma-memory, and one memory-dma? Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 19/02/2019 10:55, Maxime Ripard wrote: > Hi Robin, > > On Wed, Feb 13, 2019 at 04:40:11PM +0000, Robin Murphy wrote: >> On 13/02/2019 15:41, Maxime Ripard wrote: >>> Hi Robin, >>> >>> Thanks for your feedback! >>> >>> On Tue, Feb 12, 2019 at 06:46:40PM +0000, Robin Murphy wrote: >>>> On 11/02/2019 15:02, Maxime Ripard wrote: >>>>> Now that we can express our DMA topology, rely on those property instead of >>>>> hardcoding an offset from the dma_addr_t which wasn't really great. >>>>> >>>>> We still need to add some code to deal with the old DT that would lack that >>>>> property, but we move the offset to the DRM device dma_pfn_offset to be >>>>> able to rely on just the dma_addr_t associated to the GEM object. >>>>> >>>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>>>> --- >>>>> drivers/gpu/drm/sun4i/sun4i_backend.c | 28 +++++++++++++++++++++------- >>>>> 1 file changed, 21 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c >>>>> index 9e9255ee59cd..1846a1b30fea 100644 >>>>> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c >>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c >>>>> @@ -383,13 +383,6 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, >>>>> paddr = drm_fb_cma_get_gem_addr(fb, state, 0); >>>>> DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr); >>>>> - /* >>>>> - * backend DMA accesses DRAM directly, bypassing the system >>>>> - * bus. As such, the address range is different and the buffer >>>>> - * address needs to be corrected. >>>>> - */ >>>>> - paddr -= PHYS_OFFSET; >>>>> - >>>>> if (fb->format->is_yuv) >>>>> return sun4i_backend_update_yuv_buffer(backend, fb, paddr); >>>>> @@ -835,6 +828,27 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, >>>>> dev_set_drvdata(dev, backend); >>>>> spin_lock_init(&backend->frontend_lock); >>>>> + if (of_find_property(dev->of_node, "interconnects", NULL)) { >>>>> + /* >>>>> + * This assume we have the same DMA constraints for all our the >>>>> + * devices in our pipeline (all the backends, but also the >>>>> + * frontends). This sounds bad, but it has always been the case >>>>> + * 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); >>>> >>>> It would be even nicer if we could ensure that drm->dev originates from a DT >>>> node which has the appropriate interconnects property itself, such that we >>>> can assume it's already configured correctly. >>> >>> The thing is drm->dev comes from a node in the DT that is a virtual >>> node, and therefore doesn't have any resources attached, so I'm not >>> sure we have any other way, unfortunately. >> >> Right, I appreciate that it may not be feasible to swizzle drm->dev for one >> of the 'real' component devices, but what I was also thinking was that since >> the virtual device node effectively represents the aggregation of the other >> component devices, we could just say that it also has to have its own link >> to the MBUS interconnect (with the ID of the pipeline entrypoint it's >> associated with, I guess). That ought to be enough to get dma_configure() to >> do the job, and in fairness is no *less* accurate a description of the >> hardware, even if might look a little funky to some. > > That virtual device however can work with up to 4 devices that can > perform DMA operations, all of them going through a different port of > the MBUS controller that can account for DMA usage on a per-master > basis. > > Eventually, we should support that feature as well, but the point is > that from a DT point of view, there's not a single point of DMA access > for that particular device but more likely 2-4 points, each with a > different argument to the phandle. > > We could of course have a hack and use only one of them, but that > would be less accurate than what we currently have. Plus, this is > really due to a restriction on the DRM side, that could change in the > future (even though it's unlikely). Fixing that restriction would make > that property completely useless, confusing and wrong from an hardware > PoV. Thanks for the clarification. Much as I'd like to keep of_dma_configure() out of drivers as much as possible, I agree that it's not sufficient justification for (ab)using DT to sidestep a Linux-specific issue which might eventually get properly fixed anyway. I'm sure the last thing sunxi needs is any more awkward DT ABI concerns :) Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 05/03/2019 15:53, Maxime Ripard wrote: > Hi, > > On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: >> On 2/11/19 17:02, Maxime Ripard wrote: >>> The current DT bindings assume that the DMA will be performed by the >>> devices through their parent DT node, and rely on that assumption for the >>> address translation using dma-ranges. >>> >>> However, some SoCs have devices that will perform DMA through another bus, >>> with separate address translation rules. We therefore need to express that >>> relationship, through the special interconnect name "dma". >>> >>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>> --- >>> Documentation/devicetree/bindings/interconnect/interconnect.txt | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>> index 5a3c575b387a..e69fc2d992c3 100644 >>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt >>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path name strings sorted in the same >>> interconnect-names to match interconnect paths with interconnect >>> specifier pairs. >>> >>> + Reserved interconnect names: >>> + * dma: Path from the device to the main memory of the system >> >> Bikeshed: As it's from the device to the main memory, maybe here we can >> also denote this my calling the path dma-mem or dma-memory. For other >> paths, we are trying to mention both the source and the destination and >> maybe it would be good to be consistent although this is special one. > > I'm not sure I understand what you mean. You'd like two interconnect > names, one called dma-memory, and one memory-dma? Hmm, yes, it's not like "dma" describes an actual source or destination either :/ Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On 3/5/19 18:14, Robin Murphy wrote: > On 05/03/2019 15:53, Maxime Ripard wrote: >> Hi, >> >> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: >>> On 2/11/19 17:02, Maxime Ripard wrote: >>>> The current DT bindings assume that the DMA will be performed by the >>>> devices through their parent DT node, and rely on that assumption >>>> for the >>>> address translation using dma-ranges. >>>> >>>> However, some SoCs have devices that will perform DMA through >>>> another bus, >>>> with separate address translation rules. We therefore need to >>>> express that >>>> relationship, through the special interconnect name "dma". >>>> >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>>> --- >>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | >>>> 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>> index 5a3c575b387a..e69fc2d992c3 100644 >>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path >>>> name strings sorted in the same >>>> interconnect-names to match interconnect paths with >>>> interconnect >>>> specifier pairs. >>>> + Reserved interconnect names: >>>> + * dma: Path from the device to the main >>>> memory of the system >>> >>> Bikeshed: As it's from the device to the main memory, maybe here we can >>> also denote this my calling the path dma-mem or dma-memory. For other >>> paths, we are trying to mention both the source and the destination and >>> maybe it would be good to be consistent although this is special one. >> >> I'm not sure I understand what you mean. You'd like two interconnect >> names, one called dma-memory, and one memory-dma? > > Hmm, yes, it's not like "dma" describes an actual source or destination > either :/ IIUC, it's a path (bus) that a dma device use to access some memory (read or/and write). So i have used source-destination above more in the sense of initiator-target or master-slave. My suggestion was just to change the reserved interconnect name from "dma" to "dma-mem" or "dma-memory". Thanks, Georgi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2807 bytes --] On Thu, Mar 07, 2019 at 05:15:20PM +0200, Georgi Djakov wrote: > Hi, > > On 3/5/19 18:14, Robin Murphy wrote: > > On 05/03/2019 15:53, Maxime Ripard wrote: > >> Hi, > >> > >> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: > >>> On 2/11/19 17:02, Maxime Ripard wrote: > >>>> The current DT bindings assume that the DMA will be performed by the > >>>> devices through their parent DT node, and rely on that assumption > >>>> for the > >>>> address translation using dma-ranges. > >>>> > >>>> However, some SoCs have devices that will perform DMA through > >>>> another bus, > >>>> with separate address translation rules. We therefore need to > >>>> express that > >>>> relationship, through the special interconnect name "dma". > >>>> > >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > >>>> --- > >>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | > >>>> 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git > >>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt > >>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt > >>>> index 5a3c575b387a..e69fc2d992c3 100644 > >>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > >>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > >>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path > >>>> name strings sorted in the same > >>>> interconnect-names to match interconnect paths with > >>>> interconnect > >>>> specifier pairs. > >>>> + Reserved interconnect names: > >>>> + * dma: Path from the device to the main > >>>> memory of the system > >>> > >>> Bikeshed: As it's from the device to the main memory, maybe here we can > >>> also denote this my calling the path dma-mem or dma-memory. For other > >>> paths, we are trying to mention both the source and the destination and > >>> maybe it would be good to be consistent although this is special one. > >> > >> I'm not sure I understand what you mean. You'd like two interconnect > >> names, one called dma-memory, and one memory-dma? > > > > Hmm, yes, it's not like "dma" describes an actual source or destination > > either :/ > > IIUC, it's a path (bus) that a dma device use to access some memory > (read or/and write). So i have used source-destination above more in the > sense of initiator-target or master-slave. My suggestion was just to > change the reserved interconnect name from "dma" to "dma-mem" or > "dma-memory". If dma is an issue in itself, maybe we can call it "device-memory" ? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Mar 7, 2019 at 11:48 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Thu, Mar 07, 2019 at 05:15:20PM +0200, Georgi Djakov wrote: > > Hi, > > > > On 3/5/19 18:14, Robin Murphy wrote: > > > On 05/03/2019 15:53, Maxime Ripard wrote: > > >> Hi, > > >> > > >> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: > > >>> On 2/11/19 17:02, Maxime Ripard wrote: > > >>>> The current DT bindings assume that the DMA will be performed by the > > >>>> devices through their parent DT node, and rely on that assumption > > >>>> for the > > >>>> address translation using dma-ranges. > > >>>> > > >>>> However, some SoCs have devices that will perform DMA through > > >>>> another bus, > > >>>> with separate address translation rules. We therefore need to > > >>>> express that > > >>>> relationship, through the special interconnect name "dma". > > >>>> > > >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > >>>> --- > > >>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | > > >>>> 3 +++ > > >>>> 1 file changed, 3 insertions(+) > > >>>> > > >>>> diff --git > > >>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > >>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > >>>> index 5a3c575b387a..e69fc2d992c3 100644 > > >>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > >>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > >>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path > > >>>> name strings sorted in the same > > >>>> interconnect-names to match interconnect paths with > > >>>> interconnect > > >>>> specifier pairs. > > >>>> + Reserved interconnect names: > > >>>> + * dma: Path from the device to the main > > >>>> memory of the system > > >>> > > >>> Bikeshed: As it's from the device to the main memory, maybe here we can > > >>> also denote this my calling the path dma-mem or dma-memory. For other > > >>> paths, we are trying to mention both the source and the destination and > > >>> maybe it would be good to be consistent although this is special one. > > >> > > >> I'm not sure I understand what you mean. You'd like two interconnect > > >> names, one called dma-memory, and one memory-dma? > > > > > > Hmm, yes, it's not like "dma" describes an actual source or destination > > > either :/ > > > > IIUC, it's a path (bus) that a dma device use to access some memory > > (read or/and write). So i have used source-destination above more in the > > sense of initiator-target or master-slave. My suggestion was just to > > change the reserved interconnect name from "dma" to "dma-mem" or > > "dma-memory". > > If dma is an issue in itself, maybe we can call it "device-memory" ? Might I ask what happens if the device can both DMA to and from memory? IIRC the display frontends, backends, and mixers all have writeback capability, using the same interconnect port. ChenYu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3626 bytes --] On Fri, Mar 08, 2019 at 12:09:47AM +0800, Chen-Yu Tsai wrote: > On Thu, Mar 7, 2019 at 11:48 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Thu, Mar 07, 2019 at 05:15:20PM +0200, Georgi Djakov wrote: > > > Hi, > > > > > > On 3/5/19 18:14, Robin Murphy wrote: > > > > On 05/03/2019 15:53, Maxime Ripard wrote: > > > >> Hi, > > > >> > > > >> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: > > > >>> On 2/11/19 17:02, Maxime Ripard wrote: > > > >>>> The current DT bindings assume that the DMA will be performed by the > > > >>>> devices through their parent DT node, and rely on that assumption > > > >>>> for the > > > >>>> address translation using dma-ranges. > > > >>>> > > > >>>> However, some SoCs have devices that will perform DMA through > > > >>>> another bus, > > > >>>> with separate address translation rules. We therefore need to > > > >>>> express that > > > >>>> relationship, through the special interconnect name "dma". > > > >>>> > > > >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > >>>> --- > > > >>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | > > > >>>> 3 +++ > > > >>>> 1 file changed, 3 insertions(+) > > > >>>> > > > >>>> diff --git > > > >>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > >>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > >>>> index 5a3c575b387a..e69fc2d992c3 100644 > > > >>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > >>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > >>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path > > > >>>> name strings sorted in the same > > > >>>> interconnect-names to match interconnect paths with > > > >>>> interconnect > > > >>>> specifier pairs. > > > >>>> + Reserved interconnect names: > > > >>>> + * dma: Path from the device to the main > > > >>>> memory of the system > > > >>> > > > >>> Bikeshed: As it's from the device to the main memory, maybe here we can > > > >>> also denote this my calling the path dma-mem or dma-memory. For other > > > >>> paths, we are trying to mention both the source and the destination and > > > >>> maybe it would be good to be consistent although this is special one. > > > >> > > > >> I'm not sure I understand what you mean. You'd like two interconnect > > > >> names, one called dma-memory, and one memory-dma? > > > > > > > > Hmm, yes, it's not like "dma" describes an actual source or destination > > > > either :/ > > > > > > IIUC, it's a path (bus) that a dma device use to access some memory > > > (read or/and write). So i have used source-destination above more in the > > > sense of initiator-target or master-slave. My suggestion was just to > > > change the reserved interconnect name from "dma" to "dma-mem" or > > > "dma-memory". > > > > If dma is an issue in itself, maybe we can call it "device-memory" ? > > Might I ask what happens if the device can both DMA to and from memory? We can create another one called memory-device if that's needed? > IIRC the display frontends, backends, and mixers all have writeback > capability, using the same interconnect port. I think in both cases it's the same path. The camera driver also need to have that offset, even though it's a producer and not a consumer, and the VPU does too. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Mar 11, 2019 at 6:11 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Fri, Mar 08, 2019 at 12:09:47AM +0800, Chen-Yu Tsai wrote: > > On Thu, Mar 7, 2019 at 11:48 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Thu, Mar 07, 2019 at 05:15:20PM +0200, Georgi Djakov wrote: > > > > Hi, > > > > > > > > On 3/5/19 18:14, Robin Murphy wrote: > > > > > On 05/03/2019 15:53, Maxime Ripard wrote: > > > > >> Hi, > > > > >> > > > > >> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: > > > > >>> On 2/11/19 17:02, Maxime Ripard wrote: > > > > >>>> The current DT bindings assume that the DMA will be performed by the > > > > >>>> devices through their parent DT node, and rely on that assumption > > > > >>>> for the > > > > >>>> address translation using dma-ranges. > > > > >>>> > > > > >>>> However, some SoCs have devices that will perform DMA through > > > > >>>> another bus, > > > > >>>> with separate address translation rules. We therefore need to > > > > >>>> express that > > > > >>>> relationship, through the special interconnect name "dma". > > > > >>>> > > > > >>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > >>>> --- > > > > >>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | > > > > >>>> 3 +++ > > > > >>>> 1 file changed, 3 insertions(+) > > > > >>>> > > > > >>>> diff --git > > > > >>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > > >>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > > >>>> index 5a3c575b387a..e69fc2d992c3 100644 > > > > >>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > > >>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt > > > > >>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path > > > > >>>> name strings sorted in the same > > > > >>>> interconnect-names to match interconnect paths with > > > > >>>> interconnect > > > > >>>> specifier pairs. > > > > >>>> + Reserved interconnect names: > > > > >>>> + * dma: Path from the device to the main > > > > >>>> memory of the system > > > > >>> > > > > >>> Bikeshed: As it's from the device to the main memory, maybe here we can > > > > >>> also denote this my calling the path dma-mem or dma-memory. For other > > > > >>> paths, we are trying to mention both the source and the destination and > > > > >>> maybe it would be good to be consistent although this is special one. > > > > >> > > > > >> I'm not sure I understand what you mean. You'd like two interconnect > > > > >> names, one called dma-memory, and one memory-dma? > > > > > > > > > > Hmm, yes, it's not like "dma" describes an actual source or destination > > > > > either :/ > > > > > > > > IIUC, it's a path (bus) that a dma device use to access some memory > > > > (read or/and write). So i have used source-destination above more in the > > > > sense of initiator-target or master-slave. My suggestion was just to > > > > change the reserved interconnect name from "dma" to "dma-mem" or > > > > "dma-memory". > > > > > > If dma is an issue in itself, maybe we can call it "device-memory" ? > > > > Might I ask what happens if the device can both DMA to and from memory? > > We can create another one called memory-device if that's needed? Works for me as long as it doesn't get confusing. BTW I think it should be X-to-Y, instead of X-Y, as the latter can be taken as a noun-phrase. > > IIRC the display frontends, backends, and mixers all have writeback > > capability, using the same interconnect port. > > I think in both cases it's the same path. The camera driver also need > to have that offset, even though it's a producer and not a consumer, > and the VPU does too. Yeah. The VPU is the obvious bi-directional example. ChenYu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On 3/11/19 12:11, Maxime Ripard wrote: > On Fri, Mar 08, 2019 at 12:09:47AM +0800, Chen-Yu Tsai wrote: >> On Thu, Mar 7, 2019 at 11:48 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: >>> >>> On Thu, Mar 07, 2019 at 05:15:20PM +0200, Georgi Djakov wrote: >>>> Hi, >>>> >>>> On 3/5/19 18:14, Robin Murphy wrote: >>>>> On 05/03/2019 15:53, Maxime Ripard wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, Mar 01, 2019 at 07:48:15PM +0200, Georgi Djakov wrote: >>>>>>> On 2/11/19 17:02, Maxime Ripard wrote: >>>>>>>> The current DT bindings assume that the DMA will be performed by the >>>>>>>> devices through their parent DT node, and rely on that assumption >>>>>>>> for the >>>>>>>> address translation using dma-ranges. >>>>>>>> >>>>>>>> However, some SoCs have devices that will perform DMA through >>>>>>>> another bus, >>>>>>>> with separate address translation rules. We therefore need to >>>>>>>> express that >>>>>>>> relationship, through the special interconnect name "dma". >>>>>>>> >>>>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> >>>>>>>> --- >>>>>>>> Documentation/devicetree/bindings/interconnect/interconnect.txt | >>>>>>>> 3 +++ >>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>> >>>>>>>> diff --git >>>>>>>> a/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>>>>>> b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>>>>>> index 5a3c575b387a..e69fc2d992c3 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>>>>>> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt >>>>>>>> @@ -51,6 +51,9 @@ interconnect-names : List of interconnect path >>>>>>>> name strings sorted in the same >>>>>>>> interconnect-names to match interconnect paths with >>>>>>>> interconnect >>>>>>>> specifier pairs. >>>>>>>> + Reserved interconnect names: >>>>>>>> + * dma: Path from the device to the main >>>>>>>> memory of the system >>>>>>> >>>>>>> Bikeshed: As it's from the device to the main memory, maybe here we can >>>>>>> also denote this my calling the path dma-mem or dma-memory. For other >>>>>>> paths, we are trying to mention both the source and the destination and >>>>>>> maybe it would be good to be consistent although this is special one. >>>>>> >>>>>> I'm not sure I understand what you mean. You'd like two interconnect >>>>>> names, one called dma-memory, and one memory-dma? >>>>> >>>>> Hmm, yes, it's not like "dma" describes an actual source or destination >>>>> either :/ >>>> >>>> IIUC, it's a path (bus) that a dma device use to access some memory >>>> (read or/and write). So i have used source-destination above more in the >>>> sense of initiator-target or master-slave. My suggestion was just to >>>> change the reserved interconnect name from "dma" to "dma-mem" or >>>> "dma-memory". >>> >>> If dma is an issue in itself, maybe we can call it "device-memory" ? >> >> Might I ask what happens if the device can both DMA to and from memory? > > We can create another one called memory-device if that's needed? I don't think this is needed if in both cases the DMA device is the initiator of the transfer. In both cases the DMA device is the initiator (master), so memory-device or memory-dma does not make much sense. Sorry for the confusion. > >> IIRC the display frontends, backends, and mixers all have writeback >> capability, using the same interconnect port. > > I think in both cases it's the same path. The camera driver also need > to have that offset, even though it's a producer and not a consumer, > and the VPU does too. Again, the camera driver and probably the VPU too would be the initiators, so regarding naming it should be the same (despite that the data flow is bi-directional). Thanks, Georgi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel