Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature

[Heiner Kallweit <hkallweit1@gmail.com>]

On 06.02.2020 23:36, Dan Murphy wrote:
> Heiner
> 
> On 2/6/20 4:28 PM, Heiner Kallweit wrote:
>> 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.
> True.  This register is only going to return 1,2,4 and 8.  And it is defaulted to 4 attempts.
>>
>> 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.
> 
> Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting.  Kinda like matching the speeds.
> 

I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf
It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45)
Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's
an RW register. It's marked as RO however, maybe it's a typo in the datasheet.
Did you test whether register is writable?
Last but not least this register is exactly what's needed for the downshift tunable.

Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR.
In interrupt mode however this may require a custom interrupt handler (implementation of
handle_interrupt callback).

Alternatively you could check SPEED_OPT_STATUS in PHYSTS. It says "valid only during aneg"
but that sounds a little bit weird. Would need to be tested whether bit remains set after
downshifted aneg is finished.

> Dan
> 
Heiner