| Message ID | 20240129151618.90922-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add IAX45 support for RZ/Five SoC | expand |
Hi Prabhakar, On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <conor@kernel.org> wrote: > 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/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@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? > > + - 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@mail.gmail.com Gr{oetje,eeting}s, Geert
On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Now that we have enabled IRQC support for RZ/Five SoC switch to interrupt > mode for ethernet0/1 PHYs instead of polling mode. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Prabhakar, On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared > to the RZ/G2L (family) SoC. > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC > controller driver. Two new registers, IMSK and TMSK, are defined to > handle masking on RZ/Five SoC. The implementation utilizes a new data > structure, `struct rzg2l_irqc_data`, to determine mask support for a > specific controller instance. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/irqchip/irq-renesas-rzg2l.c > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache { > u32 titsr[2]; > }; > > +/** > + * struct rzg2l_irqc_data - OF data structure > + * @mask_supported: Indicates if mask registers are available > + */ > +struct rzg2l_irqc_data { This structure has the same name as the single static struct rzg2l_irqc_priv instance, which is confusing. > + bool mask_supported; > +}; > + > /** > * struct rzg2l_irqc_priv - IRQ controller private data structure > * @base: Controller's base address > + * @data: OF data pointer > * @fwspec: IRQ firmware specific data > * @lock: Lock to serialize access to hardware registers > * @cache: Registers cache for suspend/resume > */ > static struct rzg2l_irqc_priv { > void __iomem *base; > + const struct rzg2l_irqc_data *data; Replacing this by a bool would avoid a pointer dereference in each user, and allows you to make rzg2l_irqc_data etc. __initconst. > struct irq_fwspec fwspec[IRQC_NUM_IRQ]; > raw_spinlock_t lock; > struct rzg2l_irqc_reg_cache cache; > @@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv, > return 0; > } > > +static const struct rzg2l_irqc_data rzfive_irqc_data = { > + .mask_supported = true, > +}; > + > +static const struct rzg2l_irqc_data rzg2l_irqc_default_data = { > + .mask_supported = false, > +}; > + > +static const struct of_device_id rzg2l_irqc_matches[] = { > + { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data }, > + { } > +}; > + > static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > { > struct irq_domain *irq_domain, *parent_domain; > + const struct of_device_id *match; > struct platform_device *pdev; > struct reset_control *resetn; > int ret; > @@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent) > if (!rzg2l_irqc_data) > return -ENOMEM; > > + match = of_match_node(rzg2l_irqc_matches, node); > + if (match) > + rzg2l_irqc_data->data = match->data; > + else > + rzg2l_irqc_data->data = &rzg2l_irqc_default_data; Instead of matching a second time, I'd rather add a second IRQCHIP_MATCH() entry with a different init function, passing the actual rzg2l_irqc_data pointer. > + > rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL); > if (IS_ERR(rzg2l_irqc_data->base)) > return PTR_ERR(rzg2l_irqc_data->base); Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Tue, Jan 30, 2024 at 11:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <conor@kernel.org> wrote: > > 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/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@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 > => 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_5UMng3q__Q@mail.gmail.com > Sorry I missed this thread. Cheers, Prabhakar
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Hi All, The IAX45 block on RZ/Five SoC is almost identical to the IRQC bock found on the RZ/G2L family of SoCs. IAX45 performs various interrupt controls including synchronization for the external interrupts of NMI, IRQ, and GPIOINT and the interrupts of the built-in peripheral interrupts output by each module. And it notifies the interrupt to the PLIC. - Select 32 TINT from 82 GPIOINT. - Integration of bus error interrupts from system bus. - Integration of ECC error interrupts from On-chip RAM. - Indicate interrupt status. (NMI, IRQ, TINT, integrated bus error interrupt and integrated ECC error interrupt) - Setting of interrupt detection method. (NMI, IRQ and TINT) - All interrupts are masked by INTMASK. - Mask function for NMI, IRQ and TINT This patch series adds support for IAX45 in the IRQC driver and enables this on RZ/Five SoC. Cheers, Prabhakar Lad Prabhakar (5): dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC irqchip/renesas-rzg2l: Add support for RZ/Five SoC riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI arm64: dts: renesas: r9a07g043: Move interrupt-parent property to common DTSI riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt properties from ETH0/1 nodes .../renesas,rzg2l-irqc.yaml | 27 ++++ arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 1 + arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 4 - arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 76 ++++++++++ .../boot/dts/renesas/rzfive-smarc-som.dtsi | 16 --- drivers/irqchip/irq-renesas-rzg2l.c | 132 +++++++++++++++++- 6 files changed, 232 insertions(+), 24 deletions(-)