[PATCH] ARM: imx: only enable pinctrl as needed

[PATCH] ARM: imx: only enable pinctrl as needed

[Esben Haabendal]

As not all mach-imx platforms has support for run-time changes of pin
configurations (such as LS1021A), a more selective approach to enabling
pinctrl infrastructure makes sense, so that an e.g. an LS1021A only kernel
could be built without pinctrl support.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 arch/arm/mach-imx/Kconfig | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index b407b024dde3..3fc170456cde 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -6,7 +6,6 @@ menuconfig ARCH_MXC
 	select CLKSRC_IMX_GPT
 	select GENERIC_IRQ_CHIP
 	select GPIOLIB
-	select PINCTRL
 	select PM_OPP if PM
 	select SOC_BUS
 	select SRAM
@@ -61,6 +60,7 @@ config SOC_IMX31
 config SOC_IMX35
 	bool "i.MX35 support"
 	select MXC_AVIC
+	select PINCTRL
 	select PINCTRL_IMX35
 	help
 	  This enables support for Freescale i.MX35 processor
@@ -73,6 +73,7 @@ config SOC_IMX1
 	bool "i.MX1 support"
 	select CPU_ARM920T
 	select MXC_AVIC
+	select PINCTRL
 	select PINCTRL_IMX1
 	help
 	  This enables support for Freescale i.MX1 processor
@@ -85,6 +86,7 @@ config SOC_IMX25
 	bool "i.MX25 support"
 	select CPU_ARM926T
 	select MXC_AVIC
+	select PINCTRL
 	select PINCTRL_IMX25
 	help
 	  This enables support for Freescale i.MX25 processor
@@ -93,6 +95,7 @@ config SOC_IMX27
 	bool "i.MX27 support"
 	select CPU_ARM926T
 	select MXC_AVIC
+	select PINCTRL
 	select PINCTRL_IMX27
 	help
 	  This enables support for Freescale i.MX27 processor
@@ -110,6 +113,7 @@ config SOC_IMX5
 
 config	SOC_IMX50
 	bool "i.MX50 support"
+	select PINCTRL
 	select PINCTRL_IMX50
 	select SOC_IMX5
 
@@ -118,6 +122,7 @@ config	SOC_IMX50
 
 config SOC_IMX51
 	bool "i.MX51 support"
+	select PINCTRL
 	select PINCTRL_IMX51
 	select SOC_IMX5
 	help
@@ -125,6 +130,7 @@ config SOC_IMX51
 
 config	SOC_IMX53
 	bool "i.MX53 support"
+	select PINCTRL
 	select PINCTRL_IMX53
 	select SOC_IMX5
 
@@ -149,6 +155,7 @@ config SOC_IMX6Q
 	select ARM_ERRATA_775420
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD
+	select PINCTRL
 	select PINCTRL_IMX6Q
 	select SOC_IMX6
 
@@ -159,6 +166,7 @@ config SOC_IMX6SL
 	bool "i.MX6 SoloLite support"
 	select ARM_ERRATA_754322
 	select ARM_ERRATA_775420
+	select PINCTRL
 	select PINCTRL_IMX6SL
 	select SOC_IMX6
 
@@ -169,6 +177,7 @@ config SOC_IMX6SLL
 	bool "i.MX6 SoloLiteLite support"
 	select ARM_ERRATA_754322
 	select ARM_ERRATA_775420
+	select PINCTRL
 	select PINCTRL_IMX6SLL
 	select SOC_IMX6
 
@@ -179,6 +188,7 @@ config SOC_IMX6SX
 	bool "i.MX6 SoloX support"
 	select ARM_ERRATA_754322
 	select ARM_ERRATA_775420
+	select PINCTRL
 	select PINCTRL_IMX6SX
 	select SOC_IMX6
 
@@ -187,6 +197,7 @@ config SOC_IMX6SX
 
 config SOC_IMX6UL
 	bool "i.MX6 UltraLite support"
+	select PINCTRL
 	select PINCTRL_IMX6UL
 	select SOC_IMX6
 	select ARM_ERRATA_814220
@@ -223,6 +234,7 @@ config SOC_IMX7D_CM4
 
 config SOC_IMX7D
 	bool "i.MX7 Dual support"
+	select PINCTRL
 	select PINCTRL_IMX7D
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
@@ -233,6 +245,7 @@ config SOC_IMX7D
 config SOC_IMX7ULP
 	bool "i.MX7ULP support"
 	select CLKSRC_IMX_TPM
+	select PINCTRL
 	select PINCTRL_IMX7ULP
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
@@ -242,6 +255,7 @@ config SOC_IMX7ULP
 config SOC_VF610
 	bool "Vybrid Family VF610 support"
 	select ARM_GIC if ARCH_MULTI_V7
+	select PINCTRL
 	select PINCTRL_VF610
 
 	help
-- 
2.31.1

Re: [PATCH] ARM: imx: only enable pinctrl as needed

[Arnd Bergmann]

On Tue, May 25, 2021 at 1:22 PM Esben Haabendal <esben@geanix.com> wrote:
>
> As not all mach-imx platforms has support for run-time changes of pin
> configurations (such as LS1021A), a more selective approach to enabling
> pinctrl infrastructure makes sense, so that an e.g. an LS1021A only kernel
> could be built without pinctrl support.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>

I think it would be even better to leave all these drivers to be
user-configurable. The symbols are currently defined as e.g.

config PINCTRL_IMX51
        bool "IMX51 pinctrl driver"
        depends on SOC_IMX51
        select PINCTRL_IMX
        help
          Say Y here to enable the imx51 pinctrl driver

which could be changed to

config PINCTRL_IMX51
        bool "IMX51 pinctrl driver" if COMPILE_TEST && !SOC_IMX51
        depends on OF
        default SOC_IMX51
        select PINCTRL_IMX
        help
          Say Y here to enable the imx51 pinctrl driver

Today, having it configurable is pointless because you can't turn it off
when SOC_IMX51 is set, and you can't turn it on when SOC_IMX51
is disabled.

The second version allows turning off PINCTRL completely though, as
we do for other top-level subsystems that would likely make the system
unusable when disabled (block, serial, ...), and it allows compile-testing
on other machines, provided some dependencies (CONFIG_OF in the
example) are met. It could theoretically also allow making it a 'tristate'
option, as we do for an increasing number of drivers.

       Arnd

Re: [PATCH] ARM: imx: only enable pinctrl as needed

[Linus Walleij]

On Fri, May 28, 2021 at 9:55 AM Arnd Bergmann <arnd@arndb.de> wrote:

> I think it would be even better to leave all these drivers to be
> user-configurable. The symbols are currently defined as e.g.
>
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver"
>         depends on SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
>
> which could be changed to
>
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver" if COMPILE_TEST && !SOC_IMX51
>         depends on OF
>         default SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
>
> Today, having it configurable is pointless because you can't turn it off
> when SOC_IMX51 is set, and you can't turn it on when SOC_IMX51
> is disabled.

I agree this looks better.

With pin control drivers I think this often reflects the desire
to not make it possible to build a kernel that will not boot.

Usually this is because initramfs is assumed not to be used to house
the most necessary modules, so any modules need to be loaded
later from e.g. eMMC and that of course needs pin control
before it can be mounted.

I think it is a bit of "embedded culture" to do things like
this, because the distro way with modules and initramfs isn't
used by all embedded build environments like it is on your
regular Debian, Fedora, ... etc. Instead they assume a
monolithic kernel tailored for the hardware to a certain extent.

Maybe we need to clarify somehow that the driving idea
behind multiplatform also assumes modularization and
using initramfs for the most necessary modules and things
like that.

Yours,
Linus Walleij

Re: [PATCH] ARM: imx: only enable pinctrl as needed

[Rasmus Villemoes]

On 28/05/2021 09.53, Arnd Bergmann wrote:
> On Tue, May 25, 2021 at 1:22 PM Esben Haabendal <esben@geanix.com> wrote:
>>
>> As not all mach-imx platforms has support for run-time changes of pin
>> configurations (such as LS1021A), a more selective approach to enabling
>> pinctrl infrastructure makes sense, so that an e.g. an LS1021A only kernel
>> could be built without pinctrl support.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
> 
> I think it would be even better to leave all these drivers to be
> user-configurable. The symbols are currently defined as e.g.
> 
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver"
>         depends on SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
> 
> which could be changed to
> 
> config PINCTRL_IMX51
>         bool "IMX51 pinctrl driver" if COMPILE_TEST && !SOC_IMX51
>         depends on OF
>         default SOC_IMX51
>         select PINCTRL_IMX
>         help
>           Say Y here to enable the imx51 pinctrl driver
> 
> Today, having it configurable is pointless because you can't turn it off
> when SOC_IMX51 is set, and you can't turn it on when SOC_IMX51
> is disabled.

But if you want to allow turning it off when SOC_IMX51 is set, don't you
want this to be

config PINCTRL_IMX51
        bool "IMX51 pinctrl driver"
        depends on OF
        depends on COMPILE_TEST || SOC_IMX51
        default SOC_IMX51
        select PINCTRL_IMX
        help
          Say Y here to enable the imx51 pinctrl driver

(otherwise, the !SOC_IMX51 condition on the prompt means it's not a
visible and thus changeable item).

But I think all the COMPILE_TEST would be better done later; it's not
immediately clear what "depends on" one would have to add in lieu of
SOC_${soc}.

> The second version allows turning off PINCTRL completely though, 

I'm not really sure what "second version" you're talking about here. If
you refer to Esben's original patch, that is indeed the whole goal -
getting rid of the pinctrl core code and anything else which is under
#ifdef CONFIG_PINCTRL which is useless on ls1021a (and its absence does
not make the board unbootable).

Rasmus