| Message ID | 20230606214348.2408018-1-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a9f31047baca57d47440c879cf259b86f900260c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: bcmgenet: Fix EEE implementation | expand |
On Tue, Jun 06, 2023 at 02:43:47PM -0700, Florian Fainelli wrote: > We had a number of short comings: > > - EEE must be re-evaluated whenever the state machine detects a link > change as wight be switching from a link partner with EEE > enabled/disabled > > - tx_lpi_enabled controls whether EEE should be enabled/disabled for the > transmit path, which applies to the TBUF block > > - We do not need to forcibly enable EEE upon system resume, as the PHY > state machine will trigger a link event that will do that, too > > Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support") > Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> > --- > netdev maintainers, please do not apply without Andrew, Russell and > Oleksij reviewing first since this relates to the on-going EEE rework > from Andrew. Hi Florian, Please could you include some information on the UMAC_EEE_CTRL EEE_EN bit - is this like the main switch for EEE which needs to be set along with the bits in the tbuf register for the transmit side to signal LPI? Thanks.
On 6/6/23 15:02, Russell King (Oracle) wrote: > On Tue, Jun 06, 2023 at 02:43:47PM -0700, Florian Fainelli wrote: >> We had a number of short comings: >> >> - EEE must be re-evaluated whenever the state machine detects a link >> change as wight be switching from a link partner with EEE >> enabled/disabled >> >> - tx_lpi_enabled controls whether EEE should be enabled/disabled for the >> transmit path, which applies to the TBUF block >> >> - We do not need to forcibly enable EEE upon system resume, as the PHY >> state machine will trigger a link event that will do that, too >> >> Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support") >> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> >> --- >> netdev maintainers, please do not apply without Andrew, Russell and >> Oleksij reviewing first since this relates to the on-going EEE rework >> from Andrew. > > Hi Florian, > > Please could you include some information on the UMAC_EEE_CTRL EEE_EN > bit - is this like the main switch for EEE which needs to be set > along with the bits in the tbuf register for the transmit side to > signal LPI? EEE_EN is described as: If set, the TX LPI policy control engine is enabled and the MAC inserts LPI_idle codes if the link is idle. The rx_lpi_detect assertion is independent of this configuration. in the RBUF, EEE_EN is described as: 1: to enable Energy Efficient feature between Unimac and PHY for Rx Path and in the TBUF, EEE_EN is described as: 1: to enable Energy Efficient feature between Unimac and PHY for Tx Path The documentation is unfortunately scare about how these two signals connect :/
On Tue, Jun 06, 2023 at 03:16:03PM -0700, Florian Fainelli wrote: > On 6/6/23 15:02, Russell King (Oracle) wrote: > > On Tue, Jun 06, 2023 at 02:43:47PM -0700, Florian Fainelli wrote: > > > We had a number of short comings: > > > > > > - EEE must be re-evaluated whenever the state machine detects a link > > > change as wight be switching from a link partner with EEE > > > enabled/disabled > > > > > > - tx_lpi_enabled controls whether EEE should be enabled/disabled for the > > > transmit path, which applies to the TBUF block > > > > > > - We do not need to forcibly enable EEE upon system resume, as the PHY > > > state machine will trigger a link event that will do that, too > > > > > > Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support") > > > Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > --- > > > netdev maintainers, please do not apply without Andrew, Russell and > > > Oleksij reviewing first since this relates to the on-going EEE rework > > > from Andrew. > > > > Hi Florian, > > > > Please could you include some information on the UMAC_EEE_CTRL EEE_EN > > bit - is this like the main switch for EEE which needs to be set > > along with the bits in the tbuf register for the transmit side to > > signal LPI? > > EEE_EN is described as: > > If set, the TX LPI policy control engine is enabled and the MAC inserts > LPI_idle codes if the link is idle. The rx_lpi_detect assertion is > independent of this configuration. > > in the RBUF, EEE_EN is described as: > > 1: to enable Energy Efficient feature between Unimac and PHY for Rx Path > > and in the TBUF, EEE_EN is described as: > > 1: to enable Energy Efficient feature between Unimac and PHY for Tx Path > > The documentation is unfortunately scare about how these two signals connect > :/ Thanks for the clarification. Squaring this with my understanding of EEE, the transmit side makes sense. LPI on the transmit side is only asserted only when EEE_EN and TBUF_EEE_EN are both set, so this is the behaviour we want. If we were evaluating this in software, my understanding is it would be: if (eee_enabled && eee_active && tx_lpi_enabled) enable LPI generation at MAC; else disable LPI generation at MAC; and the code here treats eee_enabled && eee_active as the "enabled" flag controlling EEE_EN, and tx_lpi_enabled controls TBUF_EEE_EN. The hardware effectively does the last && operation for us. So this all seems fine. On the receive side, if the link partner successfully negotiates EEE, then it can assert LPI, and the local end will see that assertion (hence, rx_lpi_detect may become true.) If the transmit side doesn't generate LPI, then this won't have any effect other than maybe setting status bits, so I don't see that setting RBUF_EEE_EN when eee_enabled && eee_active would be wrong. Moving the phy_init_eee() (as it currently stands) into the adjust_link path is definitely the right thing, since it provides the resolution of the negotiated EEE state. So, all round, I think your patch makes complete sense as far as the logic goes. Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> However, one thing I will ask is whether the hardware has any configuration of the various timers for EEE operation, and if it does, are they dependent on the negotiated speed of the interface? In Marvell's neta and pp2 drivers, the timers scale with link speed and thus need reprogramming accordingly. In any case, 802.3 specifies different timer settings depending on link speed and media type. Thanks!
On 6/7/23 01:46, Russell King (Oracle) wrote: > On Tue, Jun 06, 2023 at 03:16:03PM -0700, Florian Fainelli wrote: >> On 6/6/23 15:02, Russell King (Oracle) wrote: >>> On Tue, Jun 06, 2023 at 02:43:47PM -0700, Florian Fainelli wrote: >>>> We had a number of short comings: >>>> >>>> - EEE must be re-evaluated whenever the state machine detects a link >>>> change as wight be switching from a link partner with EEE >>>> enabled/disabled >>>> >>>> - tx_lpi_enabled controls whether EEE should be enabled/disabled for the >>>> transmit path, which applies to the TBUF block >>>> >>>> - We do not need to forcibly enable EEE upon system resume, as the PHY >>>> state machine will trigger a link event that will do that, too >>>> >>>> Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support") >>>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> >>>> --- >>>> netdev maintainers, please do not apply without Andrew, Russell and >>>> Oleksij reviewing first since this relates to the on-going EEE rework >>>> from Andrew. >>> >>> Hi Florian, >>> >>> Please could you include some information on the UMAC_EEE_CTRL EEE_EN >>> bit - is this like the main switch for EEE which needs to be set >>> along with the bits in the tbuf register for the transmit side to >>> signal LPI? >> >> EEE_EN is described as: >> >> If set, the TX LPI policy control engine is enabled and the MAC inserts >> LPI_idle codes if the link is idle. The rx_lpi_detect assertion is >> independent of this configuration. >> >> in the RBUF, EEE_EN is described as: >> >> 1: to enable Energy Efficient feature between Unimac and PHY for Rx Path >> >> and in the TBUF, EEE_EN is described as: >> >> 1: to enable Energy Efficient feature between Unimac and PHY for Tx Path >> >> The documentation is unfortunately scare about how these two signals connect >> :/ > > Thanks for the clarification. Squaring this with my understanding of > EEE, the transmit side makes sense. LPI on the transmit side is only > asserted only when EEE_EN and TBUF_EEE_EN are both set, so this is > the behaviour we want. If we were evaluating this in software, my > understanding is it would be: > > if (eee_enabled && eee_active && tx_lpi_enabled) > enable LPI generation at MAC; > else > disable LPI generation at MAC; > > and the code here treats eee_enabled && eee_active as the "enabled" > flag controlling EEE_EN, and tx_lpi_enabled controls TBUF_EEE_EN. > The hardware effectively does the last && operation for us. So > this all seems fine. > > On the receive side, if the link partner successfully negotiates > EEE, then it can assert LPI, and the local end will see that > assertion (hence, rx_lpi_detect may become true.) If the transmit > side doesn't generate LPI, then this won't have any effect other > than maybe setting status bits, so I don't see that setting > RBUF_EEE_EN when eee_enabled && eee_active would be wrong. > > Moving the phy_init_eee() (as it currently stands) into the adjust_link > path is definitely the right thing, since it provides the resolution > of the negotiated EEE state. > > So, all round, I think your patch makes complete sense as far as the > logic goes. > > Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > However, one thing I will ask is whether the hardware has any > configuration of the various timers for EEE operation, and if it does, > are they dependent on the negotiated speed of the interface? In > Marvell's neta and pp2 drivers, the timers scale with link speed and > thus need reprogramming accordingly. In any case, 802.3 specifies > different timer settings depending on link speed and media type. There are a couple of timers that are available: - LPI timer (EEE_LPI_TIMER register) - WAKE_TIMER (EEE_WAKE_TIMER register) both are dependent upon the EEE_REF_COUNT register which is described as: Clock divider for 1 us quanta count in EEE. This field controls clock divider used to generate ~1us reference pulses used by EEE timers. It specifies integer number of system clock cycles contained within 1us. the value is currently set to 0x7d (125) which does make some sense given that the system clock is considered stable and is provided at the same frequency irrespective of the link speed. Hope this helps.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 6 Jun 2023 14:43:47 -0700 you wrote: > We had a number of short comings: > > - EEE must be re-evaluated whenever the state machine detects a link > change as wight be switching from a link partner with EEE > enabled/disabled > > - tx_lpi_enabled controls whether EEE should be enabled/disabled for the > transmit path, which applies to the TBUF block > > [...] Here is the summary with links: - [net] net: bcmgenet: Fix EEE implementation https://git.kernel.org/netdev/net/c/a9f31047baca You are awesome, thank you!
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index eca0c92c0c84..2b5761ad2f92 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1272,7 +1272,8 @@ 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 enable, + bool tx_lpi_enabled) { struct bcmgenet_priv *priv = netdev_priv(dev); u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL; @@ -1292,7 +1293,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 (tx_lpi_enabled) reg |= TBUF_EEE_EN | TBUF_PM_EN; else reg &= ~(TBUF_EEE_EN | TBUF_PM_EN); @@ -1313,6 +1314,7 @@ static void bcmgenet_eee_enable_set(struct net_device *dev, bool enable) priv->eee.eee_enabled = enable; priv->eee.eee_active = enable; + priv->eee.tx_lpi_enabled = tx_lpi_enabled; } static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) @@ -1328,6 +1330,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_eee *e) 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 +1340,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; @@ -1348,16 +1350,11 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_eee *e) p->eee_enabled = e->eee_enabled; if (!p->eee_enabled) { - bcmgenet_eee_enable_set(dev, false); + bcmgenet_eee_enable_set(dev, false, false); } else { - ret = phy_init_eee(dev->phydev, false); - if (ret) { - netif_err(priv, hw, dev, "EEE initialization failed\n"); - return ret; - } - + p->eee_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, true); + bcmgenet_eee_enable_set(dev, p->eee_active, e->tx_lpi_enabled); } return phy_ethtool_set_eee(dev->phydev, e); @@ -4279,9 +4276,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..1985c0ec4da2 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -703,4 +703,7 @@ 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); + #endif /* __BCMGENET_H__ */ diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index be042905ada2..c15ed0acdb77 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -87,6 +87,11 @@ static void bcmgenet_mac_config(struct net_device *dev) reg |= CMD_TX_EN | CMD_RX_EN; } bcmgenet_umac_writel(priv, reg, UMAC_CMD); + + priv->eee.eee_active = phy_init_eee(phydev, 0) >= 0; + bcmgenet_eee_enable_set(dev, + priv->eee.eee_enabled && priv->eee.eee_active, + priv->eee.tx_lpi_enabled); } /* setup netdev link state when PHY link status change and
We had a number of short comings: - EEE must be re-evaluated whenever the state machine detects a link change as wight be switching from a link partner with EEE enabled/disabled - tx_lpi_enabled controls whether EEE should be enabled/disabled for the transmit path, which applies to the TBUF block - We do not need to forcibly enable EEE upon system resume, as the PHY state machine will trigger a link event that will do that, too Fixes: 6ef398ea60d9 ("net: bcmgenet: add EEE support") Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> --- netdev maintainers, please do not apply without Andrew, Russell and Oleksij reviewing first since this relates to the on-going EEE rework from Andrew. .../net/ethernet/broadcom/genet/bcmgenet.c | 22 +++++++------------ .../net/ethernet/broadcom/genet/bcmgenet.h | 3 +++ drivers/net/ethernet/broadcom/genet/bcmmii.c | 5 +++++ 3 files changed, 16 insertions(+), 14 deletions(-)