| Message ID | 20200630212958.24030-1-mark.tomlinson@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: initialise nsp-mux earlier. | expand |
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); >
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.
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; }
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/
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.
On 6/30/2020 9:44 PM, Florian Fainelli wrote: > > > 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. > I think either we silently drop this or we explicitly make it obvious that it failed due to EPROBE_DEFER. Both seem acceptable to me. Thanks! Ray
On Wed, Jul 1, 2020 at 6:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 6/30/2020 9:37 PM, Mark Tomlinson wrote: > > 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. I never got an updated patch. My last message was: >> so you mean something like this? >> >> if (err == -EPROBE_DEFER) >> dev_info(dev, "deferring probe\n") >> else >> dev_err(dev, "... failed to register\n") > > Yes exactly. Patches welcome :D Yours, Linus Walleij
On 7/11/2020 2:07 PM, Linus Walleij wrote: > On Wed, Jul 1, 2020 at 6:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 6/30/2020 9:37 PM, Mark Tomlinson wrote: > >>> 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. > > I never got an updated patch. My last message was: > >>> so you mean something like this? >>> >>> if (err == -EPROBE_DEFER) >>> dev_info(dev, "deferring probe\n") >>> else >>> dev_err(dev, "... failed to register\n") >> >> Yes exactly. > > Patches welcome :D Not sure how useful the dev_info(dev, "deferring probe\n") is nowadays given that the device driver core will show which devices are on the probe deferral list, maybe we can turn this into a dev_dbg() instead?
On Sat, Jul 11, 2020 at 11:09 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 7/11/2020 2:07 PM, Linus Walleij wrote: > > I never got an updated patch. My last message was: > > > >>> so you mean something like this? > >>> > >>> if (err == -EPROBE_DEFER) > >>> dev_info(dev, "deferring probe\n") > >>> else > >>> dev_err(dev, "... failed to register\n") > >> > >> Yes exactly. > > > > Patches welcome :D > > Not sure how useful the dev_info(dev, "deferring probe\n") is nowadays > given that the device driver core will show which devices are on the > probe deferral list, maybe we can turn this into a dev_dbg() instead? Oh right. Yeah that sounds right, then we can see that it's the GPIO core bailing and deferring it when we turn on debug. Yours, Linus Walleij
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);
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(-)