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

[Florian Fainelli <f.fainelli@gmail.com>]

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

Doh, yes that should be fixed, and that does make sense.
-- 
Florian