On Wed, Feb 21, 2024 at 12:51 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote: > > Hi Saravana, > > On Tue, 20 Feb 2024 18:40:40 -0800 > Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > > On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote: > > > > > > Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT > > > overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE > > > is set on each overlay nodes. This flag is cleared when a struct device > > > is actually created for the DT node. > > > Also, when a device is created, the device DT node is parsed for known > > > phandle and devlinks consumer/supplier links are created between the > > > device (consumer) and the devices referenced by phandles (suppliers). > > > As these supplier device can have a struct device not already created, > > > the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the > > > devlink supplier point to the device's parent instead of the device > > > itself. > > > > > > Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just > > > before the devlink creation if a device is supposed to be created and > > > handled later in the process. > > > > > > Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays") > > > Cc: <stable@xxxxxxxxxxxxxxx> > > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx> > > > --- > > > drivers/of/property.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index 641a40cf5cf3..ff5cac477dbe 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np, > > > struct device_node *sup_np) > > > { > > > struct device_node *tmp_np = of_node_get(sup_np); > > > + struct fwnode_handle *sup_fwnode; > > > > > > /* Check that sup_np and its ancestors are available. */ > > > while (tmp_np) { > > > @@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np, > > > tmp_np = of_get_next_parent(tmp_np); > > > } > > > > > > - fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np)); > > > + /* > > > + * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE > > > + * flag set. A node can have a phandle that references an other node > > > + * added by the overlay. > > > + * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links > > > + * to this supplier instead of linking to its parent. > > > + */ > > > + sup_fwnode = of_fwnode_handle(sup_np); > > > + if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) { > > > + if (of_property_present(sup_np, "compatible") && > > > + of_device_is_available(sup_np)) > > > + sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE; > > > + } > > > + fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode); > > > > Nack. > > > > of_link_to_phandle() doesn't care about any of the fwnode flags. It > > just creates links between the consumer and supplier nodes. Don't add > > more intelligence into it please. Also, "compatible" doesn't really > > guarantee device creation and you can have devices created out of > > nodes with no compatible property. I finally managed to get away from > > looking for the "compatible" property. So, let's not add back a > > dependency on that property please. > > > > Can you please give a real example where you are hitting this? I have > > some thoughts on solutions, but I want to understand the issue fully > > before I make suggestions. > > > > I detected the issue with this overlay: > --- 8< --- > &{/} > { > reg_dock_sys_3v3: regulator-dock-sys-3v3 { > compatible = "regulator-fixed"; > regulator-name = "DOCK_SYS_3V3"; > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > gpios = <&tca6424_dock_1 5 GPIO_ACTIVE_HIGH>; // DOCK_SYS3V3_EN > enable-active-high; > regulator-always-on; > }; > }; > > &i2c5 { > tca6424_dock_1: gpio@22 { > compatible = "ti,tca6424"; > reg = <0x22>; > gpio-controller; > #gpio-cells = <2>; > interrupt-parent = <&gpio4>; > interrupts = <1 IRQ_TYPE_EDGE_FALLING>; > interrupt-controller; > #interrupt-cells = <2>; > vcc-supply = <®_dock_ctrl_3v3>; > }; > }; > --- 8< --- > > The regulator uses a gpio. > The supplier for the regulator was not the gpio chip (gpio@22) but the i2c bus. Thanks for the example. Let me think about this a bit on how we could fix this and get back to you. Please do ping me if I don't get back in a week or two. -Saravana > > I first tried to clear always the flag in of_link_to_phandle() without any check > to a "compatible" string and in that case, I broke pinctrl. > > All devices were waiting for the pinctrl they used (child of pinctrl device > node) even if the pinctrl driver was bound to the device. > > For pinctrl, the DT structure looks like the following: > --- 8< --- > { > ... > pinctrl@1234 { > reg = <1234>; > compatible = "vendor,chip"; > > pinctrl_some_device: grp { > fsl,pins = < ... >; > }; > }; > > some_device@4567 { > compablile = "foo,bar"; > reg = <4567>; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_some_device>; > ... > }; > }; > --- 8< --- > > In that case the link related to pinctrl for some_device needs to be to the > 'pinctrl_some_device' node parent (i.e. the pinctrl@1234 node). > > > Best regards, > Hervé