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