| Message ID | 20180802111144.12512-1-digetx@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v1,1/2] pinctrl: tegra: Move drivers registration to arch_init level | expand |
On 02.08.2018 13:11, Dmitry Osipenko wrote: > There is a bug in regards to deferred probing within the drivers core > that causes GPIO-driver to suspend after its users. The bug appears if > GPIO-driver probe is getting deferred, which happens after introducing > dependency on PINCTRL-driver for the GPIO-driver by defining "gpio-ranges" > property in device-tree. The bug in the drivers core is old (more than 4 > years now) and is well known, unfortunately there is no easy fix for it. > The good news is that we can workaround the deferred probe issue by > changing GPIO / PINCTRL drivers registration order and hence by moving > PINCTRL driver registration to the arch_init level and GPIO to the > subsys_init. A while back at least using those init lists were not well received even for GPIO/pinctrl drivers: https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J_azOic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba I still think we should make an exception for GPIO/pinctrl and use earlier initcalls. Platform GPIO/pinctrl drivers provide basic infrastructure often used by many other drivers, we want to have them loaded early. It avoids unnecessary EPROBE_DEFER and hence probably even boots faster. And in this case it even resolves real world issues. This should definitely go in, at least as a stop gap solution. Acked-by: Stefan Agner <stefan@agner.ch> -- Stefan > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/pinctrl/tegra/pinctrl-tegra114.c | 7 ++++++- > drivers/pinctrl/tegra/pinctrl-tegra124.c | 7 ++++++- > drivers/pinctrl/tegra/pinctrl-tegra20.c | 7 ++++++- > drivers/pinctrl/tegra/pinctrl-tegra210.c | 7 ++++++- > drivers/pinctrl/tegra/pinctrl-tegra30.c | 7 ++++++- > 5 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra114.c > b/drivers/pinctrl/tegra/pinctrl-tegra114.c > index 511a8774fd8d..d43c209e9c30 100644 > --- a/drivers/pinctrl/tegra/pinctrl-tegra114.c > +++ b/drivers/pinctrl/tegra/pinctrl-tegra114.c > @@ -1868,4 +1868,9 @@ static struct platform_driver tegra114_pinctrl_driver = { > }, > .probe = tegra114_pinctrl_probe, > }; > -builtin_platform_driver(tegra114_pinctrl_driver); > + > +static int __init tegra114_pinctrl_init(void) > +{ > + return platform_driver_register(&tegra114_pinctrl_driver); > +} > +arch_initcall(tegra114_pinctrl_init); > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra124.c > b/drivers/pinctrl/tegra/pinctrl-tegra124.c > index 57e3cdcf4503..5b07a5834d15 100644 > --- a/drivers/pinctrl/tegra/pinctrl-tegra124.c > +++ b/drivers/pinctrl/tegra/pinctrl-tegra124.c > @@ -2080,4 +2080,9 @@ static struct platform_driver tegra124_pinctrl_driver = { > }, > .probe = tegra124_pinctrl_probe, > }; > -builtin_platform_driver(tegra124_pinctrl_driver); > + > +static int __init tegra124_pinctrl_init(void) > +{ > + return platform_driver_register(&tegra124_pinctrl_driver); > +} > +arch_initcall(tegra124_pinctrl_init); > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c > b/drivers/pinctrl/tegra/pinctrl-tegra20.c > index 624889ed3a9d..1fc82a9576e0 100644 > --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c > +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c > @@ -2277,4 +2277,9 @@ static struct platform_driver tegra20_pinctrl_driver = { > }, > .probe = tegra20_pinctrl_probe, > }; > -builtin_platform_driver(tegra20_pinctrl_driver); > + > +static int __init tegra20_pinctrl_init(void) > +{ > + return platform_driver_register(&tegra20_pinctrl_driver); > +} > +arch_initcall(tegra20_pinctrl_init); > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c > b/drivers/pinctrl/tegra/pinctrl-tegra210.c > index 0956a1c73391..3e77f5474dd8 100644 > --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c > +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c > @@ -1582,4 +1582,9 @@ static struct platform_driver tegra210_pinctrl_driver = { > }, > .probe = tegra210_pinctrl_probe, > }; > -builtin_platform_driver(tegra210_pinctrl_driver); > + > +static int __init tegra210_pinctrl_init(void) > +{ > + return platform_driver_register(&tegra210_pinctrl_driver); > +} > +arch_initcall(tegra210_pinctrl_init); > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra30.c > b/drivers/pinctrl/tegra/pinctrl-tegra30.c > index c923ad58af84..10e617003e9c 100644 > --- a/drivers/pinctrl/tegra/pinctrl-tegra30.c > +++ b/drivers/pinctrl/tegra/pinctrl-tegra30.c > @@ -2503,4 +2503,9 @@ static struct platform_driver tegra30_pinctrl_driver = { > }, > .probe = tegra30_pinctrl_probe, > }; > -builtin_platform_driver(tegra30_pinctrl_driver); > + > +static int __init tegra30_pinctrl_init(void) > +{ > + return platform_driver_register(&tegra30_pinctrl_driver); > +} > +arch_initcall(tegra30_pinctrl_init); -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, 2 August 2018 14:11:43 MSK Dmitry Osipenko wrote: > There is a bug in regards to deferred probing within the drivers core > that causes GPIO-driver to suspend after its users. The bug appears if I meant "before its users", of course. If the rest of the patches is fine, please let me know if re-sending is needed or you'll correct the comment in place while applying. > GPIO-driver probe is getting deferred, which happens after introducing > dependency on PINCTRL-driver for the GPIO-driver by defining "gpio-ranges" > property in device-tree. The bug in the drivers core is old (more than 4 > years now) and is well known, unfortunately there is no easy fix for it. > The good news is that we can workaround the deferred probe issue by > changing GPIO / PINCTRL drivers registration order and hence by moving > PINCTRL driver registration to the arch_init level and GPIO to the > subsys_init. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 2, 2018 at 1:12 PM Dmitry Osipenko <digetx@gmail.com> wrote: > There is a bug in regards to deferred probing within the drivers core > that causes GPIO-driver to suspend after its users. The bug appears if > GPIO-driver probe is getting deferred, which happens after introducing > dependency on PINCTRL-driver for the GPIO-driver by defining "gpio-ranges" > property in device-tree. The bug in the drivers core is old (more than 4 > years now) and is well known, unfortunately there is no easy fix for it. > The good news is that we can workaround the deferred probe issue by > changing GPIO / PINCTRL drivers registration order and hence by moving > PINCTRL driver registration to the arch_init level and GPIO to the > subsys_init. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Patch applied with Stefan's ACK. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <stefan@agner.ch> wrote: > A while back at least using those init lists were not well received even > for GPIO/pinctrl drivers: > > https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J_azOic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba You shouldn't listen too much to that guy he's not trustworthy. > I still think we should make an exception for GPIO/pinctrl and use > earlier initcalls. Platform GPIO/pinctrl drivers provide basic > infrastructure often used by many other drivers, we want to have them > loaded early. It avoids unnecessary EPROBE_DEFER and hence probably even > boots faster. When we have the pin control and GPIO at different initlevels it makes me uneasy because I feel we have implicit init dependencies that seem more than a little fragile. My recent thinking has involved the component method used in DRM drivers such as drivers/gpu/drm/vc4/vc4_drv.c where a few different component subdrivers are linked together at bind time (not probe time!) into a master component. Rob was no big fan of this but the DRM people like it and I was thinking to make a try at it. This way we could at least probe and bind the pin control and GPIO drivers at the *same* initlevel and express the dependencies between them somewhat. > This should definitely go in, at least as a stop gap solution. Agreed. (And patch applied.) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, 3 August 2018 20:24:56 MSK Linus Walleij wrote: > On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <stefan@agner.ch> wrote: > > A while back at least using those init lists were not well received even > > for GPIO/pinctrl drivers: > > > > https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J_az > > Oic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba > You shouldn't listen too much to that guy he's not trustworthy. > > > I still think we should make an exception for GPIO/pinctrl and use > > earlier initcalls. Platform GPIO/pinctrl drivers provide basic > > infrastructure often used by many other drivers, we want to have them > > loaded early. It avoids unnecessary EPROBE_DEFER and hence probably even > > boots faster. > > When we have the pin control and GPIO at different initlevels it makes me > uneasy because I feel we have implicit init dependencies that seem more > than a little fragile. Yes, it is not very good. > My recent thinking has involved the component method used in DRM drivers > such as drivers/gpu/drm/vc4/vc4_drv.c where a few different component > subdrivers are linked together at bind time (not probe time!) into a master > component. > Rob was no big fan of this but the DRM people like it and I was thinking to > make a try at it. > > This way we could at least probe and bind the pin control and GPIO drivers > at the *same* initlevel and express the dependencies between them > somewhat. Sounds interesting, maybe you could help to convert Tegra drivers to a such method and others will follow afterwards. > > This should definitely go in, at least as a stop gap solution. > > Agreed. (And patch applied.) The best solution will be to fix the deferred probing, it's awkward that it could break suspend-resume order. Hopefully somebody with a good knowledge of driver/base will manage to fix it eventually. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, 2 August 2018 16:01:24 MSK Dmitry Osipenko wrote: > On Thursday, 2 August 2018 14:11:43 MSK Dmitry Osipenko wrote: > > There is a bug in regards to deferred probing within the drivers core > > that causes GPIO-driver to suspend after its users. The bug appears if > > I meant "before its users", of course. If the rest of the patches is fine, > please let me know if re-sending is needed or you'll correct the comment in > place while applying. Linus, please pick up the v2. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 6, 2018 at 11:15 AM Dmitry Osipenko <digetx@gmail.com> wrote: > On Thursday, 2 August 2018 16:01:24 MSK Dmitry Osipenko wrote: > > On Thursday, 2 August 2018 14:11:43 MSK Dmitry Osipenko wrote: > > > There is a bug in regards to deferred probing within the drivers core > > > that causes GPIO-driver to suspend after its users. The bug appears if > > > > I meant "before its users", of course. If the rest of the patches is fine, > > please let me know if re-sending is needed or you'll correct the comment in > > place while applying. > > Linus, please pick up the v2. It's no big deal if the only difference is the commit message. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.08.2018 16:01, Dmitry Osipenko wrote: > On Friday, 3 August 2018 20:24:56 MSK Linus Walleij wrote: >> On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <stefan@agner.ch> wrote: >> > A while back at least using those init lists were not well received even >> > for GPIO/pinctrl drivers: >> > >> > https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J_az >> > Oic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba >> You shouldn't listen too much to that guy he's not trustworthy. ;-) >> >> > I still think we should make an exception for GPIO/pinctrl and use >> > earlier initcalls. Platform GPIO/pinctrl drivers provide basic >> > infrastructure often used by many other drivers, we want to have them >> > loaded early. It avoids unnecessary EPROBE_DEFER and hence probably even >> > boots faster. >> >> When we have the pin control and GPIO at different initlevels it makes me >> uneasy because I feel we have implicit init dependencies that seem more >> than a little fragile. > > Yes, it is not very good. > Btw, just noticed this now: GPIO driver -> arch_initcall pinctrl driver -> subsys_initcall And arch is before subsys. So we initialize GPIO driver first? But isn't pinctrl required for the GPIO range? Afaik, especially with gpio-ranges enabled, the GPIO probe will return -EPROBE_DEFER (I think due to pinctrl_get_device_gpio_range). So my intuition would be that it should be the other way around... -- Stefan >> My recent thinking has involved the component method used in DRM drivers >> such as drivers/gpu/drm/vc4/vc4_drv.c where a few different component >> subdrivers are linked together at bind time (not probe time!) into a master >> component. >> Rob was no big fan of this but the DRM people like it and I was thinking to >> make a try at it. >> >> This way we could at least probe and bind the pin control and GPIO drivers >> at the *same* initlevel and express the dependencies between them >> somewhat. > > Sounds interesting, maybe you could help to convert Tegra drivers to a such > method and others will follow afterwards. > >> > This should definitely go in, at least as a stop gap solution. >> >> Agreed. (And patch applied.) > > The best solution will be to fix the deferred probing, it's awkward that it > could break suspend-resume order. Hopefully somebody with a good knowledge of > driver/base will manage to fix it eventually. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, 6 August 2018 16:03:01 MSK Stefan Agner wrote: > On 04.08.2018 16:01, Dmitry Osipenko wrote: > > On Friday, 3 August 2018 20:24:56 MSK Linus Walleij wrote: > >> On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <stefan@agner.ch> wrote: > >> > A while back at least using those init lists were not well received > >> > even > >> > for GPIO/pinctrl drivers: > >> > > >> > https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J > >> > _az > >> > Oic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba > >> > >> You shouldn't listen too much to that guy he's not trustworthy. > > ;-) > > >> > I still think we should make an exception for GPIO/pinctrl and use > >> > earlier initcalls. Platform GPIO/pinctrl drivers provide basic > >> > infrastructure often used by many other drivers, we want to have them > >> > loaded early. It avoids unnecessary EPROBE_DEFER and hence probably > >> > even > >> > boots faster. > >> > >> When we have the pin control and GPIO at different initlevels it makes me > >> uneasy because I feel we have implicit init dependencies that seem more > >> than a little fragile. > > > > Yes, it is not very good. > > Btw, just noticed this now: > GPIO driver -> arch_initcall > pinctrl driver -> subsys_initcall I'm not sure what you're talking about, it's the other way around in the patches. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06.08.2018 15:38, Dmitry Osipenko wrote: > On Monday, 6 August 2018 16:03:01 MSK Stefan Agner wrote: >> On 04.08.2018 16:01, Dmitry Osipenko wrote: >> > On Friday, 3 August 2018 20:24:56 MSK Linus Walleij wrote: >> >> On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <stefan@agner.ch> wrote: >> >> > A while back at least using those init lists were not well received >> >> > even >> >> > for GPIO/pinctrl drivers: >> >> > >> >> > https://lore.kernel.org/lkml/CACRpkdYk0zW12qNXgOstTLmdVDYacu0Un+8quTN+J >> >> > _az >> >> > Oic7AA@mail.gmail.com/T/#mf0596982324a6489b5537b0531ac5aed60a316ba >> >> >> >> You shouldn't listen too much to that guy he's not trustworthy. >> >> ;-) >> >> >> > I still think we should make an exception for GPIO/pinctrl and use >> >> > earlier initcalls. Platform GPIO/pinctrl drivers provide basic >> >> > infrastructure often used by many other drivers, we want to have them >> >> > loaded early. It avoids unnecessary EPROBE_DEFER and hence probably >> >> > even >> >> > boots faster. >> >> >> >> When we have the pin control and GPIO at different initlevels it makes me >> >> uneasy because I feel we have implicit init dependencies that seem more >> >> than a little fragile. >> > >> > Yes, it is not very good. >> >> Btw, just noticed this now: >> GPIO driver -> arch_initcall >> pinctrl driver -> subsys_initcall > > I'm not sure what you're talking about, it's the other way around in the > patches. Wow, yeah sorry... That must be the heat in our office ':-) -- Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra114.c b/drivers/pinctrl/tegra/pinctrl-tegra114.c index 511a8774fd8d..d43c209e9c30 100644 --- a/drivers/pinctrl/tegra/pinctrl-tegra114.c +++ b/drivers/pinctrl/tegra/pinctrl-tegra114.c @@ -1868,4 +1868,9 @@ static struct platform_driver tegra114_pinctrl_driver = { }, .probe = tegra114_pinctrl_probe, }; -builtin_platform_driver(tegra114_pinctrl_driver); + +static int __init tegra114_pinctrl_init(void) +{ + return platform_driver_register(&tegra114_pinctrl_driver); +} +arch_initcall(tegra114_pinctrl_init); diff --git a/drivers/pinctrl/tegra/pinctrl-tegra124.c b/drivers/pinctrl/tegra/pinctrl-tegra124.c index 57e3cdcf4503..5b07a5834d15 100644 --- a/drivers/pinctrl/tegra/pinctrl-tegra124.c +++ b/drivers/pinctrl/tegra/pinctrl-tegra124.c @@ -2080,4 +2080,9 @@ static struct platform_driver tegra124_pinctrl_driver = { }, .probe = tegra124_pinctrl_probe, }; -builtin_platform_driver(tegra124_pinctrl_driver); + +static int __init tegra124_pinctrl_init(void) +{ + return platform_driver_register(&tegra124_pinctrl_driver); +} +arch_initcall(tegra124_pinctrl_init); diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c b/drivers/pinctrl/tegra/pinctrl-tegra20.c index 624889ed3a9d..1fc82a9576e0 100644 --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c @@ -2277,4 +2277,9 @@ static struct platform_driver tegra20_pinctrl_driver = { }, .probe = tegra20_pinctrl_probe, }; -builtin_platform_driver(tegra20_pinctrl_driver); + +static int __init tegra20_pinctrl_init(void) +{ + return platform_driver_register(&tegra20_pinctrl_driver); +} +arch_initcall(tegra20_pinctrl_init); diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c index 0956a1c73391..3e77f5474dd8 100644 --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c @@ -1582,4 +1582,9 @@ static struct platform_driver tegra210_pinctrl_driver = { }, .probe = tegra210_pinctrl_probe, }; -builtin_platform_driver(tegra210_pinctrl_driver); + +static int __init tegra210_pinctrl_init(void) +{ + return platform_driver_register(&tegra210_pinctrl_driver); +} +arch_initcall(tegra210_pinctrl_init); diff --git a/drivers/pinctrl/tegra/pinctrl-tegra30.c b/drivers/pinctrl/tegra/pinctrl-tegra30.c index c923ad58af84..10e617003e9c 100644 --- a/drivers/pinctrl/tegra/pinctrl-tegra30.c +++ b/drivers/pinctrl/tegra/pinctrl-tegra30.c @@ -2503,4 +2503,9 @@ static struct platform_driver tegra30_pinctrl_driver = { }, .probe = tegra30_pinctrl_probe, }; -builtin_platform_driver(tegra30_pinctrl_driver); + +static int __init tegra30_pinctrl_init(void) +{ + return platform_driver_register(&tegra30_pinctrl_driver); +} +arch_initcall(tegra30_pinctrl_init);
There is a bug in regards to deferred probing within the drivers core that causes GPIO-driver to suspend after its users. The bug appears if GPIO-driver probe is getting deferred, which happens after introducing dependency on PINCTRL-driver for the GPIO-driver by defining "gpio-ranges" property in device-tree. The bug in the drivers core is old (more than 4 years now) and is well known, unfortunately there is no easy fix for it. The good news is that we can workaround the deferred probe issue by changing GPIO / PINCTRL drivers registration order and hence by moving PINCTRL driver registration to the arch_init level and GPIO to the subsys_init. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/pinctrl/tegra/pinctrl-tegra114.c | 7 ++++++- drivers/pinctrl/tegra/pinctrl-tegra124.c | 7 ++++++- drivers/pinctrl/tegra/pinctrl-tegra20.c | 7 ++++++- drivers/pinctrl/tegra/pinctrl-tegra210.c | 7 ++++++- drivers/pinctrl/tegra/pinctrl-tegra30.c | 7 ++++++- 5 files changed, 30 insertions(+), 5 deletions(-)