Received-SPF: pass (google.com: domain of linux-kernel+bounces-44663-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240129151618.90922-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20240129151618.90922-2-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20240129-magical-unclaimed-e725e2491ccb@spud> <CAMuHMdVhXh_Cd8m00xfVRB9JA8Mfb9+qccu94iVpUMS2z5kmUQ@mail.gmail.com>
In-Reply-To: <CAMuHMdVhXh_Cd8m00xfVRB9JA8Mfb9+qccu94iVpUMS2z5kmUQ@mail.gmail.com>
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date: Tue, 30 Jan 2024 12:59:19 +0000
Message-ID: <CA+V-a8v0tdr-xh__5rcK=xL-yYG1qLtSrAUjPcS_-ZVYy8p9pQ@mail.gmail.com>
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller:
 renesas,rzg2l-irqc: Document RZ/Five SoC
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Conor Dooley <conor@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Magnus Damm <magnus.damm@gmail.com>, linux-renesas-soc@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-riscv@lists.infradead.org, 
	linux-kernel@vger.kernel.org, Biju Das <biju.das.jz@bp.renesas.com>, 
	Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Geert,

Thank you for the review.

On Tue, Jan 30, 2024 at 11:13=E2=80=AFAM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jan 29, 2024 at 6:30=E2=80=AFPM Conor Dooley <conor@kernel.org> w=
rote:
> > On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Fiv=
e SoC
> > > is almost identical to one found on the RZ/G2L SoC with below differe=
nces,
> > > * 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@bp.renesas.com=
>
>
> 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?
>
OK.

> > > +      - description: ECCRAM0 TIE2 interrupt
>
> ECCRAM0 2bit error interrupt?
>
OK.
> > > +      - description: ECCRAM0 overflow interrupt
>
> ECCRAM0 error overflow interrupt?
>
> > > +      - description: ECCRAM1 TIE1 interrupt
> > > +      - description: ECCRAM1 TIE2 interrupt
> > > +      - description: ECCRAM1 overflow interrupt
>
> Likewise.
>
OK.

> > >    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
> =3D> ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?
>
Agreed.

> > 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?
>
Hmm not sure how this was missed earlier.

> 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.
>
Agreed for RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five, but for RZ/G3S it
becomes tricky the interrupts for ECCRAM0/1 are combined hence they
have just 3 interrupts. How do you propose the above interrupt naming?

> [1] "Re: [PATCH v3 8/9] dt-bindings: interrupt-controller:
> renesas,rzg2l-irqc: Document RZ/G3S"
> https://lore.kernel.org/r/CAMuHMdX88KRnvJchUwrWcgmPooAESOT2492Nr1Z_5UMng3=
q__Q@mail.gmail.com
>
Sorry I missed this thread.

Cheers,
Prabhakar