[PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[Quentin Schulz]

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
  *
-- 
2.37.3

Re: [PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[Marek Vasut]

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 {
     };
   };
};

Re: [PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[Quentin Schulz]

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

Re: [PATCH v2] gpio: Get rid of gpio_hog_probe_all()

[Marek Vasut]

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.

[...]