[net-next,v5,2/8] net: phy: Add phydev->enable_tx_lpi to simplify adjust link callbacks

Commit Message

Oleksij Rempel Feb. 21, 2024, 6:21 a.m. UTC

From: Andrew Lunn <[e-mail]>

MAC drivers which support EEE need to know the results of the EEE
auto-neg in order to program the hardware to perform EEE or not.  The
oddly named phy_init_eee() can be used to determine this, it returns 0
if EEE should be used, or a negative error code,
e.g. -EOPPROTONOTSUPPORT if the PHY does not support EEE or negotiate
resulted in it not being used.

However, many MAC drivers get this wrong. Add phydev->enable_tx_lpi
which indicates the result of the autoneg for EEE, including if EEE is
administratively disabled with ethtool. The MAC driver can then access
this in the same way as link speed and duplex in the adjust link
callback. If enable_tx_lpi is true, the MAC should send low power
indications and does not need to consider anything else with respect
to EEE.

Reviewed-by: Florian Fainelli <[e-mail]>
Signed-off-by: Andrew Lunn <[e-mail]>
Signed-off-by: Oleksij Rempel <[e-mail]>
---
v2 Check for errors from genphy_c45_eee_is_active
v5: Rename to enable_tx_lpi to fit better with phylink changes
---
 drivers/net/phy/phy.c | 7 +++++++
 include/linux/phy.h   | 2 ++
 2 files changed, 9 insertions(+)

Comments

Wei Fang Feb. 23, 2024, 5:36 a.m. UTC | #1

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: 2024年2月21日 14:21
> To: Wei Fang <[e-mail]>; David S. Miller <[e-mail]>;
> Eric Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>;
> Paolo Abeni <[e-mail]>; Andrew Lunn <[e-mail]>;
> Heiner Kallweit <[e-mail]>; Russell King
> <[e-mail]>
> Cc: Florian Fainelli <[e-mail]>; Oleksij Rempel
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Shenwei Wang
> <[e-mail]>; Clark Wang <[e-mail]>;
> dl-linux-imx <[e-mail]>
> Subject: [PATCH net-next v5 2/8] net: phy: Add phydev->enable_tx_lpi to
> simplify adjust link callbacks
> 
> From: Andrew Lunn <[e-mail]>
> 
> MAC drivers which support EEE need to know the results of the EEE auto-neg
> in order to program the hardware to perform EEE or not.  The oddly named
> phy_init_eee() can be used to determine this, it returns 0 if EEE should be
> used, or a negative error code, e.g. -EOPPROTONOTSUPPORT if the PHY does
> not support EEE or negotiate resulted in it not being used.
> 
> However, many MAC drivers get this wrong. Add phydev->enable_tx_lpi
> which indicates the result of the autoneg for EEE, including if EEE is
> administratively disabled with ethtool. The MAC driver can then access this in
> the same way as link speed and duplex in the adjust link callback. If
> enable_tx_lpi is true, the MAC should send low power indications and does
> not need to consider anything else with respect to EEE.
> 
> Reviewed-by: Florian Fainelli <[e-mail]>
> Signed-off-by: Andrew Lunn <[e-mail]>
> Signed-off-by: Oleksij Rempel <[e-mail]>
> ---
> v2 Check for errors from genphy_c45_eee_is_active
> v5: Rename to enable_tx_lpi to fit better with phylink changes
> ---
>  drivers/net/phy/phy.c | 7 +++++++
>  include/linux/phy.h   | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index
> 14224e06d69f..a54b1daf5d5f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -983,9 +983,16 @@ static int phy_check_link_status(struct phy_device
> *phydev)
>  	if (phydev->link && phydev->state != PHY_RUNNING) {
>  		phy_check_downshift(phydev);
>  		phydev->state = PHY_RUNNING;
> +		err = genphy_c45_eee_is_active(phydev,
> +					       NULL, NULL, NULL);
> +		if (err < 0)
> +			phydev->enable_tx_lpi = false;
> +		else
> +			phydev->enable_tx_lpi = err;

phydev->enable_tx_lpi = !!err; Is this better?

>  		phy_link_up(phydev);
>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
>  		phydev->state = PHY_NOLINK;
> +		phydev->enable_tx_lpi = false;
>  		phy_link_down(phydev);
>  	}
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h index
> e3ab2c347a59..a880f6d7170e 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -594,6 +594,7 @@ struct macsec_ops;
>   * @supported_eee: supported PHY EEE linkmodes
>   * @advertising_eee: Currently advertised EEE linkmodes
>   * @eee_enabled: Flag indicating whether the EEE feature is enabled
> + * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
>   * @lp_advertising: Current link partner advertised linkmodes
>   * @host_interfaces: PHY interface modes supported by host
>   * @eee_broken_modes: Energy efficient ethernet modes which should be
> prohibited @@ -713,6 +714,7 @@ struct phy_device {
> 
>  	/* Energy efficient ethernet modes which should be prohibited */
>  	u32 eee_broken_modes;
> +	bool enable_tx_lpi;
> 
>  #ifdef CONFIG_LED_TRIGGER_PHY
>  	struct phy_led_trigger *phy_led_triggers;
> --
> 2.39.2

Heiner Kallweit Feb. 23, 2024, 6:58 a.m. UTC | #2

On 23.02.2024 06:36, Wei Fang wrote:
>> -----Original Message-----
>> From: Oleksij Rempel <[e-mail]>
>> Sent: 2024年2月21日 14:21
>> To: Wei Fang <[e-mail]>; David S. Miller <[e-mail]>;
>> Eric Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>;
>> Paolo Abeni <[e-mail]>; Andrew Lunn <[e-mail]>;
>> Heiner Kallweit <[e-mail]>; Russell King
>> <[e-mail]>
>> Cc: Florian Fainelli <[e-mail]>; Oleksij Rempel
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; Shenwei Wang
>> <[e-mail]>; Clark Wang <[e-mail]>;
>> dl-linux-imx <[e-mail]>
>> Subject: [PATCH net-next v5 2/8] net: phy: Add phydev->enable_tx_lpi to
>> simplify adjust link callbacks
>>
>> From: Andrew Lunn <[e-mail]>
>>
>> MAC drivers which support EEE need to know the results of the EEE auto-neg
>> in order to program the hardware to perform EEE or not.  The oddly named
>> phy_init_eee() can be used to determine this, it returns 0 if EEE should be
>> used, or a negative error code, e.g. -EOPPROTONOTSUPPORT if the PHY does
>> not support EEE or negotiate resulted in it not being used.
>>
>> However, many MAC drivers get this wrong. Add phydev->enable_tx_lpi
>> which indicates the result of the autoneg for EEE, including if EEE is
>> administratively disabled with ethtool. The MAC driver can then access this in
>> the same way as link speed and duplex in the adjust link callback. If
>> enable_tx_lpi is true, the MAC should send low power indications and does
>> not need to consider anything else with respect to EEE.
>>
>> Reviewed-by: Florian Fainelli <[e-mail]>
>> Signed-off-by: Andrew Lunn <[e-mail]>
>> Signed-off-by: Oleksij Rempel <[e-mail]>
>> ---
>> v2 Check for errors from genphy_c45_eee_is_active
>> v5: Rename to enable_tx_lpi to fit better with phylink changes
>> ---
>>  drivers/net/phy/phy.c | 7 +++++++
>>  include/linux/phy.h   | 2 ++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index
>> 14224e06d69f..a54b1daf5d5f 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -983,9 +983,16 @@ static int phy_check_link_status(struct phy_device
>> *phydev)
>>  	if (phydev->link && phydev->state != PHY_RUNNING) {
>>  		phy_check_downshift(phydev);
>>  		phydev->state = PHY_RUNNING;
>> +		err = genphy_c45_eee_is_active(phydev,
>> +					       NULL, NULL, NULL);
>> +		if (err < 0)
>> +			phydev->enable_tx_lpi = false;
>> +		else
>> +			phydev->enable_tx_lpi = err;
> 
> phydev->enable_tx_lpi = !!err; Is this better?
> 
I don't think so. Effectively err is a bool value, and it's implicitly
converted.

>>  		phy_link_up(phydev);
>>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
>>  		phydev->state = PHY_NOLINK;
>> +		phydev->enable_tx_lpi = false;
>>  		phy_link_down(phydev);
>>  	}
>>
>> diff --git a/include/linux/phy.h b/include/linux/phy.h index
>> e3ab2c347a59..a880f6d7170e 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -594,6 +594,7 @@ struct macsec_ops;
>>   * @supported_eee: supported PHY EEE linkmodes
>>   * @advertising_eee: Currently advertised EEE linkmodes
>>   * @eee_enabled: Flag indicating whether the EEE feature is enabled
>> + * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
>>   * @lp_advertising: Current link partner advertised linkmodes
>>   * @host_interfaces: PHY interface modes supported by host
>>   * @eee_broken_modes: Energy efficient ethernet modes which should be
>> prohibited @@ -713,6 +714,7 @@ struct phy_device {
>>
>>  	/* Energy efficient ethernet modes which should be prohibited */
>>  	u32 eee_broken_modes;
>> +	bool enable_tx_lpi;
>>
>>  #ifdef CONFIG_LED_TRIGGER_PHY
>>  	struct phy_led_trigger *phy_led_triggers;
>> --
>> 2.39.2
>

Patch

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 14224e06d69f..a54b1daf5d5f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -983,9 +983,16 @@  static int phy_check_link_status(struct phy_device *phydev)
 	if (phydev->link && phydev->state != PHY_RUNNING) {
 		phy_check_downshift(phydev);
 		phydev->state = PHY_RUNNING;
+		err = genphy_c45_eee_is_active(phydev,
+					       NULL, NULL, NULL);
+		if (err < 0)
+			phydev->enable_tx_lpi = false;
+		else
+			phydev->enable_tx_lpi = err;
 		phy_link_up(phydev);
 	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
 		phydev->state = PHY_NOLINK;
+		phydev->enable_tx_lpi = false;
 		phy_link_down(phydev);
 	}
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e3ab2c347a59..a880f6d7170e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -594,6 +594,7 @@  struct macsec_ops;
  * @supported_eee: supported PHY EEE linkmodes
  * @advertising_eee: Currently advertised EEE linkmodes
  * @eee_enabled: Flag indicating whether the EEE feature is enabled
+ * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -713,6 +714,7 @@  struct phy_device {
 
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
+	bool enable_tx_lpi;
 
 #ifdef CONFIG_LED_TRIGGER_PHY
 	struct phy_led_trigger *phy_led_triggers;