| Message ID | 1595382353-17486-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/4] gpio: mxc: Support module build | expand |
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
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
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
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
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
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
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
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
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
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.
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
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
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
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");
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(-)