Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

[Geert Uytterhoeven]

Hi Prabhakar,

On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > is almost identical to one found on the RZ/G2L SoC with below differences,
> > * Additional BUS error interrupt
> > * Additional ECCRAM error interrupt
> > * Has additional mask control registers for NMI/IRQ/TINT
> >
> > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Thanks for your patch!

> > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > @@ -23,6 +23,7 @@ properties:
> >    compatible:
> >      items:
> >        - enum:
> > +          - renesas,r9a07g043f-irqc   # RZ/Five
> >            - renesas,r9a07g043u-irqc   # RZ/G2UL
> >            - renesas,r9a07g044-irqc    # RZ/G2{L,LC}
> >            - renesas,r9a07g054-irqc    # RZ/V2L
> > @@ -88,6 +89,12 @@ properties:
> >        - description: GPIO interrupt, TINT30
> >        - description: GPIO interrupt, TINT31
> >        - description: Bus error interrupt
> > +      - description: ECCRAM0 TIE1 interrupt

ECCRAM0 1bit error interrupt?

> > +      - description: ECCRAM0 TIE2 interrupt

ECCRAM0 2bit error interrupt?

> > +      - description: ECCRAM0 overflow interrupt

ECCRAM0 error overflow interrupt?

> > +      - description: ECCRAM1 TIE1 interrupt
> > +      - description: ECCRAM1 TIE2 interrupt
> > +      - description: ECCRAM1 overflow interrupt

Likewise.

> >    interrupt-names:
> >      minItems: 41
> > @@ -134,6 +141,12 @@ properties:
> >        - const: tint30
> >        - const: tint31
> >        - const: bus-err
> > +      - const: eccram0-tie1
> > +      - const: eccram0-tie2
> > +      - const: eccram0-ovf
> > +      - const: eccram1-tie1
> > +      - const: eccram1-tie2
> > +      - const: eccram1-ovf

Why not use the naming from the docs (all 6 include "ti")?
EC7TIE1_0, EC7TIE2_0, EC7TIOVF_0, EC7TIE1_1, EC7TIE2_1, EC7TIOVF_1
=> ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?

> I think the restrictions already in the file become incorrect with this
> patch:
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - renesas,r9a07g043u-irqc
>               - renesas,r9a08g045-irqc
>     then:
>       properties:
>         interrupts:
>           minItems: 42
>         interrupt-names:
>           minItems: 42
>       required:
>         - interrupt-names
>
> This used to require all 42 interrupts for the two compatibles here
> and at least the first 41 otherwise. Now you've increased the number of
> interrupts to 48 thereby removing the upper limits on the existing
> devices.

I'm gonna repeat (and extend) my question from [1]: How come we thought
RZ/G2L and RZ/V2L do not have the bus error and ECCRAM interrupts?
Looks like most of the conditional handling can be removed (see below).

> Given the commit message, I figure that providing 48 interrupts for
> (at least some of) those devices would be incorrect?

Looks like all of RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five support
all 48 interrupts.  RZ/G3S lacks the final three for ECCRAM1.

[1] "Re: [PATCH v3 8/9] dt-bindings: interrupt-controller:
renesas,rzg2l-irqc: Document RZ/G3S"
https://lore.kernel.org/r/CAMuHMdX88KRnvJchUwrWcgmPooAESOT2492Nr1Z_5UMng3q__Q@xxxxxxxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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