| Message ID | 20170822181140.GA11596@Red (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <andrew@lunn.ch> 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 { >
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 <andrew@lunn.ch> 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
Hi Florian, On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: > >>> 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? If possible, I'd really like to have the internal PHY in the DTSI. It's always there in hardware anyway, and duplicating the PHY, with its clock, reset line, and whatever info we might need in the future in each and every board DTS that uses it will be very error prone and we will have the usual bunch of issues that come up with duplication. Maxime
On 08/23/2017 12:49 AM, Maxime Ripard wrote: > Hi Florian, > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>> 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? > > If possible, I'd really like to have the internal PHY in the > DTSI. It's always there in hardware anyway, and duplicating the PHY, > with its clock, reset line, and whatever info we might need in the > future in each and every board DTS that uses it will be very error > prone and we will have the usual bunch of issues that come up with > duplication. OK, then what if you put the internal PHY in the DTSI, mark it with a status = "disabled" property, and have the per-board DTS put a status = "okay" property along with a "phy-is-integrated" boolean property? Would that work? What I really don't think is necessary is: - duplicating the "mdio" controller node for external vs. internal PHY, because this is not accurate, there is just one MDIO controller, but there may be different kinds of MDIO/PHY devices attached - having the STMMAC driver MDIO probing code having to deal with a "mdio" sub-node or an "internal-mdio" sub-node because this is confusing and requiring more driver-level changes that are error prone Thanks
On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: > On 08/23/2017 12:49 AM, Maxime Ripard wrote: > > Hi Florian, > > > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: > >>>>> 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? > > > > If possible, I'd really like to have the internal PHY in the > > DTSI. It's always there in hardware anyway, and duplicating the PHY, > > with its clock, reset line, and whatever info we might need in the > > future in each and every board DTS that uses it will be very error > > prone and we will have the usual bunch of issues that come up with > > duplication. > > OK, then what if you put the internal PHY in the DTSI, mark it with a > status = "disabled" property, and have the per-board DTS put a status = > "okay" property along with a "phy-is-integrated" boolean property? Would > that work? Yeah, that would work for me. > What I really don't think is necessary is: > > - duplicating the "mdio" controller node for external vs. internal PHY, > because this is not accurate, there is just one MDIO controller, but > there may be different kinds of MDIO/PHY devices attached Agreed. > - having the STMMAC driver MDIO probing code having to deal with a > "mdio" sub-node or an "internal-mdio" sub-node because this is confusing > and requiring more driver-level changes that are error prone I don't really have an opinion on that one, so I'll defer to your judgment of what's best :) I guess we have an agreement. Andrew, is that ok for you too? Maxime
On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: > On 08/23/2017 12:49 AM, Maxime Ripard wrote: > > Hi Florian, > > > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: > >>>>> 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? > > > > If possible, I'd really like to have the internal PHY in the > > DTSI. It's always there in hardware anyway, and duplicating the PHY, > > with its clock, reset line, and whatever info we might need in the > > future in each and every board DTS that uses it will be very error > > prone and we will have the usual bunch of issues that come up with > > duplication. > > OK, then what if you put the internal PHY in the DTSI, mark it with a > status = "disabled" property, and have the per-board DTS put a status = > "okay" property along with a "phy-is-integrated" boolean property? Would > that work? No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. So that adding a 'status = "disabled"' does not bring anything. > > What I really don't think is necessary is: > > - duplicating the "mdio" controller node for external vs. internal PHY, > because this is not accurate, there is just one MDIO controller, but > there may be different kinds of MDIO/PHY devices attached For me, if we want to represent the reality, we need two MDIO: - since two PHY at the same address could co-exists - since they are isolated so not on the same MDIO bus > - having the STMMAC driver MDIO probing code having to deal with a > "mdio" sub-node or an "internal-mdio" sub-node because this is confusing > and requiring more driver-level changes that are error prone My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") have to be changed to something like "register_parent_mdio" So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. Really having two MDIO seems cleaner. Regards
On Thu, Aug 24, 2017 at 10:21:24AM +0200, Corentin Labbe wrote: > On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: > > On 08/23/2017 12:49 AM, Maxime Ripard wrote: > > > Hi Florian, > > > > > > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: > > >>>>> 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? > > > > > > If possible, I'd really like to have the internal PHY in the > > > DTSI. It's always there in hardware anyway, and duplicating the PHY, > > > with its clock, reset line, and whatever info we might need in the > > > future in each and every board DTS that uses it will be very error > > > prone and we will have the usual bunch of issues that come up with > > > duplication. > > > > OK, then what if you put the internal PHY in the DTSI, mark it with a > > status = "disabled" property, and have the per-board DTS put a status = > > "okay" property along with a "phy-is-integrated" boolean property? Would > > that work? > > No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. > So that adding a 'status = "disabled"' does not bring anything. > > > > > What I really don't think is necessary is: > > > > - duplicating the "mdio" controller node for external vs. internal PHY, > > because this is not accurate, there is just one MDIO controller, but > > there may be different kinds of MDIO/PHY devices attached > > For me, if we want to represent the reality, we need two MDIO: > - since two PHY at the same address could co-exists > - since they are isolated so not on the same MDIO bus > > > - having the STMMAC driver MDIO probing code having to deal with a > > "mdio" sub-node or an "internal-mdio" sub-node because this is confusing > > and requiring more driver-level changes that are error prone > > My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") > have to be changed to something like "register_parent_mdio" > > > So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. > Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. > Really having two MDIO seems cleaner. > Hello I have speaked with Neil Amstrong, and he said that they get the same problem on amlogic. They use a mdio-mux-mmioreg, (see eth-phy-mux in arch/arm64/boot/dts/amlogic/meson-gxl.dtsi) So tomorow, i will send a patch series which do the same with the exception that we need a mdio-mux-syscon (which is easy/simple to do). Since their setup use stmmac, it means that we will need 0 changes on stmmac. Regards
On 08/24/2017 01:21 AM, Corentin Labbe wrote: > On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: >> On 08/23/2017 12:49 AM, Maxime Ripard wrote: >>> Hi Florian, >>> >>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>>>> 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? >>> >>> If possible, I'd really like to have the internal PHY in the >>> DTSI. It's always there in hardware anyway, and duplicating the PHY, >>> with its clock, reset line, and whatever info we might need in the >>> future in each and every board DTS that uses it will be very error >>> prone and we will have the usual bunch of issues that come up with >>> duplication. >> >> OK, then what if you put the internal PHY in the DTSI, mark it with a >> status = "disabled" property, and have the per-board DTS put a status = >> "okay" property along with a "phy-is-integrated" boolean property? Would >> that work? > > No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. Is not there is a mistake in the unit address not matching the "reg" property, or am I not looking at the right tree? &mdio { ext_rgmii_phy: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <0>; }; }; If the PHY is really at MDIO address 0, then it should be ethernet-phy@0, and not ethernet-phy@1, and then no problem with the merging? > So that adding a 'status = "disabled"' does not bring anything. > >> >> What I really don't think is necessary is: >> >> - duplicating the "mdio" controller node for external vs. internal PHY, >> because this is not accurate, there is just one MDIO controller, but >> there may be different kinds of MDIO/PHY devices attached > > For me, if we want to represent the reality, we need two MDIO: > - since two PHY at the same address could co-exists > - since they are isolated so not on the same MDIO bus Is that really true? It might be, but from experience with e.g: bcmgenet, the integrated PHY and the external PHYs are on the same MDIO bus, which is convenient, except when you have an address conflict. > >> - having the STMMAC driver MDIO probing code having to deal with a >> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing >> and requiring more driver-level changes that are error prone > > My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") > have to be changed to something like "register_parent_mdio" > > > So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. > Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. > Really having two MDIO seems cleaner. The only valid thing that you have provided so far is this merging problem. Anything else ranging from "we will face with lots of small patch for adding phy-is-integrated" to "Really having two MDIO seems cleaner." are hard to receive as technical arguments for correctness. What happens if someone connects an external PHY at the same MDIO address than the internal PHY, which one do you get responses from? If you shutdown the internal PHY and it stops responding, then this probably becomes deterministic, but it still supports the fact there is just one MDIO bus controller per MAC. Anyway, do whatever works best for you I guess.
On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 08/24/2017 01:21 AM, Corentin Labbe wrote: >> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: >>> On 08/23/2017 12:49 AM, Maxime Ripard wrote: >>>> Hi Florian, >>>> >>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>>>>> 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? >>>> >>>> If possible, I'd really like to have the internal PHY in the >>>> DTSI. It's always there in hardware anyway, and duplicating the PHY, >>>> with its clock, reset line, and whatever info we might need in the >>>> future in each and every board DTS that uses it will be very error >>>> prone and we will have the usual bunch of issues that come up with >>>> duplication. >>> >>> OK, then what if you put the internal PHY in the DTSI, mark it with a >>> status = "disabled" property, and have the per-board DTS put a status = >>> "okay" property along with a "phy-is-integrated" boolean property? Would >>> that work? >> >> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. > > Is not there is a mistake in the unit address not matching the "reg" > property, or am I not looking at the right tree? > > &mdio { > ext_rgmii_phy: ethernet-phy@1 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <0>; > }; > }; > > If the PHY is really at MDIO address 0, then it should be > ethernet-phy@0, and not ethernet-phy@1, and then no problem with the > merging? That is wrong. The board described in the example likely has a Realtek RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address, so it still works, but is the wrong representation. > > >> So that adding a 'status = "disabled"' does not bring anything. >> >>> >>> What I really don't think is necessary is: >>> >>> - duplicating the "mdio" controller node for external vs. internal PHY, >>> because this is not accurate, there is just one MDIO controller, but >>> there may be different kinds of MDIO/PHY devices attached >> >> For me, if we want to represent the reality, we need two MDIO: >> - since two PHY at the same address could co-exists >> - since they are isolated so not on the same MDIO bus > > Is that really true? It might be, but from experience with e.g: > bcmgenet, the integrated PHY and the external PHYs are on the same MDIO > bus, which is convenient, except when you have an address conflict. There's a mux in the hardware: either the internal MDIO+MII lines from the internal PHY are connected to the MAC, or the external MDIO+MII lines from the pin controller are connected. I believe this was already mentioned? > >> >>> - having the STMMAC driver MDIO probing code having to deal with a >>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing >>> and requiring more driver-level changes that are error prone >> >> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") >> have to be changed to something like "register_parent_mdio" >> >> >> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. >> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. >> Really having two MDIO seems cleaner. > > The only valid thing that you have provided so far is this merging > problem. Anything else ranging from "we will face with lots of small > patch for adding phy-is-integrated" to "Really having two MDIO seems > cleaner." are hard to receive as technical arguments for correctness. > > What happens if someone connects an external PHY at the same MDIO > address than the internal PHY, which one do you get responses from? If > you shutdown the internal PHY and it stops responding, then this > probably becomes deterministic, but it still supports the fact there is > just one MDIO bus controller per MAC. Depends on whichever set of pins/lines are selected. But yeah, there's only one MDIO bus controller in the MAC. ChenYu > Anyway, do whatever works best for you I guess. > -- > Florian
On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote: > On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 08/24/2017 01:21 AM, Corentin Labbe wrote: >>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: >>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote: >>>>> Hi Florian, >>>>> >>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>>>>>> 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? >>>>> >>>>> If possible, I'd really like to have the internal PHY in the >>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY, >>>>> with its clock, reset line, and whatever info we might need in the >>>>> future in each and every board DTS that uses it will be very error >>>>> prone and we will have the usual bunch of issues that come up with >>>>> duplication. >>>> >>>> OK, then what if you put the internal PHY in the DTSI, mark it with a >>>> status = "disabled" property, and have the per-board DTS put a status = >>>> "okay" property along with a "phy-is-integrated" boolean property? Would >>>> that work? >>> >>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. >> >> Is not there is a mistake in the unit address not matching the "reg" >> property, or am I not looking at the right tree? >> >> &mdio { >> ext_rgmii_phy: ethernet-phy@1 { >> compatible = "ethernet-phy-ieee802.3-c22"; >> reg = <0>; >> }; >> }; >> >> If the PHY is really at MDIO address 0, then it should be >> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the >> merging? > > That is wrong. The board described in the example likely has a Realtek > RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address, > so it still works, but is the wrong representation. > >> >> >>> So that adding a 'status = "disabled"' does not bring anything. >>> >>>> >>>> What I really don't think is necessary is: >>>> >>>> - duplicating the "mdio" controller node for external vs. internal PHY, >>>> because this is not accurate, there is just one MDIO controller, but >>>> there may be different kinds of MDIO/PHY devices attached >>> >>> For me, if we want to represent the reality, we need two MDIO: >>> - since two PHY at the same address could co-exists >>> - since they are isolated so not on the same MDIO bus >> >> Is that really true? It might be, but from experience with e.g: >> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO >> bus, which is convenient, except when you have an address conflict. > > There's a mux in the hardware: either the internal MDIO+MII lines > from the internal PHY are connected to the MAC, or the external > MDIO+MII lines from the pin controller are connected. I believe > this was already mentioned? There is obviously a mux for the data lines and clock to switch between internal PHY and external PHYs, that does not mean there is one for MDIO and MDC lines, which is what is being suggested to be used here, does the mux also takes care of these lines? > >> >>> >>>> - having the STMMAC driver MDIO probing code having to deal with a >>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing >>>> and requiring more driver-level changes that are error prone >>> >>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") >>> have to be changed to something like "register_parent_mdio" >>> >>> >>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. >>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. >>> Really having two MDIO seems cleaner. >> >> The only valid thing that you have provided so far is this merging >> problem. Anything else ranging from "we will face with lots of small >> patch for adding phy-is-integrated" to "Really having two MDIO seems >> cleaner." are hard to receive as technical arguments for correctness. >> >> What happens if someone connects an external PHY at the same MDIO >> address than the internal PHY, which one do you get responses from? If >> you shutdown the internal PHY and it stops responding, then this >> probably becomes deterministic, but it still supports the fact there is >> just one MDIO bus controller per MAC. > > Depends on whichever set of pins/lines are selected. But yeah, there's > only one MDIO bus controller in the MAC. OK, so one MDIO controller, but what about the MDIO/MDC lines then, are they also muxed, like the data/clock lines or not?
On Fri, Aug 25, 2017 at 11:05 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote: >> On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> On 08/24/2017 01:21 AM, Corentin Labbe wrote: >>>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: >>>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote: >>>>>> Hi Florian, >>>>>> >>>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>>>>>>> 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? >>>>>> >>>>>> If possible, I'd really like to have the internal PHY in the >>>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY, >>>>>> with its clock, reset line, and whatever info we might need in the >>>>>> future in each and every board DTS that uses it will be very error >>>>>> prone and we will have the usual bunch of issues that come up with >>>>>> duplication. >>>>> >>>>> OK, then what if you put the internal PHY in the DTSI, mark it with a >>>>> status = "disabled" property, and have the per-board DTS put a status = >>>>> "okay" property along with a "phy-is-integrated" boolean property? Would >>>>> that work? >>>> >>>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. >>> >>> Is not there is a mistake in the unit address not matching the "reg" >>> property, or am I not looking at the right tree? >>> >>> &mdio { >>> ext_rgmii_phy: ethernet-phy@1 { >>> compatible = "ethernet-phy-ieee802.3-c22"; >>> reg = <0>; >>> }; >>> }; >>> >>> If the PHY is really at MDIO address 0, then it should be >>> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the >>> merging? >> >> That is wrong. The board described in the example likely has a Realtek >> RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address, >> so it still works, but is the wrong representation. >> >>> >>> >>>> So that adding a 'status = "disabled"' does not bring anything. >>>> >>>>> >>>>> What I really don't think is necessary is: >>>>> >>>>> - duplicating the "mdio" controller node for external vs. internal PHY, >>>>> because this is not accurate, there is just one MDIO controller, but >>>>> there may be different kinds of MDIO/PHY devices attached >>>> >>>> For me, if we want to represent the reality, we need two MDIO: >>>> - since two PHY at the same address could co-exists >>>> - since they are isolated so not on the same MDIO bus >>> >>> Is that really true? It might be, but from experience with e.g: >>> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO >>> bus, which is convenient, except when you have an address conflict. >> >> There's a mux in the hardware: either the internal MDIO+MII lines >> from the internal PHY are connected to the MAC, or the external >> MDIO+MII lines from the pin controller are connected. I believe >> this was already mentioned? > > There is obviously a mux for the data lines and clock to switch between > internal PHY and external PHYs, that does not mean there is one for MDIO > and MDC lines, which is what is being suggested to be used here, does > the mux also takes care of these lines? > >> >>> >>>> >>>>> - having the STMMAC driver MDIO probing code having to deal with a >>>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing >>>>> and requiring more driver-level changes that are error prone >>>> >>>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") >>>> have to be changed to something like "register_parent_mdio" >>>> >>>> >>>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. >>>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. >>>> Really having two MDIO seems cleaner. >>> >>> The only valid thing that you have provided so far is this merging >>> problem. Anything else ranging from "we will face with lots of small >>> patch for adding phy-is-integrated" to "Really having two MDIO seems >>> cleaner." are hard to receive as technical arguments for correctness. >>> >>> What happens if someone connects an external PHY at the same MDIO >>> address than the internal PHY, which one do you get responses from? If >>> you shutdown the internal PHY and it stops responding, then this >>> probably becomes deterministic, but it still supports the fact there is >>> just one MDIO bus controller per MAC. >> >> Depends on whichever set of pins/lines are selected. But yeah, there's >> only one MDIO bus controller in the MAC. > > OK, so one MDIO controller, but what about the MDIO/MDC lines then, are > they also muxed, like the data/clock lines or not? Just tested. Yes the MDIO/MDC lines are also muxed and controlled through the same mux bit. ChenYu
On 08/24/2017 08:41 PM, Chen-Yu Tsai wrote: > On Fri, Aug 25, 2017 at 11:05 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> On 08/24/2017 07:54 PM, Chen-Yu Tsai wrote: >>> On Fri, Aug 25, 2017 at 3:59 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> On 08/24/2017 01:21 AM, Corentin Labbe wrote: >>>>> On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote: >>>>>> On 08/23/2017 12:49 AM, Maxime Ripard wrote: >>>>>>> Hi Florian, >>>>>>> >>>>>>> On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote: >>>>>>>>>>> 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? >>>>>>> >>>>>>> If possible, I'd really like to have the internal PHY in the >>>>>>> DTSI. It's always there in hardware anyway, and duplicating the PHY, >>>>>>> with its clock, reset line, and whatever info we might need in the >>>>>>> future in each and every board DTS that uses it will be very error >>>>>>> prone and we will have the usual bunch of issues that come up with >>>>>>> duplication. >>>>>> >>>>>> OK, then what if you put the internal PHY in the DTSI, mark it with a >>>>>> status = "disabled" property, and have the per-board DTS put a status = >>>>>> "okay" property along with a "phy-is-integrated" boolean property? Would >>>>>> that work? >>>>> >>>>> No, I tested and for example with sun8i-h3-orangepi-plus.dts, the external PHY (ethernet-phy@1) is still merged. >>>> >>>> Is not there is a mistake in the unit address not matching the "reg" >>>> property, or am I not looking at the right tree? >>>> >>>> &mdio { >>>> ext_rgmii_phy: ethernet-phy@1 { >>>> compatible = "ethernet-phy-ieee802.3-c22"; >>>> reg = <0>; >>>> }; >>>> }; >>>> >>>> If the PHY is really at MDIO address 0, then it should be >>>> ethernet-phy@0, and not ethernet-phy@1, and then no problem with the >>>> merging? >>> >>> That is wrong. The board described in the example likely has a Realtek >>> RTL8211E @ address 0x1. Address 0 for this PHY is a broadcast address, >>> so it still works, but is the wrong representation. >>> >>>> >>>> >>>>> So that adding a 'status = "disabled"' does not bring anything. >>>>> >>>>>> >>>>>> What I really don't think is necessary is: >>>>>> >>>>>> - duplicating the "mdio" controller node for external vs. internal PHY, >>>>>> because this is not accurate, there is just one MDIO controller, but >>>>>> there may be different kinds of MDIO/PHY devices attached >>>>> >>>>> For me, if we want to represent the reality, we need two MDIO: >>>>> - since two PHY at the same address could co-exists >>>>> - since they are isolated so not on the same MDIO bus >>>> >>>> Is that really true? It might be, but from experience with e.g: >>>> bcmgenet, the integrated PHY and the external PHYs are on the same MDIO >>>> bus, which is convenient, except when you have an address conflict. >>> >>> There's a mux in the hardware: either the internal MDIO+MII lines >>> from the internal PHY are connected to the MAC, or the external >>> MDIO+MII lines from the pin controller are connected. I believe >>> this was already mentioned? >> >> There is obviously a mux for the data lines and clock to switch between >> internal PHY and external PHYs, that does not mean there is one for MDIO >> and MDC lines, which is what is being suggested to be used here, does >> the mux also takes care of these lines? >> >>> >>>> >>>>> >>>>>> - having the STMMAC driver MDIO probing code having to deal with a >>>>>> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing >>>>>> and requiring more driver-level changes that are error prone >>>>> >>>>> My patch for stmmac is really small, only the name of my variable ("need_mdio_mux_ids") >>>>> have to be changed to something like "register_parent_mdio" >>>>> >>>>> >>>>> So I agree with Maxime, we need to avoid merging PHY nodes, and we can avoid it only by having two separate MDIO nodes. >>>>> Furthermore, with only one MDIO, we will face with lots of small patch for adding phy-is-integrated, with two we do not need to change any board DT, all is simply clean. >>>>> Really having two MDIO seems cleaner. >>>> >>>> The only valid thing that you have provided so far is this merging >>>> problem. Anything else ranging from "we will face with lots of small >>>> patch for adding phy-is-integrated" to "Really having two MDIO seems >>>> cleaner." are hard to receive as technical arguments for correctness. >>>> >>>> What happens if someone connects an external PHY at the same MDIO >>>> address than the internal PHY, which one do you get responses from? If >>>> you shutdown the internal PHY and it stops responding, then this >>>> probably becomes deterministic, but it still supports the fact there is >>>> just one MDIO bus controller per MAC. >>> >>> Depends on whichever set of pins/lines are selected. But yeah, there's >>> only one MDIO bus controller in the MAC. >> >> OK, so one MDIO controller, but what about the MDIO/MDC lines then, are >> they also muxed, like the data/clock lines or not? > > 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.
> > 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
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 {