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

[Dan Murphy]

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.

Dan