Re: [RFC PATCH v2 14/30] drivers/irqchip: Add SH7751 Internal INTC drivers.

[Yoshinori Sato <ysato@users.sourceforge.jp>]

On Mon, 25 Sep 2023 16:38:37 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> On Fri, Sep 22, 2023 at 12:12 PM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> > On Tue, 19 Sep 2023 20:50:14 +0900,
> > Geert Uytterhoeven wrote:
> > > On Wed, Sep 13, 2023 at 11:24 AM Yoshinori Sato
> > > <ysato@users.sourceforge.jp> wrote:
> > > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -679,4 +679,13 @@ config SUNPLUS_SP7021_INTC
> > > >           chained controller, routing all interrupt source in P-Chip to
> > > >           the primary controller on C-Chip.
> > > >
> > > > +config RENESAS_SH7751_INTC
> > > > +        bool "Renesas SH7751 Interrupt Controller"
> > > > +       depends on SH_DEVICE_TREE
> > >
> > > "|| COMPILE_TEST"?
> > >
> > > > +       select IRQ_DOMAIN
> > > > +       select IRQ_DOMAIN_HIERARCHY
> > > > +       help
> > > > +         Support for the Renesas SH7751 On-chip interrupt controller.
> > > > +         And external interrupt encoder for some targets.
> > >
> > > Inconsistent indentation
> > >
> > > > --- /dev/null
> > > > +++ b/drivers/irqchip/irq-renesas-sh7751.c
> > >
> > > > +/* INTEVT to IPR mapping */
> > > > +static const struct iprmap {
> > > > +       int intevt;
> > >
> > > irq, as you're storing the irq number not the event number?
> > >
> > > > +       int off;
> > > > +       int bit;
> > >
> > > All unsigned int ...
> > >
> > > > +} iprmaps[] = {
> > > > +#define IPRDEF(e, o, b) { .intevt = evt2irq(e), .off = o, .bit = b }
> > > > +       IPRDEF(0x240, IPRD, IPR_B12),   /* IRL0 */
> > > > +       IPRDEF(0x2a0, IPRD, IPR_B8),    /* IRL1 */
> > > > +       IPRDEF(0x300, IPRD, IPR_B4),    /* IRL2 */
> > > > +       IPRDEF(0x360, IPRD, IPR_B0),    /* IRL3 */
> > > > +       IPRDEF(0x400, IPRA, IPR_B12),   /* TMU0 */
> > > > +       IPRDEF(0x420, IPRA, IPR_B8),    /* TMU1 */
> > > > +       IPRDEF(0x440, IPRA, IPR_B4),    /* TMU2 TNUI */
> > > > +       IPRDEF(0x460, IPRA, IPR_B4),    /* TMU2 TICPI */
> > > > +       IPRDEF(0x480, IPRA, IPR_B0),    /* RTC ATI */
> > > > +       IPRDEF(0x4a0, IPRA, IPR_B0),    /* RTC PRI */
> > > > +       IPRDEF(0x4c0, IPRA, IPR_B0),    /* RTC CUI */
> > > > +       IPRDEF(0x4e0, IPRB, IPR_B4),    /* SCI ERI */
> > > > +       IPRDEF(0x500, IPRB, IPR_B4),    /* SCI RXI */
> > > > +       IPRDEF(0x520, IPRB, IPR_B4),    /* SCI TXI */
> > > > +       IPRDEF(0x540, IPRB, IPR_B4),    /* SCI TEI */
> > > > +       IPRDEF(0x560, IPRB, IPR_B12),   /* WDT */
> > > > +       IPRDEF(0x580, IPRB, IPR_B8),    /* REF RCMI */
> > > > +       IPRDEF(0x5a0, IPRB, IPR_B4),    /* REF ROVI */
> > > > +       IPRDEF(0x600, IPRC, IPR_B0),    /* H-UDI */
> > > > +       IPRDEF(0x620, IPRC, IPR_B12),   /* GPIO */
> > > > +       IPRDEF(0x640, IPRC, IPR_B8),    /* DMAC DMTE0 */
> > > > +       IPRDEF(0x660, IPRC, IPR_B8),    /* DMAC DMTE1 */
> > > > +       IPRDEF(0x680, IPRC, IPR_B8),    /* DMAC DMTE2 */
> > > > +       IPRDEF(0x6a0, IPRC, IPR_B8),    /* DMAC DMTE3 */
> > > > +       IPRDEF(0x6c0, IPRC, IPR_B8),    /* DMAC DMAE */
> > > > +       IPRDEF(0x700, IPRC, IPR_B4),    /* SCIF ERI */
> > > > +       IPRDEF(0x720, IPRC, IPR_B4),    /* SCIF RXI */
> > > > +       IPRDEF(0x740, IPRC, IPR_B4),    /* SCIF BRI */
> > > > +       IPRDEF(0x760, IPRC, IPR_B4),    /* SCIF TXI */
> > > > +       IPRDEF(0x780, IPRC, IPR_B8),    /* DMAC DMTE4 */
> > > > +       IPRDEF(0x7a0, IPRC, IPR_B8),    /* DMAC DMTE5 */
> > > > +       IPRDEF(0x7c0, IPRC, IPR_B8),    /* DMAC DMTE6 */
> > > > +       IPRDEF(0x7e0, IPRC, IPR_B8),    /* DMAC DMTE7 */
> > > > +       IPRDEF(0xa00, INTPRI00, IPR_B0),        /* PCIC PCISERR */
> > > > +       IPRDEF(0xa20, INTPRI00, IPR_B4),        /* PCIC PCIDMA3 */
> > > > +       IPRDEF(0xa40, INTPRI00, IPR_B4),        /* PCIC PCIDMA2 */
> > > > +       IPRDEF(0xa60, INTPRI00, IPR_B4),        /* PCIC PCIDMA1 */
> > > > +       IPRDEF(0xa80, INTPRI00, IPR_B4),        /* PCIC PCIDMA0 */
> > > > +       IPRDEF(0xaa0, INTPRI00, IPR_B4),        /* PCIC PCIPWON */
> > > > +       IPRDEF(0xac0, INTPRI00, IPR_B4),        /* PCIC PCIPWDWN */
> > > > +       IPRDEF(0xae0, INTPRI00, IPR_B4),        /* PCIC PCIERR */
> > > > +       IPRDEF(0xb00, INTPRI00, IPR_B8),        /* TMU3 */
> > > > +       IPRDEF(0xb80, INTPRI00, IPR_B12),       /* TMU4 */
> > >
> > > Probably the same or a very similar interrupt controller is present
> > > on other SoCs? Then the comments don't make much sense, as the actual
> > > interrupt mapping is specified in the DTS anyway.
> >
> > This interrupt controller design is quite old, so there doesn't seem to be
> > any SoC with a similar design.
> 
> OK.
> 
> > Since the SH interrupt controllers have almost the same design,
> > I think this driver can be used for other devices besides the SH7751.
> 
> Wait, this contradicts your sentence above?

I'm referring to other SH3/4 CPUs.
The only difference is the correspondence between registers and
interrupt sources, so it can be used universally not only for the
SH7751 but also for the other SH3 and 4.
I don't think it fits the philosophy of dt to embed information about
each CPU in the code and switch it with compatible.

> > I think a good way to write IPR mapping is to use dts.
> 
> If there is one thing we learned from putting full clock controller and PM
> Domain hierarchies in DT, it is that that turned out to be very fragile.
> It is hard not to make mistakes in the description, and easy to miss
> a critical aspect of the hardware that needs changes later.
> So that's why it's better to differentiate through the compatible value,
> instead of through (lots of) properties: you can always fix the driver,
> while DT is a stable ABI.

I tried defining it like this.
I think this format is easy to maintain.

renesas,ipr-map = IPRDEF(0x240, IPRD, IPR_B12),	/* IRL0 */
		  IPRDEF(0x2a0, IPRD, IPR_B8),	/* IRL1 */
		  IPRDEF(0x300, IPRD, IPR_B4),	/* IRL2 */
		  IPRDEF(0x360, IPRD, IPR_B0),	/* IRL3 

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

-- 
Yosinori Sato