Re: [PATCH 1/2] dt-bindings: soc: renesas: renesas,rzg2l-sysc: Document RZ/Five SoC

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Prabhakar,

On Fri, Jul 22, 2022 at 12:21 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, Jul 22, 2022 at 10:31 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Fri, Jul 22, 2022 at 12:15 AM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > Document RZ/Five (R9A07G043) SYSC bindings. SYSC block found on the
> > > RZ/Five SoC is almost identical to one found on the RZ/G2L (and alike)
> > > SoC's. To differentiate RZ/G2UL from RZ/Five, "-rzfive" is included in
> > > the compatible string for the RZ/Five SoC as there are no interrupts
> > > from the SYSC block to the core.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > ---
> > >  .../soc/renesas/renesas,rzg2l-sysc.yaml       | 56 +++++++++++++------
> > >  1 file changed, 39 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> > > index ce2875c89329..bdaf05f8b29b 100644
> > > --- a/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> > > +++ b/Documentation/devicetree/bindings/soc/renesas/renesas,rzg2l-sysc.yaml
> > > @@ -20,35 +20,57 @@ description:
> > >  properties:
> > >    compatible:
> > >      enum:
> > > -      - renesas,r9a07g043-sysc # RZ/G2UL
> > > -      - renesas,r9a07g044-sysc # RZ/G2{L,LC}
> > > -      - renesas,r9a07g054-sysc # RZ/V2L
> > > +      - renesas,r9a07g043-rzfive-sysc # RZ/Five
> >
> > renesas,r9a07g043f-sysc?
> >
> Agreed.
>
> > But I'm wondering if we really need a different compatible value?
> > It looks like both blocks differ only in external wiring, so if
> > anything needs to be handled differently (the removed/added registers
> > are related to CPU topology), that can be inferred from the system
> > topology (or even #ifdef CONFIG_{ARM64,RISCV} ;-)
> >
> Good point, but I wonder if we would end up in too many #ifdef
> CONFIG_{ARM64,RISCV} checks. If thats OK I will stick with
> "renesas,r9a07g043-sysc"
>
> > > +      - renesas,r9a07g043-sysc        # RZ/G2UL
> > > +      - renesas,r9a07g044-sysc        # RZ/G2{L,LC}
> > > +      - renesas,r9a07g054-sysc        # RZ/V2L
> > >
> > >    reg:
> > >      maxItems: 1
> > >
> > > -  interrupts:
> > > -    items:
> > > -      - description: CA55/CM33 Sleep/Software Standby Mode request interrupt
> > > -      - description: CA55 Software Standby Mode release request interrupt
> > > -      - description: CM33 Software Standby Mode release request interrupt
> > > -      - description: CA55 ACE Asynchronous Bridge Master/Slave interface deny request interrupt
> > > +  interrupts: true
> > >
> > > -  interrupt-names:
> > > -    items:
> > > -      - const: lpm_int
> > > -      - const: ca55stbydone_int
> > > -      - const: cm33stbyr_int
> > > -      - const: ca55_deny
> > > +  interrupt-names: true
> > >
> > >  required:
> > >    - compatible
> > >    - reg
> > > -  - interrupts
> > > -  - interrupt-names
> > >
> > >  additionalProperties: false
> > >
> > > +allOf:
> > > +  - if:
> > > +      not:
> > > +        properties:
> > > +          compatible:
> > > +            contains:
> > > +              enum:
> > > +                - renesas,r9a07g043-rzfive-sysc
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          items:
> > > +            - description: CA55/CM33 Sleep/Software Standby Mode request interrupt
> > > +            - description: CA55 Software Standby Mode release request interrupt
> > > +            - description: CM33 Software Standby Mode release request interrupt
> > > +            - description: CA55 ACE Asynchronous Bridge Master/Slave interface deny request interrupt
> > > +
> > > +        interrupt-names:
> > > +          items:
> > > +            - const: lpm_int
> > > +            - const: ca55stbydone_int
> > > +            - const: cm33stbyr_int
> > > +            - const: ca55_deny
> > > +
> > > +      required:
> > > +        - interrupts
> > > +        - interrupt-names
> > > +
> > > +    else:
> > > +      properties:
> > > +        interrupts: false
> > > +        interrupt-names: false
> >
> > Do all interrupts{,-names} have to be moved?
> > Wouldn't it be sufficient to just have
> >
> Agreed.
>
> >     if [...]
> >     then:
> >         required:
> >           - interrupts
> >           - interrupt-names
> >     else:
> >         properties:
> >             interrupts: false
> >             interrupt-names: false
> >
> > ?
> >
> > But again, without a new compatible value, you could just make
> > interrupts{,-names} not required?
> >
> You mean we just make it optional for all the SoC's?

Indeed.

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