Re: [PATCH] phy: maxlinear: add Maxlinear GPY115/21x/24x driver

[Liang Xu <lxu@maxlinear.com>]

On 2/6/2021 12:55 am, Andrew Lunn wrote:
> This email was sent from outside of MaxLinear.
>
>
>>>> +     linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>>>> +                      phydev->supported,
>>>> +                      ret & MDIO_PMA_NG_EXTABLE_5GBT);
>>>> +
>>> Does genphy_c45_pma_read_abilities() do the wrong thing here? What
>>> does it get wrong?
>> The problem comes from condition "phydev->c45_ids.mmds_present &
>> MDIO_DEVS_AN".
>>
>> Our product supports both C22 and C45.
>>
>> In the real system, we found C22 was used by customers (with indirect
>> access to C45 registers when necessary).
>>
>> Then during probe, in API "get_phy_device", it skips reading C45 IDs.
>>
>> So that genphy_c45_pma_read_abilities skip the supported flag
>> ETHTOOL_LINK_MODE_Autoneg_BIT.
> This sounds like a generic problem, which will affect any PHY which
> has both C22 and C45. I wounder if it makes sense to add a helper
> function which a PHY driver can call to get the
> phydev->c45_ids.mmds_present populated?

I thought to use get_phy_c45_ids in gpy_config_init to populate the 
c45_ids, but this is a static function inside phy_device.c.

Or maybe in genphy_c45_pma_read_abilities, it reads MII_BMSR register if 
is_c45 not set.

>>>> +static int gpy_read_status(struct phy_device *phydev)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     ret = genphy_update_link(phydev);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     phydev->speed = SPEED_UNKNOWN;
>>>> +     phydev->duplex = DUPLEX_UNKNOWN;
>>>> +     phydev->pause = 0;
>>>> +     phydev->asym_pause = 0;
>>>> +
>>>> +     if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>>>> +             ret = genphy_c45_read_lpa(phydev);
>>>> +             if (ret < 0)
>>>> +                     return ret;
>>>> +
>>>> +             /* Read the link partner's 1G advertisement */
>>>> +             ret = phy_read(phydev, MII_STAT1000);
>>>> +             if (ret < 0)
>>>> +                     return ret;
>>>> +             mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
>>> can genphy_read_lpa() be used here?
>> 2.5G is not covered in genphy_read_lpa.
>>
>> If I use genphy_c45_read_lpa first then genphy_read_lpa after, it seems
>> a bit redundant.
> I'm just trying to avoid repeating code which is in helpers. I think
> this is the first PHY driver which uses a mixture of C22 and C45 like
> this. So it could be the helpers need small modifications to make them
> work. We should make those modifications, since your PHY is not likely
> to be the only mixed C22 and C45 device.

I agree, this is issue for mixed C22/45 device.

I saw something similar in BRCM driver (bcm84881_read_status) and 
Marvell driver (mv3310_read_status_copper).

They are C45 device and use vendor specific register in MMD to access 
C22 equivalent registers.

>
>     Andrew
>