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

[Ioana Ciornei]

On Fri, Jun 02, 2023 at 02:11:14PM +0100, Russell King (Oracle) wrote:
> 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?
> 

I had a look into the AQR gen4 datasheet and the only thing that I could
find is the "PCS USX0 Auto-Neg Control Register" which has the following
field:

	USX0 Auto-Negotiation Message code [7:0]
	Configure the message opcode for USXGMII Auto-Negotiation.

It sounded promising but it's not making any sense to me because it's
just 8 bits long.

> 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?
>

No, the Lynx link status bit is 1 even though the media side is no
longer connected, for example. Here I just removed the cable, the link
on the PHY is down but the PCS link is up.

[  133.011696] fsl_dpaa2_eth dpni.2 endpmac3: Link is Down
[  133.555343] phylink_decode_usxgmii_word 3331: lpa 0x5601
[  133.560672] mdio_bus 0x0000000008c0f000:00: mode=usxgmii/10Gbps/Full link=1 an_complete=1
[  134.579348] phylink_decode_usxgmii_word 3331: lpa 0x5601


Ioana