Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus
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
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
Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus
> 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
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
On Wed, Sep 19, 2018 at 09:19:19AM +, 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@feb2 {
> >>> 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
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@feb2 {
>>> 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,
> _link_change, 0,
> phy_interface);
>
> Andrew
>
Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus
> > If you want to describe the MDIO controller, then you embed a mdio
> > subnode into your Ethernet MAC node:
> >
> > emac0: ethernet@feb2 {
> > 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,
_link_change, 0,
phy_interface);
Andrew
Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus
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
---
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@feb2 {
> 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@feb2 {
phy-handle = <>;
mdio {
#address-cells = <1>;
#size-cells = <0>;
phy0: ethernet-phy@0 {
reg = <0>;
};
phy1: ethernet-phy@1 {
reg = <1>;
};
};
};
emac1: ethernet@fexx {
phy-handle = <>;
};
emac2: ethernet@fexx {
phy-handle = <>;
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
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
>>> ---
>>> 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@feb2 {
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
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
>> ---
>> 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" = , MAC1 will get MAC0's MDIO bus
and also get the corresponding PHY device.
(DATA)
MAC0 ---PHY0
| |
| (MDIO) |
|
MAC1PHY0
| |
| (DATA) |
Cheers,
Dongsheng
> emac0: ethernet@feb2 {
> compatible = "qcom,fsm9900-emac";
> reg = <0xfeb2 0x1>,
> <0xfeb36000 0x1000>;
> interrupts = <76>;
>
> clocks = < 0>, < 1>, < 3>, < 4>, < 5>,
> < 6>, < 7>;
> clock-names = "axi_clk", "cfg_ahb_clk", "high_speed_clk",
> "mdio_clk", "tx_clk", "rx_clk", "sys_clk";
>
> internal-phy = <_sgmii>;
>
> phy-handle = <>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> phy0: ethernet-phy@0 {
> reg = <0>;
> };
>
> phy1: ethernet-phy@1 {
> reg = <1>;
> };
>
> pinctrl-names = "default";
> pinctrl-0 = <_pins_a>;
> };
>
> emac1: ethernet@3890 {
> compatible = "qcom,fsm9900-emac";
> ...
> ...
>
> phy-handle = <>;
> };
>
> Andrew
>
Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus
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
> ---
> 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@feb2 {
compatible = "qcom,fsm9900-emac";
reg = <0xfeb2 0x1>,
<0xfeb36000 0x1000>;
interrupts = <76>;
clocks = < 0>, < 1>, < 3>, < 4>, < 5>,
< 6>, < 7>;
clock-names = "axi_clk", "cfg_ahb_clk", "high_speed_clk",
"mdio_clk", "tx_clk", "rx_clk", "sys_clk";
internal-phy = <_sgmii>;
phy-handle = <>;
#address-cells = <1>;
#size-cells = <0>;
phy0: ethernet-phy@0 {
reg = <0>;
};
phy1: ethernet-phy@1 {
reg = <1>;
};
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
};
emac1: ethernet@3890 {
compatible = "qcom,fsm9900-emac";
...
...
phy-handle = <>;
};
Andrew
