| Message ID | 20240408-stmmac-eee-v1-1-3d65d671c06b@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: genet: Fixup EEE | expand |
On 4/8/24 17:54, 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 LPI 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 LPI timer value from hardware. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++-------------------- > drivers/net/ethernet/broadcom/genet/bcmgenet.h | 6 +----- > drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------ > 3 files changed, 8 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index b1f84b37032a..c090b519255a 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -1272,13 +1272,14 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, > } > } > > -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable, > - bool tx_lpi_enabled) > +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > + u32 off; > u32 reg; > > + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > + > if (enable && !priv->clk_eee_enabled) { > clk_prepare_enable(priv->clk_eee); > priv->clk_eee_enabled = true; > @@ -1293,7 +1294,7 @@ 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 (tx_lpi_enabled) > + if (enable) > reg |= TBUF_EEE_EN | TBUF_PM_EN; > else > reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); > @@ -1311,15 +1312,11 @@ 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.tx_lpi_enabled = tx_lpi_enabled; > } > > static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - struct ethtool_keee *p = &priv->eee; > > if (GENET_IS_V1(priv)) > return -EOPNOTSUPP; > @@ -1327,7 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > if (!dev->phydev) > return -ENODEV; > > - 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); > @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) > { > struct bcmgenet_priv *priv = netdev_priv(dev); > - struct ethtool_keee *p = &priv->eee; > - bool active; > > if (GENET_IS_V1(priv)) > return -EOPNOTSUPP; > @@ -1345,15 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) > if (!dev->phydev) > return -ENODEV; > > - p->eee_enabled = e->eee_enabled; > - > - if (!p->eee_enabled) { > - bcmgenet_eee_enable_set(dev, false, false); > - } else { > - active = phy_init_eee(dev->phydev, false) >= 0; > - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); > - bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); > - } > + 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 7523b60b3c1c..bb82ecb2e220 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_keee eee; > }; > > #define GENET_IO_MACRO(name, offset) \ > @@ -703,7 +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 enable, > - bool tx_lpi_enabled); > - > +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable); > #endif /* __BCMGENET_H__ */ > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > index 9ada89355747..25eeea4c1965 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > @@ -30,7 +30,6 @@ static void bcmgenet_mac_config(struct net_device *dev) > struct bcmgenet_priv *priv = netdev_priv(dev); > struct phy_device *phydev = dev->phydev; > u32 reg, cmd_bits = 0; > - bool active; > > /* speed */ > if (phydev->speed == SPEED_1000) > @@ -88,11 +87,6 @@ static void bcmgenet_mac_config(struct net_device *dev) > reg |= CMD_TX_EN | CMD_RX_EN; > } > bcmgenet_umac_writel(priv, reg, UMAC_CMD); > - > - active = phy_init_eee(phydev, 0) >= 0; > - bcmgenet_eee_enable_set(dev, > - priv->eee.eee_enabled && active, > - priv->eee.tx_lpi_enabled); You can keep the call to bcmgenet_eee_enable_set() where it currently is and just change the arguments?
On Tue, Apr 09, 2024 at 01:41:43PM -0700, Florian Fainelli wrote: > On 4/8/24 17:54, 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 LPI 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 LPI timer value from hardware. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++-------------------- > > drivers/net/ethernet/broadcom/genet/bcmgenet.h | 6 +----- > > drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------ > > 3 files changed, 8 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > > index b1f84b37032a..c090b519255a 100644 > > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > > @@ -1272,13 +1272,14 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, > > } > > } > > -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable, > > - bool tx_lpi_enabled) > > +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) > > { > > struct bcmgenet_priv *priv = netdev_priv(dev); > > - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > > + u32 off; > > u32 reg; > > + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; > > + > > if (enable && !priv->clk_eee_enabled) { > > clk_prepare_enable(priv->clk_eee); > > priv->clk_eee_enabled = true; > > @@ -1293,7 +1294,7 @@ 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 (tx_lpi_enabled) > > + if (enable) > > reg |= TBUF_EEE_EN | TBUF_PM_EN; > > else > > reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); > > @@ -1311,15 +1312,11 @@ 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.tx_lpi_enabled = tx_lpi_enabled; > > } > > static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > > { > > struct bcmgenet_priv *priv = netdev_priv(dev); > > - struct ethtool_keee *p = &priv->eee; > > if (GENET_IS_V1(priv)) > > return -EOPNOTSUPP; > > @@ -1327,7 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > > if (!dev->phydev) > > return -ENODEV; > > - 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); > > @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) > > static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) > > { > > struct bcmgenet_priv *priv = netdev_priv(dev); > > - struct ethtool_keee *p = &priv->eee; > > - bool active; > > if (GENET_IS_V1(priv)) > > return -EOPNOTSUPP; > > @@ -1345,15 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) > > if (!dev->phydev) > > return -ENODEV; > > - p->eee_enabled = e->eee_enabled; > > - > > - if (!p->eee_enabled) { > > - bcmgenet_eee_enable_set(dev, false, false); > > - } else { > > - active = phy_init_eee(dev->phydev, false) >= 0; > > - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); > > - bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); > > - } > > + 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 7523b60b3c1c..bb82ecb2e220 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_keee eee; > > }; > > #define GENET_IO_MACRO(name, offset) \ > > @@ -703,7 +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 enable, > > - bool tx_lpi_enabled); > > - > > +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable); > > #endif /* __BCMGENET_H__ */ > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c > > index 9ada89355747..25eeea4c1965 100644 > > --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c > > +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c > > @@ -30,7 +30,6 @@ static void bcmgenet_mac_config(struct net_device *dev) > > struct bcmgenet_priv *priv = netdev_priv(dev); > > struct phy_device *phydev = dev->phydev; > > u32 reg, cmd_bits = 0; > > - bool active; > > /* speed */ > > if (phydev->speed == SPEED_1000) > > @@ -88,11 +87,6 @@ static void bcmgenet_mac_config(struct net_device *dev) > > reg |= CMD_TX_EN | CMD_RX_EN; > > } > > bcmgenet_umac_writel(priv, reg, UMAC_CMD); > > - > > - active = phy_init_eee(phydev, 0) >= 0; > > - bcmgenet_eee_enable_set(dev, > > - priv->eee.eee_enabled && active, > > - priv->eee.tx_lpi_enabled); > > You can keep the call to bcmgenet_eee_enable_set() where it currently is and > just change the arguments? Hi Florian bcmgenet_eee_enable_set() configures the hardware. We should only call that once auto-neg has completed and the adjust_link callback is called. Or in the case EEE autoneg is disabled, phylib will artificially down/up the link in order to call the adjust_link callback. bcmgenet_eee_enable_set() is currently called in bcmgenet_set_eee(), which is the .set_eee ethtool_op. So that is the wrong place to do this. That is what the last hunk in the patch does, it moves it to bcmgenet_mii_setup() which is passed to of_phy_connect() as the callback. Or am i missing thing? Andrew
Hi Andrew, On 4/9/2024 2:37 PM, Andrew Lunn wrote: > On Tue, Apr 09, 2024 at 01:41:43PM -0700, Florian Fainelli wrote: >> On 4/8/24 17:54, 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 LPI 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 LPI timer value from hardware. >>> >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch> >>> --- >>> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++-------------------- >>> drivers/net/ethernet/broadcom/genet/bcmgenet.h | 6 +----- >>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------ >>> 3 files changed, 8 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> index b1f84b37032a..c090b519255a 100644 >>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> @@ -1272,13 +1272,14 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, >>> } >>> } >>> -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable, >>> - bool tx_lpi_enabled) >>> +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) >>> { >>> struct bcmgenet_priv *priv = netdev_priv(dev); >>> - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; >>> + u32 off; >>> u32 reg; >>> + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; >>> + >>> if (enable && !priv->clk_eee_enabled) { >>> clk_prepare_enable(priv->clk_eee); >>> priv->clk_eee_enabled = true; >>> @@ -1293,7 +1294,7 @@ 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 (tx_lpi_enabled) >>> + if (enable) >>> reg |= TBUF_EEE_EN | TBUF_PM_EN; >>> else >>> reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); >>> @@ -1311,15 +1312,11 @@ 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.tx_lpi_enabled = tx_lpi_enabled; >>> } >>> static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) >>> { >>> struct bcmgenet_priv *priv = netdev_priv(dev); >>> - struct ethtool_keee *p = &priv->eee; >>> if (GENET_IS_V1(priv)) >>> return -EOPNOTSUPP; >>> @@ -1327,7 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) >>> if (!dev->phydev) >>> return -ENODEV; >>> - 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); >>> @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) >>> static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) >>> { >>> struct bcmgenet_priv *priv = netdev_priv(dev); >>> - struct ethtool_keee *p = &priv->eee; >>> - bool active; >>> if (GENET_IS_V1(priv)) >>> return -EOPNOTSUPP; >>> @@ -1345,15 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) >>> if (!dev->phydev) >>> return -ENODEV; >>> - p->eee_enabled = e->eee_enabled; >>> - >>> - if (!p->eee_enabled) { >>> - bcmgenet_eee_enable_set(dev, false, false); >>> - } else { >>> - active = phy_init_eee(dev->phydev, false) >= 0; >>> - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); >>> - bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); >>> - } >>> + 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 7523b60b3c1c..bb82ecb2e220 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_keee eee; >>> }; >>> #define GENET_IO_MACRO(name, offset) \ >>> @@ -703,7 +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 enable, >>> - bool tx_lpi_enabled); >>> - >>> +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable); >>> #endif /* __BCMGENET_H__ */ >>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> index 9ada89355747..25eeea4c1965 100644 >>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c >>> @@ -30,7 +30,6 @@ static void bcmgenet_mac_config(struct net_device *dev) >>> struct bcmgenet_priv *priv = netdev_priv(dev); >>> struct phy_device *phydev = dev->phydev; >>> u32 reg, cmd_bits = 0; >>> - bool active; >>> /* speed */ >>> if (phydev->speed == SPEED_1000) >>> @@ -88,11 +87,6 @@ static void bcmgenet_mac_config(struct net_device *dev) >>> reg |= CMD_TX_EN | CMD_RX_EN; >>> } >>> bcmgenet_umac_writel(priv, reg, UMAC_CMD); >>> - >>> - active = phy_init_eee(phydev, 0) >= 0; >>> - bcmgenet_eee_enable_set(dev, >>> - priv->eee.eee_enabled && active, >>> - priv->eee.tx_lpi_enabled); >> >> You can keep the call to bcmgenet_eee_enable_set() where it currently is and >> just change the arguments? > > Hi Florian > > bcmgenet_eee_enable_set() configures the hardware. We should only call > that once auto-neg has completed and the adjust_link callback is > called. Or in the case EEE autoneg is disabled, phylib will > artificially down/up the link in order to call the adjust_link > callback. > > bcmgenet_eee_enable_set() is currently called in bcmgenet_set_eee(), > which is the .set_eee ethtool_op. So that is the wrong place to do > this. That is what the last hunk in the patch does, it moves it to > bcmgenet_mii_setup() which is passed to of_phy_connect() as the > callback. > > Or am i missing thing? You are not, I was missing the two call sites for bcmgenet_mac_config() and indeed only the one in bcmgenet_mii_setup() is meaningful here. I am seeing a functional difference with and without your patch however, and also, there appears to be something wrong within the bcmgenet driver after PHYLIB having absorbed the EEE configuration. Both cases we start on boot with: # ethtool --show-eee eth0 EEE settings for eth0: EEE status: disabled Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full I would expect the EEE status to be enabled, that's how I remember it before. Now, with your patch, once I turn on EEE with: # ethtool --set-eee eth0 eee on # ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full # there is no change to the EEE_CTRL register to set the EEE_EN, this only happens when doing: # ethtool --set-eee eth0 eee on tx-lpi on which is consistent with the patch, but I don't think this is quite correct as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi on" meant enable EEE for the TX path?
Hi Florian, On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote: > I am seeing a functional difference with and without your patch however, and > also, there appears to be something wrong within the bcmgenet driver after > PHYLIB having absorbed the EEE configuration. Both cases we start on boot > with: > > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: disabled > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > > I would expect the EEE status to be enabled, that's how I remember it > before. Yes, current default kernel implementation is to use EEE if available. > Now, with your patch, once I turn on EEE with: > > # ethtool --set-eee eth0 eee on > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: enabled - active > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > # > > there is no change to the EEE_CTRL register to set the EEE_EN, this only > happens when doing: > > # ethtool --set-eee eth0 eee on tx-lpi on > > which is consistent with the patch, but I don't think this is quite correct > as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi > on" meant enable EEE for the TX path? Yes. More precisely, with "eee on" we allow the PHY to advertise EEE link modes. On link_up, if both sides are agreed to use EEE, MAC is configured to process LPI opcodes from the PHY and send LPI opcodes to the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in case of "eee off". What is exact meaning of EEE_EN on this chip? Regards, Oleksij
On Thu, Apr 11, 2024 at 07:17:36AM +0200, Oleksij Rempel wrote: > Hi Florian, > > On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote: > > > I am seeing a functional difference with and without your patch however, and > > also, there appears to be something wrong within the bcmgenet driver after > > PHYLIB having absorbed the EEE configuration. Both cases we start on boot > > with: > > > > # ethtool --show-eee eth0 > > EEE settings for eth0: > > EEE status: disabled > > Tx LPI: disabled > > Supported EEE link modes: 100baseT/Full > > 1000baseT/Full > > Advertised EEE link modes: 100baseT/Full > > 1000baseT/Full > > Link partner advertised EEE link modes: 100baseT/Full > > 1000baseT/Full > > > > I would expect the EEE status to be enabled, that's how I remember it > > before. > > Yes, current default kernel implementation is to use EEE if available. We do however seem to be inconsistent in this example. EEE seems to be disabled, yet it is advertising? Or is it showing what we would advertise, when EEE is enabled? > > Now, with your patch, once I turn on EEE with: > > > > # ethtool --set-eee eth0 eee on > > # ethtool --show-eee eth0 > > EEE settings for eth0: > > EEE status: enabled - active > > Tx LPI: disabled > > Supported EEE link modes: 100baseT/Full > > 1000baseT/Full > > Advertised EEE link modes: 100baseT/Full > > 1000baseT/Full > > Link partner advertised EEE link modes: 100baseT/Full > > 1000baseT/Full > > # > > > > there is no change to the EEE_CTRL register to set the EEE_EN, this only > > happens when doing: > > > > # ethtool --set-eee eth0 eee on tx-lpi on > > > > which is consistent with the patch, but I don't think this is quite correct > > as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi > > on" meant enable EEE for the TX path? > > Yes. More precisely, with "eee on" we allow the PHY to advertise EEE > link modes. On link_up, if both sides are agreed to use EEE, MAC is > configured to process LPI opcodes from the PHY and send LPI opcodes to > the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in > case of "eee off". Florian seems to be suggesting the RX and TX path could have different configurations? RX EEE could be enabled but TX EEE disabled? That i don't understand, in terms of auto-neg. auto-neg is for the link as a whole, it does not appear to allow different results for each direction. Does tx-lpi only make sense when EEE is forced, not auto-neg'ed? Andrew
On 4/11/24 07:35, Andrew Lunn wrote: > On Thu, Apr 11, 2024 at 07:17:36AM +0200, Oleksij Rempel wrote: >> Hi Florian, >> >> On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote: >> >>> I am seeing a functional difference with and without your patch however, and >>> also, there appears to be something wrong within the bcmgenet driver after >>> PHYLIB having absorbed the EEE configuration. Both cases we start on boot >>> with: >>> >>> # ethtool --show-eee eth0 >>> EEE settings for eth0: >>> EEE status: disabled >>> Tx LPI: disabled >>> Supported EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> Advertised EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> Link partner advertised EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> >>> I would expect the EEE status to be enabled, that's how I remember it >>> before. >> >> Yes, current default kernel implementation is to use EEE if available. > > We do however seem to be inconsistent in this example. EEE seems to be > disabled, yet it is advertising? Or is it showing what we would > advertise, when EEE is enabled? What I consider to be the "canonical" behavior is the following on boot: # ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full whereby we advertise EEE, the link partner does too, and the adjust_link callback determined that we could EEE as a result via phy_init_eee(). This is seen on 6.8. Starting with 6.9-rc and Andrew's series to rework EEE, I have the behavior provided before: # ethtool --show-eee eth0 EEE settings for eth0: EEE status: disabled Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full whereby we need user intervention to opt-in and have EEE enabled with: ethtool --set-eee eth0 eee on This presents users with a difference in behavior which we might get regression reports for. > >>> Now, with your patch, once I turn on EEE with: >>> >>> # ethtool --set-eee eth0 eee on >>> # ethtool --show-eee eth0 >>> EEE settings for eth0: >>> EEE status: enabled - active >>> Tx LPI: disabled >>> Supported EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> Advertised EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> Link partner advertised EEE link modes: 100baseT/Full >>> 1000baseT/Full >>> # >>> >>> there is no change to the EEE_CTRL register to set the EEE_EN, this only >>> happens when doing: >>> >>> # ethtool --set-eee eth0 eee on tx-lpi on >>> >>> which is consistent with the patch, but I don't think this is quite correct >>> as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi >>> on" meant enable EEE for the TX path? >> >> Yes. More precisely, with "eee on" we allow the PHY to advertise EEE >> link modes. On link_up, if both sides are agreed to use EEE, MAC is >> configured to process LPI opcodes from the PHY and send LPI opcodes to >> the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in >> case of "eee off". > > Florian seems to be suggesting the RX and TX path could have different > configurations? RX EEE could be enabled but TX EEE disabled? That i > don't understand, in terms of auto-neg. auto-neg is for the link as a > whole, it does not appear to allow different results for each > direction. Does tx-lpi only make sense when EEE is forced, not > auto-neg'ed? To me the 'tx-lpi' parameter allows for an additional level of local control of whether TX path should sent LPI or not, irrespective of forced versus auto-negotiated EEE capability. I am not sure why the API was defined like it was in the first place and what was the rationale for offering a separate 'tx-lpi', this might have been based upon a real or hypothetical use case. If we are to honor the separate controls, we would have to agree on their meaning and we had discussed this before with Oleksij. EEE_EN means that the Ethernet MAC (called UNIMAC in this block) enables clock gating signaling from the PHY up to DMA interface, this is how the power savings are actually realized on the digital logic side.
On Thu, Apr 11, 2024 at 10:04:13AM -0700, Florian Fainelli wrote: > On 4/11/24 07:35, Andrew Lunn wrote: > > On Thu, Apr 11, 2024 at 07:17:36AM +0200, Oleksij Rempel wrote: > > > Hi Florian, > > > > > > On Wed, Apr 10, 2024 at 10:48:26AM -0700, Florian Fainelli wrote: > > > > > > > I am seeing a functional difference with and without your patch however, and > > > > also, there appears to be something wrong within the bcmgenet driver after > > > > PHYLIB having absorbed the EEE configuration. Both cases we start on boot > > > > with: > > > > > > > > # ethtool --show-eee eth0 > > > > EEE settings for eth0: > > > > EEE status: disabled > > > > Tx LPI: disabled > > > > Supported EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Link partner advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > > > > > I would expect the EEE status to be enabled, that's how I remember it > > > > before. > > > > > > Yes, current default kernel implementation is to use EEE if available. > > > > We do however seem to be inconsistent in this example. EEE seems to be > > disabled, yet it is advertising? Or is it showing what we would > > advertise, when EEE is enabled? > > What I consider to be the "canonical" behavior is the following on boot: > > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: enabled - active > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > > whereby we advertise EEE, the link partner does too, and the adjust_link > callback determined that we could EEE as a result via phy_init_eee(). This > is seen on 6.8. > > Starting with 6.9-rc and Andrew's series to rework EEE, I have the behavior > provided before: > > # ethtool --show-eee eth0 > EEE settings for eth0: > EEE status: disabled > Tx LPI: disabled > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: 100baseT/Full > 1000baseT/Full > Link partner advertised EEE link modes: 100baseT/Full > 1000baseT/Full > > whereby we need user intervention to opt-in and have EEE enabled with: > > ethtool --set-eee eth0 eee on > > This presents users with a difference in behavior which we might get > regression reports for. The phy_support_eee() is missing. This should be added on phy attach if EEE is supported by MAC. > > > > > > Now, with your patch, once I turn on EEE with: > > > > > > > > # ethtool --set-eee eth0 eee on > > > > # ethtool --show-eee eth0 > > > > EEE settings for eth0: > > > > EEE status: enabled - active > > > > Tx LPI: disabled > > > > Supported EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > Link partner advertised EEE link modes: 100baseT/Full > > > > 1000baseT/Full > > > > # > > > > > > > > there is no change to the EEE_CTRL register to set the EEE_EN, this only > > > > happens when doing: > > > > > > > > # ethtool --set-eee eth0 eee on tx-lpi on > > > > > > > > which is consistent with the patch, but I don't think this is quite correct > > > > as I remembered that "eee on" meant enable EEE for the RX path, and "tx-lpi > > > > on" meant enable EEE for the TX path? > > > > > > Yes. More precisely, with "eee on" we allow the PHY to advertise EEE > > > link modes. On link_up, if both sides are agreed to use EEE, MAC is > > > configured to process LPI opcodes from the PHY and send LPI opcodes to > > > the PHY if "tx-lpi on" was configured too. tx-lpi will not be enabled in > > > case of "eee off". > > > > Florian seems to be suggesting the RX and TX path could have different > > configurations? RX EEE could be enabled but TX EEE disabled? That i > > don't understand, in terms of auto-neg. auto-neg is for the link as a > > whole, it does not appear to allow different results for each > > direction. Does tx-lpi only make sense when EEE is forced, not > > auto-neg'ed? > > To me the 'tx-lpi' parameter allows for an additional level of local control > of whether TX path should sent LPI or not, irrespective of forced versus > auto-negotiated EEE capability. Hm.. many of combined devices like MACs with integrated PHYs or PHYs with integrated EEE-LPI support (SmartEEE...) do hardware EEE with autoneg. configuring tx-lpi without "eee on" would not bring much. > I am not sure why the API was defined like it was in the first place and > what was the rationale for offering a separate 'tx-lpi', this might have > been based upon a real or hypothetical use case. > > If we are to honor the separate controls, we would have to agree on their > meaning and we had discussed this before with Oleksij. I can imagine, this can be used to optimize for some traffic use cases. "tx-lpi off" is in general equal to "tx-timer 0" or tx-timer some big number. May be, it was a workaround, since recommended initial tx-timer value depend on the linkspeed. At least some of microchip devices have special recommendations about it. > EEE_EN means that the Ethernet MAC (called UNIMAC in this block) enables > clock gating signaling from the PHY up to DMA interface, this is how the > power savings are actually realized on the digital logic side. Hm.. so, on this MAC, at leas partial LPI mode will still work without EEE_EN? Beside, I see there is UMAC_EEE_WAKE_TIMER, which is not configured. This one is very important, the wake time depend on the link speed, the PHY and potentially link partner. If not properly configure, EEE may fail. Regards, Oleksij
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index b1f84b37032a..c090b519255a 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1272,13 +1272,14 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev, } } -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable, - bool tx_lpi_enabled) +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) { struct bcmgenet_priv *priv = netdev_priv(dev); - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; + u32 off; u32 reg; + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; + if (enable && !priv->clk_eee_enabled) { clk_prepare_enable(priv->clk_eee); priv->clk_eee_enabled = true; @@ -1293,7 +1294,7 @@ 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 (tx_lpi_enabled) + if (enable) reg |= TBUF_EEE_EN | TBUF_PM_EN; else reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); @@ -1311,15 +1312,11 @@ 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.tx_lpi_enabled = tx_lpi_enabled; } static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) { struct bcmgenet_priv *priv = netdev_priv(dev); - struct ethtool_keee *p = &priv->eee; if (GENET_IS_V1(priv)) return -EOPNOTSUPP; @@ -1327,7 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) if (!dev->phydev) return -ENODEV; - 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); @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) { struct bcmgenet_priv *priv = netdev_priv(dev); - struct ethtool_keee *p = &priv->eee; - bool active; if (GENET_IS_V1(priv)) return -EOPNOTSUPP; @@ -1345,15 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) if (!dev->phydev) return -ENODEV; - p->eee_enabled = e->eee_enabled; - - if (!p->eee_enabled) { - bcmgenet_eee_enable_set(dev, false, false); - } else { - active = phy_init_eee(dev->phydev, false) >= 0; - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); - bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); - } + 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 7523b60b3c1c..bb82ecb2e220 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_keee eee; }; #define GENET_IO_MACRO(name, offset) \ @@ -703,7 +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 enable, - bool tx_lpi_enabled); - +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable); #endif /* __BCMGENET_H__ */ diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 9ada89355747..25eeea4c1965 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -30,7 +30,6 @@ static void bcmgenet_mac_config(struct net_device *dev) struct bcmgenet_priv *priv = netdev_priv(dev); struct phy_device *phydev = dev->phydev; u32 reg, cmd_bits = 0; - bool active; /* speed */ if (phydev->speed == SPEED_1000) @@ -88,11 +87,6 @@ static void bcmgenet_mac_config(struct net_device *dev) reg |= CMD_TX_EN | CMD_RX_EN; } bcmgenet_umac_writel(priv, reg, UMAC_CMD); - - active = phy_init_eee(phydev, 0) >= 0; - bcmgenet_eee_enable_set(dev, - priv->eee.eee_enabled && active, - priv->eee.tx_lpi_enabled); } /* setup netdev link state when PHY link status change and @@ -106,6 +100,7 @@ void bcmgenet_mii_setup(struct net_device *dev) if (phydev->link) { bcmgenet_mac_config(dev); + bcmgenet_eee_enable_set(dev, phydev->enable_tx_lpi); } 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 LPI 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 LPI timer value from hardware. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++-------------------- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 6 +----- drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------ 3 files changed, 8 insertions(+), 31 deletions(-) --- base-commit: 39f59c72ad3a1eaab9a60f0671bc94d2bc826d21 change-id: 20240408-stmmac-eee-d3c8e096f6a2 Best regards,