| Message ID | 20220209202508.4419-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: realtek: rtl8365mb: read phy regs from memory when MDIO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| 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 8 of 8 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/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Luiz, Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: > In a SMI-connected switch, indirect registers are accessed commanding > the switch to access an address by reading and writing a sequence of > chip registers. However, that process might return a null reading > (0x0000) with no errors if the switch is under stress. When that > happens, the system will detect a link down, followed by a link up, > creating temporary link unavailability. > > It was observed only with idle SMP devices and interruptions are not in > use (the system was polling each interface status every second). It > happened with both SMI and MDIO-connected switches. If the OS is under > any load, the same problem does not appear, probably because the polling > process is delayed or serialized. I am not sure I agree with your description here. It is more likely that there is just a bug in the driver, rather than some problem with indirect PHY register access "when the switch is under stress". Please see my reply to your earlier thread. Having said that, using the more direct approach makes sense - I think I said the same in your original MDIO series too. So this change is justifiable in its own right. I suggest resending this without talking in too much detail about a bug which is still unresolved. Kind regards, Alvin > > Although that method to read indirect registers work independently from > the management interface, MDIO-connected can skip part of the process. > Instead of commanding the switch to read a register, the driver can read > directly from the region after RTL8365MB_PHY_BASE=0x0200. It was enough > to workaround the null readings. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/rtl8365mb.c | 115 ++++++++++++++++++++++++---- > 1 file changed, 99 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 2ed592147c20..e0c7f64bc56f 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -595,8 +595,7 @@ static int rtl8365mb_phy_poll_busy(struct realtek_priv *priv) > val, !val, 10, 100); > } > > -static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, > - u32 ocp_addr) > +static int rtl8365mb_phy_set_ocp_prefix(struct realtek_priv *priv, u32 ocp_addr) > { > u32 val; > int ret; > @@ -610,19 +609,22 @@ static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, > if (ret) > return ret; > > - /* Set PHY register address */ > - val = RTL8365MB_PHY_BASE; > - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy); > - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK, > + return 0; > +} > + > +static u32 rtl8365mb_phy_ocp_ind_addr(int phy, u32 ocp_addr) > +{ > + u32 ind_addr; > + > + ind_addr = RTL8365MB_PHY_BASE; > + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, > + phy); > + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK, > ocp_addr >> 1); > - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK, > + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK, > ocp_addr >> 6); > - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, > - val); > - if (ret) > - return ret; > > - return 0; > + return ind_addr; > } > > static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, > @@ -635,7 +637,13 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, > if (ret) > return ret; > > - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); > + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); > + if (ret) > + return ret; > + > + /* Set PHY register address */ > + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, > + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr)); > if (ret) > return ret; > > @@ -673,7 +681,13 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, > if (ret) > return ret; > > - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); > + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); > + if (ret) > + return ret; > + > + /* Set PHY register address */ > + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, > + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr)); > if (ret) > return ret; > > @@ -757,13 +771,82 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, > > static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > { > - return rtl8365mb_phy_read(ds->priv, phy, regnum); > + struct realtek_priv *priv = ds->priv; > + u32 ocp_addr; > + u32 ind_addr; > + uint val; > + int ret; > + > + if (phy > RTL8365MB_PHYADDRMAX) > + return -EINVAL; > + > + if (regnum > RTL8365MB_PHYREGMAX) > + return -EINVAL; > + > + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2; > + > + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); > + if (ret) > + return ret; > + > + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr); > + > + /* MDIO-connected switches can read directly from ind_addr (0x2000..) > + * although using RTL8365MB_INDIRECT_ACCESS_* does work. > + */ > + ret = regmap_read(priv->map, ind_addr, &val); > + if (ret) { > + dev_err(priv->dev, > + "failed to read PHY%d reg %02x @ %04x, ret %d\n", phy, > + regnum, ocp_addr, ret); > + return ret; > + } > + > + val = val & 0xFFFF; > + > + dev_dbg(priv->dev, "read PHY%d register 0x%02x @ %04x, val <- %04x\n", > + phy, regnum, ocp_addr, val); > + > + return val; > } > > static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > u16 val) > { > - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); > + struct realtek_priv *priv = ds->priv; > + u32 ocp_addr; > + u32 ind_addr; > + int ret; > + > + if (phy > RTL8365MB_PHYADDRMAX) > + return -EINVAL; > + > + if (regnum > RTL8365MB_PHYREGMAX) > + return -EINVAL; > + > + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2; > + > + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); > + if (ret) > + return ret; > + > + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr); > + > + /* MDIO-connected switches can write directly from ind_addr (0x2000..) > + * although using RTL8365MB_INDIRECT_ACCESS_* does work. > + */ > + ret = regmap_write(priv->map, ind_addr, val); > + if (ret) { > + dev_err(priv->dev, > + "failed to write PHY%d reg %02x @ %04x, ret %d\n", phy, > + regnum, ocp_addr, ret); > + return ret; > + } > + > + dev_dbg(priv->dev, "write PHY%d register 0x%02x @ %04x, val -> %04x\n", > + phy, regnum, ocp_addr, val); > + > + return 0; > } > > static enum dsa_tag_protocol
> I am not sure I agree with your description here. It is more likely that > there is just a bug in the driver, rather than some problem with > indirect PHY register access "when the switch is under stress". Please > see my reply to your earlier thread. I tend to agree. Reading PHY registers happens quite often due to polling every second, so if there is a problem, this is where you see it. But how do you know the same issue does not appear somewhere else in the driver? Maybe add code to dump the ATU, poll it frequently, and see if any entries change unexpectedly? Andrew
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 2ed592147c20..e0c7f64bc56f 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -595,8 +595,7 @@ static int rtl8365mb_phy_poll_busy(struct realtek_priv *priv) val, !val, 10, 100); } -static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, - u32 ocp_addr) +static int rtl8365mb_phy_set_ocp_prefix(struct realtek_priv *priv, u32 ocp_addr) { u32 val; int ret; @@ -610,19 +609,22 @@ static int rtl8365mb_phy_ocp_prepare(struct realtek_priv *priv, int phy, if (ret) return ret; - /* Set PHY register address */ - val = RTL8365MB_PHY_BASE; - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, phy); - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK, + return 0; +} + +static u32 rtl8365mb_phy_ocp_ind_addr(int phy, u32 ocp_addr) +{ + u32 ind_addr; + + ind_addr = RTL8365MB_PHY_BASE; + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK, + phy); + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK, ocp_addr >> 1); - val |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK, + ind_addr |= FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK, ocp_addr >> 6); - ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, - val); - if (ret) - return ret; - return 0; + return ind_addr; } static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, @@ -635,7 +637,13 @@ static int rtl8365mb_phy_ocp_read(struct realtek_priv *priv, int phy, if (ret) return ret; - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); + if (ret) + return ret; + + /* Set PHY register address */ + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr)); if (ret) return ret; @@ -673,7 +681,13 @@ static int rtl8365mb_phy_ocp_write(struct realtek_priv *priv, int phy, if (ret) return ret; - ret = rtl8365mb_phy_ocp_prepare(priv, phy, ocp_addr); + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); + if (ret) + return ret; + + /* Set PHY register address */ + ret = regmap_write(priv->map, RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG, + rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr)); if (ret) return ret; @@ -757,13 +771,82 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) { - return rtl8365mb_phy_read(ds->priv, phy, regnum); + struct realtek_priv *priv = ds->priv; + u32 ocp_addr; + u32 ind_addr; + uint val; + int ret; + + if (phy > RTL8365MB_PHYADDRMAX) + return -EINVAL; + + if (regnum > RTL8365MB_PHYREGMAX) + return -EINVAL; + + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2; + + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); + if (ret) + return ret; + + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr); + + /* MDIO-connected switches can read directly from ind_addr (0x2000..) + * although using RTL8365MB_INDIRECT_ACCESS_* does work. + */ + ret = regmap_read(priv->map, ind_addr, &val); + if (ret) { + dev_err(priv->dev, + "failed to read PHY%d reg %02x @ %04x, ret %d\n", phy, + regnum, ocp_addr, ret); + return ret; + } + + val = val & 0xFFFF; + + dev_dbg(priv->dev, "read PHY%d register 0x%02x @ %04x, val <- %04x\n", + phy, regnum, ocp_addr, val); + + return val; } static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val) { - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); + struct realtek_priv *priv = ds->priv; + u32 ocp_addr; + u32 ind_addr; + int ret; + + if (phy > RTL8365MB_PHYADDRMAX) + return -EINVAL; + + if (regnum > RTL8365MB_PHYREGMAX) + return -EINVAL; + + ocp_addr = RTL8365MB_PHY_OCP_ADDR_PHYREG_BASE + regnum * 2; + + ret = rtl8365mb_phy_set_ocp_prefix(priv, ocp_addr); + if (ret) + return ret; + + ind_addr = rtl8365mb_phy_ocp_ind_addr(phy, ocp_addr); + + /* MDIO-connected switches can write directly from ind_addr (0x2000..) + * although using RTL8365MB_INDIRECT_ACCESS_* does work. + */ + ret = regmap_write(priv->map, ind_addr, val); + if (ret) { + dev_err(priv->dev, + "failed to write PHY%d reg %02x @ %04x, ret %d\n", phy, + regnum, ocp_addr, ret); + return ret; + } + + dev_dbg(priv->dev, "write PHY%d register 0x%02x @ %04x, val -> %04x\n", + phy, regnum, ocp_addr, val); + + return 0; } static enum dsa_tag_protocol
In a SMI-connected switch, indirect registers are accessed commanding the switch to access an address by reading and writing a sequence of chip registers. However, that process might return a null reading (0x0000) with no errors if the switch is under stress. When that happens, the system will detect a link down, followed by a link up, creating temporary link unavailability. It was observed only with idle SMP devices and interruptions are not in use (the system was polling each interface status every second). It happened with both SMI and MDIO-connected switches. If the OS is under any load, the same problem does not appear, probably because the polling process is delayed or serialized. Although that method to read indirect registers work independently from the management interface, MDIO-connected can skip part of the process. Instead of commanding the switch to read a register, the driver can read directly from the region after RTL8365MB_PHY_BASE=0x0200. It was enough to workaround the null readings. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/rtl8365mb.c | 115 ++++++++++++++++++++++++---- 1 file changed, 99 insertions(+), 16 deletions(-)