Messages in this thread |  | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Heiner Kallweit <> | Date | Thu, 6 Feb 2020 23:28:13 +0100 |
| |
On 06.02.2020 23:13, Dan Murphy wrote: > Heiner > > On 2/5/20 3:16 PM, Heiner Kallweit wrote: >> On 04.02.2020 19:13, 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. >>> >>> With this bit set the PHY will auto negotiate and report the link >>> parameters in the PHYSTS register. This register provides a single >>> location within the register set for quick access to commonly accessed >>> information. >>> >>> In this case when auto negotiation is on the PHY core reads the bits >>> that have been configured or if auto negotiation is off the PHY core >>> reads the BMCR register and sets the phydev parameters accordingly. >>> >>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>> 4-wire cable. If this should occur the PHYSTS register contains the >>> current negotiated speed and duplex mode. >>> >>> In overriding the genphy_read_status the dp83867_read_status will do a >>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>> register is read and the phydev speed and duplex mode settings are >>> updated. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> v2 - Updated read status to call genphy_read_status first, added link_change >>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>> >> As stated in the first review, it would be appreciated if you implement >> also the downshift tunable. This could be a separate patch in this series. >> Most of the implementation would be boilerplate code. > > > I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. > The get operation for the downshift tunable should return after how many failed attempts the PHY starts a downshift. This doesn't match with your description of this register, so yes: Implementing the tunable for this PHY doesn't make sense.
However this register may be useful in the link_change_notify() callback to figure out whether a downshift happened, to trigger the info message you had in your first version.
> So we should probably skip this for this PHY. > > Dan > Heiner
|  |