Re: [PATCH net-next v2 4/4] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2110 PHY

[Stefan Eichenberger <eichest@gmail.com>]

Hi Andrew,

Thanks a lot for the review and all the hints, I have one short question
below.

> > +static int mv88q2xxx_read_link(struct phy_device *phydev)
> > +{
> > +	u16 ret1, ret2;
> > +
> > +	/* The 88Q2XXX PHYs do not have the PMA/PMD status register available,
> > +	 * therefore we need to read the link status from the vendor specific
> > +	 * registers.
> > +	 */
> > +	if (phydev->speed == SPEED_1000) {
> > +		/* Read twice to clear the latched status */
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> > +		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
> 
> This is generally wrong. See for example genphy_update_link() and
> genphy_c45_read_link().
> 

Would something like this look fine to you? The issue is that I mix
realtime data with latched data because the local and remote rx status
is only available in realtime from what I can find in the datasheet.
This would be for gbit, I split that up compared to the last version:

static int mv88q2xxx_read_link_gbit(struct phy_device *phydev)
{
	int ret1, ret2;

	/* The link state is latched low so that momentary link drops can be
	 * detected. Do not double-read the status in polling mode to detect
	 * such short link drops except the link was already down. In case we
	 * are not polling, we always read the realtime status.
	 */
	if (!phy_polling_mode(phydev) || !phydev->link) {
		ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
		if (ret1 < 0)
			return ret1;
	}

	ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT);
	if (ret1 < 0)
		return ret1;

	/* Read vendor specific Auto-Negotiation status register to get local
	 * and remote receiver status according to software initialization
	 * guide.
	 */
	ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STATUS);
	if (ret2 < 0)
		return ret2;

	/* Check if we have link and if the remote and local receiver are ok */
	return (ret1 & MDIO_PCS_1000BT1_STAT_LINK) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_LOCAL_RX) &&
	       (ret2 & MDIO_MMD_AN_MV_STATUS_REMOTE_RX);
}

With this we will detect link loss in polling mode and read the realtime
status in non-polling mode. Compared to genphy_c45_read_link we will not
immediately return "link up" in non polling mode but always do the
second read to get the realtime link status.

If we are only interested in the link status we could also skip the
remote and local receiver check. However, as I understand the software
initialization guide it could be that the receivers are not ready in
that moment.

Regards,
Stefan