[PATCH net-next v5 0/6] dsa: lan9303: Move to PHYLINK

[PATCH net-next v5 0/6] dsa: lan9303: Move to PHYLINK

[Jerry Ray]

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

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.  The other
ports on the LAN9303 do not need anything from the phylink_mac_link_up api.

Patches:
 0001 - Whitespace only change aligning the dsa_switch_ops members.
	No code changes.
 0002 - Moves the Turbo bit initialization out of the adjust_link api and
	places it in a driver initialization execution path. It only needs
	to be initialized once, it is never changed, and it is not a
	per-port flag.
 0003 - Adds exception handling logic in the extremely unlikely event that
	the read of the device fails.
 0004 - Performance optimization that skips a slow register write if there is
	no need to perform it.
 0005 - Change the macro used to identify the cpu port as phydev will be NULL
	when this logic is moved into phylink_mac_link_up.
 0006 - Removes adjust_link and begins using the phylink dsa_switch_ops apis.

---
v3->v5:
  - Created prep patches to better show how things migrate.
  - cleaned up comments.
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 | xx ++++++++++++--------
 1 file changed

[PATCH net-next v5 1/6] dsa: lan9303: align dsa_switch_ops members

[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>
---
v4->v5:
 changed patch title to be less generic
 Addressed space-tab issue missed in the initial patch.
---
 drivers/net/dsa/lan9303-core.c | 38 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 80f07bd20593..5a21fc96d479 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1280,25 +1280,25 @@ 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,
-	.port_bridge_join       = lan9303_port_bridge_join,
-	.port_bridge_leave      = lan9303_port_bridge_leave,
-	.port_stp_state_set     = lan9303_port_stp_state_set,
-	.port_fast_age          = lan9303_port_fast_age,
-	.port_fdb_add           = lan9303_port_fdb_add,
-	.port_fdb_del           = lan9303_port_fdb_del,
-	.port_fdb_dump          = lan9303_port_fdb_dump,
-	.port_mdb_add           = lan9303_port_mdb_add,
-	.port_mdb_del           = lan9303_port_mdb_del,
+	.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,
+	.port_fast_age		= lan9303_port_fast_age,
+	.port_fdb_add		= lan9303_port_fdb_add,
+	.port_fdb_del		= lan9303_port_fdb_del,
+	.port_fdb_dump		= lan9303_port_fdb_dump,
+	.port_mdb_add		= lan9303_port_mdb_add,
+	.port_mdb_del		= lan9303_port_mdb_del,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
-- 
2.17.1

[PATCH net-next v5 2/6] dsa: lan9303: move Turbo Mode bit initialization

[Jerry Ray]

In preparing to remove the .adjust_link api, I am moving the one-time
initialization of the device's Turbo Mode bit into a different execution
path. This code clears (disables) the Turbo Mode bit which is never used
by this driver. Turbo Mode is a non-standard mode that would allow the
100Mbps RMII interface to run at 200Mbps.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 5a21fc96d479..20fc2af62531 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -886,6 +886,12 @@ static int lan9303_check_device(struct lan9303 *chip)
 		return ret;
 	}
 
+	/* Virtual Phy: Remove Turbo 200Mbit mode */
+	lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+
+	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
+	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+
 	return 0;
 }
 
@@ -1073,14 +1079,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 		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,
-- 
2.17.1

[PATCH net-next v5 3/6] dsa: lan9303: Add exception logic for read failure

[Jerry Ray]

While it is highly unlikely a read will ever fail, This code fragment is
now in a function that allows us to return an error code. A read failure
here will cause the lan9303_probe to fail.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 20fc2af62531..b0f49d9c3d0c 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -887,7 +887,9 @@ static int lan9303_check_device(struct lan9303 *chip)
 	}
 
 	/* Virtual Phy: Remove Turbo 200Mbit mode */
-	lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+	ret = lan9303_read(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, &reg);
+	if (ret)
+		return (ret);
 
 	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
 	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
-- 
2.17.1

[PATCH net-next v5 4/6] dsa: lan9303: Performance Optimization

[Jerry Ray]

As the regmap_write() is over a slow bus that will sleep, we can speed up
the boot-up time a bit my not bothering to clear a bit that is already
clear.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index b0f49d9c3d0c..694249aa1f19 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -891,8 +891,11 @@ static int lan9303_check_device(struct lan9303 *chip)
 	if (ret)
 		return (ret);
 
-	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
-	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+	/* Clear the TURBO Mode bit if it was set. */
+	if (reg & LAN9303_VIRT_SPECIAL_TURBO) {
+		reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
+		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH net-next v5 5/6] dsa: lan9303: Determine CPU port based on dsa_switch ptr

[Jerry Ray]

In preparing to move the adjust_link logic into the phylink_mac_link_up
api, change the macro used to check for the cpu port.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 694249aa1f19..1d22e4b74308 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
 	struct lan9303 *chip = ds->priv;
 	int ctl;
 
-	if (!phy_is_pseudo_fixed_link(phydev))
+	/* On this device, we are only interested in doing something here if
+	 * this is the CPU port. All other ports are 10/100 phys using MDIO
+	 * to control there link settings.
+	 */
+	if (!dsa_is_cpu_port(ds, port))
 		return;
 
 	ctl = lan9303_phy_read(ds, port, MII_BMCR);
-- 
2.17.1

[PATCH net-next v5 6/6] dsa: lan9303: Migrate to PHYLINK

[Jerry Ray]

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

The remaining functionality from the adjust_link is now covered in the
phylink_mac_link_up api.

Removes:
.adjust_link
Adds:
.phylink_get_caps
.phylink_mac_link_up

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
---
v4->v5:
  - Added various prep patches to better show the movement of the logic.
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 | 104 ++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 33 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 1d22e4b74308..810aef527fe1 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1058,38 +1058,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;
-
-	/* On this device, we are only interested in doing something here if
-	 * this is the CPU port. All other ports are 10/100 phys using MDIO
-	 * to control there link settings.
-	 */
-	if (!dsa_is_cpu_port(ds, port))
-		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);
-}
-
 static int lan9303_port_enable(struct dsa_switch *ds, int port,
 			       struct phy_device *phy)
 {
@@ -1286,13 +1254,83 @@ 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_port_is_cpu(dsa_to_port(ds, port))) {
+		/* cpu port */
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  config->supported_interfaces);
+	} else {
+		/* internal ports */
+		__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)
+{
+	u32 ctl;
+
+	/* On this device, we are only interested in doing something here if
+	 * this is the CPU port. All other ports are 10/100 phys using MDIO
+	 * to control there link settings.
+	 */
+	if (!dsa_is_cpu_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);
+}
+
 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 v5 2/6] dsa: lan9303: move Turbo Mode bit initialization

[Jakub Kicinski]

On Fri, 9 Dec 2022 16:47:09 -0600 Jerry Ray wrote:
> In preparing to remove the .adjust_link api, I am moving the one-time
> initialization of the device's Turbo Mode bit into a different execution
> path. This code clears (disables) the Turbo Mode bit which is never used
> by this driver. Turbo Mode is a non-standard mode that would allow the
> 100Mbps RMII interface to run at 200Mbps.

> @@ -1073,14 +1079,6 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  		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,

The chip variable has to go, otherwise there will be a warning in
bisection until patch 6 removes this entire function:

drivers/net/dsa/lan9303-core.c:1059:18: warning: unused variable 'chip' [-Wunused-variable]
        struct lan9303 *chip = ds->priv;
                        ^

Re: [PATCH net-next v5 5/6] dsa: lan9303: Determine CPU port based on dsa_switch ptr

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 04:47:12PM -0600, Jerry Ray wrote:
> In preparing to move the adjust_link logic into the phylink_mac_link_up
> api, change the macro used to check for the cpu port.

No justification given for why this change is made.

As a counter argument, I could point out that DSA can support configurations
where the CPU port is one of the 100BASE-TX PHY ports, and the MII port
can be used as a regular user port where a PHY has been connected. Some
people are already doing this, for example connecting a Beaglebone Black
(can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch
evaluation board.

The LAN9303 documentation makes it rather clear to me that such a
configuration would be possible, because the Switch Engine Ingress Port
Type Register allows any of the 3 switch ports to expect DSA tags. It's
lan9303_setup_tagging() the one who hardcodes the driver configuration
to be that where port 0 is the only acceptable CPU port (as well as the
early check from lan9303_setup()).

DSA's understanding of a CPU port is only that - a port which carries
DSA-tagged traffic, and is not exposed as a net_device to user space.
Nothing about MII vs internal PHY ports - that is a separate classification,
and a dsa_is_cpu_port() test is simply not something relevant from
phylink's perspective, to put it simply.

What we see in other device drivers for phylink handling is that there
is driver-level knowledge as to which ports have internal PHYs and which
have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[]
in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx,
felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding
port number 0 is also better from my perspective than looking for the
CPU port. That's because the switch IP was built with the idea in mind
that port 0 is MII.

I would very much appreciate if this driver did not make any assumptions
that the internal PHY ports cannot carry DSA-tagged traffic. This
assumption was true when the driver was introduced, but it changed with
commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports").
Coincidentally, that is also the commit which started prompting the
lan9303 driver for an update, via the dmesg warnings you are seeing.

It looks like there is potentially more code to unlock than this simple
API change, which is something you could look at.

> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 694249aa1f19..1d22e4b74308 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  	struct lan9303 *chip = ds->priv;
>  	int ctl;
>  
> -	if (!phy_is_pseudo_fixed_link(phydev))
> +	/* On this device, we are only interested in doing something here if
> +	 * this is the CPU port. All other ports are 10/100 phys using MDIO
> +	 * to control there link settings.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))
>  		return;
>  
>  	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -- 
> 2.17.1
> 

RE: [PATCH net-next v5 5/6] dsa: lan9303: Determine CPU port based on dsa_switch ptr

[Jerry.Ray]

> > In preparing to move the adjust_link logic into the phylink_mac_link_up
> > api, change the macro used to check for the cpu port.
> 
> No justification given for why this change is made.
>

The phydev parameter being passed into phylink_mac_link_up for the CPU port
was NULL, so I have to move to something else.
 
> As a counter argument, I could point out that DSA can support configurations
> where the CPU port is one of the 100BASE-TX PHY ports, and the MII port
> can be used as a regular user port where a PHY has been connected. Some
> people are already doing this, for example connecting a Beaglebone Black
> (can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch
> evaluation board.
> 
> The LAN9303 documentation makes it rather clear to me that such a
> configuration would be possible, because the Switch Engine Ingress Port
> Type Register allows any of the 3 switch ports to expect DSA tags. It's
> lan9303_setup_tagging() the one who hardcodes the driver configuration
> to be that where port 0 is the only acceptable CPU port (as well as the
> early check from lan9303_setup()).
> 
> DSA's understanding of a CPU port is only that - a port which carries
> DSA-tagged traffic, and is not exposed as a net_device to user space.
> Nothing about MII vs internal PHY ports - that is a separate classification,
> and a dsa_is_cpu_port() test is simply not something relevant from
> phylink's perspective, to put it simply.
> 
> What we see in other device drivers for phylink handling is that there
> is driver-level knowledge as to which ports have internal PHYs and which
> have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[]
> in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx,
> felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding
> port number 0 is also better from my perspective than looking for the
> CPU port. That's because the switch IP was built with the idea in mind
> that port 0 is MII.
> 
> I would very much appreciate if this driver did not make any assumptions
> that the internal PHY ports cannot carry DSA-tagged traffic. This
> assumption was true when the driver was introduced, but it changed with
> commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports").
> Coincidentally, that is also the commit which started prompting the
> lan9303 driver for an update, via the dmesg warnings you are seeing.
> 
> It looks like there is potentially more code to unlock than this simple
> API change, which is something you could look at.
>

I understand your point. The LAN9303 is very flexible, supporting an I2C
method for managing the switch and allowing the xMII to operate as either
MAC or PHY.

The original driver was written targeting the primary configuration most
widely used by our customers. The host CPU has an xMII interface and the
MDIO bus is used for control. Adding in the flexibility to support other
configurations is something I will investigate as I expand the driver to
support LAN9353/LAN9354/LAN9355 devices. Adding the
private->info->supports_mii[] as was done in the ksz drivers is the most
likely approach. I see this as a separate patch series.  Would you agree?

I will hardcode for port 0 at this point rather than looking at CPU port.

Regards,
Jerry.

> >
> > Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> > ---
> >  drivers/net/dsa/lan9303-core.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> > index 694249aa1f19..1d22e4b74308 100644
> > --- a/drivers/net/dsa/lan9303-core.c
> > +++ b/drivers/net/dsa/lan9303-core.c
> > @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> >       struct lan9303 *chip = ds->priv;
> >       int ctl;
> >
> > -     if (!phy_is_pseudo_fixed_link(phydev))
> > +     /* On this device, we are only interested in doing something here if
> > +      * this is the CPU port. All other ports are 10/100 phys using MDIO
> > +      * to control there link settings.
> > +      */
> > +     if (!dsa_is_cpu_port(ds, port))
> >               return;
> >
> >       ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > --
> > 2.17.1
> >
>

Re: [PATCH net-next v5 5/6] dsa: lan9303: Determine CPU port based on dsa_switch ptr

[Vladimir Oltean]

On Mon, Dec 12, 2022 at 05:42:01PM +0000, Jerry.Ray@microchip.com wrote:
> > It looks like there is potentially more code to unlock than this simple
> > API change, which is something you could look at.
> 
> I understand your point. The LAN9303 is very flexible, supporting an I2C
> method for managing the switch and allowing the xMII to operate as either
> MAC or PHY.
> 
> The original driver was written targeting the primary configuration most
> widely used by our customers. The host CPU has an xMII interface and the
> MDIO bus is used for control. Adding in the flexibility to support other
> configurations is something I will investigate as I expand the driver to
> support LAN9353/LAN9354/LAN9355 devices. Adding the
> private->info->supports_mii[] as was done in the ksz drivers is the most
> likely approach. I see this as a separate patch series.  Would you agree?
> 
> I will hardcode for port 0 at this point rather than looking at CPU port.

Yes, I think that would be ok. Thanks for at least not baking in any
more assumptions in the driver that already exist.

Re: [PATCH net-next v5 4/6] dsa: lan9303: Performance Optimization

[Vladimir Oltean]

Similar comment as for "Whitespace Only" patch. There could be a million
different changes in contents for this driver which could all be summarized
as "Performance Optimization". Please use your common sense, and also
consider people who might be looking through the git log for this driver
and see what is worth backporting. This is a clickbait commit message
with disappointing contents. Find a more appropriate and descriptive
summary for it.

On Fri, Dec 09, 2022 at 04:47:11PM -0600, Jerry Ray wrote:
> As the regmap_write() is over a slow bus that will sleep, we can speed up
> the boot-up time a bit my not bothering to clear a bit that is already
> clear.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index b0f49d9c3d0c..694249aa1f19 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -891,8 +891,11 @@ static int lan9303_check_device(struct lan9303 *chip)
>  	if (ret)
>  		return (ret);
>  
> -	reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
> -	regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
> +	/* Clear the TURBO Mode bit if it was set. */
> +	if (reg & LAN9303_VIRT_SPECIAL_TURBO) {
> +		reg &= ~LAN9303_VIRT_SPECIAL_TURBO;
> +		regmap_write(chip->regmap, LAN9303_VIRT_SPECIAL_CTRL, reg);
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.17.1
>