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

[Dan Murphy]

Grygorii

On 2/14/20 12:31 PM, Dan Murphy wrote:
> 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.
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.

Dan