[net-next,4/7] net: phy: marvell10g: Add support for 2.5GBASET and 5GBASET

The Marvell Alaska family of PHYs supports 2.5GBaseT and 5GBaseT modes,
as defined in the 802.3bz specification.

When the link partner requests a 2.5GBASET link, the PHY will
reconfigure it's MII interface to 2500BASEX.

At 5G, the PHY will reconfigure it's interface to 5GBASE-R, but this
mode isn't supported by any MAC for now.

This was tested with :
 - The 88X3310, which is on the MacchiatoBin
 - The 88E2010, an Alaska PHY that has no fiber interfaces, and is
   limited to 5G maximum speed.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

On Fri, Jan 18, 2019 at 04:23:49PM +0100, Maxime Chevallier wrote:
> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	linkmode_and(phydev->advertising, phydev->advertising,
> -		     phydev->supported);
> +	/* Make sure we advertise all the supported modes, and not just the
> +	 * default one specified in the driver's .features.
> +	 */
> +	linkmode_copy(phydev->advertising, phydev->supported);

This looks wrong to me, it tramples over the work that phy_probe()
did, specifically with regard to the pause mode bits.  Please see
the comments in that function about the pause mode bits.

> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	linkmode_and(phydev->advertising, phydev->advertising,
> -		     phydev->supported);
> +	/* Make sure we advertise all the supported modes, and not just the
> +	 * default one specified in the driver's .features.
> +	 */
> +	linkmode_copy(phydev->advertising, phydev->supported);

Hi Maxime

What Russell is referring to is:

static int phy_probe(struct device *dev)
{
..
        /* Start out supporting everything. Eventually,
         * a controller will attach, and may modify one
         * or both of these values
         */
        linkmode_copy(phydev->supported, phydrv->features);
        of_set_phy_supported(phydev);
        linkmode_copy(phydev->advertising, phydev->supported);

        /* Get the EEE modes we want to prohibit. We will ask
         * the PHY stop advertising these mode later on
         */
        of_set_phy_eee_broken(phydev);

        /* The Pause Frame bits indicate that the PHY can support passing
         * pause frames. During autonegotiation, the PHYs will determine if
         * they should allow pause frames to pass.  The MAC driver should then
         * use that result to determine whether to enable flow control via
         * pause frames.
         *
         * Normally, PHY drivers should not set the Pause bits, and instead
         * allow phylib to do that.  However, there may be some situations
         * (e.g. hardware erratum) where the driver wants to set only one
         * of these bits.
         */
        if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) ||
            test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) {
                linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                   phydev->supported);
                linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                  phydev->supported);
                if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features))
                        linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                         phydev->supported);
                if (test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                             phydrv->features))
                        linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                         phydev->supported);
        } else {
                linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
                                 phydev->supported);
                linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
                                 phydev->supported);
        }

So by doing a copy of supported into advertising, you can stomping
over any restrictions applied via of_set_phy_supported(),
of_set_phy_eee_broken(phydev), and any pause control settings which
might of happened.

What might make sense here is that a PHY driver can replace its
.features member at run time, in its config_init() call. The core then
needs to perform these evaluations. So i'm guessing we need to split
this code out of probe() and move it into phy_init_hw()?

Heiner, you know this code better than anybody. What do you think?

	Andrew

Hello Andrew, Russell,

On Mon, 21 Jan 2019 21:17:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

>> @@ -264,8 +265,10 @@ static int mv3310_config_init(struct phy_device *phydev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	linkmode_and(phydev->advertising, phydev->advertising,
>> -		     phydev->supported);
>> +	/* Make sure we advertise all the supported modes, and not just the
>> +	 * default one specified in the driver's .features.
>> +	 */
>> +	linkmode_copy(phydev->advertising, phydev->supported);  
>
>So by doing a copy of supported into advertising, you can stomping
>over any restrictions applied via of_set_phy_supported(),
>of_set_phy_eee_broken(phydev), and any pause control settings which
>might of happened.

Thanks for the explanations, this is indeed clearly not a good solution.

>What might make sense here is that a PHY driver can replace its
>.features member at run time, in its config_init() call. The core then
>needs to perform these evaluations. So i'm guessing we need to split
>this code out of probe() and move it into phy_init_hw()?

So the .features won't be read-only anymore ? We could also simply make
a helper that would add a mode to both the supported and advertising
modes list, that would be used in the 'genphy_c45_pma_read_abilities'
and config_init ?

I lack the big picture of the PHY init sequence, there seems to be a
lot of quirks and complex cases that we need to take into account, so
I'll let you decide :)

>Heiner, you know this code better than anybody. What do you think?
>
>	Andrew

Thanks for the feedback,

Maxime