| Message ID | 20230331005518.2134652-14-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On 3/30/23 17:55, 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 bcmgenet_mii_setup() > which gets called by phylib when there is a change in link status. > > bcmgenet_set_eee() now just writes the LTI timer value to the > hardware. Everything else is passed to phylib, so it can correctly > setup the PHY. > > bcmgenet_get_eee() relies on phylib doing most of the work, the MAC > driver just adds the LTI timer value from hardware. > > The call to bcmgenet_eee_enable_set() in the resume function has been > removed. There is both unconditional calls to phy_init_hw() and > genphy_config_aneg, and a call to phy_resume(). As a result, the PHY > is going to perform auto-neg, and then it completes > bcmgenet_mii_setup() will be called, which will set the hardware to > the correct EEE mode. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > .../net/ethernet/broadcom/genet/bcmgenet.c | 42 +++++-------------- > .../net/ethernet/broadcom/genet/bcmgenet.h | 3 +- > drivers/net/ethernet/broadcom/genet/bcmmii.c | 1 + > 3 files changed, 12 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index d937daa8ee88..035486304e31 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -1272,19 +1272,21 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, > } > } > > -static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active) Replacing the argument name here is a bit of noise in reviewing the patch, and it does not fundamentally change the behavior or semantics IMHO. > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > + u32 off; > u32 reg; > > - if (enable && !priv->clk_eee_enabled) { > + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > + > + if (eee_active && !priv->clk_eee_enabled) { > clk_prepare_enable(priv->clk_eee); > priv->clk_eee_enabled = true; > } > > reg = bcmgenet_umac_readl(priv, UMAC_EEE_CTRL); > - if (enable) > + if (eee_active) > reg |= EEE_EN; > else > reg &= ~EEE_EN; > @@ -1292,7 +1294,7 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > > /* Enable EEE and switch to a 27Mhz clock automatically */ > reg = bcmgenet_readl(priv->base + off); > - if (enable) > + if (eee_active) > reg |= TBUF_EEE_EN | TBUF_PM_EN; > else > reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); > @@ -1300,25 +1302,21 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > > /* Do the same for thing for RBUF */ > reg = bcmgenet_rbuf_readl(priv, RBUF_ENERGY_CTRL); > - if (enable) > + if (eee_active) > reg |= RBUF_EEE_EN | RBUF_PM_EN; > else > reg &= ~(RBUF_EEE_EN | RBUF_PM_EN); > bcmgenet_rbuf_writel(priv, reg, RBUF_ENERGY_CTRL); > > - if (!enable && priv->clk_eee_enabled) { > + if (!eee_active && priv->clk_eee_enabled) { > clk_disable_unprepare(priv->clk_eee); > priv->clk_eee_enabled = false; > } > - > - priv->eee.eee_enabled = enable; > - priv->eee.eee_active = enable; > } > > static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - struct ethtool_eee *p = &priv->eee; > > if (GENET_IS_V1(priv)) > return -EOPNOTSUPP; > @@ -1326,8 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) > if (!dev->phydev) > return -ENODEV; > > - e->eee_enabled = p->eee_enabled; > - e->eee_active = p->eee_active; > e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER); > > return phy_ethtool_get_eee(dev->phydev, e); > @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) > static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - struct ethtool_eee *p = &priv->eee; > - int ret = 0; > > if (GENET_IS_V1(priv)) > return -EOPNOTSUPP; > @@ -1345,20 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) > if (!dev->phydev) > return -ENODEV; > > - p->eee_enabled = e->eee_enabled; > - > - if (!p->eee_enabled) { > - bcmgenet_eee_enable_set(dev, false); > - } else { > - ret = phy_init_eee(dev->phydev, false); > - if (ret) { > - netif_err(priv, hw, dev, "EEE initialization failed\n"); > - return ret; > - } > - > - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); > - bcmgenet_eee_enable_set(dev, true); > - } > + bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); > > return phy_ethtool_set_eee(dev->phydev, e); > } > @@ -4278,9 +4259,6 @@ static int bcmgenet_resume(struct device *d) > if (!device_may_wakeup(d)) > phy_resume(dev->phydev); > > - if (priv->eee.eee_enabled) > - bcmgenet_eee_enable_set(dev, true); > - > bcmgenet_netif_start(dev); > > netif_device_attach(dev); > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h > index 946f6e283c4e..8c9643ec738c 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h > @@ -644,8 +644,6 @@ struct bcmgenet_priv { > bool wol_active; > > struct bcmgenet_mib_counters mib; > - > - struct ethtool_eee eee; > }; > > #define GENET_IO_MACRO(name, offset) \ > @@ -703,4 +701,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, > void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv, > enum bcmgenet_power_mode mode); > > +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active); > #endif /* __BCMGENET_H__ */ > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > index be042905ada2..6c39839762a7 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > @@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev) > > if (phydev->link) { > bcmgenet_mac_config(dev); > + bcmgenet_eee_enable_set(dev, phydev->eee_active); That part is a real bug fix, I do have a tentative patch that I should be able to submit to 'net' soon after I finish testing a few things with it. Thanks Andrew!
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index d937daa8ee88..035486304e31 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1272,19 +1272,21 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, } } -static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active) { struct bcmgenet_priv *priv = netdev_priv(dev); - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; + u32 off; u32 reg; - if (enable && !priv->clk_eee_enabled) { + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; + + if (eee_active && !priv->clk_eee_enabled) { clk_prepare_enable(priv->clk_eee); priv->clk_eee_enabled = true; } reg = bcmgenet_umac_readl(priv, UMAC_EEE_CTRL); - if (enable) + if (eee_active) reg |= EEE_EN; else reg &= ~EEE_EN; @@ -1292,7 +1294,7 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) /* Enable EEE and switch to a 27Mhz clock automatically */ reg = bcmgenet_readl(priv->base + off); - if (enable) + if (eee_active) reg |= TBUF_EEE_EN | TBUF_PM_EN; else reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); @@ -1300,25 +1302,21 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) /* Do the same for thing for RBUF */ reg = bcmgenet_rbuf_readl(priv, RBUF_ENERGY_CTRL); - if (enable) + if (eee_active) reg |= RBUF_EEE_EN | RBUF_PM_EN; else reg &= ~(RBUF_EEE_EN | RBUF_PM_EN); bcmgenet_rbuf_writel(priv, reg, RBUF_ENERGY_CTRL); - if (!enable && priv->clk_eee_enabled) { + if (!eee_active && priv->clk_eee_enabled) { clk_disable_unprepare(priv->clk_eee); priv->clk_eee_enabled = false; } - - priv->eee.eee_enabled = enable; - priv->eee.eee_active = enable; } static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) { struct bcmgenet_priv *priv = netdev_priv(dev); - struct ethtool_eee *p = &priv->eee; if (GENET_IS_V1(priv)) return -EOPNOTSUPP; @@ -1326,8 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) if (!dev->phydev) return -ENODEV; - e->eee_enabled = p->eee_enabled; - e->eee_active = p->eee_active; e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER); return phy_ethtool_get_eee(dev->phydev, e); @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) { struct bcmgenet_priv *priv = netdev_priv(dev); - struct ethtool_eee *p = &priv->eee; - int ret = 0; if (GENET_IS_V1(priv)) return -EOPNOTSUPP; @@ -1345,20 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) if (!dev->phydev) return -ENODEV; - p->eee_enabled = e->eee_enabled; - - if (!p->eee_enabled) { - bcmgenet_eee_enable_set(dev, false); - } else { - ret = phy_init_eee(dev->phydev, false); - if (ret) { - netif_err(priv, hw, dev, "EEE initialization failed\n"); - return ret; - } - - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); - bcmgenet_eee_enable_set(dev, true); - } + bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); return phy_ethtool_set_eee(dev->phydev, e); } @@ -4278,9 +4259,6 @@ static int bcmgenet_resume(struct device *d) if (!device_may_wakeup(d)) phy_resume(dev->phydev); - if (priv->eee.eee_enabled) - bcmgenet_eee_enable_set(dev, true); - bcmgenet_netif_start(dev); netif_device_attach(dev); diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index 946f6e283c4e..8c9643ec738c 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -644,8 +644,6 @@ struct bcmgenet_priv { bool wol_active; struct bcmgenet_mib_counters mib; - - struct ethtool_eee eee; }; #define GENET_IO_MACRO(name, offset) \ @@ -703,4 +701,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv, enum bcmgenet_power_mode mode); +void bcmgenet_eee_enable_set(struct net_device *dev, bool eee_active); #endif /* __BCMGENET_H__ */ diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index be042905ada2..6c39839762a7 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -100,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev) if (phydev->link) { bcmgenet_mac_config(dev); + bcmgenet_eee_enable_set(dev, phydev->eee_active); } else { reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL); reg &= ~RGMII_LINK;
The enabling/disabling of EEE in the MAC should happen as a result of auto negotiation. So move the enable/disable into bcmgenet_mii_setup() which gets called by phylib when there is a change in link status. bcmgenet_set_eee() now just writes the LTI timer value to the hardware. Everything else is passed to phylib, so it can correctly setup the PHY. bcmgenet_get_eee() relies on phylib doing most of the work, the MAC driver just adds the LTI timer value from hardware. The call to bcmgenet_eee_enable_set() in the resume function has been removed. There is both unconditional calls to phy_init_hw() and genphy_config_aneg, and a call to phy_resume(). As a result, the PHY is going to perform auto-neg, and then it completes bcmgenet_mii_setup() will be called, which will set the hardware to the correct EEE mode. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- .../net/ethernet/broadcom/genet/bcmgenet.c | 42 +++++-------------- .../net/ethernet/broadcom/genet/bcmgenet.h | 3 +- drivers/net/ethernet/broadcom/genet/bcmmii.c | 1 + 3 files changed, 12 insertions(+), 34 deletions(-)