diff mbox series

[net,RFC] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode

Message ID 20211125150349.18789-1-kabel@kernel.org (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [net,RFC] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode | expand

Checks

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 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 warning 2 maintainers not CCed: olteanv@gmail.com f.fainelli@gmail.com
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 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, 22 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Marek Behún Nov. 25, 2021, 3:03 p.m. UTC
Commit 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings
in mac_config") removed forcing of speed and duplex from
mv88e6xxx_mac_config(), where the link is forced down, and left it only
in mv88e6xxx_mac_link_up(), by which time link is unforced.

It seems that in 2500base-x (at least on 88E6190), if the link is not
forced down, the forcing of new settings for speed and duplex doesn't
take.

Fix this by forcing link down.

Fixes: 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings in mac_config")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
The patch was tested on 6352, 6190, 6141 and 6393x, and should work
correctly on all models where SerDes is on separate registers from
port registers, but we don't know what would happen for on 6185, where
SerDes status is read from port registers.

I would like to get some comments on this fix before applying,
preferably from someone who can test this on 88E6185 on port which
supports 1000base-x / sgmii (port 7, 8 or 9).

Russell King says (from conversation on IRC):
  I'm still feeling uneasy about it, because forcing the link down in
  mac_link_up just feels wrong... it should already be down with the
  exception of an in-band link... but I'm also wondering if we're in
  2500base-x mode, shouldn't the hardware already be operating in 2.5G

  I'm wondering if the exception for in-band links needs to be
  conditional upon forcing it down having an effect on the PCS status

  looking at the 6352, which has the port controls separate from the
  serdes controls, and we use the serdes as the PCS, it looks like
  forcing the link down

  but on 6185, we don't have a separate serdes, and we rely on the port
  registers

  ...

  I think probably the best thing is to post it, ask for testing for
  the 6185 where one of the 7,8,9 ports is using 1000base-X

  if no one's using such a configuration... then we're not breaking
  anything
---
 drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7ed420128cea..ebeb8500c3f8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -785,12 +785,17 @@  static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 	if ((!mv88e6xxx_phy_is_internal(ds, port) &&
 	     !mv88e6xxx_port_ppu_updates(chip, port)) ||
 	    mode == MLO_AN_FIXED) {
-		/* FIXME: for an automedia port, should we force the link
-		 * down here - what if the link comes up due to "other" media
-		 * while we're bringing the port up, how is the exclusivity
-		 * handled in the Marvell hardware? E.g. port 2 on 88E6390
-		 * shared between internal PHY and Serdes.
+		/* FIXME: we need to force the link down here, otherwise the
+		 * forcing of link speed and duplex by .port_set_speed_duplex()
+		 * doesn't work for some modes.
+		 * But what if the link comes up due to "other" media while
+		 * we're bringing the port up, how is the exclusivity handled in
+		 * the Marvell hardware? E.g. port 2 on 88E6390 shared between
+		 * internal PHY and Serdes.
 		 */
+		if (ops->port_sync_link)
+			err = ops->port_sync_link(chip, port, mode, false);
+
 		err = mv88e6xxx_serdes_pcs_link_up(chip, port, mode, speed,
 						   duplex);
 		if (err)