| Message ID | 20230407152503.2320741-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add missing DSA properties for marvell switches | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Apr 07, 2023 at 05:25:01PM +0200, Andrew Lunn wrote: > The DSA framework has got more picky about always having a phy-mode > for the CPU port. The imx51 Ethernet supports MII, and RMII. Set the > switch phy-mode based on how the SoC Ethernet port has been > configured. > > Additionally, the cpu label has never actually been used in the > binding, so remove it. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on one end and phy-mode = "rev-mii" on the other, right?
On Fri, Apr 07, 2023 at 06:41:59PM +0300, Vladimir Oltean wrote: > On Fri, Apr 07, 2023 at 05:25:01PM +0200, Andrew Lunn wrote: > > The DSA framework has got more picky about always having a phy-mode > > for the CPU port. The imx51 Ethernet supports MII, and RMII. Set the > > switch phy-mode based on how the SoC Ethernet port has been > > configured. > > > > Additionally, the cpu label has never actually been used in the > > binding, so remove it. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on > one end and phy-mode = "rev-mii" on the other, right? In theory, yes. As far as i understand, it makes a difference to where the clock comes from. rev-mii is a clock provider i think. But from what i understand of the code, and the silicon, this property is going to be ignored, whatever value you give it. phy-mode is only used and respected when the port can support 1000Base-X, SGMII, and above, or use its built in PHY. For MII, GMII, RMII, RGMII the port setting is determined by strapping resistors. The DSA core however does care that there is a phy-mode, even if it is ignored. I hope after these patches land we can turn that check into enforce mode, and that then unlocks Russell to make phylink improvement. Andrew
On Fri, Apr 07, 2023 at 06:10:31PM +0200, Andrew Lunn wrote: > On Fri, Apr 07, 2023 at 06:41:59PM +0300, Vladimir Oltean wrote: > > On Fri, Apr 07, 2023 at 05:25:01PM +0200, Andrew Lunn wrote: > > > The DSA framework has got more picky about always having a phy-mode > > > for the CPU port. The imx51 Ethernet supports MII, and RMII. Set the > > > switch phy-mode based on how the SoC Ethernet port has been > > > configured. > > > > > > Additionally, the cpu label has never actually been used in the > > > binding, so remove it. > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > --- > > > > In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on > > one end and phy-mode = "rev-mii" on the other, right? > > In theory, yes. As far as i understand, it makes a difference to where > the clock comes from. rev-mii is a clock provider i think. > > But from what i understand of the code, and the silicon, this property > is going to be ignored, whatever value you give it. phy-mode is only > used and respected when the port can support 1000Base-X, SGMII, and > above, or use its built in PHY. For MII, GMII, RMII, RGMII the port > setting is determined by strapping resistors. If it's ignored, even better, one more reason to make it rev-mii and not mii, no compatibility concerns with the driver not understanding the difference... > The DSA core however does care that there is a phy-mode, even if it is > ignored. I hope after these patches land we can turn that check into > enforce mode, and that then unlocks Russell to make phylink > improvement. > > Andrew >
> If it's ignored, even better, one more reason to make it rev-mii and > not mii, no compatibility concerns with the driver not understanding the > difference... O.K. i will respin the patch tomorrow. Andrew
On Fri, Apr 07, 2023 at 06:10:31PM +0200, Andrew Lunn wrote: > On Fri, Apr 07, 2023 at 06:41:59PM +0300, Vladimir Oltean wrote: > > In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on > > one end and phy-mode = "rev-mii" on the other, right? > > In theory, yes. As far as i understand, it makes a difference to where > the clock comes from. rev-mii is a clock provider i think. > > But from what i understand of the code, and the silicon, this property > is going to be ignored, whatever value you give it. phy-mode is only > used and respected when the port can support 1000Base-X, SGMII, and > above, or use its built in PHY. For MII, GMII, RMII, RGMII the port > setting is determined by strapping resistors. > > The DSA core however does care that there is a phy-mode, even if it is > ignored. I hope after these patches land we can turn that check into > enforce mode, and that then unlocks Russell to make phylink > improvement. Actually, looking at mv88e6xxx_translate_cmode() right now, I guess it's not exactly true that the value is going to be ignored, whatever it is. A CMODE of MV88E6XXX_PORT_STS_CMODE_MII_PHY is not going to be translated into "rev-mii", but into "mii", same as MV88E6XXX_PORT_STS_CMODE_MII. Same for MV88E6XXX_PORT_STS_CMODE_RMII_PHY ("rmii" and not "rev-rmii"). So, when given "rev-mii" or "rev-rmii" as phy modes in the device tree, the generic phylink validation procedure should reject them for being unsupported. This means either the patch set moves forward with v1, or the driver is fixed to accept the dedicated PHY modes for PHY roles. Russell, what do you think?
On Mon, Apr 10, 2023 at 01:00:12PM +0300, Vladimir Oltean wrote: > On Fri, Apr 07, 2023 at 06:10:31PM +0200, Andrew Lunn wrote: > > On Fri, Apr 07, 2023 at 06:41:59PM +0300, Vladimir Oltean wrote: > > > In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on > > > one end and phy-mode = "rev-mii" on the other, right? > > > > In theory, yes. As far as i understand, it makes a difference to where > > the clock comes from. rev-mii is a clock provider i think. > > > > But from what i understand of the code, and the silicon, this property > > is going to be ignored, whatever value you give it. phy-mode is only > > used and respected when the port can support 1000Base-X, SGMII, and > > above, or use its built in PHY. For MII, GMII, RMII, RGMII the port > > setting is determined by strapping resistors. > > > > The DSA core however does care that there is a phy-mode, even if it is > > ignored. I hope after these patches land we can turn that check into > > enforce mode, and that then unlocks Russell to make phylink > > improvement. > > Actually, looking at mv88e6xxx_translate_cmode() right now, I guess it's > not exactly true that the value is going to be ignored, whatever it is. > A CMODE of MV88E6XXX_PORT_STS_CMODE_MII_PHY is not going to be translated > into "rev-mii", but into "mii", same as MV88E6XXX_PORT_STS_CMODE_MII. > Same for MV88E6XXX_PORT_STS_CMODE_RMII_PHY ("rmii" and not "rev-rmii"). > So, when given "rev-mii" or "rev-rmii" as phy modes in the device tree, > the generic phylink validation procedure should reject them for being > unsupported. > > This means either the patch set moves forward with v1, or the driver is > fixed to accept the dedicated PHY modes for PHY roles. > > Russell, what do you think? I'm afraid I didn't bother trying to understand all the *MII modes that Marvell document poorly in their functional specification, especially when they document that e.g. RMII_PHY mode can also be used to connect to a PHY. I took their table with a pinch of salt and did what I thought was best. It is entirely possible that some are wrong, especially as we don't document what the various PHY_INTERFACE_MODE_* mean in the kernel (remember that I started doing that). As far as phylink, it treats REV*MII and the corresponding *MII mode the same way in terms of their capabilities, but as you say it will determine which mode will be acceptable.
> Actually, looking at mv88e6xxx_translate_cmode() right now, I guess it's > not exactly true that the value is going to be ignored, whatever it is. > A CMODE of MV88E6XXX_PORT_STS_CMODE_MII_PHY is not going to be translated > into "rev-mii", but into "mii", same as MV88E6XXX_PORT_STS_CMODE_MII. > Same for MV88E6XXX_PORT_STS_CMODE_RMII_PHY ("rmii" and not "rev-rmii"). > So, when given "rev-mii" or "rev-rmii" as phy modes in the device tree, > the generic phylink validation procedure should reject them for being > unsupported. Ah. I did not actually test this version on hardware. I was expecting it to be ignored, since the cmode cannot be changed. I did not think about phylink performing validation. I will boot of one these DT files on hardware to see what happens. Andrew
On Mon, Apr 10, 2023 at 01:00:12PM +0300, Vladimir Oltean wrote: > On Fri, Apr 07, 2023 at 06:10:31PM +0200, Andrew Lunn wrote: > > On Fri, Apr 07, 2023 at 06:41:59PM +0300, Vladimir Oltean wrote: > > > In theory, an MII MAC-to-MAC connection should have phy-mode = "mii" on > > > one end and phy-mode = "rev-mii" on the other, right? > > > > In theory, yes. As far as i understand, it makes a difference to where > > the clock comes from. rev-mii is a clock provider i think. > > > > But from what i understand of the code, and the silicon, this property > > is going to be ignored, whatever value you give it. phy-mode is only > > used and respected when the port can support 1000Base-X, SGMII, and > > above, or use its built in PHY. For MII, GMII, RMII, RGMII the port > > setting is determined by strapping resistors. > > > > The DSA core however does care that there is a phy-mode, even if it is > > ignored. I hope after these patches land we can turn that check into > > enforce mode, and that then unlocks Russell to make phylink > > improvement. > > Actually, looking at mv88e6xxx_translate_cmode() right now, I guess it's > not exactly true that the value is going to be ignored, whatever it is. > A CMODE of MV88E6XXX_PORT_STS_CMODE_MII_PHY is not going to be translated > into "rev-mii", but into "mii", same as MV88E6XXX_PORT_STS_CMODE_MII. > Same for MV88E6XXX_PORT_STS_CMODE_RMII_PHY ("rmii" and not "rev-rmii"). > So, when given "rev-mii" or "rev-rmii" as phy modes in the device tree, > the generic phylink validation procedure should reject them for being > unsupported. I tested vf610-zii-devel-rev-c.dtb: [ 2.307416] mv88e6085 mdio_mux-0.1:00: configuring for fixed/rev-rmii link mode [ 2.314459] mv88e6085 mdio_mux-0.1:00: configuring for fixed/xaui link mode [ 2.320588] mv88e6085 mdio_mux-0.1:00: Link is Up - 100Mbps/Full - flow control off [ 2.327722] mv88e6085 mdio_mux-0.2:00: configuring for fixed/xaui link mode [ 2.334729] mv88e6085 mdio_mux-0.2:00: Link is Up - 10Gbps/Full - flow control off [ 2.343263] mv88e6085 mdio_mux-0.1:00: Link is Up - 10Gbps/Full - flow control off [ 2.396110] mv88e6085 mdio_mux-0.1:00 lan1 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:01] driver [Marvell 88E6390 Fa) [ 2.498137] mv88e6085 mdio_mux-0.1:00 lan2 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:02] driver [Marvell 88E6390 Fa) [ 2.566028] mv88e6085 mdio_mux-0.1:00 lan3 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:03] driver [Marvell 88E6390 Fa) [ 2.663995] mv88e6085 mdio_mux-0.1:00 lan4 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:04] driver [Marvell 88E6390 Fa) [ 2.746064] mv88e6085 mdio_mux-0.2:00 lan5 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:01] driver [Marvell 88E6390 Fa) [ 2.834000] mv88e6085 mdio_mux-0.2:00 lan6 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:02] driver [Marvell 88E6390 Fa) [ 2.933998] mv88e6085 mdio_mux-0.2:00 lan7 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:03] driver [Marvell 88E6390 Fa) [ 3.016031] mv88e6085 mdio_mux-0.2:00 lan8 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:04] driver [Marvell 88E6390 Fa) [ 3.033976] mv88e6085 mdio_mux-0.2:00 sff2 (uninitialized): switched to inband/2500base-x link mode So we can see link mode rev-rmii and there are no errors. Testing a board using mii, not rmii is going to be a bit harder. I don't have one set up right now. Andrew
On Mon, Apr 10, 2023 at 02:35:28PM +0200, Andrew Lunn wrote: > I tested vf610-zii-devel-rev-c.dtb: > > [ 2.307416] mv88e6085 mdio_mux-0.1:00: configuring for fixed/rev-rmii link mode > [ 2.314459] mv88e6085 mdio_mux-0.1:00: configuring for fixed/xaui link mode > [ 2.320588] mv88e6085 mdio_mux-0.1:00: Link is Up - 100Mbps/Full - flow control off > [ 2.327722] mv88e6085 mdio_mux-0.2:00: configuring for fixed/xaui link mode > [ 2.334729] mv88e6085 mdio_mux-0.2:00: Link is Up - 10Gbps/Full - flow control off > [ 2.343263] mv88e6085 mdio_mux-0.1:00: Link is Up - 10Gbps/Full - flow control off > [ 2.396110] mv88e6085 mdio_mux-0.1:00 lan1 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:01] driver [Marvell 88E6390 Fa) > [ 2.498137] mv88e6085 mdio_mux-0.1:00 lan2 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:02] driver [Marvell 88E6390 Fa) > [ 2.566028] mv88e6085 mdio_mux-0.1:00 lan3 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:03] driver [Marvell 88E6390 Fa) > [ 2.663995] mv88e6085 mdio_mux-0.1:00 lan4 (uninitialized): PHY [!mdio-mux!mdio@1!switch@0!mdio:04] driver [Marvell 88E6390 Fa) > [ 2.746064] mv88e6085 mdio_mux-0.2:00 lan5 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:01] driver [Marvell 88E6390 Fa) > [ 2.834000] mv88e6085 mdio_mux-0.2:00 lan6 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:02] driver [Marvell 88E6390 Fa) > [ 2.933998] mv88e6085 mdio_mux-0.2:00 lan7 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:03] driver [Marvell 88E6390 Fa) > [ 3.016031] mv88e6085 mdio_mux-0.2:00 lan8 (uninitialized): PHY [!mdio-mux!mdio@2!switch@0!mdio:04] driver [Marvell 88E6390 Fa) > [ 3.033976] mv88e6085 mdio_mux-0.2:00 sff2 (uninitialized): switched to inband/2500base-x link mode > > So we can see link mode rev-rmii and there are no errors. > > Testing a board using mii, not rmii is going to be a bit harder. I > don't have one set up right now. hmmm... why does this work? would you mind adding this small debug print and booting again? diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index a4111f1be375..0b9754d4db80 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -695,6 +695,10 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported, { const unsigned long *interfaces = pl->config->supported_interfaces; + phylink_err(pl, "%s: supported_interfaces=%*pbl, interface %s\n", + __func__, (int)PHY_INTERFACE_MODE_MAX, + interfaces, phy_modes(state->interface)); + if (!phy_interface_empty(interfaces)) { if (state->interface == PHY_INTERFACE_MODE_NA) return phylink_validate_mask(pl, supported, state, I would've thought this check below would fail: if (!test_bit(state->interface, interfaces)) return -EINVAL;
> hmmm... why does this work? > > would you mind adding this small debug print and booting again? mv88e6xxx_translate_cmode: cmode 4, supported: supported=7 CMODE 4 is 'RMII PHY' or 'RMII to PHY', depending on if it has found a PHY or not. mv88e6085 mdio_mux-0.1:00: phylink_validate: supported_interfaces=1,3,7, interface rev-rmii mv88e6085 mdio_mux-0.1:00: phylink_validate: supported_interfaces=1,3,7, interface rev-rmii mv88e6085 mdio_mux-0.1:00: configuring for fixed/rev-rmii link mode Both calls to phylink_validate() then returning -EINVAL. The first call is from phylink_create(), which does not check the return value. The second call is from phylink_parse_fixedlink(), which also does not check the return code. This is a bit fragile, all it would need is for these return values to be checked and it would break. So i think cmode 4 should return REVRMII and cmode 5 shoud be RMII. I will submit a patch making this change. Andrew
On Mon, Apr 10, 2023 at 11:37:15AM +0100, Russell King (Oracle) wrote: > It is entirely possible that some are wrong, especially as we don't > document what the various PHY_INTERFACE_MODE_* mean in the kernel > (remember that I started doing that). include/linux/phy.h does give a small hint about what is expected: * @PHY_INTERFACE_MODE_REVRMII: Reduced Media Independent Interface in PHY role and the commit which added that mode does reference, in its description, the conversation that led to its creation: https://lore.kernel.org/netdev/20210201214515.cx6ivvme2tlquge2@skbuf/
diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts index e537e06e11d7..8621760af1af 100644 --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts @@ -181,7 +181,7 @@ ports { port@0 { reg = <0>; - label = "cpu"; + phy-mode = "mii"; ethernet = <&fec>; fixed-link { diff --git a/arch/arm/boot/dts/imx51-zii-scu2-mezz.dts b/arch/arm/boot/dts/imx51-zii-scu2-mezz.dts index 21dd3f7abd48..883e80d92ef0 100644 --- a/arch/arm/boot/dts/imx51-zii-scu2-mezz.dts +++ b/arch/arm/boot/dts/imx51-zii-scu2-mezz.dts @@ -82,7 +82,7 @@ port@3 { port@4 { reg = <4>; - label = "cpu"; + phy-mode = "mii"; ethernet = <&fec>; fixed-link { diff --git a/arch/arm/boot/dts/imx51-zii-scu3-esb.dts b/arch/arm/boot/dts/imx51-zii-scu3-esb.dts index 9f857eb44bf7..19a3b142c964 100644 --- a/arch/arm/boot/dts/imx51-zii-scu3-esb.dts +++ b/arch/arm/boot/dts/imx51-zii-scu3-esb.dts @@ -267,7 +267,6 @@ fixed-link { port@6 { reg = <6>; - label = "cpu"; phy-mode = "mii"; ethernet = <&fec>;
The DSA framework has got more picky about always having a phy-mode for the CPU port. The imx51 Ethernet supports MII, and RMII. Set the switch phy-mode based on how the SoC Ethernet port has been configured. Additionally, the cpu label has never actually been used in the binding, so remove it. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- arch/arm/boot/dts/imx51-zii-rdu1.dts | 2 +- arch/arm/boot/dts/imx51-zii-scu2-mezz.dts | 2 +- arch/arm/boot/dts/imx51-zii-scu3-esb.dts | 1 - 3 files changed, 2 insertions(+), 3 deletions(-)