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é
