| Message ID | 20230217034230.1249661-13-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework MAC drivers EEE support | expand |
On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote: > phylib should be called in order to manage the EEE settings in the > PHY, and to return EEE status such are supported link modes, and what > the link peer supports. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/dsa/mt7530.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 214450378978..a472353f14f8 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port, > { > struct mt7530_priv *priv = ds->priv; > u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port)); > + struct dsa_port *dp = dsa_to_port(ds, port); > > e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN); > e->tx_lpi_timer = GET_LPI_THRESH(eeecr); > > + if (dp->slave->phydev) > + return phy_ethtool_get_eee(dp->slave->phydev, e); Given that DSA makes use of phylink, is there a reason why we can't use the phylink wrappers here (which may allow for future EEE improvements, e.g. moving the gating of EEE with the TX LPI enable) ? > @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port, > set |= LPI_MODE_EN; > mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set); > > + if (dp->slave->phydev) > + return phy_ethtool_set_eee(dp->slave->phydev, e); > return 0; Is this the correct place to do the set_eee operation - I mean, the register state has been altered (and it looks like it may enable LPI irrespective of the negotiated state) but what concerns me is that phy_ethtool_set_eee() can fail, and we return failure to userspace yet we've modified register state.
On Fri, Feb 17, 2023 at 11:48:01AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote: > > phylib should be called in order to manage the EEE settings in the > > PHY, and to return EEE status such are supported link modes, and what > > the link peer supports. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/dsa/mt7530.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index 214450378978..a472353f14f8 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port, > > { > > struct mt7530_priv *priv = ds->priv; > > u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port)); > > + struct dsa_port *dp = dsa_to_port(ds, port); > > > > e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN); > > e->tx_lpi_timer = GET_LPI_THRESH(eeecr); > > > > + if (dp->slave->phydev) > > + return phy_ethtool_get_eee(dp->slave->phydev, e); > > Given that DSA makes use of phylink, is there a reason why we can't use > the phylink wrappers here Ah, yes, phylink_foo would make a lot of sense. I will fix in the next version. > > @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port, > > set |= LPI_MODE_EN; > > mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set); > > > > + if (dp->slave->phydev) > > + return phy_ethtool_set_eee(dp->slave->phydev, e); > > return 0; > > > Is this the correct place to do the set_eee operation - I mean, the > register state has been altered (and it looks like it may enable LPI > irrespective of the negotiated state) but what concerns me is that > phy_ethtool_set_eee() can fail, and we return failure to userspace > yet we've modified register state. There is currently no consistency here between drivers. Some make this call last, some do it earlier. Thinking aloud... Why would it fail? Most error reports are that phy_read() or phy_write() failed. If that happens, the hardware is in an inconsistent state anyway. It could be the PHY does not support EEE. So an -EOPNOTSUPP or similar could be returned here. So long as phydev->eee_active is never true, it should not matter if the value of the LPI timer is changed here, it should never be put into use. Are there other cases where it could fail? Maybe we should make the order in MAC set_eee() function consistent. It is just a bigger change... Andrew
On Fri, Feb 17, 2023 at 11:48:01AM +0000, Russell King (Oracle) wrote: > On Fri, Feb 17, 2023 at 04:42:24AM +0100, Andrew Lunn wrote: > > phylib should be called in order to manage the EEE settings in the > > PHY, and to return EEE status such are supported link modes, and what > > the link peer supports. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/dsa/mt7530.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index 214450378978..a472353f14f8 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port, > > { > > struct mt7530_priv *priv = ds->priv; > > u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port)); > > + struct dsa_port *dp = dsa_to_port(ds, port); > > > > e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN); > > e->tx_lpi_timer = GET_LPI_THRESH(eeecr); > > > > + if (dp->slave->phydev) > > + return phy_ethtool_get_eee(dp->slave->phydev, e); > > Given that DSA makes use of phylink, is there a reason why we can't use > the phylink wrappers here (which may allow for future EEE improvements, > e.g. moving the gating of EEE with the TX LPI enable) ? Turns out this is pointless. dsa_slave_get_eee() and dsa_slave_set_eee() already make calls to phylink_ethtool_get_eee() and phylink_ethtool_set_eee() after calling into the DSA driver. So i will drop this patch, and the b53 change. Andrew
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 214450378978..a472353f14f8 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -3124,10 +3124,13 @@ static int mt753x_get_mac_eee(struct dsa_switch *ds, int port, { struct mt7530_priv *priv = ds->priv; u32 eeecr = mt7530_read(priv, MT7530_PMEEECR_P(port)); + struct dsa_port *dp = dsa_to_port(ds, port); e->tx_lpi_enabled = !(eeecr & LPI_MODE_EN); e->tx_lpi_timer = GET_LPI_THRESH(eeecr); + if (dp->slave->phydev) + return phy_ethtool_get_eee(dp->slave->phydev, e); return 0; } @@ -3136,6 +3139,7 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port, { struct mt7530_priv *priv = ds->priv; u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN; + struct dsa_port *dp = dsa_to_port(ds, port); if (e->tx_lpi_timer > 0xFFF) return -EINVAL; @@ -3146,6 +3150,8 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port, set |= LPI_MODE_EN; mt7530_rmw(priv, MT7530_PMEEECR_P(port), mask, set); + if (dp->slave->phydev) + return phy_ethtool_set_eee(dp->slave->phydev, e); return 0; }
phylib should be called in order to manage the EEE settings in the PHY, and to return EEE status such are supported link modes, and what the link peer supports. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/mt7530.c | 6 ++++++ 1 file changed, 6 insertions(+)