Message-ID: <20200218173353.GM25745@shell.armlinux.org.uk>
Date:   Tue, 18 Feb 2020 17:33:53 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Dan Murphy <dmurphy@...com>
Cc:     Grygorii Strashko <grygorii.strashko@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>, andrew@...n.ch,
        davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization
 feature

On Tue, Feb 18, 2020 at 11:12:37AM -0600, Dan Murphy wrote:
> Well now the read_status is becoming a lot more complex.  It would be better
> to remove the ack_interrupt call back and just have read_status call the
> interrupt function regardless of interrupts or not.  Because all the
> interrupt function would be doing is clearing all the interrupts in the ISR
> register on a link up/down event.  And as you pointed out we can check and
> set a flag that indicates if a downshift has happened on link up status and
> clear it on link down. We would need to set the downshift interrupt mask to
> always report that bit.  As opposed to not setting any interrupts if
> interrupts are not enabled.  If the user wants to track WoL interrupt or any
> other feature interrupt we would have to add that flag to the read_status as
> well seems like it could get a bit out of control.

To be honest, I don't like phylib's interrupt implementation; it
imposes a fixed way of dealing with interrupts on phylib drivers,
and it sounds like its ideas don't match what modern PHYs want.

For example, the Marvell 88x3310 can produce interrupts for GPIOs
that are on-board, or temperature alarms, or other stuff... but
phylib's idea is that a PHY interrupt is for signalling that the
link changed somehow, and imposes a did_interrupt(), handle
interrupt, clear_interrupt() structure whether or not it's
suitable for the PHY.  If you don't provide the functions phylib
wants, then phydev->irq gets killed.  Plus, phylib claims the
interrupt for you at certain points depending on whether the
PHY is bound to a network interface or not.

So, my suggestion to move ack_interrupt to did_interrupt will
result in phylib forcing phydev->irq to PHY_POLL, killing any
support for interrupts what so ever...

> Again this is a lot of error prone complex changes and tracking just to
> populate a message in the kernel log.  There is no guarantee that the LP did
> not force the downshift or advertise that it supports 1Gbps.  So what
> condition is really being reported here?  There seems like there are so many
> different scenarios why the PHY could not negotiate to its advertised 1Gbps.

Note that when a PHY wants to downshift, it does that by changing its
advertisement that is sent to the other PHY.

So, if both ends support 1G, 100M and 10M, and the remote end decides
to downshift to 100M, the remote end basically stops advertising 1G
and restarts negotiation afresh with the new advertisement.

In that case, if you look at ethtool's output, it will indicate that
the link partner is not advertising 1G, and the link is operating at
100M.

If 100M doesn't work (and there are cases where connections become
quite flakey) then 100M may also be omitted as well, negotiation
restarted, causing a downshift to 10M.

That's basically how downshift works - a PHY will attempt to
establish link a number of times before deciding to restart
negotiation with some of the advertisement omitted, and the
reduced negotiation advertisement appears in the remote PHY's
link partner registers.

As I mentioned, the PHY on either end of the link can be the one
which decides to downshift, and the partner PHY has no idea that
a downshift has happened.

-- 
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

Powered by blists - more mailing lists