mbox series

[0/5] Add IAX45 support for RZ/Five SoC

Message ID 20240129151618.90922-1-prabhakar.mahadev-lad.rj@bp.renesas.com
Headers show
Series Add IAX45 support for RZ/Five SoC | expand

Message

Lad, Prabhakar Jan. 29, 2024, 3:16 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Jan. 30, 2024, 11:13 a.m. UTC | #1
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
Geert Uytterhoeven Jan. 30, 2024, 11:30 a.m. UTC | #2
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
Geert Uytterhoeven Jan. 30, 2024, 11:38 a.m. UTC | #3
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
Lad, Prabhakar Jan. 30, 2024, 12:59 p.m. UTC | #4
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