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

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

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
>
  

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.
  

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.
  

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

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?