| Message ID | 20210930044421.1309538-1-vee.khee.wong@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3,1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: boon.leong.ong@intel.com; 1 maintainers not CCed: boon.leong.ong@intel.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 63 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote: > According to Synopsys DesignWare Cores Ethernet PCS databook, it is > required to disable Clause 37 auto-negotiation by programming bit-12 > (AN_ENABLE) to 0 if it is already enabled, before programming various > fields of VR_MII_AN_CTRL registers. > > After all these programming are done, it is then required to enable > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller") > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> > --- > v2 -> v3: > - Added error handling after xpcs_write(). > - Added 'changed' flag. > - Added fixes tag. > v1 -> v2: > - Removed use of xpcs_modify() helper function. > - Add conditional check on inband auto-negotiation. > --- > drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index fb0a83dc09ac..d2126f5d5016 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee); > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > { > - int ret; > + int ret, reg_val; > + int changed = 0; > > /* For AN for C37 SGMII mode, the settings are :- > - * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > - * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > + * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case > + it is already enabled) > + * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > + * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > * DW xPCS used with DW EQoS MAC is always MAC side SGMII. > - * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > + * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > * speed/duplex mode change by HW after SGMII AN complete) > + * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) > * > * Note: Since it is MAC side SGMII, there is no need to set > * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from > @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > * between PHY and Link Partner. There is also no need to > * trigger AN restart for MAC-side SGMII. > */ > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > + if (ret < 0) > + return ret; > + > + if (ret & AN_CL37_EN) { > + changed = 1; > + reg_val = ret & ~AN_CL37_EN; > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > + reg_val); > + if (ret < 0) > + return ret; > + } I don't think you need to record "changed" here - just maintain a local copy of the current state of the register, e.g: + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); + if (ret < 0) + return ret; + + reg_val = ret; + if (reg_val & AN_CL37_EN) { + reg_val &= ~AN_CL37_EN; ... What you're wanting to do is ensure this bit is clear before you do the register changes, and once complete, set the bit back to one. If the bit was previously clear, you can omit this write, otherwise the write was needed - as you have the code. However, the point is that once you're past this code, the state of the register in the device will have the AN_CL37_EN clear. See below for the rest of my comments on this... > @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > else > ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > > - return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > + if (ret < 0) > + return ret; > + > + if (changed) { > + if (phylink_autoneg_inband(mode)) > + reg_val |= AN_CL37_EN; > + else > + reg_val &= ~AN_CL37_EN; > + > + return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > + reg_val); > + } As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is clear in the register due to the effects of your change above. You say in the commit text: After all these programming are done, it is then required to enable Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. So that makes me think that you _always_ need to write back a value with AN_CL27_EN set. So: reg_val |= AN_CL37_EN; return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val); If that is not correct, the commit message is misleading and needs fixing. Lastly, I would recommend a much better name for this variable rather than the bland "reg_val" since you're needing the value preserved over a chunk of code. "ctrl" maybe?
On Thu, Sep 30, 2021 at 10:24:39AM +0100, Russell King (Oracle) wrote: > On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote: > > According to Synopsys DesignWare Cores Ethernet PCS databook, it is > > required to disable Clause 37 auto-negotiation by programming bit-12 > > (AN_ENABLE) to 0 if it is already enabled, before programming various > > fields of VR_MII_AN_CTRL registers. > > > > After all these programming are done, it is then required to enable > > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > > > Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller") > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > > Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> > > --- > > v2 -> v3: > > - Added error handling after xpcs_write(). > > - Added 'changed' flag. > > - Added fixes tag. > > v1 -> v2: > > - Removed use of xpcs_modify() helper function. > > - Add conditional check on inband auto-negotiation. > > --- > > drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++----- > > 1 file changed, 36 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index fb0a83dc09ac..d2126f5d5016 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee); > > > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > { > > - int ret; > > + int ret, reg_val; > > + int changed = 0; > > > > /* For AN for C37 SGMII mode, the settings are :- > > - * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > - * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > + * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case > > + it is already enabled) > > + * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > + * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > * DW xPCS used with DW EQoS MAC is always MAC side SGMII. > > - * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > + * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > * speed/duplex mode change by HW after SGMII AN complete) > > + * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) > > * > > * Note: Since it is MAC side SGMII, there is no need to set > > * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from > > @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > * between PHY and Link Partner. There is also no need to > > * trigger AN restart for MAC-side SGMII. > > */ > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > > + if (ret < 0) > > + return ret; > > + > > + if (ret & AN_CL37_EN) { > > + changed = 1; > > + reg_val = ret & ~AN_CL37_EN; > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > + reg_val); > > + if (ret < 0) > > + return ret; > > + } > > I don't think you need to record "changed" here - just maintain a > local copy of the current state of the register, e.g: > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > + if (ret < 0) > + return ret; > + > + reg_val = ret; > + if (reg_val & AN_CL37_EN) { > + reg_val &= ~AN_CL37_EN; > ... > > What you're wanting to do is ensure this bit is clear before you do the > register changes, and once complete, set the bit back to one. If the bit > was previously clear, you can omit this write, otherwise the write was > needed - as you have the code. However, the point is that once you're > past this code, the state of the register in the device will have the > AN_CL37_EN clear. See below for the rest of my comments on this... VK, I don't want to force you towards a certain implementation, but I just want to throw this out there, this works for me, and doesn't do more reads or writes than strictly necessary, and it also breaks up the wall-of-text comment into more digestible pieces: static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) { int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1; /* Disable SGMII AN in case it is already enabled */ mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); if (mdio_ctrl1 < 0) return mdio_ctrl1; if (mdio_ctrl1 & AN_CL37_EN) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, mdio_ctrl1 & ~AN_CL37_EN); if (ret < 0) return ret; } /* Set VR_MII_AN_CTRL[PCS_MODE] to SGMII AN, and * VR_MII_AN_CTRL[TX_CONFIG] to MAC side SGMII. */ old_an_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL); if (old_an_ctrl < 0) return old_an_ctrl; an_ctrl = old_an_ctrl & ~(DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_TX_CONFIG_MASK); an_ctrl |= (DW_VR_MII_PCS_MODE_C37_SGMII << DW_VR_MII_AN_CTRL_PCS_MODE_SHIFT) & DW_VR_MII_PCS_MODE_MASK; an_ctrl |= (DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII << DW_VR_MII_AN_CTRL_TX_CONFIG_SHIFT) & DW_VR_MII_TX_CONFIG_MASK; if (an_ctrl != old_an_ctrl) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, an_ctrl); if (ret < 0) return ret; } /* If using in-band autoneg, enable automatic speed/duplex mode change * by HW after SGMII AN complete. * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) */ old_dig_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1); if (old_dig_ctrl1 < 0) return old_dig_ctrl1; if (phylink_autoneg_inband(mode)) dig_ctrl1 = old_dig_ctrl1 | DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; else dig_ctrl1 = old_dig_ctrl1 & ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; if (dig_ctrl1 != old_dig_ctrl1) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, dig_ctrl1); if (ret < 0) return ret; } if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, mdio_ctrl1 | AN_CL37_EN); if (ret) return ret; } /* Note: Since it is MAC side SGMII, there is no need to set * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from PHY about * the link state change after C28 AN is completed between PHY and Link * Partner. There is also no need to trigger AN restart for MAC-side * SGMII. */ return 0; } > > > @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > else > > ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > > > > - return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > + if (ret < 0) > > + return ret; > > + > > + if (changed) { > > + if (phylink_autoneg_inband(mode)) > > + reg_val |= AN_CL37_EN; > > + else > > + reg_val &= ~AN_CL37_EN; > > + > > + return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > + reg_val); > > + } > > As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is > clear in the register due to the effects of your change above. You say > in the commit text: > > After all these programming are done, it is then required to enable > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > So that makes me think that you _always_ need to write back a value > with AN_CL27_EN set. So: > > reg_val |= AN_CL37_EN; > return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val); I will only say this: modifying the last part of the function I posted above like this: // if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, mdio_ctrl1 | AN_CL37_EN); if (ret) return ret; // } aka unconditionally writing an mdio_ctrl1 value with the AN_CL37_EN bit set, will still end up with a functional link. But the only reason for that is this: static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int mode, int speed, int duplex) { int val, ret; if (phylink_autoneg_inband(mode)) return; switch (speed) { case SPEED_1000: val = BMCR_SPEED1000; break; case SPEED_100: val = BMCR_SPEED100; break; case SPEED_10: val = BMCR_SPEED10; break; default: return; } if (duplex == DUPLEX_FULL) val |= BMCR_FULLDPLX; ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val); <- this clears the AN_CL37_EN bit again if (ret) pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret)); } If I add "val |= AN_CL37_EN;" before the xpcs_write in xpcs_link_up_sgmii(), my SGMII link with in-band autoneg turned off breaks. > If that is not correct, the commit message is misleading and needs > fixing. > > Lastly, I would recommend a much better name for this variable rather > than the bland "reg_val" since you're needing the value preserved over > a chunk of code. "ctrl" maybe?
On Fri, Oct 01, 2021 at 12:19:52AM +0000, Vladimir Oltean wrote: > On Thu, Sep 30, 2021 at 10:24:39AM +0100, Russell King (Oracle) wrote: > > On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote: > > > According to Synopsys DesignWare Cores Ethernet PCS databook, it is > > > required to disable Clause 37 auto-negotiation by programming bit-12 > > > (AN_ENABLE) to 0 if it is already enabled, before programming various > > > fields of VR_MII_AN_CTRL registers. > > > > > > After all these programming are done, it is then required to enable > > > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > > > > > Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller") > > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > > > Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> > > > --- > > > v2 -> v3: > > > - Added error handling after xpcs_write(). > > > - Added 'changed' flag. > > > - Added fixes tag. > > > v1 -> v2: > > > - Removed use of xpcs_modify() helper function. > > > - Add conditional check on inband auto-negotiation. > > > --- > > > drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++----- > > > 1 file changed, 36 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > > index fb0a83dc09ac..d2126f5d5016 100644 > > > --- a/drivers/net/pcs/pcs-xpcs.c > > > +++ b/drivers/net/pcs/pcs-xpcs.c > > > @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee); > > > > > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > > { > > > - int ret; > > > + int ret, reg_val; > > > + int changed = 0; > > > > > > /* For AN for C37 SGMII mode, the settings are :- > > > - * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > > - * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > > + * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case > > > + it is already enabled) > > > + * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > > + * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > > * DW xPCS used with DW EQoS MAC is always MAC side SGMII. > > > - * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > > + * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > > * speed/duplex mode change by HW after SGMII AN complete) > > > + * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) > > > * > > > * Note: Since it is MAC side SGMII, there is no need to set > > > * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from > > > @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > > * between PHY and Link Partner. There is also no need to > > > * trigger AN restart for MAC-side SGMII. > > > */ > > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret & AN_CL37_EN) { > > > + changed = 1; > > > + reg_val = ret & ~AN_CL37_EN; > > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > > + reg_val); > > > + if (ret < 0) > > > + return ret; > > > + } > > > > I don't think you need to record "changed" here - just maintain a > > local copy of the current state of the register, e.g: > > > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > > + if (ret < 0) > > + return ret; > > + > > + reg_val = ret; > > + if (reg_val & AN_CL37_EN) { > > + reg_val &= ~AN_CL37_EN; > > ... > > > > What you're wanting to do is ensure this bit is clear before you do the > > register changes, and once complete, set the bit back to one. If the bit > > was previously clear, you can omit this write, otherwise the write was > > needed - as you have the code. However, the point is that once you're > > past this code, the state of the register in the device will have the > > AN_CL37_EN clear. See below for the rest of my comments on this... > > VK, I don't want to force you towards a certain implementation, but I > just want to throw this out there, this works for me, and doesn't do > more reads or writes than strictly necessary, and it also breaks up the > wall-of-text comment into more digestible pieces: I agree on breaking up the comments, which make it more readable. Also do not perform unnecessary writes. What about doing this once the fix is merged into net-next? It seems there are a lot of clean-ups need to be introduced, which is more reasonable to be in net-next. > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > { > int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1; > > /* Disable SGMII AN in case it is already enabled */ > mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > if (mdio_ctrl1 < 0) > return mdio_ctrl1; > > if (mdio_ctrl1 & AN_CL37_EN) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > mdio_ctrl1 & ~AN_CL37_EN); > if (ret < 0) > return ret; > } > > /* Set VR_MII_AN_CTRL[PCS_MODE] to SGMII AN, and > * VR_MII_AN_CTRL[TX_CONFIG] to MAC side SGMII. > */ > old_an_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL); > if (old_an_ctrl < 0) > return old_an_ctrl; > > an_ctrl = old_an_ctrl & ~(DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_TX_CONFIG_MASK); > an_ctrl |= (DW_VR_MII_PCS_MODE_C37_SGMII << DW_VR_MII_AN_CTRL_PCS_MODE_SHIFT) & > DW_VR_MII_PCS_MODE_MASK; > an_ctrl |= (DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII << DW_VR_MII_AN_CTRL_TX_CONFIG_SHIFT) & > DW_VR_MII_TX_CONFIG_MASK; > > if (an_ctrl != old_an_ctrl) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, an_ctrl); > if (ret < 0) > return ret; > } > > /* If using in-band autoneg, enable automatic speed/duplex mode change > * by HW after SGMII AN complete. > * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) > */ > old_dig_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1); > if (old_dig_ctrl1 < 0) > return old_dig_ctrl1; > > if (phylink_autoneg_inband(mode)) > dig_ctrl1 = old_dig_ctrl1 | DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > else > dig_ctrl1 = old_dig_ctrl1 & ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > > if (dig_ctrl1 != old_dig_ctrl1) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, dig_ctrl1); > if (ret < 0) > return ret; > } > > if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > mdio_ctrl1 | AN_CL37_EN); > if (ret) > return ret; > } > > /* Note: Since it is MAC side SGMII, there is no need to set > * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from PHY about > * the link state change after C28 AN is completed between PHY and Link > * Partner. There is also no need to trigger AN restart for MAC-side > * SGMII. > */ > > return 0; > } > > > > > > @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > > else > > > ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > > > > > > - return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (changed) { > > > + if (phylink_autoneg_inband(mode)) > > > + reg_val |= AN_CL37_EN; > > > + else > > > + reg_val &= ~AN_CL37_EN; > > > + > > > + return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > > + reg_val); > > > + } > > > > As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is > > clear in the register due to the effects of your change above. You say > > in the commit text: > > > > After all these programming are done, it is then required to enable > > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > > > So that makes me think that you _always_ need to write back a value > > with AN_CL27_EN set. So: > > > > reg_val |= AN_CL37_EN; > > return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val); > > I will only say this: modifying the last part of the function I posted > above like this: > > // if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > mdio_ctrl1 | AN_CL37_EN); > if (ret) > return ret; > // } > > aka unconditionally writing an mdio_ctrl1 value with the AN_CL37_EN bit set, > will still end up with a functional link. But the only reason for that > is this: > > static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int mode, > int speed, int duplex) > { > int val, ret; > > if (phylink_autoneg_inband(mode)) > return; > > switch (speed) { > case SPEED_1000: > val = BMCR_SPEED1000; > break; > case SPEED_100: > val = BMCR_SPEED100; > break; > case SPEED_10: > val = BMCR_SPEED10; > break; > default: > return; > } > > if (duplex == DUPLEX_FULL) > val |= BMCR_FULLDPLX; > > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val); <- this clears the AN_CL37_EN bit again > if (ret) > pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret)); > } > > If I add "val |= AN_CL37_EN;" before the xpcs_write in xpcs_link_up_sgmii(), > my SGMII link with in-band autoneg turned off breaks. > > > If that is not correct, the commit message is misleading and needs > > fixing. > > > > Lastly, I would recommend a much better name for this variable rather > > than the bland "reg_val" since you're needing the value preserved over > > a chunk of code. "ctrl" maybe?
On Thu, Sep 30, 2021 at 10:24:39AM +0100, Russell King (Oracle) wrote: > On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote: > > According to Synopsys DesignWare Cores Ethernet PCS databook, it is > > required to disable Clause 37 auto-negotiation by programming bit-12 > > (AN_ENABLE) to 0 if it is already enabled, before programming various > > fields of VR_MII_AN_CTRL registers. > > > > After all these programming are done, it is then required to enable > > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > > > Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller") > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > > Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> > > --- > > v2 -> v3: > > - Added error handling after xpcs_write(). > > - Added 'changed' flag. > > - Added fixes tag. > > v1 -> v2: > > - Removed use of xpcs_modify() helper function. > > - Add conditional check on inband auto-negotiation. > > --- > > drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++----- > > 1 file changed, 36 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index fb0a83dc09ac..d2126f5d5016 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee); > > > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > { > > - int ret; > > + int ret, reg_val; > > + int changed = 0; > > > > /* For AN for C37 SGMII mode, the settings are :- > > - * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > - * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > + * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case > > + it is already enabled) > > + * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) > > + * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) > > * DW xPCS used with DW EQoS MAC is always MAC side SGMII. > > - * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > + * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic > > * speed/duplex mode change by HW after SGMII AN complete) > > + * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) > > * > > * Note: Since it is MAC side SGMII, there is no need to set > > * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from > > @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > * between PHY and Link Partner. There is also no need to > > * trigger AN restart for MAC-side SGMII. > > */ > > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > > + if (ret < 0) > > + return ret; > > + > > + if (ret & AN_CL37_EN) { > > + changed = 1; > > + reg_val = ret & ~AN_CL37_EN; > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > + reg_val); > > + if (ret < 0) > > + return ret; > > + } > > I don't think you need to record "changed" here - just maintain a > local copy of the current state of the register, e.g: > You're right. Will remove it on v4. > + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > + if (ret < 0) > + return ret; > + > + reg_val = ret; > + if (reg_val & AN_CL37_EN) { > + reg_val &= ~AN_CL37_EN; > ... > > What you're wanting to do is ensure this bit is clear before you do the > register changes, and once complete, set the bit back to one. If the bit > was previously clear, you can omit this write, otherwise the write was > needed - as you have the code. However, the point is that once you're > past this code, the state of the register in the device will have the > AN_CL37_EN clear. See below for the rest of my comments on this... > > > @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > else > > ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; > > > > - return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > > + if (ret < 0) > > + return ret; > > + > > + if (changed) { > > + if (phylink_autoneg_inband(mode)) > > + reg_val |= AN_CL37_EN; > > + else > > + reg_val &= ~AN_CL37_EN; > > + > > + return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > + reg_val); > > + } > > As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is > clear in the register due to the effects of your change above. You say > in the commit text: > > After all these programming are done, it is then required to enable > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. > > So that makes me think that you _always_ need to write back a value > with AN_CL27_EN set. So: > > reg_val |= AN_CL37_EN; > return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val); > > If that is not correct, the commit message is misleading and needs > fixing. > It is written in the Synopsys PCS Databook: 8. Enable CL37 Auto-negotiation, by programming bit[12] of the SR_MII_CTRL Register to 1. So I think we always need to write back the value. Will fix this in v4. > Lastly, I would recommend a much better name for this variable rather > than the bland "reg_val" since you're needing the value preserved over > a chunk of code. "ctrl" maybe? > I will rename it to 'mdio_ctrl'. > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Oct 01, 2021 at 12:19:52AM +0000, Vladimir Oltean wrote: > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > { > int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1; > > /* Disable SGMII AN in case it is already enabled */ > mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > if (mdio_ctrl1 < 0) > return mdio_ctrl1; > > if (mdio_ctrl1 & AN_CL37_EN) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > mdio_ctrl1 & ~AN_CL37_EN); > if (ret < 0) > return ret; > } This is fine... > if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > mdio_ctrl1 | AN_CL37_EN); > if (ret) > return ret; > } This is not. If the control register had AN_CL37_EN set initially, then in the first test above, we clear the bit. However, mdio_ctrl1 will still contain the bit set. When we get here, we will skip setting the register bit back to one even if in-band mode was requested. As I said in a previous email, at this point there is no reason to check the previous state, because if it was set on entry, we will have cleared it, so the register state at this point has the bit clear no matter what. If we need to set it, then we /always/ need to write it here - it doesn't matter what the initial state was.
On Fri, Oct 01, 2021 at 10:57:30AM +0100, Russell King (Oracle) wrote: > On Fri, Oct 01, 2021 at 12:19:52AM +0000, Vladimir Oltean wrote: > > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) > > { > > int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1; > > > > /* Disable SGMII AN in case it is already enabled */ > > mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); > > if (mdio_ctrl1 < 0) > > return mdio_ctrl1; > > > > if (mdio_ctrl1 & AN_CL37_EN) { > > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > mdio_ctrl1 & ~AN_CL37_EN); > > if (ret < 0) > > return ret; > > } > > This is fine... > > > if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) { > > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, > > mdio_ctrl1 | AN_CL37_EN); > > if (ret) > > return ret; > > } > > This is not. If the control register had AN_CL37_EN set initially, then > in the first test above, we clear the bit. However, mdio_ctrl1 will > still contain the bit set. When we get here, we will skip setting the > register bit back to one even if in-band mode was requested. > > As I said in a previous email, at this point there is no reason to check > the previous state, because if it was set on entry, we will have cleared > it, so the register state at this point has the bit clear no matter > what. If we need to set it, then we /always/ need to write it here - it > doesn't matter what the initial state was. Correct, it should have been: if (phylink_autoneg_inband(mode)) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, mdio_ctrl1 | AN_CL37_EN); if (ret) return ret; } For the record, just in case this code gets copied anywhere, there's another mistake in my placement of one of the comments. /* If using in-band autoneg, enable automatic speed/duplex mode change * by HW after SGMII AN complete. * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) <- this part of the comment */ old_dig_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1); if (old_dig_ctrl1 < 0) return old_dig_ctrl1; really belongs here: /* If using SGMII AN, enable it here */ if (phylink_autoneg_inband(mode)) { ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, mdio_ctrl1 | AN_CL37_EN); if (ret) return ret; }
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index fb0a83dc09ac..d2126f5d5016 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee); static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) { - int ret; + int ret, reg_val; + int changed = 0; /* For AN for C37 SGMII mode, the settings are :- - * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) - * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) + * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case + it is already enabled) + * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN) + * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII) * DW xPCS used with DW EQoS MAC is always MAC side SGMII. - * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic + * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic * speed/duplex mode change by HW after SGMII AN complete) + * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN) * * Note: Since it is MAC side SGMII, there is no need to set * SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) * between PHY and Link Partner. There is also no need to * trigger AN restart for MAC-side SGMII. */ + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL); + if (ret < 0) + return ret; + + if (ret & AN_CL37_EN) { + changed = 1; + reg_val = ret & ~AN_CL37_EN; + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, + reg_val); + if (ret < 0) + return ret; + } + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL); if (ret < 0) return ret; @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode) else ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW; - return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); + if (ret < 0) + return ret; + + if (changed) { + if (phylink_autoneg_inband(mode)) + reg_val |= AN_CL37_EN; + else + reg_val &= ~AN_CL37_EN; + + return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, + reg_val); + } + + return ret; } static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
According to Synopsys DesignWare Cores Ethernet PCS databook, it is required to disable Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 0 if it is already enabled, before programming various fields of VR_MII_AN_CTRL registers. After all these programming are done, it is then required to enable Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1. Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller") Cc: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> --- v2 -> v3: - Added error handling after xpcs_write(). - Added 'changed' flag. - Added fixes tag. v1 -> v2: - Removed use of xpcs_modify() helper function. - Add conditional check on inband auto-negotiation. --- drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-)