Re: [PATCH net-next v3 0/3] net: dsa: add support for phylink managed EEE

Re: [PATCH net-next v3 0/3] net: dsa: add support for phylink managed EEE

[Russell King (Oracle)]

Hi,

This series adds support for phylink managed EEE to DSA, and converts
mt753x to make use of this feature.

Patch 1 implements a helper to indicate whether the MAC LPI operations
are populated (suggested by Vladimir)

Patch 2 makes the necessary changes to the core code - we retain calling
set_mac_eee(), but this method now becomes a way to merely validate the
arguments when using phylink managed EEE rather than performing any
configuration.

Patch 3 converts the mt7530 driver to use phylink managed EEE.

v2: fix mt7530 build error
v3: send correct patch 2

 drivers/net/dsa/mt7530.c  | 68 ++++++++++++++++++++++++++++++++---------------
 drivers/net/phy/phylink.c |  3 +--
 include/linux/phylink.h   | 12 +++++++++
 net/dsa/user.c            | 21 +++++++++------
 4 files changed, 73 insertions(+), 31 deletions(-)

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

[PATCH net-next v3 1/3] net: phylink: provide phylink_mac_implements_lpi()

[Russell King (Oracle)]

Provide a helper to determine whether the MAC operations structure
implements the LPI operations, which will be used by both phylink and
DSA.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c |  3 +--
 include/linux/phylink.h   | 12 ++++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 214b62fba991..6fbb5fd5b400 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1957,8 +1957,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 		return ERR_PTR(-EINVAL);
 	}
 
-	pl->mac_supports_eee_ops = mac_ops->mac_disable_tx_lpi &&
-				   mac_ops->mac_enable_tx_lpi;
+	pl->mac_supports_eee_ops = phylink_mac_implements_lpi(mac_ops);
 	pl->mac_supports_eee = pl->mac_supports_eee_ops &&
 			       pl->config->lpi_capabilities &&
 			       !phy_interface_empty(pl->config->lpi_interfaces);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 898b00451bbf..0de78673172d 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface)
 	}
 }
 
+/**
+ * phylink_mac_implements_lpi() - determine if MAC implements LPI ops
+ * @ops: phylink_mac_ops structure
+ *
+ * Returns true if the phylink MAC operations structure indicates that the
+ * LPI operations have been implemented, false otherwise.
+ */
+static inline bool phylink_mac_implements_lpi(const struct phylink_mac_ops *ops)
+{
+	return ops && ops->mac_disable_tx_lpi && ops->mac_enable_tx_lpi;
+}
+
 void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
 				      unsigned int neg_mode, u16 bmsr, u16 lpa);
 void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
-- 
2.30.2

[PATCH net-next v3 2/3] net: dsa: allow use of phylink managed EEE support

[Russell King (Oracle)]

In order to allow DSA drivers to use phylink managed EEE, we need to
change the behaviour of the DSA's .set_eee() ethtool method.
Implementation of the DSA .set_mac_eee() method becomes optional with
phylink managed EEE as it is only used to validate the EEE parameters
supplied from userspace. The rest of the EEE state management should
be left to phylink.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 net/dsa/user.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 291ab1b4acc4..2296a4ead020 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1243,16 +1243,25 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
 	if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index))
 		return -EOPNOTSUPP;
 
-	/* Port's PHY and MAC both need to be EEE capable */
-	if (!dev->phydev)
-		return -ENODEV;
+	/* If the port is using phylink managed EEE, then an unimplemented
+	 * set_mac_eee() is permissible.
+	 */
+	if (!phylink_mac_implements_lpi(ds->phylink_mac_ops)) {
+		/* Port's PHY and MAC both need to be EEE capable */
+		if (!dev->phydev)
+			return -ENODEV;
 
-	if (!ds->ops->set_mac_eee)
-		return -EOPNOTSUPP;
+		if (!ds->ops->set_mac_eee)
+			return -EOPNOTSUPP;
 
-	ret = ds->ops->set_mac_eee(ds, dp->index, e);
-	if (ret)
-		return ret;
+		ret = ds->ops->set_mac_eee(ds, dp->index, e);
+		if (ret)
+			return ret;
+	} else if (ds->ops->set_mac_eee) {
+		ret = ds->ops->set_mac_eee(ds, dp->index, e);
+		if (ret)
+			return ret;
+	}
 
 	return phylink_ethtool_set_eee(dp->pl, e);
 }
-- 
2.30.2

[PATCH net-next v3 3/3] net: dsa: mt7530: convert to phylink managed EEE

[Russell King (Oracle)]

Convert mt7530 to use phylink managed EEE. When enabling EEE, we set
both PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 irrespective of the speed,
and clear them both when disabling.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/mt7530.c | 68 +++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1c83af805209..9fd44e55d519 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2957,28 +2957,61 @@ static void mt753x_phylink_mac_link_up(struct phylink_config *config,
 			mcr |= PMCR_FORCE_RX_FC_EN;
 	}
 
-	if (mode == MLO_AN_PHY && phydev && phy_init_eee(phydev, false) >= 0) {
-		switch (speed) {
-		case SPEED_1000:
-		case SPEED_2500:
-			mcr |= PMCR_FORCE_EEE1G;
-			break;
-		case SPEED_100:
-			mcr |= PMCR_FORCE_EEE100;
-			break;
-		}
-	}
-
 	mt7530_set(priv, MT753X_PMCR_P(dp->index), mcr);
 }
 
+static void mt753x_phylink_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct mt7530_priv *priv = dp->ds->priv;
+
+	mt7530_clear(priv, MT753X_PMCR_P(dp->index),
+		     PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100);
+}
+
+static int mt753x_phylink_mac_enable_tx_lpi(struct phylink_config *config,
+					    u32 timer, bool tx_clock_stop)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct mt7530_priv *priv = dp->ds->priv;
+	u32 val;
+
+	/* If the timer is zero, then set LPI_MODE_EN, which allows the
+	 * system to enter LPI mode immediately rather than waiting for
+	 * the LPI threshold.
+	 */
+	if (!timer)
+		val = LPI_MODE_EN;
+	else if (FIELD_FIT(LPI_THRESH_MASK, timer))
+		val = FIELD_PREP(LPI_THRESH_MASK, timer);
+	else
+		val = LPI_THRESH_MASK;
+
+	mt7530_rmw(priv, MT753X_PMEEECR_P(dp->index),
+		   LPI_THRESH_MASK | LPI_MODE_EN, val);
+
+	mt7530_set(priv, MT753X_PMCR_P(dp->index),
+		   PMCR_FORCE_EEE1G | PMCR_FORCE_EEE100);
+
+	return 0;
+}
+
 static void mt753x_phylink_get_caps(struct dsa_switch *ds, int port,
 				    struct phylink_config *config)
 {
 	struct mt7530_priv *priv = ds->priv;
+	u32 eeecr;
 
 	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
 
+	config->lpi_capabilities = MAC_100FD | MAC_1000FD | MAC_2500FD;
+
+	eeecr = mt7530_read(priv, MT753X_PMEEECR_P(port));
+	/* tx_lpi_timer should be in microseconds. The time units for
+	 * LPI threshold are unspecified.
+	 */
+	config->lpi_timer_default = FIELD_GET(LPI_THRESH_MASK, eeecr);
+
 	priv->info->mac_port_get_caps(ds, port, config);
 }
 
@@ -3088,18 +3121,9 @@ mt753x_setup(struct dsa_switch *ds)
 static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 			      struct ethtool_keee *e)
 {
-	struct mt7530_priv *priv = ds->priv;
-	u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
-
 	if (e->tx_lpi_timer > 0xFFF)
 		return -EINVAL;
 
-	set = LPI_THRESH_SET(e->tx_lpi_timer);
-	if (!e->tx_lpi_enabled)
-		/* Force LPI Mode without a delay */
-		set |= LPI_MODE_EN;
-	mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
-
 	return 0;
 }
 
@@ -3238,6 +3262,8 @@ static const struct phylink_mac_ops mt753x_phylink_mac_ops = {
 	.mac_config	= mt753x_phylink_mac_config,
 	.mac_link_down	= mt753x_phylink_mac_link_down,
 	.mac_link_up	= mt753x_phylink_mac_link_up,
+	.mac_disable_tx_lpi = mt753x_phylink_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = mt753x_phylink_mac_enable_tx_lpi,
 };
 
 const struct mt753x_info mt753x_table[] = {
-- 
2.30.2

Re: [PATCH net-next v3 1/3] net: phylink: provide phylink_mac_implements_lpi()

[Vladimir Oltean]

On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote:
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 898b00451bbf..0de78673172d 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface)
>  	}
>  }
>  
> +/**
> + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops
> + * @ops: phylink_mac_ops structure
> + *
> + * Returns true if the phylink MAC operations structure indicates that the
> + * LPI operations have been implemented, false otherwise.

This is something that I only noticed for v3 because I wanted to leave a
review tag, so I first checked the status in patchwork, but there it says:

include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi'

I am aware of this conversation from November where you raised the point
about tooling being able to accept the syntax without the colon as well:
https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/

but it looks like it didn't go anywhere, with Jon still preferring the
strict syntax for now, and no follow-up that I can see. So, the current
conventions are not these, and you haven't specifically said anywhere
that you are deliberately ignoring them.

In the end it's not something for me to decide, but I thought maybe I'm
speeding things up a little bit by opening up the discussion now, rather
than wait for a maintainer to look at it.

> + */
> +static inline bool phylink_mac_implements_lpi(const struct phylink_mac_ops *ops)
> +{
> +	return ops && ops->mac_disable_tx_lpi && ops->mac_enable_tx_lpi;
> +}
> +
>  void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
>  				      unsigned int neg_mode, u16 bmsr, u16 lpa);
>  void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,

Re: [PATCH net-next v3 0/3] net: dsa: add support for phylink managed EEE

[Vladimir Oltean]

On Mon, Feb 10, 2025 at 10:36:42AM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> This series adds support for phylink managed EEE to DSA, and converts
> mt753x to make use of this feature.
> 
> Patch 1 implements a helper to indicate whether the MAC LPI operations
> are populated (suggested by Vladimir)
> 
> Patch 2 makes the necessary changes to the core code - we retain calling
> set_mac_eee(), but this method now becomes a way to merely validate the
> arguments when using phylink managed EEE rather than performing any
> configuration.
> 
> Patch 3 converts the mt7530 driver to use phylink managed EEE.
> 
> v2: fix mt7530 build error
> v3: send correct patch 2
> 
>  drivers/net/dsa/mt7530.c  | 68 ++++++++++++++++++++++++++++++++---------------
>  drivers/net/phy/phylink.c |  3 +--
>  include/linux/phylink.h   | 12 +++++++++
>  net/dsa/user.c            | 21 +++++++++------
>  4 files changed, 73 insertions(+), 31 deletions(-)

Apart from the kernel-doc thing this looks fine to me. Please keep the
tags in patches if there is a need to resend.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v3 1/3] net: phylink: provide phylink_mac_implements_lpi()

[Russell King (Oracle)]

On Mon, Feb 10, 2025 at 03:20:54PM +0200, Vladimir Oltean wrote:
> On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote:
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 898b00451bbf..0de78673172d 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface)
> >  	}
> >  }
> >  
> > +/**
> > + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops
> > + * @ops: phylink_mac_ops structure
> > + *
> > + * Returns true if the phylink MAC operations structure indicates that the
> > + * LPI operations have been implemented, false otherwise.
> 
> This is something that I only noticed for v3 because I wanted to leave a
> review tag, so I first checked the status in patchwork, but there it says:
> 
> include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi'
> 
> I am aware of this conversation from November where you raised the point
> about tooling being able to accept the syntax without the colon as well:
> https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/
> 
> but it looks like it didn't go anywhere, with Jon still preferring the
> strict syntax for now, and no follow-up that I can see. So, the current
> conventions are not these, and you haven't specifically said anywhere
> that you are deliberately ignoring them.

It was explained in this email as part of that thread:

https://lore.kernel.org/netdev/ZzjHH-L-ylLe0YhU@shell.armlinux.org.uk/

The reason is that it goes against natural grammar. The only time that
"Returns:" would make sense in grammar is when listing with e.g. a
bulleted list, where the part before the colon doesn't have to be a
complete sentence.

This is why it's going to be an uphill battle - grammatically it is
wrong, and thus it doesn't flow when thinking about documenting the
return value.

If we want to go to a bulleted list, then it will be natural to add
the colon.

I'm not going to explain to this level of detail in every email, and
because of the grammatical nature of this, it's going to be very
difficult to use a form that goes against proper grammar.

-- 
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-next v3 1/3] net: phylink: provide phylink_mac_implements_lpi()

[Russell King (Oracle)]

On Mon, Feb 10, 2025 at 02:05:58PM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 10, 2025 at 03:20:54PM +0200, Vladimir Oltean wrote:
> > On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote:
> > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > > index 898b00451bbf..0de78673172d 100644
> > > --- a/include/linux/phylink.h
> > > +++ b/include/linux/phylink.h
> > > @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface)
> > >  	}
> > >  }
> > >  
> > > +/**
> > > + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops
> > > + * @ops: phylink_mac_ops structure
> > > + *
> > > + * Returns true if the phylink MAC operations structure indicates that the
> > > + * LPI operations have been implemented, false otherwise.
> > 
> > This is something that I only noticed for v3 because I wanted to leave a
> > review tag, so I first checked the status in patchwork, but there it says:
> > 
> > include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi'
> > 
> > I am aware of this conversation from November where you raised the point
> > about tooling being able to accept the syntax without the colon as well:
> > https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/
> > 
> > but it looks like it didn't go anywhere, with Jon still preferring the
> > strict syntax for now, and no follow-up that I can see. So, the current
> > conventions are not these, and you haven't specifically said anywhere
> > that you are deliberately ignoring them.
> 
> It was explained in this email as part of that thread:
> 
> https://lore.kernel.org/netdev/ZzjHH-L-ylLe0YhU@shell.armlinux.org.uk/
> 
> The reason is that it goes against natural grammar. The only time that
> "Returns:" would make sense in grammar is when listing with e.g. a
> bulleted list, where the part before the colon doesn't have to be a
> complete sentence.
> 
> This is why it's going to be an uphill battle - grammatically it is
> wrong, and thus it doesn't flow when thinking about documenting the
> return value.
> 
> If we want to go to a bulleted list, then it will be natural to add
> the colon.
> 
> I'm not going to explain to this level of detail in every email, and
> because of the grammatical nature of this, it's going to be very
> difficult to use a form that goes against proper grammar.

Also note that it follows the style already present in that file
with one exception (which is one of the few cases I remembered to use
the new format.)

-- 
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-next v3 1/3] net: phylink: provide phylink_mac_implements_lpi()

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 10 Feb 2025 10:36:44 +0000 you wrote:
> Provide a helper to determine whether the MAC operations structure
> implements the LPI operations, which will be used by both phylink and
> DSA.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phylink.c |  3 +--
>  include/linux/phylink.h   | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)

Here is the summary with links:
  - [net-next,v3,1/3] net: phylink: provide phylink_mac_implements_lpi()
    https://git.kernel.org/netdev/net-next/c/2001d21592e5
  - [net-next,v3,2/3] net: dsa: allow use of phylink managed EEE support
    https://git.kernel.org/netdev/net-next/c/b8927bd44f78
  - [net-next,v3,3/3] net: dsa: mt7530: convert to phylink managed EEE
    https://git.kernel.org/netdev/net-next/c/9cf21773f535

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