Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

[Ioana Ciornei]

On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@xxxxxxxxx wrote:
> > +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> > + * on some educated guesses. It appears that there are no status bits related to
> > + * autonegotiation complete or flow control.
> > + */
> > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > +						  int port, int lane,
> > +						  struct phylink_link_state *state)
> > +{
> > +	u16 status, lp_status;
> > +	int err;
> > +
> > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> > +	if (err) {
> > +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > +		return err;
> > +	}
> > +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> > +
> > +	state->link = !!(status & MDIO_USXGMII_LINK);
> > +
> > +	if (state->link) {
> > +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
> 
> What's the difference between these two registers? Specifically, what
> I'm asking is why the USXGMII partner status seems to be split between
> two separate registers.
> 
> Note that I think phylink_decode_usxgmii_word() is probably missing a
> check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
> USXGMII is pretty similar.)
> 
> MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
> media side or not. It's entirely possible for the USXGMII link to be
> up (thus allowing us to receive the "LPA" from the PHY) but for the
> PHY to be reporting to us that it has no media side link.
> 
> So, I think phylink_decode_usxgmii_word() at least needs at the
> beginning this added:
> 
> 	if (!(lpa & MDIO_USXGMII_LINK)) {
> 		state->link = false;
> 		return;
> 	}
> 
> The only user of this has been the Lynx PCS, so I'll add Ioana to this
> email as well to see whether there's any objection from Lynx PCS users
> to adding it.
>

I just tested this snippet on a LX2160ARDB that has USXGMII on two of
its lanes which go to Aquantia AQR113C PHYs.

The lpa read is 0x5601 and, with the patch, the interface does not link
up. I am not sure what is happening, if it's this PHY in particular that
does not properly set MDIO_USXGMII_LINK.

Ioana