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

From: Dan Murphy
Date: Tue Feb 18 2020 - 09:12:23 EST


On 2/14/20 12:31 PM, Dan Murphy wrote:

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

On 06/02/2020 00:01, Dan Murphy wrote:

On 2/5/20 4:00 PM, Florian Fainelli wrote:
On 2/5/20 1:51 PM, Dan Murphy wrote:

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

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

Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
v2 - Updated read status to call genphy_read_status first, added
callback to notify of speed change and use phy_set_bits -

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
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
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
term is "downshift". I'm fine with using "speed optimization" in
to be in line with the datasheet. Just a comment in the code would be
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)
ÂÂ +/* PHY STS bits */
+#define DP83867_PHYSTS_1000ÂÂÂÂÂÂÂÂÂÂÂ BIT(15)
+#define DP83867_PHYSTS_100ÂÂÂÂÂÂÂÂÂÂÂ BIT(14)
+/* CFG2 bits */
ÂÂ /* CFG3 bits */
@@ -287,6 +297,43 @@ static int dp83867_config_intr(struct phy_device
ÂÂÂÂÂÂ 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
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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_link_change()->phy_link_change()
ÂÂÂÂ|-adjust_link() ----> netdev callback

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

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.

I worked on this a bit and I think the notification is a bit complicated to get into the code just to print a message. First the notification comes from the interrupt register which is COR. So if I read the interrupt register in read_status then the ack_interrupt call back won't do anything and status will be lost so if we need to implement other features that depend on the interrupt status that status is cleared. In addition the downshift interrupt will be read and cleared so the state of any downshift is lost after the message. The link_change_notifier is called first then the ack_interrupt function is called so as I stated the downshift status will be reset to no downshift as the bit is cleared. So I don't think adding this notifier is worth the complex code to print a message.