Skip to contentSkip to conversation

of: property: fw_devlink: Add support for "phy-handle" property

PATCH Applied Saravana Kannan 5 participants 22 messages Aug 18, 2021 – Sep 14, 2021

Subsystems (4)
Networking Network drivers Arm/Amlogic Meson SoC support Device tree
lore
Share & tools
lore

Saravana Kannan

author
#0 lore raw

PATCH v2 1/1 of: property: fw_devlink: Add support for "phy-handle" property

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

---
v1 -> v2:
- Fixed patch to address my misunderstanding of how PHYs get initialized.

 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/of/property.c +2 -0
index 931340329414..0c0dc2e369c0 100644
@@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 

@@ -1379,6 +1380,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_gpio_compat, },
 	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },
Signature · 1 line
2.33.0.rc1.237.g0d66db33f3-goog
#2 lore raw
In reply to Saravana Kannan

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Quoted 21 lines

---
v1 -> v2:
- Fixed patch to address my misunderstanding of how PHYs get initialized.

  drivers/of/property.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/of/property.c +1 -0
index 931340329414..0c0dc2e369c0 100644
@@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
  DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")

drivers/of/property.c static const struct supplier_bindings of_supplier_bindings[] = { · line 1386

Quoted 7 lines
drivers/of/property.c +1 -0
@@ -1379,6 +1380,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_gpio_compat, },
  	{ .parse_prop = parse_interrupts, },
  	{ .parse_prop = parse_regulators, },

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
#3 lore raw
Quoted 17 lines · Marek Szyprowski

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Looks to me like we need to handle 'mdio-parent-bus' dependency.

Rob

#4 lore raw
In reply to Marek Szyprowski
Quoted 16 lines · Marek Szyprowski

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Hi Marek

Please try this. Completetly untested, not even compile teseted:

drivers/of/property.c +2 -0
index 0c0dc2e369c0..7c4e257c0a81 100644
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 

@@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
        { .parse_prop = parse_leds, },
        { .parse_prop = parse_backlight, },
        { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_mdio_parent_bus, },
        { .parse_prop = parse_gpio_compat, },
        { .parse_prop = parse_interrupts, },
        { .parse_prop = parse_regulators, },

Andrew

author
#5 lore raw
Quoted 43 lines · Andrew Lunn
Quoted 16 lines · Marek Szyprowski

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Hi Marek

Please try this. Completetly untested, not even compile teseted:

drivers/of/property.c +1 -0
index 0c0dc2e369c0..7c4e257c0a81 100644
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")

drivers/of/property.c static const struct supplier_bindings of_supplier_bindings[] = { · line 1388

Quoted 7 lines
drivers/of/property.c +1 -0
@@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
        { .parse_prop = parse_leds, },
        { .parse_prop = parse_backlight, },
        { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_mdio_parent_bus, },
        { .parse_prop = parse_gpio_compat, },
        { .parse_prop = parse_interrupts, },
        { .parse_prop = parse_regulators, },

Looking at the code, I'm fairly certain that the device that corresponds to a DT node pointed to by mdio-parent-bus will be a "bus" device that's registered with the mdio_bus_class.

If my understanding is right, then Nak for this patch. It'll break a lot of probes.

TL;DR is that stateful/managed device links don't make sense for devices that are never probed/bound to a driver. I plan to improve device links to try and accommodate these cases nicely, but that's in my TO DO list. Until that's completed, this patch will break stuff.

-Saravana

#6 lore raw
Quoted 56 lines · Saravana Kannan
Quoted 43 lines · Andrew Lunn
Quoted 16 lines · Marek Szyprowski

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Hi Marek

Please try this. Completetly untested, not even compile teseted:

drivers/of/property.c +1 -0
index 0c0dc2e369c0..7c4e257c0a81 100644
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")

drivers/of/property.c static const struct supplier_bindings of_supplier_bindings[] = { · line 1388

Quoted 7 lines
drivers/of/property.c +1 -0
@@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
        { .parse_prop = parse_leds, },
        { .parse_prop = parse_backlight, },
        { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_mdio_parent_bus, },
        { .parse_prop = parse_gpio_compat, },
        { .parse_prop = parse_interrupts, },
        { .parse_prop = parse_regulators, },

Looking at the code, I'm fairly certain that the device that corresponds to a DT node pointed to by mdio-parent-bus will be a "bus" device that's registered with the mdio_bus_class.

Quoted 5 lines

If my understanding is right, then Nak for this patch. It'll break a lot of probes.

TL;DR is that stateful/managed device links don't make sense for devices that are never probed/bound to a driver.

So some more background, which might help you get an idea what is going on here, and what you will need to implement.

There are a number of different ways an mdio bus driver can come into existence.

They can be classical devices, which are described in device tree and probed in the normal way. Most of the mdio bus drivers in driver/net/mdio are like this, and they have documented bindings, and compatible strings, e.g. Documentation/devicetree/bindings/net/marvell-orion-mdio.txt

Multiplexers, which are probably a subclass of the above classical devices. They have documented binds and compatible strings. They link to another MDIO bus, and some other resource to switch the multiplexor, e.g, GPIOs, a MMIO register, a Linux multiplexer.

They can be embedded inside some other device, typically an Ethernet controller, but also a Ethernet switch. In this case, the parent device should have an MDIO node in its device tree. An example would be the freescale FEC Documentation/devicetree/bindings/net/fsl,fec.yaml So if you are trying to fulfil dependencies for this sort of mdio bus, you need to probe the FEC driver, and as a side effect, the MDIO bus driver will pop into existence.

Andrew

#7 lore raw
In reply to Andrew Lunn

Hi Andrew,

Quoted 17 lines · Andrew Lunn
Quoted 16 lines · Marek Szyprowski
Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

Quoted 7 lines
Quoted 4 lines

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

Hi Marek

Please try this. Completetly untested, not even compile teseted:

Nope, this doesn't help in this case.

Quoted 21 lines
drivers/of/property.c +2 -0
index 0c0dc2e369c0..7c4e257c0a81 100644
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
  DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 

@@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
         { .parse_prop = parse_leds, },
         { .parse_prop = parse_backlight, },
         { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_mdio_parent_bus, },
         { .parse_prop = parse_gpio_compat, },
         { .parse_prop = parse_interrupts, },
         { .parse_prop = parse_regulators, },
Quoted 2 lines

Andrew

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author
#8 lore raw
In reply to Marek Szyprowski
Quoted 17 lines · Marek Szyprowski

Hi,

Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device doesn't have a compatible property. And so the device link dependency is propagated up to the mdio bus. But busses as suppliers aren't good because busses never "probe".

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it. You can confirm my theory by checking for the list of suppliers under ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look at the "status" file under the folder it should be "dormant". If you add a compatible property that fits the formats a PHY node can have, that should also fix your issue (not the solution though).

I'll send out a fix this week (once you confirm my analysis). Thanks for reporting it.

-Saravana

Quoted 35 lines
Quoted 22 lines

---
v1 -> v2:
- Fixed patch to address my misunderstanding of how PHYs get initialized.

  drivers/of/property.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/of/property.c +1 -0
index 931340329414..0c0dc2e369c0 100644
@@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
  DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")

drivers/of/property.c static const struct supplier_bindings of_supplier_bindings[] = { · line 1386

Quoted 7 lines
drivers/of/property.c +1 -0
@@ -1379,6 +1380,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_gpio_compat, },
      { .parse_prop = parse_interrupts, },
      { .parse_prop = parse_regulators, },

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

#9 lore raw
Quoted 2 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it.

Correct. They are like PCI or USB devices. You can ask it, what are you? There are two registers in standard locations which give you a vendor and product ID. We use that to find the correct driver.

You only need a compatible when things are not so simple.

1) The IDs are wrong. Some silicon vendors do stupid things

2) Chicken/egg problems, you cannot read the ID registers until you load the driver and some resource is enabled.

3) It is a C45 devices, e.g. part of clause 45 of 802.3, which requires a different protocol to be talked over the bus. So the compatible string tells you to talk C45 to get the IDs.

4) It is not a PHY, but some sort of other MDIO device, and hence there are no ID registers.

Andrew

author
#10 lore raw
Quoted 7 lines · Andrew Lunn
Quoted 3 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it.

Correct. They are like PCI or USB devices. You can ask it, what are you? There are two registers in standard locations which give you a vendor and product ID. We use that to find the correct driver.

For all the cases of PHYs that currently don't need any compatible string, requiring a compatible string of type "ethernet-phy-standard" would have been nice. That would have made PHYs consistent with the general DT norm of "you need a compatible string to be matched with the device". Anyway, it's too late to do that now. So I'll have to deal with this some other way (I have a bunch of ideas, so it's not the end of the world).

Quoted 7 lines

You only need a compatible when things are not so simple.

1) The IDs are wrong. Some silicon vendors do stupid things

2) Chicken/egg problems, you cannot read the ID registers until you load the driver and some resource is enabled.

Quoted 6 lines

3) It is a C45 devices, e.g. part of clause 45 of 802.3, which requires a different protocol to be talked over the bus. So the compatible string tells you to talk C45 to get the IDs.

4) It is not a PHY, but some sort of other MDIO device, and hence there are no ID registers.

Yeah, I was digging through of_mdiobus_child_is_phy() when I was doing the mdio-mux fixes and noticed this. But I missed/forgot the mdiobus doesn't probe part when I sent out the phy-handle patch.

-Saravana

#11 lore raw
Quoted 5 lines · Saravana Kannan
Quoted 4 lines · Andrew Lunn
Quoted 3 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it.

Quoted 7 lines
Quoted 3 lines

Correct. They are like PCI or USB devices. You can ask it, what are you? There are two registers in standard locations which give you a vendor and product ID. We use that to find the correct driver.

For all the cases of PHYs that currently don't need any compatible string, requiring a compatible string of type "ethernet-phy-standard" would have been nice.

How does this help you? You cannot match that against anything?

How do you handle PCI and USB devices? e.g.

arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi

&pcie {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_pcie>;
        reset-gpio = <&gpio7 12 GPIO_ACTIVE_LOW>;
        status = "okay";

        host@0 {
                reg = <0 0 0 0 0>;

                #address-cells = <3>;
                #size-cells = <2>;

                i210: i210@0 {
                        reg = <0 0 0 0 0>;
                };
        };
};

There is an intel i210 Ethernet control on the PCIe bus. There is no compatible string, none is needed. This is no different to a PHY.

Andrew

#12 lore raw
In reply to Saravana Kannan
Quoted 10 lines · Saravana Kannan
Quoted 7 lines · Andrew Lunn
Quoted 3 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it.

Correct. They are like PCI or USB devices. You can ask it, what are you? There are two registers in standard locations which give you a vendor and product ID. We use that to find the correct driver.

Quoted 7 lines

For all the cases of PHYs that currently don't need any compatible string, requiring a compatible string of type "ethernet-phy-standard" would have been nice. That would have made PHYs consistent with the general DT norm of "you need a compatible string to be matched with the device". Anyway, it's too late to do that now. So I'll have to deal with this some other way (I have a bunch of ideas, so it's not the end of the world).

This is not the first time the need for compatible strings for MDIO devices has come up, but MDIO devices are special (evidently). I should have taken a harder stance on this which should be simply, if your device requires having a node in DT, then it requires a compatible string.

Rob

#13 lore raw
In reply to Saravana Kannan

Hi,

Quoted 27 lines · Saravana Kannan
Quoted 13 lines · Marek Szyprowski
Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device doesn't have a compatible property. And so the device link dependency is propagated up to the mdio bus. But busses as suppliers aren't good because busses never "probe".

Quoted 7 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it. You can confirm my theory by checking for the list of suppliers under ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look at the "status" file under the folder it should be "dormant". If you add a compatible property that fits the formats a PHY node can have, that should also fix your issue (not the solution though).

Where should I look for the mentioned device links 'status' file?

# find /sys -name ff64c000.mdio-multiplexer
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
/sys/bus/platform/devices/ff64c000.mdio-multiplexer
# ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
total 0
lrwxrwxrwx 1 root root    0 Jan  1 00:04 
consumer:platform:ff3f0000.ethernet -> 
../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet
-rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
-r--r--r-- 1 root root 4096 Jan  1 00:04 modalias
lrwxrwxrwx 1 root root    0 Jan  1 00:04 of_node -> 
../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000
drwxr-xr-x 2 root root    0 Jan  1 00:02 power
lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem -> 
../../../../../bus/platform
lrwxrwxrwx 1 root root    0 Jan  1 00:04 
supplier:platform:ff63c000.system-controller:clock-controller -> 
../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer
-rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
-r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
# cat 
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
0

I'm also not sure what compatible string should I add there.

Quoted 2 lines

I'll send out a fix this week (once you confirm my analysis). Thanks for reporting it.

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author
#14 lore raw
Quoted 39 lines · Marek Szyprowski

Hi,

Quoted 27 lines · Saravana Kannan
Quoted 13 lines · Marek Szyprowski
Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device doesn't have a compatible property. And so the device link dependency is propagated up to the mdio bus. But busses as suppliers aren't good because busses never "probe".

Quoted 7 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it. You can confirm my theory by checking for the list of suppliers under ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look at the "status" file under the folder it should be "dormant". If you add a compatible property that fits the formats a PHY node can have, that should also fix your issue (not the solution though).

Quoted 8 lines

Where should I look for the mentioned device links 'status' file?

# find /sys -name ff64c000.mdio-multiplexer
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
/sys/bus/platform/devices/ff64c000.mdio-multiplexer
# ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
total 0

This is the folder I wanted you to check.

Quoted 3 lines

lrwxrwxrwx 1 root root 0 Jan 1 00:04
consumer:platform:ff3f0000.ethernet -> ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet

But I should have asked to look for the consumer list and not the supplier list. In any case, we can see that the ethernet is marked as the consumer of the mdio-multiplexer instead of the PHY device. So my hunch seems to be right.

Quoted 13 lines
(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
-r--r--r-- 1 root root 4096 Jan  1 00:04 modalias

lrwxrwxrwx 1 root root 0 Jan 1 00:04 of_node ->
../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000

drwxr-xr-x 2 root root    0 Jan  1 00:02 power
lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->

../../../../../bus/platform
lrwxrwxrwx 1 root root 0 Jan 1 00:04
supplier:platform:ff63c000.system-controller:clock-controller -> ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer

(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
-r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
Quoted 5 lines
# cat
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
0

I'm also not sure what compatible string should I add there.

It should have been added to external_phy: ethernet-phy@0. But don't worry about it (because you need to use a specific format for the compatible string).

-Saravana

Quoted 10 lines
Quoted 3 lines

I'll send out a fix this week (once you confirm my analysis). Thanks for reporting it.

Best regards

--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

author
#15 lore raw
Quoted 79 lines · Saravana Kannan
Quoted 39 lines · Marek Szyprowski

Hi,

Quoted 27 lines · Saravana Kannan
Quoted 13 lines · Marek Szyprowski
Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device doesn't have a compatible property. And so the device link dependency is propagated up to the mdio bus. But busses as suppliers aren't good because busses never "probe".

Quoted 7 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it. You can confirm my theory by checking for the list of suppliers under ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look at the "status" file under the folder it should be "dormant". If you add a compatible property that fits the formats a PHY node can have, that should also fix your issue (not the solution though).

Quoted 8 lines

Where should I look for the mentioned device links 'status' file?

# find /sys -name ff64c000.mdio-multiplexer
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
/sys/bus/platform/devices/ff64c000.mdio-multiplexer
# ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
total 0

This is the folder I wanted you to check.

Quoted 3 lines

lrwxrwxrwx 1 root root 0 Jan 1 00:04
consumer:platform:ff3f0000.ethernet -> ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet

But I should have asked to look for the consumer list and not the supplier list. In any case, we can see that the ethernet is marked as the consumer of the mdio-multiplexer instead of the PHY device. So my hunch seems to be right.

Quoted 17 lines
(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
-r--r--r-- 1 root root 4096 Jan  1 00:04 modalias

lrwxrwxrwx 1 root root 0 Jan 1 00:04 of_node ->
../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000

drwxr-xr-x 2 root root    0 Jan  1 00:02 power
lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->

../../../../../bus/platform
lrwxrwxrwx 1 root root 0 Jan 1 00:04
supplier:platform:ff63c000.system-controller:clock-controller -> ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer

(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
-r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier

# cat
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
0
Quoted 6 lines
Quoted 1 line

I'm also not sure what compatible string should I add there.

It should have been added to external_phy: ethernet-phy@0. But don't worry about it (because you need to use a specific format for the compatible string).

Marek,

Can you give this a shot?
https://lore.kernel.org/lkml/[email protected]/

This is not the main fix for the case you brought up, but it should fix your issue as a side effect of fixing another issue.

The main fix for your issue would be to teach fw_devlink that phy-handle always points to the actual DT node that'll become a device even if it doesn't have a compatible property. I'll send that out later.

-Saravana

#16 lore raw

Hi Saravana,

Quoted 78 lines · Saravana Kannan
Quoted 68 lines · Saravana Kannan
Quoted 35 lines · Marek Szyprowski
Quoted 27 lines · Saravana Kannan
Quoted 13 lines · Marek Szyprowski
Quoted 5 lines · Saravana Kannan

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc Andrew Lunn <[email protected]>
Cc [email protected]
Signed-off-by Saravana Kannan <[email protected]>

This patch landed recently in linux-next as commit cf4b94c8530d ("of:
property: fw_devlink: Add support for "phy-handle" property"). It breaks ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

Quoted 7 lines

In case of OdroidC4 I see the following entries in the /sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device doesn't have a compatible property. And so the device link dependency is propagated up to the mdio bus. But busses as suppliers aren't good because busses never "probe".

Quoted 7 lines

PHY seems to be one of those cases where it's okay to have the compatible property but also okay to not have it. You can confirm my theory by checking for the list of suppliers under ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look at the "status" file under the folder it should be "dormant". If you add a compatible property that fits the formats a PHY node can have, that should also fix your issue (not the solution though).

Quoted 8 lines

Where should I look for the mentioned device links 'status' file?

# find /sys -name ff64c000.mdio-multiplexer
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
/sys/bus/platform/devices/ff64c000.mdio-multiplexer
# ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
total 0

This is the folder I wanted you to check.

Quoted 3 lines

lrwxrwxrwx 1 root root 0 Jan 1 00:04
consumer:platform:ff3f0000.ethernet -> ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet

But I should have asked to look for the consumer list and not the supplier list. In any case, we can see that the ethernet is marked as the consumer of the mdio-multiplexer instead of the PHY device. So my hunch seems to be right.

Quoted 13 lines
(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
-r--r--r-- 1 root root 4096 Jan  1 00:04 modalias

lrwxrwxrwx 1 root root 0 Jan 1 00:04 of_node ->
../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000

drwxr-xr-x 2 root root    0 Jan  1 00:02 power
lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->

../../../../../bus/platform
lrwxrwxrwx 1 root root 0 Jan 1 00:04
supplier:platform:ff63c000.system-controller:clock-controller -> ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer

(quoted diff) +0 -2
-rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
-r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
Quoted 9 lines
Quoted 5 lines
# cat
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
0

I'm also not sure what compatible string should I add there.

It should have been added to external_phy: ethernet-phy@0. But don't worry about it (because you need to use a specific format for the compatible string).

Quoted 7 lines

Marek,

Can you give this a shot?
https://lore.kernel.org/lkml/[email protected]/

This is not the main fix for the case you brought up, but it should fix your issue as a side effect of fixing another issue.

I've just checked it and it doesn't help in my case. ff64c000.mdio-multiplexer and ff3f0000.ethernet are still not probed after applying this patch.

Quoted 4 lines

The main fix for your issue would be to teach fw_devlink that phy-handle always points to the actual DT node that'll become a device even if it doesn't have a compatible property. I'll send that out later.

I'm waiting for the proper fix then.

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author
#17 lore raw

PATCH 1/1 RFC: of: property: fix phy-hanlde issue

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 <[email protected]>
---
 drivers/of/property.c | 76 ++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 33 deletions(-)
drivers/of/property.c +43 -33
index 0c0dc2e369c0..039e1cb07348 100644
@@ -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);
 		}
Signature · 1 line
2.33.0.153.gba50c8fa24-goog
#18 lore raw

Re: [PATCH v1] RFC: of: property: fix phy-hanlde issue

Hi

Quoted 4 lines · Saravana Kannan

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. :(

Quoted 126 lines
Signed-off-by Saravana Kannan <[email protected]>
---
  drivers/of/property.c | 76 ++++++++++++++++++++++++-------------------
  1 file changed, 43 insertions(+), 33 deletions(-)
drivers/of/property.c +42 -32
index 0c0dc2e369c0..039e1cb07348 100644
@@ -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, },

drivers/of/property.c static int of_link_property(struct device_node *con_np, const char *prop_name) · line 1446

Quoted 8 lines
drivers/of/property.c +1 -1
@@ -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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author
#19 lore raw

Re: [PATCH v1] RFC: of: property: fix phy-hanlde issue

Quoted 10 lines · Marek Szyprowski

Hi

Quoted 4 lines · Saravana Kannan

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

author
#20 lore raw

Re: [PATCH v1] RFC: of: property: fix phy-hanlde issue

Quoted 28 lines · Saravana Kannan
Quoted 10 lines · Marek Szyprowski

Hi

Quoted 4 lines · Saravana Kannan

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.

Quoted 8 lines

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/[email protected]/

Quoted 2 lines

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

Quoted 29 lines

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

Tested-by
#21 lore raw

Re: [PATCH v1] RFC: of: property: fix phy-hanlde issue

Hi Saravana,

Quoted 32 lines · Saravana Kannan
Quoted 23 lines · Saravana Kannan
Quoted 7 lines · Marek Szyprowski
Quoted 4 lines · Saravana Kannan

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.

Quoted 8 lines

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.

Quoted 7 lines

Here's the fix I promised:
https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines

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 <[email protected]>

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland