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

[PATCH net-next v3 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

Clearing the Turbo Mode bit previously done in the adjust_link
API is moved to the driver initialization immediately following
the successful detection of a LAN93xx device.  It is forced to a
disabled state and never enabled.

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>

---
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 v3 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

Re: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 12:34:59PM -0600, Jerry Ray wrote:
> +/* 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));

You can put debugging prints in the code, but please, in the code that
you submit, do remove gratuitous poking in the master net_device.

> +	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)))

The ds->ops->port_max_mtu() function is never called for the CPU port.
You must know this, you put a debugging print right above. If this would
have been called for anything other than user ports, dsa_port_to_master()
would have triggered a NULL pointer dereference (dp->cpu_dp is set to
NULL for CPU ports).

So please remove dead code.

> +		return 1522 - ETH_HLEN - ETH_FCS_LEN;
> +	else
> +		return 1518 - ETH_HLEN - ETH_FCS_LEN;

Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".

Which brings me to a more serious question. If you say that the max_mtu
is equal to the default interface MTU (1500), and you provide no means
for the user to change the MTU to a different value, then why write the
patch? What behaves differently with and without it?

> +}
> +
>  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,
>  };

RE: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API

[Jerry.Ray]

> > +/* 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));
> 
> You can put debugging prints in the code, but please, in the code that
> you submit, do remove gratuitous poking in the master net_device.
> 
> > +     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)))
> 
> The ds->ops->port_max_mtu() function is never called for the CPU port.
> You must know this, you put a debugging print right above. If this would
> have been called for anything other than user ports, dsa_port_to_master()
> would have triggered a NULL pointer dereference (dp->cpu_dp is set to
> NULL for CPU ports).
> 
> So please remove dead code.
> 

I've written the function to handle being called with any port.  While I
couldn't directly exercise calling the port_max_mtu with the cpu port, I did
simulate it to verify it would work.

I'm using the dsa_to_port() rather than the dsa_port_to_master() function.

I'd rather include support for calling the api with the cpu port. I didn't
want to assume otherwise.  That's why I don't consider this dead code.

> > +             return 1522 - ETH_HLEN - ETH_FCS_LEN;
> > +     else
> > +             return 1518 - ETH_HLEN - ETH_FCS_LEN;
> 
> Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".
> 
> Which brings me to a more serious question. If you say that the max_mtu
> is equal to the default interface MTU (1500), and you provide no means
> for the user to change the MTU to a different value, then why write the
> patch? What behaves differently with and without it?
> 

I began adding the port_max_mtu api to attempt to get rid of the following
error message:
"macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead"

If someone were to check the max_mtu supported on the CPU port of the LAN9303,
they would see that 1504 is okay.

> > +}
> > +
> >  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,
> >  };
> 

Regards,
Jerry.

Re: [PATCH net-next v3 1/2] dsa: lan9303: Add port_max_mtu API

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 11:44:58PM +0000, Jerry.Ray@microchip.com wrote:
> > > +/* 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));
> > 
> > You can put debugging prints in the code, but please, in the code that
> > you submit, do remove gratuitous poking in the master net_device.
> > 
> > > +     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)))
> > 
> > The ds->ops->port_max_mtu() function is never called for the CPU port.
> > You must know this, you put a debugging print right above. If this would
> > have been called for anything other than user ports, dsa_port_to_master()
> > would have triggered a NULL pointer dereference (dp->cpu_dp is set to
> > NULL for CPU ports).
> > 
> > So please remove dead code.
> > 
> 
> I've written the function to handle being called with any port.  While I
> couldn't directly exercise calling the port_max_mtu with the cpu port, I did
> simulate it to verify it would work.
> 
> I'm using the dsa_to_port() rather than the dsa_port_to_master() function.

No, you're using the dsa_to_port() *and* the dsa_port_to_master() functions.
See? It's in the code you posted:

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));
			       ~~~~~~~~~~~~~~~~~~

> I'd rather include support for calling the api with the cpu port. I didn't
> want to assume otherwise.  That's why I don't consider this dead code.
> 
> > > +             return 1522 - ETH_HLEN - ETH_FCS_LEN;
> > > +     else
> > > +             return 1518 - ETH_HLEN - ETH_FCS_LEN;
> > 
> > Please replace "1518 - ETH_HLEN - ETH_FCS_LEN" with "ETH_DATA_LEN".
> > 
> > Which brings me to a more serious question. If you say that the max_mtu
> > is equal to the default interface MTU (1500), and you provide no means
> > for the user to change the MTU to a different value, then why write the
> > patch? What behaves differently with and without it?
> > 
> 
> I began adding the port_max_mtu api to attempt to get rid of the following
> error message:
> "macb f802c000.ethernet eth0: error -22 setting MTU to 1504 to include DSA overhead"

And how well did that go? That error message is saying that the macb driver
(drivers/net/ethernet/cadence/macb_main.c) does not accept the MTU of 1504.
Maybe because it doesn't have MACB_CAPS_JUMBO, I don't know. But this
patch is clearly unrelated to the problem you've observed.

> If someone were to check the max_mtu supported on the CPU port of the LAN9303,
> they would see that 1504 is okay.

No, they would not see that 1504 is okay. They would get a NULL pointer
dereference in your function, if port_max_mtu() was ever called for a
CPU port.

Don't believe me? You don't even have to. Please look at this patch,
study it, run it, and see what happens with your port_max_mtu()
implementation.

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..636e4b4df79a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -473,6 +473,12 @@ static int dsa_port_setup(struct dsa_port *dp)
 			break;
 		dsa_port_enabled = true;
 
+		if (ds->ops->port_max_mtu) {
+			dev_info(ds->dev, "max MTU of CPU port %d is %d\n",
+				 dp->index,
+				 ds->ops->port_max_mtu(ds, dp->index));
+		}
+
 		break;
 	case DSA_PORT_TYPE_DSA:
 		if (dp->dn) {

The max_mtu of the CPU port is simply a question that the DSA core does
not ask, so there's no reason to report it. How things are supposed to
work is that the max_mtu of the user ports is propagated to their
net_devices, and when the MTU of any user port is changed, the
port_change_mtu() of that user port is called, and the maximum MTU of
all user ports is recalculated and all CPU and DSA ports also get a
port_change_mtu() call with that maximum value. If those ports need to
program their hardware with something that also includes their tagging
protocol overhead, they do so privately.

[PATCH net-next v3 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>
---
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 | 79 ++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index baa336bb9d15..c6236b328ed8 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -886,6 +886,13 @@ static int lan9303_check_device(struct lan9303 *chip)
 		return ret;
 	}
 
+	/* Virtual Phy: Always disable Turbo 200Mbit mode */
+	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);
+
 	return 0;
 }
 
@@ -1047,42 +1054,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 +1250,40 @@ 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;
+}
+
 /* 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 +1309,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 v3 2/2] dsa: lan9303: Move to PHYLINK

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 12:35:00PM -0600, Jerry Ray wrote:
> This patch replaces the .adjust_link api with the .phylink_get_caps api.

Am I supposed to read this commit description and understand what the
change does?

You can't "replace" adjust_link with phylink_get_caps, since they don't
do the same thing. The equivalent set of operations are roughly
phylink_mac_config and phylink_mac_link_up, probably just the latter in
your case.

By deleting adjust_link and not replacing with any of the above, the
change is telling me that nothing from adjust_link was needed?

> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
> 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 | 79 ++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index baa336bb9d15..c6236b328ed8 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -886,6 +886,13 @@ static int lan9303_check_device(struct lan9303 *chip)
>  		return ret;
>  	}
>  
> +	/* Virtual Phy: Always disable Turbo 200Mbit mode */
> +	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);

Separate patch which moves this, please.

> +
>  	return 0;
>  }
>  
> @@ -1047,42 +1054,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);

Are you going to explain why modifying this register is no longer needed?

> -
> -	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 +1250,40 @@ 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 */

This comment and the "internal ports" are absolutely redundant, they
bring nothing to the understanding of the code.

> +		__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 +1309,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,

Some of the lan9303_switch_ops are not aligned, and some are aligned
with spaces. None with tabs. Please be consistent with something that
exists, or create a preparatory patch which brings some more consistence
with the way in which you want things to be.

>  	.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 v3 2/2] dsa: lan9303: Move to PHYLINK

[Russell King (Oracle)]

On Tue, Dec 06, 2022 at 09:32:24PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 06, 2022 at 12:35:00PM -0600, Jerry Ray wrote:
> > This patch replaces the .adjust_link api with the .phylink_get_caps api.
> 
> Am I supposed to read this commit description and understand what the
> change does?
> 
> You can't "replace" adjust_link with phylink_get_caps, since they don't
> do the same thing. The equivalent set of operations are roughly
> phylink_mac_config and phylink_mac_link_up, probably just the latter in
> your case.
> 
> By deleting adjust_link and not replacing with any of the above, the
> change is telling me that nothing from adjust_link was needed?

...

> > -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;

If this is a not a fixed link, adjust_link does nothing.

> > -
> > -	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);
> 
> Are you going to explain why modifying this register is no longer needed?

... otherwise it is a fixed link, so the PHY is configured for the fixed
link setting - which I think would end up writing to the an emulation of
the PHY, and would end up writing the same settings back to the PHY as
the PHY was already configured.

So, I don't think adjust_link does anything useful, and I think this is
an entirely appropriate change.

-- 
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 v3 2/2] dsa: lan9303: Move to PHYLINK

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 09:07:54PM +0000, Russell King (Oracle) wrote:
> > Are you going to explain why modifying this register is no longer needed?
> 
> ... otherwise it is a fixed link, so the PHY is configured for the fixed
> link setting - which I think would end up writing to the an emulation of
> the PHY, and would end up writing the same settings back to the PHY as
> the PHY was already configured.

To be clear, when you say "an emulation of the PHY", are you talking
about the swphy behind the fixed-link, or about the LAN9303_VIRT_PHY_BASE
registers, which correspond to the RevMII Virtual PHY of the switch CPU port?

As far as I can understand the Microchip LAN9303 documentation, the DSA
master can have a phy-handle to the switch node (which
devicetree/bindings/net/dsa/lan9303.txt seems to confirm), and the
switch can pretend it's a PHY when accessed by a switch-unaware
(Generic) PHY driver at the usual PHY MDIO registers. Through the
Virtual PHY feature and registers, it can also pretend it's the "other"
PHY, and this MII_BMCR register of the Virtual PHY can ultimately
autoneg with "itself" and control what the DSA master sees in terms of
reported speed, duplex, and AN complete.

Prior to this change, the driver, when given a DT blob with a fixed-link
on the switch CPU port, would disable BMCR_ANENABLE in the Virtual PHY.
After the change, it would leave things as they are (which is not
necessarily the way things are out of reset). Which way is better?
Does it matter? Is it a stupid question? No clue.

> So, I don't think adjust_link does anything useful, and I think this is
> an entirely appropriate change.

That it may well be, but its presentation is entirely inappropriate.
Andrew has told Jerry before that it's important to split, describe and
justify his changes accordingly, so it's not like the things I'm
complaining about are news to him. Things would go a lot smoother if
Jerry explained his patches better.

Reviewing patches which do stuff that isn't explained in the commit
message reminds me of Forrest Gump. Life is like a box of chocolates,
you never know what you're going to get. That's not how it's supposed to
work, a box of chocolates should contain chocolates, at least here.

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

[Jerry.Ray]

> > > This patch replaces the .adjust_link api with the .phylink_get_caps api.
> >
> > Am I supposed to read this commit description and understand what the
> > change does?
> >
> > You can't "replace" adjust_link with phylink_get_caps, since they don't
> > do the same thing. The equivalent set of operations are roughly
> > phylink_mac_config and phylink_mac_link_up, probably just the latter in
> > your case.
> >
> > By deleting adjust_link and not replacing with any of the above, the
> > change is telling me that nothing from adjust_link was needed?
> 
> ...
> 
> > > -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;
> 
> If this is a not a fixed link, adjust_link does nothing.
> 
> > > -
> > > -   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);
> >
> > Are you going to explain why modifying this register is no longer needed?
> 
> ... otherwise it is a fixed link, so the PHY is configured for the fixed
> link setting - which I think would end up writing to the an emulation of
> the PHY, and would end up writing the same settings back to the PHY as
> the PHY was already configured.
> 
> So, I don't think adjust_link does anything useful, and I think this is
> an entirely appropriate change.
> 
> 

I, too, thought I would need phylink_mac_config and phylink_mac_link_up to
completely migrate away from PHYLIB to PHYLINK.  But it seems as though the
phylink layer is now taking care of the speed / duplex / etc settings of the
phy registers.  There is no DSA driver housecleaning needed at these other
phylink_ api hook functions.  At least, that's my current level of
understanding...

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

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 10:58:25PM +0000, Jerry.Ray@microchip.com wrote:
> I, too, thought I would need phylink_mac_config and phylink_mac_link_up to
> completely migrate away from PHYLIB to PHYLINK.  But it seems as though the
> phylink layer is now taking care of the speed / duplex / etc settings of the
> phy registers.  There is no DSA driver housecleaning needed at these other
> phylink_ api hook functions.  At least, that's my current level of
> understanding...

Did you understand my request? To create a separate patch, not phylink
conversion, which deletes the MII_BMCR writes to the Virtual PHY, which
says that you don't know why they're there and you don't think that
they're needed, but at least you thought about what you're doing?

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

[Vladimir Oltean]

On Tue, Dec 06, 2022 at 12:34:58PM -0600, Jerry Ray wrote:
> 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

What does the max MTU have to do with phylink? What is it that makes
these two patches related?

> 
> Clearing the Turbo Mode bit previously done in the adjust_link
> API is moved to the driver initialization immediately following
> the successful detection of a LAN93xx device.  It is forced to a
> disabled state and never enabled.
> 
> 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>

You don't need to put your sign off on the cover letter.

RE: [PATCH net-next v3 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
> 
> What does the max MTU have to do with phylink? What is it that makes
> these two patches related?
> 

I'm touching the same file, so I created this series of patches to avoid
piecewise patching conflicts that might have resulted if they were
independent patches.

> >
> > Clearing the Turbo Mode bit previously done in the adjust_link
> > API is moved to the driver initialization immediately following
> > the successful detection of a LAN93xx device.  It is forced to a
> > disabled state and never enabled.
> >
> > 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>
> 
> You don't need to put your sign off on the cover letter.
> 

Understood.  Thx,
J.