Re: [PATCH RFC net-next 1/2] net: pcs: xpcs: Add special code to operate in Microchip KSZ9477 switch

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Jan 27, 2025 at 07:32:25PM -0800, Tristram.Ha@microchip.com wrote:
> For 1000BaseX mode setting neg_mode to false works, but that does not
> work for SGMII mode.  Setting 0x18 value in register 0x1f8001 allows
> 1000BaseX mode to work with auto-negotiation enabled.

I'm not sure (a) exactly what the above paragraph is trying to tell me,
and (b) why setting the AN control register to 0x18, which should only
affect SGMII, has an effect on 1000BASE-X.

Note that a config word formatted for SGMII can result in a link with
1000BASE-X to come up, but it is not correct. So, I highly recommend you
check the config word sent by the XPCS to the other end of the link.
Bit 0 of that will tell you whether it is SGMII-formatted or 802.3z
formatted.

> However SGMII mode in KSZ9477 has a bug in which the current speed
> needs to be set in MII_BMCR to pass traffic.  The current driver code
> does not do anything when link is up with auto-negotiation enabled, so
> that code needs to be changed for KSZ9477.
> 
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  drivers/net/pcs/pcs-xpcs.c   | 52 ++++++++++++++++++++++++++++++++++--
>  drivers/net/pcs/pcs-xpcs.h   |  2 ++
>  include/linux/pcs/pcs-xpcs.h |  6 +++++
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 1faa37f0e7b9..ddf6cd4b37a7 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -768,6 +768,14 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
>  		val |= DW_VR_MII_AN_INTR_EN;
>  	}
>  
> +	if (xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		mask |= DW_VR_MII_SGMII_LINK_STS | DW_VR_MII_TX_CONFIG_MASK;
> +		val |= FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
> +				  DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII);
> +		val |= DW_VR_MII_SGMII_LINK_STS;
> +	}
> +
>  	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, mask, val);
>  	if (ret < 0)
>  		return ret;
> @@ -982,6 +990,15 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* DW_VR_MII_AN_STS_C37_ANCMPLT_INTR just means link change in SGMII
> +	 * mode, so needs to be cleared.  It can appear just by itself, which
> +	 * does not mean the link is up.
> +	 */
> +	if (xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ &&
> +	    (ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR)) {
> +		ret &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
> +		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	}

*_get_state() is not an interrupt acknowledgement function. It isn't
_necessarily_ called when an interrupt has happened, and it will be
called "sometime after" the interrupt has been handled as it's called
from an entirely separate workqueue.

Would it be better to just ignore the block following:

	} else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {

instead? I'm not sure that block of code/if statement was correct,
and was added in "net: pcs: xpcs: adapt Wangxun NICs for SGMII mode".

>  	if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
>  		int speed_value;
>  
> @@ -1037,6 +1054,18 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
>  {
>  	int lpa, bmsr;
>  
> +	/* DW_VR_MII_AN_STS_C37_ANCMPLT_INTR just means link change, so needs
> +	 * to be cleared.  If polling is not used then it is cleared by
> +	 * following code.
> +	 */
> +	if (xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ && xpcs->pcs.poll) {
> +		int val;
> +
> +		val = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
> +		if (val & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR)
> +			xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS,
> +				   0);
> +	}

Same concerns.

>  	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>  			      state->advertising)) {
>  		/* Reset link state */
> @@ -1138,9 +1167,14 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
>  					 phy_interface_t interface,
>  					 int speed, int duplex)
>  {
> +	u16 val = 0;
>  	int ret;
>  
> -	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +	/* Microchip KSZ SGMII implementation has a bug that needs MII_BMCR
> +	 * register to be updated with current speed to pass traffic.
> +	 */
> +	if (xpcs->quirk != DW_XPCS_QUIRK_MICROCHIP_KSZ &&

	if (!(xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ &&
	      interface != PHY_INTERFACE_MODE_SGMII) &&

> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>  		return;
>  
>  	if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> @@ -1155,10 +1189,18 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
>  			dev_err(&xpcs->mdiodev->dev,
>  				"%s: half duplex not supported\n",
>  				__func__);
> +
> +		/* No need to update MII_BMCR register if not in SGMII mode. */
> +		if (xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ &&
> +		    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +			return;

then you don't need this.

>  	}
>  
> +	if (xpcs->quirk == DW_XPCS_QUIRK_MICROCHIP_KSZ &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +		val = BMCR_ANENABLE;
>  	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> -			 mii_bmcr_encode_fixed(speed, duplex));
> +			 val | mii_bmcr_encode_fixed(speed, duplex));

I think in this case, I'd prefer this to just modify the speed and
duplex bits rather than writing the whole register.

>  	if (ret)
>  		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
>  			__func__, ERR_PTR(ret));
> @@ -1563,5 +1605,11 @@ void xpcs_destroy_pcs(struct phylink_pcs *pcs)
>  }
>  EXPORT_SYMBOL_GPL(xpcs_destroy_pcs);
>  
> +void xpcs_set_quirk(struct dw_xpcs *xpcs, int quirk)
> +{
> +	xpcs->quirk = quirk;
> +}
> +EXPORT_SYMBOL_GPL(xpcs_set_quirk);

According to the KSZ9477 data, the physid is 0x7996ced0 (which is the
DW value according to the xpcs header file). We also read the PMA ID
(xpcs->info.pma). Can this be used to identify the KSZ9477 without
introducing quirks?

I would prefer to avoid going down the route of introducing a quirk
mask - that seems to be a recipe to breed lots of flags that make
long term maintenance more difficult.

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