| Message ID | 20210114024055.17602-1-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: mv88e6xxx: do not allow inband AN for 2500base-x mode | 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 | success | CCed 8 of 8 maintainers |
| 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, 56 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c > index 3195936dc5be..b8241820679e 100644 > --- a/drivers/net/dsa/mv88e6xxx/serdes.c > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c > @@ -55,9 +55,20 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > { > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > + > + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) { > + if (state->link) { > + state->speed = SPEED_2500; > + state->duplex = DUPLEX_FULL; > + } > + > + return 0; > + } > + > + state->an_complete = 1; Should this be here? It seems like a logically different change, it is not clear to me it is to do with PHY_INTERFACE_MODE_2500BASEX. > state->duplex = status & > MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ? > - DUPLEX_FULL : DUPLEX_HALF; > + DUPLEX_FULL : DUPLEX_HALF; This looks like an unintended white space change. Andrew
On Fri, 15 Jan 2021 00:00:33 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c > > index 3195936dc5be..b8241820679e 100644 > > --- a/drivers/net/dsa/mv88e6xxx/serdes.c > > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c > > @@ -55,9 +55,20 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > > { > > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > > state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > + > > + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) { > > + if (state->link) { > > + state->speed = SPEED_2500; > > + state->duplex = DUPLEX_FULL; > > + } > > + > > + return 0; > > + } > > + > > + state->an_complete = 1; > > Should this be here? It seems like a logically different change, it is > not clear to me it is to do with PHY_INTERFACE_MODE_2500BASEX. This function does not set an_complete at all, and as I understand it, it should. But maybe this should be in different commit, and more thought put into it. I will rethink it and send another version. > > state->duplex = status & > > MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ? > > - DUPLEX_FULL : DUPLEX_HALF; > > + DUPLEX_FULL : DUPLEX_HALF; > > This looks like an unintended white space change. This change is intended. There were 17 space there istead of 2 tabs + 1 space. And the last space is not needed, since it does not provide any other alignment. Should this be in separate commit? BTW Andrew, the code in serdes.c does many read and write calls, and it could be simplified a lot by implementing modify, setbits and clearbits methods, like phy.h implements. Or maybe we can use phy_mmd_* methods here instead of mv88e6390_serdes_read/write ? I fear such change will make future backporting of new fix commits a pain. But I still think it should be done. What do you think? Marek
On Fri, Jan 15, 2021 at 12:55:16AM +0100, Marek Behún wrote: > On Fri, 15 Jan 2021 00:00:33 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c > > > index 3195936dc5be..b8241820679e 100644 > > > --- a/drivers/net/dsa/mv88e6xxx/serdes.c > > > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c > > > @@ -55,9 +55,20 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > > > { > > > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > > > state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > > + > > > + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) { > > > + if (state->link) { > > > + state->speed = SPEED_2500; > > > + state->duplex = DUPLEX_FULL; > > > + } > > > + > > > + return 0; > > > + } > > > + > > > + state->an_complete = 1; > > > > Should this be here? It seems like a logically different change, it is > > not clear to me it is to do with PHY_INTERFACE_MODE_2500BASEX. > > This function does not set an_complete at all, and as I understand it, > it should. But maybe this should be in different commit, and more > thought put into it. I will rethink it and send another version. Thanks. A patch should do one thing, do it well, and have a good commit message explaining why it is making the change. So yes, please do put this into a patch of its own. > > > > state->duplex = status & > > > MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ? > > > - DUPLEX_FULL : DUPLEX_HALF; > > > + DUPLEX_FULL : DUPLEX_HALF; > > > > This looks like an unintended white space change. > > This change is intended. There were 17 space there istead of 2 tabs + 1 > space. And the last space is not needed, since it does not provide any > other alignment. Should this be in separate commit? Yes, white space changes should always be in a separate patch. They distract from the actual change being made here. > BTW Andrew, the code in serdes.c does many read and write calls, and it > could be simplified a lot by implementing modify, setbits and clearbits > methods, like phy.h implements. Or maybe we can use phy_mmd_* methods > here instead of mv88e6390_serdes_read/write ? > > I fear such change will make future backporting of new fix commits > a pain. But I still think it should be done. What do you think? As you say, it make backporting hard. Are the simplifications worth it? And it is not limited to just SERDES code, the whole driver has bits of code doing read/modify/write, so if you are going to make such changes, it should be done everywhere. Andrew
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c index 3195936dc5be..b8241820679e 100644 --- a/drivers/net/dsa/mv88e6xxx/serdes.c +++ b/drivers/net/dsa/mv88e6xxx/serdes.c @@ -55,9 +55,20 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, { if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); + + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) { + if (state->link) { + state->speed = SPEED_2500; + state->duplex = DUPLEX_FULL; + } + + return 0; + } + + state->an_complete = 1; state->duplex = status & MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ? - DUPLEX_FULL : DUPLEX_HALF; + DUPLEX_FULL : DUPLEX_HALF; if (status & MV88E6390_SGMII_PHY_STATUS_TX_PAUSE) state->pause |= MLO_PAUSE_TX; @@ -66,10 +77,7 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, switch (status & MV88E6390_SGMII_PHY_STATUS_SPEED_MASK) { case MV88E6390_SGMII_PHY_STATUS_SPEED_1000: - if (state->interface == PHY_INTERFACE_MODE_2500BASEX) - state->speed = SPEED_2500; - else - state->speed = SPEED_1000; + state->speed = SPEED_1000; break; case MV88E6390_SGMII_PHY_STATUS_SPEED_100: state->speed = SPEED_100; @@ -85,10 +93,7 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, state->link = false; } - if (state->interface == PHY_INTERFACE_MODE_2500BASEX) - mii_lpa_mod_linkmode_x(state->lp_advertising, lpa, - ETHTOOL_LINK_MODE_2500baseX_Full_BIT); - else if (state->interface == PHY_INTERFACE_MODE_1000BASEX) + if (state->interface == PHY_INTERFACE_MODE_1000BASEX) mii_lpa_mod_linkmode_x(state->lp_advertising, lpa, ETHTOOL_LINK_MODE_1000baseX_Full_BIT); @@ -820,9 +825,10 @@ int mv88e6390_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port, break; case PHY_INTERFACE_MODE_2500BASEX: - adv = linkmode_adv_to_mii_adv_x(advertise, - ETHTOOL_LINK_MODE_2500baseX_Full_BIT); - break; + if (phylink_autoneg_inband(mode)) + dev_warn(chip->dev, + "inband AN unsupported on 2500base-x mode\n"); + return 0; default: return 0;
Commit a5a6858b793ff ("net: dsa: mv88e6xxx: extend phylink to Serdes PHYs") introduced method mv88e6390_serdes_pcs_config(), which is called (indirectly) by phylink to configure the PCS. This method enables inband AN if requested by phylink. It seems though that for 2500base-x mode some of these switches (at least Amethyst) do not support inband AN on Serdes. Moreover the above mentioned commit causes a regression when the Serdes of the switch is connected to a Marvell 88X3310 PHY in 2500base-x mode, because this PHY does not enable inband AN when it self-switches to 2500base-x (nor does it seem to work if manually enabling AN by writing the register), and it seems that 88E6390 devices won't link on 2500base-x mode if AN is enabled on the switch and disabled on the peer. Since it seems that 2500base-x mode has no definitive documentation of what exactly it is, and that Marvell devices disable inband AN when switching to this mode, this patch disables inband AN when configuring for 2500base-x in the mv88e6390_serdes_pcs_config() function and prints a warning if such mode is requested. mv88e6xxx_serdes_pcs_get_state is edited accordingly. We may need to refactor phylink code to not request inband AN for 2500base-x at all, but until then we need at least to fix this regression. Signed-off-by: Marek Behún <kabel@kernel.org> Fixes: a5a6858b793ff ("net: dsa: mv88e6xxx: extend phylink to Serdes PHYs") Cc: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/mv88e6xxx/serdes.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) base-commit: f50e2f9f791647aa4e5b19d0064f5cabf630bf6e