Skip to contentSkip to conversation

net: qcom/emac: add shared mdio bus support

PATCH Superseded Wang Dongsheng 5 participants 39 messages Sep 17, 2018 – Nov 13, 2018

Subsystems (7)
lore
Share & tools
lore
#0 lore raw

v2 cover net: qcom/emac: add shared mdio bus support

The emac include MDIO controller, and the motherboard has more than one PHY connected to an MDIO bus. So share the shared mii_bus for others MAC device that not has MDIO bus connected.

Tested: QDF2400 (ACPI), buildin/insmod/rmmod

V2:
- Separate patch.
- bindings: s/Since QDF2400 emac/Since emac/

Wang Dongsheng (4):
net: qcom/emac: split phy_config to mdio bus create and get phy device
dt-bindings: net: qcom: Add binding for shared mdio bus
net: qcom/emac: add of shared mdio bus support
net: qcom/emac: add acpi shared mdio bus support

 .../devicetree/bindings/net/qcom-emac.txt     |   4 +
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 207 ++++++++++++++----
 drivers/net/ethernet/qualcomm/emac/emac.c     |   8 +-
 3 files changed, 175 insertions(+), 44 deletions(-)
Signature · 1 line
2.18.0
#1 lore raw

PATCH v2 1/4 net: qcom/emac: split phy_config to mdio bus create and get phy device

This patch separate emac_mdio_bus_create and emac_get_phydev from emac_phy_config, and do some codes clean.

---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 99 +++++++++++--------
 1 file changed, 57 insertions(+), 42 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-phy.c +57 -42
index 53dbf1e163a8..2d16c6b9ef49 100644
@@ -96,15 +96,14 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
-/* Configure the MDIO bus and connect the external PHY */
-int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+static int emac_mdio_bus_create(struct platform_device *pdev,
+				struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *mii_bus;
 	int ret;
 
-	/* Create the mii_bus object for talking to the MDIO bus */
-	adpt->mii_bus = mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
 	if (!mii_bus)
 		return -ENOMEM;
 
@@ -115,50 +114,66 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	if (has_acpi_companion(&pdev->dev)) {
-		u32 phy_addr;
-
-		ret = mdiobus_register(mii_bus);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
-		ret = device_property_read_u32(&pdev->dev, "phy-channel",
-					       &phy_addr);
-		if (ret)
-			/* If we can't read a valid phy address, then assume
-			 * that there is only one phy on this mdio bus.
-			 */
-			adpt->phydev = phy_find_first(mii_bus);
-		else
-			adpt->phydev = mdiobus_get_phy(mii_bus, phy_addr);
-
-		/* of_phy_find_device() claims a reference to the phydev,
-		 * so we do that here manually as well. When the driver
-		 * later unloads, it can unilaterally drop the reference
-		 * without worrying about ACPI vs DT.
-		 */
-		if (adpt->phydev)
-			get_device(&adpt->phydev->mdio.dev);
-	} else {
-		struct device_node *phy_np;
+	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
+				  NULL : np);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not register mdio bus\n");
+		return ret;
+	}
 
-		ret = of_mdiobus_register(mii_bus, np);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
+	adpt->mii_bus = mii_bus;
+	return 0;
+}
 
+static void emac_get_phydev(struct platform_device *pdev,
+			    struct emac_adapter *adpt)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mii_bus *bus = adpt->mii_bus;
+	struct device_node *phy_np;
+	u32 phy_addr;
+	int ret;
+
+	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
 		of_node_put(phy_np);
+		return;
 	}
 
-	if (!adpt->phydev) {
-		dev_err(&pdev->dev, "could not find external phy\n");
-		mdiobus_unregister(mii_bus);
-		return -ENODEV;
-	}
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	if (ret)
+		/* If we can't read a valid phy address, then assume
+		 * that there is only one phy on this mdio bus.
+		 */
+		adpt->phydev = phy_find_first(bus);
+	else
+		adpt->phydev = mdiobus_get_phy(bus, phy_addr);
+
+	/* of_phy_find_device() claims a reference to the phydev,
+	 * so we do that here manually as well. When the driver
+	 * later unloads, it can unilaterally drop the reference
+	 * without worrying about ACPI vs DT.
+	 */
+	if (adpt->phydev)
+		get_device(&adpt->phydev->mdio.dev);
+}
 
-	return 0;
+/* Configure the MDIO bus and connect the external PHY */
+int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	int ret;
+
+	ret = emac_mdio_bus_create(pdev, adpt);
+	if (ret)
+		return ret;
+
+	emac_get_phydev(pdev, adpt);
+	if (adpt->phydev)
+		return 0;
+
+	dev_err(&pdev->dev, "Could not find external phy\n");
+	mdiobus_unregister(adpt->mii_bus);
+	return -ENODEV;
 }
Signature · 1 line
2.18.0
#2 lore raw

PATCH v2 2/4 dt-bindings: net: qcom: Add binding for shared mdio bus

In reply to Wang Dongsheng

This property copy from "ibm,emac.txt" to describe a shared MIDO bus. Since emac include MDIO, so If the motherboard has more than one PHY connected to an MDIO bus, this property will point to the MAC device that has the MDIO bus.

---
V2: s/Since QDF2400 emac/Since emac/

---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/net/qcom-emac.txt +4 -0
index 346e6c7f47b7..50db71771358 100644
@@ -24,6 +24,9 @@ Internal PHY node:
 The external phy child node:
 - reg : The phy address
 
+Optional properties:
+- mdio-device : Shared MIDO bus.
+
 Example:
 
 FSM9900:
@@ -86,6 +89,7 @@ soc {
 		reg = <0x0 0x38800000 0x0 0x10000>,
 		      <0x0 0x38816000 0x0 0x1000>;
 		interrupts = <0 256 4>;
+		mdio-device = <&emac1>;
 
 		clocks = <&gcc 0>, <&gcc 1>, <&gcc 3>, <&gcc 4>, <&gcc 5>,
 			 <&gcc 6>, <&gcc 7>;
Signature · 1 line
2.18.0
maintainer
#3 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 17 lines · Wang Dongsheng

This property copy from "ibm,emac.txt" to describe a shared MIDO bus. Since emac include MDIO, so If the motherboard has more than one PHY connected to an MDIO bus, this property will point to the MAC device that has the MDIO bus.

---
V2: s/Since QDF2400 emac/Since emac/

---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/net/qcom-emac.txt +0 -0
index 346e6c7f47b7..50db71771358 100644

Documentation/devicetree/bindings/net/qcom-emac.txt Internal PHY node: · line 28

Quoted 5 lines
Documentation/devicetree/bindings/net/qcom-emac.txt +2 -0
@@ -24,6 +24,9 @@ Internal PHY node:
 The external phy child node:
 - reg : The phy address
 
+Optional properties:
+- mdio-device : Shared MIDO bus.

Hi Dongsheng

I don't see why you need this property. The ethernet interface has a phy-handle which points to a PHY. That is all you need to find the PHY.

        emac0: ethernet@feb20000 {
                compatible = "qcom,fsm9900-emac";
                reg = <0xfeb20000 0x10000>,
                      <0xfeb36000 0x1000>;
                interrupts = <76>;

                clocks = <&gcc 0>, <&gcc 1>, <&gcc 3>, <&gcc 4>, <&gcc 5>,
                        <&gcc 6>, <&gcc 7>;
                clock-names = "axi_clk", "cfg_ahb_clk", "high_speed_clk",
                        "mdio_clk", "tx_clk", "rx_clk", "sys_clk";

                internal-phy = <&emac_sgmii>;

                phy-handle = <&phy0>;

                #address-cells = <1>;
                #size-cells = <0>;

                phy0: ethernet-phy@0 {
                        reg = <0>;
                };

                phy1: ethernet-phy@1 {
                        reg = <1>;
                };

                pinctrl-names = "default";
                pinctrl-0 = <&mdio_pins_a>;
        };

        emac1: ethernet@38900000 {
                compatible = "qcom,fsm9900-emac";
		...
		...

                phy-handle = <&phy1>;
	};

	Andrew
#4 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 21 lines · Andrew Lunn
Quoted 20 lines · Wang Dongsheng

This property copy from "ibm,emac.txt" to describe a shared MIDO bus. Since emac include MDIO, so If the motherboard has more than one PHY connected to an MDIO bus, this property will point to the MAC device that has the MDIO bus.

---
V2: s/Since QDF2400 emac/Since emac/

---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/net/qcom-emac.txt +0 -0
index 346e6c7f47b7..50db71771358 100644
@@ -24,6 +24,9 @@ Internal PHY node:
 The external phy child node:
 - reg : The phy address
 
Quoted 6 lines

Documentation/devicetree/bindings/net/qcom-emac.txt Internal PHY node: · line 28

Quoted 2 lines
Documentation/devicetree/bindings/net/qcom-emac.txt +2 -0
+Optional properties:
+- mdio-device : Shared MIDO bus.

Hi Dongsheng

I don't see why you need this property. The ethernet interface has a phy-handle which points to a PHY. That is all you need to find the PHY.

phy-handle is description PHY address. This property is describing an MDIO controller.
Each QCOM emac include an MDIO controller, normally each emac only connect one
PHY device, but when all of the PHY devices mdio lines connect one MDIO controller
that is included in EMAC, we need to share this MDIO controller for others EMAC.

Normally:

                                (MDIO)
MAC0 ---------------------------------------PHY0
     |                                                               |
     |                          (DATA)                         |
     ---------------------------------------------


                                (MDIO)
MAC1 ---------------------------------------PHY1
     |                                                               |
     |                          (DATA)                         |
     ---------------------------------------------

Shared MDIO bus: "mdio-device" = &MAC0, MAC1 will get MAC0's MDIO bus and also get the corresponding PHY device.

                                (DATA)
MAC0 ---------------------------------------PHY0
     |                                                               |
     |                          (MDIO)                         |
     --------------------------------------------
                                                                     |
MAC1                                                        PHY0
     |                                                               |
     |                          (DATA)                         |
     --------------------------------------------

Cheers,
Dongsheng

Quoted 40 lines
        emac0: ethernet@feb20000 {
                compatible = "qcom,fsm9900-emac";
                reg = <0xfeb20000 0x10000>,
                      <0xfeb36000 0x1000>;
                interrupts = <76>;

                clocks = <&gcc 0>, <&gcc 1>, <&gcc 3>, <&gcc 4>, <&gcc 5>,
                        <&gcc 6>, <&gcc 7>;
                clock-names = "axi_clk", "cfg_ahb_clk", "high_speed_clk",
                        "mdio_clk", "tx_clk", "rx_clk", "sys_clk";

                internal-phy = <&emac_sgmii>;

                phy-handle = <&phy0>;

                #address-cells = <1>;
                #size-cells = <0>;

                phy0: ethernet-phy@0 {
                        reg = <0>;
                };

                phy1: ethernet-phy@1 {
                        reg = <1>;
                };

                pinctrl-names = "default";
                pinctrl-0 = <&mdio_pins_a>;
        };

        emac1: ethernet@38900000 {
                compatible = "qcom,fsm9900-emac";
		...
		...

                phy-handle = <&phy1>;
	};

	Andrew
#5 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 28 lines · Dongsheng
Quoted 21 lines · Andrew Lunn
Quoted 20 lines · Wang Dongsheng

This property copy from "ibm,emac.txt" to describe a shared MIDO bus. Since emac include MDIO, so If the motherboard has more than one PHY connected to an MDIO bus, this property will point to the MAC device that has the MDIO bus.

---
V2: s/Since QDF2400 emac/Since emac/

---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/net/qcom-emac.txt +0 -0
index 346e6c7f47b7..50db71771358 100644
@@ -24,6 +24,9 @@ Internal PHY node:
 The external phy child node:
 - reg : The phy address
 
Quoted 6 lines

Documentation/devicetree/bindings/net/qcom-emac.txt Internal PHY node: · line 28

Quoted 2 lines
Documentation/devicetree/bindings/net/qcom-emac.txt +2 -0
+Optional properties:
+- mdio-device : Shared MIDO bus.

Hi Dongsheng

I don't see why you need this property. The ethernet interface has a phy-handle which points to a PHY. That is all you need to find the PHY.

Quoted 8 lines

phy-handle is description PHY address. This property is describing an MDIO controller.
Each QCOM emac include an MDIO controller, normally each emac only connect one
PHY device, but when all of the PHY devices mdio lines connect one MDIO controller
that is included in EMAC, we need to share this MDIO controller for others EMAC.

If you want to describe the MDIO controller, then you embed a mdio subnode into your Ethernet MAC node:

 emac0: ethernet@feb20000 {
	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: ethernet-phy@0 {
			reg = <0>;
		};
	};
};

And then each Ethernet MAC controller refers to their appropriate PHY device tree node using a phy-handle property to point to either their own MDIO controller, or another MAC's MDIO controller.

The IBM Emac is a old (not to say bad) example and it does not use the PHY library and the standard Device Tree node property, please don't use it as a reference.

Signature · 1 line
Florian
#6 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 51 lines · Florian Fainelli
Quoted 28 lines · Dongsheng
Quoted 21 lines · Andrew Lunn
Quoted 20 lines · Wang Dongsheng

This property copy from "ibm,emac.txt" to describe a shared MIDO bus. Since emac include MDIO, so If the motherboard has more than one PHY connected to an MDIO bus, this property will point to the MAC device that has the MDIO bus.

---
V2: s/Since QDF2400 emac/Since emac/

---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/net/qcom-emac.txt +0 -0
index 346e6c7f47b7..50db71771358 100644
@@ -24,6 +24,9 @@ Internal PHY node:
 The external phy child node:
 - reg : The phy address
 
Quoted 6 lines

Documentation/devicetree/bindings/net/qcom-emac.txt Internal PHY node: · line 28

Quoted 2 lines
Documentation/devicetree/bindings/net/qcom-emac.txt +2 -0
+Optional properties:
+- mdio-device : Shared MIDO bus.

Hi Dongsheng

I don't see why you need this property. The ethernet interface has a phy-handle which points to a PHY. That is all you need to find the PHY.

Quoted 8 lines

phy-handle is description PHY address. This property is describing an MDIO controller.
Each QCOM emac include an MDIO controller, normally each emac only connect one
PHY device, but when all of the PHY devices mdio lines connect one MDIO controller
that is included in EMAC, we need to share this MDIO controller for others EMAC.

If you want to describe the MDIO controller, then you embed a mdio subnode into your Ethernet MAC node:

 emac0: ethernet@feb20000 {
	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: ethernet-phy@0 {
			reg = <0>;
		};
	};
};
Quoted 3 lines

And then each Ethernet MAC controller refers to their appropriate PHY device tree node using a phy-handle property to point to either their own MDIO controller, or another MAC's MDIO controller.

Sorry, I do not understand how phy-handle point to MDIO controller, because phy-handle is defined to point to a phy.
I suppose you mean:
mdio_node = of_get_parent(phy_node);

emac0: ethernet@feb20000 {
    phy-handle = <&phy0>;

    mdio {
        #address-cells = <1>;
        #size-cells = <0>;

        phy0: ethernet-phy@0 {
            reg = <0>;
        };

        phy1: ethernet-phy@1 {
            reg = <1>;
        };
    };
};
emac1: ethernet@fexxxxxx {
    phy-handle = <&phy1>;
};
emac2: ethernet@fexxxxxx {
    phy-handle = <&phy2>;
    mdio {
        #address-cells = <1>;
        #size-cells = <0>;

        phy2: ethernet-phy@2 {
            reg = <0>;
        };
    };
};
Quoted 3 lines

The IBM Emac is a old (not to say bad) example and it does not use the PHY library and the standard Device Tree node property, please don't use it as a reference.

Ok, thanks.

Cheers,
Dongsheng

maintainer
#7 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 14 lines
Quoted 14 lines

If you want to describe the MDIO controller, then you embed a mdio subnode into your Ethernet MAC node:

 emac0: ethernet@feb20000 {
	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: ethernet-phy@0 {
			reg = <0>;
		};
	};
};
Quoted 6 lines
Quoted 3 lines

And then each Ethernet MAC controller refers to their appropriate PHY device tree node using a phy-handle property to point to either their own MDIO controller, or another MAC's MDIO controller.

Sorry, I do not understand how phy-handle point to MDIO controller, because phy-handle is defined to point to a phy.

The MAC driver does not care what MDIO controller a PHY is on. All you need to do to register the PHY is:

	phy_node = of_parse_phandle(np, "phy-handle", 0);
	phy_interface = of_get_phy_mode(np);
	phydev = of_phy_connect(dev, phy_node,
                                &handle_link_change, 0,
                                phy_interface);

	Andrew
#8 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 14 lines · Andrew Lunn
Quoted 14 lines
Quoted 14 lines

If you want to describe the MDIO controller, then you embed a mdio subnode into your Ethernet MAC node:

 emac0: ethernet@feb20000 {
	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: ethernet-phy@0 {
			reg = <0>;
		};
	};
};
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

And then each Ethernet MAC controller refers to their appropriate PHY device tree node using a phy-handle property to point to either their own MDIO controller, or another MAC's MDIO controller.

Sorry, I do not understand how phy-handle point to MDIO controller, because phy-handle is defined to point to a phy.

The MAC driver does not care what MDIO controller a PHY is on. All you need to do to register the PHY is:

Yes, these are all things that must be done, and emac driver will connect phy when mac up.
If we had a separate MDIO controller, the MAC would not care about MDIO bus. But MDIO is integrated within the EMAC, and emac driver maintains the mdio.

Each EMAC do their mdio register/unregister. But in the shared scenario, the EMACs that use the shared bus do not need to create an MDIO and cannot release the Shared bus.

In device tree environment as you and Florian said. Just use phy-handle get the phy_node.
The EMAC would not care about the phy come from which MDIO bus because the phy device gets from the device_node match(phy-handle). And if the phy_dev cannot get through phy_node means, the mdio bus is not ready.

But ACPI environment my understand is this:
First method. EMAC driver gets the shared MDIO bus, and maintain it. The second way, EMAC match the phy_dev from the name. These patch series try to use the FIRST way. Now I prefer to use the second way to do the shared function.

I will rework this patchset and maybe patches will be a delay for a few days.

Cheers,
Dongsheng

Quoted 8 lines
	phy_node = of_parse_phandle(np, "phy-handle", 0);
	phy_interface = of_get_phy_mode(np);
	phydev = of_phy_connect(dev, phy_node,
                                &handle_link_change, 0,
                                phy_interface);

	Andrew
maintainer
#9 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 29 lines · Dongsheng
Quoted 14 lines · Andrew Lunn
Quoted 14 lines
Quoted 14 lines

If you want to describe the MDIO controller, then you embed a mdio subnode into your Ethernet MAC node:

 emac0: ethernet@feb20000 {
	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		phy0: ethernet-phy@0 {
			reg = <0>;
		};
	};
};
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

And then each Ethernet MAC controller refers to their appropriate PHY device tree node using a phy-handle property to point to either their own MDIO controller, or another MAC's MDIO controller.

Sorry, I do not understand how phy-handle point to MDIO controller, because phy-handle is defined to point to a phy.

The MAC driver does not care what MDIO controller a PHY is on. All you need to do to register the PHY is:

Yes, these are all things that must be done, and emac driver will connect phy when mac up.
If we had a separate MDIO controller, the MAC would not care about MDIO bus. But MDIO is integrated within the EMAC, and emac driver maintains the mdio.

Quoted 3 lines

Each EMAC do their mdio register/unregister. But in the shared scenario, the EMACs that use the shared bus do not need to create an MDIO and cannot release the Shared bus.

Hi Dongsheng

There is nothing new here. Many Ethernet drivers export an MDIO bus which is then used by some other device, often an Ethernet switch. Ordering should not be a problem, you just need to handle EPROBE_DEFER, which will happen if the MDIO bus has not yet been probed when you try to lookup the phy-handle. And once the phy has been connected, the MDIO bus will be locked, preventing it from being removed.

Quoted 1 line

But ACPI environment my understand is this:

ACPI is completely separate and should not affect the DT binding. I've not yet looked at the ACPI changes you added.

Quoted 2 lines

I will rework this patchset and maybe patches will be a delay for a few days.

Thanks

Andrew

maintainer
#10 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 2 lines · Andrew Lunn

ACPI is completely separate and should not affect the DT binding. I've not yet looked at the ACPI changes you added.

Just FYI, there is no device tree platform on which the upstream EMAC driver works. All of the DT code in the driver is theoretical. It worked once on a prototype platform, when I originally wrote the code, but since then DT support is mostly a guess.

The focus of any patches for the EMAC should be ACPI, not DT. If anything, ACPI support should come first. No one should be writing or reviewing DT code before ACPI code.

The upstream EMAC driver is only known to work on the QDF2400, which is an ACPI-only chip. I feel like I've been repeating this too often.

maintainer
#11 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 3 lines

The focus of any patches for the EMAC should be ACPI, not DT. If anything, ACPI support should come first. No one should be writing or reviewing DT code before ACPI code.

I suspect that is not going to be easy. Last time i looked, the ACPI standard had nothing about MDIO busses or PHYs. Marcin Wojtas did some work in this area a while back for the mvpp2, but if i remember correctly, he worked around this by simply not having a PHY when using ACPI, and making use of a MAC interrupt which indicated when there was link.

Whoever implements this first needs to be an ACPI expert and probably needs to write it up and submit it as an amendment to the ACPI standard.

Andrew

maintainer
#12 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

Quoted 10 lines · Andrew Lunn

I suspect that is not going to be easy. Last time i looked, the ACPI standard had nothing about MDIO busses or PHYs. Marcin Wojtas did some work in this area a while back for the mvpp2, but if i remember correctly, he worked around this by simply not having a PHY when using ACPI, and making use of a MAC interrupt which indicated when there was link.

Whoever implements this first needs to be an ACPI expert and probably needs to write it up and submit it as an amendment to the ACPI standard.

If that's what it takes, then so be it. But adding DT support for a device that is only used on ACPI platforms is not a worthwhile endeavor.

After ACPI support is merged, Dongsheng can choose to add DT support to maintain parity, if he wants. Maybe one day the MSM developers will use the upstream driver on one of their SOCs.

#13 lore raw

v3 cover net: qcom/emac: add shared mdio bus support

The emac include MDIO controller, and the motherboard has more than one PHY connected to an MDIO bus. So share the shared mii_bus for others MAC device that not has MDIO bus connected.

Based on ACPI, since "phy-handle" cannot directly point to a _DSD sub-package, so we use "phy-handle" to point an internal MDIO device port. The port describes the phy address.

Tested: QDF2400 (ACPI), buildin/insmod/rmmod

V3:
- Add "phy-handle" support.
- Remove all of DT changes.

V2:
- Separate patch.

Wang Dongsheng (2):
net: qcom/emac: split phy_config to mdio bus create and get phy device
net: qcom/emac: add phy-handle support for ACPI

 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 183 ++++++++++++++----
 1 file changed, 142 insertions(+), 41 deletions(-)
Signature · 1 line
2.18.0
#14 lore raw

PATCH v3 1/2 net: qcom/emac: split phy_config to mdio bus create and get phy device

This patch separate emac_mdio_bus_create and emac_get_phydev from emac_phy_config, and do some codes clean.

---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 96 +++++++++++--------
 1 file changed, 56 insertions(+), 40 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-phy.c +56 -40
index 53dbf1e163a8..f2ed013ce5d5 100644
@@ -96,15 +96,15 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
-/* Configure the MDIO bus and connect the external PHY */
-int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+static int emac_mdio_bus_create(struct platform_device *pdev,
+				struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *mii_bus;
 	int ret;
 
 	/* Create the mii_bus object for talking to the MDIO bus */
-	adpt->mii_bus = mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
 	if (!mii_bus)
 		return -ENOMEM;
 
@@ -115,50 +115,66 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	if (has_acpi_companion(&pdev->dev)) {
-		u32 phy_addr;
-
-		ret = mdiobus_register(mii_bus);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
-		ret = device_property_read_u32(&pdev->dev, "phy-channel",
-					       &phy_addr);
-		if (ret)
-			/* If we can't read a valid phy address, then assume
-			 * that there is only one phy on this mdio bus.
-			 */
-			adpt->phydev = phy_find_first(mii_bus);
-		else
-			adpt->phydev = mdiobus_get_phy(mii_bus, phy_addr);
-
-		/* of_phy_find_device() claims a reference to the phydev,
-		 * so we do that here manually as well. When the driver
-		 * later unloads, it can unilaterally drop the reference
-		 * without worrying about ACPI vs DT.
-		 */
-		if (adpt->phydev)
-			get_device(&adpt->phydev->mdio.dev);
-	} else {
-		struct device_node *phy_np;
-
-		ret = of_mdiobus_register(mii_bus, np);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
+	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
+				  NULL : np);
+	if (ret)
+		dev_err(&pdev->dev, "Could not register mdio bus\n");
+
+	adpt->mii_bus = ret ? NULL : mii_bus;
+	return ret;
+}
+
+static int emac_get_phydev(struct platform_device *pdev,
+			   struct emac_adapter *adpt)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mii_bus *bus = adpt->mii_bus;
+	struct device_node *phy_np;
+	struct phy_device *phydev;
 
+	u32 phy_addr;
+	int ret;
+
+	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
 		of_node_put(phy_np);
+		return adpt->phydev ? 0 : -ENODEV;
 	}
 
-	if (!adpt->phydev) {
-		dev_err(&pdev->dev, "could not find external phy\n");
-		mdiobus_unregister(mii_bus);
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	/* If we can't read a valid phy address, then assume
+	 * that there is only one phy on this mdio bus.
+	 */
+	phydev = ret ? phy_find_first(bus) : mdiobus_get_phy(bus, phy_addr);
+	if (!phydev)
 		return -ENODEV;
-	}
 
+	/* of_phy_find_device() claims a reference to the phydev,
+	 * so we do that here manually as well. When the driver
+	 * later unloads, it can unilaterally drop the reference
+	 * without worrying about ACPI vs DT.
+	 */
+	get_device(&phydev->mdio.dev);
+	adpt->phydev = phydev;
 	return 0;
 }
+
+/* Configure the MDIO bus and connect the external PHY */
+int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	int ret;
+
+	ret = emac_mdio_bus_create(pdev, adpt);
+	if (ret)
+		return ret;
+
+	ret = emac_get_phydev(pdev, adpt);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not find external phy\n");
+		mdiobus_unregister(adpt->mii_bus);
+	}
+
+	return ret;
+}
Signature · 1 line
2.18.0
#15 lore raw

PATCH v3 2/2 net: qcom/emac: add phy-handle support for ACPI

In reply to Wang Dongsheng

Use "phy-handle" to porint an internal MDIO device port.

---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 115 +++++++++++++++---
 1 file changed, 100 insertions(+), 15 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-phy.c +100 -15
index f2ed013ce5d5..3dc3ae55e5bb 100644
@@ -96,6 +96,96 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
+static int acpi_device_match(struct device *dev, void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
+static struct phy_device *
+emac_acpi_get_phydev_from_phy_handle(struct platform_device *pdev)
+{
+	struct fwnode_reference_args args;
+	struct fwnode_handle *fw_node;
+	struct acpi_device *adev;
+	acpi_handle handle;
+	struct device *dev;
+	struct phy_device *phydev;
+	struct net_device *netdev;
+	struct emac_adapter *adpt;
+	int phy_addr;
+	int ret;
+
+	/* Get PHY Port reference from phy-handle */
+	fw_node = acpi_fwnode_handle(ACPI_COMPANION(&pdev->dev));
+	ret = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
+					       &args);
+	if (ACPI_FAILURE(ret) || !is_acpi_device_node(args.fwnode))
+		return ERR_PTR(-ENODEV);
+
+	/* Get PHY addr from the port node */
+	if (fwnode_property_read_u32(args.fwnode, "phy-channel", &phy_addr))
+		return ERR_PTR(-ENODEV);
+
+	/* Get the MDIO bus that included the port */
+	handle = ACPI_HANDLE_FWNODE(args.fwnode);
+	if (!handle || acpi_bus_get_device(handle, &adev))
+		return ERR_PTR(-ENODEV);
+
+	while (adev->parent) {
+		if (!strcmp(acpi_device_hid(adev), "QCOM8070"))
+			break;
+		adev = adev->parent;
+	}
+	if (!adev->parent)
+		return ERR_PTR(-ENODEV);
+
+	dev = bus_find_device(&platform_bus_type, NULL,
+			      &adev->fwnode,
+			      acpi_device_match);
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	netdev = dev_get_drvdata(dev);
+	if (!netdev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	adpt = netdev_priv(netdev);
+	if (!adpt->mii_bus)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	phydev = mdiobus_get_phy(adpt->mii_bus, phy_addr);
+	return phydev ? phydev : ERR_PTR(-ENODEV);
+}
+
+static struct phy_device *
+emac_acpi_get_phydev(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	struct phy_device *phydev = NULL;
+	int phy_addr;
+	int ret;
+
+	/* Compatible with "phy-channel" */
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	if (!ret)
+		phydev = mdiobus_get_phy(adpt->mii_bus, phy_addr);
+	if (phydev)
+		return phydev;
+
+	/* Get PHY Port reference from phy-handle */
+	phydev = emac_acpi_get_phydev_from_phy_handle(pdev);
+	if (!IS_ERR(phydev))
+		return phydev;
+	if (PTR_ERR(phydev) == -EPROBE_DEFER)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	/* If we can't read a valid phy address from "phy-channel"/"phy-handle",
+	 * then assume that there is only one phy on local mdio bus.
+	 */
+	phydev = phy_find_first(adpt->mii_bus);
+	return phydev ? phydev : ERR_PTR(-ENODEV);
+}
+
 static int emac_mdio_bus_create(struct platform_device *pdev,
 				struct emac_adapter *adpt)
 {
@@ -128,13 +218,9 @@ static int emac_get_phydev(struct platform_device *pdev,
 			    struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct mii_bus *bus = adpt->mii_bus;
 	struct device_node *phy_np;
 	struct phy_device *phydev;
 
-	u32 phy_addr;
-	int ret;
-
 	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
@@ -142,14 +228,9 @@ static int emac_get_phydev(struct platform_device *pdev,
 		return adpt->phydev ? 0 : -ENODEV;
 	}
 
-	ret = device_property_read_u32(&pdev->dev, "phy-channel",
-				       &phy_addr);
-	/* If we can't read a valid phy address, then assume
-	 * that there is only one phy on this mdio bus.
-	 */
-	phydev = ret ? phy_find_first(bus) : mdiobus_get_phy(bus, phy_addr);
-	if (!phydev)
-		return -ENODEV;
+	phydev = emac_acpi_get_phydev(pdev, adpt);
+	if (IS_ERR(phydev))
+		return PTR_ERR(phydev);
 
 	/* of_phy_find_device() claims a reference to the phydev,
 	 * so we do that here manually as well. When the driver
@@ -171,10 +252,14 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 		return ret;
 
 	ret = emac_get_phydev(pdev, adpt);
-	if (ret) {
+	if (!ret)
+		return 0;
+
+	if (ret != -EPROBE_DEFER)
 		dev_err(&pdev->dev, "Could not find external phy\n");
-		mdiobus_unregister(adpt->mii_bus);
-	}
+	else
+		dev_warn(&pdev->dev, "Phy is not available yet, deferred probing\n");
 
+	mdiobus_unregister(adpt->mii_bus);
 	return ret;
 }
Signature · 1 line
2.18.0
maintainer
#16 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 1 line · Wang Dongsheng

Use "phy-handle" to porint an internal MDIO device port.

Hi Dongsheng

You are basically defining how all future ACPI based MAC drivers get access to their PHY. This needs to become part of the ACPI standard, etc.

This code should not be hidden away in the emac driver. It needs to be placed somewhere public so other drivers can use it. And it needs good documentation, including an example of what needs to go into the ACPI tables, etc.

Thanks
Andrew

#17 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 4 lines · Andrew Lunn
Quoted 1 line · Wang Dongsheng

Use "phy-handle" to porint an internal MDIO device port.

Hi Dongsheng

Quoted 8 lines

You are basically defining how all future ACPI based MAC drivers get access to their PHY. This needs to become part of the ACPI standard, etc.

This code should not be hidden away in the emac driver. It needs to be placed somewhere public so other drivers can use it. And it needs good documentation, including an example of what needs to go into the ACPI tables, etc.

Hi Andrew

I saw AppliedMicro(apm) xgene has used "phy-handle" for ACPI method, so I guess "phy-handle" has become part of the ACPI standard. But I cannot make sure.
I tried to confirm the property is defined in the document(Like DT binding). However, I did not find any documentation on the description property definition on the UEFI/ACPICA website.
But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

Cheers,
Dongsheng

Quoted 3 lines

Thanks
Andrew

maintainer
#18 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

#19 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 8 lines · Timur Tabi
Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Yes, here's a database concept, and I asked some Intel guys, the answer I got was there is no such database or document. :(

Cheers,

Dongsheng

maintainer
#20 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 5 lines · Dongsheng
Quoted 4 lines · Timur Tabi
Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

Quoted 7 lines
Quoted 4 lines

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Yes, here's a database concept, and I asked some Intel guys, the answer I got was there is no such database or document. :(

Hi Dongsheng

If there is no clear documentation for ACPI, it becomes even more important that the xgene code is refactored into a central location, and you make use of it. We really need to avoid every ACPI ethernet driver doing its own thing.

Thanks
Andrew

#21 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 10 lines · Andrew Lunn
Quoted 9 lines · Dongsheng
Quoted 8 lines · Timur Tabi
Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Quoted 8 lines
Quoted 2 lines

Yes, here's a database concept, and I asked some Intel guys, the answer I got was there is no such database or document. :(

Hi Dongsheng

If there is no clear documentation for ACPI, it becomes even more important that the xgene code is refactored into a central location, and you make use of it. We really need to avoid every ACPI ethernet driver doing its own thing.

However, without a document specifying MDIO and phy-handle, it is almost difficult for us to do this. Because maybe the ACPI device or property corresponding to each platform is different.
Just like APM looks different to us. APM's MDIO adev doesn't describe the concept of port, and our platform does. Besides, I cannot get the ACPI table of APM or other manufacturers.
The table of ACPI cannot be obtained from kernel source as easily as DT. We can't know without a platform to do ACPI dump. Unless some of the manufacturers have pushed the table to upstream.
So I think we might have a hard time doing this without a document. And it's likely that this work involves code modifications by BIOS vendors.

Cheers,
Dongsheng

maintainer
#22 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 20 lines · Dongsheng
Quoted 10 lines · Andrew Lunn
Quoted 9 lines · Dongsheng
Quoted 8 lines · Timur Tabi
Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Quoted 8 lines
Quoted 2 lines

Yes, here's a database concept, and I asked some Intel guys, the answer I got was there is no such database or document. :(

Hi Dongsheng

If there is no clear documentation for ACPI, it becomes even more important that the xgene code is refactored into a central location, and you make use of it. We really need to avoid every ACPI ethernet driver doing its own thing.

Quoted 11 lines

However, without a document specifying MDIO and phy-handle, it is almost difficult for us to do this. Because maybe the ACPI device or property corresponding to each platform is different.
Just like APM looks different to us. APM's MDIO adev doesn't describe the concept of port, and our platform does. Besides, I cannot get the ACPI table of APM or other manufacturers.
The table of ACPI cannot be obtained from kernel source as easily as DT. We can't know without a platform to do ACPI dump. Unless some of the manufacturers have pushed the table to upstream.
So I think we might have a hard time doing this without a document. And it's likely that this work involves code modifications by BIOS vendors.

Hi Dongsheng

There are two different options here.

1) Everybody does their own thing, ignoring what everybody else has done, and invents their own wheel. There is no shared code, no shared description, everybody has their own bugs, etc. ACPI as a standard is pointless for Ethernet MDIOs and PHYs because it is not a standard, everybody does something different.

2) Somebody takes the time to design a concept for Ethernet PHYs and MDIO busses using ACPI. They implement the common code, try to modify any existing users if possible, and submit the whole thing to become part of ACPI 6.3.

I would really prefer we go the second route here. It is more initial effort, but in the long run, everybody benefits.

Andrew

#23 lore raw

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

Quoted 41 lines · Andrew Lunn
Quoted 19 lines · Dongsheng
Quoted 10 lines · Andrew Lunn
Quoted 9 lines · Dongsheng
Quoted 8 lines · Timur Tabi
Quoted 3 lines · Dongsheng

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my understanding is we should try to conform to DT bindings. So maybe ACPI doesn't have such a document, just DT bindings.

There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Quoted 8 lines
Quoted 2 lines

Yes, here's a database concept, and I asked some Intel guys, the answer I got was there is no such database or document. :(

Hi Dongsheng

If there is no clear documentation for ACPI, it becomes even more important that the xgene code is refactored into a central location, and you make use of it. We really need to avoid every ACPI ethernet driver doing its own thing.

Quoted 11 lines

However, without a document specifying MDIO and phy-handle, it is almost difficult for us to do this. Because maybe the ACPI device or property corresponding to each platform is different.
Just like APM looks different to us. APM's MDIO adev doesn't describe the concept of port, and our platform does. Besides, I cannot get the ACPI table of APM or other manufacturers.
The table of ACPI cannot be obtained from kernel source as easily as DT. We can't know without a platform to do ACPI dump. Unless some of the manufacturers have pushed the table to upstream.
So I think we might have a hard time doing this without a document. And it's likely that this work involves code modifications by BIOS vendors.

Hi Dongsheng

There are two different options here.

1) Everybody does their own thing, ignoring what everybody else has done, and invents their own wheel. There is no shared code, no shared description, everybody has their own bugs, etc. ACPI as a standard is pointless for Ethernet MDIOs and PHYs because it is not a standard, everybody does something different.

Quoted 7 lines

2) Somebody takes the time to design a concept for Ethernet PHYs and MDIO busses using ACPI. They implement the common code, try to modify any existing users if possible, and submit the whole thing to become part of ACPI 6.3.

I would really prefer we go the second route here. It is more initial effort, but in the long run, everybody benefits.

Yes, I also would like the second one. I will reply in a few days.

Cheers,
Dongsheng

Quoted 2 lines

Andrew

#24 lore raw

cover acpi: Add acpi mdio support code

In reply to Andrew Lunn

Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400 platform. After some discussion and following Andrew's advice, I send out with a generic version of ACPI.

Current there is no clear documentation about MDIO/PHY for ACPI, so when I reading some documents about ACPI [1], I think we just need to reuse the DT binding in the ACPI.[2]. However, this series of patches are not fully compatible with all contents specified in DT binding.

The most important thing about this iseries is link the phy device and fwnode of acpi. Besides, we need to carry out bus scan at the mdio register. Therefore, I am not compatible with more DT binding properties in this series of patches. More support will be in the follow-up patches support, or some people do the support.

Example:
Based on ACPI doc:
Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Tested: QDF2400 (ACPI), buildin/insmod/rmmod

[1]:
Documentation/acpi/dsd/data-node-references.txt
Documentation/acpi/dsd/graph.txt

[2]:
Documentation/devicetree/bindings/phy/phy-bindings.txt
https://lore.kernel.org/patchwork/patch/597296/

Wang Dongsheng (3):
acpi: Add acpi mdio support code
net: qcom/emac: split phy_config to mdio bus create and get phy device
net: qcom/emac: add phy-handle support for ACPI

 drivers/acpi/Kconfig                          |   6 +
 drivers/acpi/Makefile                         |   1 +
 drivers/acpi/acpi_mdio.c                      | 167 ++++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  19 +-
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 142 ++++++++++-----
 drivers/net/phy/mdio_bus.c                    |   3 +
 include/linux/acpi_mdio.h                     |  82 +++++++++
 7 files changed, 369 insertions(+), 51 deletions(-)
 create mode 100644 drivers/acpi/acpi_mdio.c
 create mode 100644 include/linux/acpi_mdio.h
Signature · 1 line
2.18.0
#25 lore raw

RFC PATCH 1/3 acpi: Add acpi mdio support code

Add support for parsing the ACPI data node for PHY devices on an MDIO bus. The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
The DT "phy-handle" binding that we reuse for ACPI is documented in Documentation/devicetree/bindings/phy/phy-bindings.txt

Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt

---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)
drivers/acpi/Kconfig +6 -0
index 9705fc986da9..0fefa3410ce9 100644
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
drivers/acpi/Makefile +1 -0
index 6d59aa109a91..ec7461a064fc 100644
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
drivers/acpi/acpi_mdio.c new file +167 -0

167 lines not on this page · raw message

drivers/net/phy/mdio_bus.c +3 -0
index 2e59a8419b17..d7bca2145d0f 100644
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi_mdio.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/errno.h>
@@ -516,6 +517,8 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 	 * in the bus node, and set the of_node pointer in this case.
 	 */
 	of_mdiobus_link_mdiodev(bus, &phydev->mdio);
+	/* Link the phy device with ACPI phy fwnode. */
+	acpi_mdiobus_link_mdiodev(bus, &phydev->mdio);
 
 	err = phy_device_register(phydev);
 	if (err) {
include/linux/acpi_mdio.h new file +82 -0

82 lines not on this page · raw message

Signature · 1 line
2.18.0
#26 lore raw

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

Quoted 73 lines · CET Wang Dongsheng

Add support for parsing the ACPI data node for PHY devices on an MDIO bus. The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
The DT "phy-handle" binding that we reuse for ACPI is documented in Documentation/devicetree/bindings/phy/phy-bindings.txt

Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt

---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)
drivers/acpi/Kconfig +6 -0
index 9705fc986da9..0fefa3410ce9 100644
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
drivers/acpi/Makefile +1 -0
index 6d59aa109a91..ec7461a064fc 100644
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
drivers/acpi/acpi_mdio.c new file +0 -0
new file mode 100644
index 000000000000..293bf9a63197

drivers/acpi/acpi_mdio.c line 2

Quoted 2 lines
drivers/acpi/acpi_mdio.c new file +2 -0
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Lots of code in this file is copy from drivers/of/of_mdio.c

Would it be possible to re-factor that code and share it instead?

Thanks,
Rafael

#27 lore raw

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

Quoted 69 lines · Rafael J. Wysocki
Quoted 68 lines · CET Wang Dongsheng

Add support for parsing the ACPI data node for PHY devices on an MDIO bus. The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
The DT "phy-handle" binding that we reuse for ACPI is documented in Documentation/devicetree/bindings/phy/phy-bindings.txt

Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt

---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)
drivers/acpi/Kconfig +6 -0
index 9705fc986da9..0fefa3410ce9 100644
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
drivers/acpi/Makefile +1 -0
index 6d59aa109a91..ec7461a064fc 100644
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
drivers/acpi/acpi_mdio.c +0 -0
Quoted 8 lines

drivers/acpi/acpi_mdio.c line 2

Quoted 7 lines

new file mode 100644
index 000000000000..293bf9a63197

drivers/acpi/acpi_mdio.c new file +2 -0
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Lots of code in this file is copy from drivers/of/of_mdio.c

Would it be possible to re-factor that code and share it instead?

I thought about it, we can actually do it with fwnode.

But I don't have a lot of concentrate to do this. I'm going to focus on a few other things...:(

Maybe in the second half of 2019 I can re-factor them if there is no one re-factor them.

Cheers,

Dongsheng

Quoted 4 lines

Thanks,
Rafael

#28 lore raw

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

Quoted 79 lines · Dongsheng
Quoted 69 lines · Rafael J. Wysocki
Quoted 68 lines · CET Wang Dongsheng

Add support for parsing the ACPI data node for PHY devices on an MDIO bus. The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
The DT "phy-handle" binding that we reuse for ACPI is documented in Documentation/devicetree/bindings/phy/phy-bindings.txt

Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt

---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)
drivers/acpi/Kconfig +6 -0
index 9705fc986da9..0fefa3410ce9 100644
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
drivers/acpi/Makefile +1 -0
index 6d59aa109a91..ec7461a064fc 100644
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
drivers/acpi/acpi_mdio.c +0 -0
Quoted 8 lines

drivers/acpi/acpi_mdio.c line 2

Quoted 7 lines

new file mode 100644
index 000000000000..293bf9a63197

drivers/acpi/acpi_mdio.c new file +2 -0
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Lots of code in this file is copy from drivers/of/of_mdio.c

Would it be possible to re-factor that code and share it instead?

Quoted 7 lines

I thought about it, we can actually do it with fwnode.

But I don't have a lot of concentrate to do this. I'm going to focus on a few other things...:(

Maybe in the second half of 2019 I can re-factor them if there is no one re-factor them.

Well, I'd rather avoid code duplication upfront if possible.

Thanks,
Rafael

maintainer
#29 lore raw

Re: [RFC PATCH 1/3] acpi: Add acpi mdio support code

In reply to Wang Dongsheng
Quoted 74 lines · Wang Dongsheng

Add support for parsing the ACPI data node for PHY devices on an MDIO bus. The current implementation depend on mdio bus scan.
With _DSD device properties we can finally do this:

    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })
    }

    Device (MACO) {
        Name (_DSD, Package () {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "phy-handle", \_SB.MDIO, "ethernet-phy@0" }, }
        })
    }

Documentations:
The DT "phy-handle" binding that we reuse for ACPI is documented in Documentation/devicetree/bindings/phy/phy-bindings.txt

Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt

---
 drivers/acpi/Kconfig       |   6 ++
 drivers/acpi/Makefile      |   1 +
 drivers/acpi/acpi_mdio.c   | 167 +++++++++++++++++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c |   3 +
 include/linux/acpi_mdio.h  |  82 ++++++++++++++++++
 5 files changed, 259 insertions(+)
drivers/acpi/Kconfig +6 -0
index 9705fc986da9..0fefa3410ce9 100644
@@ -252,6 +252,12 @@ config ACPI_PROCESSOR_IDLE
 config ACPI_MCFG
 	bool
 
+config ACPI_MDIO
+	def_tristate PHYLIB
+	depends on PHYLIB
+	help
+	  ACPI MDIO bus (Ethernet PHY) accessors
+
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
drivers/acpi/Makefile +1 -0
index 6d59aa109a91..ec7461a064fc 100644
@@ -41,6 +41,7 @@ acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
 obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
+acpi-$(CONFIG_ACPI_MDIO)	+= acpi_mdio.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
drivers/acpi/acpi_mdio.c new file +1 -1
new file mode 100644
index 000000000000..293bf9a63197
--- /dev/null
 
+++ b/drivers/acpi/acpi_mdio.c

drivers/acpi/acpi_mdio.c line 3

Quoted 3 lines
drivers/acpi/acpi_mdio.c new file +3 -0
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Lots of code in this file is copy from drivers/of/of_mdio.c
+// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.

I agree with Rafael here. We should try to refactor and combine this code. And where possible, we should try to add a uniform fwnode_ API which underneath either uses OF or ACPI, depending on the type of the node. We already have fwnode_get_mac_address(), fwmode_irq_get(), fwnode_get_phy_mode(), so where possible, PHYs should be no different.

Andrew

#30 lore raw

RFC PATCH 2/3 net: qcom/emac: split phy_config to mdio bus create and get phy device

In reply to Wang Dongsheng

This patch separate emac_mdio_bus_create and emac_get_phydev from emac_phy_config, and do some codes clean.

---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 96 +++++++++++--------
 1 file changed, 56 insertions(+), 40 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-phy.c +56 -40
index 53dbf1e163a8..8289fdda4be7 100644
@@ -96,15 +96,15 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
-/* Configure the MDIO bus and connect the external PHY */
-int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+static int emac_mdio_bus_create(struct platform_device *pdev,
+				struct emac_adapter *adpt)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct mii_bus *mii_bus;
 	int ret;
 
 	/* Create the mii_bus object for talking to the MDIO bus */
-	adpt->mii_bus = mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
 	if (!mii_bus)
 		return -ENOMEM;
 
@@ -115,50 +115,66 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	if (has_acpi_companion(&pdev->dev)) {
-		u32 phy_addr;
-
-		ret = mdiobus_register(mii_bus);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
-		ret = device_property_read_u32(&pdev->dev, "phy-channel",
-					       &phy_addr);
-		if (ret)
-			/* If we can't read a valid phy address, then assume
-			 * that there is only one phy on this mdio bus.
-			 */
-			adpt->phydev = phy_find_first(mii_bus);
-		else
-			adpt->phydev = mdiobus_get_phy(mii_bus, phy_addr);
-
-		/* of_phy_find_device() claims a reference to the phydev,
-		 * so we do that here manually as well. When the driver
-		 * later unloads, it can unilaterally drop the reference
-		 * without worrying about ACPI vs DT.
-		 */
-		if (adpt->phydev)
-			get_device(&adpt->phydev->mdio.dev);
-	} else {
-		struct device_node *phy_np;
-
-		ret = of_mdiobus_register(mii_bus, np);
-		if (ret) {
-			dev_err(&pdev->dev, "could not register mdio bus\n");
-			return ret;
-		}
+	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
+				  NULL : np);
+	if (ret)
+		dev_err(&pdev->dev, "Could not register mdio bus\n");
+
+	adpt->mii_bus = ret ? NULL : mii_bus;
+	return ret;
+}
+
+static int emac_get_phydev(struct platform_device *pdev,
+			   struct emac_adapter *adpt)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mii_bus *bus = adpt->mii_bus;
+	struct device_node *phy_np;
+	struct phy_device *phydev;
 
+	u32 phy_addr;
+	int ret;
+
+	if (!has_acpi_companion(&pdev->dev)) {
 		phy_np = of_parse_phandle(np, "phy-handle", 0);
 		adpt->phydev = of_phy_find_device(phy_np);
 		of_node_put(phy_np);
+		return adpt->phydev ? 0 : -ENODEV;
 	}
 
-	if (!adpt->phydev) {
-		dev_err(&pdev->dev, "could not find external phy\n");
-		mdiobus_unregister(mii_bus);
+	ret = device_property_read_u32(&pdev->dev, "phy-channel",
+				       &phy_addr);
+	/* If we can't read a valid phy address, then assume
+	 * that there is only one phy on this mdio bus.
+	 */
+	phydev = ret ? phy_find_first(bus) : mdiobus_get_phy(bus, phy_addr);
+	if (!phydev)
 		return -ENODEV;
-	}
 
+	/* of_phy_find_device() claims a reference to the phydev,
+	 * so we do that here manually as well. When the driver
+	 * later unloads, it can unilaterally drop the reference
+	 * without worrying about ACPI vs DT.
+	 */
+	get_device(&phydev->mdio.dev);
+	adpt->phydev = phydev;
 	return 0;
 }
+
+/* Configure the MDIO bus and connect the external PHY */
+int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
+{
+	int ret;
+
+	ret = emac_mdio_bus_create(pdev, adpt);
+	if (ret)
+		return ret;
+
+	ret = emac_get_phydev(pdev, adpt);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not find external phy\n");
+		mdiobus_unregister(adpt->mii_bus);
+	}
+
+	return ret;
+}
Signature · 1 line
2.18.0
#31 lore raw

RFC PATCH 3/3 net: qcom/emac: add phy-handle support for ACPI

In reply to Wang Dongsheng

Use "phy-handle" to point to an internal MDIO device port.

---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 19 ++---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 78 ++++++++++++++-----
 2 files changed, 70 insertions(+), 27 deletions(-)
drivers/net/ethernet/qualcomm/emac/emac-mac.c +10 -9
index 031f6e6ee9c1..74cfe7b95bb3 100644
@@ -13,6 +13,7 @@
 /* Qualcomm Technologies, Inc. EMAC Ethernet Controller MAC layer support
  */
 
+#include <linux/acpi_mdio.h>
 #include <linux/tcp.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
@@ -939,28 +940,28 @@ static void emac_adjust_link(struct net_device *netdev)
 int emac_mac_up(struct emac_adapter *adpt)
 {
 	struct net_device *netdev = adpt->netdev;
-	int ret;
+	struct phy_device *phydev = adpt->phydev;
+	struct fwnode_handle *phy_np = acpi_get_phy_node(phydev);
 
 	emac_mac_rx_tx_ring_reset_all(adpt);
 	emac_mac_config(adpt);
 	emac_mac_rx_descs_refill(adpt, &adpt->rx_q);
 
-	adpt->phydev->irq = PHY_POLL;
-	ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
-				 PHY_INTERFACE_MODE_SGMII);
-	if (ret) {
+	phydev->irq = PHY_POLL;
+	phydev = acpi_phy_connect(netdev, phy_np, emac_adjust_link,
+				  0, PHY_INTERFACE_MODE_SGMII);
+	if (!phydev) {
 		netdev_err(adpt->netdev, "could not connect phy\n");
-		return ret;
+		return -ENODEV;
 	}
 
-	phy_attached_print(adpt->phydev, NULL);
+	phy_attached_print(phydev, NULL);
 
 	/* enable mac irq */
 	writel((u32)~DIS_INT, adpt->base + EMAC_INT_STATUS);
 	writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
 
-	phy_start(adpt->phydev);
-
+	phy_start(phydev);
 	napi_enable(&adpt->rx_q.napi);
 	netif_start_queue(netdev);
 
drivers/net/ethernet/qualcomm/emac/emac-phy.c +60 -18

121 lines not on this page · raw message

Signature · 1 line
2.18.0
maintainer
#32 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

In reply to Wang Dongsheng
Quoted 20 lines · Wang Dongsheng

Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400 platform. After some discussion and following Andrew's advice, I send out with a generic version of ACPI.

Current there is no clear documentation about MDIO/PHY for ACPI, so when I reading some documents about ACPI [1], I think we just need to reuse the DT binding in the ACPI.[2]. However, this series of patches are not fully compatible with all contents specified in DT binding.

The most important thing about this iseries is link the phy device and fwnode of acpi. Besides, we need to carry out bus scan at the mdio register. Therefore, I am not compatible with more DT binding properties in this series of patches. More support will be in the follow-up patches support, or some people do the support.

Example:
Based on ACPI doc:
Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt
With _DSD device properties we can finally do this:

Quoted 9 lines
    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })

I don't know much about ACPI. I do know DT. MDIO busses can have multiple PHYs on them. Is the following valid to list two PHYs?

     Device (MDIO) {
         Name (_DSD, Package () {
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@0", PHY0 }, }
         })
         Name (PHY0, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x0 }, }
         })
         Name (_DSD, Package () {
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@10", PHY1 }, }
         })
         Name (PHY1, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x10 }, }
         })
     }

An MDIO bus can also have more than PHYs on them. There can be Ethernet switches. Broadcom also have some with generic PHY devices on them, and other odd things. That means whatever is on an MDIO bus is a device in the Linux device model. How does that work? Do we need some form Device (PHY) {}?

     Device (MDIO) {
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@0", PHY0 }, }
             })
             Name (PHY0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x0 }, }
             })
         }
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@10", PHY1 }, }
             })
             Name (PHY1, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x10 }, }
             })
         Device (SWITCH) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "switch@11", SWITCH0 }, }
             })
             Name (SWITCH0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x11 }, }
             })
         }
      }

I'm just trying to ensure whatever is defined is flexible enough that we really can later support everything which DT does. We have PHYs on MDIO busses, inside switches, which are on MDIO busses, which are inside Ethernet interfaces, etc.

An MDIO bus is very similar to an i2c bus. How is that described in ACPI? Anything we can learn from that?

Thanks
Andrew

maintainer
#33 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

Quoted 13 lines · Andrew Lunn

I don't know much about ACPI. I do know DT. MDIO busses can have multiple PHYs on them. Is the following valid to list two PHYs?

      Device (MDIO) {
          Name (_DSD, Package () {
              ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
              Package () { Package () { "ethernet-phy@0", PHY0 }, }
          })
          Name (PHY0, Package() {
              ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
              Package () { Package () { "reg", 0x0 }, }
          })
          Name (_DSD, Package () {
Quoted 8 lines
              ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
              Package () { Package () { "ethernet-phy@10", PHY1 }, }
          })
          Name (PHY1, Package() {
              ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
              Package () { Package () { "reg", 0x10 }, }
          })
      }

You can't have the same DSD twice. It would need to look like this:

         Name (PHY1, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0, 0x10 }, }
         })
#34 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

In reply to Andrew Lunn

Hi Andrew,

Quoted 43 lines · Andrew Lunn
Quoted 20 lines · Wang Dongsheng

Originally I just push "phy-handle" support for ACPI on the QCOM QDF2400 platform. After some discussion and following Andrew's advice, I send out with a generic version of ACPI.

Current there is no clear documentation about MDIO/PHY for ACPI, so when I reading some documents about ACPI [1], I think we just need to reuse the DT binding in the ACPI.[2]. However, this series of patches are not fully compatible with all contents specified in DT binding.

The most important thing about this iseries is link the phy device and fwnode of acpi. Besides, we need to carry out bus scan at the mdio register. Therefore, I am not compatible with more DT binding properties in this series of patches. More support will be in the follow-up patches support, or some people do the support.

Example:
Based on ACPI doc:
Documentation/acpi/dsd/data-node-references.txt Documentation/acpi/dsd/graph.txt
With _DSD device properties we can finally do this:

Quoted 9 lines
    Device (MDIO) {
        Name (_DSD, Package () {
            ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () { Package () { "ethernet-phy@0", PHY0 }, }
        })
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () { Package () { "reg", 0x0 }, }
        })

I don't know much about ACPI. I do know DT. MDIO busses can have multiple PHYs on them. Is the following valid to list two PHYs?

     Device (MDIO) {
         Name (_DSD, Package () {
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@0", PHY0 }, }
         })
         Name (PHY0, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x0 }, }
         })
         Name (_DSD, Package () {
Quoted 8 lines
             ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
             Package () { Package () { "ethernet-phy@10", PHY1 }, }
         })
         Name (PHY1, Package() {
             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
             Package () { Package () { "reg", 0x10 }, }
         })
     }

Multiple PHYs example:

    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {

                Package () { "ethernet-phy@0", PHY0 },

                Package () { "ethernet-phy@1", PHY1 },
                ...

                ...

            }
        })
       
        Name (PHY0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x0 },
            }
        })


        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })

}

    Device (MAC0)

    {

        // _DSD: Device-Specific Data
        Name (_DSD, Package (0x02) {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
            Package () {
                Package () { "phy-handle", Package () { \_SB.MDIO,
"ethernet-phy@0" } },
                ...
                ...
            }
        })

    }


    Device (MAC1)

    {

        // _DSD: Device-Specific Data
        Name (_DSD, Package (0x02) {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
            Package () {
                Package () { "phy-handle", Package () { \_SB.MDIO,
"ethernet-phy@1" } },
                ...
                ...
            }
        })

    }
Quoted 6 lines

An MDIO bus can also have more than PHYs on them. There can be Ethernet switches. Broadcom also have some with generic PHY devices on them, and other odd things. That means whatever is on an MDIO bus is a device in the Linux device model. How does that work? Do we need some form Device (PHY) {}?

ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b") describes a ACPI data node.

The data node can contain property or pointer.

Let's look at the table I'm using:

Device (MAC1)

{
    // _DSD: Device-Specific Data
    Name (_DSD, Package (0x02) {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
Properties for _DSD */,
        Package (0x07) {
            Package () { "phy-handle", Package () { \_SB.MAC1.MDIO,
"ethernet-phy@1" } },
            Package () { "dev-refs", \_SB.MAC0 },
            Package () { "refs0-dev", Package () { \_SB.MAC1.MDIO,
"refs@0" } },
            Package () { "refs1-dev", Package () { \_SB.MAC1.MDIO,
"refs@1" } },
            ...
            ...
        }
    })

    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {
                Package () { "ethernet-phy@1", PHY1 },
                Package () { "refs@0",  REF0},
                Package () { "refs@1",  REF1},
            }
        })
        
        //Contain a property
        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })
        
        //Point to a device
        Name (REF0, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "child-refs-dev", \_SB.MAC0 },
            }
        })
        
        //Contain a property and a pointer that point to a device
        Name (REF1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "property", 0x5 },
                Package () { "child-refs-dev", \_SB.MAC0 },
            }
        })
    }
}
Quoted 22 lines
     Device (MDIO) {
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@0", PHY0 }, }
             })
             Name (PHY0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x0 }, }
             })
         }
         Device (PHY) {
             Name (_DSD, Package () {
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "ethernet-phy@10", PHY1 }, }
             })
             Name (PHY1, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x10 }, }
             })
         Device (SWITCH) {
             Name (_DSD, Package () {
Quoted 8 lines
                 ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
                 Package () { Package () { "switch@11", SWITCH0 }, }
             })
             Name (SWITCH0, Package() {
                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                 Package () { Package () { "reg", 0x11 }, }
             })
         }
    Device (MDIO)
    {
        Name (_DSD, Package () {
        ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
            Package () {
                Package () { "ethernet-phy@1", PHY1 },
                Package () { "switch@0",  SW00 },

                Package () { "switch@1",  SW01 },
            }
        })
        
        //Contain a property
        Name (PHY1, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x1 },
            }
        })
 
        Name (SW00, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "reg", 0x0 },
            }
        })

        Name (SW01, Package() {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package () {
                Package () { "property", 0x5 },
            }
        })
    }
Quoted 6 lines
      }

I'm just trying to ensure whatever is defined is flexible enough that we really can later support everything which DT does. We have PHYs on MDIO busses, inside switches, which are on MDIO busses, which are inside Ethernet interfaces, etc.

I think it can be satisfied. See the table I'm using above.

Quoted 2 lines

An MDIO bus is very similar to an i2c bus. How is that described in ACPI? Anything we can learn from that?

About the data node, I just read the Kernel Documentation/acpi/dsd/

And others learn from ACPI spec.

Cheers,

Dongsheng

maintainer
#35 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

Quoted 6 lines
Quoted 4 lines

I'm just trying to ensure whatever is defined is flexible enough that we really can later support everything which DT does. We have PHYs on MDIO busses, inside switches, which are on MDIO busses, which are inside Ethernet interfaces, etc.

I think it can be satisfied. See the table I'm using above.

Hi Dongsheng

Since i don't know anything better, i think i have to trust you have this correct.

It would be good to document this, so that the next person who needs to add ACPI support for a PHY has some documentation to look at. Could you add something to Documentation/acpi/dsd?

Andrew

#36 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

Hello Rafael,

Quoted 8 lines · Andrew Lunn
Quoted 5 lines
Quoted 4 lines

I'm just trying to ensure whatever is defined is flexible enough that we really can later support everything which DT does. We have PHYs on MDIO busses, inside switches, which are on MDIO busses, which are inside Ethernet interfaces, etc.

I think it can be satisfied. See the table I'm using above.

Hi Dongsheng

Quoted 6 lines

Since i don't know anything better, i think i have to trust you have this correct.

It would be good to document this, so that the next person who needs to add ACPI support for a PHY has some documentation to look at. Could you add something to Documentation/acpi/dsd?

How about Andrew suggestion? I agree with Andrew.

Or We need to add the documentation to Documentation/acpi? I see that both GPIO and I2C are down here.

Cheers,
Dongsheng

#37 lore raw

Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

In reply to Andrew Lunn
Quoted 7 lines · Andrew Lunn

I'm just trying to ensure whatever is defined is flexible enough that we really can later support everything which DT does. We have PHYs on MDIO busses, inside switches, which are on MDIO busses, which are inside Ethernet interfaces, etc.

An MDIO bus is very similar to an i2c bus. How is that described in ACPI? Anything we can learn from that?

All the documents I've seen are from UEFII/ACPICA and I have not seen any independent description of MDIO in the ACPI documents. Maybe I missed a document, too.

I think not all devices need to be described in ACPI. The ACPI defines what should be included in a device, as well as some special definitions. Our MDIO
doesn't need a special definition, because it's enough to use a generic ACPI description. I think that's why MDIO doesn't have a separate description.

Cheers,
Dongsheng

#38 lore raw

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

In reply to Timur Tabi
Quoted 8 lines · Timur Tabi
Quoted 2 lines · Andrew Lunn

ACPI is completely separate and should not affect the DT binding. I've not yet looked at the ACPI changes you added.

Just FYI, there is no device tree platform on which the upstream EMAC driver works. All of the DT code in the driver is theoretical. It worked once on a prototype platform, when I originally wrote the code, but since then DT support is mostly a guess.

Quoted 7 lines

The focus of any patches for the EMAC should be ACPI, not DT. If anything, ACPI support should come first. No one should be writing or reviewing DT code before ACPI code.

The upstream EMAC driver is only known to work on the QDF2400, which is an ACPI-only chip. I feel like I've been repeating this too often.

Ok, I just focus on ACPI, and keep DT code no changes.

Cheers,
Dongsheng