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 <[email protected]>
---
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);
--
2.18.0
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 <[email protected]>
---
drivers/gpio/gpio-tegra.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 2d940785bad0..a53c77db9744 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -712,4 +712,4 @@ static int __init tegra_gpio_init(void)
{
return platform_driver_register(&tegra_gpio_driver);
}
-postcore_initcall(tegra_gpio_init);
+subsys_initcall(tegra_gpio_init);
--
2.18.0
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 <[email protected]>
--
Stefan
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> 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);
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 <[email protected]>
On Thu, Aug 2, 2018 at 1:12 PM Dmitry Osipenko <[email protected]> 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 <[email protected]>
Patch applied with Stefan's ACK.
Yours,
Linus Walleij
On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <[email protected]> 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
On Friday, 3 August 2018 20:24:56 MSK Linus Walleij wrote:
> On Thu, Aug 2, 2018 at 1:31 PM Stefan Agner <[email protected]> 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
> > [email protected]/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.
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.
On Mon, Aug 6, 2018 at 11:15 AM Dmitry Osipenko <[email protected]> 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
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 <[email protected]> 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
>> > [email protected]/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.
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 <[email protected]> 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
> >> > [email protected]/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.
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 <[email protected]> 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
>> >> > [email protected]/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