[RFC net-next] net: macb: add support for configuring eee via ethtool

[RFC net-next] net: macb: add support for configuring eee via ethtool

[Conor Dooley]

From: Steve Wilkins <steve.wilkins@raymarine.com>

Add ethtool_ops for configuring Energy Efficient Ethernet in the PHY.

Signed-off-by: Steve Wilkins <steve.wilkins@raymarine.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Steve sent me this patch (modulo the eee -> keee change), but I know
nothing about the macb driver, so I asked Nicolas whether the patch
made sense. His response was:
> Interesting although I have the feeling that some support from our MAC
> is missing for pretending to support the feature.
> I'm not sure the phylink without the MAC support is valid.
>
> I think we need a real task to be spawn to support EEE / LPI on cadence
> driver (but I don't see it scheduled in a way or another 🙁 ).

Since he was not sure, next port of call is lkml.. Is this patch
sufficient in isolation, or are additional changes required to the driver
for it?

The other drivers that I looked at that use phylink_ethtool_set_eee()
vary between doing what's done here and just forwarding the call, but
others are more complex, so without an understanding of the subsystem
I cannot tell :)

Alternatively, Steve, shout if you can tell me why forwarding to the phy
is sufficient, and I'll update the commit message and send this as
non-RFC.

Thanks,
Conor.

CC: valentina.fernandezalanis@microchip.com
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Claudiu Beznea <claudiu.beznea@tuxon.dev>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Russell King <linux@armlinux.org.uk>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 95e8742dce1d..a2a222954ebf 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3321,6 +3321,20 @@ static int macb_set_link_ksettings(struct net_device *netdev,
 	return phylink_ethtool_ksettings_set(bp->phylink, kset);
 }
 
+static int macb_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
+{
+	struct macb *bp = netdev_priv(netdev);
+
+	return phylink_ethtool_get_eee(bp->phylink, edata);
+}
+
+static int macb_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
+{
+	struct macb *bp = netdev_priv(netdev);
+
+	return phylink_ethtool_set_eee(bp->phylink, edata);
+}
+
 static void macb_get_ringparam(struct net_device *netdev,
 			       struct ethtool_ringparam *ring,
 			       struct kernel_ethtool_ringparam *kernel_ring,
@@ -3767,6 +3781,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
 	.set_wol		= macb_set_wol,
 	.get_link_ksettings     = macb_get_link_ksettings,
 	.set_link_ksettings     = macb_set_link_ksettings,
+	.get_eee		= macb_get_eee,
+	.set_eee		= macb_set_eee,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
 };
@@ -3783,6 +3799,8 @@ static const struct ethtool_ops gem_ethtool_ops = {
 	.get_sset_count		= gem_get_sset_count,
 	.get_link_ksettings     = macb_get_link_ksettings,
 	.set_link_ksettings     = macb_set_link_ksettings,
+	.get_eee		= macb_get_eee,
+	.set_eee		= macb_set_eee,
 	.get_ringparam		= macb_get_ringparam,
 	.set_ringparam		= macb_set_ringparam,
 	.get_rxnfc			= gem_get_rxnfc,
-- 
2.43.0

Re: [RFC net-next] net: macb: add support for configuring eee via ethtool

[Andrew Lunn]

On Tue, Aug 27, 2024 at 12:29:23PM +0100, Conor Dooley wrote:
> From: Steve Wilkins <steve.wilkins@raymarine.com>
> 
> Add ethtool_ops for configuring Energy Efficient Ethernet in the PHY.
> 
> Signed-off-by: Steve Wilkins <steve.wilkins@raymarine.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Steve sent me this patch (modulo the eee -> keee change), but I know
> nothing about the macb driver, so I asked Nicolas whether the patch
> made sense. His response was:
> > Interesting although I have the feeling that some support from our MAC
> > is missing for pretending to support the feature.
> > I'm not sure the phylink without the MAC support is valid.
> >
> > I think we need a real task to be spawn to support EEE / LPI on cadence
> > driver (but I don't see it scheduled in a way or another 🙁 ).
> 
> Since he was not sure, next port of call is lkml.. Is this patch
> sufficient in isolation, or are additional changes required to the driver
> for it?
> 
> The other drivers that I looked at that use phylink_ethtool_set_eee()
> vary between doing what's done here and just forwarding the call, but
> others are more complex, so without an understanding of the subsystem
> I cannot tell :)
> 
> Alternatively, Steve, shout if you can tell me why forwarding to the phy
> is sufficient, and I'll update the commit message and send this as
> non-RFC.

This is not sufficient. EEE is negotiated with the link peer, so
phylib/phylink is involved so that negotiation is performed, and the
results reported back to the MAC.

The MAC then needs to act on the results of the negotiation. The MAC
does most of the work. It keeps a timer of when it last sent a
packet. If this time exceeds 'tx-timer', it signals to the PHY it can
swap into low power mode, because there is nothing to send. When the
next packet comes along, it needs to signal the PHY to transition back
into full power mode, and it needs to wait a little for that to
happen.

You are missing the MAC parts.

	Andrew

Re: [RFC net-next] net: macb: add support for configuring eee via ethtool

[Andrew Lunn]

On Wed, Aug 28, 2024 at 03:47:21PM +0000, Wilkins, Stephen wrote:
> Thanks for the feedback.
> 
> In my particular use-case I wanted to ensure the PHY didn't advertise EEE
> support, as it can cause issues in our deployment environment. The problem I
> had was the PHY we are using enables EEE advertisement by default, and the
> generic phy support in phy_device.c reads the c45 registers and enables EEE if
> there are any linkmodes already advertised. Without the phylink hook in macb, I
> couldn't use ethtool to disable it, but I now see my patch is only a partial
> solution and would also imply support that is missing. That's why code reviews
> are important. Maybe I need an alternative approach for ensuring the PHY
> advertising is disabled if the MAC layer support is missing. 

In this particular case, do you know what is causing you problems?

I agree that if the MAC does not support EEE, the PHY should not be
advertising it. But historically EEE has been a mess. It could be the
MAC does EEE by default, using default settings, and the PHY is
advertising EEE, and the link partner is happy, and EEE just works. So
if we turn advertisement of EEE off by default, we might cause
regressions :-(

Now, we know some PHYs are actually broken. And we have a standard way
to express this:

Documentation/devicetree/bindings/net/ethernet-phy.yaml

  eee-broken-100tx:
    $ref: /schemas/types.yaml#/definitions/flag
    description:
      Mark the corresponding energy efficient ethernet mode as
      broken and request the ethernet to stop advertising it.

  eee-broken-1000t:
    $ref: /schemas/types.yaml#/definitions/flag
    description:
      Mark the corresponding energy efficient ethernet mode as
      broken and request the ethernet to stop advertising it.

If you know this MAC/PHY combination really is broken, not that it is
just missing support for EEE, you could add these properties to your
device tree.

Otherwise, you do a very minimal EEE implementation. After connecting
to the PHY call phy_ethtool_set_eee() with everything in data set to
0. That should disable adverting of EEE.

	Andrew

Re: [RFC net-next] net: macb: add support for configuring eee via ethtool

[Wilkins, Stephen]

In my case, it's not that I've had an issue with EEE not working, it's that I've found on other products it makes the Ethernet link more susceptible to link down events during some harsh EMC immunity tests, and we have prioritised link robustness over power saving.

I don't know enough about the Cadence MAC in the PolarFire SOC to know if EEE would work correctly without extra setup, but the initial feedback from Nicolas implied probably not. On other platforms, I've used ethtool to disable advertisement via an init script, but I think the cleanest option for this project is to force the PHY to disable advertisement via the dts, at least until EEE support for the macb driver is fully implemented.

Steve
________________________________________
From: Andrew Lunn <andrew@lunn.ch>
Sent: 28 August 2024 17:18
To: Wilkins, Stephen <Stephen.Wilkins@Teledyne.com>
Cc: Conor Dooley <conor@kernel.org>; netdev@vger.kernel.org <netdev@vger.kernel.org>; Conor Dooley <conor.dooley@microchip.com>; valentina.fernandezalanis@microchip.com <valentina.fernandezalanis@microchip.com>; Nicolas Ferre <nicolas.ferre@microchip.com>; Claudiu Beznea <claudiu.beznea@tuxon.dev>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Russell King <linux@armlinux.org.uk>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [RFC net-next] net: macb: add support for configuring eee via ethtool
 
---External Email---

On Wed, Aug 28, 2024 at 03:47:21PM +0000, Wilkins, Stephen wrote:
> Thanks for the feedback.
>
> In my particular use-case I wanted to ensure the PHY didn't advertise EEE
> support, as it can cause issues in our deployment environment. The problem I
> had was the PHY we are using enables EEE advertisement by default, and the
> generic phy support in phy_device.c reads the c45 registers and enables EEE if
> there are any linkmodes already advertised. Without the phylink hook in macb, I
> couldn't use ethtool to disable it, but I now see my patch is only a partial
> solution and would also imply support that is missing. That's why code reviews
> are important. Maybe I need an alternative approach for ensuring the PHY
> advertising is disabled if the MAC layer support is missing.

In this particular case, do you know what is causing you problems?

I agree that if the MAC does not support EEE, the PHY should not be
advertising it. But historically EEE has been a mess. It could be the
MAC does EEE by default, using default settings, and the PHY is
advertising EEE, and the link partner is happy, and EEE just works. So
if we turn advertisement of EEE off by default, we might cause
regressions :-(

Now, we know some PHYs are actually broken. And we have a standard way
to express this:

Documentation/devicetree/bindings/net/ethernet-phy.yaml

  eee-broken-100tx:
    $ref: /schemas/types.yaml#/definitions/flag
    description:
      Mark the corresponding energy efficient ethernet mode as
      broken and request the ethernet to stop advertising it.

  eee-broken-1000t:
    $ref: /schemas/types.yaml#/definitions/flag
    description:
      Mark the corresponding energy efficient ethernet mode as
      broken and request the ethernet to stop advertising it.

If you know this MAC/PHY combination really is broken, not that it is
just missing support for EEE, you could add these properties to your
device tree.

Otherwise, you do a very minimal EEE implementation. After connecting
to the PHY call phy_ethtool_set_eee() with everything in data set to
0. That should disable adverting of EEE.

        Andrew