| Message ID | 20230119171248.3882021-3-bjorn@mork.no (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fixes for mtk_eth_soc | expand |
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > On Thu, Jan 19, 2023 at 06:12:47PM +0100, Bjørn Mork wrote: >> sgmii mode fails if autonegotiation is disabled. >> >> Signed-off-by: Bjørn Mork <bjorn@mork.no> >> --- >> drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++-------- >> 1 file changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c >> index 481f2f1e39f5..d1f2bcb21242 100644 >> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c >> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c >> @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, >> * other words, 1000Mbps or 2500Mbps). >> */ >> if (interface == PHY_INTERFACE_MODE_SGMII) { >> - sgm_mode = SGMII_IF_MODE_SGMII; >> - if (phylink_autoneg_inband(mode)) { >> - sgm_mode |= SGMII_REMOTE_FAULT_DIS | >> - SGMII_SPEED_DUPLEX_AN; >> - use_an = true; >> - } else { >> - use_an = false; >> - } >> + sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS | >> + SGMII_SPEED_DUPLEX_AN; >> + use_an = true; > > I wasn't actually suggesting in our discussion that this is something > which should be changed. > > The reference implementation for the expected behaviour is > phylink_mii_c22_pcs_config(), and it only enables in-band if "mode" > says so. If we have a PHY which has in-band disabled (yes, they do > exist) then having SGMII in-band unconditionally enabled breaks them, > and yes, those PHYs appear on SFP modules. > > The proper answer is to use 'managed = "in-band-status";' in your DT > to have in-band used with SGMII. Well, yeah, I'd love to. But then I'm back to the drawing board without a link. That just doesn't work for me. What I did here also reflects what I saw in the mt7530.c debug dumps, and how I read that code: static int mt7531_sgmii_setup_mode_an(struct mt7530_priv *priv, int port, phy_interface_t interface) { if (!mt753x_is_mac_port(port)) return -EINVAL; mt7530_set(priv, MT7531_QPHY_PWR_STATE_CTRL(port), MT7531_SGMII_PHYA_PWD); mt7530_rmw(priv, MT7531_PHYA_CTRL_SIGNAL3(port), MT7531_RG_TPHY_SPEED_MASK, MT7531_RG_TPHY_SPEED_1_25G); mt7530_set(priv, MT7531_SGMII_MODE(port), MT7531_SGMII_REMOTE_FAULT_DIS | MT7531_SGMII_SPEED_DUPLEX_AN); mt7530_rmw(priv, MT7531_PCS_SPEED_ABILITY(port), MT7531_SGMII_TX_CONFIG_MASK, 1); mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_ENABLE); mt7530_set(priv, MT7531_PCS_CONTROL_1(port), MT7531_SGMII_AN_RESTART); mt7530_write(priv, MT7531_QPHY_PWR_STATE_CTRL(port), 0); return 0; } static int mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode, phy_interface_t interface) { struct mt7530_priv *priv = ds->priv; struct phy_device *phydev; struct dsa_port *dp; if (!mt753x_is_mac_port(port)) { dev_err(priv->dev, "port %d is not a MAC port\n", port); return -EINVAL; } switch (interface) { case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII_RXID: case PHY_INTERFACE_MODE_RGMII_TXID: dp = dsa_to_port(ds, port); phydev = dp->slave->phydev; return mt7531_rgmii_setup(priv, port, interface, phydev); case PHY_INTERFACE_MODE_SGMII: return mt7531_sgmii_setup_mode_an(priv, port, interface); case PHY_INTERFACE_MODE_NA: case PHY_INTERFACE_MODE_1000BASEX: case PHY_INTERFACE_MODE_2500BASEX: return mt7531_sgmii_setup_mode_force(priv, port, interface); default: return -EINVAL; } return -EINVAL; } AFAICS, this calls mt7531_sgmii_setup_mode_an() unconditionally for PHY_INTERFACE_MODE_SGMII. Resulting in AN_ENABLE|AN_RESTART being set in PCS_CONTROL_1 and SGMII_REMOTE_FAULT_DIS|SGMII_SPEED_DUPLEX_AN being set in SGMII_MODE. Regardless of inband or not. Bjørn
On Thu, Jan 19, 2023 at 08:33:17PM +0100, Bjørn Mork wrote: > "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > > On Thu, Jan 19, 2023 at 06:12:47PM +0100, Bjørn Mork wrote: > >> sgmii mode fails if autonegotiation is disabled. > >> > >> Signed-off-by: Bjørn Mork <bjorn@mork.no> > >> --- > >> drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++-------- > >> 1 file changed, 3 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > >> index 481f2f1e39f5..d1f2bcb21242 100644 > >> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > >> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > >> @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, > >> * other words, 1000Mbps or 2500Mbps). > >> */ > >> if (interface == PHY_INTERFACE_MODE_SGMII) { > >> - sgm_mode = SGMII_IF_MODE_SGMII; > >> - if (phylink_autoneg_inband(mode)) { > >> - sgm_mode |= SGMII_REMOTE_FAULT_DIS | > >> - SGMII_SPEED_DUPLEX_AN; > >> - use_an = true; > >> - } else { > >> - use_an = false; > >> - } > >> + sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS | > >> + SGMII_SPEED_DUPLEX_AN; > >> + use_an = true; > > > > I wasn't actually suggesting in our discussion that this is something > > which should be changed. > > > > The reference implementation for the expected behaviour is > > phylink_mii_c22_pcs_config(), and it only enables in-band if "mode" > > says so. If we have a PHY which has in-band disabled (yes, they do > > exist) then having SGMII in-band unconditionally enabled breaks them, > > and yes, those PHYs appear on SFP modules. > > > > The proper answer is to use 'managed = "in-band-status";' in your DT > > to have in-band used with SGMII. > > Well, yeah, I'd love to. But then I'm back to the drawing board without > a link. That just doesn't work for me. If you have 'managed = "in-band-status";' in your DT, that will set "mode" to be MLO_AN_INBAND, and phylink_autoneg_inband(mode) will be true - which should result in the link being programmed for in-band mode. You should also find that mtk_pcs_get_state() gets called. Hmm, it looks like setting ss->pcs[i].pcs.poll to true was missed when support for inband was properly added, so that might be the issue there - as the mtk ethernet driver doesn't make use of phylink_mac_change().
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > If you have 'managed = "in-band-status";' in your DT, that will set > "mode" to be MLO_AN_INBAND, and phylink_autoneg_inband(mode) will be > true - which should result in the link being programmed for in-band > mode. You should also find that mtk_pcs_get_state() gets called. > > Hmm, it looks like setting ss->pcs[i].pcs.poll to true was missed > when support for inband was properly added, so that might be the > issue there - as the mtk ethernet driver doesn't make use of > phylink_mac_change(). OK, this doesn't just work as-is either. But I guess that's something else. Will try to debug some more. But I wonder: Why would I want to use "in-band-status" here? Is that the preferred mode? Probably stupid question. But shouldn't we also make this link work without it, whatever that takes? Note that I don't have to care about unknown phys. No SFP cage on this board. Bjørn
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 481f2f1e39f5..d1f2bcb21242 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -62,14 +62,9 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, * other words, 1000Mbps or 2500Mbps). */ if (interface == PHY_INTERFACE_MODE_SGMII) { - sgm_mode = SGMII_IF_MODE_SGMII; - if (phylink_autoneg_inband(mode)) { - sgm_mode |= SGMII_REMOTE_FAULT_DIS | - SGMII_SPEED_DUPLEX_AN; - use_an = true; - } else { - use_an = false; - } + sgm_mode = SGMII_IF_MODE_SGMII | SGMII_REMOTE_FAULT_DIS | + SGMII_SPEED_DUPLEX_AN; + use_an = true; } else if (phylink_autoneg_inband(mode)) { /* 1000base-X or 2500base-X autoneg */ sgm_mode = SGMII_REMOTE_FAULT_DIS;
sgmii mode fails if autonegotiation is disabled. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/ethernet/mediatek/mtk_sgmii.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)