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

[Russell King (Oracle)]

On Fri, Jun 02, 2023 at 02:28:04PM +0300, Ioana Ciornei wrote:
> 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.

Thanks for testing. I wonder if the USXGMII control word that the PHY
transmits can be read from one of its registers?

If the PHY is correctly setting the link bit, but it's not appearing
in the Lynx PCS registers as set, that would be weird, and suggest the
PCS is handling that itself. Does the Lynx link status bit change
according to the media link status, while the AN complete bit stays
set?

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