| Message ID | 20220919194502.106252-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | gpio: Get rid of gpio_hog_probe_all() | expand |
On 9/19/22 14:45, Marek Vasut wrote: > The gpio_hog_probe_all() functionality can be perfectly well replaced by > DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback > of each GPIO hog driver instance after .bind() and thus configure the > hogged GPIO accordingly. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Samuel Holland <samuel@sholland.org> > Cc: Simon Glass <sjg@chromium.org> > --- > common/board_r.c | 3 --- > common/spl/spl.c | 3 --- > doc/README.gpio | 6 ++---- > drivers/gpio/gpio-uclass.c | 25 ++----------------------- > include/asm-generic/gpio.h | 8 -------- > 5 files changed, 4 insertions(+), 41 deletions(-) Reviewed-by: Samuel Holland <samuel@sholland.org>
Hi, On 9/19/22 21:45, Marek Vasut wrote: > The gpio_hog_probe_all() functionality can be perfectly well replaced by > DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback > of each GPIO hog driver instance after .bind() and thus configure the > hogged GPIO accordingly. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Samuel Holland <samuel@sholland.org> > Cc: Simon Glass <sjg@chromium.org> > --- > common/board_r.c | 3 --- > common/spl/spl.c | 3 --- > doc/README.gpio | 6 ++---- > drivers/gpio/gpio-uclass.c | 25 ++----------------------- > include/asm-generic/gpio.h | 8 -------- > 5 files changed, 4 insertions(+), 41 deletions(-) > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Thanks Patrick
Hi Marek, On 9/19/22 21:45, Marek Vasut wrote: > The gpio_hog_probe_all() functionality can be perfectly well replaced by > DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback > of each GPIO hog driver instance after .bind() and thus configure the > hogged GPIO accordingly. > > Signed-off-by: Marek Vasut <marex@denx.de> This patch breaks the U-Boot proper loading fallback mechanism on Puma RK3399. https://lore.kernel.org/u-boot/20220915091432.789294-1-foss+uboot@0leil.net/ is the base I used, on top of commit d6a03711fd with your patch applied on top. I need the GPIO hogs to be probed before the SPL starts looking for storage media for U-Boot proper because the GPIO hog is necessary on the HW level for the eMMC and SPI-NOR flash to be usable. Basically, we have a switch on the board disabling eMMC and SPI-NOR so that we can boot from SD card, but we want to be able to load U-Boot proper from eMMC/SPI-NOR. The GPIO hog overrides this HW switch. Use case is to recover from a corrupted SPL without touching U-Boot proper. Cheers, Quentin
On 9/20/22 11:00, Quentin Schulz wrote: > Hi Marek, Hi, > On 9/19/22 21:45, Marek Vasut wrote: >> The gpio_hog_probe_all() functionality can be perfectly well replaced by >> DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback >> of each GPIO hog driver instance after .bind() and thus configure the >> hogged GPIO accordingly. >> >> Signed-off-by: Marek Vasut <marex@denx.de> > > This patch breaks the U-Boot proper loading fallback mechanism on Puma > RK3399. > > https://lore.kernel.org/u-boot/20220915091432.789294-1-foss+uboot@0leil.net/ is the base I used, on top of commit d6a03711fd with your patch applied on top. > > I need the GPIO hogs to be probed before the SPL starts looking for > storage media for U-Boot proper because the GPIO hog is necessary on the > HW level for the eMMC and SPI-NOR flash to be usable. That _should_ still work. Look at common/spl/spl.c board_init_r() calls spl_init() calls spl_common_init() -> dm_init_and_scan() . That should bind and probe the GPIO hogs for you . The old gpio_hog_probe_all() happened AFTER all this. Can you use e.g. dm_dump_all() call in or around spl_board_init() to check whether the hogs got probed in SPL ? > Basically, we have a switch on the board disabling eMMC and SPI-NOR so > that we can boot from SD card, but we want to be able to load U-Boot > proper from eMMC/SPI-NOR. The GPIO hog overrides this HW switch. Use > case is to recover from a corrupted SPL without touching U-Boot proper. I don't quite get this -- isn't board_boot_order() or spl_boot_device() used for the purpose of selecting boot media , including fallback ? [...]
Hi all, Just so there's a written summary on the ML rather than only on IRC. On 9/20/22 11:53, Marek Vasut wrote: > On 9/20/22 11:00, Quentin Schulz wrote: >> Hi Marek, > > Hi, > >> On 9/19/22 21:45, Marek Vasut wrote: >>> The gpio_hog_probe_all() functionality can be perfectly well replaced by >>> DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback >>> of each GPIO hog driver instance after .bind() and thus configure the >>> hogged GPIO accordingly. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >> >> This patch breaks the U-Boot proper loading fallback mechanism on Puma >> RK3399. >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220915091432.789294-2D1-2Dfoss-2Buboot-400leil.net_&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=aMC9n5Cgsl79NAwWBVdx4n9MNHT7Jcm1g4sKww821Kd8JuvW1wjvVHLLPU2XsXVe&s=YbHd2jLJ2foRj_fTpj_PNmI5mXcUi5wGpzC3oL_rcVs&e= is the base I used, on top of commit d6a03711fd with your patch applied on top. >> >> I need the GPIO hogs to be probed before the SPL starts looking for >> storage media for U-Boot proper because the GPIO hog is necessary on >> the HW level for the eMMC and SPI-NOR flash to be usable. > > That _should_ still work. > > Look at common/spl/spl.c board_init_r() calls spl_init() calls > spl_common_init() -> dm_init_and_scan() . That should bind and probe the > GPIO hogs for you . The old gpio_hog_probe_all() happened AFTER all this. > > Can you use e.g. dm_dump_all() call in or around spl_board_init() to > check whether the hogs got probed in SPL ? > After some debugging with Marek, we found out that DM_FLAG_PRE_RELOC flag is not set on the gpio hogs and thus, dm_probe_devices function in drivers/core/root.c will not call device_probe() function. diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 32df0448a7..37a8706746 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -1481,7 +1481,7 @@ static int gpio_post_bind(struct udevice *dev) if (ret) return ret; - dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND); + dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND | DM_FLAG_PRE_RELOC); } } } made it work but it is not good enough (the flag needs to be added only if the DT property is there for example). One way could be to add this flag in a post_bind callback of the gpio-hog driver. But I believe a better way would have the DM code actually handle this on its own instead of relying on subsystems/drivers to do it. Something like: diff --git a/drivers/core/device.c b/drivers/core/device.c index d9ce546c0c..103ec47b88 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -174,6 +174,10 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv, if (ret) goto fail_child_post_bind; } + + if (ofnode_pre_reloc(node)) + dev_or_flags(dev, DM_FLAG_PRE_RELOC); + if (uc->uc_drv->post_bind) { ret = uc->uc_drv->post_bind(dev); if (ret) but with the knowledge of pre_reloc_only flag I guess? I have no experience in the DM code so maybe someone will have a better idea/quick implementation, so that this patch can actually be merged without breaking stuff :) >> Basically, we have a switch on the board disabling eMMC and SPI-NOR so >> that we can boot from SD card, but we want to be able to load U-Boot >> proper from eMMC/SPI-NOR. The GPIO hog overrides this HW switch. Use >> case is to recover from a corrupted SPL without touching U-Boot proper. > > I don't quite get this -- isn't board_boot_order() or spl_boot_device() > used for the purpose of selecting boot media , including fallback ? > > [...] In short, we have a switch on the devkit which allows to cut the power and clock lines to the eMMC and put the SPI-NOR flash in HOLD mode (reset basically). We then have a GPIO we can control with SW that allows to electrically override this switch on the devkit so that at runtime we can enable eMMC and SPI-NOR flash. The boot selection is handled by U-Boot (Rockchip mach directory deals with spl_boot_order), but the devices aren't physically usable until this GPIO is used to electrically override the switch on the devkit. The use case being: I manually (and electrically) disable eMMC and SPI flash with the switch to boot from SD Card, then if U-Boot proper on the SD card is corrupted or not present, fallback to loading it from eMMC or SPI flash. Cheers, Quentin
On 9/20/22 14:26, Quentin Schulz wrote: > Hi all, Hi, > Just so there's a written summary on the ML rather than only on IRC. Thanks [...] > Something like: > > diff --git a/drivers/core/device.c b/drivers/core/device.c > index d9ce546c0c..103ec47b88 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -174,6 +174,10 @@ static int device_bind_common(struct udevice > *parent, const struct driver *drv, > if (ret) > goto fail_child_post_bind; > } > + > + if (ofnode_pre_reloc(node)) > + dev_or_flags(dev, DM_FLAG_PRE_RELOC); > + > if (uc->uc_drv->post_bind) { > ret = uc->uc_drv->post_bind(dev); > if (ret) > > but with the knowledge of pre_reloc_only flag I guess? I have no > experience in the DM code so maybe someone will have a better idea/quick > implementation, so that this patch can actually be merged without > breaking stuff :) Can you send the above as an actual proper patch and CC Simon and me please ? I think this is actually even a bugfix for DM core. The rest is clear, thanks .
Hi Marek, On 9/19/22 21:45, Marek Vasut wrote: > The gpio_hog_probe_all() functionality can be perfectly well replaced by > DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback > of each GPIO hog driver instance after .bind() and thus configure the > hogged GPIO accordingly. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Samuel Holland <samuel@sholland.org> > Cc: Simon Glass <sjg@chromium.org> > --- > common/board_r.c | 3 --- > common/spl/spl.c | 3 --- > doc/README.gpio | 6 ++---- > drivers/gpio/gpio-uclass.c | 25 ++----------------------- > include/asm-generic/gpio.h | 8 -------- > 5 files changed, 4 insertions(+), 41 deletions(-) > > diff --git a/common/board_r.c b/common/board_r.c > index 56eb60fa275..c556aa5a073 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -750,9 +750,6 @@ static init_fnc_t init_sequence_r[] = { > initr_status_led, > #endif > /* PPC has a udelay(20) here dating from 2002. Why? */ > -#if defined(CONFIG_GPIO_HOG) > - gpio_hog_probe_all, > -#endif > #ifdef CONFIG_BOARD_LATE_INIT > board_late_init, > #endif > diff --git a/common/spl/spl.c b/common/spl/spl.c > index 29e0898f03d..683e0dfc526 100644 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -770,9 +770,6 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > } > } > > - if (CONFIG_IS_ENABLED(GPIO_HOG)) > - gpio_hog_probe_all(); > - > #if CONFIG_IS_ENABLED(BOARD_INIT) > spl_board_init(); > #endif > diff --git a/doc/README.gpio b/doc/README.gpio > index 548ff37b8cc..d253f654fad 100644 > --- a/doc/README.gpio > +++ b/doc/README.gpio > @@ -2,10 +2,8 @@ > GPIO hog (CONFIG_GPIO_HOG) > -------- > > -All the GPIO hog are initialized in gpio_hog_probe_all() function called in > -board_r.c just before board_late_init() but you can also acces directly to > -the gpio with gpio_hog_lookup_name(). > - > +All the GPIO hog are initialized using DM_FLAG_PROBE_AFTER_BIND DM flag > +after bind(). > > Example, for the device tree: > > diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c > index 0ed32b72170..32df0448a7b 100644 > --- a/drivers/gpio/gpio-uclass.c > +++ b/drivers/gpio/gpio-uclass.c > @@ -315,34 +315,11 @@ static int gpio_hog_probe(struct udevice *dev) > return 0; > } > > -int gpio_hog_probe_all(void) > -{ > - struct udevice *dev; > - int ret; > - int retval = 0; > - > - for (uclass_first_device(UCLASS_NOP, &dev); > - dev; > - uclass_find_next_device(&dev)) { > - if (dev->driver == DM_DRIVER_GET(gpio_hog)) { > - ret = device_probe(dev); > - if (ret) { > - printf("Failed to probe device %s err: %d\n", > - dev->name, ret); > - retval = ret; > - } > - } > - } > - > - return retval; > -} > - > int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc) > { > struct udevice *dev; > > *desc = NULL; > - gpio_hog_probe_all(); > if (!uclass_get_device_by_name(UCLASS_NOP, name, &dev)) { > struct gpio_hog_priv *priv = dev_get_priv(dev); > > @@ -1503,6 +1480,8 @@ static int gpio_post_bind(struct udevice *dev) > &child); > if (ret) > return ret; > + > + dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND); I replaced this line with: + /* + * Make sure gpio-hogs are probed after bind + * since hogs can be essential to the hardware + * system. + */ + dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND); + + /* + * Since gpio-hog is a U_BOOT_DRIVER and not + * a U_BOOT_CLASS, the DM core does not bind + * it and therefore it's up to this driver to + * set the DM_FLAG_PRE_RELOC appropriately. + */ + if (ofnode_pre_reloc(node)) + dev_or_flags(child, DM_FLAG_PRE_RELOC); and it works fine for me now (no dependency on the core DM patch I sent just a few minutes ago; I assume it's because we revert-recursively probe the parents of such devices?) Cheers, Quentin
On 9/22/22 15:59, Quentin Schulz wrote: > Hi Marek, Hi, [...] >> diff --git a/doc/README.gpio b/doc/README.gpio >> index 548ff37b8cc..d253f654fad 100644 >> --- a/doc/README.gpio >> +++ b/doc/README.gpio >> @@ -2,10 +2,8 @@ >> GPIO hog (CONFIG_GPIO_HOG) >> -------- >> -All the GPIO hog are initialized in gpio_hog_probe_all() function >> called in >> -board_r.c just before board_late_init() but you can also acces >> directly to >> -the gpio with gpio_hog_lookup_name(). >> - >> +All the GPIO hog are initialized using DM_FLAG_PROBE_AFTER_BIND DM flag >> +after bind(). >> Example, for the device tree: >> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c >> index 0ed32b72170..32df0448a7b 100644 >> --- a/drivers/gpio/gpio-uclass.c >> +++ b/drivers/gpio/gpio-uclass.c >> @@ -315,34 +315,11 @@ static int gpio_hog_probe(struct udevice *dev) >> return 0; >> } >> -int gpio_hog_probe_all(void) >> -{ >> - struct udevice *dev; >> - int ret; >> - int retval = 0; >> - >> - for (uclass_first_device(UCLASS_NOP, &dev); >> - dev; >> - uclass_find_next_device(&dev)) { >> - if (dev->driver == DM_DRIVER_GET(gpio_hog)) { >> - ret = device_probe(dev); >> - if (ret) { >> - printf("Failed to probe device %s err: %d\n", >> - dev->name, ret); >> - retval = ret; >> - } >> - } >> - } >> - >> - return retval; >> -} >> - >> int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc) >> { >> struct udevice *dev; >> *desc = NULL; >> - gpio_hog_probe_all(); >> if (!uclass_get_device_by_name(UCLASS_NOP, name, &dev)) { >> struct gpio_hog_priv *priv = dev_get_priv(dev); >> @@ -1503,6 +1480,8 @@ static int gpio_post_bind(struct udevice *dev) >> &child); >> if (ret) >> return ret; >> + >> + dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND); > > I replaced this line with: > > + /* > + * Make sure gpio-hogs are probed after > bind > + * since hogs can be essential to the > hardware > + * system. > + */ > + dev_or_flags(child, > DM_FLAG_PROBE_AFTER_BIND); > + > + /* > + * Since gpio-hog is a U_BOOT_DRIVER and > not > + * a U_BOOT_CLASS, the DM core does not > bind > + * it and therefore it's up to this > driver to > + * set the DM_FLAG_PRE_RELOC appropriately. > + */ > + if (ofnode_pre_reloc(node)) > + dev_or_flags(child, > DM_FLAG_PRE_RELOC); This second part should be handled by the DM, or you need dm-pre-reloc in your GPIO controller in DT. This would fail e.g. in case your GPIO controller has higher depth of hog subnodes, like: gpio-controller { something { gpio-hog { u-boot,dm-pre-reloc; }; }; }; Should really be: gpio-controller { u-boot,dm-pre-reloc; something { u-boot,dm-pre-reloc; gpio-hog { u-boot,dm-pre-reloc; }; }; }; At some point, I had the idea to instead of littering the DT with u-boot,dm-pre-reloc , we could use phandles instead and do something like: / { config { u-boot,dm-pre-reloc = <&node1 &node2 ... &gpio_hog ...>; }; } ... gpio-controller { something { gpio_hog: gpio-hog { }; }; };
diff --git a/common/board_r.c b/common/board_r.c index 56eb60fa275..c556aa5a073 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -750,9 +750,6 @@ static init_fnc_t init_sequence_r[] = { initr_status_led, #endif /* PPC has a udelay(20) here dating from 2002. Why? */ -#if defined(CONFIG_GPIO_HOG) - gpio_hog_probe_all, -#endif #ifdef CONFIG_BOARD_LATE_INIT board_late_init, #endif diff --git a/common/spl/spl.c b/common/spl/spl.c index 29e0898f03d..683e0dfc526 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -770,9 +770,6 @@ void board_init_r(gd_t *dummy1, ulong dummy2) } } - if (CONFIG_IS_ENABLED(GPIO_HOG)) - gpio_hog_probe_all(); - #if CONFIG_IS_ENABLED(BOARD_INIT) spl_board_init(); #endif diff --git a/doc/README.gpio b/doc/README.gpio index 548ff37b8cc..d253f654fad 100644 --- a/doc/README.gpio +++ b/doc/README.gpio @@ -2,10 +2,8 @@ GPIO hog (CONFIG_GPIO_HOG) -------- -All the GPIO hog are initialized in gpio_hog_probe_all() function called in -board_r.c just before board_late_init() but you can also acces directly to -the gpio with gpio_hog_lookup_name(). - +All the GPIO hog are initialized using DM_FLAG_PROBE_AFTER_BIND DM flag +after bind(). Example, for the device tree: diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 0ed32b72170..32df0448a7b 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -315,34 +315,11 @@ static int gpio_hog_probe(struct udevice *dev) return 0; } -int gpio_hog_probe_all(void) -{ - struct udevice *dev; - int ret; - int retval = 0; - - for (uclass_first_device(UCLASS_NOP, &dev); - dev; - uclass_find_next_device(&dev)) { - if (dev->driver == DM_DRIVER_GET(gpio_hog)) { - ret = device_probe(dev); - if (ret) { - printf("Failed to probe device %s err: %d\n", - dev->name, ret); - retval = ret; - } - } - } - - return retval; -} - int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc) { struct udevice *dev; *desc = NULL; - gpio_hog_probe_all(); if (!uclass_get_device_by_name(UCLASS_NOP, name, &dev)) { struct gpio_hog_priv *priv = dev_get_priv(dev); @@ -1503,6 +1480,8 @@ static int gpio_post_bind(struct udevice *dev) &child); if (ret) return ret; + + dev_or_flags(child, DM_FLAG_PROBE_AFTER_BIND); } } } diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 81f63f06f15..e56d3777ae5 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -460,14 +460,6 @@ int dm_gpio_lookup_name(const char *name, struct gpio_desc *desc); */ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc); -/** - * gpio_hog_probe_all() - probe all gpio devices with - * gpio-hog subnodes. - * - * @return: Returns return value from device_probe() - */ -int gpio_hog_probe_all(void); - /** * gpio_lookup_name - Look up a GPIO name and return its details *
The gpio_hog_probe_all() functionality can be perfectly well replaced by DM_FLAG_PROBE_AFTER_BIND DM flag, which would trigger .probe() callback of each GPIO hog driver instance after .bind() and thus configure the hogged GPIO accordingly. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> Cc: Samuel Holland <samuel@sholland.org> Cc: Simon Glass <sjg@chromium.org> --- common/board_r.c | 3 --- common/spl/spl.c | 3 --- doc/README.gpio | 6 ++---- drivers/gpio/gpio-uclass.c | 25 ++----------------------- include/asm-generic/gpio.h | 8 -------- 5 files changed, 4 insertions(+), 41 deletions(-)