[PATCH 1/1] irqchip: gpcv2: make config option visible

[PATCH 1/1] irqchip: gpcv2: make config option visible

[Aisheng Dong]

Make GPCv2 config option visible, then user can select it in defconfig
for ARMv8 platforms.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/irqchip/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3d1e607..ed04d47 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -289,7 +289,8 @@ config RENESAS_H8S_INTC
 	select IRQ_DOMAIN
 
 config IMX_GPCV2
-	bool
+	bool "i.MX GPCv2 IRQ chip"
+	depends on ARCH_MXC || (COMPILE_TEST && OF)
 	select IRQ_DOMAIN
 	help
 	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
-- 
2.7.4

Re: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Marc Zyngier]

On 18/01/2019 07:41, Aisheng Dong wrote:
> Make GPCv2 config option visible, then user can select it in defconfig
> for ARMv8 platforms.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/irqchip/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3d1e607..ed04d47 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -289,7 +289,8 @@ config RENESAS_H8S_INTC
>  	select IRQ_DOMAIN
>  
>  config IMX_GPCV2
> -	bool
> +	bool "i.MX GPCv2 IRQ chip"
> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
>  	select IRQ_DOMAIN
>  	help
>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> 

How does it help exactly? It is pretty difficult for a user to know
exactly what they need. I'd rather see it selected by ARCH_MXC, which
makes it unambiguous.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Uwe Kleine-König]

On Fri, Jan 18, 2019 at 07:41:16AM +0000, Aisheng Dong wrote:
> Make GPCv2 config option visible, then user can select it in defconfig
> for ARMv8 platforms.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/irqchip/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3d1e607..ed04d47 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -289,7 +289,8 @@ config RENESAS_H8S_INTC
>  	select IRQ_DOMAIN
>  
>  config IMX_GPCV2
> -	bool
> +	bool "i.MX GPCv2 IRQ chip"
> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
>  	select IRQ_DOMAIN
>  	help
>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block

If you target better compile coverage, how about:

	bool "i.MX GPCv2 IRQ chip" if COMPILE_TEST
	depends on OF

This way the item is still hidden normally.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Aisheng Dong]

> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 5:13 PM
> On 18/01/2019 07:41, Aisheng Dong wrote:
> > Make GPCv2 config option visible, then user can select it in defconfig
> > for ARMv8 platforms.
> >
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Jason Cooper <jason@lakedaemon.net>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/irqchip/Kconfig | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > 3d1e607..ed04d47 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -289,7 +289,8 @@ config RENESAS_H8S_INTC
> >  	select IRQ_DOMAIN
> >
> >  config IMX_GPCV2
> > -	bool
> > +	bool "i.MX GPCv2 IRQ chip"
> > +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> >  	select IRQ_DOMAIN
> >  	help
> >  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> >
> 
> How does it help exactly? It is pretty difficult for a user to know exactly what
> they need. I'd rather see it selected by ARCH_MXC, which makes it

ARM64 SoC maintainers suggest not add more driver specific options except an
Generic ARCH option.

As GPCv2 is also used in MX8MQ. So we may select it in armv8 defconfig.
If this option is invisible, we can't select it.

Regards
Dong Aisheng

> unambiguous.
> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Marc Zyngier]

On 18/01/2019 09:49, Aisheng Dong wrote:
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
>> Sent: Friday, January 18, 2019 5:13 PM
>> On 18/01/2019 07:41, Aisheng Dong wrote:
>>> Make GPCv2 config option visible, then user can select it in defconfig
>>> for ARMv8 platforms.
>>>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Jason Cooper <jason@lakedaemon.net>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>>> ---
>>>  drivers/irqchip/Kconfig | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
>>> 3d1e607..ed04d47 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -289,7 +289,8 @@ config RENESAS_H8S_INTC
>>>  	select IRQ_DOMAIN
>>>
>>>  config IMX_GPCV2
>>> -	bool
>>> +	bool "i.MX GPCv2 IRQ chip"
>>> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
>>>  	select IRQ_DOMAIN
>>>  	help
>>>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
>>>
>>
>> How does it help exactly? It is pretty difficult for a user to know exactly what
>> they need. I'd rather see it selected by ARCH_MXC, which makes it
> 
> ARM64 SoC maintainers suggest not add more driver specific options except an
> Generic ARCH option.
> 
> As GPCv2 is also used in MX8MQ. So we may select it in armv8 defconfig.
> If this option is invisible, we can't select it.

And conversely, users have no idea of what letter soup they have to
select to make their HW work properly. Selecting the driver when the
platform is supposed to be supported is the right way to solve this problem.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Aisheng Dong]

> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 6:10 PM
[...]
> >>>
> >>>  config IMX_GPCV2
> >>> -	bool
> >>> +	bool "i.MX GPCv2 IRQ chip"
> >>> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> >>>  	select IRQ_DOMAIN
> >>>  	help
> >>>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> >>>
> >>
> >> How does it help exactly? It is pretty difficult for a user to know
> >> exactly what they need. I'd rather see it selected by ARCH_MXC, which
> >> makes it
> >
> > ARM64 SoC maintainers suggest not add more driver specific options
> > except an Generic ARCH option.
> >
> > As GPCv2 is also used in MX8MQ. So we may select it in armv8 defconfig.
> > If this option is invisible, we can't select it.
> 
> And conversely, users have no idea of what letter soup they have to select to
> make their HW work properly. Selecting the driver when the platform is
> supposed to be supported is the right way to solve this problem.
> 

I think the problem is that we have no platform specific CONFIGs for arm v8 platforms.
We have only one CONFIG_ARCH_MXC for all MX8 SoCs, e.g. mx8qxp, mx8mq...
Only MX8MQ needs to use GPCv2. Selecting GPCv2 under ARCH_MXC means users
have no chance to disable it for mx8qxp which does not need it.

We probably could introduce SOC option under drivers/soc/ to do the default configs
selection. But we've already handled all other driver selections in defconfig
e.g. firmware, clk, pinctrl, power domain and etc.
Not sure whether GPCv2 should be an exception.

Regards
Dong Aisheng

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Marc Zyngier]

On Tue, 22 Jan 2019 11:04:48 +0000,
Aisheng Dong <aisheng.dong@nxp.com> wrote:
> 
> 
> 
> > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Friday, January 18, 2019 6:10 PM
> [...]
> > >>>
> > >>>  config IMX_GPCV2
> > >>> -	bool
> > >>> +	bool "i.MX GPCv2 IRQ chip"
> > >>> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> > >>>  	select IRQ_DOMAIN
> > >>>  	help
> > >>>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> > >>>
> > >>
> > >> How does it help exactly? It is pretty difficult for a user to know
> > >> exactly what they need. I'd rather see it selected by ARCH_MXC, which
> > >> makes it
> > >
> > > ARM64 SoC maintainers suggest not add more driver specific options
> > > except an Generic ARCH option.
> > >
> > > As GPCv2 is also used in MX8MQ. So we may select it in armv8 defconfig.
> > > If this option is invisible, we can't select it.
> > 
> > And conversely, users have no idea of what letter soup they have to select to
> > make their HW work properly. Selecting the driver when the platform is
> > supposed to be supported is the right way to solve this problem.
> > 
> 
> I think the problem is that we have no platform specific CONFIGs for arm v8 platforms.
> We have only one CONFIG_ARCH_MXC for all MX8 SoCs, e.g. mx8qxp, mx8mq...
> Only MX8MQ needs to use GPCv2. Selecting GPCv2 under ARCH_MXC means users
> have no chance to disable it for mx8qxp which does not need it.

And where is the problem to select this on platforms that do not
strictly require it? Code bloat?

If you want fine grained selection for people dealing with a single
SoC, make it depend on CONFIG_EXPERT. Don't force this on unsuspecting
users who expect their HW to just work.

Something like:

config IMX_GPCV2
       	bool "i.MX GPCv2 IRQ chip" if EXPERT
	def_bool ARCH_MXC
	select IRQ_DOMAIN
	help
	  Enables the wakeup IRQs for IMX platforms with GPCv2 block

> 
> We probably could introduce SOC option under drivers/soc/ to do the default configs
> selection. But we've already handled all other driver selections in defconfig
> e.g. firmware, clk, pinctrl, power domain and etc.
> Not sure whether GPCv2 should be an exception.

I think something like the above should be the rule. Configuration
feature creep is not helping anyone.

	M.

-- 
Jazz is not dead, it just smell funny.

RE: [PATCH 1/1] irqchip: gpcv2: make config option visible

[Aisheng Dong]

> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Tuesday, January 22, 2019 7:40 PM
> On Tue, 22 Jan 2019 11:04:48 +0000,
> Aisheng Dong <aisheng.dong@nxp.com> wrote:
> >
> >
> >
> > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > > Sent: Friday, January 18, 2019 6:10 PM
> > [...]
> > > >>>
> > > >>>  config IMX_GPCV2
> > > >>> -	bool
> > > >>> +	bool "i.MX GPCv2 IRQ chip"
> > > >>> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> > > >>>  	select IRQ_DOMAIN
> > > >>>  	help
> > > >>>  	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> > > >>>
> > > >>
> > > >> How does it help exactly? It is pretty difficult for a user to
> > > >> know exactly what they need. I'd rather see it selected by
> > > >> ARCH_MXC, which makes it
> > > >
> > > > ARM64 SoC maintainers suggest not add more driver specific options
> > > > except an Generic ARCH option.
> > > >
> > > > As GPCv2 is also used in MX8MQ. So we may select it in armv8 defconfig.
> > > > If this option is invisible, we can't select it.
> > >
> > > And conversely, users have no idea of what letter soup they have to
> > > select to make their HW work properly. Selecting the driver when the
> > > platform is supposed to be supported is the right way to solve this
> problem.
> > >
> >
> > I think the problem is that we have no platform specific CONFIGs for arm v8
> platforms.
> > We have only one CONFIG_ARCH_MXC for all MX8 SoCs, e.g. mx8qxp,
> mx8mq...
> > Only MX8MQ needs to use GPCv2. Selecting GPCv2 under ARCH_MXC means
> > users have no chance to disable it for mx8qxp which does not need it.
> 
> And where is the problem to select this on platforms that do not strictly
> require it? Code bloat?

I think it's not a big problem.

> 
> If you want fine grained selection for people dealing with a single SoC, make it
> depend on CONFIG_EXPERT. Don't force this on unsuspecting users who expect
> their HW to just work.
> 

Seems not too necessary

> Something like:
> 
> config IMX_GPCV2
>        	bool "i.MX GPCv2 IRQ chip" if EXPERT
> 	def_bool ARCH_MXC
> 	select IRQ_DOMAIN
> 	help
> 	  Enables the wakeup IRQs for IMX platforms with GPCv2 block
> 
> >
> > We probably could introduce SOC option under drivers/soc/ to do the
> > default configs selection. But we've already handled all other driver
> > selections in defconfig e.g. firmware, clk, pinctrl, power domain and etc.
> > Not sure whether GPCv2 should be an exception.
> 
> I think something like the above should be the rule. Configuration feature
> creep is not helping anyone.

Got it, thanks for the suggestion.

Regards
Dong Aisheng

> 
> 	M.
> 
> --
> Jazz is not dead, it just smell funny.