On 1/31/20 10:29 AM, Dan Murphy wrote: > Florian > > On 1/31/20 11:49 AM, Florian Fainelli wrote: >> On 1/31/20 7:11 AM, Dan Murphy wrote: >>> 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. > >> 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? > >>> 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
