[PATCH v2 0/3] dmaengine: sh: rz-dmac: add r7s72100 support

[PATCH v2 0/3] dmaengine: sh: rz-dmac: add r7s72100 support

[Wolfram Sang]

Changes since v1:
* added tags to patches 1 and 3 (Thanks Biju, Claudiu, and Philipp!)
* used A1H instead of A1L (Thanks, Geert!)
* reworded comments and descriptions to use a more generic "RZ DMA"
  term without mentioning all the SoCs in patches 2 and 3.

Old cover-letter:

When activating good old Genmai board for regression testing, I found
out that not much is needed to activate the DMA controller for A1H.
Which makes sense, because the driver was initially written for this
SoC. Let it come home ;)

Patch 1 is a generic fix. The other patches are the actual enablement.
A branch with DTS additions for MMCIF can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/genmai-upstreaming

These will be upstreamed once the driver parts are in next. Adding SDHI
is still WIP because RZ/A1L usage exposes a SDHI driver bug. So much for
the value of regression testing...

Wolfram Sang (3):
  dmaengine: sh: rz-dmac: handle configs where one address is zero
  dt-bindings: dma: rz-dmac: Document RZ/A1H SoC
  dmaengine: sh: rz-dmac: add r7s72100 support

 .../bindings/dma/renesas,rz-dmac.yaml         | 27 +++++++++++++------
 drivers/dma/sh/Kconfig                        |  8 +++---
 drivers/dma/sh/rz-dmac.c                      | 27 ++++++++++---------
 3 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.45.2

[PATCH v2 2/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Wolfram Sang]

Document the Renesas RZ/A1H DMAC block. This one does not require clocks
and resets, so update the bindings accordingly. Introduce a generic name
in the header to make future additions easier.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 .../bindings/dma/renesas,rz-dmac.yaml         | 27 +++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml b/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
index ca24cf48769f..83d79b7d85d2 100644
--- a/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
+++ b/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
@@ -4,18 +4,16 @@
 $id: http://devicetree.org/schemas/dma/renesas,rz-dmac.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Renesas RZ/{G2L,G2UL,V2L} DMA Controller
+title: Renesas RZ DMA Controller
 
 maintainers:
   - Biju Das <biju.das.jz@bp.renesas.com>
 
-allOf:
-  - $ref: dma-controller.yaml#
-
 properties:
   compatible:
     items:
       - enum:
+          - renesas,r7s72100-dmac # RZ/A1H
           - renesas,r9a07g043-dmac # RZ/G2UL and RZ/Five
           - renesas,r9a07g044-dmac # RZ/G2{L,LC}
           - renesas,r9a07g054-dmac # RZ/V2L
@@ -93,13 +91,26 @@ required:
   - reg
   - interrupts
   - interrupt-names
-  - clocks
-  - clock-names
   - '#dma-cells'
   - dma-channels
   - power-domains
-  - resets
-  - reset-names
+
+allOf:
+  - $ref: dma-controller.yaml#
+
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - renesas,r7s72100-dmac
+    then:
+      required:
+        - clocks
+        - clock-names
+        - resets
+        - reset-names
 
 additionalProperties: false
 
-- 
2.45.2

Re: [PATCH v2 2/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Krzysztof Kozlowski]

On Tue, Oct 01, 2024 at 02:43:08PM +0200, Wolfram Sang wrote:
> Document the Renesas RZ/A1H DMAC block. This one does not require clocks
> and resets, so update the bindings accordingly. Introduce a generic name
> in the header to make future additions easier.

Does not require or does not have? What does it mean that device does
not require clocks? It has its own, internal clock oscillator? But then
how does it match with external clocks which are still apparently
supplied?

It looks like constrains are relaxed because of current driver
structure or current DTS...

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]


> Does not require or does not have? What does it mean that device does
> not require clocks? It has its own, internal clock oscillator? But then

You are right, it requires "clocks" but not "clock-names". Seems I got
carried away when removing the reset properties :(

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Oct 2, 2024 at 8:37 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Does not require or does not have? What does it mean that device does
> > not require clocks? It has its own, internal clock oscillator? But then
>
> You are right, it requires "clocks" but not "clock-names". Seems I got
> carried away when removing the reset properties :(

According to the documentation, there is no bit in a Standby Control
Register to control the DMAC clock.  The driver doesn't care about the
clock or its rate, so you can use P0 if you want.

Anyway:
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/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]


> According to the documentation, there is no bit in a Standby Control
> Register to control the DMAC clock.  The driver doesn't care about the
> clock or its rate, so you can use P0 if you want.

Would you prefer using 'p0' or leaving this patch as is?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Oct 4, 2024 at 9:57 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > According to the documentation, there is no bit in a Standby Control
> > Register to control the DMAC clock.  The driver doesn't care about the
> > clock or its rate, so you can use P0 if you want.
>
> Would you prefer using 'p0' or leaving this patch as is?

Leaving the patch as-is is fine for me.

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/3] dt-bindings: dma: rz-dmac: Document RZ/A1H SoC

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Oct 7, 2024 at 9:30 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Oct 4, 2024 at 9:57 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > > According to the documentation, there is no bit in a Standby Control
> > > Register to control the DMAC clock.  The driver doesn't care about the
> > > clock or its rate, so you can use P0 if you want.
> >
> > Would you prefer using 'p0' or leaving this patch as is?
>
> Leaving the patch as-is is fine for me.

Upon second thought: the clock would only be used by the PM Domain
code.  So without a clocks property, "power-domains = <&cpg_clocks>"
would not make any sense, and the power-domains property should be
made optional.  The pinctrl and irqc also don't have it.

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 0/3] dmaengine: sh: rz-dmac: add r7s72100 support

[Geert Uytterhoeven]

Hi Wolfram,

CC linux-mmc

On Tue, Oct 1, 2024 at 2:43 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> When activating good old Genmai board for regression testing, I found
> out that not much is needed to activate the DMA controller for A1H.
> Which makes sense, because the driver was initially written for this
> SoC. Let it come home ;)

[...]

> Adding SDHI
> is still WIP because RZ/A1L usage exposes a SDHI driver bug. So much for
> the value of regression testing...

I had completely forgotten about this, so I just ran into the same
issue while trying to enable more DMA support :-(

The SDHI callback does:

    static void renesas_sdhi_sys_dmac_dma_callback(void *arg)
    {
            ...

            wait_for_completion(&priv->dma_priv.dma_dataend);
            ...
    }

i.e. it assumes it is not called in atomic context.
On R-Car Gen2, that is true, as the R-Car DMAC IRQ thread does:

    static irqreturn_t rcar_dmac_isr_channel_thread(int irq, void *dev)
    {
            ...
            spin_unlock_irq(&chan->lock);
            dmaengine_desc_callback_invoke(&cb, NULL);
            spin_lock_irq(&chan->lock);
            ...
    }

On RZ/A1, the RZ DMAC driver uses virt-dma, and offloads this to the
vchan tasklet, which does:

    static void vchan_complete(struct tasklet_struct *t)
    {
            ...
            spin_unlock_irq(&vc->lock);

            dmaengine_desc_callback_invoke(&cb, &vd->tx_result);
            ...
    }

However, the tasklet runs in softirq context, causing:

    BUG: scheduling while atomic: ksoftirqd/0/8/0x00000100
    CPU: 0 UID: 0 PID: 8 Comm: ksoftirqd/0 Not tainted
6.12.0-rc7-rskrza1-08263-g3b9979a62f8e #885
    Hardware name: Generic R7S72100 (Flattened Device Tree)
    Call trace:
     unwind_backtrace from show_stack+0x10/0x14
     show_stack from dump_stack_lvl+0x34/0x54
     dump_stack_lvl from __schedule_bug+0x44/0x64
     __schedule_bug from __schedule+0x44/0x48c
     __schedule from schedule+0x28/0x44
     schedule from schedule_timeout+0x28/0xdc
     schedule_timeout from __wait_for_common+0x80/0x108
     __wait_for_common from renesas_sdhi_sys_dmac_dma_callback+0x58/0x84
     renesas_sdhi_sys_dmac_dma_callback from
dmaengine_desc_callback_invoke+0x6c/0x7c
     dmaengine_desc_callback_invoke from vchan_complete+0x118/0x13c
     vchan_complete from tasklet_action_common+0x64/0x90
     tasklet_action_common from handle_softirqs+0x164/0x1cc
     handle_softirqs from run_ksoftirqd+0x20/0x38

I am not sure if the SDHI driver or the RZ-DMAC driver (or virt-dma)
should be fixed, as the documentation[1] states:

     Note that callbacks will always be invoked from the DMA
     engines tasklet, never from interrupt context.

Thanks for your comments!

[1] https://elixir.bootlin.com/linux/v6.11.8/source/Documentation/driver-api/dmaengine/client.rst#L164

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 0/3] dmaengine: sh: rz-dmac: add r7s72100 support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]


> I am not sure if the SDHI driver or the RZ-DMAC driver (or virt-dma)
> should be fixed, as the documentation[1] states:
> 
>      Note that callbacks will always be invoked from the DMA
>      engines tasklet, never from interrupt context.

Back then, I had the impression that we can rework the SDHI SYSDMAC part
to not use a completion like the internal DMAC version does. But it has
been a while and I got completely side-tracked meanwhile.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]