[PATCH net] net: bcmgenet: Fix EEE implementation

[PATCH net] net: bcmgenet: Fix EEE implementation

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 4935 bytes --]

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(-)

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
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net] net: bcmgenet: Fix EEE implementation

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: bcmgenet: Fix EEE implementation

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

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 :/
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net] net: bcmgenet: Fix EEE implementation

[Russell King (Oracle)]

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!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: bcmgenet: Fix EEE implementation

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 4300 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net] net: bcmgenet: Fix EEE implementation

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html