Messages in this thread | | | From | "Ong, Boon Leong" <> | Subject | RE: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN support | Date | Mon, 13 Jun 2022 23:44:35 +0000 |
| |
>> +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!
| |