[PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[Michael Pratt]

Device linking to an OF node specified by the "nvmem-cells" property
as a supplier device causes probing to hangup on the consumer device.
A previous discussion is linked below.

NVMEM framework does not create standalone devices (i.e. no probing,
not in the hardware layer). Specifically, nvmem devices
are dependent on other subsystem devices and are created when
another driver (mtd, thunderbolt, etc.) calls nvmem_register() or
devm_nvmem_register() during struct device creation in that driver.

The "compatible" property nvmem-cells serves as a trigger
for parsers in other drivers, not for the nvmem driver itself.
An example for MTD devices is commit 658c4448bbbf
("mtd: core: add nvmem-cells compatible to parse mtd as nvmem cells").

This commit introduces function of_get_next_compat_node_parent()
in order to select the next parent node with a "compatible" property,
and the condition sup_not_dev in order to trigger the function,
so that an ancestor node is selected as the supplier device node instead.

Link: https://lore.kernel.org/linux-devicetree/696cb2da-20b9-b3dd-46d9-de4bf91a1506@gmail.com/
Fixes: 53e6a671f70a ("of: property: Add device link support for multiple DT bindings")
Signed-off-by: Michael Pratt <mcpratt@pm.me>
---
 drivers/of/property.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..4b7ee036f3e8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1104,6 +1104,18 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np)
 	return node;
 }
 
+static struct device_node *of_get_next_compat_node_parent(struct device_node *np)
+{
+	struct device_node *compat, *node;
+
+	compat = of_get_compat_node(np);
+	node = of_get_compat_node_parent(compat);
+	of_node_put(compat);
+	of_node_put(np);
+
+	return node;
+}
+
 /**
  * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
  * @con_np: consumer device tree node
@@ -1281,6 +1293,9 @@ static struct device_node *parse_##fname(struct device_node *np,	     \
  * @node_not_dev: The consumer node containing the property is never converted
  *		  to a struct device. Instead, parse ancestor nodes for the
  *		  compatible property to find a node corresponding to a device.
+ * @sup_not_dev: The supplier node and nearest parent node with a "compatible"
+ *		 property are not a struct device. Instead, parse ancestor nodes for the
+ *		 next "compatible" property to find a node corresponding to the device.
  *
  * Returns:
  * parse_prop() return values are
@@ -1293,6 +1308,7 @@ struct supplier_bindings {
 					  const char *prop_name, int index);
 	bool optional;
 	bool node_not_dev;
+	bool sup_not_dev;
 };
 
 DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
@@ -1393,7 +1409,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_power_domains, },
 	{ .parse_prop = parse_hwlocks, },
 	{ .parse_prop = parse_extcon, },
-	{ .parse_prop = parse_nvmem_cells, },
+	{ .parse_prop = parse_nvmem_cells, .sup_not_dev = true, },
 	{ .parse_prop = parse_phys, },
 	{ .parse_prop = parse_wakeup_parent, },
 	{ .parse_prop = parse_pinctrl0, },
@@ -1457,6 +1473,10 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
 			con_dev_np = s->node_not_dev
 					? of_get_compat_node_parent(con_np)
 					: of_node_get(con_np);
+
+			if (s->sup_not_dev)
+				phandle = of_get_next_compat_node_parent(phandle);
+
 			matched = true;
 			i++;
 			of_link_to_phandle(con_dev_np, phandle);
-- 
2.30.2

Re: [PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[Saravana Kannan]

On Thu, Oct 27, 2022 at 12:20 PM Michael Pratt <mcpratt@pm.me> wrote:
>
> Device linking to an OF node specified by the "nvmem-cells" property
> as a supplier device causes probing to hangup on the consumer device.
> A previous discussion is linked below.
>
> NVMEM framework does not create standalone devices (i.e. no probing,
> not in the hardware layer). Specifically, nvmem devices
> are dependent on other subsystem devices and are created when
> another driver (mtd, thunderbolt, etc.) calls nvmem_register() or
> devm_nvmem_register() during struct device creation in that driver.
>
> The "compatible" property nvmem-cells serves as a trigger
> for parsers in other drivers, not for the nvmem driver itself.
> An example for MTD devices is commit 658c4448bbbf
> ("mtd: core: add nvmem-cells compatible to parse mtd as nvmem cells").
>
> This commit introduces function of_get_next_compat_node_parent()
> in order to select the next parent node with a "compatible" property,
> and the condition sup_not_dev in order to trigger the function,
> so that an ancestor node is selected as the supplier device node instead.
>
> Link: https://lore.kernel.org/linux-devicetree/696cb2da-20b9-b3dd-46d9-de4bf91a1506@gmail.com/
> Fixes: 53e6a671f70a ("of: property: Add device link support for multiple DT bindings")
> Signed-off-by: Michael Pratt <mcpratt@pm.me>

Hi Michael,

Thanks a lot for submitting a patch to fix issues in fw_devlink. I
think my other patch series[1] should fix this in a generic way for
all such cases where the phandle doesn't actually point to the
supplier struct device. The series itself has some bugs, but there are
"try this if it fixes it" code snippets in the thread that I need to
roll into a v2.

Give it a shot if you can. I'll try to get back to the series soon.

[1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
-Saravana

> ---
>  drivers/of/property.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..4b7ee036f3e8 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1104,6 +1104,18 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np)
>         return node;
>  }
>
> +static struct device_node *of_get_next_compat_node_parent(struct device_node *np)
> +{
> +       struct device_node *compat, *node;
> +
> +       compat = of_get_compat_node(np);
> +       node = of_get_compat_node_parent(compat);
> +       of_node_put(compat);
> +       of_node_put(np);
> +
> +       return node;
> +}
> +
>  /**
>   * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
>   * @con_np: consumer device tree node
> @@ -1281,6 +1293,9 @@ static struct device_node *parse_##fname(struct device_node *np,       \
>   * @node_not_dev: The consumer node containing the property is never converted
>   *               to a struct device. Instead, parse ancestor nodes for the
>   *               compatible property to find a node corresponding to a device.
> + * @sup_not_dev: The supplier node and nearest parent node with a "compatible"
> + *              property are not a struct device. Instead, parse ancestor nodes for the
> + *              next "compatible" property to find a node corresponding to the device.
>   *
>   * Returns:
>   * parse_prop() return values are
> @@ -1293,6 +1308,7 @@ struct supplier_bindings {
>                                           const char *prop_name, int index);
>         bool optional;
>         bool node_not_dev;
> +       bool sup_not_dev;
>  };
>
>  DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
> @@ -1393,7 +1409,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_power_domains, },
>         { .parse_prop = parse_hwlocks, },
>         { .parse_prop = parse_extcon, },
> -       { .parse_prop = parse_nvmem_cells, },
> +       { .parse_prop = parse_nvmem_cells, .sup_not_dev = true, },
>         { .parse_prop = parse_phys, },
>         { .parse_prop = parse_wakeup_parent, },
>         { .parse_prop = parse_pinctrl0, },
> @@ -1457,6 +1473,10 @@ static int of_link_property(struct device_node *con_np, const char *prop_name)
>                         con_dev_np = s->node_not_dev
>                                         ? of_get_compat_node_parent(con_np)
>                                         : of_node_get(con_np);
> +
> +                       if (s->sup_not_dev)
> +                               phandle = of_get_next_compat_node_parent(phandle);
> +
>                         matched = true;
>                         i++;
>                         of_link_to_phandle(con_dev_np, phandle);
> --
> 2.30.2
>
>

Re: [PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[mcpratt]

> 
> Hi Michael,
> 
> Thanks a lot for submitting a patch to fix issues in fw_devlink. I
> think my other patch series[1] should fix this in a generic way for
> all such cases where the phandle doesn't actually point to the
> supplier struct device. The series itself has some bugs, but there are
> "try this if it fixes it" code snippets in the thread that I need to
> roll into a v2.
> 
> Give it a shot if you can. I'll try to get back to the series soon.
> 
> [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
> -Saravana

Hi Saravana,

It's definitely good to hear that someone is working on it already :D

It looks like the "dangling consumers" function would probably fix
the issue in Openwrt with fw_devlink. However, I noticed that in your series
the function of_get_compat_node_parent() is still there. I'm not sure whether
or not that could be simplified as well, since that is how I got the idea
for this patch. I understand your goal is to remove the dependency on
the "compatible" properties in total (at least for supplier devices).

I'll try the series and let you know how it goes (unless your V2 is coming soon).

FYI the device I test this on is Engenius EPG600 (MT7620A + QCA8337)

-- 
Thanks,
MCP

Re: [PATCH] of: property: fw_devlink: Fix support for nvmem-cells

[Saravana Kannan]

On Thu, Oct 27, 2022 at 2:43 PM <mcpratt@pm.me> wrote:
>
>
> >
> > Hi Michael,
> >
> > Thanks a lot for submitting a patch to fix issues in fw_devlink. I
> > think my other patch series[1] should fix this in a generic way for
> > all such cases where the phandle doesn't actually point to the
> > supplier struct device. The series itself has some bugs, but there are
> > "try this if it fixes it" code snippets in the thread that I need to
> > roll into a v2.
> >
> > Give it a shot if you can. I'll try to get back to the series soon.
> >
> > [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
> > -Saravana
>
> Hi Saravana,
>
> It's definitely good to hear that someone is working on it already :D
>
> It looks like the "dangling consumers" function would probably fix
> the issue in Openwrt with fw_devlink. However, I noticed that in your series
> the function of_get_compat_node_parent() is still there.

Yeah, it's there mainly for the weird/annoying "remote-endpoint" case.
Annoying because it's of zero use to fw_devlink since it always causes
cyclic dependency, but I also can't ignore it because the cycle could
be part of other cycles (and I can't ignore those cycles).

> I'm not sure whether
> or not that could be simplified as well,

I don't think I can -- too long to explain and I'm not feeling it
right now. But it has to do with how devices are populated and being
able to use that for lazily converting dependencies in DT to device
links.

> since that is how I got the idea
> for this patch. I understand your goal is to remove the dependency on
> the "compatible" properties in total (at least for supplier devices).
>
> I'll try the series and let you know how it goes (unless your V2 is coming soon).

It's most likely not "coming soon". Kinda busy with some stuff that I
can't postpone.

-Saravana

>
> FYI the device I test this on is Engenius EPG600 (MT7620A + QCA8337)
>
> --
> Thanks,
> MCP