| Message ID | 20170818122118.4925-4-clabbe.montjoie@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > In case of a MDIO switch, the registered MDIO node should be > the parent of the PHY. Otherwise of_phy_connect will fail. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index a366b3747eeb..ca3cc99d8960 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > static const struct of_device_id need_mdio_ids[] = { > { .compatible = "snps,dwc-qos-ethernet-4.10" }, > { .compatible = "allwinner,sun8i-a83t-emac" }, > - { .compatible = "allwinner,sun8i-h3-emac" }, > { .compatible = "allwinner,sun8i-v3s-emac" }, > { .compatible = "allwinner,sun50i-a64-emac" }, > }; > + static const struct of_device_id need_mdio_mux_ids[] = { > + { .compatible = "allwinner,sun8i-h3-emac" }, > + }; > > /* If phy-handle property is passed from DT, use it as the PHY */ > plat->phy_node = of_parse_phandle(np, "phy-handle", 0); > @@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > mdio = false; > } > > - if (of_match_node(need_mdio_ids, np)) { > + /* > + * In case of a MDIO switch/mux, the registered MDIO node should be > + * the parent of the PHY. Otherwise of_phy_connect will fail. > + */ > + if (of_match_node(need_mdio_mux_ids, np)) { > + plat->mdio_node = of_get_parent(plat->phy_node); Extra space before of_get_parent. Also this is going to fail horribly if a fixed link is used. ChenYu > + } else if (of_match_node(need_mdio_ids, np)) { > plat->mdio_node = of_get_child_by_name(np, "mdio"); > } else { > /** > -- > 2.13.0 >
On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote: > On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe > <clabbe.montjoie@gmail.com> wrote: > > In case of a MDIO switch, the registered MDIO node should be > > the parent of the PHY. Otherwise of_phy_connect will fail. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > index a366b3747eeb..ca3cc99d8960 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > @@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > > static const struct of_device_id need_mdio_ids[] = { > > { .compatible = "snps,dwc-qos-ethernet-4.10" }, > > { .compatible = "allwinner,sun8i-a83t-emac" }, > > - { .compatible = "allwinner,sun8i-h3-emac" }, > > { .compatible = "allwinner,sun8i-v3s-emac" }, > > { .compatible = "allwinner,sun50i-a64-emac" }, > > }; > > + static const struct of_device_id need_mdio_mux_ids[] = { > > + { .compatible = "allwinner,sun8i-h3-emac" }, > > + }; > > > > /* If phy-handle property is passed from DT, use it as the PHY */ > > plat->phy_node = of_parse_phandle(np, "phy-handle", 0); > > @@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, > > mdio = false; > > } > > > > - if (of_match_node(need_mdio_ids, np)) { > > + /* > > + * In case of a MDIO switch/mux, the registered MDIO node should be > > + * the parent of the PHY. Otherwise of_phy_connect will fail. > > + */ > > + if (of_match_node(need_mdio_mux_ids, np)) { > > + plat->mdio_node = of_get_parent(plat->phy_node); > > Extra space before of_get_parent. > > Also this is going to fail horribly if a fixed link is used. > Hello I will add an extra patch for handling fixed-link for both need_mdio_mux_ids/need_mdio_ids cases. Thanks Regards
On Sat, Aug 19, 2017 at 08:50:25PM +0200, Corentin Labbe wrote: > On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote: > > On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe > > <clabbe.montjoie@gmail.com> wrote: > > > In case of a MDIO switch, the registered MDIO node should be > > > the parent of the PHY. Otherwise of_phy_connect will fail. Hi Corentin Sorry, I missed this patch series. Looking at patchwork... Can you represent the MDIO mux using Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt It would be better if you could reuse existing infrastructure than invent something new. Andrew
On Sat, Aug 19, 2017 at 10:38:36PM +0200, Andrew Lunn wrote: > On Sat, Aug 19, 2017 at 08:50:25PM +0200, Corentin Labbe wrote: > > On Sat, Aug 19, 2017 at 01:05:21AM +0800, Chen-Yu Tsai wrote: > > > On Fri, Aug 18, 2017 at 8:21 PM, Corentin Labbe > > > <clabbe.montjoie@gmail.com> wrote: > > > > In case of a MDIO switch, the registered MDIO node should be > > > > the parent of the PHY. Otherwise of_phy_connect will fail. > > Hi Corentin > > Sorry, I missed this patch series. Looking at patchwork... That's my fault, I forgot to set you in recipient like in last send. > > Can you represent the MDIO mux using > > Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt > > It would be better if you could reuse existing infrastructure than > invent something new. > I think we cannot use mdio-mux-mmioreg since the register for doing the switch is in middle of the "System Control" and shared with other functions. This is why we use a sycon/regmap for selecting the MDIO. Regards
> I think we cannot use mdio-mux-mmioreg since the register for doing > the switch is in middle of the "System Control" and shared with > other functions. This is why we use a sycon/regmap for selecting > the MDIO. You could add a mdio-mux-regmap.c. However, it probably need restructuring of the stmmac mdio code, to make the mdio bus usable as a separate driver. You need stmmac mdio to probe first, then mdio-mux-remap should probe, and then lastly stmmmac mac driver. With stmmac mdio and stmmac mac being in one driver, there is no time in the middle to allow the mux driver to probe. It is some effort, but a nice cleanup and generalization. Andrew
On Sun, Aug 20, 2017 at 10:25 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> I think we cannot use mdio-mux-mmioreg since the register for doing >> the switch is in middle of the "System Control" and shared with >> other functions. This is why we use a sycon/regmap for selecting >> the MDIO. > > You could add a mdio-mux-regmap.c. > > However, it probably need restructuring of the stmmac mdio code, to > make the mdio bus usable as a separate driver. You need stmmac mdio > to probe first, then mdio-mux-remap should probe, and then lastly > stmmmac mac driver. > > With stmmac mdio and stmmac mac being in one driver, there is no time > in the middle to allow the mux driver to probe. > > It is some effort, but a nice cleanup and generalization. I'm not sure mdio-mux is the right thing here. Looking at mdio-mux, it seems to provide a way to access multiplexed MDIO buses. It says nothing about the MAC<->PHY connection. With our hardware, and likely Rockchip's as well, the muxed connections include the MDIO and MII connections, which are muxed at the same time. It's likely to cause some confusion or even bugs if we implement it as a separate driver. ChenYu
> 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
Hi Andrew, On Mon, Aug 21, 2017 at 03:20:15PM +0200, Andrew Lunn wrote: > > 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. Do you have any suggestion on what the binding would look like? All muxes are mostly always represented the same way afaik, or do you want to simply introduce a new compatible / property? Thanks! Maxime
> 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? Andrew
On Mon, Aug 21, 2017 at 04:23:21PM +0200, Andrew Lunn 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? > Exactly you can change it on the fly, but you need to reset the MAC for enabling the new configuration. The stmmac driver does not handle mdio-mux. It is why I have a patch which automaticly select parent MDIO node of the PHY node. For representation we could keep the current. (With a big comment stating that it is a switch) We can also add a mdio-switch type node, or add a mdio-switch property to mdio-mux. Regards
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. 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). Regards ChenYu
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?
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
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index a366b3747eeb..ca3cc99d8960 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -312,10 +312,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, static const struct of_device_id need_mdio_ids[] = { { .compatible = "snps,dwc-qos-ethernet-4.10" }, { .compatible = "allwinner,sun8i-a83t-emac" }, - { .compatible = "allwinner,sun8i-h3-emac" }, { .compatible = "allwinner,sun8i-v3s-emac" }, { .compatible = "allwinner,sun50i-a64-emac" }, }; + static const struct of_device_id need_mdio_mux_ids[] = { + { .compatible = "allwinner,sun8i-h3-emac" }, + }; /* If phy-handle property is passed from DT, use it as the PHY */ plat->phy_node = of_parse_phandle(np, "phy-handle", 0); @@ -332,7 +334,13 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat, mdio = false; } - if (of_match_node(need_mdio_ids, np)) { + /* + * In case of a MDIO switch/mux, the registered MDIO node should be + * the parent of the PHY. Otherwise of_phy_connect will fail. + */ + if (of_match_node(need_mdio_mux_ids, np)) { + plat->mdio_node = of_get_parent(plat->phy_node); + } else if (of_match_node(need_mdio_ids, np)) { plat->mdio_node = of_get_child_by_name(np, "mdio"); } else { /**
In case of a MDIO switch, the registered MDIO node should be the parent of the PHY. Otherwise of_phy_connect will fail. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)