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

[Ong, Boon Leong]

>> +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.
Thank for the suggestion. I will change it accordingly to use
phylink_mii_c22_pcs_decode_state()

>
>> +
>> +		/* 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.
Thanks for pointing it. Ya, it is unnecessary.

>
>> +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?
Thanks. Ya, it does not need. Will fix. 


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