Messages in this thread | | | Date | Tue, 18 Feb 2020 16:49:28 +0000 | From | Russell King - ARM Linux admin <> | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature |
| |
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
| |