Re: [PATCH net-next v2 13/14] net: phy: add Broadcom BCM84881 PHY driver

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

On Tue, Dec 10, 2019 at 05:58:37PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Dec 10, 2019 at 09:34:16AM -0800, Florian Fainelli wrote:
> > On 12/9/19 7:19 AM, Russell King wrote:
> > > Add a rudimentary Clause 45 driver for the BCM84881 PHY, found on
> > > Methode DM7052 SFPs.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > > ---
> > >  drivers/net/phy/Kconfig    |   6 +
> > >  drivers/net/phy/Makefile   |   1 +
> > >  drivers/net/phy/bcm84881.c | 269 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 276 insertions(+)
> > >  create mode 100644 drivers/net/phy/bcm84881.c
> > > 
> > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > > index fe602648b99f..41272106dea9 100644
> > > --- a/drivers/net/phy/Kconfig
> > > +++ b/drivers/net/phy/Kconfig
> > > @@ -329,6 +329,12 @@ config BROADCOM_PHY
> > >  	  Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464,
> > >  	  BCM5481, BCM54810 and BCM5482 PHYs.
> > >  
> > > +config BCM84881_PHY
> > > +	bool "Broadcom BCM84881 PHY"
> > > +	depends on PHYLIB=y
> > > +	---help---
> > > +	  Support the Broadcom BCM84881 PHY.
> > 
> > Cannot we make this tristate, I believe we cannot until there are more
> > fundamental issues (that you just reported) to be fixed, correct?
> 
> Indeed.  The problem I saw was that although the bcm84881 has the
> PHY correctly described, for whatever reason, the module was not
> loaded.
> 
> What I think is going in is that with modern udev userspace,
> request_module() is not functional, and we do not publish the
> module IDs for Clause 45 PHYs via uevent.  Consequently, there
> exists no mechanism to load a Clause 45 PHY driver from the
> filesystem.

I just attempted booting with sfp as a module, bcm84881 as a module.
sfp has to be loaded for the SFP cage to be recognised, so module
loading is availble prior to the PHY being known to the kernel.

The SFP is probed, and the PHY identified (via my debug):

[    7.209549] sfp sfp: phy PMA devid: 0xae02 0x5151

The PHY is not bound to its driver at this point.

We then try to connect to the PHY, but the support mask is zero,
so we know nothing about what modes this PHY supports:

[    7.215985] mvneta f1034000.ethernet eno2: phylink_sfp_connect_phy: s=00,00000000,00000000 a=00,00000000,00000000
[    7.215997] mvneta f1034000.ethernet eno2: validation with support 00,00000000,00000000 failed: -22
[    7.226343] sfp sfp: sfp_add_phy failed: -22

and we fail - because we are unable to identify what mode we should
configure the MAC side for, because we have no idea what the
capabilities of the PHY are at this stage.

We can't wait until we've called phylink_attach_phy(), because that
configures the PHY for the phy interface mode that was passed in.

There is no sign of the bcm84881 module being loaded.

This is the opposite problem to the one I recently posted about
regarding the 88e1111 issue, as here we rely on knowing something
about the PHY capabilities.

I think this is showing that phylink has a very difficult job trying
to match the capabilities of the SFP module with the capabilities of
the host MAC - we have no real idea what phy interface modes the
host MAC supports, or how the PHY is going to behave (which of the
many modes the PHY is going to choose for particular speeds.)
Plus the problem that the phylib support mask no longer reflects
the PHYs abilities after phy_attach_direct() has been called.

I thought I had something sorted, but only if the PHY driver is
built-in.

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