Re: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN support

[Russell King (Oracle)]

On Fri, Jun 10, 2022 at 11:29:38AM +0800, Ong Boon Leong wrote:
> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> +					struct phylink_link_state *state)
> +{
> +	int lpa, adv;
> +	int ret;
> +
> +	if (state->an_enabled) {
> +		/* Reset link state */
> +		state->link = false;
> +
> +		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
> +		if (lpa < 0 || lpa & LPA_RFAULT)
> +			return lpa;
> +
> +		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
> +		if (adv < 0)
> +			return adv;
> +
> +		if (lpa & ADVERTISE_1000XFULL &&
> +		    adv & ADVERTISE_1000XFULL) {
> +			state->link = true;
> +			state->speed = SPEED_1000;
> +			state->duplex = DUPLEX_FULL;
> +		}

phylink_mii_c22_pcs_decode_state() is your friend here, will implement
this correctly, and will set lp_advertising correctly as well.

> +
> +		/* Clear CL37 AN complete status */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;

Why do you need to clear the interrupt status here? This function will
be called from a work queue sometime later after an interrupt has fired.
It will also be called at random times when userspace enquires what the
link parameters are, so clearing the interrupt here can result in lost
link changes.

> +static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, int speed,
> +				   int duplex)
> +{
> +	int val, ret;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		val = BMCR_SPEED1000;
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +	default:
> +		pr_err("%s: speed = %d\n", __func__, speed);
> +		return;
> +	}
> +
> +	if (duplex == DUPLEX_FULL)
> +		val |= BMCR_FULLDPLX;
> +	else
> +		pr_err("%s: half duplex not supported\n", __func__);
> +
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
> +	if (ret)
> +		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));

Does this need to be done even when AN is enabled?

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