[PATCH V2 1/4] gpio: mxc: Support module build

[PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Change config to tristate, add module device table, module author,
description and license to support module build for i.MX GPIO driver.

As this is a SoC GPIO module, it provides common functions for most
of the peripheral devices, such as GPIO pins control, secondary
interrupt controller for GPIO pins IRQ etc., without GPIO driver, most
of the peripheral devices will NOT work properly, so GPIO module is
similar with clock, pinctrl driver that should be loaded ONCE and
never unloaded.

Since MXC GPIO driver needs to have init function to register syscore
ops once, here still use subsys_initcall(), NOT module_platform_driver().

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
	- no code change, just add detail explanation about why this patch
	  does NOT support module unloaded.
---
 drivers/gpio/Kconfig    | 2 +-
 drivers/gpio/gpio-mxc.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8030fd9..468916b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -397,7 +397,7 @@ config GPIO_MVEBU
 	select REGMAP_MMIO
 
 config GPIO_MXC
-	def_bool y
+	tristate "i.MX GPIO support"
 	depends on ARCH_MXC || COMPILE_TEST
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 64278a4..643f4c55 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -15,6 +15,7 @@
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
@@ -158,6 +159,7 @@ static const struct of_device_id mxc_gpio_dt_ids[] = {
 	{ .compatible = "fsl,imx7d-gpio", .data = &mxc_gpio_devtype[IMX35_GPIO], },
 	{ /* sentinel */ }
 };
+MODULE_DEVICE_TABLE(of, mxc_gpio_dt_ids);
 
 /*
  * MX2 has one interrupt *for all* gpio ports. The list is used
@@ -604,3 +606,7 @@ static int __init gpio_mxc_init(void)
 	return platform_driver_register(&mxc_gpio_driver);
 }
 subsys_initcall(gpio_mxc_init);
+
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("i.MX GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH V2 2/4] arm64: defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
as built-in manually.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 070c7e3..43b2bfd 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -500,6 +500,7 @@ CONFIG_GPIO_PCA953X=y
 CONFIG_GPIO_PCA953X_IRQ=y
 CONFIG_GPIO_BD9571MWV=m
 CONFIG_GPIO_MAX77620=y
+CONFIG_GPIO_MXC=y
 CONFIG_POWER_AVS=y
 CONFIG_QCOM_CPR=y
 CONFIG_ROCKCHIP_IODOMAIN=y
-- 
2.7.4

[PATCH V2 3/4] ARM: imx_v6_v7_defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
as built-in manually.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No change.
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 82d3ffb..02f83c8 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -226,6 +226,7 @@ CONFIG_GPIO_PCA953X=y
 CONFIG_GPIO_PCF857X=y
 CONFIG_GPIO_STMPE=y
 CONFIG_GPIO_74X164=y
+CONFIG_GPIO_MXC=y
 CONFIG_POWER_RESET=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_POWER_RESET_SYSCON_POWEROFF=y
-- 
2.7.4

[PATCH V2 4/4] ARM: multi_v7_defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
as built-in manually.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
new patch.
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index e9e76e3..abfd4da 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -463,6 +463,7 @@ CONFIG_GPIO_PALMAS=y
 CONFIG_GPIO_TPS6586X=y
 CONFIG_GPIO_TPS65910=y
 CONFIG_GPIO_TWL4030=y
+CONFIG_GPIO_MXC=y
 CONFIG_POWER_AVS=y
 CONFIG_ROCKCHIP_IODOMAIN=y
 CONFIG_POWER_RESET_AS3722=y
-- 
2.7.4

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Arnd Bergmann]

On Wed, Jul 22, 2020 at 3:50 AM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Change config to tristate, add module device table, module author,
> description and license to support module build for i.MX GPIO driver.
>
> As this is a SoC GPIO module, it provides common functions for most
> of the peripheral devices, such as GPIO pins control, secondary
> interrupt controller for GPIO pins IRQ etc., without GPIO driver, most
> of the peripheral devices will NOT work properly, so GPIO module is
> similar with clock, pinctrl driver that should be loaded ONCE and
> never unloaded.
>
> Since MXC GPIO driver needs to have init function to register syscore
> ops once, here still use subsys_initcall(), NOT module_platform_driver().

I'm not following this explanation.

Why is this driver using syscore_ops rather than
SIMPLE_DEV_PM_OPS() or similar?

Why can the driver not unregister the syscore_ops the way it
registers them when unloading the module?

Why do you need subsys_initcall() rather than a device_initcall()?

If the subsys_initcall() is indeed required here instead of
device_initcall(), how can it work if the driver is a loadable
module?

       Arnd

RE: [PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> 
> On Wed, Jul 22, 2020 at 3:50 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > Change config to tristate, add module device table, module author,
> > description and license to support module build for i.MX GPIO driver.
> >
> > As this is a SoC GPIO module, it provides common functions for most of
> > the peripheral devices, such as GPIO pins control, secondary interrupt
> > controller for GPIO pins IRQ etc., without GPIO driver, most of the
> > peripheral devices will NOT work properly, so GPIO module is similar
> > with clock, pinctrl driver that should be loaded ONCE and never
> > unloaded.
> >
> > Since MXC GPIO driver needs to have init function to register syscore
> > ops once, here still use subsys_initcall(), NOT module_platform_driver().
> 
> I'm not following this explanation.
> 
> Why is this driver using syscore_ops rather than
> SIMPLE_DEV_PM_OPS() or similar?

Below is the original patch of using syscore_ops, it has explanation:

commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb
Author: Anson Huang <anson.huang@nxp.com>
Date:   Fri Nov 9 04:56:56 2018 +0000

    gpio: mxc: move gpio noirq suspend/resume to syscore phase

    During noirq suspend/resume phase, GPIO irq could arrive
    and its registers like IMR will be changed by irq handle
    process, to make the GPIO registers exactly when it is
    powered ON after resume, move the GPIO noirq suspend/resume
    callback to syscore suspend/resume phase, local irq is
    disabled at this phase so GPIO registers are atomic.

> 
> Why can the driver not unregister the syscore_ops the way it registers them
> when unloading the module?

As per previous discussion, for SoC level GPIO, since it acts as interrupt controller and
most of peripheral devices now depends on GPIO driver for proper function, so it makes
sense to keep SoC level GPIO once loaded and never unload.


> 
> Why do you need subsys_initcall() rather than a device_initcall()?

The subsys_initcal() is done by below commit, the commit log has detail explanation.


commit e188cbf7564fba80e8339b9406e8740f3e495c63
Author: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Date:   Thu Sep 8 04:48:15 2016 +0300

    gpio: mxc: shift gpio_mxc_init() to subsys_initcall level


> 
> If the subsys_initcall() is indeed required here instead of device_initcall(), how
> can it work if the driver is a loadable module?

My understanding is: there are two scenarios, one for built-in case, the other is for loadable module,
the subsys_initcall() is for built-in case according to the upper commit, for loadable
module, the user needs to handle the sequence of modules loaded.

Thanks,
Anson

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> >
> > On Wed, Jul 22, 2020 at 3:50 AM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > Change config to tristate, add module device table, module author,
> > > description and license to support module build for i.MX GPIO driver.
> > >
> > > As this is a SoC GPIO module, it provides common functions for most of
> > > the peripheral devices, such as GPIO pins control, secondary interrupt
> > > controller for GPIO pins IRQ etc., without GPIO driver, most of the
> > > peripheral devices will NOT work properly, so GPIO module is similar
> > > with clock, pinctrl driver that should be loaded ONCE and never
> > > unloaded.
> > >
> > > Since MXC GPIO driver needs to have init function to register syscore
> > > ops once, here still use subsys_initcall(), NOT module_platform_driver().
> >
> > I'm not following this explanation.
> >
> > Why is this driver using syscore_ops rather than
> > SIMPLE_DEV_PM_OPS() or similar?
>
> Below is the original patch of using syscore_ops, it has explanation:
>
> commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb
> Author: Anson Huang <anson.huang@nxp.com>
> Date:   Fri Nov 9 04:56:56 2018 +0000
>
>     gpio: mxc: move gpio noirq suspend/resume to syscore phase
>
>     During noirq suspend/resume phase, GPIO irq could arrive
>     and its registers like IMR will be changed by irq handle
>     process, to make the GPIO registers exactly when it is
>     powered ON after resume, move the GPIO noirq suspend/resume
>     callback to syscore suspend/resume phase, local irq is
>     disabled at this phase so GPIO registers are atomic.

The description makes sense, but this must be a problem that
other gpio/pinctrl irqchip drivers have as well.

Linus, could you have a look? I see these other pinctrl drivers
using SIMPLE_DEV_PM_OPS:

drivers/pinctrl/nomadik/pinctrl-nomadik.c:static
SIMPLE_DEV_PM_OPS(nmk_pinctrl_pm_ops,
drivers/pinctrl/pinctrl-rockchip.c:static
SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops,
rockchip_pinctrl_suspend,
drivers/pinctrl/pinctrl-stmfx.c:static
SIMPLE_DEV_PM_OPS(stmfx_pinctrl_dev_pm_ops,
drivers/pinctrl/qcom/pinctrl-msm.c:SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_pm_ops,
msm_pinctrl_suspend,
drivers/pinctrl/spear/pinctrl-plgpio.c:static
SIMPLE_DEV_PM_OPS(plgpio_dev_pm_ops, plgpio_suspend, plgpio_resume);

Linus, can you have a look and see if that same problem applies
to all of the above?

It seems that some drivers use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS()
instead, which looks like it is meant to address the same problem, but
as I have not used that myself, I may be misunderstanding the problem
or what this one does.

> > Why do you need subsys_initcall() rather than a device_initcall()?
>
> The subsys_initcal() is done by below commit, the commit log has detail explanation.
>
>
> commit e188cbf7564fba80e8339b9406e8740f3e495c63
> Author: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Date:   Thu Sep 8 04:48:15 2016 +0300
>
>     gpio: mxc: shift gpio_mxc_init() to subsys_initcall level

That commit made the initialization later not earlier, as it originally
was a postcore_initcall(). In the loadable module case, you make
it even later than that, possibly as the last module loaded when
booting up the system (followed by a storm of deferred probes).

> > If the subsys_initcall() is indeed required here instead of device_initcall(), how
> > can it work if the driver is a loadable module?
>
> My understanding is: there are two scenarios, one for built-in case, the other is for loadable module,
> the subsys_initcall() is for built-in case according to the upper commit, for loadable
> module, the user needs to handle the sequence of modules loaded.

I don't think we can rely on user space to coordinate module load order.
The modules are generally loaded in an arbitrary order during the
coldplug phase of the boot when user space looks at the available
devices and loads a module for each one of them in the order it
finds them in sysfs.

This means all drivers that rely on gpio, pinctrl or irqchip interfaces
exported from this driver have to be able to deal with them not being
there. This can also happen when the pinctrl driver is the only one
that is a loadable module, while everything else is built-in. While
that is not a configuration that users would likely choose intentionally,
I don't see a reason why it shouldn't work.

Using module_init() or builtin_platform_driver() here would make
give similar behavior for the built-in and modular cases and be
somewhat more consistent, so you don't run into bugs only when
the driver is a loadable module but make them obvious even to
existing users with a builtin driver.

       Arnd

RE: [PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> 
> On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> > >
> > > On Wed, Jul 22, 2020 at 3:50 AM Anson Huang <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > Change config to tristate, add module device table, module author,
> > > > description and license to support module build for i.MX GPIO driver.
> > > >
> > > > As this is a SoC GPIO module, it provides common functions for
> > > > most of the peripheral devices, such as GPIO pins control,
> > > > secondary interrupt controller for GPIO pins IRQ etc., without
> > > > GPIO driver, most of the peripheral devices will NOT work
> > > > properly, so GPIO module is similar with clock, pinctrl driver
> > > > that should be loaded ONCE and never unloaded.
> > > >
> > > > Since MXC GPIO driver needs to have init function to register
> > > > syscore ops once, here still use subsys_initcall(), NOT
> module_platform_driver().
> > >
> > > I'm not following this explanation.
> > >
> > > Why is this driver using syscore_ops rather than
> > > SIMPLE_DEV_PM_OPS() or similar?
> >
> > Below is the original patch of using syscore_ops, it has explanation:
> >
> > commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb
> > Author: Anson Huang <anson.huang@nxp.com>
> > Date:   Fri Nov 9 04:56:56 2018 +0000
> >
> >     gpio: mxc: move gpio noirq suspend/resume to syscore phase
> >
> >     During noirq suspend/resume phase, GPIO irq could arrive
> >     and its registers like IMR will be changed by irq handle
> >     process, to make the GPIO registers exactly when it is
> >     powered ON after resume, move the GPIO noirq suspend/resume
> >     callback to syscore suspend/resume phase, local irq is
> >     disabled at this phase so GPIO registers are atomic.
> 
> The description makes sense, but this must be a problem that other
> gpio/pinctrl irqchip drivers have as well.
> 
> Linus, could you have a look? I see these other pinctrl drivers using
> SIMPLE_DEV_PM_OPS:
> 
> drivers/pinctrl/nomadik/pinctrl-nomadik.c:static
> SIMPLE_DEV_PM_OPS(nmk_pinctrl_pm_ops,
> drivers/pinctrl/pinctrl-rockchip.c:static
> SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops,
> rockchip_pinctrl_suspend,
> drivers/pinctrl/pinctrl-stmfx.c:static
> SIMPLE_DEV_PM_OPS(stmfx_pinctrl_dev_pm_ops,
> drivers/pinctrl/qcom/pinctrl-msm.c:SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_
> pm_ops,
> msm_pinctrl_suspend,
> drivers/pinctrl/spear/pinctrl-plgpio.c:static
> SIMPLE_DEV_PM_OPS(plgpio_dev_pm_ops, plgpio_suspend, plgpio_resume);
> 
> Linus, can you have a look and see if that same problem applies to all of the
> above?
> 
> It seems that some drivers use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() instead,
> which looks like it is meant to address the same problem, but as I have not
> used that myself, I may be misunderstanding the problem or what this one
> does.
> 
> > > Why do you need subsys_initcall() rather than a device_initcall()?
> >
> > The subsys_initcal() is done by below commit, the commit log has detail
> explanation.
> >
> >
> > commit e188cbf7564fba80e8339b9406e8740f3e495c63
> > Author: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > Date:   Thu Sep 8 04:48:15 2016 +0300
> >
> >     gpio: mxc: shift gpio_mxc_init() to subsys_initcall level
> 
> That commit made the initialization later not earlier, as it originally was a
> postcore_initcall(). In the loadable module case, you make it even later than
> that, possibly as the last module loaded when booting up the system (followed
> by a storm of deferred probes).
>

Yes, loadable module will make it even later, the assumption is userspace can load it
before any users depend on GPIO driver. Given that we have to support loadable module
for all SoC specific module, do you have any other suggestion of how to proceed this
requirement for SoC GPIO driver?

 
> > > If the subsys_initcall() is indeed required here instead of
> > > device_initcall(), how can it work if the driver is a loadable module?
> >
> > My understanding is: there are two scenarios, one for built-in case,
> > the other is for loadable module, the subsys_initcall() is for
> > built-in case according to the upper commit, for loadable module, the user
> needs to handle the sequence of modules loaded.
> 
> I don't think we can rely on user space to coordinate module load order.
> The modules are generally loaded in an arbitrary order during the coldplug
> phase of the boot when user space looks at the available devices and loads a
> module for each one of them in the order it finds them in sysfs.
> 
> This means all drivers that rely on gpio, pinctrl or irqchip interfaces exported
> from this driver have to be able to deal with them not being there. This can
> also happen when the pinctrl driver is the only one that is a loadable module,
> while everything else is built-in. While that is not a configuration that users
> would likely choose intentionally, I don't see a reason why it shouldn't work.
> 
> Using module_init() or builtin_platform_driver() here would make give similar
> behavior for the built-in and modular cases and be somewhat more consistent,
> so you don't run into bugs only when the driver is a loadable module but make
> them obvious even to existing users with a builtin driver.
> 

My original idea of adding loadable module support for SoC specific module is, try
to keep it exactly same when the driver is built-in, but for GKI support, first, we need
to support GPIO driver built as module, and we definitely need to think about the module
load sequence to handle these dependency, but thinking about the common module widely
used by devices, such as pinctrl, clock and GPIO, maybe other modules need some patches
to handle the dependency, but that will be done later when we are working for those modules.

So, could you please help advise how to proceed it for this GPIO driver to support loadable module?

Thanks,
Anson

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 1:21 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> > On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com> wrote:
> > > > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> > >
> > > commit e188cbf7564fba80e8339b9406e8740f3e495c63
> > > Author: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > > Date:   Thu Sep 8 04:48:15 2016 +0300
> > >
> > >     gpio: mxc: shift gpio_mxc_init() to subsys_initcall level
> >
> > That commit made the initialization later not earlier, as it originally was a
> > postcore_initcall(). In the loadable module case, you make it even later than
> > that, possibly as the last module loaded when booting up the system (followed
> > by a storm of deferred probes).
> >
>
> Yes, loadable module will make it even later, the assumption is userspace can load it
> before any users depend on GPIO driver. Given that we have to support loadable module
> for all SoC specific module, do you have any other suggestion of how to proceed this
> requirement for SoC GPIO driver?

I think in general, drivers should be prepared for -EPROBE_DEFER error
codes returned from interfaces such as devm_gpiod_get().

> > I don't think we can rely on user space to coordinate module load order.
> > The modules are generally loaded in an arbitrary order during the coldplug
> > phase of the boot when user space looks at the available devices and loads a
> > module for each one of them in the order it finds them in sysfs.
> >
> > This means all drivers that rely on gpio, pinctrl or irqchip interfaces exported
> > from this driver have to be able to deal with them not being there. This can
> > also happen when the pinctrl driver is the only one that is a loadable module,
> > while everything else is built-in. While that is not a configuration that users
> > would likely choose intentionally, I don't see a reason why it shouldn't work.
> >
> > Using module_init() or builtin_platform_driver() here would make give similar
> > behavior for the built-in and modular cases and be somewhat more consistent,
> > so you don't run into bugs only when the driver is a loadable module but make
> > them obvious even to existing users with a builtin driver.
> >
>
> My original idea of adding loadable module support for SoC specific module is, try
> to keep it exactly same when the driver is built-in, but for GKI support, first, we need
> to support GPIO driver built as module, and we definitely need to think about the module
> load sequence to handle these dependency, but thinking about the common module widely
> used by devices, such as pinctrl, clock and GPIO, maybe other modules need some patches
> to handle the dependency, but that will be done later when we are working for those modules.

Overall, my feeling is that making sure all drivers that depend on the pinctrl
driver can deal with deferred probing is a prerequisite before this can be
made a loadable module itself (same for clk, irqchip, etc drivers that others
may rely on).

I understand that your primary motivation is to fit into Google's GKI framework,
but I think that doing the conversion only partially would neither serve to
improve the kernel nor actually meet the GKI requirements.

Most pinctrl drivers are currently always built-in to work around the
load order dependencies. This of course is a bit of a hack and we'd be
better off if all drivers managed to avoid the dependencies, but this
can also require a lot of work.

> So, could you please help advise how to proceed it for this GPIO driver to
>  support loadable module?

I would start by getting a reference board to work with a kernel in which
all drivers are built-in except for the pinctrl driver, to see what exactly
breaks when you do that, and what other drivers may have the same
problems. Maybe it's not that bad after all and you only need a few
modifications.

      Arnd

RE: [PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> 
> On Mon, Jul 27, 2020 at 1:21 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build On Mon,
> > > Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com> wrote:
> > > > > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> > > >
> > > > commit e188cbf7564fba80e8339b9406e8740f3e495c63
> > > > Author: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > > > Date:   Thu Sep 8 04:48:15 2016 +0300
> > > >
> > > >     gpio: mxc: shift gpio_mxc_init() to subsys_initcall level
> > >
> > > That commit made the initialization later not earlier, as it
> > > originally was a postcore_initcall(). In the loadable module case,
> > > you make it even later than that, possibly as the last module loaded
> > > when booting up the system (followed by a storm of deferred probes).
> > >
> >
> > Yes, loadable module will make it even later, the assumption is
> > userspace can load it before any users depend on GPIO driver. Given
> > that we have to support loadable module for all SoC specific module,
> > do you have any other suggestion of how to proceed this requirement for
> SoC GPIO driver?
> 
> I think in general, drivers should be prepared for -EPROBE_DEFER error codes
> returned from interfaces such as devm_gpiod_get().
> 
> > > I don't think we can rely on user space to coordinate module load order.
> > > The modules are generally loaded in an arbitrary order during the
> > > coldplug phase of the boot when user space looks at the available
> > > devices and loads a module for each one of them in the order it finds them
> in sysfs.
> > >
> > > This means all drivers that rely on gpio, pinctrl or irqchip
> > > interfaces exported from this driver have to be able to deal with
> > > them not being there. This can also happen when the pinctrl driver
> > > is the only one that is a loadable module, while everything else is
> > > built-in. While that is not a configuration that users would likely choose
> intentionally, I don't see a reason why it shouldn't work.
> > >
> > > Using module_init() or builtin_platform_driver() here would make
> > > give similar behavior for the built-in and modular cases and be
> > > somewhat more consistent, so you don't run into bugs only when the
> > > driver is a loadable module but make them obvious even to existing users
> with a builtin driver.
> > >
> >
> > My original idea of adding loadable module support for SoC specific
> > module is, try to keep it exactly same when the driver is built-in,
> > but for GKI support, first, we need to support GPIO driver built as
> > module, and we definitely need to think about the module load sequence
> > to handle these dependency, but thinking about the common module
> > widely used by devices, such as pinctrl, clock and GPIO, maybe other
> modules need some patches to handle the dependency, but that will be done
> later when we are working for those modules.
> 
> Overall, my feeling is that making sure all drivers that depend on the pinctrl
> driver can deal with deferred probing is a prerequisite before this can be made
> a loadable module itself (same for clk, irqchip, etc drivers that others may rely
> on).
> 
> I understand that your primary motivation is to fit into Google's GKI
> framework, but I think that doing the conversion only partially would neither
> serve to improve the kernel nor actually meet the GKI requirements.
> 
> Most pinctrl drivers are currently always built-in to work around the load order
> dependencies. This of course is a bit of a hack and we'd be better off if all
> drivers managed to avoid the dependencies, but this can also require a lot of
> work.
> 
> > So, could you please help advise how to proceed it for this GPIO
> > driver to  support loadable module?
> 
> I would start by getting a reference board to work with a kernel in which all
> drivers are built-in except for the pinctrl driver, to see what exactly breaks
> when you do that, and what other drivers may have the same problems.
> Maybe it's not that bad after all and you only need a few modifications.
> 

I agreed, but the situation is i.MX SoC contains more than 20 modules, and most of
them are NOT owned by me, so I am NOT sure when the module owner will start
working on the support. And if with minimum devices enabled, such as tiny kernel
with ramfs, it is working even with pinctrl/clock etc. built as loadable module.

Meanwhile, as you said, most of the users are still using built-in model, so adding the
support for GPIO can be in parallel with other modules' work, in other words, with this
GPIO loadable module support patch, if other modules can NOT work due to lack of
defer probe implementation, then the patch should be done in other module, adding
that the default configuration of GPIO is still built-in, do you think it can be an independent
patch and get into linux-next first?

Thanks,
Anson

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Arnd Bergmann]

On Mon, Jul 27, 2020 at 2:23 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> >
> > > So, could you please help advise how to proceed it for this GPIO
> > > driver to  support loadable module?
> >
> > I would start by getting a reference board to work with a kernel in which all
> > drivers are built-in except for the pinctrl driver, to see what exactly breaks
> > when you do that, and what other drivers may have the same problems.
> > Maybe it's not that bad after all and you only need a few modifications.
> >
>
> I agreed, but the situation is i.MX SoC contains more than 20 modules, and most of
> them are NOT owned by me, so I am NOT sure when the module owner will start
> working on the support. And if with minimum devices enabled, such as tiny kernel
> with ramfs, it is working even with pinctrl/clock etc. built as loadable module.

Do you have an example that is actually broken? I checked how the gpio
chip is actually used and found that "regulator-fixed", "virtual,mdio-gpio",
"regulator-gpio", "gpio-leds", "marvell,mv88e6085", "microchip,usb2513b",
"fsl,imx7d-usdhc", "fsl,imx6sx-fec", "mmc-pwrseq-simple", "brcm,bcm43438-bt",
 "rohm,bd71837", "nxp,pca9546",  "rtc-m41t80", should all work fine here.

I'm not sure about "fsl,mma8451", maybe test that one manually or look
at the driver in more detail.

"fsl,imx8mq-pcie" looks broken but easily fixed, and this is something we
have already discussed.

imx8mq-nitrogen.c has a "vsel-gpios" property in its "fcs,fan53555"
device node that is neither part of the binding nor handled by the
driver, so this is broken regardless of the gpio driver.

> Meanwhile, as you said, most of the users are still using built-in model, so adding the
> support for GPIO can be in parallel with other modules' work, in other words, with this
> GPIO loadable module support patch, if other modules can NOT work due to lack of
> defer probe implementation, then the patch should be done in other module, adding
> that the default configuration of GPIO is still built-in, do you think it can be an independent
> patch and get into linux-next first?

I think you should be reasonably sure that making the driver a loadable module
does not break other drivers that might rely on the probe order and
that are known
to be used with an i.MX chip. With the list above, that seems to actually be
the case for the most part, but testing is always better.

If there are boards that use other drivers which do not support deferred probing
but don't have those listed in the dts files in the kernel, then that
is not something
you have to worry about I think.

I'll let Linus Walleij comment on whether he thinks the initcall should stay
at subsys_initcall() to avoid breaking users with buggy drivers, or whether
this should be changed to module_init() or builtin_platform_driver() to
have a better chance of finding and fixing those broken drivers.

     Arnd

RE: [PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Hi, Linus
	Do you have chance to take a look at this patch series?

Thanks,
Anson


> Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> 
> On Mon, Jul 27, 2020 at 2:23 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> > >
> > > > So, could you please help advise how to proceed it for this GPIO
> > > > driver to  support loadable module?
> > >
> > > I would start by getting a reference board to work with a kernel in
> > > which all drivers are built-in except for the pinctrl driver, to see
> > > what exactly breaks when you do that, and what other drivers may have
> the same problems.
> > > Maybe it's not that bad after all and you only need a few modifications.
> > >
> >
> > I agreed, but the situation is i.MX SoC contains more than 20 modules,
> > and most of them are NOT owned by me, so I am NOT sure when the module
> > owner will start working on the support. And if with minimum devices
> > enabled, such as tiny kernel with ramfs, it is working even with pinctrl/clock
> etc. built as loadable module.
> 
> Do you have an example that is actually broken? I checked how the gpio chip
> is actually used and found that "regulator-fixed", "virtual,mdio-gpio",
> "regulator-gpio", "gpio-leds", "marvell,mv88e6085", "microchip,usb2513b",
> "fsl,imx7d-usdhc", "fsl,imx6sx-fec", "mmc-pwrseq-simple",
> "brcm,bcm43438-bt",  "rohm,bd71837", "nxp,pca9546",  "rtc-m41t80",
> should all work fine here.
> 
> I'm not sure about "fsl,mma8451", maybe test that one manually or look at
> the driver in more detail.
> 
> "fsl,imx8mq-pcie" looks broken but easily fixed, and this is something we have
> already discussed.
> 
> imx8mq-nitrogen.c has a "vsel-gpios" property in its "fcs,fan53555"
> device node that is neither part of the binding nor handled by the driver, so
> this is broken regardless of the gpio driver.
> 
> > Meanwhile, as you said, most of the users are still using built-in
> > model, so adding the support for GPIO can be in parallel with other
> > modules' work, in other words, with this GPIO loadable module support
> > patch, if other modules can NOT work due to lack of defer probe
> > implementation, then the patch should be done in other module, adding
> > that the default configuration of GPIO is still built-in, do you think it can be
> an independent patch and get into linux-next first?
> 
> I think you should be reasonably sure that making the driver a loadable
> module does not break other drivers that might rely on the probe order and
> that are known to be used with an i.MX chip. With the list above, that seems
> to actually be the case for the most part, but testing is always better.
> 
> If there are boards that use other drivers which do not support deferred
> probing but don't have those listed in the dts files in the kernel, then that is
> not something you have to worry about I think.
> 
> I'll let Linus Walleij comment on whether he thinks the initcall should stay at
> subsys_initcall() to avoid breaking users with buggy drivers, or whether this
> should be changed to module_init() or builtin_platform_driver() to have a
> better chance of finding and fixing those broken drivers.
> 
>      Arnd

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Linus Walleij]

On Thu, Sep 3, 2020 at 3:31 AM Anson Huang <anson.huang@nxp.com> wrote:

> Hi, Linus
>         Do you have chance to take a look at this patch series?

I've seen there was plenty discussion about them, like this one,
so I expected a repost (didn't anything change at all?) also a rebase
on v5.9-rc1 so I do not have to deal with any merge conflicts.

Could you rebase and resend?

Yours,
Linus Walleij

RE: [PATCH V2 1/4] gpio: mxc: Support module build

[Anson Huang]

Hi, Linus

> Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> 
> On Thu, Sep 3, 2020 at 3:31 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> 
> > Hi, Linus
> >         Do you have chance to take a look at this patch series?
> 
> I've seen there was plenty discussion about them, like this one, so I expected a
> repost (didn't anything change at all?) also a rebase on v5.9-rc1 so I do not
> have to deal with any merge conflicts.

I went through the discussion again, the main concern is whether need to support
unload if the driver supports module build, but for SoC GPIO driver, as john said, it is
commonly used by peripheral devices, similar as clock, pinctrl driver, sorting out the
unloading is particularly complicated or there is some missing infrastructure, and in
those cases being able to load a "permanent" module seems to me like a clear benefit.

So I think the first step is to just enable it as "permanent" modules like pinctrl/clk driver,
as currently, most of the SoC GPIO users are NOT supporting module build at all, so the
unload support for SoC GPIO driver is NOT available currently.

> 
> Could you rebase and resend?

I will rebase to latest linux-next and resend the patch series.

Thanks,
Anson

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Linus Walleij]

On Mon, Jul 27, 2020 at 1:57 PM Arnd Bergmann <arnd@arndb.de> wrote:

> Overall, my feeling is that making sure all drivers that depend on the pinctrl
> driver can deal with deferred probing is a prerequisite before this can be
> made a loadable module itself (same for clk, irqchip, etc drivers that others
> may rely on).
>
> I understand that your primary motivation is to fit into Google's GKI framework,
> but I think that doing the conversion only partially would neither serve to
> improve the kernel nor actually meet the GKI requirements.

This has been my worry as well when it comes to these GKI-initiated
patches that are flying right now.

> Most pinctrl drivers are currently always built-in to work around the
> load order dependencies. This of course is a bit of a hack and we'd be
> better off if all drivers managed to avoid the dependencies, but this
> can also require a lot of work.

Several people have argued that it is reasonable to cut corners to
achieve the "greater good" of GKI.

I try to handle it on a "does the kernel look better after than
before" basis, while pushing gently for at least trying to
properly modularize the whole thing. It can become pretty hard
to test I think. If it is things like GPIO expanders on I2C
that can be used by several SoCs I would be more hard on
this, on a single SoC not as much.

One discussion thread got inflamed because of ARM vs x86
discussions "x86 is better modularized" which is something I want
to avoid, it is easy to be modularized when your irqs, clocks,
regulators and pins are handled by the BIOS. This is a SoC
problem and x86 SoCs with no BIOS, RISCV, ARM and whatever
doesn't have a fix-it-all-BIOS have this problem. :/

Yours,
Linus Walleij

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Linus Walleij]

On Mon, Jul 27, 2020 at 12:44 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com> wrote:

> > > Why is this driver using syscore_ops rather than
> > > SIMPLE_DEV_PM_OPS() or similar?
> >
> > Below is the original patch of using syscore_ops, it has explanation:
> >
> > commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb
> > Author: Anson Huang <anson.huang@nxp.com>
> > Date:   Fri Nov 9 04:56:56 2018 +0000
> >
> >     gpio: mxc: move gpio noirq suspend/resume to syscore phase
> >
> >     During noirq suspend/resume phase, GPIO irq could arrive
> >     and its registers like IMR will be changed by irq handle
> >     process, to make the GPIO registers exactly when it is
> >     powered ON after resume, move the GPIO noirq suspend/resume
> >     callback to syscore suspend/resume phase, local irq is
> >     disabled at this phase so GPIO registers are atomic.

This looks like it would have been easier to use
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS
as pointed out later...

> The description makes sense, but this must be a problem that
> other gpio/pinctrl irqchip drivers have as well.
>
> Linus, could you have a look? I see these other pinctrl drivers
> using SIMPLE_DEV_PM_OPS:
>
> drivers/pinctrl/nomadik/pinctrl-nomadik.c:static
> SIMPLE_DEV_PM_OPS(nmk_pinctrl_pm_ops,

This one does not involve IRQs rather calls
pinctrl_force_sleep/default which sets up hogged
pins for energy saving while sleeping.

> drivers/pinctrl/pinctrl-rockchip.c:static
> SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops,
> rockchip_pinctrl_suspend,

Pretty much the same as Nomadik, with some extra
register (also not IRQ-related).

> drivers/pinctrl/pinctrl-stmfx.c:static
> SIMPLE_DEV_PM_OPS(stmfx_pinctrl_dev_pm_ops,

This one is problematic. However this is on an I2C
expander meaning the slow bus traffic needs to be
working and if IRQs are off at syscore suspend/resume
time, I2C will not work. I think Amelie has tested this thing
pretty thoroughly, and that expanders are less sensitive
to this.

> drivers/pinctrl/qcom/pinctrl-msm.c:SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_pm_ops,
> msm_pinctrl_suspend,

This one is like the Nomadik: just forcing some hogs to
go into low power mode.

> drivers/pinctrl/spear/pinctrl-plgpio.c:static
> SIMPLE_DEV_PM_OPS(plgpio_dev_pm_ops, plgpio_suspend, plgpio_resume);

This one is affected by the same problem, I don't know if anyone
really has this hardware anymore, but there are some
SPEAr products deployed so the users should be notified
that they may need to move this to syscore ops.

Viresh?

> It seems that some drivers use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS()
> instead, which looks like it is meant to address the same problem, but
> as I have not used that myself, I may be misunderstanding the problem
> or what this one does.

IIUC that callback is for exactly this, and occurs after IRQs
are disabled and before IRQs are
re-enabled. Again the same problem if you need slow bus
traffic in your callback (I2C/SPI devices): it is not going to work.

Yours,
Linus Walleij

Re: [PATCH V2 1/4] gpio: mxc: Support module build

[Viresh Kumar]

On 28-07-20, 09:59, Linus Walleij wrote:
> On Mon, Jul 27, 2020 at 12:44 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@nxp.com> wrote:
> > drivers/pinctrl/spear/pinctrl-plgpio.c:static
> > SIMPLE_DEV_PM_OPS(plgpio_dev_pm_ops, plgpio_suspend, plgpio_resume);
> 
> This one is affected by the same problem, I don't know if anyone
> really has this hardware anymore, but there are some
> SPEAr products deployed so the users should be notified
> that they may need to move this to syscore ops.
> 
> Viresh?

Yeah, I can't test the patches but Ack them if someone can send them to me or if
someone can tell me exactly what to do here.

-- 
viresh