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

[Lukasz Majewski]

Hi Oleksij,

> 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.
> 

We would need another idea on fixing this problem as there is following
order of function calls:

-> ksz9477_setup
-> ksz9477_get_features  (here we are supposed to use the MICREL_NO_EEE
flag)
-> ksz_get_phy_flags  (here we are supposed to set the MICREL_NO_EEE
flag)

The ksz9477_get_features is called early. 



It looks like the most optimal solution would be the one proposed by
Tristam:
https://www.spinics.net/lists/netdev/msg932044.html

It would clear the 7.60 per-port register with two ksz_write16
functions.

That exactly follows the recommendation for KSZ9477 Errata Module4.

> Regards,
> Oleskij




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgpMSwCExnrAb.pgp
Description: OpenPGP digital signature