From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 97F15E77363 for ; Sat, 30 Sep 2023 15:15:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234276AbjI3PPD convert rfc822-to-8bit (ORCPT ); Sat, 30 Sep 2023 11:15:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47010 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234403AbjI3PPC (ORCPT ); Sat, 30 Sep 2023 11:15:02 -0400 Received: from hsmtpd-def.xspmail.jp (hsmtpd-def.xspmail.jp [202.238.198.239]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E68ADE3 for ; Sat, 30 Sep 2023 08:14:59 -0700 (PDT) X-Country-Code: JP Received: from sakura.ysato.name (ik1-413-38519.vs.sakura.ne.jp [153.127.30.23]) by hsmtpd-out-0.asahinet.cluster.xspmail.jp (Halon) with ESMTPA id dc095e5f-97af-47ad-8057-e068e50a703d; Sun, 01 Oct 2023 00:14:58 +0900 (JST) Received: from SIOS1075.ysato.ml (ZM005235.ppp.dion.ne.jp [222.8.5.235]) by sakura.ysato.name (Postfix) with ESMTPSA id 7FF1B1C01EE; Sun, 1 Oct 2023 00:14:56 +0900 (JST) Date: Sun, 01 Oct 2023 00:14:55 +0900 Message-ID: <87h6nbu1zk.wl-ysato@users.sourceforge.jp> From: Yoshinori Sato To: Geert Uytterhoeven Cc: linux-sh@vger.kernel.org, glaubitz@physik.fu-berlin.de Subject: Re: [RFC PATCH v2 14/30] drivers/irqchip: Add SH7751 Internal INTC drivers. In-Reply-To: References: <1f9decd26e1321e30ca5091c2447456f0e81efe1.1694596125.git.ysato@users.sourceforge.jp> <87pm2att29.wl-ysato@users.sourceforge.jp> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org 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 > wrote: > > On Tue, 19 Sep 2023 20:50:14 +0900, > > Geert Uytterhoeven wrote: > > > On Wed, Sep 13, 2023 at 11:24 AM Yoshinori Sato > > > wrote: > > > > Signed-off-by: Yoshinori Sato > > > > > > 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