[PATCH v2 0/8] RZN1 DMA support

[PATCH v2 0/8] RZN1 DMA support

[Miquel Raynal]

Hello,

Here is a first series bringing DMA support to RZN1 platforms. I'm not a
DMA expert at all so criticism is welcome.

Soon a second series will come with changes made to the UART controller
driver, in order to interact with the RZN1 DMA controller.

Cheers,
Miquèl

Changes in v2:
* Clarified that the 'fix' regarding non aligned reads would only apply
  to the DEV_TO_MEM case.
* Fix the DMA controller compatible string (copy-paste error).
* s/syscon/sysctrl/ as advised by Geert.
* Disabled irqs when taking the spinlock from the clocks driver.
* Moved the DMAMUX offset inside the driver.
* Removed extra commas.
* Improved the style as suggested by Andy.
* Removed a dupplicated check against the device node presence.
* Reduced the number of lines of code by using dev_err_probe().
* Created a Kconfig symbol for DMAMUX to fix the two robot reports
  received and be sure there was no useless overhead with other
  platforms.

Miquel Raynal (7):
  dt-bindings: dma: Introduce RZN1 dmamux bindings
  dt-bindings: dma: Introduce RZN1 DMA compatible
  soc: renesas: rzn1-sysc: Export function to set dmamux
  dma: dmamux: Introduce RZN1 DMA router support
  dma: dw: Add RZN1 compatible
  ARM: dts: r9a06g032: Add the two DMA nodes
  ARM: dts: r9a06g032: Describe the DMA router

Phil Edworthy (1):
  dma: dw: Avoid partial transfers

 .../bindings/dma/renesas,rzn1-dmamux.yaml     |  42 +++++
 .../bindings/dma/snps,dma-spear1340.yaml      |   8 +-
 MAINTAINERS                                   |   1 +
 arch/arm/boot/dts/r9a06g032.dtsi              |  37 ++++
 drivers/clk/renesas/r9a06g032-clocks.c        |  31 ++++
 drivers/dma/dw/Kconfig                        |   8 +
 drivers/dma/dw/Makefile                       |   2 +
 drivers/dma/dw/core.c                         |   3 +
 drivers/dma/dw/dmamux.c                       | 167 ++++++++++++++++++
 drivers/dma/dw/platform.c                     |   1 +
 include/linux/soc/renesas/r9a06g032-sysctrl.h |  11 ++
 11 files changed, 310 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
 create mode 100644 drivers/dma/dw/dmamux.c
 create mode 100644 include/linux/soc/renesas/r9a06g032-sysctrl.h

-- 
2.27.0

[PATCH v2 1/8] dt-bindings: dma: Introduce RZN1 dmamux bindings

[Miquel Raynal]

The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
dmamux register located in the system control area which can take up to
32 requests (16 per DMA controller). Each DMA channel can be wired to
two different peripherals.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/dma/renesas,rzn1-dmamux.yaml     | 42 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml

diff --git a/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml b/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
new file mode 100644
index 000000000000..e2c82e43b8b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/renesas,rzn1-dmamux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/N1 DMA mux
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+allOf:
+  - $ref: "dma-router.yaml#"
+
+properties:
+  compatible:
+    const: renesas,rzn1-dmamux
+
+  '#dma-cells':
+    const: 6
+    description:
+      The first four cells are dedicated to the master DMA controller. The fifth
+      cell gives the DMA mux bit index that must be set starting from 0. The
+      sixth cell gives the binary value that must be written there, ie. 0 or 1.
+
+  dma-masters:
+    minItems: 1
+    maxItems: 2
+
+  dma-requests:
+    const: 32
+
+additionalProperties: false
+
+examples:
+  - |
+    dma-router {
+      compatible = "renesas,rzn1-dmamux";
+      #dma-cells = <6>;
+      dma-masters = <&dma0 &dma1>;
+      dma-requests = <32>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ea3e6c914384..c70c9c39a2f3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18636,6 +18636,7 @@ SYNOPSYS DESIGNWARE DMAC DRIVER
 M:	Viresh Kumar <vireshk@kernel.org>
 R:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 S:	Maintained
+F:	Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
 F:	Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
 F:	drivers/dma/dw/
 F:	include/dt-bindings/dma/dw-dmac.h
-- 
2.27.0

Re: [PATCH v2 1/8] dt-bindings: dma: Introduce RZN1 dmamux bindings

[Geert Uytterhoeven]

Hi Miquel,

On Tue, Feb 22, 2022 at 11:34 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> dmamux register located in the system control area which can take up to
> 32 requests (16 per DMA controller). Each DMA channel can be wired to
> two different peripherals.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/renesas,rzn1-dmamux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/N1 DMA mux
> +
> +maintainers:
> +  - Miquel Raynal <miquel.raynal@bootlin.com>
> +
> +allOf:
> +  - $ref: "dma-router.yaml#"
> +
> +properties:
> +  compatible:
> +    const: renesas,rzn1-dmamux

Do we want an SoC-specific compatible value, too?
See also my comments on the dmamux driver.

> +
> +  '#dma-cells':
> +    const: 6
> +    description:
> +      The first four cells are dedicated to the master DMA controller. The fifth
> +      cell gives the DMA mux bit index that must be set starting from 0. The
> +      sixth cell gives the binary value that must be written there, ie. 0 or 1.
> +
> +  dma-masters:
> +    minItems: 1
> +    maxItems: 2
> +
> +  dma-requests:
> +    const: 32

Do we need this in DT? It depends on the actual dmamux hardware,
and is (currently) the register width of the CFG_DMAMUX register.

The rest LGTM (I'm no dma-router expert),so with the above clarified:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Miquel Raynal]

Just like for the NAND controller that is also on this SoC, let's
provide a SoC generic and a more specific couple of compatibles for the
DMA controller.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/dma/snps,dma-spear1340.yaml       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
index 6b35089ac017..c13649bf7f19 100644
--- a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
+++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
@@ -15,7 +15,13 @@ allOf:
 
 properties:
   compatible:
-    const: snps,dma-spear1340
+    oneOf:
+      - const: snps,dma-spear1340
+      - items:
+          - enum:
+              - renesas,r9a06g032-dma
+          - const: renesas,rzn1-dma
+
 
   "#dma-cells":
     minimum: 3
-- 
2.27.0

Re: [PATCH v2 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Geert Uytterhoeven]

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Just like for the NAND controller that is also on this SoC, let's
> provide a SoC generic and a more specific couple of compatibles for the
> DMA controller.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

> +++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml

Perhaps you want to add the power-domains property?
The RZ/N1 clock driver is also a clock-domain provider.

Apart from that:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:21:47 +0100:

> On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Just like for the NAND controller that is also on this SoC, let's
> > provide a SoC generic and a more specific couple of compatibles for the
> > DMA controller.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> > +++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml  
> 
> Perhaps you want to add the power-domains property?
> The RZ/N1 clock driver is also a clock-domain provider.

I haven't looked at the power domains yet, but I don't plan to invest
time on it right now. Unless I don't understand your request, and you
are telling me that someone else added the description and we should
now point to the right domain from each new node?

> Apart from that:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

Miquèl

Re: [PATCH v2 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Geert Uytterhoeven]

Hi Miquel,

On Wed, Feb 23, 2022 at 4:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:21:47 +0100:
> > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > > Just like for the NAND controller that is also on this SoC, let's
> > > provide a SoC generic and a more specific couple of compatibles for the
> > > DMA controller.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > > +++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
> >
> > Perhaps you want to add the power-domains property?
> > The RZ/N1 clock driver is also a clock-domain provider.
>
> I haven't looked at the power domains yet, but I don't plan to invest
> time on it right now. Unless I don't understand your request, and you
> are telling me that someone else added the description and we should
> now point to the right domain from each new node?

The RZ/N1 System Controller is a clock-domain provider.  This means
it can automatically manage the module clocks of devices that are
part of the clock domain, assuming device drivers are using Runtime PM.

The upstream RZ/N1 DTS doesn't have many devices enabled yet.
Most of them are (variants of) Synopsis IP cores, and their drivers
manage clocks explicitly, instead of relying on Runtime PM.

BTW, I have just noticed the system-controller node[1] even lacks
the #power-domain-cells property, while the example[2] does have it.
When that is added, device nodes can gain "power-domains = <&sysctrl>",
and module clocks can be managed from Runtime PM.  Perhaps the NAND
driver would be a good target for conversion to Runtime PM, as its
driver is not shared with SoCs from other vendors yet?
Note this is not mandatory, and drivers can keep on using explicit
clock handling (until the IP core is reused on an SoC that not only
has a clock-domain, but also real power-domains).

[1] arch/arm/boot/dts/r9a06g032.dtsi
[2] Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 17:16:32 +0100:

> Hi Miquel,
> 
> On Wed, Feb 23, 2022 at 4:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:21:47 +0100:  
> > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > > Just like for the NAND controller that is also on this SoC, let's
> > > > provide a SoC generic and a more specific couple of compatibles for the
> > > > DMA controller.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > >  
> > > > +++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml  
> > >
> > > Perhaps you want to add the power-domains property?
> > > The RZ/N1 clock driver is also a clock-domain provider.  
> >
> > I haven't looked at the power domains yet, but I don't plan to invest
> > time on it right now. Unless I don't understand your request, and you
> > are telling me that someone else added the description and we should
> > now point to the right domain from each new node?  
> 
> The RZ/N1 System Controller is a clock-domain provider. 

Yes, there are many domains managed there.

> This means
> it can automatically manage the module clocks of devices that are
> part of the clock domain, assuming device drivers are using Runtime PM.
> 
> The upstream RZ/N1 DTS doesn't have many devices enabled yet.
> Most of them are (variants of) Synopsis IP cores, and their drivers
> manage clocks explicitly, instead of relying on Runtime PM.
> 
> BTW, I have just noticed the system-controller node[1] even lacks
> the #power-domain-cells property, while the example[2] does have it.

Yes, that's why I didn't understand the initial remark.

> When that is added, device nodes can gain "power-domains = <&sysctrl>",
> and module clocks can be managed from Runtime PM.  Perhaps the NAND
> driver would be a good target for conversion to Runtime PM, as its
> driver is not shared with SoCs from other vendors yet?
> Note this is not mandatory, and drivers can keep on using explicit
> clock handling (until the IP core is reused on an SoC that not only
> has a clock-domain, but also real power-domains).

Got it, thanks for the details. Indeed it would be interesting to gain
runtime PM support if this SoC has a good power-domain support.

Power domain cells must first be contributed.

> [1] arch/arm/boot/dts/r9a06g032.dtsi
> [2] Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds


Thanks,
Miquèl

[PATCH v2 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal]

The dmamux register is located within the system controller.

Without syscon, we need an extra helper in order to give write access to
this register to a dmamux driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/renesas/r9a06g032-clocks.c        | 31 +++++++++++++++++++
 include/linux/soc/renesas/r9a06g032-sysctrl.h | 11 +++++++
 2 files changed, 42 insertions(+)
 create mode 100644 include/linux/soc/renesas/r9a06g032-sysctrl.h

diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
index c99942f0e4d4..370c89490be9 100644
--- a/drivers/clk/renesas/r9a06g032-clocks.c
+++ b/drivers/clk/renesas/r9a06g032-clocks.c
@@ -20,9 +20,12 @@
 #include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
+#include <linux/soc/renesas/r9a06g032-sysctrl.h>
 #include <linux/spinlock.h>
 #include <dt-bindings/clock/r9a06g032-sysctrl.h>
 
+#define R9A06G032_SYSCTRL_DMAMUX 0xA0
+
 struct r9a06g032_gate {
 	u16 gate, reset, ready, midle,
 		scon, mirack, mistat;
@@ -315,6 +318,28 @@ struct r9a06g032_priv {
 	void __iomem *reg;
 };
 
+/* Exported helper to access the DMAMUX register */
+static struct r9a06g032_priv *sysctrl_priv;
+int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val)
+{
+	unsigned long flags;
+	u32 dmamux;
+
+	if (!sysctrl_priv)
+		return -EPROBE_DEFER;
+
+	spin_lock_irqsave(&sysctrl_priv->lock, flags);
+
+	dmamux = readl(sysctrl_priv->reg + R9A06G032_SYSCTRL_DMAMUX);
+	dmamux &= ~mask;
+	dmamux |= val & mask;
+	writel(dmamux, sysctrl_priv->reg + R9A06G032_SYSCTRL_DMAMUX);
+
+	spin_unlock_irqrestore(&sysctrl_priv->lock, flags);
+
+	return 0;
+}
+
 /* register/bit pairs are encoded as an uint16_t */
 static void
 clk_rdesc_set(struct r9a06g032_priv *clocks,
@@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 	clocks->reg = of_iomap(np, 0);
 	if (WARN_ON(!clocks->reg))
 		return -ENOMEM;
+
+	if (sysctrl_priv)
+		return -EBUSY;
+
+	sysctrl_priv = clocks;
+
 	for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) {
 		const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i];
 		const char *parent_name = d->source ?
diff --git a/include/linux/soc/renesas/r9a06g032-sysctrl.h b/include/linux/soc/renesas/r9a06g032-sysctrl.h
new file mode 100644
index 000000000000..066dfb15cbdd
--- /dev/null
+++ b/include/linux/soc/renesas/r9a06g032-sysctrl.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__
+#define __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__
+
+#ifdef CONFIG_CLK_R9A06G032
+int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val);
+#else
+static inline int r9a06g032_sysctrl_set_dmamux(u32 mask, u32 val) { return -ENODEV; }
+#endif
+
+#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCTRL_H__ */
-- 
2.27.0

Re: [PATCH v2 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Geert Uytterhoeven]

Hi Miquel,

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> The dmamux register is located within the system controller.
>
> Without syscon, we need an extra helper in order to give write access to
> this register to a dmamux driver.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
>         clocks->reg = of_iomap(np, 0);
>         if (WARN_ON(!clocks->reg))
>                 return -ENOMEM;
> +
> +       if (sysctrl_priv)
> +               return -EBUSY;

Can this actually happen, or can the check be removed?

> +
> +       sysctrl_priv = clocks;

Oh yes, it can happen, if any of the clock registrations below fail
due to -EPROBE_DEFER. So I think the assignment to sysctrl_priv
should be postponed until we're sure that can no longer happen.
Then the check above can be removed, too.

> +
>         for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) {
>                 const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i];
>                 const char *parent_name = d->source ?

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:28:35 +0100:

> Hi Miquel,
> 
> On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > The dmamux register is located within the system controller.
> >
> > Without syscon, we need an extra helper in order to give write access to
> > this register to a dmamux driver.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a06g032-clocks.c
> > +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> > @@ -922,6 +947,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
> >         clocks->reg = of_iomap(np, 0);
> >         if (WARN_ON(!clocks->reg))
> >                 return -ENOMEM;
> > +
> > +       if (sysctrl_priv)
> > +               return -EBUSY;  
> 
> Can this actually happen, or can the check be removed?

I mostly wanted to prevent the case where a SoC would need the clock
driver to probe twice (sanity check let's say).

> > +
> > +       sysctrl_priv = clocks;  
> 
> Oh yes, it can happen, if any of the clock registrations below fail
> due to -EPROBE_DEFER. So I think the assignment to sysctrl_priv
> should be postponed until we're sure that can no longer happen.
> Then the check above can be removed, too.

Ok, no problem.

> > +
> >         for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) {
> >                 const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i];
> >                 const char *parent_name = d->source ?  
> 
> The rest LGTM.

Great, thanks!

Miquèl

[PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
dmamux register located in the system control area which can take up to
32 requests (16 per DMA controller). Each DMA channel can be wired to
two different peripherals.

We need two additional information from the 'dmas' property: the channel
(bit in the dmamux register) that must be accessed and the value of the
mux for this channel.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/Kconfig  |   8 ++
 drivers/dma/dw/Makefile |   2 +
 drivers/dma/dw/dmamux.c | 167 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 drivers/dma/dw/dmamux.c

diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig
index db25f9b7778c..dd53d4a9fa92 100644
--- a/drivers/dma/dw/Kconfig
+++ b/drivers/dma/dw/Kconfig
@@ -16,6 +16,14 @@ config DW_DMAC
 	  Support the Synopsys DesignWare AHB DMA controller. This
 	  can be integrated in chips such as the Intel Cherrytrail.
 
+config RZN1_DMAMUX
+	tristate "Renesas RZ/N1 DMAMUX driver"
+	depends on DW_DMAC
+	help
+	  Support the Renesas RZ/N1 DMAMUX which is located in front of
+	  the Synopsys DesignWare AHB DMA controller located on Renesas
+	  SoCs.
+
 config DW_DMAC_PCI
 	tristate "Synopsys DesignWare AHB DMA PCI driver"
 	depends on PCI
diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile
index a6f358ad8591..7c6e0ab6fcd8 100644
--- a/drivers/dma/dw/Makefile
+++ b/drivers/dma/dw/Makefile
@@ -7,5 +7,7 @@ obj-$(CONFIG_DW_DMAC)		+= dw_dmac.o
 dw_dmac-y			:= platform.o
 dw_dmac-$(CONFIG_OF)		+= of.o
 
+obj-$(CONFIG_RZN1_DMAMUX)	+= dmamux.o
+
 obj-$(CONFIG_DW_DMAC_PCI)	+= dw_dmac_pci.o
 dw_dmac_pci-y			:= pci.o
diff --git a/drivers/dma/dw/dmamux.c b/drivers/dma/dw/dmamux.c
new file mode 100644
index 000000000000..5fb3ffb82e88
--- /dev/null
+++ b/drivers/dma/dw/dmamux.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Schneider-Electric
+ * Author: Miquel Raynal <miquel.raynal@bootlin.com
+ * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
+ */
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/soc/renesas/r9a06g032-sysctrl.h>
+
+#define RZN1_DMAMUX_LINES	64
+
+struct rzn1_dmamux_data {
+	struct dma_router dmarouter;
+	unsigned int dmac_requests;
+	unsigned int dmamux_requests;
+	u32 used_chans;
+	struct mutex lock;
+};
+
+struct rzn1_dmamux_map {
+	unsigned int req_idx;
+};
+
+static void rzn1_dmamux_free(struct device *dev, void *route_data)
+{
+	struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev);
+	struct rzn1_dmamux_map *map = route_data;
+
+	dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx);
+
+	mutex_lock(&dmamux->lock);
+	dmamux->used_chans &= ~BIT(map->req_idx);
+	mutex_unlock(&dmamux->lock);
+
+	kfree(map);
+}
+
+static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
+					struct of_dma *ofdma)
+{
+	struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
+	struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
+	struct rzn1_dmamux_map *map;
+	unsigned int master, chan, val;
+	u32 mask;
+	int ret;
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return ERR_PTR(-ENOMEM);
+
+	if (dma_spec->args_count != 6)
+		return ERR_PTR(-EINVAL);
+
+	chan = dma_spec->args[0];
+	map->req_idx = dma_spec->args[4];
+	val = dma_spec->args[5];
+	dma_spec->args_count -= 2;
+
+	if (chan >= dmamux->dmac_requests) {
+		dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (map->req_idx >= dmamux->dmamux_requests ||
+	    map->req_idx % dmamux->dmac_requests != chan) {
+		dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* The of_node_put() will be done in the core for the node */
+	master = map->req_idx >= dmamux->dmac_requests ? 1 : 0;
+	dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master);
+	if (!dma_spec->np) {
+		dev_err(&pdev->dev, "Can't get DMA master\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
+		map->req_idx, master, chan);
+
+	mask = BIT(map->req_idx);
+	mutex_lock(&dmamux->lock);
+	dmamux->used_chans |= mask;
+	ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
+	mutex_unlock(&dmamux->lock);
+	if (ret) {
+		rzn1_dmamux_free(&pdev->dev, map);
+		return ERR_PTR(ret);
+	}
+
+	return map;
+}
+
+static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
+	{ .compatible = "renesas,rzn1-dma" },
+	{},
+};
+
+static int rzn1_dmamux_probe(struct platform_device *pdev)
+{
+	struct device_node *mux_node = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct device_node *dmac_node;
+	struct rzn1_dmamux_data *dmamux;
+
+	dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
+	if (!dmamux)
+		return -ENOMEM;
+
+	mutex_init(&dmamux->lock);
+
+	dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
+	if (!dmac_node)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
+
+	match = of_match_node(rzn1_dmac_match, dmac_node);
+	if (!match) {
+		of_node_put(dmac_node);
+		return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
+	}
+
+	if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
+		of_node_put(dmac_node);
+		return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
+	}
+
+	of_node_put(dmac_node);
+
+	if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {
+		return dev_err_probe(&pdev->dev, -EINVAL, "Missing mux requests information\n");
+	}
+
+	dmamux->dmarouter.dev = &pdev->dev;
+	dmamux->dmarouter.route_free = rzn1_dmamux_free;
+
+	platform_set_drvdata(pdev, dmamux);
+
+	return of_dma_router_register(mux_node, rzn1_dmamux_route_allocate,
+				      &dmamux->dmarouter);
+}
+
+static const struct of_device_id rzn1_dmamux_match[] = {
+	{ .compatible = "renesas,rzn1-dmamux" },
+	{},
+};
+
+static struct platform_driver rzn1_dmamux_driver = {
+	.driver = {
+		.name = "renesas,rzn1-dmamux",
+		.of_match_table = rzn1_dmamux_match,
+	},
+	.probe	= rzn1_dmamux_probe,
+};
+
+static int rzn1_dmamux_init(void)
+{
+	return platform_driver_register(&rzn1_dmamux_driver);
+}
+arch_initcall(rzn1_dmamux_init);
-- 
2.27.0

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[kernel test robot]

Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on geert-renesas-devel/next]
[also build test ERROR on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc5 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/RZN1-DMA-support/20220222-183549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220223/202202230444.xjzzeOlo-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b62059e893c7e3779b96e1c69ae6818260d352de
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220222-183549
        git checkout b62059e893c7e3779b96e1c69ae6818260d352de
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: missing MODULE_LICENSE() in drivers/dma/dw/dmamux.o
>> ERROR: modpost: "r9a06g032_sysctrl_set_dmamux" [drivers/dma/dw/dmamux.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hello,

> ERROR: modpost: missing MODULE_LICENSE() in drivers/dma/dw/dmamux.o
> ERROR: modpost: "r9a06g032_sysctrl_set_dmamux" [drivers/dma/dw/dmamux.ko] undefined!  

I will wait for more feedback against this version and then send a v3
with the missing macros.

Thanks,
Miquèl

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

[...]

> +static int rzn1_dmamux_init(void)
> +{
> +	return platform_driver_register(&rzn1_dmamux_driver);
> +}
> +arch_initcall(rzn1_dmamux_init);

I don't think this driver actually needs the arch_initcall() level,
I'll propose a v3 using the regular platform init level.

And this driver is now missing MODULE_* macros.

Thanks,
Miquèl

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Geert Uytterhoeven]

Hi Miquel,

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> dmamux register located in the system control area which can take up to
> 32 requests (16 per DMA controller). Each DMA channel can be wired to
> two different peripherals.
>
> We need two additional information from the 'dmas' property: the channel
> (bit in the dmamux register) that must be accessed and the value of the
> mux for this channel.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/dma/dw/dmamux.c

rzn1-dmamux.c?

> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Schneider-Electric
> + * Author: Miquel Raynal <miquel.raynal@bootlin.com
> + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
> + */
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/soc/renesas/r9a06g032-sysctrl.h>
> +
> +#define RZN1_DMAMUX_LINES      64

Unused. But using it wouldn't hurt, I guess?

> +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> +                                       struct of_dma *ofdma)
> +{
> +       struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> +       struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
> +       struct rzn1_dmamux_map *map;
> +       unsigned int master, chan, val;
> +       u32 mask;
> +       int ret;
> +
> +       map = kzalloc(sizeof(*map), GFP_KERNEL);
> +       if (!map)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (dma_spec->args_count != 6)
> +               return ERR_PTR(-EINVAL);
> +
> +       chan = dma_spec->args[0];
> +       map->req_idx = dma_spec->args[4];
> +       val = dma_spec->args[5];
> +       dma_spec->args_count -= 2;
> +
> +       if (chan >= dmamux->dmac_requests) {
> +               dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan);

%u

> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (map->req_idx >= dmamux->dmamux_requests ||
> +           map->req_idx % dmamux->dmac_requests != chan) {

The reliance on .dmac_requests (i.e. "dma-requests" in the parent
DMA controller DT node) looks fragile to me.  Currently there are two
masters, each providing 16 channels, hence using all 2 x 16 =
32 bits in the DMAMUX register.
What if a variant used the same mux, and the same 16/16 split, but
the parent DMACs don't have all channels available?
I think it would be safer to hardcode this as 16 (using a #define, ofc).

> +               dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx);

%u

> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       /* The of_node_put() will be done in the core for the node */
> +       master = map->req_idx >= dmamux->dmac_requests ? 1 : 0;

The name "master" confused me: initially I thought it was used as a
boolean flag, but it really is the index of the parent DMAC.

> +       dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master);
> +       if (!dma_spec->np) {
> +               dev_err(&pdev->dev, "Can't get DMA master\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
> +               map->req_idx, master, chan);
> +
> +       mask = BIT(map->req_idx);
> +       mutex_lock(&dmamux->lock);
> +       dmamux->used_chans |= mask;
> +       ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> +       mutex_unlock(&dmamux->lock);
> +       if (ret) {
> +               rzn1_dmamux_free(&pdev->dev, map);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return map;
> +}
> +
> +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> +       { .compatible = "renesas,rzn1-dma" },
> +       {},
> +};
> +
> +static int rzn1_dmamux_probe(struct platform_device *pdev)
> +{
> +       struct device_node *mux_node = pdev->dev.of_node;
> +       const struct of_device_id *match;
> +       struct device_node *dmac_node;
> +       struct rzn1_dmamux_data *dmamux;
> +
> +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> +       if (!dmamux)
> +               return -ENOMEM;
> +
> +       mutex_init(&dmamux->lock);
> +
> +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> +       if (!dmac_node)
> +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> +
> +       match = of_match_node(rzn1_dmac_match, dmac_node);
> +       if (!match) {
> +               of_node_put(dmac_node);
> +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> +       }
> +
> +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> +               of_node_put(dmac_node);
> +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> +       }
> +
> +       of_node_put(dmac_node);

When hardcoding dmac_requests to 16, I guess the whole dmac_node
handling can be removed?

> +
> +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {

Don't obtain from DT, but fix to 32?

> +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing mux requests information\n");
> +       }
> +
> +       dmamux->dmarouter.dev = &pdev->dev;
> +       dmamux->dmarouter.route_free = rzn1_dmamux_free;
> +
> +       platform_set_drvdata(pdev, dmamux);
> +
> +       return of_dma_router_register(mux_node, rzn1_dmamux_route_allocate,
> +                                     &dmamux->dmarouter);
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:

> Hi Miquel,
> 
> On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> > dmamux register located in the system control area which can take up to
> > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > two different peripherals.
> >
> > We need two additional information from the 'dmas' property: the channel
> > (bit in the dmamux register) that must be accessed and the value of the
> > mux for this channel.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/dma/dw/dmamux.c  
> 
> rzn1-dmamux.c?

Ok.

> 
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 Schneider-Electric
> > + * Author: Miquel Raynal <miquel.raynal@bootlin.com
> > + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
> > + */
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/list.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/soc/renesas/r9a06g032-sysctrl.h>
> > +
> > +#define RZN1_DMAMUX_LINES      64  
> 
> Unused. But using it wouldn't hurt, I guess?
> 
> > +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> > +                                       struct of_dma *ofdma)
> > +{
> > +       struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> > +       struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
> > +       struct rzn1_dmamux_map *map;
> > +       unsigned int master, chan, val;
> > +       u32 mask;
> > +       int ret;
> > +
> > +       map = kzalloc(sizeof(*map), GFP_KERNEL);
> > +       if (!map)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       if (dma_spec->args_count != 6)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       chan = dma_spec->args[0];
> > +       map->req_idx = dma_spec->args[4];
> > +       val = dma_spec->args[5];
> > +       dma_spec->args_count -= 2;
> > +
> > +       if (chan >= dmamux->dmac_requests) {
> > +               dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan);  
> 
> %u
> 
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       if (map->req_idx >= dmamux->dmamux_requests ||
> > +           map->req_idx % dmamux->dmac_requests != chan) {  
> 
> The reliance on .dmac_requests (i.e. "dma-requests" in the parent
> DMA controller DT node) looks fragile to me.  Currently there are two
> masters, each providing 16 channels, hence using all 2 x 16 =
> 32 bits in the DMAMUX register.
> What if a variant used the same mux, and the same 16/16 split, but
> the parent DMACs don't have all channels available?
> I think it would be safer to hardcode this as 16 (using a #define, ofc).

That's right, I assumed this was safe but indeed it does not work in
all cases. I will change the second condition to:

		map->req_idx % <16> != chan

> 
> > +               dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx);  
> 
> %u
> 
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       /* The of_node_put() will be done in the core for the node */
> > +       master = map->req_idx >= dmamux->dmac_requests ? 1 : 0;  
> 
> The name "master" confused me: initially I thought it was used as a
> boolean flag, but it really is the index of the parent DMAC.

I personally prefer using true/false for booleans ;) Whatever, the name
is badly chosen I agree, I'll switch to "dmac_idx" which seems more
accurate.

> 
> > +       dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master);
> > +       if (!dma_spec->np) {
> > +               dev_err(&pdev->dev, "Can't get DMA master\n");
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
> > +               map->req_idx, master, chan);
> > +
> > +       mask = BIT(map->req_idx);
> > +       mutex_lock(&dmamux->lock);
> > +       dmamux->used_chans |= mask;
> > +       ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0);
> > +       mutex_unlock(&dmamux->lock);
> > +       if (ret) {
> > +               rzn1_dmamux_free(&pdev->dev, map);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return map;
> > +}
> > +
> > +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> > +       { .compatible = "renesas,rzn1-dma" },
> > +       {},
> > +};
> > +
> > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *mux_node = pdev->dev.of_node;
> > +       const struct of_device_id *match;
> > +       struct device_node *dmac_node;
> > +       struct rzn1_dmamux_data *dmamux;
> > +
> > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > +       if (!dmamux)
> > +               return -ENOMEM;
> > +
> > +       mutex_init(&dmamux->lock);
> > +
> > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > +       if (!dmac_node)
> > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > +
> > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > +       if (!match) {
> > +               of_node_put(dmac_node);
> > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > +       }
> > +
> > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > +               of_node_put(dmac_node);
> > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > +       }
> > +
> > +       of_node_put(dmac_node);  
> 
> When hardcoding dmac_requests to 16, I guess the whole dmac_node
> handling can be removed?

Not really, I think the following checks are still valid and fortunate,
and they need some of_ handling to work properly:
- verify that the chan requested is within the range of dmac_requests
  in the _route_allocate() callback
- ensure the dmamux is wired to a supported DMAC in the DT (this
  condition might be loosen in the future if needed or dropped entirely
  if considered useless)
- I would like to add a check against the number of requests supported
  by the dmamux and the dmac (not done yet).
For the record, I've taken inspiration to write these lines on the other
dma router driver from TI.

Unless, and I know some people think like that, we do not try to
validate the DT and if the DT is wrong that is none of our business.

> 
> > +
> > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {  
> 
> Don't obtain from DT, but fix to 32?

I believe the answer to the previous question should give me a clue
about why you would prefer hardcoding than reading from the DT such
an information. Perhaps I should mention that all these properties are
already part of the bindings, and are not specific to the driver, the
information will be in the DT anyway.

Thanks,
Miquèl

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Geert Uytterhoeven]

Hi Miquel,

On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:
> > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > > The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> > > dmamux register located in the system control area which can take up to
> > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > two different peripherals.
> > >
> > > We need two additional information from the 'dmas' property: the channel
> > > (bit in the dmamux register) that must be accessed and the value of the
> > > mux for this channel.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/drivers/dma/dw/dmamux.c

> > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > +       const struct of_device_id *match;
> > > +       struct device_node *dmac_node;
> > > +       struct rzn1_dmamux_data *dmamux;
> > > +
> > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > +       if (!dmamux)
> > > +               return -ENOMEM;
> > > +
> > > +       mutex_init(&dmamux->lock);
> > > +
> > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > +       if (!dmac_node)
> > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > +
> > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > +       if (!match) {
> > > +               of_node_put(dmac_node);
> > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > +       }
> > > +
> > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > +               of_node_put(dmac_node);
> > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > +       }
> > > +
> > > +       of_node_put(dmac_node);
> >
> > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > handling can be removed?
>
> Not really, I think the following checks are still valid and fortunate,
> and they need some of_ handling to work properly:
> - verify that the chan requested is within the range of dmac_requests
>   in the _route_allocate() callback
> - ensure the dmamux is wired to a supported DMAC in the DT (this
>   condition might be loosen in the future if needed or dropped entirely
>   if considered useless)
> - I would like to add a check against the number of requests supported
>   by the dmamux and the dmac (not done yet).
> For the record, I've taken inspiration to write these lines on the other
> dma router driver from TI.
>
> Unless, and I know some people think like that, we do not try to
> validate the DT and if the DT is wrong that is none of our business.
>
> >
> > > +
> > > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {
> >
> > Don't obtain from DT, but fix to 32?
>
> I believe the answer to the previous question should give me a clue
> about why you would prefer hardcoding than reading from the DT such
> an information. Perhaps I should mention that all these properties are
> already part of the bindings, and are not specific to the driver, the
> information will be in the DT anyway.

The 32 is a property of the hardware (32 bits in DMAMUX register).
So IMHO it falls under the "differentiate by compatible value,
not by property" rule.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Thu, 24 Feb 2022 10:14:48 +0100:

> Hi Miquel,
> 
> On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:  
> > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > > The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> > > > dmamux register located in the system control area which can take up to
> > > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > > two different peripherals.
> > > >
> > > > We need two additional information from the 'dmas' property: the channel
> > > > (bit in the dmamux register) that must be accessed and the value of the
> > > > mux for this channel.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> > >
> > > Thanks for your patch!
> > >  
> > > > --- /dev/null
> > > > +++ b/drivers/dma/dw/dmamux.c  
> 
> > > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > > +{
> > > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > > +       const struct of_device_id *match;
> > > > +       struct device_node *dmac_node;
> > > > +       struct rzn1_dmamux_data *dmamux;
> > > > +
> > > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > > +       if (!dmamux)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       mutex_init(&dmamux->lock);
> > > > +
> > > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > > +       if (!dmac_node)
> > > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > > +
> > > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > > +       if (!match) {
> > > > +               of_node_put(dmac_node);
> > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > > +       }
> > > > +
> > > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > > +               of_node_put(dmac_node);
> > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > > +       }
> > > > +
> > > > +       of_node_put(dmac_node);  
> > >
> > > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > > handling can be removed?  
> >
> > Not really, I think the following checks are still valid and fortunate,
> > and they need some of_ handling to work properly:
> > - verify that the chan requested is within the range of dmac_requests
> >   in the _route_allocate() callback
> > - ensure the dmamux is wired to a supported DMAC in the DT (this
> >   condition might be loosen in the future if needed or dropped entirely
> >   if considered useless)
> > - I would like to add a check against the number of requests supported
> >   by the dmamux and the dmac (not done yet).
> > For the record, I've taken inspiration to write these lines on the other
> > dma router driver from TI.
> >
> > Unless, and I know some people think like that, we do not try to
> > validate the DT and if the DT is wrong that is none of our business.
> >  
> > >  
> > > > +
> > > > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {  
> > >
> > > Don't obtain from DT, but fix to 32?  
> >
> > I believe the answer to the previous question should give me a clue
> > about why you would prefer hardcoding than reading from the DT such
> > an information. Perhaps I should mention that all these properties are
> > already part of the bindings, and are not specific to the driver, the
> > information will be in the DT anyway.  
> 
> The 32 is a property of the hardware (32 bits in DMAMUX register).
> So IMHO it falls under the "differentiate by compatible value,
> not by property" rule.

I agree this is a property of the hardware and feels redundant here.

What about the checks below, do you agree with the fact that I should
keep them or do you prefer dropping them (all? partially?)?


Thanks,
Miquèl

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Geert Uytterhoeven]

Hi Miquel,

On Thu, Feb 24, 2022 at 10:27 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Thu, 24 Feb 2022 10:14:48 +0100:
> > On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100:
> > > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > > > <miquel.raynal@bootlin.com> wrote:
> > > > > The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> > > > > dmamux register located in the system control area which can take up to
> > > > > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > > > > two different peripherals.
> > > > >
> > > > > We need two additional information from the 'dmas' property: the channel
> > > > > (bit in the dmamux register) that must be accessed and the value of the
> > > > > mux for this channel.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/drivers/dma/dw/dmamux.c
> >
> > > > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > > > +       const struct of_device_id *match;
> > > > > +       struct device_node *dmac_node;
> > > > > +       struct rzn1_dmamux_data *dmamux;
> > > > > +
> > > > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > > > +       if (!dmamux)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       mutex_init(&dmamux->lock);
> > > > > +
> > > > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > > > +       if (!dmac_node)
> > > > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > > > +
> > > > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > > > +       if (!match) {
> > > > > +               of_node_put(dmac_node);
> > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > > > +       }
> > > > > +
> > > > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > > > +               of_node_put(dmac_node);
> > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > > > +       }
> > > > > +
> > > > > +       of_node_put(dmac_node);
> > > >
> > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > > > handling can be removed?
> > >
> > > Not really, I think the following checks are still valid and fortunate,
> > > and they need some of_ handling to work properly:
> > > - verify that the chan requested is within the range of dmac_requests
> > >   in the _route_allocate() callback
> > > - ensure the dmamux is wired to a supported DMAC in the DT (this
> > >   condition might be loosen in the future if needed or dropped entirely
> > >   if considered useless)
> > > - I would like to add a check against the number of requests supported
> > >   by the dmamux and the dmac (not done yet).
> > > For the record, I've taken inspiration to write these lines on the other
> > > dma router driver from TI.
> > >
> > > Unless, and I know some people think like that, we do not try to
> > > validate the DT and if the DT is wrong that is none of our business.
> > >
> > > >
> > > > > +
> > > > > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {
> > > >
> > > > Don't obtain from DT, but fix to 32?
> > >
> > > I believe the answer to the previous question should give me a clue
> > > about why you would prefer hardcoding than reading from the DT such
> > > an information. Perhaps I should mention that all these properties are
> > > already part of the bindings, and are not specific to the driver, the
> > > information will be in the DT anyway.
> >
> > The 32 is a property of the hardware (32 bits in DMAMUX register).
> > So IMHO it falls under the "differentiate by compatible value,
> > not by property" rule.
>
> I agree this is a property of the hardware and feels redundant here.
>
> What about the checks below, do you agree with the fact that I should
> keep them or do you prefer dropping them (all? partially?)?

There are no checks below?
/me confused.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hi Geert,

> > > > > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > > > > +       const struct of_device_id *match;
> > > > > > +       struct device_node *dmac_node;
> > > > > > +       struct rzn1_dmamux_data *dmamux;
> > > > > > +
> > > > > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > > > > +       if (!dmamux)
> > > > > > +               return -ENOMEM;
> > > > > > +
> > > > > > +       mutex_init(&dmamux->lock);
> > > > > > +
> > > > > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > > > > +       if (!dmac_node)
> > > > > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > > > > +
> > > > > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > > > > +       if (!match) {
> > > > > > +               of_node_put(dmac_node);
> > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > > > > +       }
> > > > > > +
> > > > > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > > > > +               of_node_put(dmac_node);
> > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > > > > +       }
> > > > > > +
> > > > > > +       of_node_put(dmac_node);  
> > > > >
> > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > > > > handling can be removed?  
> > > >
> > > > Not really, I think the following checks are still valid and fortunate,
> > > > and they need some of_ handling to work properly:
> > > > - verify that the chan requested is within the range of dmac_requests
> > > >   in the _route_allocate() callback
> > > > - ensure the dmamux is wired to a supported DMAC in the DT (this
> > > >   condition might be loosen in the future if needed or dropped entirely
> > > >   if considered useless)
> > > > - I would like to add a check against the number of requests supported
> > > >   by the dmamux and the dmac (not done yet).
> > > > For the record, I've taken inspiration to write these lines on the other
> > > > dma router driver from TI.

        ^^^^^^^^^^^
... these checks

> > > >
> > > > Unless, and I know some people think like that, we do not try to
> > > > validate the DT and if the DT is wrong that is none of our business.
> > > >  
> > > > >  
> > > > > > +
> > > > > > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {  
> > > > >
> > > > > Don't obtain from DT, but fix to 32?  
> > > >
> > > > I believe the answer to the previous question should give me a clue
> > > > about why you would prefer hardcoding than reading from the DT such
> > > > an information. Perhaps I should mention that all these properties are
> > > > already part of the bindings, and are not specific to the driver, the
> > > > information will be in the DT anyway.  
> > >
> > > The 32 is a property of the hardware (32 bits in DMAMUX register).
> > > So IMHO it falls under the "differentiate by compatible value,
> > > not by property" rule.  
> >
> > I agree this is a property of the hardware and feels redundant here.
> >
> > What about the checks below, do you agree with the fact that I should
> > keep them or do you prefer dropping them (all? partially?)?  
> 
> There are no checks below?

I meant above /o\ ...

> /me confused.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Geert Uytterhoeven]

Hi Miquel,

On Thu, Feb 24, 2022 at 12:36 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> > > > > > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > > > > > +       const struct of_device_id *match;
> > > > > > > +       struct device_node *dmac_node;
> > > > > > > +       struct rzn1_dmamux_data *dmamux;
> > > > > > > +
> > > > > > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > > > > > +       if (!dmamux)
> > > > > > > +               return -ENOMEM;
> > > > > > > +
> > > > > > > +       mutex_init(&dmamux->lock);
> > > > > > > +
> > > > > > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > > > > > +       if (!dmac_node)
> > > > > > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > > > > > +
> > > > > > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > > > > > +       if (!match) {
> > > > > > > +               of_node_put(dmac_node);
> > > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > > > > > +               of_node_put(dmac_node);
> > > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       of_node_put(dmac_node);
> > > > > >
> > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > > > > > handling can be removed?
> > > > >
> > > > > Not really, I think the following checks are still valid and fortunate,
> > > > > and they need some of_ handling to work properly:
> > > > > - verify that the chan requested is within the range of dmac_requests
> > > > >   in the _route_allocate() callback
> > > > > - ensure the dmamux is wired to a supported DMAC in the DT (this
> > > > >   condition might be loosen in the future if needed or dropped entirely
> > > > >   if considered useless)
> > > > > - I would like to add a check against the number of requests supported
> > > > >   by the dmamux and the dmac (not done yet).
> > > > > For the record, I've taken inspiration to write these lines on the other
> > > > > dma router driver from TI.
>
>         ^^^^^^^^^^^
> ... these checks

I don't know. Some of them will be checked when calling into the
parent DMAC, right?

>
> > > > >
> > > > > Unless, and I know some people think like that, we do not try to
> > > > > validate the DT and if the DT is wrong that is none of our business.
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +       if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) {
> > > > > >
> > > > > > Don't obtain from DT, but fix to 32?
> > > > >
> > > > > I believe the answer to the previous question should give me a clue
> > > > > about why you would prefer hardcoding than reading from the DT such
> > > > > an information. Perhaps I should mention that all these properties are
> > > > > already part of the bindings, and are not specific to the driver, the
> > > > > information will be in the DT anyway.
> > > >
> > > > The 32 is a property of the hardware (32 bits in DMAMUX register).
> > > > So IMHO it falls under the "differentiate by compatible value,
> > > > not by property" rule.
> > >
> > > I agree this is a property of the hardware and feels redundant here.
> > >
> > > What about the checks below, do you agree with the fact that I should
> > > keep them or do you prefer dropping them (all? partially?)?
> >
> > There are no checks below?
>
> I meant above /o\ ...


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Thu, 24 Feb 2022 13:16:09 +0100:

> Hi Miquel,
> 
> On Thu, Feb 24, 2022 at 12:36 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > > > > > > > +static int rzn1_dmamux_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +       struct device_node *mux_node = pdev->dev.of_node;
> > > > > > > > +       const struct of_device_id *match;
> > > > > > > > +       struct device_node *dmac_node;
> > > > > > > > +       struct rzn1_dmamux_data *dmamux;
> > > > > > > > +
> > > > > > > > +       dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
> > > > > > > > +       if (!dmamux)
> > > > > > > > +               return -ENOMEM;
> > > > > > > > +
> > > > > > > > +       mutex_init(&dmamux->lock);
> > > > > > > > +
> > > > > > > > +       dmac_node = of_parse_phandle(mux_node, "dma-masters", 0);
> > > > > > > > +       if (!dmac_node)
> > > > > > > > +               return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n");
> > > > > > > > +
> > > > > > > > +       match = of_match_node(rzn1_dmac_match, dmac_node);
> > > > > > > > +       if (!match) {
> > > > > > > > +               of_node_put(dmac_node);
> > > > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n");
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) {
> > > > > > > > +               of_node_put(dmac_node);
> > > > > > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n");
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       of_node_put(dmac_node);  
> > > > > > >
> > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node
> > > > > > > handling can be removed?  
> > > > > >
> > > > > > Not really, I think the following checks are still valid and fortunate,
> > > > > > and they need some of_ handling to work properly:
> > > > > > - verify that the chan requested is within the range of dmac_requests
> > > > > >   in the _route_allocate() callback
> > > > > > - ensure the dmamux is wired to a supported DMAC in the DT (this
> > > > > >   condition might be loosen in the future if needed or dropped entirely
> > > > > >   if considered useless)
> > > > > > - I would like to add a check against the number of requests supported
> > > > > >   by the dmamux and the dmac (not done yet).
> > > > > > For the record, I've taken inspiration to write these lines on the other
> > > > > > dma router driver from TI.  
> >
> >         ^^^^^^^^^^^
> > ... these checks  
> 
> I don't know. Some of them will be checked when calling into the
> parent DMAC, right?

Only the first item above will be validated by the DMAC driver. But I
prefer to error out sooner than later, because getting the mux in
place while knowing that the request is invalid sounds silly.

Thanks,
Miquèl

[PATCH v2 5/8] dma: dw: Avoid partial transfers

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

Pausing a partial transfer only causes data to be written to memory that
is a multiple of the memory width setting.

However, when a DMA client driver finishes DMA early, e.g. due to UART
char timeout interrupt, all data read from the device must be written to
memory.

Therefore, allow the slave to limit the memory width to ensure all data
read from the device is written to memory when DMA is paused.

This change only applies to the DMA_DEV_TO_MEM case.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 7ab83fe601ed..48cdefe997f1 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -705,6 +705,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) :
 			DWC_CTLL_FC(DW_DMA_FC_D_P2M);
 
+		if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width)
+			data_width = sconfig->dst_addr_width;
+
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
 			u32		len, mem;
-- 
2.27.0

Re: [PATCH v2 5/8] dma: dw: Avoid partial transfers

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Pausing a partial transfer only causes data to be written to memory that
> is a multiple of the memory width setting.
> 
> However, when a DMA client driver finishes DMA early, e.g. due to UART
> char timeout interrupt, all data read from the device must be written to
> memory.
> 
> Therefore, allow the slave to limit the memory width to ensure all data
> read from the device is written to memory when DMA is paused.

(I have only 2.17d and 2.21a datasheets, so below based on the latter)

It seems you are referring to the chapter 7.7 "Disabling a Channel Prior
to Transfer Completion" of the data sheet where it stays that it does not
guarantee to have last burst to be completed in case of
SRC_TR_WIDTH < DST_TR_WIDTH and the CH_SUSP bit is high, when the FIFO_EMPTY
is asserted.

Okay, in iDMA 32-bit we have a specific bit (seems like a fix) that drains
FIFO, but still it doesn't drain the FIFO fully (in case of misalignment)
and the last piece of data (which is less than TR width) is lost when channel
gets disabled.

Now, if we look at the implementation of the serial8250_rx_dma_flush() we
may see that it does
 1. Pause channel without draining FIFO
 2. Moves data to TTY buffer
 3. Terminates channel.

During termination it does pause channel again (with draining enabled),
followed by disabling channel and resuming it again.

According to the 7.7 the resuming channel allows to finish the transfer
normally.

It seems the logic in the ->terminate_all() is broken and we actually need
to resume channel first (possibly conditionally, if it was suspended), then
pause it and disable and resume again.

The problem with ->terminate_all() is that it has no knowledge if it has
been called on paused channel (that's why it has to pause channel itself).
The pause on termination is required due to some issues in early steppings
of iDMA 32-bit hardware implementations.

If my theory is correct, the above change should fix the issues you see.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] dma: dw: Avoid partial transfers

[Miquel Raynal]

Hi Andy, Phil,

andriy.shevchenko@linux.intel.com wrote on Wed, 23 Feb 2022 15:35:58
+0200:

> On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> > 
> > Pausing a partial transfer only causes data to be written to memory that
> > is a multiple of the memory width setting.
> > 
> > However, when a DMA client driver finishes DMA early, e.g. due to UART
> > char timeout interrupt, all data read from the device must be written to
> > memory.
> > 
> > Therefore, allow the slave to limit the memory width to ensure all data
> > read from the device is written to memory when DMA is paused.  
> 
> (I have only 2.17d and 2.21a datasheets, so below based on the latter)
> 
> It seems you are referring to the chapter 7.7 "Disabling a Channel Prior
> to Transfer Completion" of the data sheet where it stays that it does not
> guarantee to have last burst to be completed in case of
> SRC_TR_WIDTH < DST_TR_WIDTH and the CH_SUSP bit is high, when the FIFO_EMPTY
> is asserted.
> 
> Okay, in iDMA 32-bit we have a specific bit (seems like a fix) that drains
> FIFO, but still it doesn't drain the FIFO fully (in case of misalignment)
> and the last piece of data (which is less than TR width) is lost when channel
> gets disabled.
> 
> Now, if we look at the implementation of the serial8250_rx_dma_flush() we
> may see that it does
>  1. Pause channel without draining FIFO
>  2. Moves data to TTY buffer
>  3. Terminates channel.
> 
> During termination it does pause channel again (with draining enabled),
> followed by disabling channel and resuming it again.
> 
> According to the 7.7 the resuming channel allows to finish the transfer
> normally.
> 
> It seems the logic in the ->terminate_all() is broken and we actually need
> to resume channel first (possibly conditionally, if it was suspended), then
> pause it and disable and resume again.
> 
> The problem with ->terminate_all() is that it has no knowledge if it has
> been called on paused channel (that's why it has to pause channel itself).
> The pause on termination is required due to some issues in early steppings
> of iDMA 32-bit hardware implementations.
> 
> If my theory is correct, the above change should fix the issues you see.

I don't have access to these datasheets so I will believe your words
and try to apply Andy's solution. I ended up with the following fix,
hopefully I got it right:

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 48cdefe997f1..59822664d8ec 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -865,6 +865,10 @@ static int dwc_terminate_all(struct dma_chan *chan)
 
        clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
 
+       /* Ensure the last byte(s) are drained before disabling the channel */
+       if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags))
+               dwc_chan_resume(dwc, true);
+
        dwc_chan_pause(dwc, true);
 
        dwc_chan_disable(dw, dwc);

Phil, I know it's been 3 years since you investigated this issue, but
do you still have access to the script reproducing the issue? Even
better, do you still have the hardware to test?

Thanks,
Miquèl

Re: [PATCH v2 5/8] dma: dw: Avoid partial transfers

[Andy Shevchenko]

On Thu, Feb 24, 2022 at 05:30:09PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Wed, 23 Feb 2022 15:35:58
> +0200:
> > On Tue, Feb 22, 2022 at 11:34:34AM +0100, Miquel Raynal wrote:

...

> > It seems the logic in the ->terminate_all() is broken and we actually need
> > to resume channel first (possibly conditionally, if it was suspended), then
> > pause it and disable and resume again.
> > 
> > The problem with ->terminate_all() is that it has no knowledge if it has
> > been called on paused channel (that's why it has to pause channel itself).
> > The pause on termination is required due to some issues in early steppings
> > of iDMA 32-bit hardware implementations.
> > 
> > If my theory is correct, the above change should fix the issues you see.
> 
> I don't have access to these datasheets so I will believe your words
> and try to apply Andy's solution. I ended up with the following fix,
> hopefully I got it right:
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 48cdefe997f1..59822664d8ec 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -865,6 +865,10 @@ static int dwc_terminate_all(struct dma_chan *chan)
>  
>         clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
>  
> +       /* Ensure the last byte(s) are drained before disabling the channel */
> +       if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags))
> +               dwc_chan_resume(dwc, true);
> +
>         dwc_chan_pause(dwc, true);
>  
>         dwc_chan_disable(dw, dwc);

Yes, this is good enough PoC. Needs to be tested, thanks!

> Phil, I know it's been 3 years since you investigated this issue, but
> do you still have access to the script reproducing the issue? Even
> better, do you still have the hardware to test?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 6/8] dma: dw: Add RZN1 compatible

[Miquel Raynal]

The Renesas RZN1 DMA IP is very close to the original DW DMA IP, a DMA
routeur has been introduced to handle the wiring options that have been
added.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 246118955877..47f2292dba98 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -137,6 +137,7 @@ static void dw_shutdown(struct platform_device *pdev)
 #ifdef CONFIG_OF
 static const struct of_device_id dw_dma_of_id_table[] = {
 	{ .compatible = "snps,dma-spear1340", .data = &dw_dma_chip_pdata },
+	{ .compatible = "renesas,rzn1-dma", .data = &dw_dma_chip_pdata },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dw_dma_of_id_table);
-- 
2.27.0

Re: [PATCH v2 6/8] dma: dw: Add RZN1 compatible

[Geert Uytterhoeven]

Hi Miquel,

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> The Renesas RZN1 DMA IP is very close to the original DW DMA IP, a DMA
> routeur has been introduced to handle the wiring options that have been

router

> added.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -137,6 +137,7 @@ static void dw_shutdown(struct platform_device *pdev)
>  #ifdef CONFIG_OF
>  static const struct of_device_id dw_dma_of_id_table[] = {
>         { .compatible = "snps,dma-spear1340", .data = &dw_dma_chip_pdata },
> +       { .compatible = "renesas,rzn1-dma", .data = &dw_dma_chip_pdata },

I don't like all these copies of dw_dma_chip_pdata, due to it being
defined in drivers/dma/dw/internal.h, but that's a pre-existing problem.

>         {}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 7/8] ARM: dts: r9a06g032: Add the two DMA nodes

[Miquel Raynal]

Describe the two DMA controllers available on this SoC.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index db657224688a..640c3eb4bbcd 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -184,6 +184,36 @@ nand_controller: nand-controller@40102000 {
 			status = "disabled";
 		};
 
+		dma0: dma-controller@40104000 {
+			compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
+			reg = <0x40104000 0x1000>;
+			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "hclk";
+			clocks = <&sysctrl R9A06G032_HCLK_DMA0>;
+			dma-channels = <8>;
+			dma-requests = <16>;
+			dma-masters = <1>;
+			#dma-cells = <3>;
+			block_size = <0xfff>;
+			data_width = <3>;
+			status = "disabled";
+		};
+
+		dma1: dma-controller@40105000 {
+			compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
+			reg = <0x40105000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "hclk";
+			clocks = <&sysctrl R9A06G032_HCLK_DMA1>;
+			dma-channels = <8>;
+			dma-requests = <16>;
+			dma-masters = <1>;
+			#dma-cells = <3>;
+			block_size = <0xfff>;
+			data_width = <3>;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@44101000 {
 			compatible = "arm,gic-400", "arm,cortex-a7-gic";
 			interrupt-controller;
-- 
2.27.0

Re: [PATCH v2 7/8] ARM: dts: r9a06g032: Add the two DMA nodes

[Geert Uytterhoeven]

Hi Miquel,

On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Describe the two DMA controllers available on this SoC.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -184,6 +184,36 @@ nand_controller: nand-controller@40102000 {
>                         status = "disabled";
>                 };
>
> +               dma0: dma-controller@40104000 {
> +                       compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
> +                       reg = <0x40104000 0x1000>;
> +                       interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> +                       clock-names = "hclk";
> +                       clocks = <&sysctrl R9A06G032_HCLK_DMA0>;

power-domains?

> +                       dma-channels = <8>;
> +                       dma-requests = <16>;
> +                       dma-masters = <1>;
> +                       #dma-cells = <3>;

<4>? The dmamux bindings say:

+      The first four cells are dedicated to the master DMA
controller. The fifth
+      cell gives the DMA mux bit index that must be set starting from 0. The
+      sixth cell gives the binary value that must be written there, ie. 0 or 1.

> +                       block_size = <0xfff>;
> +                       data_width = <3>;
> +                       status = "disabled";
> +               };

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 7/8] ARM: dts: r9a06g032: Add the two DMA nodes

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:54:20 +0100:

> Hi Miquel,
> 
> On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Describe the two DMA controllers available on this SoC.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -184,6 +184,36 @@ nand_controller: nand-controller@40102000 {
> >                         status = "disabled";
> >                 };
> >
> > +               dma0: dma-controller@40104000 {
> > +                       compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
> > +                       reg = <0x40104000 0x1000>;
> > +                       interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clock-names = "hclk";
> > +                       clocks = <&sysctrl R9A06G032_HCLK_DMA0>;  
> 
> power-domains?
> 
> > +                       dma-channels = <8>;
> > +                       dma-requests = <16>;
> > +                       dma-masters = <1>;
> > +                       #dma-cells = <3>;  
> 
> <4>? The dmamux bindings say:
> 
> +      The first four cells are dedicated to the master DMA
> controller. The fifth
> +      cell gives the DMA mux bit index that must be set starting from 0. The
> +      sixth cell gives the binary value that must be written there, ie. 0 or 1.

The DMAC bindings had initially 3 cells, and then received a fourth
optional one. We do not need it here, that's why I'm keeping #dma-cells
to 3.

But on the mux side, I don't want to deal with the presence or absence
of the optional cell so I assumed we would always request 4 cells for
the DMAC to be on the safe side.

Is this assumption wrong?

Thanks,
Miquèl

Re: [PATCH v2 7/8] ARM: dts: r9a06g032: Add the two DMA nodes

[Geert Uytterhoeven]

Hi Miquel,

On Wed, Feb 23, 2022 at 6:14 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:54:20 +0100:
> > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > > Describe the two DMA controllers available on this SoC.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >
> > Thanks for your patch!
> >
> > > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > > @@ -184,6 +184,36 @@ nand_controller: nand-controller@40102000 {
> > >                         status = "disabled";
> > >                 };
> > >
> > > +               dma0: dma-controller@40104000 {
> > > +                       compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
> > > +                       reg = <0x40104000 0x1000>;
> > > +                       interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> > > +                       clock-names = "hclk";
> > > +                       clocks = <&sysctrl R9A06G032_HCLK_DMA0>;
> >
> > power-domains?
> >
> > > +                       dma-channels = <8>;
> > > +                       dma-requests = <16>;
> > > +                       dma-masters = <1>;
> > > +                       #dma-cells = <3>;
> >
> > <4>? The dmamux bindings say:
> >
> > +      The first four cells are dedicated to the master DMA
> > controller. The fifth
> > +      cell gives the DMA mux bit index that must be set starting from 0. The
> > +      sixth cell gives the binary value that must be written there, ie. 0 or 1.
>
> The DMAC bindings had initially 3 cells, and then received a fourth
> optional one. We do not need it here, that's why I'm keeping #dma-cells
> to 3.
>
> But on the mux side, I don't want to deal with the presence or absence
> of the optional cell so I assumed we would always request 4 cells for
> the DMAC to be on the safe side.
>
> Is this assumption wrong?

OK.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 8/8] ARM: dts: r9a06g032: Describe the DMA router

[Miquel Raynal]

There is a dmamux on this SoC which allows picking two different sources
for a single DMA request.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 640c3eb4bbcd..0eb12c3d9cfd 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -59,6 +59,13 @@ ext_rtc_clk: extrtcclk {
 		clock-frequency = <0>;
 	};
 
+	dmamux: dma-router {
+		compatible = "renesas,rzn1-dmamux";
+		#dma-cells = <6>;
+		dma-requests = <32>;
+		dma-masters = <&dma0 &dma1>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.27.0