diff mbox series

pinctrl: initialise nsp-mux earlier.

Message ID 20200630212958.24030-1-mark.tomlinson@alliedtelesis.co.nz
State New
Headers show
Series pinctrl: initialise nsp-mux earlier. | expand

Commit Message

Mark Tomlinson June 30, 2020, 9:29 p.m. UTC
The GPIO specified in the DTS file references the pinctrl, which is
specified after the GPIO. If the GPIO is initialised before pinctrl,
an error message for the -EPROBE_DEFER ends up in the kernel log. Even
though the probe will succeed when the driver is re-initialised, the
error can be scary to end users. To fix this, change the time the
pinctrl is probed, so that it is always before the GPIO driver.

Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/pinctrl/bcm/pinctrl-nsp-mux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ray Jui June 30, 2020, 10:08 p.m. UTC | #1
Hi Mark,

On 6/30/2020 2:29 PM, Mark Tomlinson wrote:
> The GPIO specified in the DTS file references the pinctrl, which is
> specified after the GPIO. If the GPIO is initialised before pinctrl,

May I know which GPIO driver you are referring to on NSP? Both the iProc
GPIO driver and the NSP GPIO driver are initialized at the level of
'arch_initcall_sync', which is supposed to be after 'arch_initcall' used
here in the pinmux driver

> an error message for the -EPROBE_DEFER ends up in the kernel log. Even
> though the probe will succeed when the driver is re-initialised, the
> error can be scary to end users. To fix this, change the time the

Scary to end users? I don't know about that. -EPROBE_DEFER was
introduced exactly for this purpose. Perhaps users need to learn what
-EPROBE_DEFER errno means?

> pinctrl is probed, so that it is always before the GPIO driver.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/pinctrl/bcm/pinctrl-nsp-mux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> index f1d60a708815..7586949f83ec 100644
> --- a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> @@ -639,4 +639,4 @@ static int __init nsp_pinmux_init(void)
>  {
>  	return platform_driver_register(&nsp_pinmux_driver);
>  }
> -arch_initcall(nsp_pinmux_init);
> +postcore_initcall(nsp_pinmux_init);
>
Mark Tomlinson July 1, 2020, 2:23 a.m. UTC | #2
On Tue, 2020-06-30 at 15:08 -0700, Ray Jui wrote:
> May I know which GPIO driver you are referring to on NSP? Both the iProc
> GPIO driver and the NSP GPIO driver are initialized at the level of
> 'arch_initcall_sync', which is supposed to be after 'arch_initcall' used
> here in the pinmux driver

Sorry, it looks like I made a mistake in my testing (or I was lucky),
and this patch doesn't fix the issue. What is happening is:
1) nsp-pinmux driver is registered (arch_initcall).
2) nsp-gpio-a driver is registered (arch_initcall_sync).
3) of_platform_default_populate_init() is called (also at level
arch_initcall_sync), which scans the device tree, adds the nsp-gpio-a
device, runs its probe, and this returns -EPROBE_DEFER with the error
message.
4) Only now nsp-pinmux device is probed.

Changing the 'arch_initcall_sync' to 'device_initcall' in nsp-gpio-a
ensures that the pinmux is probed first since
of_platform_default_populate_init() will be called between the two
register calls, and the error goes away. Is this change acceptable as a
solution?

> > though the probe will succeed when the driver is re-initialised, the
> > error can be scary to end users. To fix this, change the time the
> 
> Scary to end users? I don't know about that. -EPROBE_DEFER was
> introduced exactly for this purpose. Perhaps users need to learn what
> -EPROBE_DEFER errno means?

The actual error message in syslog is:

kern.err kernel: gpiochip_add_data_with_key: GPIOs 480..511
(18000020.gpio) failed to register, -517

So an end user sees "err" and "failed", and doesn't know what "-517"
means.
Florian Fainelli July 1, 2020, 3:14 a.m. UTC | #3
On 6/30/2020 7:23 PM, Mark Tomlinson wrote:
> On Tue, 2020-06-30 at 15:08 -0700, Ray Jui wrote:
>> May I know which GPIO driver you are referring to on NSP? Both the iProc
>> GPIO driver and the NSP GPIO driver are initialized at the level of
>> 'arch_initcall_sync', which is supposed to be after 'arch_initcall' used
>> here in the pinmux driver
> 
> Sorry, it looks like I made a mistake in my testing (or I was lucky),
> and this patch doesn't fix the issue. What is happening is:
> 1) nsp-pinmux driver is registered (arch_initcall).
> 2) nsp-gpio-a driver is registered (arch_initcall_sync).
> 3) of_platform_default_populate_init() is called (also at level
> arch_initcall_sync), which scans the device tree, adds the nsp-gpio-a
> device, runs its probe, and this returns -EPROBE_DEFER with the error
> message.
> 4) Only now nsp-pinmux device is probed.
> 
> Changing the 'arch_initcall_sync' to 'device_initcall' in nsp-gpio-a
> ensures that the pinmux is probed first since
> of_platform_default_populate_init() will be called between the two
> register calls, and the error goes away. Is this change acceptable as a
> solution?

If probe deferral did not work, certainly but it sounds like this is
being done just for the sake of eliminating a round of probe deferral,
is there a functional problem this is fixing?

> 
>>> though the probe will succeed when the driver is re-initialised, the
>>> error can be scary to end users. To fix this, change the time the
>>
>> Scary to end users? I don't know about that. -EPROBE_DEFER was
>> introduced exactly for this purpose. Perhaps users need to learn what
>> -EPROBE_DEFER errno means?
> 
> The actual error message in syslog is:
> 
> kern.err kernel: gpiochip_add_data_with_key: GPIOs 480..511
> (18000020.gpio) failed to register, -517
> 
> So an end user sees "err" and "failed", and doesn't know what "-517"
> means.

How about this instead:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4fa075d49fbc..10d9d0c17c9e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1818,9 +1818,10 @@ int gpiochip_add_data_with_key(struct gpio_chip
*gc, void *data,
        ida_simple_remove(&gpio_ida, gdev->id);
 err_free_gdev:
        /* failures here can mean systems won't boot... */
-       pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
-              gdev->base, gdev->base + gdev->ngpio - 1,
-              gc->label ? : "generic", ret);
+       if (ret != -EPROBE_DEFER)
+               pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n",
+                       __func__, gdev->base, gdev->base + gdev->ngpio - 1,
+                       gc->label ? : "generic", ret);
        kfree(gdev);
        return ret;
 }
Mark Tomlinson July 1, 2020, 4:37 a.m. UTC | #4
On Tue, 2020-06-30 at 20:14 -0700, Florian Fainelli wrote:
> Sorry, it looks like I made a mistake in my testing (or I was lucky),
> > and this patch doesn't fix the issue. What is happening is:
> > 1) nsp-pinmux driver is registered (arch_initcall).
> > 2) nsp-gpio-a driver is registered (arch_initcall_sync).
> > 3) of_platform_default_populate_init() is called (also at level
> > arch_initcall_sync), which scans the device tree, adds the nsp-gpio-a
> > device, runs its probe, and this returns -EPROBE_DEFER with the error
> > message.
> > 4) Only now nsp-pinmux device is probed.
> > 
> > Changing the 'arch_initcall_sync' to 'device_initcall' in nsp-gpio-a
> > ensures that the pinmux is probed first since
> > of_platform_default_populate_init() will be called between the two
> > register calls, and the error goes away. Is this change acceptable as a
> > solution?
> 
> If probe deferral did not work, certainly but it sounds like this is
> being done just for the sake of eliminating a round of probe deferral,
> is there a functional problem this is fixing?

No, I'm just trying to prevent an "error" message appearing in syslog.

> > The actual error message in syslog is:
> > 
> > kern.err kernel: gpiochip_add_data_with_key: GPIOs 480..511
> > (18000020.gpio) failed to register, -517
> > 
> > So an end user sees "err" and "failed", and doesn't know what "-517"
> > means.
> 
> How about this instead:
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4fa075d49fbc..10d9d0c17c9e 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1818,9 +1818,10 @@ int gpiochip_add_data_with_key(struct gpio_chip
> *gc, void *data,
>         ida_simple_remove(&gpio_ida, gdev->id);
>  err_free_gdev:
>         /* failures here can mean systems won't boot... */
> -       pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
> -              gdev->base, gdev->base + gdev->ngpio - 1,
> -              gc->label ? : "generic", ret);
> +       if (ret != -EPROBE_DEFER)
> +               pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n",
> +                       __func__, gdev->base, gdev->base + gdev->ngpio - 1,
> +                       gc->label ? : "generic", ret);
>         kfree(gdev);
>         return ret;
>  }
> 
That was one of my thoughts too. I found someone had tried that
earlier, but it was rejected:


https://patchwork.ozlabs.org/project/linux-gpio/patch/1516566774-1786-1-git-send-email-david@lechnology.com/
Florian Fainelli July 1, 2020, 4:44 a.m. UTC | #5
On 6/30/2020 9:37 PM, Mark Tomlinson wrote:
> On Tue, 2020-06-30 at 20:14 -0700, Florian Fainelli wrote:
>> Sorry, it looks like I made a mistake in my testing (or I was lucky),
>>> and this patch doesn't fix the issue. What is happening is:
>>> 1) nsp-pinmux driver is registered (arch_initcall).
>>> 2) nsp-gpio-a driver is registered (arch_initcall_sync).
>>> 3) of_platform_default_populate_init() is called (also at level
>>> arch_initcall_sync), which scans the device tree, adds the nsp-gpio-a
>>> device, runs its probe, and this returns -EPROBE_DEFER with the error
>>> message.
>>> 4) Only now nsp-pinmux device is probed.
>>>
>>> Changing the 'arch_initcall_sync' to 'device_initcall' in nsp-gpio-a
>>> ensures that the pinmux is probed first since
>>> of_platform_default_populate_init() will be called between the two
>>> register calls, and the error goes away. Is this change acceptable as a
>>> solution?
>>
>> If probe deferral did not work, certainly but it sounds like this is
>> being done just for the sake of eliminating a round of probe deferral,
>> is there a functional problem this is fixing?
> 
> No, I'm just trying to prevent an "error" message appearing in syslog.
> 
>>> The actual error message in syslog is:
>>>
>>> kern.err kernel: gpiochip_add_data_with_key: GPIOs 480..511
>>> (18000020.gpio) failed to register, -517
>>>
>>> So an end user sees "err" and "failed", and doesn't know what "-517"
>>> means.
>>
>> How about this instead:
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 4fa075d49fbc..10d9d0c17c9e 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1818,9 +1818,10 @@ int gpiochip_add_data_with_key(struct gpio_chip
>> *gc, void *data,
>>         ida_simple_remove(&gpio_ida, gdev->id);
>>  err_free_gdev:
>>         /* failures here can mean systems won't boot... */
>> -       pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
>> -              gdev->base, gdev->base + gdev->ngpio - 1,
>> -              gc->label ? : "generic", ret);
>> +       if (ret != -EPROBE_DEFER)
>> +               pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n",
>> +                       __func__, gdev->base, gdev->base + gdev->ngpio - 1,
>> +                       gc->label ? : "generic", ret);
>>         kfree(gdev);
>>         return ret;
>>  }
>>
> That was one of my thoughts too. I found someone had tried that
> earlier, but it was rejected:
> 
> 
> https://patchwork.ozlabs.org/project/linux-gpio/patch/1516566774-1786-1-git-send-email-david@lechnology.com/

clk or reset APIs do not complain loudly on EPROBE_DEFER, it seems to me
that GPIO should follow here. Also, it does look like Linus was in
agreement in the end, not sure why it was not applied though.
diff mbox series

Patch

diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
index f1d60a708815..7586949f83ec 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
@@ -639,4 +639,4 @@  static int __init nsp_pinmux_init(void)
 {
 	return platform_driver_register(&nsp_pinmux_driver);
 }
-arch_initcall(nsp_pinmux_init);
+postcore_initcall(nsp_pinmux_init);