devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] net: add and use phy_disable_eee
@ 2024-12-16 21:29 Heiner Kallweit
  2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-16 21:29 UTC (permalink / raw)
  To: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman
  Cc: linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
Add phy_disable_eee() for this purpose, and use it in cpsw driver.

Heiner Kallweit (3):
  net: phy: add phy_disable_eee
  net: ethernet: ti: cpsw: disable PHY EEE advertisement
  ARM: dts: ti/omap: remove eee-broken properties

 arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi     |  2 --
 .../arm/boot/dts/ti/omap/am335x-myirtech-myd.dts |  1 -
 .../arm/boot/dts/ti/omap/am5729-beagleboneai.dts |  2 --
 drivers/net/ethernet/ti/cpsw.c                   |  3 ++-
 drivers/net/ethernet/ti/cpsw_ethtool.c           | 12 ------------
 drivers/net/ethernet/ti/cpsw_new.c               |  3 ++-
 drivers/net/ethernet/ti/cpsw_priv.h              |  1 -
 drivers/net/phy/phy_device.c                     | 16 ++++++++++++++++
 include/linux/phy.h                              |  1 +
 9 files changed, 21 insertions(+), 20 deletions(-)

-- 
2.47.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-16 21:29 [PATCH net-next 0/3] net: add and use phy_disable_eee Heiner Kallweit
@ 2024-12-16 21:31 ` Heiner Kallweit
  2024-12-17 10:43   ` Andrew Lunn
  2024-12-19  8:34   ` Andrew Lunn
  2024-12-16 21:32 ` [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement Heiner Kallweit
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-16 21:31 UTC (permalink / raw)
  To: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman
  Cc: linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
Add phy_disable_eee() for this purpose.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 16 ++++++++++++++++
 include/linux/phy.h          |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b26bb33cd..fe18a12c4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2993,6 +2993,22 @@ void phy_support_eee(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_support_eee);
 
+/**
+ * phy_disable_eee - Disable EEE for the PHY
+ * @phydev: Target phy_device struct
+ *
+ * This function is used by MAC drivers for MAC's which don't support EEE.
+ * It disables EEE on the PHY layer.
+ */
+void phy_disable_eee(struct phy_device *phydev)
+{
+	linkmode_zero(phydev->supported_eee);
+	linkmode_zero(phydev->advertising_eee);
+	phydev->eee_cfg.tx_lpi_enabled = false;
+	phydev->eee_cfg.eee_enabled = false;
+}
+EXPORT_SYMBOL_GPL(phy_disable_eee);
+
 /**
  * phy_support_sym_pause - Enable support of symmetrical pause
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e597a32cc..5bc71d599 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
 void phy_support_sym_pause(struct phy_device *phydev);
 void phy_support_asym_pause(struct phy_device *phydev);
 void phy_support_eee(struct phy_device *phydev);
+void phy_disable_eee(struct phy_device *phydev);
 void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
 		       bool autoneg);
 void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement
  2024-12-16 21:29 [PATCH net-next 0/3] net: add and use phy_disable_eee Heiner Kallweit
  2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
@ 2024-12-16 21:32 ` Heiner Kallweit
  2024-12-19  8:34   ` Andrew Lunn
  2024-12-16 21:33 ` [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties Heiner Kallweit
  2024-12-20  3:30 ` [PATCH net-next 0/3] net: add and use phy_disable_eee patchwork-bot+netdevbpf
  3 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-16 21:32 UTC (permalink / raw)
  To: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman
  Cc: linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

It seems the cpsw MAC doesn't support EEE. See e.g. the commit message of
ce2899428ec0 ("ARM: dts: am335x-baltos: disable EEE for Atheros 8035 PHY").
There are cases where this causes issues if the PHY's on both sides have
negotiated EEE. As a workaround EEE modes of the PHY are marked broken
in DT, effectively disabling EEE advertisement.
Improve this by using new function phy_disable_eee() in the MAC driver.
This properly disables EEE advertisement, and allows to remove the
eee-broken-xxx properties from DT. As EEE is disabled anyway, we can
remove also the set_eee ethtool op.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c         |  3 ++-
 drivers/net/ethernet/ti/cpsw_ethtool.c | 12 ------------
 drivers/net/ethernet/ti/cpsw_new.c     |  3 ++-
 drivers/net/ethernet/ti/cpsw_priv.h    |  1 -
 4 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 4ef8cf6ea..1e290ee8e 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -635,6 +635,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 
 	slave->phy = phy;
 
+	phy_disable_eee(slave->phy);
+
 	phy_attached_info(slave->phy);
 
 	phy_start(slave->phy);
@@ -1225,7 +1227,6 @@ static const struct ethtool_ops cpsw_ethtool_ops = {
 	.get_link_ksettings	= cpsw_get_link_ksettings,
 	.set_link_ksettings	= cpsw_set_link_ksettings,
 	.get_eee	= cpsw_get_eee,
-	.set_eee	= cpsw_set_eee,
 	.nway_reset	= cpsw_nway_reset,
 	.get_ringparam = cpsw_get_ringparam,
 	.set_ringparam = cpsw_set_ringparam,
diff --git a/drivers/net/ethernet/ti/cpsw_ethtool.c b/drivers/net/ethernet/ti/cpsw_ethtool.c
index 21d55a180..bdc4db0d1 100644
--- a/drivers/net/ethernet/ti/cpsw_ethtool.c
+++ b/drivers/net/ethernet/ti/cpsw_ethtool.c
@@ -434,18 +434,6 @@ int cpsw_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
 		return -EOPNOTSUPP;
 }
 
-int cpsw_set_eee(struct net_device *ndev, struct ethtool_keee *edata)
-{
-	struct cpsw_priv *priv = netdev_priv(ndev);
-	struct cpsw_common *cpsw = priv->cpsw;
-	int slave_no = cpsw_slave_index(cpsw, priv);
-
-	if (cpsw->slaves[slave_no].phy)
-		return phy_ethtool_set_eee(cpsw->slaves[slave_no].phy, edata);
-	else
-		return -EOPNOTSUPP;
-}
-
 int cpsw_nway_reset(struct net_device *ndev)
 {
 	struct cpsw_priv *priv = netdev_priv(ndev);
diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
index a98bcc5eb..be4d90c1c 100644
--- a/drivers/net/ethernet/ti/cpsw_new.c
+++ b/drivers/net/ethernet/ti/cpsw_new.c
@@ -778,6 +778,8 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 
 	slave->phy = phy;
 
+	phy_disable_eee(slave->phy);
+
 	phy_attached_info(slave->phy);
 
 	phy_start(slave->phy);
@@ -1209,7 +1211,6 @@ static const struct ethtool_ops cpsw_ethtool_ops = {
 	.get_link_ksettings	= cpsw_get_link_ksettings,
 	.set_link_ksettings	= cpsw_set_link_ksettings,
 	.get_eee		= cpsw_get_eee,
-	.set_eee		= cpsw_set_eee,
 	.nway_reset		= cpsw_nway_reset,
 	.get_ringparam		= cpsw_get_ringparam,
 	.set_ringparam		= cpsw_set_ringparam,
diff --git a/drivers/net/ethernet/ti/cpsw_priv.h b/drivers/net/ethernet/ti/cpsw_priv.h
index 1f448290b..f2fc55d92 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.h
+++ b/drivers/net/ethernet/ti/cpsw_priv.h
@@ -497,7 +497,6 @@ int cpsw_get_link_ksettings(struct net_device *ndev,
 int cpsw_set_link_ksettings(struct net_device *ndev,
 			    const struct ethtool_link_ksettings *ecmd);
 int cpsw_get_eee(struct net_device *ndev, struct ethtool_keee *edata);
-int cpsw_set_eee(struct net_device *ndev, struct ethtool_keee *edata);
 int cpsw_nway_reset(struct net_device *ndev);
 void cpsw_get_ringparam(struct net_device *ndev,
 			struct ethtool_ringparam *ering,
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties
  2024-12-16 21:29 [PATCH net-next 0/3] net: add and use phy_disable_eee Heiner Kallweit
  2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
  2024-12-16 21:32 ` [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement Heiner Kallweit
@ 2024-12-16 21:33 ` Heiner Kallweit
  2024-12-19  8:35   ` Andrew Lunn
  2024-12-20  3:30 ` [PATCH net-next 0/3] net: add and use phy_disable_eee patchwork-bot+netdevbpf
  3 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-16 21:33 UTC (permalink / raw)
  To: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman
  Cc: linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

Now that the cpsw(-new) MAC driver disables PHY EEE advertisement,
we can remove the eee-broken-xxx workaround.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi      | 2 --
 arch/arm/boot/dts/ti/omap/am335x-myirtech-myd.dts | 1 -
 arch/arm/boot/dts/ti/omap/am5729-beagleboneai.dts | 2 --
 3 files changed, 5 deletions(-)

diff --git a/arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi b/arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi
index ae2e8dffb..e69e5e817 100644
--- a/arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi
+++ b/arch/arm/boot/dts/ti/omap/am335x-baltos.dtsi
@@ -354,8 +354,6 @@ &davinci_mdio_sw {
 
 	phy1: ethernet-phy@1 {
 		reg = <7>;
-		eee-broken-100tx;
-		eee-broken-1000t;
 	};
 };
 
diff --git a/arch/arm/boot/dts/ti/omap/am335x-myirtech-myd.dts b/arch/arm/boot/dts/ti/omap/am335x-myirtech-myd.dts
index fd91a3c01..8b86918bd 100644
--- a/arch/arm/boot/dts/ti/omap/am335x-myirtech-myd.dts
+++ b/arch/arm/boot/dts/ti/omap/am335x-myirtech-myd.dts
@@ -96,7 +96,6 @@ &cpsw_port2 {
 &davinci_mdio_sw {
 	phy1: ethernet-phy@6 {
 		reg = <6>;
-		eee-broken-1000t;
 	};
 };
 
diff --git a/arch/arm/boot/dts/ti/omap/am5729-beagleboneai.dts b/arch/arm/boot/dts/ti/omap/am5729-beagleboneai.dts
index e6a18954e..e8007a8fd 100644
--- a/arch/arm/boot/dts/ti/omap/am5729-beagleboneai.dts
+++ b/arch/arm/boot/dts/ti/omap/am5729-beagleboneai.dts
@@ -492,8 +492,6 @@ &davinci_mdio_sw {
 
 	phy0: ethernet-phy@4 {
 		reg = <4>;
-		eee-broken-100tx;
-		eee-broken-1000t;
 	};
 };
 
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
@ 2024-12-17 10:43   ` Andrew Lunn
  2024-12-17 20:50     ` Heiner Kallweit
  2024-12-19  2:59     ` Jakub Kicinski
  2024-12-19  8:34   ` Andrew Lunn
  1 sibling, 2 replies; 18+ messages in thread
From: Andrew Lunn @ 2024-12-17 10:43 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

> @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
>  void phy_support_sym_pause(struct phy_device *phydev);
>  void phy_support_asym_pause(struct phy_device *phydev);
>  void phy_support_eee(struct phy_device *phydev);
> +void phy_disable_eee(struct phy_device *phydev);

So we have three states:

MAC tells PHYLIB it does support EEE
MAC tells PHYLIB it does not support EEE
MAC says nothing.

Do we really want this?

For phylink, i think we have a nice new clean design and can say, if
the MAC does not indicate it supports EEE, we turn it off in the
PHY. For phylib, we have more of a mess, and there could be MACs
actually doing EEE by default using default setting but with no user
space control. Do we want to keep this, or should we say any MAC which
does not call phy_support_eee() before phy_start() would have EEE
disabled in the PHY?

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-17 10:43   ` Andrew Lunn
@ 2024-12-17 20:50     ` Heiner Kallweit
  2024-12-17 22:34       ` Andrew Lunn
  2024-12-19  2:59     ` Jakub Kicinski
  1 sibling, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-17 20:50 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

On 17.12.2024 11:43, Andrew Lunn wrote:
>> @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
>>  void phy_support_sym_pause(struct phy_device *phydev);
>>  void phy_support_asym_pause(struct phy_device *phydev);
>>  void phy_support_eee(struct phy_device *phydev);
>> +void phy_disable_eee(struct phy_device *phydev);
> 
> So we have three states:
> 
> MAC tells PHYLIB it does support EEE
> MAC tells PHYLIB it does not support EEE
> MAC says nothing.
> 
> Do we really want this?
> 
> For phylink, i think we have a nice new clean design and can say, if
> the MAC does not indicate it supports EEE, we turn it off in the
> PHY. For phylib, we have more of a mess, and there could be MACs
> actually doing EEE by default using default setting but with no user
> space control. Do we want to keep this, or should we say any MAC which
> does not call phy_support_eee() before phy_start() would have EEE
> disabled in the PHY?
> 
The case "MAC says nothing" isn't desirable. However, if we did what
you mention, we'd silently change the behavior of several drivers,
resulting in disabled EEE and higher power consumption.
I briefly grepped the kernel source for phy_start() and found about
70 drivers. Some of them have the phylib EEE call, and in some cases
like cpsw the MAC doesn't support EEE. But what remains is IMO too
many drivers where we'd change the behavior.

> 	Andrew

Heiner


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-17 20:50     ` Heiner Kallweit
@ 2024-12-17 22:34       ` Andrew Lunn
  2024-12-18  7:01         ` Heiner Kallweit
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2024-12-17 22:34 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

On Tue, Dec 17, 2024 at 09:50:12PM +0100, Heiner Kallweit wrote:
> On 17.12.2024 11:43, Andrew Lunn wrote:
> >> @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
> >>  void phy_support_sym_pause(struct phy_device *phydev);
> >>  void phy_support_asym_pause(struct phy_device *phydev);
> >>  void phy_support_eee(struct phy_device *phydev);
> >> +void phy_disable_eee(struct phy_device *phydev);
> > 
> > So we have three states:
> > 
> > MAC tells PHYLIB it does support EEE
> > MAC tells PHYLIB it does not support EEE
> > MAC says nothing.
> > 
> > Do we really want this?
> > 
> > For phylink, i think we have a nice new clean design and can say, if
> > the MAC does not indicate it supports EEE, we turn it off in the
> > PHY. For phylib, we have more of a mess, and there could be MACs
> > actually doing EEE by default using default setting but with no user
> > space control. Do we want to keep this, or should we say any MAC which
> > does not call phy_support_eee() before phy_start() would have EEE
> > disabled in the PHY?
> > 
> The case "MAC says nothing" isn't desirable. However, if we did what
> you mention, we'd silently change the behavior of several drivers,
> resulting in disabled EEE and higher power consumption.
> I briefly grepped the kernel source for phy_start() and found about
> 70 drivers. Some of them have the phylib EEE call, and in some cases
> like cpsw the MAC doesn't support EEE. But what remains is IMO too
> many drivers where we'd change the behavior.

So for phylib, we keep with the three states. But phylink? Can we
disable EEE when the MAC says nothing?

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-17 22:34       ` Andrew Lunn
@ 2024-12-18  7:01         ` Heiner Kallweit
  0 siblings, 0 replies; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-18  7:01 UTC (permalink / raw)
  To: Andrew Lunn, Russell King - ARM Linux
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Andrew Lunn, Paolo Abeni, Jakub Kicinski, David Miller,
	Eric Dumazet, Simon Horman, linux-omap,
	devicetree@vger.kernel.org, netdev@vger.kernel.org

On 17.12.2024 23:34, Andrew Lunn wrote:
> On Tue, Dec 17, 2024 at 09:50:12PM +0100, Heiner Kallweit wrote:
>> On 17.12.2024 11:43, Andrew Lunn wrote:
>>>> @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
>>>>  void phy_support_sym_pause(struct phy_device *phydev);
>>>>  void phy_support_asym_pause(struct phy_device *phydev);
>>>>  void phy_support_eee(struct phy_device *phydev);
>>>> +void phy_disable_eee(struct phy_device *phydev);
>>>
>>> So we have three states:
>>>
>>> MAC tells PHYLIB it does support EEE
>>> MAC tells PHYLIB it does not support EEE
>>> MAC says nothing.
>>>
>>> Do we really want this?
>>>
>>> For phylink, i think we have a nice new clean design and can say, if
>>> the MAC does not indicate it supports EEE, we turn it off in the
>>> PHY. For phylib, we have more of a mess, and there could be MACs
>>> actually doing EEE by default using default setting but with no user
>>> space control. Do we want to keep this, or should we say any MAC which
>>> does not call phy_support_eee() before phy_start() would have EEE
>>> disabled in the PHY?
>>>
>> The case "MAC says nothing" isn't desirable. However, if we did what
>> you mention, we'd silently change the behavior of several drivers,
>> resulting in disabled EEE and higher power consumption.
>> I briefly grepped the kernel source for phy_start() and found about
>> 70 drivers. Some of them have the phylib EEE call, and in some cases
>> like cpsw the MAC doesn't support EEE. But what remains is IMO too
>> many drivers where we'd change the behavior.
> 
> So for phylib, we keep with the three states. But phylink? Can we
> disable EEE when the MAC says nothing?
> 
Looking at patch 5 of Russell's series behavior doesn't change if
pl->mac_supports_eee is false. So I think he also goes with the three
states, at least initially, until all drivers using phylink have
implemented the new phylink ops. AFAICS this affects about 25 drivers.

> 	Andrew
Heiner

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-17 10:43   ` Andrew Lunn
  2024-12-17 20:50     ` Heiner Kallweit
@ 2024-12-19  2:59     ` Jakub Kicinski
  2024-12-19  8:41       ` Andrew Lunn
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2024-12-19  2:59 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Heiner Kallweit, Tony Lindgren, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	David Miller, Eric Dumazet, Simon Horman, linux-omap,
	devicetree@vger.kernel.org, netdev@vger.kernel.org

On Tue, 17 Dec 2024 11:43:11 +0100 Andrew Lunn wrote:
> > @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
> >  void phy_support_sym_pause(struct phy_device *phydev);
> >  void phy_support_asym_pause(struct phy_device *phydev);
> >  void phy_support_eee(struct phy_device *phydev);
> > +void phy_disable_eee(struct phy_device *phydev);  
> 
> So we have three states:
> 
> MAC tells PHYLIB it does support EEE
> MAC tells PHYLIB it does not support EEE
> MAC says nothing.
> 
> Do we really want this?

Hi Andrew, do you feel convinced? I think I messed up merging some EEE
patches recently, an explicit Ack would boost my confidence..

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
  2024-12-17 10:43   ` Andrew Lunn
@ 2024-12-19  8:34   ` Andrew Lunn
  1 sibling, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2024-12-19  8:34 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

On Mon, Dec 16, 2024 at 10:31:18PM +0100, Heiner Kallweit wrote:
> If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
> Add phy_disable_eee() for this purpose.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement
  2024-12-16 21:32 ` [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement Heiner Kallweit
@ 2024-12-19  8:34   ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2024-12-19  8:34 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

On Mon, Dec 16, 2024 at 10:32:25PM +0100, Heiner Kallweit wrote:
> It seems the cpsw MAC doesn't support EEE. See e.g. the commit message of
> ce2899428ec0 ("ARM: dts: am335x-baltos: disable EEE for Atheros 8035 PHY").
> There are cases where this causes issues if the PHY's on both sides have
> negotiated EEE. As a workaround EEE modes of the PHY are marked broken
> in DT, effectively disabling EEE advertisement.
> Improve this by using new function phy_disable_eee() in the MAC driver.
> This properly disables EEE advertisement, and allows to remove the
> eee-broken-xxx properties from DT. As EEE is disabled anyway, we can
> remove also the set_eee ethtool op.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties
  2024-12-16 21:33 ` [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties Heiner Kallweit
@ 2024-12-19  8:35   ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2024-12-19  8:35 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Tony Lindgren, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	Jakub Kicinski, David Miller, Eric Dumazet, Simon Horman,
	linux-omap, devicetree@vger.kernel.org, netdev@vger.kernel.org

On Mon, Dec 16, 2024 at 10:33:21PM +0100, Heiner Kallweit wrote:
> Now that the cpsw(-new) MAC driver disables PHY EEE advertisement,
> we can remove the eee-broken-xxx workaround.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee
  2024-12-19  2:59     ` Jakub Kicinski
@ 2024-12-19  8:41       ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2024-12-19  8:41 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Heiner Kallweit, Tony Lindgren, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Russell King - ARM Linux, Andrew Lunn, Paolo Abeni,
	David Miller, Eric Dumazet, Simon Horman, linux-omap,
	devicetree@vger.kernel.org, netdev@vger.kernel.org

On Wed, Dec 18, 2024 at 06:59:09PM -0800, Jakub Kicinski wrote:
> On Tue, 17 Dec 2024 11:43:11 +0100 Andrew Lunn wrote:
> > > @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
> > >  void phy_support_sym_pause(struct phy_device *phydev);
> > >  void phy_support_asym_pause(struct phy_device *phydev);
> > >  void phy_support_eee(struct phy_device *phydev);
> > > +void phy_disable_eee(struct phy_device *phydev);  
> > 
> > So we have three states:
> > 
> > MAC tells PHYLIB it does support EEE
> > MAC tells PHYLIB it does not support EEE
> > MAC says nothing.
> > 
> > Do we really want this?
> 
> Hi Andrew, do you feel convinced? I think I messed up merging some EEE
> patches recently, an explicit Ack would boost my confidence..

For phylib, yes, we have to live with this unknown state. so these
patches are O.K.

For phylink, i would like Russells opinion. It would be better if we
could avoid having the third state. Maybe we need a couple of cycles
where if the MAC says nothing, but the PHY negotiates EEE, we issue a
warning? We might then get an idea of how many systems are in this
unknown category, and can encourage MAC driver Maintainers to add the
missing EEE support.

Russell?

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/3] net: add and use phy_disable_eee
  2024-12-16 21:29 [PATCH net-next 0/3] net: add and use phy_disable_eee Heiner Kallweit
                   ` (2 preceding siblings ...)
  2024-12-16 21:33 ` [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties Heiner Kallweit
@ 2024-12-20  3:30 ` patchwork-bot+netdevbpf
  2024-12-20  8:26   ` Heiner Kallweit
  3 siblings, 1 reply; 18+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-12-20  3:30 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: tony, robh, krzk+dt, conor+dt, linux, andrew, andrew+netdev,
	pabeni, kuba, davem, edumazet, horms, linux-omap, devicetree,
	netdev

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 16 Dec 2024 22:29:58 +0100 you wrote:
> If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
> Add phy_disable_eee() for this purpose, and use it in cpsw driver.
> 
> Heiner Kallweit (3):
>   net: phy: add phy_disable_eee
>   net: ethernet: ti: cpsw: disable PHY EEE advertisement
>   ARM: dts: ti/omap: remove eee-broken properties
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: phy: add phy_disable_eee
    https://git.kernel.org/netdev/net-next/c/b55498ff14bd
  - [net-next,2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement
    https://git.kernel.org/netdev/net-next/c/c9f5a5dabbf5
  - [net-next,3/3] ARM: dts: ti/omap: remove eee-broken properties
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/3] net: add and use phy_disable_eee
  2024-12-20  3:30 ` [PATCH net-next 0/3] net: add and use phy_disable_eee patchwork-bot+netdevbpf
@ 2024-12-20  8:26   ` Heiner Kallweit
  2024-12-20  8:40     ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-20  8:26 UTC (permalink / raw)
  To: kuba
  Cc: tony, robh, krzk+dt, conor+dt, linux, andrew, andrew+netdev,
	pabeni, davem, edumazet, horms, linux-omap, devicetree, netdev

On 20.12.2024 04:30, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This series was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
> 
> On Mon, 16 Dec 2024 22:29:58 +0100 you wrote:
>> If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
>> Add phy_disable_eee() for this purpose, and use it in cpsw driver.
>>
>> Heiner Kallweit (3):
>>   net: phy: add phy_disable_eee
>>   net: ethernet: ti: cpsw: disable PHY EEE advertisement
>>   ARM: dts: ti/omap: remove eee-broken properties
>>
>> [...]
> 
> Here is the summary with links:
>   - [net-next,1/3] net: phy: add phy_disable_eee
>     https://git.kernel.org/netdev/net-next/c/b55498ff14bd
>   - [net-next,2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement
>     https://git.kernel.org/netdev/net-next/c/c9f5a5dabbf5
>   - [net-next,3/3] ARM: dts: ti/omap: remove eee-broken properties
>     (no matching commit)
> 
Patch 3 is marked "not applicable" in patchwork and didn't make it to net-next.
Any issue with this patch?

> You are awesome, thank you!


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/3] net: add and use phy_disable_eee
  2024-12-20  8:26   ` Heiner Kallweit
@ 2024-12-20  8:40     ` Andrew Lunn
  2024-12-20 14:25       ` Jakub Kicinski
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2024-12-20  8:40 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: kuba, tony, robh, krzk+dt, conor+dt, linux, andrew+netdev, pabeni,
	davem, edumazet, horms, linux-omap, devicetree, netdev

On Fri, Dec 20, 2024 at 09:26:54AM +0100, Heiner Kallweit wrote:
> On 20.12.2024 04:30, patchwork-bot+netdevbpf@kernel.org wrote:
> > Hello:
> > 
> > This series was applied to netdev/net-next.git (main)
> > by Jakub Kicinski <kuba@kernel.org>:
> > 
> > On Mon, 16 Dec 2024 22:29:58 +0100 you wrote:
> >> If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it.
> >> Add phy_disable_eee() for this purpose, and use it in cpsw driver.
> >>
> >> Heiner Kallweit (3):
> >>   net: phy: add phy_disable_eee
> >>   net: ethernet: ti: cpsw: disable PHY EEE advertisement
> >>   ARM: dts: ti/omap: remove eee-broken properties
> >>
> >> [...]
> > 
> > Here is the summary with links:
> >   - [net-next,1/3] net: phy: add phy_disable_eee
> >     https://git.kernel.org/netdev/net-next/c/b55498ff14bd
> >   - [net-next,2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement
> >     https://git.kernel.org/netdev/net-next/c/c9f5a5dabbf5
> >   - [net-next,3/3] ARM: dts: ti/omap: remove eee-broken properties
> >     (no matching commit)
> > 
> Patch 3 is marked "not applicable" in patchwork and didn't make it to net-next.
> Any issue with this patch?

Maybe Jakub wants you to submit it to the TI DT Maintainer? Or get an
Acked-by: from said Maintainer.

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/3] net: add and use phy_disable_eee
  2024-12-20  8:40     ` Andrew Lunn
@ 2024-12-20 14:25       ` Jakub Kicinski
  2024-12-20 22:14         ` Heiner Kallweit
  0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2024-12-20 14:25 UTC (permalink / raw)
  To: Heiner Kallweit
  Cc: Andrew Lunn, tony, robh, krzk+dt, conor+dt, linux, andrew+netdev,
	pabeni, davem, edumazet, horms, linux-omap, devicetree, netdev

On Fri, 20 Dec 2024 09:40:38 +0100 Andrew Lunn wrote:
> > Patch 3 is marked "not applicable" in patchwork and didn't make it to net-next.
> > Any issue with this patch?  
> 
> Maybe Jakub wants you to submit it to the TI DT Maintainer? Or get an
> Acked-by: from said Maintainer.

Indeed, we got screamed at once for applying dts changes.
I filter them out now 🤷️ Sorry for not letting you know.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next 0/3] net: add and use phy_disable_eee
  2024-12-20 14:25       ` Jakub Kicinski
@ 2024-12-20 22:14         ` Heiner Kallweit
  0 siblings, 0 replies; 18+ messages in thread
From: Heiner Kallweit @ 2024-12-20 22:14 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Lunn, tony, robh, krzk+dt, conor+dt, linux, andrew+netdev,
	pabeni, davem, edumazet, horms, linux-omap, devicetree, netdev

On 20.12.2024 15:25, Jakub Kicinski wrote:
> On Fri, 20 Dec 2024 09:40:38 +0100 Andrew Lunn wrote:
>>> Patch 3 is marked "not applicable" in patchwork and didn't make it to net-next.
>>> Any issue with this patch?  
>>
>> Maybe Jakub wants you to submit it to the TI DT Maintainer? Or get an
>> Acked-by: from said Maintainer.
> 
> Indeed, we got screamed at once for applying dts changes.
> I filter them out now 🤷️ Sorry for not letting you know.

OK, thanks for the explanation. Then I hope Tony as TI Omap DT maintainer
picks it up.


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-12-20 22:14 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-16 21:29 [PATCH net-next 0/3] net: add and use phy_disable_eee Heiner Kallweit
2024-12-16 21:31 ` [PATCH net-next 1/3] net: phy: add phy_disable_eee Heiner Kallweit
2024-12-17 10:43   ` Andrew Lunn
2024-12-17 20:50     ` Heiner Kallweit
2024-12-17 22:34       ` Andrew Lunn
2024-12-18  7:01         ` Heiner Kallweit
2024-12-19  2:59     ` Jakub Kicinski
2024-12-19  8:41       ` Andrew Lunn
2024-12-19  8:34   ` Andrew Lunn
2024-12-16 21:32 ` [PATCH net-next 2/3] net: ethernet: ti: cpsw: disable PHY EEE advertisement Heiner Kallweit
2024-12-19  8:34   ` Andrew Lunn
2024-12-16 21:33 ` [PATCH net-next 3/3] ARM: dts: ti/omap: remove eee-broken properties Heiner Kallweit
2024-12-19  8:35   ` Andrew Lunn
2024-12-20  3:30 ` [PATCH net-next 0/3] net: add and use phy_disable_eee patchwork-bot+netdevbpf
2024-12-20  8:26   ` Heiner Kallweit
2024-12-20  8:40     ` Andrew Lunn
2024-12-20 14:25       ` Jakub Kicinski
2024-12-20 22:14         ` Heiner Kallweit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).