Re: [PATCH net-next v2 08/10] net: phy: marvell: Add cable test support

[Andrew Lunn <andrew@lunn.ch>]

> > +static int marvell_vct7_cable_test_start(struct phy_device *phydev)
> > +{
> > +	int bmcr, bmsr, ret;
> > +
> > +	/* If auto-negotiation is enabled, but not complete, the cable
> > +	 * test never completes. So disable auto-neg.
> > +	 */
> > +	bmcr = phy_read(phydev, MII_BMCR);
> > +	if (bmcr < 0)
> > +		return bmcr;
> > +
> > +	bmsr = phy_read(phydev, MII_BMSR);
> > +
> > +	if (bmsr < 0)
> > +		return bmsr;
> > +
> > +	if (bmcr & BMCR_ANENABLE) {
> > +		ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
> > +		if (ret < 0)
> > +			return ret;
> > +		ret = genphy_soft_reset(phydev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	/* If the link is up, allow it some time to go down */
> > +	if (bmsr & BMSR_LSTATUS)
> > +		msleep(1500);
> 
> Is it mandatory to wait 1.5s unconditionally or can we poll for link down?

Polling is fine. I think i got this from the Marvell SDK.

> > +
> > +	return phy_write_paged(phydev, MII_MARVELL_VCT7_PAGE,
> > +			       MII_VCT7_CTRL,
> > +			       MII_VCT7_CTRL_RUN_NOW |
> > +			       MII_VCT7_CTRL_CENTIMETERS);
> > +}
> > +
> > +static int marvell_vct7_distance_to_length(int distance, bool meter)
> > +{
> > +	if (meter)
> > +		distance *= 100;
> 
> I've never understood the use of the meter unit. If we always use
> centimeters, we have 2^16 cm = 655m, shouldn't that be enough given
> that the max cable length is 100m in TP ethernet? Also you hardcode
> the unit to centimeters, so this should be superfluous, making this
> function a noop.

Yes, it should never be used now. But i did use it initially, to see
if the results were different/better. It is a rather odd design, and
i'm wondering if some of the older PHYs only have meters? I guess i
should go look at the SDK.

> > +	return distance;
> > +}
> > +
> > +static bool marvell_vct7_distance_valid(int result)
> > +{
> > +	switch (result) {
> > +	case MII_VCT7_RESULTS_OPEN:
> > +	case MII_VCT7_RESULTS_SAME_SHORT:
> > +	case MII_VCT7_RESULTS_CROSS_SHORT:
> 
> btw on the BCM54140 I've observed, that if you have a intra-pair
> short, the length is wrong; looks like it is twice the value it
> should be.
> 
> Does the Marvell PHY report the correct value?

I've not tested that.

Chris, do you have results for this test?

> > +static int marvell_vct7_cable_test_report(struct phy_device *phydev)
> > +{
> > +	int pair0, pair1, pair2, pair3;
> > +	bool meter;
> > +	int ret;
> > +
> > +	ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE,
> > +			     MII_VCT7_RESULTS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pair3 = (ret & MII_VCT7_RESULTS_PAIR3_MASK) >>
> > +		MII_VCT7_RESULTS_PAIR3_SHIFT;
> > +	pair2 = (ret & MII_VCT7_RESULTS_PAIR2_MASK) >>
> > +		MII_VCT7_RESULTS_PAIR2_SHIFT;
> > +	pair1 = (ret & MII_VCT7_RESULTS_PAIR1_MASK) >>
> > +		MII_VCT7_RESULTS_PAIR1_SHIFT;
> > +	pair0 = (ret & MII_VCT7_RESULTS_PAIR0_MASK) >>
> > +		MII_VCT7_RESULTS_PAIR0_SHIFT;
> 
> I'm sure you know FIELD_GET(), so there must be another
> reason why you use mask and shift, consistency?

Consistency, and FIELD_GET() just looks odd, only taking a mask, not a
shift.

     Andrew