Re: [PATCH net-next] net: phy: marvell10g: add downshift tunable support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Sep 17, 2021 at 03:58:01PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 17, 2021 at 04:45:03PM +0200, Andrew Lunn wrote:
> > > +static int mv3310_set_downshift(struct phy_device *phydev, u8 ds)
> > > +{
> > > +	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> > > +	u16 val;
> > > +	int err;
> > > +
> > > +	if (!priv->has_downshift)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (ds == DOWNSHIFT_DEV_DISABLE)
> > > +		return phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, MV_PCS_DSC1,
> > > +					  MV_PCS_DSC1_ENABLE);
> > > +
> > > +	/* FIXME: The default is disabled, so should we disable? */
> > > +	if (ds == DOWNSHIFT_DEV_DEFAULT_COUNT)
> > > +		ds = 2;
> > 
> > Hi Russell
> > 
> > Rather than a FIXME, maybe just document that the hardware default is
> > disabled, which does not make too much sense, so default to 2 attempts?
> 
> Sadly, the downshift parameters aren't documented at all in the kernel,
> and one has to dig into the ethtool source to find out what they mean:
> 
> DOWNSHIFT_DEV_DEFAULT_COUNT -
> 	ethtool --set-phy-tunable ethN downshift on
> DOWNSHIFT_DEV_DISABLE -
> 	ethtool --set-phy-tunable ethN downshift off
> otherwise:
> 	ethtool --set-phy-tunable ethN downshift count N
> 
> This really needs to be documented somewhere in the kernel.

I was hoping that this would cause further discussion on what the
exact meaning of "DOWNSHIFT_DEV_DEFAULT_COUNT" is. Clearly, it's
meant to turn downshift on, but what does "default" actually mean?

If we define "default" as "whatever the hardware defaults to" then
for this phy, that would be turning off downshift.

So, should we rename "DOWNSHIFT_DEV_DEFAULT_COUNT" to be
"DOWNSHIFT_DEV_ENABLE" rather than trying to imply that it's
some kind of default that may need to be made up?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!