linux-kernel - RE: [PATCH v2 2/5] dt-bindings: interrupt-controller: Add Renesas RZ/V2H(P) Interrupt Controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <TY3PR01MB12089C5DB18DE3865453E3055C27F2@TY3PR01MB12089.jpnprd01.prod.outlook.com>
Date: Wed, 9 Oct 2024 22:10:50 +0000
From: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
CC: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Magnus Damm <magnus.damm@...il.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>, Chris Paterson
	<Chris.Paterson2@...esas.com>, Biju Das <biju.das.jz@...renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: RE: [PATCH v2 2/5] dt-bindings: interrupt-controller: Add Renesas
 RZ/V2H(P) Interrupt Controller

Hi Geert,

Thanks for your feedback!

> From: Geert Uytterhoeven <geert@...ux-m68k.org>
> Sent: Friday, October 4, 2024 11:26 AM
> Subject: Re: [PATCH v2 2/5] dt-bindings: interrupt-controller: Add Renesas RZ/V2H(P) Interrupt
> Controller
> 
> Hi Fabrizio,
> 
> On Mon, Sep 30, 2024 at 4:53 PM Fabrizio Castro <fabrizio.castro.jz@...esas.com> wrote:
> > Add DT bindings for the Renesas RZ/V2H(P) Interrupt Controller.
> >
> > Also add macros for the NMI and IRQ0-15 interrupts which map the
> > SPI0-16 interrupts on the RZ/V2H(P) SoC so that they can be used in
> > the first cell of the interrupt specifiers.
> >
> > For the second cell of the interrupt specifier, since NMI, IRQn and
> > TINTn support different types of interrupts between themselves, add
> > helper macros to make it easier for the user to work out what's
> > available.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> > ---
> > v1->v2:
> > * Removed '|' from main description
> > * Reworked main description
> > * Fixed indentation of #interrupt-cells
> > * Reworked description of #interrupt-cells
> > * Dropped file include/dt-bindings/interrupt-controller/icu-rzv2h.h
> 
> Thanks for the update!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,r
> > +++ zv2h-icu.yaml
> 
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a09g057-icu          # RZ/V2H(P)
> 
> Too many spaces before "#"?

Indeed. I'll replace with 1 space.

> 
> > +
> > +  '#interrupt-cells':
> > +    description: The first cell is the SPI number of the NMI or the
> > +      PORT_IRQ[0-15] interrupt, as per user manual. The second cell is used to
> > +      specify the flag.
> > +    const: 2
> > +
> > +  '#address-cells':
> > +    const: 0
> > +
> > +  interrupt-controller: true
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 58
> > +    items:
> > +      - description: NMI interrupt
> > +      - description: IRQ0 interrupt
> > +      - description: IRQ1 interrupt
> > +      - description: IRQ2 interrupt
> > +      - description: IRQ3 interrupt
> > +      - description: IRQ4 interrupt
> > +      - description: IRQ5 interrupt
> > +      - description: IRQ6 interrupt
> > +      - description: IRQ7 interrupt
> > +      - description: IRQ8 interrupt
> > +      - description: IRQ9 interrupt
> > +      - description: IRQ10 interrupt
> > +      - description: IRQ11 interrupt
> > +      - description: IRQ12 interrupt
> > +      - description: IRQ13 interrupt
> > +      - description: IRQ14 interrupt
> > +      - description: IRQ15 interrupt
> 
> "PORT_IRQ<n>", to match Table 4.6-22 ("List of Input Events") and '#interrupt-cells' above.

Good shout.

> 
> > +      - description: GPIO interrupt, TINT0
> > +      - description: GPIO interrupt, TINT1
> > +      - description: GPIO interrupt, TINT2
> > +      - description: GPIO interrupt, TINT3
> > +      - description: GPIO interrupt, TINT4
> > +      - description: GPIO interrupt, TINT5
> > +      - description: GPIO interrupt, TINT6
> > +      - description: GPIO interrupt, TINT7
> > +      - description: GPIO interrupt, TINT8
> > +      - description: GPIO interrupt, TINT9
> > +      - description: GPIO interrupt, TINT10
> > +      - description: GPIO interrupt, TINT11
> > +      - description: GPIO interrupt, TINT12
> > +      - description: GPIO interrupt, TINT13
> > +      - description: GPIO interrupt, TINT14
> > +      - description: GPIO interrupt, TINT15
> > +      - description: GPIO interrupt, TINT16
> > +      - description: GPIO interrupt, TINT17
> > +      - description: GPIO interrupt, TINT18
> > +      - description: GPIO interrupt, TINT19
> > +      - description: GPIO interrupt, TINT20
> > +      - description: GPIO interrupt, TINT21
> > +      - description: GPIO interrupt, TINT22
> > +      - description: GPIO interrupt, TINT23
> > +      - description: GPIO interrupt, TINT24
> > +      - description: GPIO interrupt, TINT25
> > +      - description: GPIO interrupt, TINT26
> > +      - description: GPIO interrupt, TINT27
> > +      - description: GPIO interrupt, TINT28
> > +      - description: GPIO interrupt, TINT29
> > +      - description: GPIO interrupt, TINT30
> > +      - description: GPIO interrupt, TINT31
> > +      - description: Software interrupt, INTA55_0
> > +      - description: Software interrupt, INTA55_1
> > +      - description: Software interrupt, INTA55_2
> > +      - description: Software interrupt, INTA55_3
> > +      - description: Error interrupt to CA55
> > +      - description: GTCCRA compare match/input capture (U0)
> > +      - description: GTCCRB compare match/input capture (U0)
> > +      - description: GTCCRA compare match/input capture (U1)
> > +      - description: GTCCRB compare match/input capture (U1)
> > +
> > +  interrupt-names:
> > +    minItems: 58
> > +    items:
> > +      - const: nmi
> > +      - const: irq0
> > +      - const: irq1
> > +      - const: irq2
> > +      - const: irq3
> > +      - const: irq4
> > +      - const: irq5
> > +      - const: irq6
> > +      - const: irq7
> > +      - const: irq8
> > +      - const: irq9
> > +      - const: irq10
> > +      - const: irq11
> > +      - const: irq12
> > +      - const: irq13
> > +      - const: irq14
> > +      - const: irq15
> 
> port_irq<n>?

Will change.

> 
> The rest LGTM, although I think you may want to add more interrupts later, for various events? However,
> it's not really clear to me which interrupts go through the ICU, and which go directly to the GIC.

The interrupts listed in here are the ones from Table 4.6-23 where ICU is explicitly listed in the "Unit"
column, on top of the interrupts coming from PFC. We'll add more interrupts if needed later on, because
as you said, some things are not super clear from the manual.

I'll send a new version soon.

Kind regards,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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