| Message ID | 20230714160612.11701-1-ante.knezic@helmholz.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
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
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
> 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.
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()?
> > > 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.
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?
> 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
> 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.
> > 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?
> > 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.
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; }
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(+)