| Message ID | 20161227172003.6517-2-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> 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 <tony@atomide.com> Patch applied. I felt the code path needed some extra comments so I sent those as a separate patch. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tony, On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> 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 <tony@atomide.com> 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 e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211 +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register +sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring... sh-pfc e6050000.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 -- geert@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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Geert Uytterhoeven <geert@linux-m68k.org> [170110 06:09]: > Hi Tony, > > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> 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 <tony@atomide.com> > > 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 e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211 > +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register > +sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring... > sh-pfc e6050000.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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/core.c b/drivers/pinctrl/core.c --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -720,7 +720,8 @@ static struct pinctrl_state *create_state(struct pinctrl *p, return state; } -static int add_setting(struct pinctrl *p, struct pinctrl_map const *map) +static int add_setting(struct pinctrl *p, struct pinctrl_dev *pctldev, + struct pinctrl_map const *map) { struct pinctrl_state *state; struct pinctrl_setting *setting; @@ -744,7 +745,11 @@ static int add_setting(struct pinctrl *p, struct pinctrl_map const *map) setting->type = map->type; - setting->pctldev = get_pinctrl_dev_from_devname(map->ctrl_dev_name); + if (pctldev) + setting->pctldev = pctldev; + else + setting->pctldev = + get_pinctrl_dev_from_devname(map->ctrl_dev_name); if (setting->pctldev == NULL) { kfree(setting); /* Do not defer probing of hogs (circular loop) */ @@ -800,7 +805,8 @@ static struct pinctrl *find_pinctrl(struct device *dev) static void pinctrl_free(struct pinctrl *p, bool inlist); -static struct pinctrl *create_pinctrl(struct device *dev) +static struct pinctrl *create_pinctrl(struct device *dev, + struct pinctrl_dev *pctldev) { struct pinctrl *p; const char *devname; @@ -823,7 +829,7 @@ static struct pinctrl *create_pinctrl(struct device *dev) INIT_LIST_HEAD(&p->states); INIT_LIST_HEAD(&p->dt_maps); - ret = pinctrl_dt_to_map(p); + ret = pinctrl_dt_to_map(p, pctldev); if (ret < 0) { kfree(p); return ERR_PTR(ret); @@ -838,7 +844,7 @@ static struct pinctrl *create_pinctrl(struct device *dev) if (strcmp(map->dev_name, devname)) continue; - ret = add_setting(p, map); + ret = add_setting(p, pctldev, map); /* * At this point the adding of a setting may: * @@ -899,7 +905,7 @@ struct pinctrl *pinctrl_get(struct device *dev) return p; } - return create_pinctrl(dev); + return create_pinctrl(dev, NULL); } EXPORT_SYMBOL_GPL(pinctrl_get); @@ -1738,6 +1744,46 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev) } /** + * pinctrl_late_init() - finish pin controller device registration + * @work: work struct + */ +static void pinctrl_late_init(struct work_struct *work) +{ + struct pinctrl_dev *pctldev; + + pctldev = container_of(work, struct pinctrl_dev, late_init.work); + + pctldev->p = create_pinctrl(pctldev->dev, pctldev); + if (!IS_ERR(pctldev->p)) { + kref_get(&pctldev->p->users); + pctldev->hog_default = + pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT); + if (IS_ERR(pctldev->hog_default)) { + dev_dbg(pctldev->dev, + "failed to lookup the default state\n"); + } else { + if (pinctrl_select_state(pctldev->p, + pctldev->hog_default)) + dev_err(pctldev->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(pctldev->dev, + "failed to lookup the sleep state\n"); + } + + mutex_lock(&pinctrldev_list_mutex); + list_add_tail(&pctldev->node, &pinctrldev_list); + mutex_unlock(&pinctrldev_list_mutex); + + pinctrl_init_device_debugfs(pctldev); +} + +/** * pinctrl_register() - register a pin controller device * @pctldesc: descriptor for this pin controller * @dev: parent device for this pin controller @@ -1766,6 +1812,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc, pctldev->driver_data = driver_data; INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL); INIT_LIST_HEAD(&pctldev->gpio_ranges); + INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init); pctldev->dev = dev; mutex_init(&pctldev->mutex); @@ -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)) + schedule_delayed_work(&pctldev->late_init, 0); + else + pinctrl_late_init(&pctldev->late_init.work); return pctldev; @@ -1848,6 +1873,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev) if (pctldev == NULL) return; + cancel_delayed_work_sync(&pctldev->late_init); mutex_lock(&pctldev->mutex); pinctrl_remove_device_debugfs(pctldev); mutex_unlock(&pctldev->mutex); diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -33,6 +33,7 @@ struct pinctrl_gpio_range; * @p: result of pinctrl_get() for this device * @hog_default: default state for pins hogged by this device * @hog_sleep: sleep state for pins hogged by this device + * @late_init: delayed work for pin controller to finish registration * @mutex: mutex taken on each pin controller specific action * @device_root: debugfs root for this device */ @@ -47,6 +48,7 @@ struct pinctrl_dev { struct pinctrl *p; struct pinctrl_state *hog_default; struct pinctrl_state *hog_sleep; + struct delayed_work late_init; struct mutex mutex; #ifdef CONFIG_DEBUG_FS struct dentry *device_root; diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c --- a/drivers/pinctrl/devicetree.c +++ b/drivers/pinctrl/devicetree.c @@ -100,11 +100,12 @@ struct pinctrl_dev *of_pinctrl_get(struct device_node *np) return get_pinctrl_dev_from_of_node(np); } -static int dt_to_map_one_config(struct pinctrl *p, const char *statename, +static int dt_to_map_one_config(struct pinctrl *p, + struct pinctrl_dev *pctldev, + const char *statename, struct device_node *np_config) { struct device_node *np_pctldev; - struct pinctrl_dev *pctldev; const struct pinctrl_ops *ops; int ret; struct pinctrl_map *map; @@ -121,7 +122,8 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename, /* OK let's just assume this will appear later then */ return -EPROBE_DEFER; } - pctldev = get_pinctrl_dev_from_of_node(np_pctldev); + if (!pctldev) + pctldev = get_pinctrl_dev_from_of_node(np_pctldev); if (pctldev) break; /* Do not defer probing of hogs (circular loop) */ @@ -166,7 +168,22 @@ static int dt_remember_dummy_state(struct pinctrl *p, const char *statename) return dt_remember_or_free_map(p, statename, NULL, map, 1); } -int pinctrl_dt_to_map(struct pinctrl *p) +bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev) +{ + struct device_node *np; + struct property *prop; + int size; + + np = pctldev->dev->of_node; + if (!np) + return false; + + prop = of_find_property(np, "pinctrl-0", &size); + + return prop ? true : false; +} + +int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev) { struct device_node *np = p->dev->of_node; int state, ret; @@ -233,7 +250,8 @@ int pinctrl_dt_to_map(struct pinctrl *p) } /* Parse the node */ - ret = dt_to_map_one_config(p, statename, np_config); + ret = dt_to_map_one_config(p, pctldev, statename, + np_config); of_node_put(np_config); if (ret < 0) goto err; diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h --- a/drivers/pinctrl/devicetree.h +++ b/drivers/pinctrl/devicetree.h @@ -20,8 +20,10 @@ struct of_phandle_args; #ifdef CONFIG_OF +bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev); + void pinctrl_dt_free_maps(struct pinctrl *p); -int pinctrl_dt_to_map(struct pinctrl *p); +int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev); int pinctrl_count_index_with_args(const struct device_node *np, const char *list_name); @@ -32,7 +34,13 @@ int pinctrl_parse_index_with_args(const struct device_node *np, #else -static inline int pinctrl_dt_to_map(struct pinctrl *p) +static inline bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev) +{ + return false; +} + +static inline int pinctrl_dt_to_map(struct pinctrl *p, + struct pinctrl_dev *pctldev) { return 0; }
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 <tony@atomide.com> --- drivers/pinctrl/core.c | 90 ++++++++++++++++++++++++++++---------------- drivers/pinctrl/core.h | 2 + drivers/pinctrl/devicetree.c | 28 +++++++++++--- drivers/pinctrl/devicetree.h | 12 +++++- 4 files changed, 93 insertions(+), 39 deletions(-)