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

22 Sep 2022


      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

    