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

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 06/02/2020 00:01, Dan Murphy wrote:
> Florian
> 
> On 2/5/20 4:00 PM, Florian Fainelli wrote:
>> On 2/5/20 1:51 PM, 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 just don't have a requirement from our customer to make it adjustable
>>> so I did not want to add something extra.
>>>
>>> I can add in for v3.
>>>
>>>> And I have to admit that I'm not too happy with the term "speed
>>>> optimization".
>>>> This sounds like the PHY has some magic to establish a 1.2Gbps link.
>>>> Even though the vendor may call it this way in the datasheet, the
>>>> standard
>>>> term is "downshift". I'm fine with using "speed optimization" in
>>>> constants
>>>> to be in line with the datasheet. Just a comment in the code would be
>>>> helpful
>>>> that speed optimization is the vendor's term for downshift.
>>> Ack.  The data sheet actually says "Speed optimization, also known as
>>> link downshift"
>>>
>>> So I probably will just rename everything down shift.
>>>
>>>>>    drivers/net/phy/dp83867.c | 55 +++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
>>>>> index 967f57ed0b65..6f86ca1ebb51 100644
>>>>> --- a/drivers/net/phy/dp83867.c
>>>>> +++ b/drivers/net/phy/dp83867.c
>>>>> @@ -21,6 +21,7 @@
>>>>>    #define DP83867_DEVADDR        0x1f
>>>>>      #define MII_DP83867_PHYCTRL    0x10
>>>>> +#define MII_DP83867_PHYSTS    0x11
>>>>>    #define MII_DP83867_MICR    0x12
>>>>>    #define MII_DP83867_ISR        0x13
>>>>>    #define DP83867_CFG2        0x14
>>>>> @@ -118,6 +119,15 @@
>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_MASK    (0x1f << 8)
>>>>>    #define DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT    8
>>>>>    +/* PHY STS bits */
>>>>> +#define DP83867_PHYSTS_1000            BIT(15)
>>>>> +#define DP83867_PHYSTS_100            BIT(14)
>>>>> +#define DP83867_PHYSTS_DUPLEX            BIT(13)
>>>>> +#define DP83867_PHYSTS_LINK            BIT(10)
>>>>> +
>>>>> +/* CFG2 bits */
>>>>> +#define DP83867_SPEED_OPTIMIZED_EN        (BIT(8) | BIT(9))
>>>>> +
>>>>>    /* CFG3 bits */
>>>>>    #define DP83867_CFG3_INT_OE            BIT(7)
>>>>>    #define DP83867_CFG3_ROBUST_AUTO_MDIX        BIT(9)
>>>>> @@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device
>>>>> *phydev)
>>>>>        return phy_write(phydev, MII_DP83867_MICR, micr_status);
>>>>>    }
>>>>>    +static void dp83867_link_change_notify(struct phy_device *phydev)
>>>>> +{
>>>>> +    if (phydev->state != PHY_RUNNING)
>>>>> +        return;
>>>>> +
>>>>> +    if (phydev->speed == SPEED_100 || phydev->speed == SPEED_10)
>>>>> +        phydev_warn(phydev, "Downshift detected connection is
>>>>> %iMbps\n",
>>>>> +                phydev->speed);
>>>> The link partner may simply not advertise 1Gbps. How do you know that
>>>> a link speed of e.g. 100Mbps is caused by a downshift?
>>>> Some PHY's I've seen with this feature have a flag somewhere indicating
>>>> that downshift occurred. How about the PHY here?
>>> I don't see a register that gives us that status
>>>
>>> I will ask the hardware team if there is one.
>>>
>>> This is a 1Gbps PHY by default so if a slower connection is established
>>> due to faulty cabling or LP advertisement then this would be a down
>>> shift IMO.
>> With your current link_change_notify function it would not be possible
>> to know whether the PHY was connected to a link partner that advertised
>> only 10/100 and so 100 ended up being the link speed, or the link
>> partner was capable of 10/100/1000 and downshift reduced the link speed.
>>
>> If you cannot tell the difference from a register, it might be better to
>> simply omit that function then.
> 
> Yeah I thought it was a bit redundant and wonky to see in the log that the link established to xG/Mbps and then see another message saying the downshift occurred.

I think it's good idea to have this message as just wrong cable might be used.

But this notifier make no sense in it current form - it will produce noise in case of forced 100m/10M.

FYI. PHY sequence to update link:
phy_state_machine()
|-phy_check_link_status()
   |-phy_link_down/up()
     |- .phy_link_change()->phy_link_change()
	|-adjust_link() ----> netdev callback
|-phydev->drv->link_change_notify(phydev);

So, log output has to be done or in .read_status() or
some info has to be saved in .read_status() and then re-used in
.link_change_notify().

-- 
Best regards,
grygorii