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

[Dan Murphy <dmurphy@ti.com>]

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.

Dan