| Message ID | 20230217034230.1249661-10-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework MAC drivers EEE support | expand |
On 2/16/2023 7:42 PM, 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 > and stores if TX LPI should be enabled. 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 and the stored > tx_lpi_enabled. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> This looks similar to a number of patches for GENET that I need to resurrect against net-next, or even submit to net. LGTM at first glance, I will give you series a test. > --- > .../net/ethernet/broadcom/genet/bcmgenet.c | 31 ++++++------------- > .../net/ethernet/broadcom/genet/bcmgenet.h | 1 + > drivers/net/ethernet/broadcom/genet/bcmmii.c | 1 + > 3 files changed, 12 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index d937daa8ee88..2793d94ed32c 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -1272,12 +1272,17 @@ 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; Seems unnecessary, yes it does not quite abide by the RCT style, but no need to fix that yet. > + struct ethtool_eee *p = &priv->eee; > + bool enable; > + u32 off; > u32 reg; > > + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > + enable = eee_active && p->tx_lpi_enabled; > + > if (enable && !priv->clk_eee_enabled) { > clk_prepare_enable(priv->clk_eee); > priv->clk_eee_enabled = true; > @@ -1310,9 +1315,6 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > 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) > @@ -1326,8 +1328,7 @@ 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_enabled = p->tx_lpi_enabled; > e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER); > > return phy_ethtool_get_eee(dev->phydev, e); > @@ -1337,7 +1338,6 @@ 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 +1345,9 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) > if (!dev->phydev) > return -ENODEV; > > - p->eee_enabled = e->eee_enabled; > + p->tx_lpi_enabled = e->tx_lpi_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); > } > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h > index 946f6e283c4e..7458a62afc2c 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h > @@ -703,4 +703,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 b615176338b2..eb1747503c2e 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;
> This looks similar to a number of patches for GENET that I need to resurrect > against net-next, or even submit to net. LGTM at first glance, I will give > you series a test. Thanks for testing. > > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > > @@ -1272,12 +1272,17 @@ 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; > > Seems unnecessary, yes it does not quite abide by the RCT style, but no need > to fix that yet. Yes, it is unnecassary. I can drop it. Andrew
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index d937daa8ee88..2793d94ed32c 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1272,12 +1272,17 @@ 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; + struct ethtool_eee *p = &priv->eee; + bool enable; + u32 off; u32 reg; + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; + enable = eee_active && p->tx_lpi_enabled; + if (enable && !priv->clk_eee_enabled) { clk_prepare_enable(priv->clk_eee); priv->clk_eee_enabled = true; @@ -1310,9 +1315,6 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) 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) @@ -1326,8 +1328,7 @@ 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_enabled = p->tx_lpi_enabled; e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER); return phy_ethtool_get_eee(dev->phydev, e); @@ -1337,7 +1338,6 @@ 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 +1345,9 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) if (!dev->phydev) return -ENODEV; - p->eee_enabled = e->eee_enabled; + p->tx_lpi_enabled = e->tx_lpi_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); } diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index 946f6e283c4e..7458a62afc2c 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -703,4 +703,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 b615176338b2..eb1747503c2e 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 and stores if TX LPI should be enabled. 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 and the stored tx_lpi_enabled. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- .../net/ethernet/broadcom/genet/bcmgenet.c | 31 ++++++------------- .../net/ethernet/broadcom/genet/bcmgenet.h | 1 + drivers/net/ethernet/broadcom/genet/bcmmii.c | 1 + 3 files changed, 12 insertions(+), 21 deletions(-)