Hi Rob,
Thanks for your feedback!
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 11 February 2025 22:27
> Subject: Re: [PATCH 3/7] dt-bindings: dma: rz-dmac: Document RZ/V2H(P) family of SoCs
>
> On Thu, Feb 06, 2025 at 10:03:04PM +0000, Fabrizio Castro wrote:
> > Document the Renesas RZ/V2H(P) family of SoCs DMAC block.
> > The Renesas RZ/V2H(P) DMAC is very similar to the one found on the
> > Renesas RZ/G2L family of SoCs, but there are some differences:
> > * It only uses one register area
> > * It only uses one clock
> > * It only uses one reset
> > * Instead of using MID/IRD it uses REQ NO/ACK NO
> > * It is connected to the Interrupt Control Unit (ICU)
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > ---
> > .../bindings/dma/renesas,rz-dmac.yaml | 152 +++++++++++++++---
> > 1 file changed, 127 insertions(+), 25 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
> b/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
> > index 82de3b927479..d4dd22432e49 100644
> > --- a/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
> > +++ b/Documentation/devicetree/bindings/dma/renesas,rz-dmac.yaml
> > @@ -11,19 +11,23 @@ maintainers:
> >
> > 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
> > - - renesas,r9a08g045-dmac # RZ/G3S
> > - - const: renesas,rz-dmac
> > + oneOf:
> > + - 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
> > + - renesas,r9a08g045-dmac # RZ/G3S
> > + - const: renesas,rz-dmac
> > +
> > + - const: renesas,r9a09g057-dmac # RZ/V2H(P)
> >
> > reg:
> > items:
> > - description: Control and channel register block
> > - description: DMA extended resource selector block
> > + minItems: 1
> >
> > interrupts:
> > maxItems: 17
> > @@ -52,6 +56,7 @@ properties:
> > items:
> > - description: DMA main clock
> > - description: DMA register access clock
> > + minItems: 1
> >
> > clock-names:
> > items:
> > @@ -61,14 +66,22 @@ properties:
> > '#dma-cells':
> > const: 1
> > description:
> > - The cell specifies the encoded MID/RID values of the DMAC port
> > - connected to the DMA client and the slave channel configuration
> > - parameters.
> > + For the RZ/A1H, RZ/Five, RZ/G2{L,LC,UL}, RZ/V2L, and RZ/G3S SoCs, the cell
> > + specifies the encoded MID/RID values of the DMAC port connected to the
> > + DMA client and the slave channel configuration parameters.
> > bits[0:9] - Specifies MID/RID value
> > bit[10] - Specifies DMA request high enable (HIEN)
> > bit[11] - Specifies DMA request detection type (LVL)
> > bits[12:14] - Specifies DMAACK output mode (AM)
> > bit[15] - Specifies Transfer Mode (TM)
> > + For the RZ/V2H(P) SoC the cell specifies the REQ NO, the ACK NO, and the
> > + slave channel configuration parameters.
> > + bits[0:9] - Specifies the REQ NO
> > + bits[10:16] - Specifies the ACK NO
> > + bit[17] - Specifies DMA request high enable (HIEN)
> > + bit[18] - Specifies DMA request detection type (LVL)
> > + bits[19:21] - Specifies DMAACK output mode (AM)
> > + bit[22] - Specifies Transfer Mode (TM)
> >
> > dma-channels:
> > const: 16
> > @@ -80,12 +93,29 @@ properties:
> > items:
> > - description: Reset for DMA ARESETN reset terminal
> > - description: Reset for DMA RST_ASYNC reset terminal
> > + minItems: 1
> >
> > reset-names:
> > items:
> > - const: arst
> > - const: rst_async
> >
> > + renesas,icu:
> > + description:
> > + On the RZ/V2H(P) SoC configures the ICU to which the DMAC is connected to.
> > + It must contain the phandle to the ICU, and the index of the DMAC as seen
> > + from the ICU (e.g. parameter k from register ICU_DMkSELy).
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + items:
> > + - items:
> > + - description: phandle to the ICU node.
> > + - description: The DMAC index.
> > + 4 for DMAC0
> > + 0 for DMAC1
> > + 1 for DMAC2
> > + 2 for DMAC3
> > + 3 for DMAC4
> > +
> > required:
> > - compatible
> > - reg
> > @@ -98,27 +128,62 @@ allOf:
> > - $ref: dma-controller.yaml#
> >
> > - if:
> > - not:
> > - properties:
> > - compatible:
> > - contains:
> > - enum:
> > - - renesas,r7s72100-dmac
> > + properties:
> > + compatible:
> > + contains:
> > + const: renesas,r9a09g057-dmac
> > then:
> > + properties:
> > + reg:
> > + maxItems: 1
> > + clocks:
> > + maxItems: 1
> > + resets:
> > + maxItems: 1
> > +
> > + clock-names: false
> > + reset-names: false
> > +
> > required:
> > - clocks
> > - - clock-names
> > - power-domains
> > + - renesas,icu
> > - resets
> > - - reset-names
> >
> > else:
> > - properties:
> > - clocks: false
> > - clock-names: false
> > - power-domains: false
> > - resets: false
> > - reset-names: false
> > + if:
>
> Please try to avoid nesting if/then/else. Not sure that's easy or not
> here. This diff is hard to read.
I think I can use 3 if statements as opposed to nesting them, and I'll have to
explicitly list the platforms in order to cover all of the cases.
It should be a lot more readable, and hopefully simpler to maintain.
I'll send something across soon for you to look at.
>
> > + not:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - renesas,r7s72100-dmac
> > + then:
> > + properties:
> > + reg:
> > + minItems: 2
> > + clocks:
> > + minItems: 2
> > + resets:
> > + minItems: 2
> > +
> > + renesas,icu: false
> > +
> > + required:
> > + - clocks
> > + - clock-names
> > + - power-domains
> > + - resets
> > + - reset-names
> > +
> > + else:
> > + properties:
> > + clocks: false
> > + clock-names: false
> > + power-domains: false
> > + resets: false
> > + reset-names: false
> > + renesas,icu: false
> >
> > additionalProperties: false
> >
> > @@ -164,3 +229,40 @@ examples:
> > #dma-cells = <1>;
> > dma-channels = <16>;
> > };
> > +
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/renesas-cpg-mssr.h>
> > +
> > + dmac0: dma-controller@11400000 {
> > + compatible = "renesas,r9a09g057-dmac";
>
> Is this example really different enough from the others to need it? I
> would drop it.
I think the new example can be dropped in v2.
Thank you.
Cheers,
Fab
>
> Rob
