[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d4484799-a7f9-5edc-2242-78b41d6840a6@gmail.com>
Date: Wed, 18 Dec 2019 16:57:52 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 13/14] net: phy: add Broadcom BCM84881 PHY
driver
On 12/18/19 4:55 PM, Russell King - ARM Linux admin wrote:
> 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@...linux.org.uk>
>>>>
>>>> Reviewed-by: Florian Fainelli <f.fainelli@...il.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?
Doh, yes that should be fixed, and that does make sense.
--
Florian
Powered by blists - more mailing lists