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

From: Dan Murphy
Date: Fri Feb 14 2020 - 13:36:46 EST

Grygorii

On 2/14/20 12:32 PM, Grygorii Strashko wrote:


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

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().

OK I will try to find a way to give some sort of message.

Also we did get confirmation from HW guys and you also confirmed that the number of attempts for downshift is configurable. So I will be adding back the tunable code once net-next opens.

Dan