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

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

[Jerry Ray]

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

1) adds port_max_mtu api support.
2) Replace .adjust_link with .phylink_get_caps dsa api

At this point, I do not see anything this driver needs from the other
phylink APIs.

Signed-off-by: Jerry Ray <jerry.ray@microchip.com>

---
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 v2 1/2] dsa: lan9303: Add port_max_mtu API

[Jerry Ray]

Adding in support for reporting the max mtu for a given
port.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 80f07bd20593..baa336bb9d15 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1279,6 +1279,25 @@ static int lan9303_port_mdb_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+/* For non-cpu ports, the max frame size is 1518.
+ * The CPU port supports a max frame size of 1522.
+ * There is a JUMBO flag to make the max size 2048, but this driver
+ * presently does not support using it.
+ */
+static int lan9303_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	struct net_device *p = dsa_port_to_master(dsa_to_port(ds, port));
+	struct lan9303 *chip = ds->priv;
+
+	dev_dbg(chip->dev, "%s(%d) entered. NET max_mtu is %d",
+		__func__, port, p->max_mtu);
+
+	if (dsa_port_is_cpu(dsa_to_port(ds, port)))
+		return 1522 - ETH_HLEN - ETH_FCS_LEN;
+	else
+		return 1518 - ETH_HLEN - ETH_FCS_LEN;
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
@@ -1299,6 +1318,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 	.port_fdb_dump          = lan9303_port_fdb_dump,
 	.port_mdb_add           = lan9303_port_mdb_add,
 	.port_mdb_del           = lan9303_port_mdb_del,
+	.port_max_mtu		= lan9303_port_max_mtu,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
-- 
2.17.1

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

[Jerry Ray]

This patch replaces the .adjust_link api with the .phylink_get_caps api.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index baa336bb9d15..2baf6ae7e0a0 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,6 +1243,41 @@ 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 */
+		phy_interface_empty(config->supported_interfaces);
+		__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;
+}
+
 /* For non-cpu ports, the max frame size is 1518.
  * The CPU port supports a max frame size of 1522.
  * There is a JUMBO flag to make the max size 2048, but this driver
@@ -1304,7 +1303,7 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 	.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,
 	.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 v2 2/2] dsa: lan9303: Move to PHYLINK

[Russell King (Oracle)]

Hi Jerry,

On Fri, Dec 02, 2022 at 01:17:49PM -0600, Jerry Ray wrote:
> -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);
> -	}
> -}

Is this functionality no longer necessary? For example, I don't see
anywhere else in the driver that this turbo mode is disabled.

I'm guessing the above code writing MII_BMCR is to force the
configuration of integrated PHYs to be the fixed-link settings?
How is that dealt with after the removal of the above code?

> -
>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  			       struct phy_device *phy)
>  {
> @@ -1279,6 +1243,41 @@ 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 */
> +		phy_interface_empty(config->supported_interfaces);

This should not be necessary - the supported_interfaces member should
already be zero.

Thanks.

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

RE: [PATCH net-next v2 2/2] dsa: lan9303: Move to PHYLINK

[Jerry.Ray]

>> -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);
>> -     }
>> -}
>
>Is this functionality no longer necessary? For example, I don't see
>anywhere else in the driver that this turbo mode is disabled.
>
>I'm guessing the above code writing MII_BMCR is to force the
>configuration of integrated PHYs to be the fixed-link settings?
>How is that dealt with after the removal of the above code?
>

While it should be disabled by the HW config strap settings, I'll add
disabling Turbo Mode into the initialization sequence to keep the driver
operation consistent.

>> -
>>  static int lan9303_port_enable(struct dsa_switch *ds, int port,
>>                              struct phy_device *phy)
>>  {
>> @@ -1279,6 +1243,41 @@ 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 */
>> +             phy_interface_empty(config->supported_interfaces);
>
>This should not be necessary - the supported_interfaces member should
>already be zero.
>
>Thanks.
>

Yes, I agree.  I'll remove it.

Regards,
J.