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

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