| Message ID | 20210908215806.2748361-1-saravanak@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] RFC: of: property: fix phy-hanlde issue | expand |
Hi On 08.09.2021 23:58, Saravana Kannan wrote: > This is a test patch. I'll split it up into multiple commits and clean > it up once it's shown to help. > > Marek, can you please test this and let me know if it helps? I've just checked and nope, it doesn't help for my case. Ethernet is still not probed on Amlogic G12A/B SoC based boards. :( > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/of/property.c | 76 ++++++++++++++++++++++++------------------- > 1 file changed, 43 insertions(+), 33 deletions(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 0c0dc2e369c0..039e1cb07348 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -30,6 +30,35 @@ > > #include "of_private.h" > > +/** > + * struct supplier_bindings - Property parsing functions for suppliers > + * > + * @parse_prop: function name > + * parse_prop() finds the node corresponding to a supplier phandle > + * @parse_prop.np: Pointer to device node holding supplier phandle property > + * @parse_prop.prop_name: Name of property holding a phandle value > + * @parse_prop.index: For properties holding a list of phandles, this is the > + * index into the list > + * @optional: Describes whether a supplier is mandatory or not > + * @node_not_dev: The consumer node containing the property is never a device. > + * @sup_node_always_dev: The supplier node pointed to by the property will > + * always have a struct device created for it even if it > + * doesn't have a "compatible" property. > + * > + * Returns: > + * parse_prop() return values are > + * - phandle node pointer with refcount incremented. Caller must of_node_put() > + * on it when done. > + * - NULL if no phandle found at index > + */ > +struct supplier_bindings { > + struct device_node *(*parse_prop)(struct device_node *np, > + const char *prop_name, int index); > + bool optional; > + bool node_not_dev; > + bool sup_node_always_dev; > +}; > + > /** > * of_graph_is_present() - check graph's presence > * @node: pointer to device_node containing graph port > @@ -1079,6 +1108,7 @@ static struct device_node *of_get_compat_node(struct device_node *np) > * of_link_to_phandle - Add fwnode link to supplier from supplier phandle > * @con_np: consumer device tree node > * @sup_np: supplier device tree node > + * @s: The supplier binding used to get the supplier phandle > * > * Given a phandle to a supplier device tree node (@sup_np), this function > * finds the device that owns the supplier device tree node and creates a > @@ -1093,7 +1123,8 @@ static struct device_node *of_get_compat_node(struct device_node *np) > * - -ENODEV if struct device will never be create for supplier > */ > static int of_link_to_phandle(struct device_node *con_np, > - struct device_node *sup_np) > + struct device_node *sup_np, > + const struct supplier_bindings *s) > { > struct device *sup_dev; > struct device_node *tmp_np = sup_np; > @@ -1102,11 +1133,15 @@ static int of_link_to_phandle(struct device_node *con_np, > * Find the device node that contains the supplier phandle. It may be > * @sup_np or it may be an ancestor of @sup_np. > */ > - sup_np = of_get_compat_node(sup_np); > - if (!sup_np) { > - pr_debug("Not linking %pOFP to %pOFP - No device\n", > - con_np, tmp_np); > - return -ENODEV; > + if (s->sup_node_always_dev) { > + of_node_get(sup_np); > + } else { > + sup_np = of_get_compat_node(sup_np); > + if (!sup_np) { > + pr_debug("Not linking %pOFP to %pOFP - No device\n", > + con_np, tmp_np); > + return -ENODEV; > + } > } > > /* > @@ -1239,31 +1274,6 @@ static struct device_node *parse_##fname(struct device_node *np, \ > return parse_suffix_prop_cells(np, prop_name, index, suffix, cells); \ > } > > -/** > - * struct supplier_bindings - Property parsing functions for suppliers > - * > - * @parse_prop: function name > - * parse_prop() finds the node corresponding to a supplier phandle > - * @parse_prop.np: Pointer to device node holding supplier phandle property > - * @parse_prop.prop_name: Name of property holding a phandle value > - * @parse_prop.index: For properties holding a list of phandles, this is the > - * index into the list > - * @optional: Describes whether a supplier is mandatory or not > - * @node_not_dev: The consumer node containing the property is never a device. > - * > - * Returns: > - * parse_prop() return values are > - * - phandle node pointer with refcount incremented. Caller must of_node_put() > - * on it when done. > - * - NULL if no phandle found at index > - */ > -struct supplier_bindings { > - struct device_node *(*parse_prop)(struct device_node *np, > - const char *prop_name, int index); > - bool optional; > - bool node_not_dev; > -}; > - > DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") > DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells") > DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells") > @@ -1380,7 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_resets, }, > { .parse_prop = parse_leds, }, > { .parse_prop = parse_backlight, }, > - { .parse_prop = parse_phy_handle, }, > + { .parse_prop = parse_phy_handle, .sup_node_always_dev = true, }, > { .parse_prop = parse_gpio_compat, }, > { .parse_prop = parse_interrupts, }, > { .parse_prop = parse_regulators, }, > @@ -1430,7 +1440,7 @@ static int of_link_property(struct device_node *con_np, const char *prop_name) > : of_node_get(con_np); > matched = true; > i++; > - of_link_to_phandle(con_dev_np, phandle); > + of_link_to_phandle(con_dev_np, phandle, s); > of_node_put(phandle); > of_node_put(con_dev_np); > } Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
On Thu, Sep 9, 2021 at 1:03 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi > > On 08.09.2021 23:58, Saravana Kannan wrote: > > This is a test patch. I'll split it up into multiple commits and clean > > it up once it's shown to help. > > > > Marek, can you please test this and let me know if it helps? > I've just checked and nope, it doesn't help for my case. Ethernet is > still not probed on Amlogic G12A/B SoC based boards. :( Hi Marek, Thanks for testing out the patch. Turns out the issue was a lot more complicated than I thought. Thanks to a bunch of debug logs that Rob provided off-list, I was able to root cause the actual issue. Looks like the problem is cyclic dependency between the mdio-multiplexer and the ethernet: ethmac -(phy-handle)-> external_phy -(parent) -> mdio-multiplexer -(mdio-bus-parent)-> mdio0 -(parent)-> ethmac Relevant parts of the DT combined from multiple files and trimmed and pasted below. If fw_devlink sees a cycle, it'll stop enforcing ordering between all the devices in the cycle since it can't figure out which one of the dependencies isn't real. So, the confusing part was that, when Andrew Lunn gave the patch for adding support for "mdio-bus-parent", that should have allowed fw_devlink to see the cycle and stop enforcing the dependencies. But that didn't happen because of a bug in fw_devlink cycle handling (it worked for most cases, but not for this specific ordering in DT). I'll send out a fix for that soon. That combined with Andrew's "mdio-bus-parent" patch should fix things for you. But I think I'll revert the phy-handle patch for other reasons (I'll explain that in the patch that reverts it). Thanks, Saravana ethmac: ethernet@ff3f0000 { compatible = "amlogic,meson-g12a-dwmac" phy-handle = <&external_phy>; mdio0: mdio { compatible = "snps,dwmac-mdio"; } }; mdio-multiplexer { mdio-bus-parent = <&mdio0>; ext_mdio: mdio@0 { /* no compatible prop */ external_phy: ethernet-phy@0 { /* no compatible prop */ } } } -Saravana
On Mon, Sep 13, 2021 at 5:54 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 9, 2021 at 1:03 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > > > > Hi > > > > On 08.09.2021 23:58, Saravana Kannan wrote: > > > This is a test patch. I'll split it up into multiple commits and clean > > > it up once it's shown to help. > > > > > > Marek, can you please test this and let me know if it helps? > > I've just checked and nope, it doesn't help for my case. Ethernet is > > still not probed on Amlogic G12A/B SoC based boards. :( > > Hi Marek, > > Thanks for testing out the patch. Turns out the issue was a lot more > complicated than I thought. Thanks to a bunch of debug logs that Rob > provided off-list, I was able to root cause the actual issue. > > Looks like the problem is cyclic dependency between the mdio-multiplexer and the > ethernet: > ethmac -(phy-handle)-> external_phy -(parent) -> > mdio-multiplexer -(mdio-bus-parent)-> mdio0 -(parent)-> ethmac > > Relevant parts of the DT combined from multiple files and trimmed and > pasted below. > > If fw_devlink sees a cycle, it'll stop enforcing ordering between all > the devices in the cycle since it can't figure out which one of the > dependencies isn't real. So, the confusing part was that, when Andrew > Lunn gave the patch for adding support for "mdio-bus-parent", that > should have allowed fw_devlink to see the cycle and stop enforcing the > dependencies. But that didn't happen because of a bug in fw_devlink > cycle handling (it worked for most cases, but not for this specific > ordering in DT). I'll send out a fix for that soon. Here's the fix I promised: https://lore.kernel.org/lkml/20210914043928.4066136-2-saravanak@google.com/ > That combined with > Andrew's "mdio-bus-parent" patch should fix things for you. Fairly certain the fix above and Andrew's patch should fix it for you if you want to test it. Rob already verified a very similar patch for me. -Saravana > But I > think I'll revert the phy-handle patch for other reasons (I'll explain > that in the patch that reverts it). > > > Thanks, > Saravana > > ethmac: ethernet@ff3f0000 { > compatible = "amlogic,meson-g12a-dwmac" > > phy-handle = <&external_phy>; > mdio0: mdio { > compatible = "snps,dwmac-mdio"; > } > }; > > mdio-multiplexer { > mdio-bus-parent = <&mdio0>; > > ext_mdio: mdio@0 { > /* no compatible prop */ > external_phy: ethernet-phy@0 { > /* no compatible prop */ > } > } > } > > -Saravana
Hi Saravana, On 14.09.2021 06:44, Saravana Kannan wrote: > On Mon, Sep 13, 2021 at 5:54 PM Saravana Kannan <saravanak@google.com> wrote: >> On Thu, Sep 9, 2021 at 1:03 AM Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >>> On 08.09.2021 23:58, Saravana Kannan wrote: >>>> This is a test patch. I'll split it up into multiple commits and clean >>>> it up once it's shown to help. >>>> >>>> Marek, can you please test this and let me know if it helps? >>> I've just checked and nope, it doesn't help for my case. Ethernet is >>> still not probed on Amlogic G12A/B SoC based boards. :( >> Hi Marek, >> >> Thanks for testing out the patch. Turns out the issue was a lot more >> complicated than I thought. Thanks to a bunch of debug logs that Rob >> provided off-list, I was able to root cause the actual issue. >> >> Looks like the problem is cyclic dependency between the mdio-multiplexer and the >> ethernet: >> ethmac -(phy-handle)-> external_phy -(parent) -> >> mdio-multiplexer -(mdio-bus-parent)-> mdio0 -(parent)-> ethmac >> >> Relevant parts of the DT combined from multiple files and trimmed and >> pasted below. >> >> If fw_devlink sees a cycle, it'll stop enforcing ordering between all >> the devices in the cycle since it can't figure out which one of the >> dependencies isn't real. So, the confusing part was that, when Andrew >> Lunn gave the patch for adding support for "mdio-bus-parent", that >> should have allowed fw_devlink to see the cycle and stop enforcing the >> dependencies. But that didn't happen because of a bug in fw_devlink >> cycle handling (it worked for most cases, but not for this specific >> ordering in DT). I'll send out a fix for that soon. > Here's the fix I promised: > https://lore.kernel.org/lkml/20210914043928.4066136-2-saravanak@google.com/ > >> That combined with >> Andrew's "mdio-bus-parent" patch should fix things for you. > Fairly certain the fix above and Andrew's patch should fix it for you > if you want to test it. Rob already verified a very similar patch for me. Right. Those both patches finally fixed the ethernet issue. Feel free to add: Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
diff --git a/drivers/of/property.c b/drivers/of/property.c index 0c0dc2e369c0..039e1cb07348 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -30,6 +30,35 @@ #include "of_private.h" +/** + * struct supplier_bindings - Property parsing functions for suppliers + * + * @parse_prop: function name + * parse_prop() finds the node corresponding to a supplier phandle + * @parse_prop.np: Pointer to device node holding supplier phandle property + * @parse_prop.prop_name: Name of property holding a phandle value + * @parse_prop.index: For properties holding a list of phandles, this is the + * index into the list + * @optional: Describes whether a supplier is mandatory or not + * @node_not_dev: The consumer node containing the property is never a device. + * @sup_node_always_dev: The supplier node pointed to by the property will + * always have a struct device created for it even if it + * doesn't have a "compatible" property. + * + * Returns: + * parse_prop() return values are + * - phandle node pointer with refcount incremented. Caller must of_node_put() + * on it when done. + * - NULL if no phandle found at index + */ +struct supplier_bindings { + struct device_node *(*parse_prop)(struct device_node *np, + const char *prop_name, int index); + bool optional; + bool node_not_dev; + bool sup_node_always_dev; +}; + /** * of_graph_is_present() - check graph's presence * @node: pointer to device_node containing graph port @@ -1079,6 +1108,7 @@ static struct device_node *of_get_compat_node(struct device_node *np) * of_link_to_phandle - Add fwnode link to supplier from supplier phandle * @con_np: consumer device tree node * @sup_np: supplier device tree node + * @s: The supplier binding used to get the supplier phandle * * Given a phandle to a supplier device tree node (@sup_np), this function * finds the device that owns the supplier device tree node and creates a @@ -1093,7 +1123,8 @@ static struct device_node *of_get_compat_node(struct device_node *np) * - -ENODEV if struct device will never be create for supplier */ static int of_link_to_phandle(struct device_node *con_np, - struct device_node *sup_np) + struct device_node *sup_np, + const struct supplier_bindings *s) { struct device *sup_dev; struct device_node *tmp_np = sup_np; @@ -1102,11 +1133,15 @@ static int of_link_to_phandle(struct device_node *con_np, * Find the device node that contains the supplier phandle. It may be * @sup_np or it may be an ancestor of @sup_np. */ - sup_np = of_get_compat_node(sup_np); - if (!sup_np) { - pr_debug("Not linking %pOFP to %pOFP - No device\n", - con_np, tmp_np); - return -ENODEV; + if (s->sup_node_always_dev) { + of_node_get(sup_np); + } else { + sup_np = of_get_compat_node(sup_np); + if (!sup_np) { + pr_debug("Not linking %pOFP to %pOFP - No device\n", + con_np, tmp_np); + return -ENODEV; + } } /* @@ -1239,31 +1274,6 @@ static struct device_node *parse_##fname(struct device_node *np, \ return parse_suffix_prop_cells(np, prop_name, index, suffix, cells); \ } -/** - * struct supplier_bindings - Property parsing functions for suppliers - * - * @parse_prop: function name - * parse_prop() finds the node corresponding to a supplier phandle - * @parse_prop.np: Pointer to device node holding supplier phandle property - * @parse_prop.prop_name: Name of property holding a phandle value - * @parse_prop.index: For properties holding a list of phandles, this is the - * index into the list - * @optional: Describes whether a supplier is mandatory or not - * @node_not_dev: The consumer node containing the property is never a device. - * - * Returns: - * parse_prop() return values are - * - phandle node pointer with refcount incremented. Caller must of_node_put() - * on it when done. - * - NULL if no phandle found at index - */ -struct supplier_bindings { - struct device_node *(*parse_prop)(struct device_node *np, - const char *prop_name, int index); - bool optional; - bool node_not_dev; -}; - DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells") DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells") @@ -1380,7 +1390,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_resets, }, { .parse_prop = parse_leds, }, { .parse_prop = parse_backlight, }, - { .parse_prop = parse_phy_handle, }, + { .parse_prop = parse_phy_handle, .sup_node_always_dev = true, }, { .parse_prop = parse_gpio_compat, }, { .parse_prop = parse_interrupts, }, { .parse_prop = parse_regulators, }, @@ -1430,7 +1440,7 @@ static int of_link_property(struct device_node *con_np, const char *prop_name) : of_node_get(con_np); matched = true; i++; - of_link_to_phandle(con_dev_np, phandle); + of_link_to_phandle(con_dev_np, phandle, s); of_node_put(phandle); of_node_put(con_dev_np); }
This is a test patch. I'll split it up into multiple commits and clean it up once it's shown to help. Marek, can you please test this and let me know if it helps? Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 76 ++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 33 deletions(-)