[PATCH v2] gpio: Get rid of gpio_hog_probe_all()
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.
For SPL/TPL, the DM_FLAG_PRE_RELOC also needs to be set if u-boot,dm-pre-reloc property is present for the gpio-hog.
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 ---
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 | 40 ++++++++++++++++---------------------- include/asm-generic/gpio.h | 8 -------- 5 files changed, 19 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..77a6bc1506 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,26 @@ 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); + + /* + * 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); } } } + 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 *
On 9/22/22 16:13, Quentin Schulz wrote:
[...]
@@ -1503,9 +1480,26 @@ 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);/** 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 { }; }; };
Hi Marek,
On 9/22/22 16:29, Marek Vasut wrote:
On 9/22/22 16:13, Quentin Schulz wrote:
[...]
@@ -1503,9 +1480,26 @@ 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);
+ /* + * 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 {
You need u-boot,dm-pre-reloc here otherwise the gpio controller device tree node content won't be part of the SPL Device Tree. e.g. I get:
gpio@ff260000 {
bios_disable_override { gpios = <0x0d 0x01>; output-high; line-name = "bios_disable_override"; gpio-hog; }; };
in the SPL if I don't have u-boot,dm-pre-reloc for gpio2 (gpio@ff260000).
Obviously no driver will be bound to gpio@ff260000.
If I have the property:
gpio@ff260000 { compatible = "rockchip,gpio-bank"; reg = <0x00 0xff260000 0x00 0x100>; interrupts = <0x00 0x05 0x04>; clocks = <0x02 0x15d>; gpio-controller; #gpio-cells = <0x02>; interrupt-controller; #interrupt-cells = <0x02>; phandle = <0x6c>;
bios_disable_override { gpios = <0x0d 0x01>; output-high; line-name = "bios_disable_override"; gpio-hog; }; };
But I'm not sure this is what you wanted to highlight? Can you rephrase please?
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 ...>;
I don't think that's a good idea for inheritance.
E.g. today we have px30-u-boot.dtsi with u-boot,dm-pre-reloc for many nodes. If I want to add more nodes to it, I'd have to override it and then I need to specify all that are in px30-u-boot.dtsi already. This is not good because then a change made to u-boot,dm-pre-reloc in px30-u-boot.dtsi wouldn't be propagated to my board unless I update the property in my "end" device-tree.
Also, the Device Tree is supposed to represent the hardware and I don't feel like specifying the devices to have available in TPL/SPL in Device Tree is correct. It is somewhat user-friendly though compared to a defconfig or constant in an include file. I don't have anything to suggest at the moment though :/
Cheers, Quentin
On 9/22/22 17:33, Quentin Schulz wrote:
Hi Marek,
Hi,
[...]
+ /* + * 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 {
You need u-boot,dm-pre-reloc here otherwise the gpio controller device tree node content won't be part of the SPL Device Tree. e.g. I get:
gpio@ff260000 {
bios_disable_override { gpios = <0x0d 0x01>; output-high; line-name = "bios_disable_override"; gpio-hog; }; };
in the SPL if I don't have u-boot,dm-pre-reloc for gpio2 (gpio@ff260000).
Obviously no driver will be bound to gpio@ff260000.
If I have the property:
gpio@ff260000 { compatible = "rockchip,gpio-bank"; reg = <0x00 0xff260000 0x00 0x100>; interrupts = <0x00 0x05 0x04>; clocks = <0x02 0x15d>; gpio-controller; #gpio-cells = <0x02>; interrupt-controller; #interrupt-cells = <0x02>; phandle = <0x6c>;
bios_disable_override { gpios = <0x0d 0x01>; output-high; line-name = "bios_disable_override"; gpio-hog; }; };
But I'm not sure this is what you wanted to highlight? Can you rephrase please?
Nevermind, I think the DM core patch solves the problem I thought you had in DT already.
[...]
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 ...>;
I don't think that's a good idea for inheritance.
E.g. today we have px30-u-boot.dtsi with u-boot,dm-pre-reloc for many nodes. If I want to add more nodes to it, I'd have to override it and then I need to specify all that are in px30-u-boot.dtsi already. This is not good because then a change made to u-boot,dm-pre-reloc in px30-u-boot.dtsi wouldn't be propagated to my board unless I update the property in my "end" device-tree.
That part could be solved by some sort of syntax sugar in DTC I think, some /append-node/ or so.
Also, the Device Tree is supposed to represent the hardware and I don't feel like specifying the devices to have available in TPL/SPL in Device Tree is correct. It is somewhat user-friendly though compared to a defconfig or constant in an include file. I don't have anything to suggest at the moment though :/
The /config {} node is a bit special in that aspect, although I am not entirely sure whether it is even part of the DT spec. /chosen I think is.
[...]
participants (3)
-
Marek Vasut -
Quentin Schulz -
Quentin Schulz