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

[Florian Fainelli]

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@xxxxxx>
>>> ---
>>> 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.
-- 
Florian