On 1/31/20 10:29 AM, Dan Murphy wrote:

Florian

On 1/31/20 11:49 AM, Florian Fainelli wrote:

quoted

On 1/31/20 7:11 AM, Dan Murphy wrote:

quoted

Set the speed optimization bit on the DP83867 PHY.
This feature can also be strapped on the 64 pin PHY devices
but the 48 pin devices do not have the strap pin available to enable
this feature in the hardware.  PHY team suggests to have this bit set.

OK, but why and how does that optimization work exactly?

I described this in the cover letter.  And it is explained in the data
sheet Section 8.4.6.6

Sorry I complete missed that and just focused on the patch, you should
consider not providing a cover letter for a single patch, and especially
not when the cover letter contains more information than the patch
commit message itself.

quoted

  Departing from
the BMSR reads means you possibly are going to introduce bugs and/or
incomplete information. For instance, you set phydev->pause and
phydev->asym_pause to 0 now, is there no way to extract what the link
partner has advertised?

I was using the marvel.c as my template as it appears to have a separate
status register as well.

Instead of setting those bits in the call back I can call the
genphy_read_status then override the duplex and speed based on the
physts register like below.  This way link status and pause values can
be updated and then we can update the speed and duplex settings.

      ret = genphy_read_status(phydev);
    if (ret)
        return ret;

    if (status < 0)
        return status;

    if (status & DP83867_PHYSTS_DUPLEX)
        phydev->duplex = DUPLEX_FULL;
    else
        phydev->duplex = DUPLEX_HALF;

    if (status & DP83867_PHYSTS_1000)
        phydev->speed = SPEED_1000;
    else if (status & DP83867_PHYSTS_100)
        phydev->speed = SPEED_100;
    else
        phydev->speed = SPEED_10;

OK, but what if they disagree, are they consistently latched with
respect to one another?

quoted

quoted

With this bit set the PHY will auto negotiate and report the link
parameters in the PHYSTS register and not in the BMCR.

That should be BMSR, the BMCR is about control, not status.

OK.

Dan

-- 
Florian