| Message ID | 20230217034230.1249661-9-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:20AM +0100, Andrew Lunn wrote: > The enabling/disabling of EEE in the MAC should happen as a result of > auto negotiation. So move the enable/disable into > fec_enet_adjust_link() which gets called by phylib when there is a > change in link status. > > fec_enet_set_eee() now just stores away the LTI timer value and if TX > LPI should be enabled. Everything else is passed to phylib, so it can > correctly setup the PHY. > > fec_enet_get_eee() relies on phylib doing most of the work, > the MAC driver just adds the LTI timer value and the stored tx_lpi_enabled. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/ethernet/freescale/fec_main.c | 27 ++++------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 195df75ee614..5aca705876fe 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us) > return us * (fep->clk_ref_rate / 1000) / 1000; > } > > -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) > +static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active) > { > struct fec_enet_private *fep = netdev_priv(ndev); > struct ethtool_eee *p = &fep->eee; > unsigned int sleep_cycle, wake_cycle; > - int ret = 0; > - > - if (enable) { > - ret = phy_init_eee(ndev->phydev, false); > - if (ret) > - return ret; > > + if (eee_active && p->tx_lpi_enabled) { > sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer); > wake_cycle = sleep_cycle; > } else { > @@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) > wake_cycle = 0; > } > > - p->tx_lpi_enabled = enable; > - p->eee_enabled = enable; > - p->eee_active = enable; > - > writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP); > writel(wake_cycle, fep->hwp + FEC_LPI_WAKE); > > @@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev) > netif_tx_unlock_bh(ndev); > napi_enable(&fep->napi); > } > + fec_enet_eee_mode_set(ndev, phy_dev->eee_active); Most of iMX variants do not support EEE. It should be something like this: if (fep->quirks & FEC_QUIRK_HAS_EEE) fec_enet_eee_mode_set(ndev, phy_dev->eee_active); > } else { > if (fep->link) { > napi_disable(&fep->napi); > @@ -3109,8 +3101,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata) > if (!netif_running(ndev)) > return -ENETDOWN; > > - edata->eee_enabled = p->eee_enabled; > - edata->eee_active = p->eee_active; > edata->tx_lpi_timer = p->tx_lpi_timer; > edata->tx_lpi_enabled = p->tx_lpi_enabled; > > @@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) > { > struct fec_enet_private *fep = netdev_priv(ndev); > struct ethtool_eee *p = &fep->eee; > - int ret = 0; > > if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) > return -EOPNOTSUPP; > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) > return -ENETDOWN; > > p->tx_lpi_timer = edata->tx_lpi_timer; > - > - if (!edata->eee_enabled || !edata->tx_lpi_enabled || > - !edata->tx_lpi_timer) > - ret = fec_enet_eee_mode_set(ndev, false); > - else > - ret = fec_enet_eee_mode_set(ndev, true); > - > - if (ret) > - return ret; > + p->tx_lpi_enabled = edata->tx_lpi_enabled; Hm.. this change have effect only after link restart. Should we do something like this? if (phydev->link) fec_enet_eee_mode_set(ndev, phydev->eee_active); or, execute phy_ethtool_set_eee() first and some how detect if link changed? Or restart link by phylib on every change? > > return phy_ethtool_set_eee(ndev->phydev, edata); > } > -- > 2.39.1 > >
On Fri, Feb 17, 2023 at 09:19:43AM +0100, Oleksij Rempel wrote: > On Fri, Feb 17, 2023 at 04:42:20AM +0100, Andrew Lunn wrote: > > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) > > return -ENETDOWN; > > > > p->tx_lpi_timer = edata->tx_lpi_timer; > > - > > - if (!edata->eee_enabled || !edata->tx_lpi_enabled || > > - !edata->tx_lpi_timer) > > - ret = fec_enet_eee_mode_set(ndev, false); > > - else > > - ret = fec_enet_eee_mode_set(ndev, true); > > - > > - if (ret) > > - return ret; > > + p->tx_lpi_enabled = edata->tx_lpi_enabled; > > Hm.. this change have effect only after link restart. Should we do > something like this? > > if (phydev->link) > fec_enet_eee_mode_set(ndev, phydev->eee_active); > > or, execute phy_ethtool_set_eee() first and some how detect if link > changed? Or restart link by phylib on every change? Restarting the link on every "change" (even when nothing has changed) is a dirty hack, and can be very annoying, leading to multiple link restarts e.g. at boot time which can turn into several seconds of boot delay depending on how much is configured. I would suggest as a general principle, we should be keeping link renegotiations to a minimum - and phylib already tries to do that in several places. Also note that reading phydev->eee_active without holding the phydev mutex can be racy - and I would also ask what would prevent two calls to fec_enet_eee_mode_set() running concurrently, once by the adjust_link callback and once via the set_eee method. This applies to reading phydev->link as well, so it may be best to hold the phydev mutex around that entire if() clause.
> > @@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev) > > netif_tx_unlock_bh(ndev); > > napi_enable(&fep->napi); > > } > > + fec_enet_eee_mode_set(ndev, phy_dev->eee_active); > > Most of iMX variants do not support EEE. It should be something like this: > if (fep->quirks & FEC_QUIRK_HAS_EEE) > fec_enet_eee_mode_set(ndev, phy_dev->eee_active); Yes, i missed that. Thanks. > > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) > > return -ENETDOWN; > > > > p->tx_lpi_timer = edata->tx_lpi_timer; > > - > > - if (!edata->eee_enabled || !edata->tx_lpi_enabled || > > - !edata->tx_lpi_timer) > > - ret = fec_enet_eee_mode_set(ndev, false); > > - else > > - ret = fec_enet_eee_mode_set(ndev, true); > > - > > - if (ret) > > - return ret; > > + p->tx_lpi_enabled = edata->tx_lpi_enabled; > > Hm.. this change have effect only after link restart. Should we do > something like this? > > if (phydev->link) > fec_enet_eee_mode_set(ndev, phydev->eee_active); > > or, execute phy_ethtool_set_eee() first and some how detect if link > changed? Or restart link by phylib on every change? The whole startup sequence needs looking at, and ties in with the phy_supports_eee() call we need to add. Given that EEE is broken with most MAC drivers, i thought we could do that in a follow up patch series. As Russell says, we want to avoid multiple auto-neg cycles. Ideally we want phy_supports_eee() to be called before phy_start() so that EEE advertisement is set correctly for the first auto-neg. What is missing from many MAC drivers is a default value from the LPI timer. It seems like the need eee_set() has to be called with a value, or they are relying on hardware reset values. Maybe we need to define a default? We also need to discuss policy of if EEE should be enabled by default for those systems which support it. As you have pointed out, it can effect PTP quality. Andrew
> > p->tx_lpi_timer = edata->tx_lpi_timer; > > - > > - if (!edata->eee_enabled || !edata->tx_lpi_enabled || > > - !edata->tx_lpi_timer) > > - ret = fec_enet_eee_mode_set(ndev, false); > > - else > > - ret = fec_enet_eee_mode_set(ndev, true); > > - > > - if (ret) > > - return ret; > > + p->tx_lpi_enabled = edata->tx_lpi_enabled; > > Hm.. this change have effect only after link restart. Should we do > something like this? I think moving tx_lpi_enabled into phylib will help here. phylib can track if only this changes, and then call the adjust_link callback without actually doing an auto neg is only that changes. Andrew
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 195df75ee614..5aca705876fe 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1930,18 +1930,13 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us) return us * (fep->clk_ref_rate / 1000) / 1000; } -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) +static int fec_enet_eee_mode_set(struct net_device *ndev, bool eee_active) { struct fec_enet_private *fep = netdev_priv(ndev); struct ethtool_eee *p = &fep->eee; unsigned int sleep_cycle, wake_cycle; - int ret = 0; - - if (enable) { - ret = phy_init_eee(ndev->phydev, false); - if (ret) - return ret; + if (eee_active && p->tx_lpi_enabled) { sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer); wake_cycle = sleep_cycle; } else { @@ -1949,10 +1944,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) wake_cycle = 0; } - p->tx_lpi_enabled = enable; - p->eee_enabled = enable; - p->eee_active = enable; - writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP); writel(wake_cycle, fep->hwp + FEC_LPI_WAKE); @@ -1997,6 +1988,7 @@ static void fec_enet_adjust_link(struct net_device *ndev) netif_tx_unlock_bh(ndev); napi_enable(&fep->napi); } + fec_enet_eee_mode_set(ndev, phy_dev->eee_active); } else { if (fep->link) { napi_disable(&fep->napi); @@ -3109,8 +3101,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata) if (!netif_running(ndev)) return -ENETDOWN; - edata->eee_enabled = p->eee_enabled; - edata->eee_active = p->eee_active; edata->tx_lpi_timer = p->tx_lpi_timer; edata->tx_lpi_enabled = p->tx_lpi_enabled; @@ -3122,7 +3112,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) { struct fec_enet_private *fep = netdev_priv(ndev); struct ethtool_eee *p = &fep->eee; - int ret = 0; if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) return -EOPNOTSUPP; @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) return -ENETDOWN; p->tx_lpi_timer = edata->tx_lpi_timer; - - if (!edata->eee_enabled || !edata->tx_lpi_enabled || - !edata->tx_lpi_timer) - ret = fec_enet_eee_mode_set(ndev, false); - else - ret = fec_enet_eee_mode_set(ndev, true); - - if (ret) - return ret; + p->tx_lpi_enabled = edata->tx_lpi_enabled; return phy_ethtool_set_eee(ndev->phydev, edata); }
The enabling/disabling of EEE in the MAC should happen as a result of auto negotiation. So move the enable/disable into fec_enet_adjust_link() which gets called by phylib when there is a change in link status. fec_enet_set_eee() now just stores away the LTI timer value and if TX LPI should be enabled. Everything else is passed to phylib, so it can correctly setup the PHY. fec_enet_get_eee() relies on phylib doing most of the work, the MAC driver just adds the LTI timer value and the stored tx_lpi_enabled. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/freescale/fec_main.c | 27 ++++------------------- 1 file changed, 4 insertions(+), 23 deletions(-)