Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
* Tony Lindgren [170111 08:29]: > * Linus Walleij [170111 07:34]: > > On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren wrote: > > > > > Below is an experimental fix to intorduce pinctrl_start() that I've > > > tested with pinctrl-single. Then we should probably make all pin > > > controller > > > drivers call pinctrl_start() to properly fix the issue of struct > > > pinctrl_dev > > > handle not being initialized before driver functions are called. > > > > Hm I guess that could work, but can we keep pinctrl_register() with the old > > semantics and add a separate pinctrl_register_and_defer() > > for those who just wanna start it later by a separate call? > > > > Then we don't need any special flags. > > OK I'll take a look. > > > > Or do you guys have any better ideas? > > > > Not really. So you mean revert the previous patch and apply something > > like this instead? > > Let me first take a look to see if we can fix it by making drivers using > GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with > pinctrl_register_and_defer(). I'll post a patch for that today. Yeah we can fix this by reverting the late_init parts of the earlier attempt and introducing a new pinctrl_register_and_init() for controllers to use: extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc, struct device *dev, void *driver_data, struct pinctrl_dev **pctldev); > Then maybe for v4.12 we can attempt to move all pin controller drivers > to using it so we can fix the problem for good. And that will also make converting existing drivers to use it later on trivial. Will post a patch shortly after some more testing. Regards, Tony
Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
* Linus Walleij [170111 07:34]: > On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren wrote: > > > Below is an experimental fix to intorduce pinctrl_start() that I've > > tested with pinctrl-single. Then we should probably make all pin controller > > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev > > handle not being initialized before driver functions are called. > > Hm I guess that could work, but can we keep pinctrl_register() with the old > semantics and add a separate pinctrl_register_and_defer() > for those who just wanna start it later by a separate call? > > Then we don't need any special flags. OK I'll take a look. > > Or do you guys have any better ideas? > > Not really. So you mean revert the previous patch and apply something > like this instead? Let me first take a look to see if we can fix it by making drivers using GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with pinctrl_register_and_defer(). I'll post a patch for that today. Then maybe for v4.12 we can attempt to move all pin controller drivers to using it so we can fix the problem for good. Regards, Tony
Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren wrote: > Below is an experimental fix to intorduce pinctrl_start() that I've > tested with pinctrl-single. Then we should probably make all pin controller > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev > handle not being initialized before driver functions are called. Hm I guess that could work, but can we keep pinctrl_register() with the old semantics and add a separate pinctrl_register_and_defer() for those who just wanna start it later by a separate call? Then we don't need any special flags. > Or do you guys have any better ideas? Not really. So you mean revert the previous patch and apply something like this instead? Yours, Linus Walleij
Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
* Tony Lindgren [170110 07:32]:
> * Geert Uytterhoeven [170110 06:09]:
> > Hi Tony,
> >
> > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren wrote:
> > > Having the pin control framework call pin controller functions
> > > before it's probe has finished is not nice as the pin controller
> > > device driver does not yet have struct pinctrl_dev handle.
> > >
> > > Let's fix this issue by adding deferred work for late init. This is
> > > needed to be able to add pinctrl generic helper functions that expect
> > > to know struct pinctrl_dev handle. Note that we now need to call
> > > create_pinctrl() directly as we don't want to add the pin controller
> > > to the list of controllers until the hogs are claimed. We also need
> > > to pass the pinctrl_dev to the device tree parser functions as they
> > > otherwise won't find the right controller at this point.
> > >
> > > Signed-off-by: Tony Lindgren
> >
> > I believe this patch causes a regression on r8a7740/armadillo, where the
> > pin controller is also a GPIO controller, and lcd0 needs a hog
> > (cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
> >
> > -GPIO line 176 (lcd0) hogged as output/high
> > -sh-pfc e605.pfc: r8a7740_pfc handling gpio 0 -> 211
> > +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
> > +sh-pfc e605.pfc: failed to init GPIO chip, ignoring...
> > sh-pfc e605.pfc: r8a7740_pfc support registered
> >
> > Hence all drivers using GPIOs fail to initialize because their GPIOs never
> > become available.
> >
> > Adding debug prints to the failure paths shows that the call to
> > of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
> > Adding a debug print to the top of gpiochip_add_data() makes the problem go
> > away, presumably because it introduces a delay that allows the delayed work
> > to kick in...
>
> OK. What if we added also an optional pinctrl function that the pin
> controller driver could call to initialize hogs? Then the pin controller
> driver could call it during or after probe as needed. That is after
> there's a valid struct pinctrl_dev handle.
...
> We could also pass some flag if should always call pinctrl_late_init()
> directly. But that does not remove the problem of struct pinctrl_dev handle
> being uninitialized when the pin controller driver functionas are called.
Looks like we need both a flag and a way for the pin controller driver
to start things.
Below is an experimental fix to intorduce pinctrl_start() that I've
tested with pinctrl-single. Then we should probably make all pin controller
drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev
handle not being initialized before driver functions are called.
Or do you guys have any better ideas?
Regards,
Tony
8<
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1962,6 +1962,17 @@ static void pinctrl_late_init(struct work_struct *work)
pinctrl_init_device_debugfs(pctldev);
}
+int pinctrl_start(struct pinctrl_dev *pctldev)
+{
+ if (!IS_ERR(pctldev->p))
+ return -EEXIST;
+
+ pinctrl_late_init(&pctldev->late_init.work);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_start);
+
/**
* pinctrl_register() - register a pin controller device
* @pctldesc: descriptor for this pin controller
@@ -2035,9 +2046,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc
*pctldesc,
/*
* If the device has hogs we want the probe() function of the driver
* to complete before we go in and hog them and add the pin controller
-* to the list of controllers. If it has no hogs, we can just complete
-* the registration immediately.
+* to the list of controllers. If the pin controller driver initializes
+* hogs, or the pin controller instance has no hogs, we can just
+* complete the registration immediately.
*/
+
+ if (pctldesc->flags & PINCTRL_DRIVER_START)
+ return pctldev;
+
if (pinctrl_dt_has_hogs(pctldev))
schedule_delayed_work(&pctldev->late_init, 0);
else
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1741,6 +1741,7 @@ static int pcs_probe(struct platform_device *pdev)
pcs->desc.pmxops = &pcs_pinmux_ops;
if (PCS_HAS_PINCONF)
pcs->desc.confops = &pcs_pinconf_ops;
+ pcs->desc.flags = PINCTRL_DRIVER_START;
pcs->desc.owner = THIS_MODULE;
ret = pcs_allocate_pin_table(pcs);
@@ -1754,6 +1755,10 @@ static int pcs_probe(struct platform_device *pdev)
goto free;
}
+ ret = pinctrl_start(pcs->pctl);
+ if (ret)
+ goto free;
+
ret = pcs_add_gpio_func(np, pcs);
if (ret < 0)
goto free;
diff --
Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
* Geert Uytterhoeven [170110 06:09]:
> Hi Tony,
>
> On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren wrote:
> > Having the pin control framework call pin controller functions
> > before it's probe has finished is not nice as the pin controller
> > device driver does not yet have struct pinctrl_dev handle.
> >
> > Let's fix this issue by adding deferred work for late init. This is
> > needed to be able to add pinctrl generic helper functions that expect
> > to know struct pinctrl_dev handle. Note that we now need to call
> > create_pinctrl() directly as we don't want to add the pin controller
> > to the list of controllers until the hogs are claimed. We also need
> > to pass the pinctrl_dev to the device tree parser functions as they
> > otherwise won't find the right controller at this point.
> >
> > Signed-off-by: Tony Lindgren
>
> I believe this patch causes a regression on r8a7740/armadillo, where the
> pin controller is also a GPIO controller, and lcd0 needs a hog
> (cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
>
> -GPIO line 176 (lcd0) hogged as output/high
> -sh-pfc e605.pfc: r8a7740_pfc handling gpio 0 -> 211
> +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
> +sh-pfc e605.pfc: failed to init GPIO chip, ignoring...
> sh-pfc e605.pfc: r8a7740_pfc support registered
>
> Hence all drivers using GPIOs fail to initialize because their GPIOs never
> become available.
>
> Adding debug prints to the failure paths shows that the call to
> of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
> Adding a debug print to the top of gpiochip_add_data() makes the problem go
> away, presumably because it introduces a delay that allows the delayed work
> to kick in...
OK. What if we added also an optional pinctrl function that the pin
controller driver could call to initialize hogs? Then the pin controller
driver could call it during or after probe as needed. That is after
there's a valid struct pinctrl_dev handle.
> Jon's fix ("pinctrl: core: Fix panic when pinctrl devices with hogs are
> unregistered") doesn't help, as it affects unregistration only.
>
> > --- a/drivers/pinctrl/core.c
> > +++ b/drivers/pinctrl/core.c
>
> > @@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct
> > pinctrl_desc *pctldesc,
> > goto out_err;
> > }
> >
> > - mutex_lock(&pinctrldev_list_mutex);
> > - list_add_tail(&pctldev->node, &pinctrldev_list);
> > - mutex_unlock(&pinctrldev_list_mutex);
> > -
> > - pctldev->p = pinctrl_get(pctldev->dev);
> > -
> > - if (!IS_ERR(pctldev->p)) {
> > - pctldev->hog_default =
> > - pinctrl_lookup_state(pctldev->p,
> > PINCTRL_STATE_DEFAULT);
> > - if (IS_ERR(pctldev->hog_default)) {
> > - dev_dbg(dev, "failed to lookup the default
> > state\n");
> > - } else {
> > - if (pinctrl_select_state(pctldev->p,
> > - pctldev->hog_default))
> > - dev_err(dev,
> > - "failed to select default state\n");
> > - }
> > -
> > - pctldev->hog_sleep =
> > - pinctrl_lookup_state(pctldev->p,
> > - PINCTRL_STATE_SLEEP);
> > - if (IS_ERR(pctldev->hog_sleep))
> > - dev_dbg(dev, "failed to lookup the sleep state\n");
> > - }
> > -
> > - pinctrl_init_device_debugfs(pctldev);
> > + if (pinctrl_dt_has_hogs(pctldev))
>
> Changing the above line to "if (0 && pinctrl_dt_has_hogs(pctldev))"
> fixes the issue for me.
>
> > + schedule_delayed_work(&pctldev->late_init, 0);
> > + else
> > + pinctrl_late_init(&pctldev->late_init.work);
> >
> > return pctldev;
We could also pass some flag if should always call pinctrl_late_init()
directly. But that does not remove the problem of struct pinctrl_dev handle
being uninitialized when the pin controller driver functionas are called.
Regards,
Tony
Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs
Hi Tony,
On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren wrote:
> Having the pin control framework call pin controller functions
> before it's probe has finished is not nice as the pin controller
> device driver does not yet have struct pinctrl_dev handle.
>
> Let's fix this issue by adding deferred work for late init. This is
> needed to be able to add pinctrl generic helper functions that expect
> to know struct pinctrl_dev handle. Note that we now need to call
> create_pinctrl() directly as we don't want to add the pin controller
> to the list of controllers until the hogs are claimed. We also need
> to pass the pinctrl_dev to the device tree parser functions as they
> otherwise won't find the right controller at this point.
>
> Signed-off-by: Tony Lindgren
I believe this patch causes a regression on r8a7740/armadillo, where the
pin controller is also a GPIO controller, and lcd0 needs a hog
(cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
-GPIO line 176 (lcd0) hogged as output/high
-sh-pfc e605.pfc: r8a7740_pfc handling gpio 0 -> 211
+gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
+sh-pfc e605.pfc: failed to init GPIO chip, ignoring...
sh-pfc e605.pfc: r8a7740_pfc support registered
Hence all drivers using GPIOs fail to initialize because their GPIOs never
become available.
Adding debug prints to the failure paths shows that the call to
of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
Adding a debug print to the top of gpiochip_add_data() makes the problem go
away, presumably because it introduces a delay that allows the delayed work
to kick in...
Jon's fix ("pinctrl: core: Fix panic when pinctrl devices with hogs are
unregistered") doesn't help, as it affects unregistration only.
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct
> pinctrl_desc *pctldesc,
> goto out_err;
> }
>
> - mutex_lock(&pinctrldev_list_mutex);
> - list_add_tail(&pctldev->node, &pinctrldev_list);
> - mutex_unlock(&pinctrldev_list_mutex);
> -
> - pctldev->p = pinctrl_get(pctldev->dev);
> -
> - if (!IS_ERR(pctldev->p)) {
> - pctldev->hog_default =
> - pinctrl_lookup_state(pctldev->p,
> PINCTRL_STATE_DEFAULT);
> - if (IS_ERR(pctldev->hog_default)) {
> - dev_dbg(dev, "failed to lookup the default state\n");
> - } else {
> - if (pinctrl_select_state(pctldev->p,
> - pctldev->hog_default))
> - dev_err(dev,
> - "failed to select default state\n");
> - }
> -
> - pctldev->hog_sleep =
> - pinctrl_lookup_state(pctldev->p,
> - PINCTRL_STATE_SLEEP);
> - if (IS_ERR(pctldev->hog_sleep))
> - dev_dbg(dev, "failed to lookup the sleep state\n");
> - }
> -
> - pinctrl_init_device_debugfs(pctldev);
> + if (pinctrl_dt_has_hogs(pctldev))
Changing the above line to "if (0 && pinctrl_dt_has_hogs(pctldev))"
fixes the issue for me.
> + schedule_delayed_work(&pctldev->late_init, 0);
> + else
> + pinctrl_late_init(&pctldev->late_init.work);
>
> return pctldev;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
