Re: [PATCH] net: phy: marvell: Add errata section 5.1 for Alaska PHY

["Marek Behún" <kabel@kernel.org>]

On Thu, 17 Mar 2022 07:43:52 +0100
Stefan Roese <sr@denx.de> wrote:

> On 3/15/22 17:09, Marek Behún wrote:
> > On Tue, 15 Mar 2022 08:48:27 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> From: Leszek Polak <lpolak@arri.de>
> >>
> >> As per Errata Section 5.1, if EEE is intended to be used, some register
> >> writes must be done once after every hardware reset. This patch now adds
> >> the necessary register writes as listed in the Marvell errata.
> >>
> >> Without this fix we experience ethernet problems on some of our boards
> >> equipped with a new version of this ethernet PHY (different supplier).
> >>
> >> The fix applies to Marvell Alaska 88E1510/88E1518/88E1512/88E1514
> >> Rev. A0.
> >>
> >> Signed-off-by: Leszek Polak <lpolak@arri.de>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> Cc: Andrew Lunn <andrew@lunn.ch>
> >> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> >> Cc: Russell King <linux@armlinux.org.uk>
> >> Cc: David S. Miller <davem@davemloft.net>
> >> ---
> >>   drivers/net/phy/marvell.c | 42 +++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> >> index 2429db614b59..0f4a3ab4a415 100644
> >> --- a/drivers/net/phy/marvell.c
> >> +++ b/drivers/net/phy/marvell.c
> >> @@ -1179,6 +1179,48 @@ static int m88e1510_config_init(struct phy_device *phydev)
> >>   {
> >>   	int err;
> >>   
> >> +	/* As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/
> >> +	 * 88E1514 Rev A0, Errata Section 5.1:
> >> +	 * If EEE is intended to be used, the following register writes
> >> +	 * must be done once after every hardware reset.
> >> +	 */
> >> +	err = marvell_set_page(phydev, 0x00FF);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 17, 0x214B);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 16, 0x2144);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 17, 0x0C28);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 16, 0x2146);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 17, 0xB233);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 16, 0x214D);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 17, 0xCC0C);
> >> +	if (err < 0)
> >> +		return err;
> >> +	err = phy_write(phydev, 16, 0x2159);
> >> +	if (err < 0)
> >> +		return err;  
> > 
> > Can you please create a static const struct and then do this in a for
> > cycle? Somethign like this
> > 
> > static const struct { u16 reg17, reg16; } errata_vals = {
> >    { 0x214B, 0x2144 }, ...
> > };
> > 
> > for (i = 0; i < ARRAY_SIZE(errata_vals); ++i) {
> >    err = phy_write(phydev, 17, errata_vals[i].reg17);
> >    if (err)
> >      return err;
> >    err = phy_write(phydev, 16, errata_vals[i].reg16);
> >    if (err)
> >      return err;
> > }  
> 
> I could do this, sure. But frankly I'm not so sure, if this really
> improves the code much. This list will most likely never be extended.
> And in this current version, it's easier to compare the values written
> with the one described in the errata.
> 
> But again, I have no hard feeling here. If it's the general option to
> move to the "loop version", then I'll gladly send an updated patch
> version.

I always try to do something like that. Look for example at

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/serdes.c?h=v5.17-rc8#n1417

Marek