Re: [RFC/RFTv3 13/24] net: genet: Fixup EEE

[Florian Fainelli <f.fainelli@gmail.com>]

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!
-- 
Florian