[PATCH v3 1/2] dm: fix probing of all devices that have u-boot, dm-pre-reloc in SPL/TPL

From: Quentin Schulz quentin.schulz@theobroma-systems.com
Currently, dm_probe_devices checks that the flags of the device contains DM_FLAG_PRE_RELOC. However DM_FLAG_PRE_RELOC is a driver - and not a device - flag. This means that the check in pre_reloc_only mode would always fail.
Instead, what was aimed to be checked is that either the driver of the device has the flag set, or that the device has the u-boot,dm-pre-reloc Device Tree property set.
So let's fix the check to allow u-boot,dm-pre-reloc devices to be probed.
Cc: Quentin Schulz foss+uboot@0leil.net Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com ---
v3: - instead of adding the DM_FLAG_PRE_RELOC driver flag to the device, check ofnode_pre_reloc(node) in dm_probe_devices before attempting to probe, This is better than v1 in lists_bind_fdt because the latter function does not work on non UCLASS drivers (e.g. gpio-hog) and also because it fixes the cause and does not create a work-around,
v1: - https://lore.kernel.org/u-boot/20220922134540.3268586-1-foss+uboot@0leil.net...
drivers/core/root.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/core/root.c b/drivers/core/root.c index f24ddfa521..c4fb48548b 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -363,20 +363,22 @@ void *dm_priv_to_rw(void *priv)
static int dm_probe_devices(struct udevice *dev, bool pre_reloc_only) { - u32 mask = DM_FLAG_PROBE_AFTER_BIND; - u32 flags = dev_get_flags(dev); + ofnode node = dev_ofnode(dev); struct udevice *child; int ret;
- if (pre_reloc_only) - mask |= DM_FLAG_PRE_RELOC; + if (pre_reloc_only && + (!ofnode_valid(node) || !ofnode_pre_reloc(node)) && + !(dev->driver->flags & DM_FLAG_PRE_RELOC)) + goto probe_children;
- if ((flags & mask) == mask) { + if (dev_get_flags(dev) & DM_FLAG_PROBE_AFTER_BIND) { ret = device_probe(dev); if (ret) return ret; }
+probe_children: list_for_each_entry(child, &dev->child_head, sibling_node) dm_probe_devices(child, pre_reloc_only);

From: Marek Vasut marex@denx.de
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 Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Samuel Holland samuel@sholland.org ---
v3: - removed DM_FLAG_PRE_RELOC flag since it is now handled by patch 1/2 in this series, in the DM core,
v2: - added missing DM_FLAG_PRE_RELOC flag on the gpio-hog device, - added comments for flags setting, - tested on a PX30 ringneck where the gpio-hog is necessary in SPL to be able to load U-Boot proper from eMMC when booting SPL from SD card,
common/board_r.c | 3 --- common/spl/spl.c | 3 --- doc/README.gpio | 6 ++---- drivers/gpio/gpio-uclass.c | 31 ++++++++----------------------- include/asm-generic/gpio.h | 8 -------- 5 files changed, 10 insertions(+), 41 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 56eb60fa27..c556aa5a07 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 29e0898f03..683e0dfc52 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 548ff37b8c..d253f654fa 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 0ed32b7217..b08e482ab3 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,9 +1480,17 @@ static int gpio_post_bind(struct udevice *dev) &child); if (ret) return ret; + + /* + * 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); } } } + return 0; }
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 81f63f06f1..e56d3777ae 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 *

Hi Tom
This patch is landing in the mailing list since a while. Do you expect to merge it in master or in next branch soon ? This patch will be useful for STM32MP SoC in order to clean according machine code.
Thanks Patrice
On 9/22/22 17:53, Quentin Schulz wrote:
From: Marek Vasut marex@denx.de
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 Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Samuel Holland samuel@sholland.org
v3:
- removed DM_FLAG_PRE_RELOC flag since it is now handled by patch 1/2
in this series, in the DM core,
v2:
- added missing DM_FLAG_PRE_RELOC flag on the gpio-hog device,
- added comments for flags setting,
- tested on a PX30 ringneck where the gpio-hog is necessary in SPL to
be able to load U-Boot proper from eMMC when booting SPL from SD card,
common/board_r.c | 3 --- common/spl/spl.c | 3 --- doc/README.gpio | 6 ++---- drivers/gpio/gpio-uclass.c | 31 ++++++++----------------------- include/asm-generic/gpio.h | 8 -------- 5 files changed, 10 insertions(+), 41 deletions(-)
diff --git a/common/board_r.c b/common/board_r.c index 56eb60fa27..c556aa5a07 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 29e0898f03..683e0dfc52 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 548ff37b8c..d253f654fa 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 0ed32b7217..b08e482ab3 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,9 +1480,17 @@ static int gpio_post_bind(struct udevice *dev) &child); if (ret) return ret;
/*
* 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); }
- return 0;
}
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 81f63f06f1..e56d3777ae 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

Hi Patrice,
On 1/3/23 14:35, Patrice CHOTARD wrote:
Hi Tom
This patch is landing in the mailing list since a while. Do you expect to merge it in master or in next branch soon ? This patch will be useful for STM32MP SoC in order to clean according machine code.
The main issue here is that Simon would like a test case for the first patch in the series and I wasn't able to add tests to the sandbox to guarantee there's no regression in the future, hence why it was kinda parked on the ML without much work on it.
From my U-Boot IRC archive: 2022-09-26 15:22:58 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0msjg1: it seems I'm unable to write a unit test for this dm patch 2022-09-26 15:24:33 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0msjg1: https://paste.ack.tf/dc8598 is what I came up with, but impossible to actually run this test 2022-09-26 15:25:23 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0mway too many changes to be able to even compile this with sandbox_spl_defconfig 2022-09-26 15:25:47 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0mand with sandbox_defconfig, ./u-boot -T -c "ut dm fdt" does not run it either 2022-09-26 15:27:18 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0mso i'm not against some guidance on how to write a unit test for that patch 2022-09-26 15:28:52 ^[[94m^[[0m^[[1m^[[38;5;7m^[[49mqschulz ^[[0mwhat we want to test is that in pre-reloc, a driver with DM_FLAG_PROBE_AFTER_BIND flag but not DM_FLAG_PRE_RELOC gets probed if it is bound to a device tree node where u-boot,dm-pre-reloc is set
Cheers, Quentin

On 1/3/23 14:50, Quentin Schulz wrote:
Hi Patrice,
On 1/3/23 14:35, Patrice CHOTARD wrote:
Hi Tom
This patch is landing in the mailing list since a while. Do you expect to merge it in master or in next branch soon ? This patch will be useful for STM32MP SoC in order to clean according machine code.
The main issue here is that Simon would like a test case for the first patch in the series and I wasn't able to add tests to the sandbox to guarantee there's no regression in the future, hence why it was kinda parked on the ML without much work on it.
Hmmm, seems there is little input on how to write the test or even what to test, so maybe this should be just merged, esp. if it is useful.

On Tue, Jan 03, 2023 at 04:28:13PM +0100, Marek Vasut wrote:
On 1/3/23 14:50, Quentin Schulz wrote:
Hi Patrice,
On 1/3/23 14:35, Patrice CHOTARD wrote:
Hi Tom
This patch is landing in the mailing list since a while. Do you expect to merge it in master or in next branch soon ? This patch will be useful for STM32MP SoC in order to clean according machine code.
The main issue here is that Simon would like a test case for the first patch in the series and I wasn't able to add tests to the sandbox to guarantee there's no regression in the future, hence why it was kinda parked on the ML without much work on it.
Hmmm, seems there is little input on how to write the test or even what to test, so maybe this should be just merged, esp. if it is useful.
What's missing from https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html for this case?

On Tue, Jan 03, 2023 at 02:35:00PM +0100, Patrice CHOTARD wrote:
Hi Tom
This patch is landing in the mailing list since a while. Do you expect to merge it in master or in next branch soon ? This patch will be useful for STM32MP SoC in order to clean according machine code.
Thanks for following up. In addition to what Quentin said, there were a few related solutions to a problem and it wasn't quite clear to me if this was also still needed. I'll keep an eye out for v4.

On Thu, Sep 22, 2022 at 05:53:26PM +0200, Quentin Schulz wrote:
From: Marek Vasut marex@denx.de
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 Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com Reviewed-by: Patrick Delaunay patrick.delaunay@foss.st.com Reviewed-by: Samuel Holland samuel@sholland.org
With the expectation that a test will be written later, applied to u-boot/master, thanks!

Hi Quentin,
On Thu, 22 Sept 2022 at 09:53, Quentin Schulz foss+uboot@0leil.net wrote:
From: Quentin Schulz quentin.schulz@theobroma-systems.com
Currently, dm_probe_devices checks that the flags of the device contains DM_FLAG_PRE_RELOC. However DM_FLAG_PRE_RELOC is a driver - and not a device - flag. This means that the check in pre_reloc_only mode would always fail.
Please can you shorten the subject?
Instead, what was aimed to be checked is that either the driver of the device has the flag set, or that the device has the u-boot,dm-pre-reloc Device Tree property set.
So let's fix the check to allow u-boot,dm-pre-reloc devices to be probed.
Cc: Quentin Schulz foss+uboot@0leil.net Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com
v3:
- instead of adding the DM_FLAG_PRE_RELOC driver flag to the device,
check ofnode_pre_reloc(node) in dm_probe_devices before attempting to probe, This is better than v1 in lists_bind_fdt because the latter function does not work on non UCLASS drivers (e.g. gpio-hog) and also because it fixes the cause and does not create a work-around,
v1:
drivers/core/root.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/core/root.c b/drivers/core/root.c index f24ddfa521..c4fb48548b 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -363,20 +363,22 @@ void *dm_priv_to_rw(void *priv)
static int dm_probe_devices(struct udevice *dev, bool pre_reloc_only) {
u32 mask = DM_FLAG_PROBE_AFTER_BIND;
u32 flags = dev_get_flags(dev);
ofnode node = dev_ofnode(dev); struct udevice *child; int ret;
if (pre_reloc_only)
mask |= DM_FLAG_PRE_RELOC;
if (pre_reloc_only &&
(!ofnode_valid(node) || !ofnode_pre_reloc(node)) &&
!(dev->driver->flags & DM_FLAG_PRE_RELOC))
goto probe_children;
if ((flags & mask) == mask) {
if (dev_get_flags(dev) & DM_FLAG_PROBE_AFTER_BIND) { ret = device_probe(dev); if (ret) return ret; }
+probe_children:
can you use some other way, to avoid the goto?
list_for_each_entry(child, &dev->child_head, sibling_node) dm_probe_devices(child, pre_reloc_only);
-- 2.37.3
If this fixes a bug, can we we have a test for it, please?
REgards, Simon

On Thu, Sep 22, 2022 at 05:53:25PM +0200, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@theobroma-systems.com
Currently, dm_probe_devices checks that the flags of the device contains DM_FLAG_PRE_RELOC. However DM_FLAG_PRE_RELOC is a driver - and not a device - flag. This means that the check in pre_reloc_only mode would always fail.
Instead, what was aimed to be checked is that either the driver of the device has the flag set, or that the device has the u-boot,dm-pre-reloc Device Tree property set.
So let's fix the check to allow u-boot,dm-pre-reloc devices to be probed.
Cc: Quentin Schulz foss+uboot@0leil.net Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com
Applied to u-boot/master, thanks!
participants (6)
-
Marek Vasut
-
Patrice CHOTARD
-
Quentin Schulz
-
Quentin Schulz
-
Simon Glass
-
Tom Rini