[PATCH net-next v4 0/2] dsa: lan9303: Move to PHYLINK

[PATCH net-next v4 0/2] dsa: lan9303: Move to PHYLINK

[Jerry Ray]

This patch series moves the lan9303 driver to use the phylink
api away from phylib.

Note a preparatory patch addresses whitespace issues to make the
dsa_switch_ops code consistent.

Note the .port_max_mtu api patch is now removed from this series.  It
was unrelated and has little to no value if the api is never being
called for the cpu port.

Migrating to phylink means removing the .adjust_link api. The
functionality from the adjust_link is moved to the phylink_mac_link_up
api.  The code being removed only affected the cpu port.

---
v3-> v4:
  - Addressed whitespace issues as a separate patch.
  - Removed port_max_mtu api patch as it is unrelated to phylink migration.
  - Reworked the implementation to preserve the adjust_link functionality
    by including it in the phylink_mac_link_up api.

v2-> v3:
  Added back in disabling Turbo Mode on the CPU MII interface.
  Removed the unnecessary clearing of the phyvsupported interfaces.
v1-> v2:
  corrected the reported mtu size, removing ETH_HLEN and ETH_FCS_LEN

 drivers/net/dsa/lan9303-core.c | 93 ++++++++++++--------
 1 file changed, 56 insertions(+), 37 deletions(-)

[PATCH net-next v4 1/2] dsa: lan9303: Whitespace Only

[Jerry Ray]

Whitespace preparatory patch, making the dsa_switch_ops table consistent.
No code is added or removed.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
 drivers/net/dsa/lan9303-core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 80f07bd20593..d9f7b554a423 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1280,16 +1280,16 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
 }
 
 static const struct dsa_switch_ops lan9303_switch_ops = {
-	.get_tag_protocol = lan9303_get_tag_protocol,
-	.setup = lan9303_setup,
-	.get_strings = lan9303_get_strings,
-	.phy_read = lan9303_phy_read,
-	.phy_write = lan9303_phy_write,
-	.adjust_link = lan9303_adjust_link,
-	.get_ethtool_stats = lan9303_get_ethtool_stats,
-	.get_sset_count = lan9303_get_sset_count,
-	.port_enable = lan9303_port_enable,
-	.port_disable = lan9303_port_disable,
+	.get_tag_protocol	= lan9303_get_tag_protocol,
+	.setup			= lan9303_setup,
+	.get_strings		= lan9303_get_strings,
+	.phy_read		= lan9303_phy_read,
+	.phy_write		= lan9303_phy_write,
+	.adjust_link		= lan9303_adjust_link,
+	.get_ethtool_stats	= lan9303_get_ethtool_stats,
+	.get_sset_count		= lan9303_get_sset_count,
+	.port_enable		= lan9303_port_enable,
+	.port_disable		= lan9303_port_disable,
 	.port_bridge_join       = lan9303_port_bridge_join,
 	.port_bridge_leave      = lan9303_port_bridge_leave,
 	.port_stp_state_set     = lan9303_port_stp_state_set,
-- 
2.17.1

Re: [PATCH net-next v4 1/2] dsa: lan9303: Whitespace Only

[Vladimir Oltean]

Please name the commit message something more specific than "Whitespace Only",
that is likely to not be confused with some other patch. A "Whitespace Only"
patch can take place anywhere in this file. Like "align dsa_switch_ops members".

On Wed, Dec 07, 2022 at 05:28:27PM -0600, Jerry Ray wrote:
> Whitespace preparatory patch, making the dsa_switch_ops table consistent.
> No code is added or removed.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 80f07bd20593..d9f7b554a423 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1280,16 +1280,16 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
>  }
>  
>  static const struct dsa_switch_ops lan9303_switch_ops = {
> -	.get_tag_protocol = lan9303_get_tag_protocol,
> -	.setup = lan9303_setup,
> -	.get_strings = lan9303_get_strings,
> -	.phy_read = lan9303_phy_read,
> -	.phy_write = lan9303_phy_write,
> -	.adjust_link = lan9303_adjust_link,
> -	.get_ethtool_stats = lan9303_get_ethtool_stats,
> -	.get_sset_count = lan9303_get_sset_count,
> -	.port_enable = lan9303_port_enable,
> -	.port_disable = lan9303_port_disable,
> +	.get_tag_protocol	= lan9303_get_tag_protocol,
> +	.setup			= lan9303_setup,
> +	.get_strings		= lan9303_get_strings,
> +	.phy_read		= lan9303_phy_read,
> +	.phy_write		= lan9303_phy_write,
> +	.adjust_link		= lan9303_adjust_link,
> +	.get_ethtool_stats	= lan9303_get_ethtool_stats,
> +	.get_sset_count		= lan9303_get_sset_count,
> +	.port_enable		= lan9303_port_enable,
> +	.port_disable		= lan9303_port_disable,

Do you use a text editor which highlights tabs? The members above this
line are aligned with tabs, the ones below with spaces. Still not
exactly what I'd call "consistent".

>  	.port_bridge_join       = lan9303_port_bridge_join,
>  	.port_bridge_leave      = lan9303_port_bridge_leave,
>  	.port_stp_state_set     = lan9303_port_stp_state_set,
> -- 
> 2.17.1
> 

RE: [PATCH net-next v4 1/2] dsa: lan9303: Whitespace Only

[Jerry.Ray]

> Please name the commit message something more specific than "Whitespace Only",
> that is likely to not be confused with some other patch. A "Whitespace Only"
> patch can take place anywhere in this file. Like "align dsa_switch_ops members".
> 
> Do you use a text editor which highlights tabs? The members above this
> line are aligned with tabs, the ones below with spaces. Still not
> exactly what I'd call "consistent".
> 

Hi Vladimir,

Thank you for your comments.  I will rename the patch to be more explicit and
will address the tabs-spaces issue you pointed out.

I'll look for a better Linux-based text editor this weekend.  Do you have a
recommendation?

Regards,
Jerry.

Re: [PATCH net-next v4 1/2] dsa: lan9303: Whitespace Only

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 07:25:15PM +0000, Jerry.Ray@microchip.com wrote:
> Hi Vladimir,
> 
> Thank you for your comments.  I will rename the patch to be more explicit and
> will address the tabs-spaces issue you pointed out.
> 
> I'll look for a better Linux-based text editor this weekend.  Do you have a
> recommendation?

Text editors are more of a personal choice, I'm in no position to recommend
one over another. There are quite a few which fulfill the requirements of
proper code highlighting, given the right configuration.

[PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

[Jerry Ray]

This patch replaces the adjust_link api with the phylink apis to provide
equivalent functionality.

The functionality from the adjust_link is moved to the phylink_mac_link_up
api.  The code being removed only affected the cpu port.

Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_up

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v3-> v4:
  - Reworked the implementation to preserve the adjust_link functionality
    by including it in the phylink_mac_link_up api.
v2-> v3:
  Added back in disabling Turbo Mode on the CPU MII interface.
  Removed the unnecessary clearing of the phy supported interfaces.
---
 drivers/net/dsa/lan9303-core.c | 123 +++++++++++++++++++++++----------
 1 file changed, 86 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index d9f7b554a423..a800448c9433 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1047,42 +1047,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	return chip->ops->phy_write(chip, phy, regnum, val);
 }
 
-static void lan9303_adjust_link(struct dsa_switch *ds, int port,
-				struct phy_device *phydev)
-{
-	struct lan9303 *chip = ds->priv;
-	int ctl;
-
-	if (!phy_is_pseudo_fixed_link(phydev))
-		return;
-
-	ctl = lan9303_phy_read(ds, port, MII_BMCR);
-
-	ctl &= ~BMCR_ANENABLE;
-
-	if (phydev->speed == SPEED_100)
-		ctl |= BMCR_SPEED100;
-	else if (phydev->speed == SPEED_10)
-		ctl &= ~BMCR_SPEED100;
-	else
-		dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
-
-	if (phydev->duplex == DUPLEX_FULL)
-		ctl |= BMCR_FULLDPLX;
-	else
-		ctl &= ~BMCR_FULLDPLX;
-
-	lan9303_phy_write(ds, port, MII_BMCR, ctl);
-
-	if (port == chip->phy_addr_base) {
-		/* Virtual Phy: Remove Turbo 200Mbit mode */
-		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
-
-		ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
-		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
-	}
-}
-
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
 			       struct phy_device *phy)
 {
@@ -1279,13 +1243,98 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
+				     struct phylink_config *config)
+{
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
+
+	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
+				   MAC_SYM_PAUSE;
+
+	if (dsa_is_cpu_port(ds, port)) {
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  config->supported_interfaces);
+	} else {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		/* Compatibility for phylib's default interface type when the
+		 * phy-mode property is absent
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	}
+
+	/* This driver does not make use of the speed, duplex, pause or the
+	 * advertisement in its mac_config, so it is safe to mark this driver
+	 * as non-legacy.
+	 */
+	config->legacy_pre_march2020 = false;
+}
+
+static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
+					unsigned int mode,
+					phy_interface_t interface,
+					struct phy_device *phydev, int speed,
+					int duplex, bool tx_pause,
+					bool rx_pause)
+{
+	struct lan9303 *chip = ds->priv;
+	u32 ctl;
+	int ret;
+
+	dev_dbg(chip->dev, "%s(%d) entered - %s.", __func__, port,
+		phy_modes(interface));
+
+	/* if this is not the cpu port, then simply return. */
+	if (!dsa_port_is_cpu(dsa_to_port(ds, port)))
+		return;
+
+	ctl = lan9303_phy_read(ds, port, MII_BMCR);
+
+	ctl &= ~BMCR_ANENABLE;
+
+	if (speed == SPEED_100)
+		ctl |= BMCR_SPEED100;
+	else if (speed == SPEED_10)
+		ctl &= ~BMCR_SPEED100;
+	else
+		dev_err(ds->dev, "unsupported speed: %d\n", speed);
+
+	if (duplex == DUPLEX_FULL)
+		ctl |= BMCR_FULLDPLX;
+	else
+		ctl &= ~BMCR_FULLDPLX;
+
+	lan9303_phy_write(ds, port, MII_BMCR, ctl);
+
+	if (port == chip->phy_addr_base) {
+		/* Virtual Phy: Remove Turbo 200Mbit mode */
+		ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
+				   &ctl);
+		if (ret)
+			return;
+
+		/* Clear the TURBO Mode bit if it was set. */
+		if (ctl & LAN9303_VIRT_SPECIAL_TURBO) {
+			ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
+			regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
+				     ctl);
+		}
+	}
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol	= lan9303_get_tag_protocol,
 	.setup			= lan9303_setup,
 	.get_strings		= lan9303_get_strings,
 	.phy_read		= lan9303_phy_read,
 	.phy_write		= lan9303_phy_write,
-	.adjust_link		= lan9303_adjust_link,
+	.phylink_get_caps	= lan9303_phylink_get_caps,
+	.phylink_mac_link_up	= lan9303_phylink_mac_link_up,
 	.get_ethtool_stats	= lan9303_get_ethtool_stats,
 	.get_sset_count		= lan9303_get_sset_count,
 	.port_enable		= lan9303_port_enable,
-- 
2.17.1

Re: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

[Vladimir Oltean]

On Wed, Dec 07, 2022 at 05:28:28PM -0600, Jerry Ray wrote:
> This patch replaces the adjust_link api with the phylink apis to provide
> equivalent functionality.
> 
> The functionality from the adjust_link is moved to the phylink_mac_link_up
> api.  The code being removed only affected the cpu port.
> 
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_up
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
> v3-> v4:
>   - Reworked the implementation to preserve the adjust_link functionality
>     by including it in the phylink_mac_link_up api.
> v2-> v3:
>   Added back in disabling Turbo Mode on the CPU MII interface.
>   Removed the unnecessary clearing of the phy supported interfaces.
> ---
>  drivers/net/dsa/lan9303-core.c | 123 +++++++++++++++++++++++----------
>  1 file changed, 86 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index d9f7b554a423..a800448c9433 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1047,42 +1047,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>  	return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>  
> -static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> -				struct phy_device *phydev)
> -{
> -	struct lan9303 *chip = ds->priv;
> -	int ctl;
> -
> -	if (!phy_is_pseudo_fixed_link(phydev))
> -		return;

We discourage code movement which also changes the code in question.
Code movement should be just that, movement. If you don't like this
check and want to replace it with dsa_port_is_cpu(), do so in a
preparatory patch.

> -
> -	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -
> -	ctl &= ~BMCR_ANENABLE;
> -
> -	if (phydev->speed == SPEED_100)
> -		ctl |= BMCR_SPEED100;
> -	else if (phydev->speed == SPEED_10)
> -		ctl &= ~BMCR_SPEED100;
> -	else
> -		dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
> -
> -	if (phydev->duplex == DUPLEX_FULL)
> -		ctl |= BMCR_FULLDPLX;
> -	else
> -		ctl &= ~BMCR_FULLDPLX;
> -
> -	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> -
> -	if (port == chip->phy_addr_base) {
> -		/* Virtual Phy: Remove Turbo 200Mbit mode */
> -		lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> -
> -		ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> -		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> -	}
> -}
> -
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  			       struct phy_device *phy)
>  {
> @@ -1279,13 +1243,98 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> +				     struct phylink_config *config)
> +{
> +	struct lan9303 *chip = ds->priv;
> +
> +	dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> +	config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> +				   MAC_SYM_PAUSE;
> +
> +	if (dsa_is_cpu_port(ds, port)) {
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_MII,
> +			  config->supported_interfaces);
> +	} else {
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		/* Compatibility for phylib's default interface type when the
> +		 * phy-mode property is absent
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +	}
> +
> +	/* This driver does not make use of the speed, duplex, pause or the
> +	 * advertisement in its mac_config, so it is safe to mark this driver
> +	 * as non-legacy.
> +	 */
> +	config->legacy_pre_march2020 = false;
> +}
> +
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +					unsigned int mode,
> +					phy_interface_t interface,
> +					struct phy_device *phydev, int speed,
> +					int duplex, bool tx_pause,
> +					bool rx_pause)
> +{
> +	struct lan9303 *chip = ds->priv;
> +	u32 ctl;
> +	int ret;
> +
> +	dev_dbg(chip->dev, "%s(%d) entered - %s.", __func__, port,
> +		phy_modes(interface));
> +
> +	/* if this is not the cpu port, then simply return. */

As a reader, I find my intelligence insulted by self-evident comments such as this.

Especially in contrast with the writes below to the MII_BMCR of the
Virtual PHY, which would certainly deserve a bit more of an explanation,
yet there is none there.

> +	if (!dsa_port_is_cpu(dsa_to_port(ds, port)))
> +		return;
> +
> +	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +	ctl &= ~BMCR_ANENABLE;
> +
> +	if (speed == SPEED_100)
> +		ctl |= BMCR_SPEED100;
> +	else if (speed == SPEED_10)
> +		ctl &= ~BMCR_SPEED100;
> +	else
> +		dev_err(ds->dev, "unsupported speed: %d\n", speed);
> +
> +	if (duplex == DUPLEX_FULL)
> +		ctl |= BMCR_FULLDPLX;
> +	else
> +		ctl &= ~BMCR_FULLDPLX;
> +
> +	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +
> +	if (port == chip->phy_addr_base) {
> +		/* Virtual Phy: Remove Turbo 200Mbit mode */
> +		ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +				   &ctl);
> +		if (ret)
> +			return;
> +
> +		/* Clear the TURBO Mode bit if it was set. */
> +		if (ctl & LAN9303_VIRT_SPECIAL_TURBO) {
> +			ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> +			regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +				     ctl);
> +		}
> +	}

You actually had something good going in the previous patch. The action
of disabling Turbo MII mode seems invariant of the negotiated link
settings. So it would be better to do it just once, during switch setup.
It would be good if you could add one more preparatory patch which does
just that. Assuming, of course, that the reordering in register writes
between LAN9303_VIRT_SPECIAL_CTRL and MII_BMCR will not cause a problem,
and that the link still works.

> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol	= lan9303_get_tag_protocol,
>  	.setup			= lan9303_setup,
>  	.get_strings		= lan9303_get_strings,
>  	.phy_read		= lan9303_phy_read,
>  	.phy_write		= lan9303_phy_write,
> -	.adjust_link		= lan9303_adjust_link,
> +	.phylink_get_caps	= lan9303_phylink_get_caps,
> +	.phylink_mac_link_up	= lan9303_phylink_mac_link_up,
>  	.get_ethtool_stats	= lan9303_get_ethtool_stats,
>  	.get_sset_count		= lan9303_get_sset_count,
>  	.port_enable		= lan9303_port_enable,
> -- 
> 2.17.1
> 

RE: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

[Jerry.Ray]

Hi Vladimir,

> We discourage code movement which also changes the code in question.
> Code movement should be just that, movement. If you don't like this
> check and want to replace it with dsa_port_is_cpu(), do so in a
> preparatory patch.
> 

Understood.  Will do.


> As a reader, I find my intelligence insulted by self-evident comments such as this.
> 
> Especially in contrast with the writes below to the MII_BMCR of the
> Virtual PHY, which would certainly deserve a bit more of an explanation,
> yet there is none there.
> 

I struggle with the lack of comments I see in the kernel codebase. While
experts can look at the source code and understand it, I find I spend a
good deal of time chasing down macros - following data structures - and
reverse engineering an understanding of the purpose of something that could
have been explained in the maintained source.  In-line comments target the
unfamiliar reader as there are a lot of us out here and far too few experts.
But I do see your point and I'll try to do a better job on this.


> You actually had something good going in the previous patch. The action
> of disabling Turbo MII mode seems invariant of the negotiated link
> settings. So it would be better to do it just once, during switch setup.
> It would be good if you could add one more preparatory patch which does
> just that. Assuming, of course, that the reordering in register writes
> between LAN9303_VIRT_SPECIAL_CTRL and MII_BMCR will not cause a problem,
> and that the link still works.

I'll rework the patch series to break this change out.

Regards,
Jerry.

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com> 
Sent: Thursday, December 8, 2022 11:21 AM
To: Jerry Ray - C33025 <Jerry.Ray@microchip.com>
Cc: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux@armlinux.org.uk
Subject: Re: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Wed, Dec 07, 2022 at 05:28:28PM -0600, Jerry Ray wrote:
> This patch replaces the adjust_link api with the phylink apis to provide
> equivalent functionality.
>
> The functionality from the adjust_link is moved to the phylink_mac_link_up
> api.  The code being removed only affected the cpu port.
>
> Removes:
> .adjust_link
> Adds:
> .phylink_get_caps
> .phylink_mac_link_up
>
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
> v3-> v4:
>   - Reworked the implementation to preserve the adjust_link functionality
>     by including it in the phylink_mac_link_up api.
> v2-> v3:
>   Added back in disabling Turbo Mode on the CPU MII interface.
>   Removed the unnecessary clearing of the phy supported interfaces.
> ---
>  drivers/net/dsa/lan9303-core.c | 123 +++++++++++++++++++++++----------
>  1 file changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index d9f7b554a423..a800448c9433 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1047,42 +1047,6 @@ static int lan9303_phy_write(struct dsa_switch *ds, int phy, int regnum,
>       return chip->ops->phy_write(chip, phy, regnum, val);
>  }
>
> -static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> -                             struct phy_device *phydev)
> -{
> -     struct lan9303 *chip = ds->priv;
> -     int ctl;
> -
> -     if (!phy_is_pseudo_fixed_link(phydev))
> -             return;

We discourage code movement which also changes the code in question.
Code movement should be just that, movement. If you don't like this
check and want to replace it with dsa_port_is_cpu(), do so in a
preparatory patch.

> -
> -     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -
> -     ctl &= ~BMCR_ANENABLE;
> -
> -     if (phydev->speed == SPEED_100)
> -             ctl |= BMCR_SPEED100;
> -     else if (phydev->speed == SPEED_10)
> -             ctl &= ~BMCR_SPEED100;
> -     else
> -             dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
> -
> -     if (phydev->duplex == DUPLEX_FULL)
> -             ctl |= BMCR_FULLDPLX;
> -     else
> -             ctl &= ~BMCR_FULLDPLX;
> -
> -     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> -
> -     if (port == chip->phy_addr_base) {
> -             /* Virtual Phy: Remove Turbo 200Mbit mode */
> -             lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &ctl);
> -
> -             ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> -             regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, ctl);
> -     }
> -}
> -
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
>                              struct phy_device *phy)
>  {
> @@ -1279,13 +1243,98 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
>       return 0;
>  }
>
> +static void lan9303_phylink_get_caps(struct dsa_switch *ds, int port,
> +                                  struct phylink_config *config)
> +{
> +     struct lan9303 *chip = ds->priv;
> +
> +     dev_dbg(chip->dev, "%s(%d) entered.", __func__, port);
> +
> +     config->mac_capabilities = MAC_10 | MAC_100 | MAC_ASYM_PAUSE |
> +                                MAC_SYM_PAUSE;
> +
> +     if (dsa_is_cpu_port(ds, port)) {
> +             __set_bit(PHY_INTERFACE_MODE_RMII,
> +                       config->supported_interfaces);
> +             __set_bit(PHY_INTERFACE_MODE_MII,
> +                       config->supported_interfaces);
> +     } else {
> +             __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +                       config->supported_interfaces);
> +             /* Compatibility for phylib's default interface type when the
> +              * phy-mode property is absent
> +              */
> +             __set_bit(PHY_INTERFACE_MODE_GMII,
> +                       config->supported_interfaces);
> +     }
> +
> +     /* This driver does not make use of the speed, duplex, pause or the
> +      * advertisement in its mac_config, so it is safe to mark this driver
> +      * as non-legacy.
> +      */
> +     config->legacy_pre_march2020 = false;
> +}
> +
> +static void lan9303_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +                                     unsigned int mode,
> +                                     phy_interface_t interface,
> +                                     struct phy_device *phydev, int speed,
> +                                     int duplex, bool tx_pause,
> +                                     bool rx_pause)
> +{
> +     struct lan9303 *chip = ds->priv;
> +     u32 ctl;
> +     int ret;
> +
> +     dev_dbg(chip->dev, "%s(%d) entered - %s.", __func__, port,
> +             phy_modes(interface));
> +
> +     /* if this is not the cpu port, then simply return. */

As a reader, I find my intelligence insulted by self-evident comments such as this.

Especially in contrast with the writes below to the MII_BMCR of the
Virtual PHY, which would certainly deserve a bit more of an explanation,
yet there is none there.

> +     if (!dsa_port_is_cpu(dsa_to_port(ds, port)))
> +             return;
> +
> +     ctl = lan9303_phy_read(ds, port, MII_BMCR);
> +
> +     ctl &= ~BMCR_ANENABLE;
> +
> +     if (speed == SPEED_100)
> +             ctl |= BMCR_SPEED100;
> +     else if (speed == SPEED_10)
> +             ctl &= ~BMCR_SPEED100;
> +     else
> +             dev_err(ds->dev, "unsupported speed: %d\n", speed);
> +
> +     if (duplex == DUPLEX_FULL)
> +             ctl |= BMCR_FULLDPLX;
> +     else
> +             ctl &= ~BMCR_FULLDPLX;
> +
> +     lan9303_phy_write(ds, port, MII_BMCR, ctl);
> +
> +     if (port == chip->phy_addr_base) {
> +             /* Virtual Phy: Remove Turbo 200Mbit mode */
> +             ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +                                &ctl);
> +             if (ret)
> +                     return;
> +
> +             /* Clear the TURBO Mode bit if it was set. */
> +             if (ctl & LAN9303_VIRT_SPECIAL_TURBO) {
> +                     ctl &= ~LAN9303_VIRT_SPECIAL_TURBO;
> +                     regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL,
> +                                  ctl);
> +             }
> +     }

You actually had something good going in the previous patch. The action
of disabling Turbo MII mode seems invariant of the negotiated link
settings. So it would be better to do it just once, during switch setup.
It would be good if you could add one more preparatory patch which does
just that. Assuming, of course, that the reordering in register writes
between LAN9303_VIRT_SPECIAL_CTRL and MII_BMCR will not cause a problem,
and that the link still works.

> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>       .get_tag_protocol       = lan9303_get_tag_protocol,
>       .setup                  = lan9303_setup,
>       .get_strings            = lan9303_get_strings,
>       .phy_read               = lan9303_phy_read,
>       .phy_write              = lan9303_phy_write,
> -     .adjust_link            = lan9303_adjust_link,
> +     .phylink_get_caps       = lan9303_phylink_get_caps,
> +     .phylink_mac_link_up    = lan9303_phylink_mac_link_up,
>       .get_ethtool_stats      = lan9303_get_ethtool_stats,
>       .get_sset_count         = lan9303_get_sset_count,
>       .port_enable            = lan9303_port_enable,
> --
> 2.17.1
>

Re: [PATCH net-next v4 2/2] dsa: lan9303: Migrate to PHYLINK

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 06:00:47PM +0000, Jerry.Ray@microchip.com wrote:
> > As a reader, I find my intelligence insulted by self-evident comments such as this.
> > 
> > Especially in contrast with the writes below to the MII_BMCR of the
> > Virtual PHY, which would certainly deserve a bit more of an explanation,
> > yet there is none there.
> > 
> 
> I struggle with the lack of comments I see in the kernel codebase. While
> experts can look at the source code and understand it, I find I spend a
> good deal of time chasing down macros - following data structures - and
> reverse engineering an understanding of the purpose of something that could
> have been explained in the maintained source.  In-line comments target the
> unfamiliar reader as there are a lot of us out here and far too few experts.
> But I do see your point and I'll try to do a better job on this.

I do see that maybe my observation about the rest of this driver's code
lacking comments might have been unfair to you since it's not you who
added that uncommented code. But still, let's try to add comments where
those add value, and there are plenty of other places in this driver
which sorely need that. I still maintain that "if (!dsa_is_cpu_port()) return"
doesn't need a repeat in words of what that set of instructions does.
Maybe why, or something that isn't completely obvious from actually
reading what's already in the code.