Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

On Wed, Aug 30, 2023 at 12:52:24PM +0200, Lukasz Majewski wrote:
> Hi Oleksij,
> 
> > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote:
> > > The KSZ9477 errata points out (in 'Module 4') the link up/down
> > > problem when EEE (Energy Efficient Ethernet) is enabled in the
> > > device to which the KSZ9477 tries to auto negotiate.
> > > 
> > > The suggested workaround is to clear advertisement of EEE for PHYs
> > > in this chip driver.
> > > 
> > > Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> > > ---
> > >  drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++-
> > >  1 file changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > > index 87b090ad2874..469dcd8a5711 100644
> > > --- a/drivers/net/phy/micrel.c
> > > +++ b/drivers/net/phy/micrel.c
> > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct
> > > phy_device *phydev) return 0;
> > >  }
> > >  
> > > +static int ksz9477_get_features(struct phy_device *phydev)
> > > +{
> > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, };
> > > +	int ret;
> > > +
> > > +	ret = genphy_read_abilities(phydev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* KSZ9477 Errata DS80000754C
> > > +	 *
> > > +	 * Module 4: Energy Efficient Ethernet (EEE) feature
> > > select must be
> > > +	 * manually disabled
> > > +	 *   The EEE feature is enabled by default, but it is not
> > > fully
> > > +	 *   operational. It must be manually disabled through
> > > register
> > > +	 *   controls. If not disabled, the PHY ports can
> > > auto-negotiate
> > > +	 *   to enable EEE, and this feature can cause link drops
> > > when linked
> > > +	 *   to another device supporting EEE.
> > > +	 *
> > > +	 *   Although, the KSZ9477 MMD register
> > > +	 *   (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that
> > > EEE is
> > > +	 *   operational one needs to manualy clear them to follow
> > > the chip
> > > +	 *   errata.
> > > +	 */
> > > +	linkmode_and(phydev->supported_eee, phydev->supported,
> > > zero); +
> > > +	return 0;
> > > +}
> > > +
> > >  #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
> > >  #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
> > >  #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
> > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = {
> > >  	.handle_interrupt = kszphy_handle_interrupt,
> > >  	.suspend	= genphy_suspend,
> > >  	.resume		= genphy_resume,
> > > -	.get_features	= ksz9131_get_features,
> > > +	.get_features	= ksz9477_get_features,  
> > 
> > Sorry, i didn't described all details how to implement it.
> > 
> > This code will break EEE support for the KSZ8563R switch.
> > 
> 
> And then another switch (KSZ8563R) pops up.... with regression....

Initially ksz9477_get_features() was introduced to fix KSZ8563R.

> In the micrel.c the ksz9477_get_features was only present in:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4832
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4874
> so I only changed it there.
> 
> Apparently the KSZ8563R re-uses the KSZ9477 code.

Most (all?) switch variant support by the ksz9477 DSA driver share the same
PHYid, so it is not possible to identify it by the micrel.c PHY driver.
As far as a know, the only commonly accepted way to notify about some quirks
between this both drivers is not user dev_flags.

Regards,
Oleskij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |