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

From: Ioana Ciornei <ioana.ciornei@nxp.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: msmulski2@gmail.com, andrew@lunn.ch, f.fainelli@gmail.com,
	olteanv@gmail.com, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, simon.horman@corigine.com,
	kabel@kernel.org, Michal Smulski <michal.smulski@ooma.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x
Date: Fri, 2 Jun 2023 14:28:04 +0300	[thread overview]
Message-ID: <20230602112804.32ffi7mpk5xfzvq4@LXL00007.wbi.nxp.com> (raw)
In-Reply-To: <ZHnEzPadBBbfwj18@shell.armlinux.org.uk>

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@gmail.com 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