Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Dec 2022 19:21:05 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Jerry Ray <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
Message-ID: <20221208172105.4736qmzfckottfvm@skbuf>
References: <20221207232828.7367-1-jerry.ray@microchip.com>
 <20221207232828.7367-1-jerry.ray@microchip.com>
 <20221207232828.7367-3-jerry.ray@microchip.com>
 <20221207232828.7367-3-jerry.ray@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221207232828.7367-3-jerry.ray@microchip.com>
 <20221207232828.7367-3-jerry.ray@microchip.com>
Precedence: bulk

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
>