From: Russell King - ARM Linux admin <linux@armlinux•org.uk>
To: Florian Fainelli <f.fainelli@gmail•com>
Cc: Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>,
"David S. Miller" <davem@davemloft•net>,
netdev@vger•kernel.org
Subject: Re: [PATCH net-next v2 13/14] net: phy: add Broadcom BCM84881 PHY driver
Date: Thu, 19 Dec 2019 00:55:45 +0000 [thread overview]
Message-ID: <20191219005545.GY1344@shell.armlinux.org.uk> (raw)
In-Reply-To: <20191210184640.GU1344@shell.armlinux.org.uk>
On Tue, Dec 10, 2019 at 06:46:40PM +0000, Russell King - ARM Linux admin wrote:
> 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.
Okay, I see what is going on - I just added debug into __request_module,
and got:
[ 234.729163] __request_module: mdio:-10101110000000100101000101010001
[ 234.732561] __request_module: mdio:-10101110000000100101000101010001
[ 234.735729] __request_module: mdio:00000011011000100000000000000000
on inserting this SFP. This comes from this:
#define MDIO_ID_FMT "%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d"
#define MDIO_ID_ARGS(_id) \
(_id)>>31, ((_id)>>30) & 1, ((_id)>>29) & 1, ((_id)>>28) & 1, \
((_id)>>27) & 1, ((_id)>>26) & 1, ((_id)>>25) & 1, ((_id)>>24) & 1, \
((_id)>>23) & 1, ((_id)>>22) & 1, ((_id)>>21) & 1, ((_id)>>20) & 1, \
((_id)>>19) & 1, ((_id)>>18) & 1, ((_id)>>17) & 1, ((_id)>>16) & 1, \
((_id)>>15) & 1, ((_id)>>14) & 1, ((_id)>>13) & 1, ((_id)>>12) & 1, \
((_id)>>11) & 1, ((_id)>>10) & 1, ((_id)>>9) & 1, ((_id)>>8) & 1, \
((_id)>>7) & 1, ((_id)>>6) & 1, ((_id)>>5) & 1, ((_id)>>4) & 1, \
((_id)>>3) & 1, ((_id)>>2) & 1, ((_id)>>1) & 1, (_id) & 1
coupled with:
static int phy_request_driver_module(struct phy_device *dev, int phy_id)
{
...
ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
MDIO_ID_ARGS(phy_id));
The signed-ness of the parameter passed into MDIO_ID_ARGS() matters.
Hence, (0xae025151)>>31 becomes -1, and %d prints it as -1.
phy_id should be u32, just like it is everywhere else in phylib. Also,
MDIO_ID_ARGS() should probably be adapted to not care about the signed-
ness of its argument.
Thoughts?
--
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
next prev parent reply other threads:[~2019-12-19 0:55 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-09 15:15 [PATCH net-next v2 00/14] Add support for SFP+ copper modules Russell King - ARM Linux admin
2019-12-09 15:18 ` [PATCH net-next v2 01/14] net: sfp: remove incomplete 100BASE-FX and 100BASE-LX support Russell King
2019-12-10 16:52 ` Andrew Lunn
2019-12-09 15:18 ` [PATCH net-next v2 02/14] net: sfp: derive interface mode from ethtool link modes Russell King
2019-12-10 16:53 ` Andrew Lunn
2019-12-09 15:18 ` [PATCH net-next v2 03/14] net: sfp: add more extended compliance codes Russell King
2019-12-10 16:57 ` Andrew Lunn
2019-12-10 17:21 ` Russell King - ARM Linux admin
2019-12-09 15:18 ` [PATCH net-next v2 04/14] net: sfp: add module start/stop upstream notifications Russell King
2019-12-10 16:59 ` Andrew Lunn
2019-12-09 15:18 ` [PATCH net-next v2 05/14] net: sfp: move phy_start()/phy_stop() to phylink Russell King
2019-12-10 17:00 ` Andrew Lunn
2019-12-09 15:18 ` [PATCH net-next v2 06/14] net: mdio-i2c: add support for Clause 45 accesses Russell King
2019-12-10 17:05 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 07/14] net: phylink: re-split __phylink_connect_phy() Russell King
2019-12-10 17:06 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 08/14] net: phylink: support Clause 45 PHYs on SFP+ modules Russell King
2019-12-10 17:07 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 09/14] net: phylink: split link_an_mode configured and current settings Russell King
2019-12-10 17:09 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 10/14] net: phylink: split phylink_sfp_module_insert() Russell King
2019-12-10 17:10 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 11/14] net: phylink: delay MAC configuration for copper SFP modules Russell King
2019-12-10 17:14 ` Andrew Lunn
2019-12-09 15:19 ` [PATCH net-next v2 12/14] net: phylink: make Broadcom BCM84881 based SFPs work Russell King
2019-12-10 17:32 ` Florian Fainelli
2019-12-09 15:19 ` [PATCH net-next v2 13/14] net: phy: add Broadcom BCM84881 PHY driver Russell King
2019-12-10 17:34 ` Florian Fainelli
2019-12-10 17:58 ` Russell King - ARM Linux admin
2019-12-10 18:46 ` Russell King - ARM Linux admin
2019-12-19 0:55 ` Russell King - ARM Linux admin [this message]
2019-12-19 0:57 ` Florian Fainelli
2019-12-09 15:19 ` [PATCH net-next v2 14/14] net: sfp: add support for Clause 45 PHYs Russell King
2019-12-10 17:21 ` Andrew Lunn
2019-12-11 1:23 ` [PATCH net-next v2 00/14] Add support for SFP+ copper modules David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191219005545.GY1344@shell.armlinux.org.uk \
--to=linux@armlinux$(echo .)org.uk \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=f.fainelli@gmail$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox