[PATCH v2 0/4] net: qcom/emac: add shared mdio bus support

[PATCH v2 0/4] net: qcom/emac: add shared mdio bus support

[Wang Dongsheng]

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(-)

-- 
2.18.0

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

[Wang Dongsheng]

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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 99 +++++++++++--------
 1 file changed, 57 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e163a8..2d16c6b9ef49 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -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;
 }
-- 
2.18.0

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

[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.

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
V2: s/Since QDF2400 emac/Since emac/
---
 Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
index 346e6c7f47b7..50db71771358 100644
--- a/Documentation/devicetree/bindings/net/qcom-emac.txt
+++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
@@ -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>;
-- 
2.18.0

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

[Andrew Lunn]

On Mon, Sep 17, 2018 at 04:53:29PM +0800, Wang Dongsheng wrote:
> 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.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> ---
> V2: s/Since QDF2400 emac/Since emac/
> ---
>  Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
> index 346e6c7f47b7..50db71771358 100644
> --- a/Documentation/devicetree/bindings/net/qcom-emac.txt
> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
> @@ -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

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

[Wang, Dongsheng]

On 9/17/2018 10:50 PM, Andrew Lunn wrote:
> On Mon, Sep 17, 2018 at 04:53:29PM +0800, Wang Dongsheng wrote:
>> 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.
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
>> ---
>> V2: s/Since QDF2400 emac/Since emac/
>> ---
>>  Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
>> index 346e6c7f47b7..50db71771358 100644
>> --- a/Documentation/devicetree/bindings/net/qcom-emac.txt
>> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
>> @@ -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.
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


>         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
>

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

[Florian Fainelli]

On 09/17/2018 09:47 AM, Wang, Dongsheng wrote:
> On 9/17/2018 10:50 PM, Andrew Lunn wrote:
>> On Mon, Sep 17, 2018 at 04:53:29PM +0800, Wang Dongsheng wrote:
>>> 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.
>>>
>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
>>> ---
>>> V2: s/Since QDF2400 emac/Since emac/
>>> ---
>>>  Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> index 346e6c7f47b7..50db71771358 100644
>>> --- a/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>> @@ -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.
> 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.
-- 
Florian

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

[Wang, Dongsheng]

On 2018/9/18 0:54, Florian Fainelli wrote:
> On 09/17/2018 09:47 AM, Wang, Dongsheng wrote:
>> On 9/17/2018 10:50 PM, Andrew Lunn wrote:
>>> On Mon, Sep 17, 2018 at 04:53:29PM +0800, Wang Dongsheng wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
>>>> ---
>>>> V2: s/Since QDF2400 emac/Since emac/
>>>> ---
>>>>  Documentation/devicetree/bindings/net/qcom-emac.txt | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>>> index 346e6c7f47b7..50db71771358 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom-emac.txt
>>>> +++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
>>>> @@ -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.
>> 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.
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>;
        };
    };
};

> 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

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

[Andrew Lunn]

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

> 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

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

[Wang, Dongsheng]

On 2018/9/18 20:35, Andrew Lunn wrote:
>>> 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.
>> 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
> 	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
>

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

[Andrew Lunn]

On Wed, Sep 19, 2018 at 09:19:19AM +0000, Wang, Dongsheng wrote:
> On 2018/9/18 20:35, Andrew Lunn wrote:
> >>> 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.
> >> 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.

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.

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

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

Thanks

	Andrew

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

[Timur Tabi]

On 9/19/18 7:25 AM, Andrew Lunn wrote:
> 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.

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

[Andrew Lunn]

> 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

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

[Timur Tabi]

On 9/19/18 10:20 AM, Andrew Lunn wrote:
> 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.

[PATCH v3 0/2] net: qcom/emac: add shared mdio bus support

[Wang Dongsheng]

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(-)

-- 
2.18.0

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

[Wang Dongsheng]

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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 96 +++++++++++--------
 1 file changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e163a8..f2ed013ce5d5 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -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;
+}
-- 
2.18.0

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

[Wang Dongsheng]

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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 115 +++++++++++++++---
 1 file changed, 100 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index f2ed013ce5d5..3dc3ae55e5bb 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -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;
 }
-- 
2.18.0

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

[Andrew Lunn]

On Thu, Oct 25, 2018 at 06:09:15PM +0800, Wang Dongsheng wrote:
> 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

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

[Wang, Dongsheng]

On 2018/10/26 3:24, Andrew Lunn wrote:
> On Thu, Oct 25, 2018 at 06:09:15PM +0800, Wang Dongsheng wrote:
>> 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.
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

> Thanks
> 	Andrew
>

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

[Timur Tabi]

On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> 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

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

[Wang, Dongsheng]

On 2018/10/26 10:37, Timur Tabi wrote:
> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>> 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

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

[Andrew Lunn]

On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
> On 2018/10/26 10:37, Timur Tabi wrote:
> > On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> >> 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. :(

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

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

[Wang, Dongsheng]

On 2018/10/26 21:12, Andrew Lunn wrote:
> On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
>> On 2018/10/26 10:37, Timur Tabi wrote:
>>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>>>> 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. :(
> 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

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

[Andrew Lunn]

On Mon, Oct 29, 2018 at 02:39:36AM +0000, Wang, Dongsheng wrote:
> On 2018/10/26 21:12, Andrew Lunn wrote:
> > On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
> >> On 2018/10/26 10:37, Timur Tabi wrote:
> >>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
> >>>> 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. :(
> > 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.

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

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

[Wang, Dongsheng]

On 2018/10/29 20:40, Andrew Lunn wrote:
> On Mon, Oct 29, 2018 at 02:39:36AM +0000, Wang, Dongsheng wrote:
>> On 2018/10/26 21:12, Andrew Lunn wrote:
>>> On Fri, Oct 26, 2018 at 03:04:25AM +0000, Wang, Dongsheng wrote:
>>>> On 2018/10/26 10:37, Timur Tabi wrote:
>>>>> On 10/25/18 9:18 PM, Wang, Dongsheng wrote:
>>>>>> 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. :(
>>> 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.
> 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.

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

Cheers,
Dongsheng

> 	Andrew
>

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

[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:
    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

-- 
2.18.0

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

[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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 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(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 9705fc986da9..0fefa3410ce9 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -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
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 6d59aa109a91..ec7461a064fc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -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
diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
new file mode 100644
index 000000000000..293bf9a63197
--- /dev/null
+++ b/drivers/acpi/acpi_mdio.c
@@ -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.
+
+#include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+#include <linux/err.h>
+#include <linux/phy.h>
+#include <linux/phy_fixed.h>
+
+/* Helper function for acpi_phy_find_device */
+static int phy_match(struct device *dev, void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
+/**
+ * acpi_phy_find_device - Give a PHY fwnode, find the phy_device
+ * @fwnode: Pointer to the phy's acpi data node
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *acpi_phy_find_device(struct fwnode_handle *fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!fwnode)
+		return NULL;
+
+	d = bus_find_device(&mdio_bus_type, NULL, fwnode, phy_match);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(acpi_phy_find_device);
+
+static int do_acpi_mdiodev_match(struct fwnode_handle *fwnode,
+				 struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct fwnode_handle *child_node;
+	int addr;
+	int ret;
+
+	fwnode_for_each_child_node(fwnode, child_node) {
+		do {
+			addr = acpi_mdio_parse_addr(dev, child_node);
+			if (addr < 0)
+				break;
+
+			if (mdiodev->addr != addr)
+				break;
+
+			dev->fwnode = child_node;
+			return 0;
+		} while (0);
+
+		/* Walk hierarchical extension data nodes */
+		ret = do_acpi_mdiodev_match(child_node, mdiodev);
+		if (!ret)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
+/* Walk the list of subnodes of a mdio bus and look for a node that
+ * matches the mdio device's address with its 'reg' property. If
+ * found, set the fwnode pointer for the mdio device.
+ */
+void acpi_mdiobus_link_mdiodev(struct mii_bus *bus,
+			       struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+
+	if (dev->fwnode || !bus->dev.fwnode)
+		return;
+
+	if (!has_acpi_companion(&bus->dev))
+		return;
+
+	do_acpi_mdiodev_match(bus->dev.fwnode, mdiodev);
+}
+
+/**
+ * acpi_phy_connect - Connect to the phy
+ * @dev: pointer to net_device claiming the phy
+ * @fwnode: Pointer to ACPI data node for the PHY
+ * @hndlr: Link state callback for the network device
+ * @flags: flags to pass to the PHY
+ * @iface: PHY data interface type
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure. The
+ * refcount must be dropped by calling phy_disconnect() or phy_detach().
+ */
+struct phy_device *acpi_phy_connect(struct net_device *dev,
+				    struct fwnode_handle *fwnode,
+				    void (*hndlr)(struct net_device *),
+				    u32 flags,
+				    phy_interface_t iface)
+{
+	struct phy_device *phy = acpi_phy_find_device(fwnode);
+	int ret;
+
+	if (!phy)
+		return NULL;
+
+	phy->dev_flags = flags;
+
+	ret = phy_connect_direct(dev, phy, hndlr, iface);
+
+	/* refcount is held by phy_connect_direct() on success */
+	put_device(&phy->mdio.dev);
+
+	return ret ? NULL : phy;
+}
+EXPORT_SYMBOL(acpi_phy_connect);
+
+static int acpi_mdio_node_verify(struct fwnode_handle *fwnode)
+{
+	return is_acpi_device_node(fwnode) ? 0 : -ENODEV;
+}
+
+static int fwnode_mdiobus_verify_node(struct fwnode_handle *fwnode)
+{
+	if (!is_acpi_node(fwnode))
+		return -ENODEV;
+	return acpi_mdio_node_verify(fwnode);
+}
+
+/**
+ * acpi_mdiobus_register - Register mii_bus and create PHYs
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fw_node of MDIO bus.
+ *
+ * This function registers the mii_bus structure and scan the phy_devices
+ * for each child node of @fwnode.
+ */
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+	int ret;
+
+	if (!fwnode)
+		return mdiobus_register(mdio);
+
+	ret = fwnode_mdiobus_verify_node(fwnode);
+	if (ret)
+		return ret;
+
+	/* Scan PHYs on MDIO bus */
+	mdio->phy_mask = 0;
+	mdio->dev.fwnode = fwnode;
+
+	/* Register the MDIO bus */
+	return mdiobus_register(mdio);
+}
+EXPORT_SYMBOL(acpi_mdiobus_register);
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2e59a8419b17..d7bca2145d0f 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -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) {
diff --git a/include/linux/acpi_mdio.h b/include/linux/acpi_mdio.h
new file mode 100644
index 000000000000..1a4a30258ebc
--- /dev/null
+++ b/include/linux/acpi_mdio.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+// Copyright (c) 2018 Huaxintong Semiconductor Technology Co., Ltd.
+
+#ifndef __LINUX_ACPI_MDIO_H
+#define __LINUX_ACPI_MDIO_H
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/phy.h>
+#include <linux/property.h>
+
+#if IS_ENABLED(CONFIG_ACPI_MDIO)
+static inline int acpi_mdio_parse_addr(struct device *dev,
+				       const struct fwnode_handle *fwnode)
+{
+	u32 addr;
+
+	if (!is_acpi_data_node(fwnode))
+		return -ENODEV;
+
+	if (!fwnode_property_present(fwnode, "reg"))
+		return -ENODEV;
+
+	if (fwnode_property_read_u32(fwnode, "reg", &addr)) {
+		dev_err(dev, "Invalid PHY address\n");
+		return -ENODEV;
+	}
+
+	/* A PHY must have a reg property in the range [0-31] */
+	if (addr >= PHY_MAX_ADDR) {
+		dev_err(dev, "PHY address %i is too large\n", addr);
+		return -EINVAL;
+	}
+
+	return addr;
+}
+
+struct phy_device *acpi_phy_find_device(struct fwnode_handle *fwnode);
+struct phy_device *acpi_phy_connect(struct net_device *dev,
+				    struct fwnode_handle *fwnode,
+				    void (*hndlr)(struct net_device *),
+				    u32 flags, phy_interface_t iface);
+int acpi_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode);
+void acpi_mdiobus_link_mdiodev(struct mii_bus *bus,
+			       struct mdio_device *mdiodev);
+#else
+static inline int acpi_mdio_parse_addr(struct device *dev,
+				       const struct fwnode_handle *fwnode)
+{
+	return -EINVAL;
+}
+
+static inline struct phy_device *
+acpi_phy_find_device(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *
+acpi_phy_connect(struct net_device *dev, struct fwnode_handle *fwnode,
+		 void (*hndlr)(struct net_device *), u32 flags,
+		 phy_interface_t iface)
+{
+	return NULL;
+}
+
+static inline int acpi_mdiobus_register(struct mii_bus *mdio,
+					struct fwnode_handle *fwnode)
+{
+	return -ENODEV;
+}
+
+static inline void
+acpi_mdiobus_link_mdiodev(struct mii_bus *bus, struct mdio_device *mdiodev) { }
+#endif
+
+static inline struct fwnode_handle *acpi_get_phy_node(struct phy_device *phydev)
+{
+	return !phydev ? NULL : phydev->mdio.dev.fwnode;
+}
+
+#endif /* __LINUX_ACPI_MDIO_H */
-- 
2.18.0

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

[Rafael J. Wysocki]

On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
> 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
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> ---
>  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(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 9705fc986da9..0fefa3410ce9 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -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
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6d59aa109a91..ec7461a064fc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -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
> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> new file mode 100644
> index 000000000000..293bf9a63197
> --- /dev/null
> +++ b/drivers/acpi/acpi_mdio.c
> @@ -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

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

[Wang, Dongsheng]

On 2018/11/8 15:44, Rafael J. Wysocki wrote:
> On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
>> 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
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
>> ---
>>  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(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 9705fc986da9..0fefa3410ce9 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -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
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 6d59aa109a91..ec7461a064fc 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -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
>> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
>> new file mode 100644
>> index 000000000000..293bf9a63197
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_mdio.c
>> @@ -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


> Thanks,
> Rafael
>
>

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

[Rafael J. Wysocki]

On Thursday, November 8, 2018 8:55:47 AM CET Wang, Dongsheng wrote:
> On 2018/11/8 15:44, Rafael J. Wysocki wrote:
> > On Thursday, November 8, 2018 8:22:16 AM CET Wang Dongsheng wrote:
> >> 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
> >>
> >> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> >> ---
> >>  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(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 9705fc986da9..0fefa3410ce9 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -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
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 6d59aa109a91..ec7461a064fc 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -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
> >> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> >> new file mode 100644
> >> index 000000000000..293bf9a63197
> >> --- /dev/null
> >> +++ b/drivers/acpi/acpi_mdio.c
> >> @@ -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.

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

Thanks,
Rafael

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

[Andrew Lunn]

On Thu, Nov 08, 2018 at 03:22:16PM +0800, Wang Dongsheng wrote:
> 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
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
> ---
>  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(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 9705fc986da9..0fefa3410ce9 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -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
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 6d59aa109a91..ec7461a064fc 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -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
> diff --git a/drivers/acpi/acpi_mdio.c b/drivers/acpi/acpi_mdio.c
> new file mode 100644
> index 000000000000..293bf9a63197
> --- /dev/null

> +++ b/drivers/acpi/acpi_mdio.c
> @@ -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

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

[Wang Dongsheng]

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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 96 +++++++++++--------
 1 file changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e163a8..8289fdda4be7 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -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;
+}
-- 
2.18.0

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

[Wang Dongsheng]

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

Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
---
 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(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 031f6e6ee9c1..74cfe7b95bb3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -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);
 
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 8289fdda4be7..6616014292b0 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -17,6 +17,7 @@
 #include <linux/phy.h>
 #include <linux/iopoll.h>
 #include <linux/acpi.h>
+#include <linux/acpi_mdio.h>
 #include "emac.h"
 
 /* EMAC base register offsets */
@@ -96,10 +97,60 @@ static int emac_mdio_write(struct mii_bus *bus, int addr, int regnum, u16 val)
 	return 0;
 }
 
+static struct phy_device *
+emac_acpi_get_phydev_from_phy_handle(struct platform_device *pdev)
+{
+	struct fwnode_reference_args args = {0};
+	struct fwnode_handle *fw_node;
+	struct phy_device *phydev;
+	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 (ret)
+		return ERR_PTR(-ENODEV);
+
+	if (!is_acpi_data_node(args.fwnode))
+		return ERR_PTR(-ENODEV);
+
+	phydev = acpi_phy_find_device(args.fwnode);
+	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 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)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct fwnode_handle *fwnode = pdev->dev.fwnode;
 	struct mii_bus *mii_bus;
 	int ret;
 
@@ -115,8 +166,8 @@ static int emac_mdio_bus_create(struct platform_device *pdev,
 	mii_bus->parent = &pdev->dev;
 	mii_bus->priv = adpt;
 
-	ret = of_mdiobus_register(mii_bus, has_acpi_companion(&pdev->dev) ?
-				  NULL : np);
+	ret = is_of_node(fwnode) ? of_mdiobus_register(mii_bus, np) :
+				   acpi_mdiobus_register(mii_bus, fwnode);
 	if (ret)
 		dev_err(&pdev->dev, "Could not register mdio bus\n");
 
@@ -128,13 +179,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 +189,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 +213,10 @@ int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt)
 		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);
-	}
+	if (!ret)
+		return 0;
 
+	dev_err(&pdev->dev, "Could not find external phy\n");
+	mdiobus_unregister(adpt->mii_bus);
 	return ret;
 }
-- 
2.18.0

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

[Andrew Lunn]

On Thu, Nov 08, 2018 at 03:21:29PM +0800, Wang Dongsheng wrote:
> 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 }, }
>         })

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

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

[Timur Tabi]

On 11/8/18 5:23 PM, Andrew Lunn wrote:
> 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 }, }
>           })
>       }

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 }, }
          })

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

[Wang, Dongsheng]

Hi Andrew,

On 2018/11/9 7:23, Andrew Lunn wrote:
> On Thu, Nov 08, 2018 at 03:21:29PM +0800, Wang Dongsheng wrote:
>> 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 }, }
>>         })
> 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 }, }
>          })
>      }

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" } },
                ...
                ...
            }
        })

    }

>
> 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 },
            }
        })
    }
}


>      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 }, }
>              })
>          }

    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 },
            }
        })
    }


>       }
>
> 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.


> 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

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

[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.
> 
> 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

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

[Wang, Dongsheng]

Hello Rafael,

On 2018/11/13 1:19, Andrew Lunn wrote:
>>> 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?

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

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

[Wang, Dongsheng]

On 2018/11/9 7:23, Andrew Lunn wrote:
> 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

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

[Wang, Dongsheng]

On 2018/9/19 22:15, Timur Tabi wrote:
> On 9/19/18 7:25 AM, Andrew Lunn wrote:
>> 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.
>
Ok, I just focus on ACPI, and keep DT code no changes.

Cheers,
Dongsheng