| Message ID | 20230224155235.512695-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Regressions in Ocelot switch drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 14 of 14 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 2/24/23 07:52, Vladimir Oltean wrote: > During the refactoring in the commit below, vsc9953_mdio_read() was > replaced with mscc_miim_read(), which has one extra step: it checks for > the MSCC_MIIM_DATA_ERROR bits before returning the result. > > On T1040RDB, there are 8 QSGMII PCSes belonging to the switch, and they > are organized in 2 groups. First group responds to MDIO addresses 4-7 > because QSGMIIACR1[MDEV_PORT] is 1, and the second group responds to > MDIO addresses 8-11 because QSGMIIBCR1[MDEV_PORT] is 2. I have double > checked that these values are correctly set in the SERDES, as well as > PCCR1[QSGMA_CFG] and PCCR1[QSGMB_CFG] are both 0b01. > > mscc_miim_read: phyad 8 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 8 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 8 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 8 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 9 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 9 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 9 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 9 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 10 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 10 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 10 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 10 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 11 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 11 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 11 reg 0x1 MIIM_DATA 0x2d > mscc_miim_read: phyad 11 reg 0x5 MIIM_DATA 0x5801 > mscc_miim_read: phyad 4 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 4 reg 0x5 MIIM_DATA 0x3da01, ERROR > mscc_miim_read: phyad 5 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 5 reg 0x5 MIIM_DATA 0x35801, ERROR > mscc_miim_read: phyad 5 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 5 reg 0x5 MIIM_DATA 0x35801, ERROR > mscc_miim_read: phyad 6 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 6 reg 0x5 MIIM_DATA 0x35801, ERROR > mscc_miim_read: phyad 6 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 6 reg 0x5 MIIM_DATA 0x35801, ERROR > mscc_miim_read: phyad 7 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 7 reg 0x5 MIIM_DATA 0x35801, ERROR > mscc_miim_read: phyad 7 reg 0x1 MIIM_DATA 0x3002d, ERROR > mscc_miim_read: phyad 7 reg 0x5 MIIM_DATA 0x35801, ERROR > > As can be seen, the data in MIIM_DATA is still valid despite having the > MSCC_MIIM_DATA_ERROR bits set. The driver as introduced in commit > 84705fc16552 ("net: dsa: felix: introduce support for Seville VSC9953 > switch") was ignoring these bits, perhaps deliberately (although > unbeknownst to me). > > This is an old IP and the hardware team cannot seem to be able to help > me track down a plausible reason for these failures. I'll keep > investigating, but in the meantime, this is a direct regression which > must be restored to a working state. > > The only thing I can do is keep ignoring the errors as before. > > Fixes: b99658452355 ("net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index 287b64b788db..563ad338da25 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -893,8 +893,8 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot) rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus", ocelot->targets[GCB], - ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK]); - + ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK], + true); if (rc) { dev_err(dev, "failed to setup MDIO bus\n"); return rc; diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c index c87e991d1a17..1a1b95ae95fa 100644 --- a/drivers/net/mdio/mdio-mscc-miim.c +++ b/drivers/net/mdio/mdio-mscc-miim.c @@ -52,6 +52,7 @@ struct mscc_miim_info { struct mscc_miim_dev { struct regmap *regs; int mii_status_offset; + bool ignore_read_errors; struct regmap *phy_regs; const struct mscc_miim_info *info; struct clk *clk; @@ -135,7 +136,7 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum) goto out; } - if (val & MSCC_MIIM_DATA_ERROR) { + if (!miim->ignore_read_errors && !!(val & MSCC_MIIM_DATA_ERROR)) { ret = -EIO; goto out; } @@ -212,7 +213,8 @@ static const struct regmap_config mscc_miim_phy_regmap_config = { }; int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name, - struct regmap *mii_regmap, int status_offset) + struct regmap *mii_regmap, int status_offset, + bool ignore_read_errors) { struct mscc_miim_dev *miim; struct mii_bus *bus; @@ -234,6 +236,7 @@ int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name, miim->regs = mii_regmap; miim->mii_status_offset = status_offset; + miim->ignore_read_errors = ignore_read_errors; *pbus = bus; @@ -285,7 +288,7 @@ static int mscc_miim_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(phy_regmap), "Unable to create phy register regmap\n"); - ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_regmap, 0); + ret = mscc_miim_setup(dev, &bus, "mscc_miim", mii_regmap, 0, false); if (ret < 0) { dev_err(dev, "Unable to setup the MDIO bus\n"); return ret; diff --git a/include/linux/mdio/mdio-mscc-miim.h b/include/linux/mdio/mdio-mscc-miim.h index 5b4ed2c3cbb9..1ce699740af6 100644 --- a/include/linux/mdio/mdio-mscc-miim.h +++ b/include/linux/mdio/mdio-mscc-miim.h @@ -14,6 +14,6 @@ int mscc_miim_setup(struct device *device, struct mii_bus **bus, const char *name, struct regmap *mii_regmap, - int status_offset); + int status_offset, bool ignore_read_errors); #endif
During the refactoring in the commit below, vsc9953_mdio_read() was replaced with mscc_miim_read(), which has one extra step: it checks for the MSCC_MIIM_DATA_ERROR bits before returning the result. On T1040RDB, there are 8 QSGMII PCSes belonging to the switch, and they are organized in 2 groups. First group responds to MDIO addresses 4-7 because QSGMIIACR1[MDEV_PORT] is 1, and the second group responds to MDIO addresses 8-11 because QSGMIIBCR1[MDEV_PORT] is 2. I have double checked that these values are correctly set in the SERDES, as well as PCCR1[QSGMA_CFG] and PCCR1[QSGMB_CFG] are both 0b01. mscc_miim_read: phyad 8 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 8 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 8 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 8 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 9 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 9 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 9 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 9 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 10 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 10 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 10 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 10 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 11 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 11 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 11 reg 0x1 MIIM_DATA 0x2d mscc_miim_read: phyad 11 reg 0x5 MIIM_DATA 0x5801 mscc_miim_read: phyad 4 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 4 reg 0x5 MIIM_DATA 0x3da01, ERROR mscc_miim_read: phyad 5 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 5 reg 0x5 MIIM_DATA 0x35801, ERROR mscc_miim_read: phyad 5 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 5 reg 0x5 MIIM_DATA 0x35801, ERROR mscc_miim_read: phyad 6 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 6 reg 0x5 MIIM_DATA 0x35801, ERROR mscc_miim_read: phyad 6 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 6 reg 0x5 MIIM_DATA 0x35801, ERROR mscc_miim_read: phyad 7 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 7 reg 0x5 MIIM_DATA 0x35801, ERROR mscc_miim_read: phyad 7 reg 0x1 MIIM_DATA 0x3002d, ERROR mscc_miim_read: phyad 7 reg 0x5 MIIM_DATA 0x35801, ERROR As can be seen, the data in MIIM_DATA is still valid despite having the MSCC_MIIM_DATA_ERROR bits set. The driver as introduced in commit 84705fc16552 ("net: dsa: felix: introduce support for Seville VSC9953 switch") was ignoring these bits, perhaps deliberately (although unbeknownst to me). This is an old IP and the hardware team cannot seem to be able to help me track down a plausible reason for these failures. I'll keep investigating, but in the meantime, this is a direct regression which must be restored to a working state. The only thing I can do is keep ignoring the errors as before. Fixes: b99658452355 ("net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/ocelot/seville_vsc9953.c | 4 ++-- drivers/net/mdio/mdio-mscc-miim.c | 9 ++++++--- include/linux/mdio/mdio-mscc-miim.h | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-)