by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

> With our hardware, and likely Rockchip's as well, the muxed connections
> include the MDIO and MII connections

Ah, i did not realise the MII was muxed as well. Then i agree, an MDIO
mux is wrong.

However, please try to make the binding not look like an mdio mux. We
don't want people misunderstanding the binding and thinking it is an
mdio mux.

Andrew

2017-08-21 13:31:18

On Tue, Aug 22, 2017 at 11:39:22PM +0800, Chen-Yu Tsai wrote:
> Now can we please decide on something? We're a week and a half from
> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> nodes (internal-mdio & external-mdio).

I can only emphasize this. I'm afraid that if we don't have an
agreement and a patch implementing it with the proper acks by the end
of the week, we'll have to revert all the DT bits and the bindings.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (552.00 B)
signature.asc (801.00 B)
Download all attachments

2017-08-22 18:11:52

by Corentin Labbe

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
> > On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <[email protected]> wrote:
> >>> All muxes are mostly always represented the same way afaik, or do you
> >>> want to simply introduce a new compatible / property?
> >>
> >> + mdio-mux {
> >> + compatible = "allwinner,sun8i-h3-mdio-switch";
> >> + mdio-parent-bus = <&mdio_parent>;
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + internal_mdio: mdio@1 {
> >> reg = <1>;
> >> - clocks = <&ccu CLK_BUS_EPHY>;
> >> - resets = <&ccu RST_BUS_EPHY>;
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + int_mii_phy: ethernet-phy@1 {
> >> + compatible = "ethernet-phy-ieee802.3-c22";
> >> + reg = <1>;
> >> + clocks = <&ccu CLK_BUS_EPHY>;
> >> + resets = <&ccu RST_BUS_EPHY>;
> >> + phy-is-integrated;
> >> + };
> >> + };
> >> + mdio: mdio@0 {
> >> + reg = <0>;
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> };
> >>
> >> Hi Maxim
> >>
> >> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> >> time mux. You swap the mux per MDIO transaction. You can access all
> >> the PHY and switches on the mux'ed MDIO bus.
> >>
> >> However here, it is effectively a boot-time MUX. You cannot change it
> >> on the fly. What happens when somebody has a phandle to a PHY on the
> >> internal and a phandle to a phy on the external? Does the driver at
> >> least return -EINVAL, or -EBUSY? Is there a representation which
> >> eliminates this possibility?
> >
> > There is only one controller. Either you use the internal PHY, which
> > is then directly coupled (no magnetics needed) to the RJ45 port, or
> > you use an external PHY over MII/RMII/RGMII. You could supposedly
> > have both on a board, and let the user choose one. But why bother
> > with the extra complexity and cost? Either you use the internal PHY
> > at 100M, or an external RGMII PHY for gigabit speeds.
>
> I agree, there is no point in over-engineering any of this. I don't
> think there is actually any MDIO mux per-se in that the MDIO clock and
> data lines are muxed, however there has to be some kind of built-in port
> multiplexer that lets you chose between connecting to the internal PHY
> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
>
> >
> > So I think what you are saying is either impossible or engineering-wise
> > a very stupid design, like using an external MAC with a discrete PHY
> > connected to the internal MAC's MDIO bus, while using the internal MAC
> > with the internal PHY.
> >
> > Now can we please decide on something? We're a week and a half from
> > the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> > nodes (internal-mdio & external-mdio).
>
> I really don't see a need for a mdio-mux in the first place, just have
> one MDIO controller (current state) sub-node which describes the
> built-in STMMAC MDIO controller and declare the internal PHY as a child
> node (along with 'phy-is-integrated'). If a different configuration is
> used, then just put the external PHY as a child node there.
>
> If fixed-link is required, the mdio node becomes unused anyway.
>
> Works for everyone?

If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.

Does two MDIO node "internal-mdio" and "mdio" works for you ?
(We keep "mdio" for external MDIO for reducing the number of patchs)

Thanks
Regards

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 4b599b5d26f6..d5e7cf0d9454 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -417,7 +417,8 @@
#size-cells = <0>;
status = "disabled";

- mdio: mdio {
+ /* Only one MDIO is usable at the time */
+ internal_mdio: mdio@1 {
#address-cells = <1>;
#size-cells = <0>;
int_mii_phy: ethernet-phy@1 {
@@ -425,8 +426,13 @@
reg = <1>;
clocks = <&ccu CLK_BUS_EPHY>;
resets = <&ccu RST_BUS_EPHY>;
+ phy-is-integrated;
};
};
+ mdio: mdio@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
};

spi0: spi@01c68000 {

2017-08-22 18:35:12

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

On 08/22/2017 11:11 AM, Corentin Labbe wrote:
> On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
>> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
>>> On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <[email protected]> wrote:
>>>>> All muxes are mostly always represented the same way afaik, or do you
>>>>> want to simply introduce a new compatible / property?
>>>>
>>>> + mdio-mux {
>>>> + compatible = "allwinner,sun8i-h3-mdio-switch";
>>>> + mdio-parent-bus = <&mdio_parent>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> +
>>>> + internal_mdio: mdio@1 {
>>>> reg = <1>;
>>>> - clocks = <&ccu CLK_BUS_EPHY>;
>>>> - resets = <&ccu RST_BUS_EPHY>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + int_mii_phy: ethernet-phy@1 {
>>>> + compatible = "ethernet-phy-ieee802.3-c22";
>>>> + reg = <1>;
>>>> + clocks = <&ccu CLK_BUS_EPHY>;
>>>> + resets = <&ccu RST_BUS_EPHY>;
>>>> + phy-is-integrated;
>>>> + };
>>>> + };
>>>> + mdio: mdio@0 {
>>>> + reg = <0>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> };
>>>>
>>>> Hi Maxim
>>>>
>>>> Anybody who knows the MDIO-mux code/binding, knows that it is a run
>>>> time mux. You swap the mux per MDIO transaction. You can access all
>>>> the PHY and switches on the mux'ed MDIO bus.
>>>>
>>>> However here, it is effectively a boot-time MUX. You cannot change it
>>>> on the fly. What happens when somebody has a phandle to a PHY on the
>>>> internal and a phandle to a phy on the external? Does the driver at
>>>> least return -EINVAL, or -EBUSY? Is there a representation which
>>>> eliminates this possibility?
>>>
>>> There is only one controller. Either you use the internal PHY, which
>>> is then directly coupled (no magnetics needed) to the RJ45 port, or
>>> you use an external PHY over MII/RMII/RGMII. You could supposedly
>>> have both on a board, and let the user choose one. But why bother
>>> with the extra complexity and cost? Either you use the internal PHY
>>> at 100M, or an external RGMII PHY for gigabit speeds.
>>
>> I agree, there is no point in over-engineering any of this. I don't
>> think there is actually any MDIO mux per-se in that the MDIO clock and
>> data lines are muxed, however there has to be some kind of built-in port
>> multiplexer that lets you chose between connecting to the internal PHY
>> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
>>
>>>
>>> So I think what you are saying is either impossible or engineering-wise
>>> a very stupid design, like using an external MAC with a discrete PHY
>>> connected to the internal MAC's MDIO bus, while using the internal MAC
>>> with the internal PHY.
>>>
>>> Now can we please decide on something? We're a week and a half from
>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
>>> nodes (internal-mdio & external-mdio).
>>
>> I really don't see a need for a mdio-mux in the first place, just have
>> one MDIO controller (current state) sub-node which describes the
>> built-in STMMAC MDIO controller and declare the internal PHY as a child
>> node (along with 'phy-is-integrated'). If a different configuration is
>> used, then just put the external PHY as a child node there.
>>
>> If fixed-link is required, the mdio node becomes unused anyway.
>>
>> Works for everyone?
>
> If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.

Then have the .dtsi file contain just the mdio node, but no internal or
external PHY and push all the internal and external PHY node definition
(in its entirety) to the per-board DTS file, does not that work?

>
> Does two MDIO node "internal-mdio" and "mdio" works for you ?
> (We keep "mdio" for external MDIO for reducing the number of patchs)

How does that solve the problem and not create a new one where both MDIO
nodes end-up being registered? Does that mean you force the writer of
the board-level DTS to systematically disable the internal MDIO node
when using an external PHY and vice versa? How is that better than not
being explicit like I suggested earlier?

>
> Thanks
> Regards
>
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index 4b599b5d26f6..d5e7cf0d9454 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -417,7 +417,8 @@
> #size-cells = <0>;
> status = "disabled";
>
> - mdio: mdio {
> + /* Only one MDIO is usable at the time */
> + internal_mdio: mdio@1 {
> #address-cells = <1>;
> #size-cells = <0>;
> int_mii_phy: ethernet-phy@1 {
> @@ -425,8 +426,13 @@
> reg = <1>;
> clocks = <&ccu CLK_BUS_EPHY>;
> resets = <&ccu RST_BUS_EPHY>;
> + phy-is-integrated;
> };
> };
> + mdio: mdio@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> };
>
> spi0: spi@01c68000 {
>

--
Florian

2017-08-22 19:37:26

by Corentin Labbe

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> On 08/22/2017 11:11 AM, Corentin Labbe wrote:
> > On Tue, Aug 22, 2017 at 09:40:24AM -0700, Florian Fainelli wrote:
> >> On 08/22/2017 08:39 AM, Chen-Yu Tsai wrote:
> >>> On Mon, Aug 21, 2017 at 10:23 PM, Andrew Lunn <[email protected]> wrote:
> >>>>> All muxes are mostly always represented the same way afaik, or do you
> >>>>> want to simply introduce a new compatible / property?
> >>>>
> >>>> + mdio-mux {
> >>>> + compatible = "allwinner,sun8i-h3-mdio-switch";
> >>>> + mdio-parent-bus = <&mdio_parent>;
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> +
> >>>> + internal_mdio: mdio@1 {
> >>>> reg = <1>;
> >>>> - clocks = <&ccu CLK_BUS_EPHY>;
> >>>> - resets = <&ccu RST_BUS_EPHY>;
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> + int_mii_phy: ethernet-phy@1 {
> >>>> + compatible = "ethernet-phy-ieee802.3-c22";
> >>>> + reg = <1>;
> >>>> + clocks = <&ccu CLK_BUS_EPHY>;
> >>>> + resets = <&ccu RST_BUS_EPHY>;
> >>>> + phy-is-integrated;
> >>>> + };
> >>>> + };
> >>>> + mdio: mdio@0 {
> >>>> + reg = <0>;
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> };
> >>>>
> >>>> Hi Maxim
> >>>>
> >>>> Anybody who knows the MDIO-mux code/binding, knows that it is a run
> >>>> time mux. You swap the mux per MDIO transaction. You can access all
> >>>> the PHY and switches on the mux'ed MDIO bus.
> >>>>
> >>>> However here, it is effectively a boot-time MUX. You cannot change it
> >>>> on the fly. What happens when somebody has a phandle to a PHY on the
> >>>> internal and a phandle to a phy on the external? Does the driver at
> >>>> least return -EINVAL, or -EBUSY? Is there a representation which
> >>>> eliminates this possibility?
> >>>
> >>> There is only one controller. Either you use the internal PHY, which
> >>> is then directly coupled (no magnetics needed) to the RJ45 port, or
> >>> you use an external PHY over MII/RMII/RGMII. You could supposedly
> >>> have both on a board, and let the user choose one. But why bother
> >>> with the extra complexity and cost? Either you use the internal PHY
> >>> at 100M, or an external RGMII PHY for gigabit speeds.
> >>
> >> I agree, there is no point in over-engineering any of this. I don't
> >> think there is actually any MDIO mux per-se in that the MDIO clock and
> >> data lines are muxed, however there has to be some kind of built-in port
> >> multiplexer that lets you chose between connecting to the internal PHY
> >> and any external PHY/MAC, but that is not what a "mdio-mux" node represents.
> >>
> >>>
> >>> So I think what you are saying is either impossible or engineering-wise
> >>> a very stupid design, like using an external MAC with a discrete PHY
> >>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>> with the internal PHY.
> >>>
> >>> Now can we please decide on something? We're a week and a half from
> >>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>> nodes (internal-mdio & external-mdio).
> >>
> >> I really don't see a need for a mdio-mux in the first place, just have
> >> one MDIO controller (current state) sub-node which describes the
> >> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >> node (along with 'phy-is-integrated'). If a different configuration is
> >> used, then just put the external PHY as a child node there.
> >>
> >> If fixed-link is required, the mdio node becomes unused anyway.
> >>
> >> Works for everyone?
> >
> > If we put an external PHY with reg=1 as a child of internal MDIO, il will be merged with internal PHY node and get phy-is-integrated.
>
> Then have the .dtsi file contain just the mdio node, but no internal or
> external PHY and push all the internal and external PHY node definition
> (in its entirety) to the per-board DTS file, does not that work?
>

It is near what I sent in v2 of this serie. (only one MDIO with internal PHY, but phy-is-integrated is only set per-board DTS)
But at that time Rob said to use a mdio-mux.

> >
> > Does two MDIO node "internal-mdio" and "mdio" works for you ?
> > (We keep "mdio" for external MDIO for reducing the number of patchs)
>
> How does that solve the problem and not create a new one where both MDIO
> nodes end-up being registered? Does that mean you force the writer of
> the board-level DTS to systematically disable the internal MDIO node
> when using an external PHY and vice versa? How is that better than not
> being explicit like I suggested earlier?
>

Only one node is registered.
stmmac register only MDIO called "mdio".
So it is why I have added a patch "register parent MDIO node for sun8i-h3-emac" which for H3 only register the PHY's parent MDIO

But yes back to your solution "only one mdio with internal PHY," and phy-is-integrated only set on board DT which use internal PHY will work.

Regards

2017-08-23 07:49:55

[permalink] [raw]

Subject: Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

> > Just tested. Yes the MDIO/MDC lines are also muxed and controlled through
> > the same mux bit.
>
> Alright then the mdio-mux seems appropriate, thanks.

Please add a comment that it muxes the MII lines as well. That is not
normal for an mdio-mux, so we should document it in the dtsi file.

Thanks
Andrew