Re: [PATCH 1/1] irqchip: Add config MTK_SYSIRQ and MTK_CIRQ

[Hanks Chen]

On Thu, 2020-06-18 at 22:22 +0800, Hanks Chen wrote:
> On Thu, 2020-06-18 at 13:41 +0100, Marc Zyngier wrote:
> > On 2020-06-18 13:31, Hanks Chen wrote:
> > > Mediatek sysirq and cirq drivers as-is were bound together to the 
> > > config
> > > of ARCH_MEDIATEK.  These two drivers should be able to be configured
> > > separately.  For example, on new Mediatek mobile chips such as 
> > > Dimensity
> > > 820, the sysirq driver is not used since the hardware module is 
> > > removed.
> > > 
> > > Add two new configs to sysirq and cirq drivers.
> > > - config MTK_SYSIRQ for the interrupt polarity controller driver: 
> > > sysirq
> > > - config MTK_CIRQ for the low-power interrupt driver: cirq
> > > 
> > > Signed-off-by: cc.hwang <cc.hwang@xxxxxxxxxxxx>
> > > Signed-off-by: Hanks Chen <hanks.chen@xxxxxxxxxxxx>
> > > ---
> > >  drivers/irqchip/Kconfig  |   12 ++++++++++++
> > >  drivers/irqchip/Makefile |    3 ++-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 29fead2..cc9aa18 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -572,4 +572,16 @@ config LOONGSON_PCH_MSI
> > >  	help
> > >  	  Support for the Loongson PCH MSI Controller.
> > > 
> > > +config MTK_SYSIRQ
> > > +	tristate "Mediatek interrupt polarity controller"
> > 
> > How do you expect this to work as a module?
> 
> My fault, I missed some patches.I'll add them in next patches.
> 
> These should be three patches:
> 1. break existing configurations and create new config for them
> 2. Change Kconfig to be loadable as a module and ensure all platforms
> are executable.
> 3. Make the irq-mtk-sysirq driver as a loadable kernel module for GKI
> and flexibility.
> .
> Thank you for your comment
> 
> > 
> > > +	help
> > > +	  Interrupt polarity controller driver to swap polarity for
> > > +	  interrupts for Mediatek mobile chips.
> > > +
> > > +config MTK_CIRQ
> > > +	bool "Mediatek low-power interrupt controller"
> > > +	help
> > > +	  Low-power interrupt controller driver to monitor IRQS
> > > +	  in the sleep mode for Mediatek mobile chips.
> > > +
> > >  endmenu
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index 133f9c4..30421d2 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -69,7 +69,8 @@ obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
> > >  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
> > >  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
> > >  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
> > > -obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o irq-mtk-cirq.o
> > > +obj-$(CONFIG_MTK_SYSIRQ)		+= irq-mtk-sysirq.o
> > > +obj-$(CONFIG_MTK_CIRQ)			+= irq-mtk-cirq.o
> > >  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
> > >  obj-$(CONFIG_RENESAS_H8300H_INTC)	+= irq-renesas-h8300h.o
> > >  obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
> > 
> > In general, this approach doesn't look right. As it stands, this is just 
> > breaking existing configurations
> > 
> > Do you really expect users to know exactly which interrupt controllers 
> > their system is going to use? This seems like the wrong assumption. If 
> > you really want to save the handful of bytes these drivers take in your 
> > image, then add the relevant dependency information in Kconfig.
> > 
> 
> 1. This patch will add a dependency on it in Kconfig, which ensures all
> platforms are executable.
> 2. We want to follow the Android GKI and according to the requirement of
> GKI(General Kernel Image), all SoC and Board Peripheral drivers are
> loadable kernel modules.so I need to breaking existing configurations
> and create new config for them.

> Thank you for your comment

Hi Marc,

I want to break the dependency between ARCH_MEDIATEK and CIRQ/SYSIRQ,
because we need to follow the GKI.

Could I add the MTK_CIRQ and MTK_SYSIRQ into defconfig?
(arch/arm64/config/defconfig)

It would ensures all platform are executable.

e.g.
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig

 CONFIG_QCOM_PDC=y
+CONFIG_MTK_SYSIRQ=m
+CONFIG_MTK_CIRQ=y
 CONFIG_RESET_QCOM_AOSS=y

--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -572,4 +572,18 @@ config LOONGSON_PCH_MSI
        help
          Support for the Loongson PCH MSI Controller.

+config MTK_SYSIRQ
+       tristate "Mediatek interrupt polarity controller"
+       depends on ARCH_MEDIATEK || COMPILE_TEST
+       help
+         Interrupt polarity controller driver to swap polarity for
+         interrupts for Mediatek mobile chips.
+
+config MTK_CIRQ
+       bool "Mediatek low-power interrupt controller"
+       depends on ARCH_MEDIATEK || COMPILE_TEST
+       help
+         Low-power interrupt controller driver to monitor IRQS
+         in the sleep mode for Mediatek mobile chips.
+


P.S I'll make the irq-mtk-sysirq driver as a loadable kernel module for
GKI

Thanks


> >          M.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel