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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Feb 18, 2020 at 10:36:47AM -0600, Dan Murphy wrote:
> Russell
> 
> On 2/18/20 10:25 AM, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 14, 2020 at 12:31:52PM -0600, Dan Murphy wrote:
> > > Grygorii
> > > 
> > > On 2/14/20 12:32 PM, Grygorii Strashko wrote:
> > > > 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.
> > How do you know the speed that the PHY downshifted to?
> 
> The DP83867 has a register PHYSTS where BIT 15:14 indicate the speed that
> the PHY negotiated.
> 
> In the same register BIT 13 indicates the duplex mode.
> 
> > If the speed and duplex are available in some PHY specific status
> > register, then one way you can detect downshift is to decode the
> > negotiated speed/duplex from the advertisements (specifically the LPA
> > read from the registers and the advertisement that we should be
> > advertising - some PHYs modify their registers when downshifting) and
> > check whether it matches the negotiated parameters in the PHY
> > specific status register.
> > 
> > Alternatively, if the PHY modifies the advertisement register on
> > downshift, comparing the advertisement register with what it should
> > be will tell you if downshift has occurred.
> 
> The ISR register BIT 5 indicates if a downshift occurred or not. So we can
> indicate that the PHY downshifted but there is no cause in the registers bit
> field.  My concern for this bit though is the register is clear on read so
> all other interrupts are lost if we only read to check downshift.  And the
> link_change_notifier is called before the interrupt ACK call back.  We could
> call the interrupt function and get the downshift status but again it will
> clear the interrupt register and any other statuses may be lost.

What's wrong with having an ack_interrupt() method that reads the
PHY ISR register, and records in a driver private flag that bit 5
has been set?  The read_status() method can clear the flag if link
goes down, or check the flag if link is up and report that a
downshift event occurred.

If IRQs are not in use, then read_status() would have to read the
ISR itself.

It may be better to move ack_interrupt() to did_interrupt(), which
will ensure that it gets executed before the PHY state machine is
triggered by phy_interrupt().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up