| Message ID | 20220401103604.8705-12-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | gpiolib: Two new helpers and way toward fwnode | expand |
Hi On 01.04.2022 12:36, Andy Shevchenko wrote: > Since we have generic function to count GPIO controller nodes > under a given device, there is no need to open code it. Replace > custom code by gpiochip_node_count() call. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> This patch landed in linux next-20220413 as commit 88834c75cae5 ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). Unfortunately it breaks booting of all my Amlogic-based test boards (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and boards are unable to mount rootfs. Reverting this patch on top of linux-next fixes the issue. > --- > drivers/pinctrl/meson/pinctrl-meson.c | 28 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c > index 5b46a0979db7..1b078da81523 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.c > +++ b/drivers/pinctrl/meson/pinctrl-meson.c > @@ -49,6 +49,7 @@ > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinmux.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/regmap.h> > #include <linux/seq_file.h> > > @@ -662,27 +663,22 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc, > return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config); > } > > -static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, > - struct device_node *node) > +static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc) > { > - struct device_node *np, *gpio_np = NULL; > + struct device_node *gpio_np; > + unsigned int chips; > > - for_each_child_of_node(node, np) { > - if (!of_find_property(np, "gpio-controller", NULL)) > - continue; > - if (gpio_np) { > - dev_err(pc->dev, "multiple gpio nodes\n"); > - of_node_put(np); > - return -EINVAL; > - } > - gpio_np = np; > - } > - > - if (!gpio_np) { > + chips = gpiochip_node_count(pc->dev); > + if (!chips) { > dev_err(pc->dev, "no gpio node found\n"); > return -EINVAL; > } > + if (chips > 1) { > + dev_err(pc->dev, "multiple gpio nodes\n"); > + return -EINVAL; > + } > > + gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller")); > pc->of_node = gpio_np; > > pc->reg_mux = meson_map_resource(pc, gpio_np, "mux"); > @@ -751,7 +747,7 @@ int meson_pinctrl_probe(struct platform_device *pdev) > pc->dev = dev; > pc->data = (struct meson_pinctrl_data *) of_device_get_match_data(dev); > > - ret = meson_pinctrl_parse_dt(pc, dev->of_node); > + ret = meson_pinctrl_parse_dt(pc); > if (ret) > return ret; > Best regards
On Thu, Apr 14, 2022 at 12:44 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > On 01.04.2022 12:36, Andy Shevchenko wrote: > > Since we have generic function to count GPIO controller nodes > > under a given device, there is no need to open code it. Replace > > custom code by gpiochip_node_count() call. ... > This patch landed in linux next-20220413 as commit 88834c75cae5 > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). > Unfortunately it breaks booting of all my Amlogic-based test boards > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and > boards are unable to mount rootfs. Reverting this patch on top of > linux-next fixes the issue. Thank you for letting me know, I'll withdraw it and investigate.
Hi Andy, On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: [...] > > This patch landed in linux next-20220413 as commit 88834c75cae5 > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). > > Unfortunately it breaks booting of all my Amlogic-based test boards > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and > > boards are unable to mount rootfs. Reverting this patch on top of > > linux-next fixes the issue. > > Thank you for letting me know, I'll withdraw it and investigate. If needed I can investigate further later today/tomorrow. I think the problem is that our node name doesn't follow the .dts recommendation. For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO controller nodes are for example: gpio: bank@4b0 { ... } and gpio_ao: bank@14 { ... } See also: $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi Marek did not state which error he's getting but I suspect it fails with "no gpio node found". Best regards, Martin
On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > [...] > > > This patch landed in linux next-20220413 as commit 88834c75cae5 > > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). > > > Unfortunately it breaks booting of all my Amlogic-based test boards > > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and > > > boards are unable to mount rootfs. Reverting this patch on top of > > > linux-next fixes the issue. > > > > Thank you for letting me know, I'll withdraw it and investigate. > If needed I can investigate further later today/tomorrow. I think the > problem is that our node name doesn't follow the .dts recommendation. > > For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO > controller nodes are for example: > gpio: bank@4b0 { > ... > } > and > gpio_ao: bank@14 { > ... > } > > See also: > $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi > > Marek did not state which error he's getting but I suspect it fails > with "no gpio node found". Would be interesting to know that, yeah. The subtle difference between the patched and unpatched version is that the former uses only available nodes, it means that node is not available by some reason and then the error would be the one you guessed.
On Thu, Apr 14, 2022 at 07:06:21PM +0300, Andy Shevchenko wrote: > On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > [...] > > > > This patch landed in linux next-20220413 as commit 88834c75cae5 > > > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). > > > > Unfortunately it breaks booting of all my Amlogic-based test boards > > > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and > > > > boards are unable to mount rootfs. Reverting this patch on top of > > > > linux-next fixes the issue. > > > > > > Thank you for letting me know, I'll withdraw it and investigate. > > If needed I can investigate further later today/tomorrow. I think the > > problem is that our node name doesn't follow the .dts recommendation. > > > > For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO > > controller nodes are for example: > > gpio: bank@4b0 { > > ... > > } > > and > > gpio_ao: bank@14 { > > ... > > } > > > > See also: > > $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi > > > > Marek did not state which error he's getting but I suspect it fails > > with "no gpio node found". > > Would be interesting to know that, yeah. > > The subtle difference between the patched and unpatched version is > that the former uses only available nodes, it means that node is not > available by some reason and then the error would be the one you > guessed. Looking into the difference between iterating via available nodes I have found nothing suspicious. Your DTSes do not have status property, so it assumes the node is available. I'm quite puzzled what's going on there. Because I can't see what the logical difference the patch brought in. P.S. In any case it's withdrawn now and shouldn't appear in the next Linux Next. But I would really appreciate more input on this.
On Thu, Apr 14, 2022 at 09:28:30PM +0300, Andy Shevchenko wrote: > On Thu, Apr 14, 2022 at 07:06:21PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 14, 2022 at 6:32 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > > On Thu, Apr 14, 2022 at 3:51 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > [...] > > > > > This patch landed in linux next-20220413 as commit 88834c75cae5 > > > > > ("pinctrl: meson: Replace custom code by gpiochip_node_count() call"). > > > > > Unfortunately it breaks booting of all my Amlogic-based test boards > > > > > (Odroid C4, N2, Khadas VIM3, VIM3l). MMC driver is no longer probed and > > > > > boards are unable to mount rootfs. Reverting this patch on top of > > > > > linux-next fixes the issue. > > > > > > > > Thank you for letting me know, I'll withdraw it and investigate. > > > If needed I can investigate further later today/tomorrow. I think the > > > problem is that our node name doesn't follow the .dts recommendation. > > > > > > For GXL (arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) the GPIO > > > controller nodes are for example: > > > gpio: bank@4b0 { > > > ... > > > } > > > and > > > gpio_ao: bank@14 { > > > ... > > > } > > > > > > See also: > > > $ git grep -C6 gpio-controller arch/arm64/boot/dts/amlogic/*.dtsi > > > > > > Marek did not state which error he's getting but I suspect it fails > > > with "no gpio node found". > > > > Would be interesting to know that, yeah. > > > > The subtle difference between the patched and unpatched version is > > that the former uses only available nodes, it means that node is not > > available by some reason and then the error would be the one you > > guessed. > > Looking into the difference between iterating via available nodes I have found > nothing suspicious. Your DTSes do not have status property, so it assumes the > node is available. > > I'm quite puzzled what's going on there. Because I can't see what the logical > difference the patch brought in. > > P.S. In any case it's withdrawn now and shouldn't appear in the next Linux > Next. But I would really appreciate more input on this. Okay, now I got it. The "name" of the node is not the same as containing the property with a given name. So, the faulting line of the code is this one: gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller"));
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c index 5b46a0979db7..1b078da81523 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.c +++ b/drivers/pinctrl/meson/pinctrl-meson.c @@ -49,6 +49,7 @@ #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/regmap.h> #include <linux/seq_file.h> @@ -662,27 +663,22 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc, return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config); } -static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, - struct device_node *node) +static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc) { - struct device_node *np, *gpio_np = NULL; + struct device_node *gpio_np; + unsigned int chips; - for_each_child_of_node(node, np) { - if (!of_find_property(np, "gpio-controller", NULL)) - continue; - if (gpio_np) { - dev_err(pc->dev, "multiple gpio nodes\n"); - of_node_put(np); - return -EINVAL; - } - gpio_np = np; - } - - if (!gpio_np) { + chips = gpiochip_node_count(pc->dev); + if (!chips) { dev_err(pc->dev, "no gpio node found\n"); return -EINVAL; } + if (chips > 1) { + dev_err(pc->dev, "multiple gpio nodes\n"); + return -EINVAL; + } + gpio_np = to_of_node(device_get_named_child_node(pc->dev, "gpio-controller")); pc->of_node = gpio_np; pc->reg_mux = meson_map_resource(pc, gpio_np, "mux"); @@ -751,7 +747,7 @@ int meson_pinctrl_probe(struct platform_device *pdev) pc->dev = dev; pc->data = (struct meson_pinctrl_data *) of_device_get_match_data(dev); - ret = meson_pinctrl_parse_dt(pc, dev->of_node); + ret = meson_pinctrl_parse_dt(pc); if (ret) return ret;