[PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

Fixes XAUI/RXAUI lane alignment errors.
Issue causes dropped packets when trying to communicate over
fiber via SERDES lanes of port 9 and 10.
Errata document applies only to 88E6190X and 88E6390X devices.
Requires poking in undocumented registers.

Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
---
V2 : Rework as suggested by Andrew Lunn <andrew@lun.ch> 
 * make int lanes[] const 
 * reorder prod_nums
 * update commit message to indicate we are dealing with
   undocumented Marvell registers and magic values
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 80167d53212f..b36049595c6b 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -829,6 +829,37 @@ static int mv88e6390_serdes_enable_checker(struct mv88e6xxx_chip *chip, int lane
 				      MV88E6390_PG_CONTROL, reg);
 }
 
+static int mv88e6390x_serdes_erratum_3_14(struct mv88e6xxx_chip *chip)
+{
+	const int lanes[] = { MV88E6390_PORT9_LANE0, MV88E6390_PORT9_LANE1,
+		MV88E6390_PORT9_LANE2, MV88E6390_PORT9_LANE3,
+		MV88E6390_PORT10_LANE0, MV88E6390_PORT10_LANE1,
+		MV88E6390_PORT10_LANE2, MV88E6390_PORT10_LANE3 };
+	int err, i;
+
+	/* 88e6190x and 88e6390x errata 3.14:
+	 * After chip reset, SERDES reconfiguration or SERDES core
+	 * Software Reset, the SERDES lanes may not be properly aligned
+	 * resulting in CRC errors
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(lanes); i++) {
+		err = mv88e6390_serdes_write(chip, lanes[i],
+					     MDIO_MMD_PHYXS,
+					     0xf054, 0x400C);
+		if (err)
+			return err;
+
+		err = mv88e6390_serdes_write(chip, lanes[i],
+					     MDIO_MMD_PHYXS,
+					     0xf054, 0x4000);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 			   bool up)
 {
@@ -853,6 +884,12 @@ int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
 	if (!err && up)
 		err = mv88e6390_serdes_enable_checker(chip, lane);
 
+	if (!err && up) {
+		if (chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6190X ||
+		    chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6390X)
+			err = mv88e6390x_serdes_erratum_3_14(chip);
+	}
+
 	return err;
 }
 
-- 
2.11.0

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Andrew Lunn]

On Fri, Jul 14, 2023 at 06:06:12PM +0200, Ante Knezic wrote:
> Fixes XAUI/RXAUI lane alignment errors.
> Issue causes dropped packets when trying to communicate over
> fiber via SERDES lanes of port 9 and 10.
> Errata document applies only to 88E6190X and 88E6390X devices.
> Requires poking in undocumented registers.
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Paolo Abeni]

On Fri, 2023-07-14 at 18:06 +0200, Ante Knezic wrote:
> Fixes XAUI/RXAUI lane alignment errors.
> Issue causes dropped packets when trying to communicate over
> fiber via SERDES lanes of port 9 and 10.
> Errata document applies only to 88E6190X and 88E6390X devices.
> Requires poking in undocumented registers.
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>

It does not apply cleanly to net-next. Please respin. You can retain
Andrew's Reviewed-by tag.

Thanks!

Paolo

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

> It does not apply cleanly to net-next. Please respin. You can retain
> Andrew's Reviewed-by tag.

Respin might need a complete rework of the patch as with the
conversion of 88e639x to phylink_pcs (introduced with commit
e5b732a275f5fae0f1342fb8cf76de654cd51e50) the original code flow
has completely changed so it will not be as simple as finding a new
place to stick the patch. 
The new phylink mostly hides away mv88e6xxx_chip struct which is needed 
to identify the correct device and writing to relevant registers has also
changed in favor of mv88e639x_pcs struct etc.
I think you can see where I am going with this. In this sense I am not sure 
about keeping the Reviewed-by tag, more likely a complete rewrite 
should be done.
I will repost V3 once I figure out how to make it work with the new
framework.

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Vladimir Oltean]

On Tue, Jul 18, 2023 at 04:43:10PM +0200, Ante Knezic wrote:
> > It does not apply cleanly to net-next. Please respin. You can retain
> > Andrew's Reviewed-by tag.
> 
> Respin might need a complete rework of the patch as with the
> conversion of 88e639x to phylink_pcs (introduced with commit
> e5b732a275f5fae0f1342fb8cf76de654cd51e50) the original code flow
> has completely changed so it will not be as simple as finding a new
> place to stick the patch. 
> The new phylink mostly hides away mv88e6xxx_chip struct which is needed 
> to identify the correct device and writing to relevant registers has also
> changed in favor of mv88e639x_pcs struct etc.
> I think you can see where I am going with this. In this sense I am not sure 
> about keeping the Reviewed-by tag, more likely a complete rewrite 
> should be done.
> I will repost V3 once I figure out how to make it work with the new
> framework.
> 

Can't you simply replicate the positioning of mv88e6393x_erratum_4_6()
from mv88e6393x_pcs_init()?

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

> > > It does not apply cleanly to net-next. Please respin. You can retain
> > > Andrew's Reviewed-by tag.
> > 
> > Respin might need a complete rework of the patch as with the
> > conversion of 88e639x to phylink_pcs (introduced with commit
> > e5b732a275f5fae0f1342fb8cf76de654cd51e50) the original code flow
> > has completely changed so it will not be as simple as finding a new
> > place to stick the patch. 
> > The new phylink mostly hides away mv88e6xxx_chip struct which is needed 
> > to identify the correct device and writing to relevant registers has also
> > changed in favor of mv88e639x_pcs struct etc.
> > I think you can see where I am going with this. In this sense I am not sure 
> > about keeping the Reviewed-by tag, more likely a complete rewrite 
> > should be done.
> > I will repost V3 once I figure out how to make it work with the new
> > framework.
> > 
> 
> Can't you simply replicate the positioning of mv88e6393x_erratum_4_6()
> from mv88e6393x_pcs_init()?

I don't think so. The erratum from the patch needs to be applied on each
SERDES reconfiguration or reset. For example, when replugging different 
SFPs (sgmii - 10g - sgmii interface). Erratum 4_6 is done only once? 
My guess is to put it in mv88e639x_sgmii_pcs_post_config but still I 
need the device product number - maybe embedding a pointer to the 
mv88e6xxx_chip chip inside the mv88e639x_pcs struct would be the cleanest way.

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Vladimir Oltean]

On Tue, Jul 18, 2023 at 05:25:12PM +0200, Ante Knezic wrote:
> > > > It does not apply cleanly to net-next. Please respin. You can retain
> > > > Andrew's Reviewed-by tag.
> > > 
> > > Respin might need a complete rework of the patch as with the
> > > conversion of 88e639x to phylink_pcs (introduced with commit
> > > e5b732a275f5fae0f1342fb8cf76de654cd51e50) the original code flow
> > > has completely changed so it will not be as simple as finding a new
> > > place to stick the patch. 
> > > The new phylink mostly hides away mv88e6xxx_chip struct which is needed 
> > > to identify the correct device and writing to relevant registers has also
> > > changed in favor of mv88e639x_pcs struct etc.
> > > I think you can see where I am going with this. In this sense I am not sure 
> > > about keeping the Reviewed-by tag, more likely a complete rewrite 
> > > should be done.
> > > I will repost V3 once I figure out how to make it work with the new
> > > framework.
> > > 
> > 
> > Can't you simply replicate the positioning of mv88e6393x_erratum_4_6()
> > from mv88e6393x_pcs_init()?
> 
> I don't think so. The erratum from the patch needs to be applied on each
> SERDES reconfiguration or reset. For example, when replugging different 
> SFPs (sgmii - 10g - sgmii interface). Erratum 4_6 is done only once? 
> My guess is to put it in mv88e639x_sgmii_pcs_post_config but still I 
> need the device product number - maybe embedding a pointer to the 
> mv88e6xxx_chip chip inside the mv88e639x_pcs struct would be the cleanest way.
> 
> 

It needs to be implemented exactly as posted here? After mv88e6390_serdes_power()
is called on any port/lane, mv88e6390x_serdes_erratum_3_14() needs to run
for all lanes? That might be a problem.

Do we know if the register writes are disruptive for the ports which are
already up and running?

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Andrew Lunn]

> I don't think so. The erratum from the patch needs to be applied on each
> SERDES reconfiguration or reset. For example, when replugging different 
> SFPs (sgmii - 10g - sgmii interface). Erratum 4_6 is done only once? 
> My guess is to put it in mv88e639x_sgmii_pcs_post_config but still I 
> need the device product number

You might be able to read the product number from the ID registers of
the SERDES, registers 2 and 3 ? That is kind of cleaner. It is the
SERDES which needs the workaround, so look at the SERDES ID ...

- maybe embedding a pointer to the 
> mv88e6xxx_chip chip inside the mv88e639x_pcs struct would be the cleanest way.
 
That would also work.

     Andrew

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

> It needs to be implemented exactly as posted here? After mv88e6390_serdes_power()
> is called on any port/lane, mv88e6390x_serdes_erratum_3_14() needs to run
> for all lanes? That might be a problem.

Actually, I tested applying erratum only on requested lane in pcs_post_config and
it seems to work out fine, so we might use something like:

static int mv88e6390_erratum_3_14(struct mv88e639x_pcs *mpcs)
{
	int err;

	/* 88e6190x and 88e6390x errata 3.14:
	 * After chip reset, SERDES reconfiguration or SERDES core
	 * Software Reset, the SERDES lanes may not be properly aligned
	 * resulting in CRC errors
	 */

	err = mdiodev_c45_write(&mpcs->mdio, MDIO_MMD_PHYXS,
					 0xf054, 0x400C);
	if (err)
	        return err;

	err = mdiodev_c45_write(&mpcs->mdio, MDIO_MMD_PHYXS,
				 0xf054, 0x4000);
	if (err)
	        return err;

	return 0;
}

> Do we know if the register writes are disruptive for the ports which are
> already up and running?

I was not able to see any issues when appling the errata for already active
and running ports.

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

> > I don't think so. The erratum from the patch needs to be applied on each
> > SERDES reconfiguration or reset. For example, when replugging different 
> > SFPs (sgmii - 10g - sgmii interface). Erratum 4_6 is done only once? 
> > My guess is to put it in mv88e639x_sgmii_pcs_post_config but still I 
> > need the device product number
> 
> You might be able to read the product number from the ID registers of
> the SERDES, registers 2 and 3 ? That is kind of cleaner. It is the
> SERDES which needs the workaround, so look at the SERDES ID ...
> 
> > maybe embedding a pointer to the 
> > mv88e6xxx_chip chip inside the mv88e639x_pcs struct would be the cleanest way.
>  
>  That would also work.

Correct me if I am wrong but I think we still need the chip ptr as pcs interface
provides access only to SERDES registers. If you are refering to "PHY IDENTIFIER"
registers (Page 0, Register 2,3), we need something like mv88e6xxx_port_read
unless we want to do some direct mdio magic?

Re: [PATCH net-next v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

[Ante Knezic]

> > It needs to be implemented exactly as posted here? After mv88e6390_serdes_power()
> > is called on any port/lane, mv88e6390x_serdes_erratum_3_14() needs to run
> > for all lanes? That might be a problem.
> 
> Actually, I tested applying erratum only on requested lane in pcs_post_config and
> it seems to work out fine, so we might use something like:
> static int mv88e6390_erratum_3_14(struct mv88e639x_pcs *mpcs)
> {
> 	int err;
> 
> 	/* 88e6190x and 88e6390x errata 3.14:
> 	 * After chip reset, SERDES reconfiguration or SERDES core
> 	 * Software Reset, the SERDES lanes may not be properly aligned
> 	 * resulting in CRC errors
> 	 */
> 
> 	err = mdiodev_c45_write(&mpcs->mdio, MDIO_MMD_PHYXS,
> 					 0xf054, 0x400C);
> 	if (err)
> 	        return err;
> 
> 	err = mdiodev_c45_write(&mpcs->mdio, MDIO_MMD_PHYXS,
> 				 0xf054, 0x4000);
> 	if (err)
> 	        return err;
> 
> 	return 0;
> }

Unfortunatelly, above statement is not correct. I managed to occasionally replicate
the issue when applying erratum on requested lane only. This happens on occasion
only but it looks like we need to apply erratum on all serdes lanes to ensure 
proper operation.
The Errata document falls short on this detail and does not clearly state whether all 
or only specific lanes need to be written to.