RE: [PATCH v5 01/14] ASoC: dt-bindings: sound: Add DT binding for RZ/G3E sound

[John Madieu]

Hi Krzysztof,

Thanks for your review.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Freitag, 17. April 2026 10:27
> To: John Madieu <john.madieu@xxxxxxxxx>
> Subject: Re: [PATCH v5 01/14] ASoC: dt-bindings: sound: Add DT binding for
> RZ/G3E sound
> 
> On Wed, Apr 15, 2026 at 12:47:18PM +0000, John Madieu wrote:
> > Add a standalone device tree binding for the Renesas RZ/G3E
> > (R9A09G047) sound controller.
> >
> > The RZ/G3E sound IP is based on R-Car Sound but differs in several ways:
> > - Uses unprefixed sub-node names (ssi, ssiu, src, dvc, mix, ctu) instead
> >   of R-Car's rcar_sound,xxx prefixed names.
> > - Supports up to 5 DMA controllers per direction, allowing multiple DMA
> >   entries with repeated channel names in SSIU, SRC and DVC sub-nodes.
> > - Has 47 clocks including per-SSI ADG clocks (adg.ssi.0-9), SCU clocks
> >   (scu, scu_x2, scu_supply), SSIF supply clock, AUDMAC peri-peri clock,
> >   and ADG clock.
> > - Has 14 reset lines including SCU, ADG and AUDMAC peri-peri resets.
> > - SSI operates exclusively in BUSIF mode.
> >
> > These differences make the RZ/G3E binding incompatible with the
> > existing renesas,rsnd.yaml, so it is added as a separate standalone
> > binding with its own $ref to dai-common.yaml.
> >
> > Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> 
> DCO/author mismatch.
> 
> Did you run checkpatch?
> 

Will take care of this in v6. 

> > ---
> >
> > Changes:
> >
> > v5:
> >  - Drop the two-patch rsnd.yaml split approach from v4.
> >    Replace with a single self-contained standalone binding that does
> >    not touch renesas,rsnd.yaml at all.
> >  - Remove select: false, redundant blanket properties (compatible: true,
> >    reg: true, etc.) and pointless patternProperties per Krzystof's
> > review
> >  - Add missing #clock-cells and #sound-dai-cells constraints
> >  - Add hardware description text instead of "Binding for ..." phrasing
> >  - Move G3E-specific DMA comment into the binding itself rather than
> >    relying on a shared schema
> >  - Use unprefixed sub-node names (ssi, ssiu, src, dvc, mix, ctu) to
> >    reflect the actual RZ/G3E DT binding
> >
> > v4: No changes
> > v3: No changes
> > v2:
> >  - Introduce RZ/G3E sound binding as a standalone schema
> >
> >  .../sound/renesas,r9a09g047-sound.yaml        | 770 ++++++++++++++++++
> >  1 file changed, 770 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/renesas,r9a09g047-sound.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/renesas,r9a09g047-sound.yaml
> > b/Documentation/devicetree/bindings/sound/renesas,r9a09g047-sound.yaml
> > new file mode 100644
> > index 000000000000..b7e5348636bb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/renesas,r9a09g047-sound.
> > +++ yaml
> > @@ -0,0 +1,770 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/sound/renesas,r9a09g047-sound.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas RZ/G3E Sound Controller
> > +
> > +maintainers:
> > +  - Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> > +  - John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> > +
> > +description:
> > +  The RZ/G3E (R9A09G047) sound controller is based on R-Car Sound IP
> > +  with extended DMA channel support (up to 5 DMACs per direction),
> > +  additional clock domains (47 clocks including per-SSI ADG clocks),
> > +  and additional reset lines (14 including SCU, ADG and Audio DMAC
> > +  peri-peri resets). SSI operates exclusively in BUSIF mode with
> > +  2-4 BUSIF channels per SSI.
> > +
> > +allOf:
> > +  - $ref: dai-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a09g047-sound
> > +
> > +  reg:
> > +    maxItems: 5
> > +
> > +  reg-names:
> > +    items:
> > +      - const: scu
> > +      - const: adg
> > +      - const: ssiu
> > +      - const: ssi
> > +      - const: audmapp
> > +
> > +  "#sound-dai-cells":
> > +    enum: [0, 1]
> 
> Why is this flexible? That's a defined device meaning you have one XOR
> more DAIs. Not "1 and more".
> 

The IP exposes ten independent SSI interfaces, and a board can
reasonably wire either a single SSI to one codec or several SSIs
to several codecs. The cells value follows that wiring: 0 when
the phandle is `<&rcar_sound>` for the single-DAI case, 1 when
it is `<&rcar_sound N>` selecting a specific DAI index.

So the two values describe two legitimate board topologies, both
supported by the silicon. Would it be acceptable to keep the enum
and add a description making the two-mode meaning explicit, or
would you prefer a different shape ?

> > +
> > +  "#clock-cells":
> > +    const: 0
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  clocks:
> > +    maxItems: 47
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ssi-all
> > +      - const: ssi.9
> 
> 
> Use consistently -
> 

Agreed, I'll switch to hyphens for all indexed entries
in both lists (ssi-0..9, src-0..9, mix-0..1, ctu-0..7,
dvc-0..1, adg-ssi-0..9).

> > +      - const: ssi.8
> > +      - const: ssi.7
> > +      - const: ssi.6
> > +      - const: ssi.5
> > +      - const: ssi.4
> > +      - const: ssi.3
> > +      - const: ssi.2
> > +      - const: ssi.1
> > +      - const: ssi.0
> > +      - const: src.9
> > +      - const: src.8
> > +      - const: src.7
> > +      - const: src.6
> > +      - const: src.5
> > +      - const: src.4
> > +      - const: src.3
> > +      - const: src.2
> > +      - const: src.1
> > +      - const: src.0
> > +      - const: mix.1
> > +      - const: mix.0
> > +      - const: ctu.1
> > +      - const: ctu.0
> > +      - const: dvc.0
> > +      - const: dvc.1
> > +      - const: clk_a
> 
> And here as well
> 
> name "clk_a" is half useless, because this cannot be anything else than
> clk, thus basically you said "a". What is a?
> 

Those names come from the silicon pinout: the IP has three external
master-clock input pins named AUDIO_CLKA, AUDIO_CLKB, AUDIO_CLKC,
plus an internal clock named CLKI in the datasheet. The short form
was chosen so a schematic reviewer sees the same label in the DT as
on the datasheet / reference design.

Would you prefer to keep the short form matching the pin labels,
Or would something like `audio-clka` / `audio-clkb` / `audio-clkc` /
`audio-clki` be more in line with current binding conventions?

For information, these clocks are fed to the CPG and documented
here [1].

[1] https://lore.kernel.org/all/20260402163126.12135-2-john.madieu.xa@xxxxxxxxxxxxxx/
> 
> > +      - const: clk_b
> > +      - const: clk_c
> > +      - const: clk_i
> > +      - const: ssif_supply
> > +      - const: scu
> > +      - const: scu_x2
> > +      - const: scu_supply
> > +      - const: adg.ssi.9
> > +      - const: adg.ssi.8
> > +      - const: adg.ssi.7
> > +      - const: adg.ssi.6
> > +      - const: adg.ssi.5
> > +      - const: adg.ssi.4
> > +      - const: adg.ssi.3
> > +      - const: adg.ssi.2
> > +      - const: adg.ssi.1
> > +      - const: adg.ssi.0
> > +      - const: audmapp
> > +      - const: adg
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 14
> > +
> > +  reset-names:
> > +    items:
> > +      - const: ssi-all
> > +      - const: ssi.9
> 
> s/./-/
> 
> > +      - const: ssi.8
> > +      - const: ssi.7
> > +      - const: ssi.6
> > +      - const: ssi.5
> > +      - const: ssi.4
> > +      - const: ssi.3
> > +      - const: ssi.2
> > +      - const: ssi.1
> > +      - const: ssi.0
> > +      - const: scu
> > +      - const: adg
> > +      - const: audmapp
> > +
> > +  clock-frequency:
> > +    description: Audio clock output frequency.
> 
> Drop, this is a legacy property, not really allowed for new devices which
> are not I2C buses.
> 
> > +
> > +  clkout-lr-asynchronous:
> 
> Missing vendor prefix.
> 

I'd rather drop the property as it was part of the old
rsnd.yaml binding, which is not needed for RZ/G3E.

> > +    description: audio_clkoutn is asynchronous with lr-clock.
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +
> > +  dvc:
> 
> Mixing nodes with and without addressing is discouraged. Why do you have
> such mixup?
> 
> This node looks empty, so just define dvc-[01] directly. Same for other
> cases.

dvc, mix, ctu, src, ssi, ssiu group the six IP blocks inside the
sound controller. Each block has a fixed count of instances
dictated by the silicon (2 DVC, 2 MIX, 8 CTU, 10 SRC, 10 SSI,
up to 28 SSIU) identified by an integer index, not a register
offset.

These aren't only grouping convenience, they're also the on/off
switch for each block's instances (the driver counts their children
to decide which instances to enable). Some of them look empty on
this IP because CTU and MIX have no per-instance IRQ / DMA / pinmux;
They are fed by the SRC DMA chain, while DVC, SRC, SSI and SSIU do
Carry per-instance properties on their children.

Keeping the schema symmetric across the six blocks (anchor + indexed
children) makes it obvious at review time which block-instances a
given board enables. Flattening would mix `dvc-[01]`, `mix-[01]`,
`ctu-[0-7]`, `src-[0-9]`, `ssi-[0-9]`, `ssiu-[0-9]+` at the top level
of the sound node, alongside `ports`, `reg`, `clocks`, etc., which I
found harder to read.

Is the anchored shape acceptable if I make the mix / ctu anchors
require at least one child (so they can't be written as literally
empty), or would you still prefer flattening?

> 
> > +    type: object
> > +    patternProperties:
> > +      "^dvc-[0-1]$":
> > +        type: object
> > +        additionalProperties: false
> > +        properties:
> > +          dmas:
> > +            maxItems: 5
> > +          dma-names:
> > +            maxItems: 5
> > +            allOf:
> > +              - items:
> > +                  enum:
> > +                    - tx
> > +        required:
> > +          - dmas
> > +          - dma-names
> > +    additionalProperties: false
> > +
> > +  mix:
> > +    type: object
> > +    patternProperties:
> > +      "^mix-[0-1]$":
> > +        type: object
> > +        additionalProperties: false
> > +    additionalProperties: false
> > +
> > +  ctu:
> > +    type: object
> > +    patternProperties:
> > +      "^ctu-[0-7]$":
> > +        type: object
> > +        additionalProperties: false
> > +    additionalProperties: false
> > +
> > +  src:
> > +    type: object
> > +    patternProperties:
> > +      "^src-[0-9]$":
> > +        type: object
> > +        additionalProperties: false
> > +        properties:
> > +          interrupts:
> > +            maxItems: 1
> > +          dmas:
> > +            maxItems: 10
> > +          dma-names:
> > +            maxItems: 10
> > +            allOf:
> > +              - items:
> > +                  enum:
> > +                    - tx
> > +                    - rx
> > +    additionalProperties: false
> > +
> > +  ssiu:
> > +    type: object
> > +    patternProperties:
> > +      "^ssiu-[0-9]+$":
> > +        type: object
> > +        additionalProperties: false
> > +        properties:
> > +          dmas:
> > +            maxItems: 10
> > +          dma-names:
> > +            maxItems: 10
> > +            allOf:
> > +              - items:
> > +                  enum:
> > +                    - tx
> > +                    - rx
> > +        required:
> > +          - dmas
> > +          - dma-names
> > +    additionalProperties: false
> > +
> > +  ssi:
> > +    type: object
> > +    patternProperties:
> > +      "^ssi-[0-9]$":
> > +        type: object
> > +        additionalProperties: false
> > +        properties:
> > +          interrupts:
> > +            maxItems: 1
> > +          dmas: true
> > +          dma-names: true
> > +          shared-pin:
> > +            description: Shared clock pin.
> > +            $ref: /schemas/types.yaml#/definitions/flag
> > +        required:
> > +          - interrupts
> > +    additionalProperties: false
> > +
> > +  port:
> > +    $ref: audio-graph-port.yaml#/definitions/port-base
> > +    unevaluatedProperties: false
> > +    patternProperties:
> > +      "^endpoint(@[0-9a-f]+)?$":
> > +        $ref: audio-graph-port.yaml#/definitions/endpoint-base
> > +        properties:
> > +          playback:
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +          capture:
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +        unevaluatedProperties: false
> > +
> > +patternProperties:
> > +  '^dai(@[0-9a-f]+)?$':
> 
> Why node addressing is optional?
> 
> > +    type: object
> > +    patternProperties:
> > +      "^dai([0-9]+)?$":
> 
> You did not verify your DTS, you have warnings here. And I really do not
> understand why dai@0 has dai@0 again.
> 

RZ/G3E boards only connect to their codec through audio-graph
`ports`, so I'll drop the `dai` wrapper entirely. The nested
`dai / daiN` pattern was there to describe the legacy
simple-card style from older Renesas platforms.

> 
> > +        type: object
> > +        additionalProperties: false
> > +        properties:
> > +          playback:
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +          capture:
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +        anyOf:
> > +          - required:
> > +              - playback
> > +          - required:
> > +              - capture
> > +    additionalProperties: false
> > +
> > +  'ports(@[0-9a-f]+)?$':
> 
> Why ports even have addferssing?
> 

There's only one ports container per sound node; I'll drop the @N.

> > +    $ref: audio-graph-port.yaml#/definitions/port-base
> 
> So this is port base or ports? How port-base could have one more port as a
> child? Open the port-base definition and look there.
> 
> > +    unevaluatedProperties: false
> > +    patternProperties:
> > +      '^port(@[0-9a-f]+)?$':
> 
> No, you must define exactly what the ports are.
> 

Agreed.

> > +        $ref: "#/properties/port"
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - reset-names
> 
Regards,
John