[PATCH net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[PATCH net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[Tobias Waldekranz]

This series provides a set of bug fixes discovered while bringing up a
new board using mv88e6393x chips.

1/4 adds logging of low-level I/O errors that where previously only
logged at a much higher layer, e.g. "probe failed" or "failed to add
VLAN", at which time the origin of the error was long gone. Not
exactly a bugfix, though still suitable for -net IMHO; but I'm also
happy to send it via net-next instead if that makes more sense.

2/4 fixes an issue I've never seen on any other board. At first I
assumed that there was some board-specific issue, but we've not been
able to find one. If you give the chip enough time, it will eventually
signal "PPU Polling" and everything else will work as
expected. Therefore I assume that all is in order, and that we simply
need to increase the timeout.

3/4 just broadens Chris' original fix to apply to all chips. Though I
have obviously not tested this on every supported device, I can't see
how this could possibly be chip specific. Was there some specific
reason for originally limiting the set of chips that this applied to?

4/4 can only be supported on the Amethyst, which can control the
ieee-multicast policy per-port, rather than via a global setting as
it's done on the older families.

Tobias Waldekranz (4):
  net: dsa: mv88e6xxx: Improve I/O related error logging
  net: dsa: mv88e6xxx: Give chips more time to activate their PPUs
  net: dsa: mv88e6xxx: Never force link on in-band managed MACs
  net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

 drivers/net/dsa/mv88e6xxx/chip.c    | 92 ++++++++++++++++-------------
 drivers/net/dsa/mv88e6xxx/chip.h    |  6 +-
 drivers/net/dsa/mv88e6xxx/global1.c | 19 +++++-
 drivers/net/dsa/mv88e6xxx/port.c    | 48 +++++++--------
 drivers/net/dsa/mv88e6xxx/port.h    |  1 -
 5 files changed, 97 insertions(+), 69 deletions(-)

-- 
2.43.0

[PATCH net 1/4] net: dsa: mv88e6xxx: Improve I/O related error logging

[Tobias Waldekranz]

In the rare event of an I/O error - e.g. a broken bus controller,
frozen chip, etc. - make sure to log all available information, so
that there is some hope of determining _where_ the error; not just
_that_ an error occurred.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 57 +++++++++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 3a792f79270d..16fc9a21dc59 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -59,8 +59,11 @@ int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val)
 	assert_reg_lock(chip);
 
 	err = mv88e6xxx_smi_read(chip, addr, reg, val);
-	if (err)
+	if (err) {
+		dev_err_ratelimited(chip->dev, "Failed to read from 0x%02x/0x%02x: %d",
+				    addr, reg, err);
 		return err;
+	}
 
 	dev_dbg(chip->dev, "<- addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
 		addr, reg, *val);
@@ -75,17 +78,19 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
 	assert_reg_lock(chip);
 
 	err = mv88e6xxx_smi_write(chip, addr, reg, val);
-	if (err)
+	if (err) {
+		dev_err_ratelimited(chip->dev, "Failed to write 0x%04x to 0x%02x/0x%02x: %d",
+				    val, addr, reg, err);
 		return err;
-
+	}
 	dev_dbg(chip->dev, "-> addr: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
 		addr, reg, val);
 
 	return 0;
 }
 
-int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
-			u16 mask, u16 val)
+static int _mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
+				u16 mask, u16 val, u16 *last)
 {
 	const unsigned long timeout = jiffies + msecs_to_jiffies(50);
 	u16 data;
@@ -117,15 +122,51 @@ int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
 	if ((data & mask) == val)
 		return 0;
 
-	dev_err(chip->dev, "Timeout while waiting for switch\n");
+	if (last)
+		*last = data;
+
 	return -ETIMEDOUT;
 }
 
+int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
+			u16 mask, u16 val)
+{
+	u16 last;
+	int err;
+
+	err = _mv88e6xxx_wait_mask(chip, addr, reg, mask, val, &last);
+	if (!err)
+		return 0;
+
+	dev_err(chip->dev,
+		"%s waiting for 0x%02x/0x%02x to match 0x%04x (mask:0x%04x last:0x%04x)\n",
+		(err == -ETIMEDOUT) ? "Timed out" : "Failed",
+		addr, reg, val, mask, last);
+	return err;
+}
+
+static int _mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
+			       int bit, int val, u16 *last)
+{
+	return _mv88e6xxx_wait_mask(chip, addr, reg, BIT(bit),
+				   val ? BIT(bit) : 0x0000, last);
+}
+
 int mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
 		       int bit, int val)
 {
-	return mv88e6xxx_wait_mask(chip, addr, reg, BIT(bit),
-				   val ? BIT(bit) : 0x0000);
+	u16 last;
+	int err;
+
+	err = _mv88e6xxx_wait_bit(chip, addr, reg, bit, val, &last);
+	if (!err)
+		return 0;
+
+	dev_err(chip->dev,
+		"%s waiting for bit %d in 0x%02x/0x%02x to %s (last:0x%04x)\n",
+		(err == -ETIMEDOUT) ? "Timed out" : "Failed",
+		bit, addr, reg, val ? "set" : "clear", last);
+	return err;
 }
 
 struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
-- 
2.43.0

[PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

In a daisy-chain of three 6393X devices, delays of up to 750ms are
sometimes observed before completion of PPU initialization (Global 1,
register 0, bit 15) is signaled. Therefore, allow chips more time
before giving up.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |  4 ++--
 drivers/net/dsa/mv88e6xxx/chip.h    |  2 ++
 drivers/net/dsa/mv88e6xxx/global1.c | 19 ++++++++++++++++++-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 16fc9a21dc59..20cd25fb4b75 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -145,8 +145,8 @@ int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
 	return err;
 }
 
-static int _mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
-			       int bit, int val, u16 *last)
+int _mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
+			int bit, int val, u16 *last)
 {
 	return _mv88e6xxx_wait_mask(chip, addr, reg, BIT(bit),
 				   val ? BIT(bit) : 0x0000, last);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 9fe8e8a7856b..dfdb0380e664 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -824,6 +824,8 @@ int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val);
 int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val);
 int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg,
 			u16 mask, u16 val);
+int _mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
+			int bit, int val, u16 *last);
 int mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
 		       int bit, int val);
 struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip);
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index 9820cd596757..27e98e729563 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -60,8 +60,25 @@ static int mv88e6185_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
 static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
 {
 	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
+	int err, i;
 
-	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
+	for (i = 0; i < 20; i++) {
+		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
+					  MV88E6XXX_G1_STS, bit, 1, NULL);
+		if (err != -ETIMEDOUT)
+			break;
+	}
+
+	if (err) {
+		dev_err(chip->dev, "PPU did not come online: %d\n", err);
+		return err;
+	}
+
+	if (i)
+		dev_warn(chip->dev,
+			 "PPU was slow to come online, retried %d times\n", i);
+
+	return 0;
 }
 
 static int mv88e6xxx_g1_wait_init_ready(struct mv88e6xxx_chip *chip)
-- 
2.43.0

[PATCH net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz]

NOTE: This issue was addressed in the referenced commit, but a
conservative approach was chosen, where only 6095, 6097 and 6185 got
the fix.

Before the referenced commit, in the following setup, when the PHY
detected loss of link on the MDI, mv88e6xxx would force the MAC
down. If the MDI-side link was then re-established later on, there was
no longer any MII link over which the PHY could communicate that
information back to the MAC.

        .-SGMII/USXGMII
        |
.-----. v .-----.   .--------------.
| MAC +---+ PHY +---+ MDI (Cu/SFP) |
'-----'   '-----'   '--------------'

Since this a generic problem on all MACs connected to a SERDES - which
is the only time when in-band-status is used - move all chips to a
common mv88e6xxx_port_sync_link() implementation which avoids forcing
links on _all_ in-band managed ports.

Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 35 +++-----------------------------
 drivers/net/dsa/mv88e6xxx/chip.h |  4 ----
 drivers/net/dsa/mv88e6xxx/port.c | 17 ----------------
 drivers/net/dsa/mv88e6xxx/port.h |  1 -
 4 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 20cd25fb4b75..13a97e6314ed 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1013,8 +1013,8 @@ static void mv88e6xxx_mac_link_down(struct phylink_config *config,
 	 * updated by the switch or if we are using fixed-link mode.
 	 */
 	if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
-	     mode == MLO_AN_FIXED) && ops->port_sync_link)
-		err = ops->port_sync_link(chip, port, mode, false);
+	     mode == MLO_AN_FIXED))
+		err = mv88e6xxx_port_sync_link(chip, port, mode, false);
 
 	if (!err && ops->port_set_speed_duplex)
 		err = ops->port_set_speed_duplex(chip, port, SPEED_UNFORCED,
@@ -1054,8 +1054,7 @@ static void mv88e6xxx_mac_link_up(struct phylink_config *config,
 				goto error;
 		}
 
-		if (ops->port_sync_link)
-			err = ops->port_sync_link(chip, port, mode, true);
+		err = mv88e6xxx_port_sync_link(chip, port, mode, true);
 	}
 error:
 	mv88e6xxx_reg_unlock(chip);
@@ -4219,7 +4218,6 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4263,7 +4261,6 @@ static const struct mv88e6xxx_ops mv88e6095_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6185_port_set_forward_unknown,
@@ -4298,7 +4295,6 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4345,7 +4341,6 @@ static const struct mv88e6xxx_ops mv88e6123_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6352_port_set_ucast_flood,
@@ -4383,7 +4378,6 @@ static const struct mv88e6xxx_ops mv88e6131_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_frame_mode = mv88e6351_port_set_frame_mode,
@@ -4428,7 +4422,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6341_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6341_port_max_speed_mode,
@@ -4489,7 +4482,6 @@ static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
 	.port_set_policy = mv88e6352_port_set_policy,
@@ -4535,7 +4527,6 @@ static const struct mv88e6xxx_ops mv88e6165_ops = {
 	.phy_read = mv88e6165_phy_read,
 	.phy_write = mv88e6165_phy_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
 	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
@@ -4574,7 +4565,6 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4622,7 +4612,6 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4677,7 +4666,6 @@ static const struct mv88e6xxx_ops mv88e6175_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4725,7 +4713,6 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -4779,7 +4766,6 @@ static const struct mv88e6xxx_ops mv88e6185_ops = {
 	.phy_read = mv88e6185_phy_ppu_read,
 	.phy_write = mv88e6185_phy_ppu_write,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_set_frame_mode = mv88e6085_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6185_port_set_forward_unknown,
@@ -4821,7 +4807,6 @@ static const struct mv88e6xxx_ops mv88e6190_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -4881,7 +4866,6 @@ static const struct mv88e6xxx_ops mv88e6190x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390x_port_max_speed_mode,
@@ -4941,7 +4925,6 @@ static const struct mv88e6xxx_ops mv88e6191_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -5001,7 +4984,6 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5062,7 +5044,6 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6250_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5106,7 +5087,6 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -5168,7 +5148,6 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6320_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5217,7 +5196,6 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6320_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5265,7 +5243,6 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6341_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6341_port_max_speed_mode,
@@ -5328,7 +5305,6 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5374,7 +5350,6 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5424,7 +5399,6 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6352_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
@@ -5487,7 +5461,6 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390_port_max_speed_mode,
@@ -5551,7 +5524,6 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6390x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6390x_port_max_speed_mode,
@@ -5614,7 +5586,6 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
 	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
 	.port_set_link = mv88e6xxx_port_set_link,
-	.port_sync_link = mv88e6xxx_port_sync_link,
 	.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
 	.port_set_speed_duplex = mv88e6393x_port_set_speed_duplex,
 	.port_max_speed_mode = mv88e6393x_port_max_speed_mode,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index dfdb0380e664..23a9466aa01d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -525,10 +525,6 @@ struct mv88e6xxx_ops {
 	 */
 	int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
 
-	/* Synchronise the port link state with that of the SERDES
-	 */
-	int (*port_sync_link)(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
-
 #define PAUSE_ON		1
 #define PAUSE_OFF		0
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index dc777ddce1f3..56ed2f57fef8 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -187,23 +187,6 @@ int mv88e6xxx_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int
 	int err = 0;
 	int link;
 
-	if (isup)
-		link = LINK_FORCED_UP;
-	else
-		link = LINK_FORCED_DOWN;
-
-	if (ops->port_set_link)
-		err = ops->port_set_link(chip, port, link);
-
-	return err;
-}
-
-int mv88e6185_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup)
-{
-	const struct mv88e6xxx_ops *ops = chip->info->ops;
-	int err = 0;
-	int link;
-
 	if (mode == MLO_AN_INBAND)
 		link = LINK_UNFORCED;
 	else if (isup)
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index c1d2f99efb1c..26452e0a8448 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -484,7 +484,6 @@ int mv88e6390_port_set_rgmii_delay(struct mv88e6xxx_chip *chip, int port,
 int mv88e6xxx_port_set_link(struct mv88e6xxx_chip *chip, int port, int link);
 
 int mv88e6xxx_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
-int mv88e6185_port_sync_link(struct mv88e6xxx_chip *chip, int port, unsigned int mode, bool isup);
 
 int mv88e6185_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
 				    int speed, int duplex);
-- 
2.43.0

[PATCH net 4/4] net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X

[Tobias Waldekranz]

For packets with a DA in the IEEE reserved L2 group range, originating
from a CPU, forward it as normal, rather than classifying it as
management.

Example use-case:

     bridge (group_fwd_mask 0x4000)
     / |  \
 swp1 swp2 tap0
   \   /
(mv88e6xxx)

We've created a bridge with a non-zero group_fwd_mask (allowing LLDP
in this example) containing a set of ports managed by mv88e6xxx and
some foreign interface (e.g. an L2 VPN tunnel).

Since an LLDP packet coming in to the bridge from the other side of
tap0 is eligable for tx forward offloading, a FORWARD frame destined
for swp1 and swp2 would be send to the conduit interface.

Before this change, due to rsvd2cpu being enabled on the CPU port, the
switch would try to trap it back to the CPU. Given that the CPU is
trusted, instead assume that it indeed meant for the packet to be
forwarded like any other.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/port.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 56ed2f57fef8..bf6d558c112c 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1416,6 +1416,23 @@ static int mv88e6393x_port_policy_write_all(struct mv88e6xxx_chip *chip,
 	return 0;
 }
 
+static int mv88e6393x_port_policy_write_user(struct mv88e6xxx_chip *chip,
+					     u16 pointer, u8 data)
+{
+	int err, port;
+
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		if (!dsa_is_user_port(chip->ds, port))
+			continue;
+
+		err = mv88e6393x_port_policy_write(chip, port, pointer, data);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 int mv88e6393x_set_egress_port(struct mv88e6xxx_chip *chip,
 			       enum mv88e6xxx_egress_direction direction,
 			       int port)
@@ -1457,26 +1474,28 @@ int mv88e6393x_port_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip)
 	int err;
 
 	/* Consider the frames with reserved multicast destination
-	 * addresses matching 01:80:c2:00:00:00 and
-	 * 01:80:c2:00:00:02 as MGMT.
+	 * addresses matching 01:80:c2:00:00:00 and 01:80:c2:00:00:02
+	 * as MGMT when received on user ports. Forward as normal on
+	 * CPU/DSA ports, to support bridges with non-zero
+	 * group_fwd_masks.
 	 */
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XLO;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XHI;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XLO;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
 	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XHI;
-	err = mv88e6393x_port_policy_write_all(chip, ptr, 0xff);
+	err = mv88e6393x_port_policy_write_user(chip, ptr, 0xff);
 	if (err)
 		return err;
 
-- 
2.43.0

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Andrew Lunn]

On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
> In a daisy-chain of three 6393X devices, delays of up to 750ms are
> sometimes observed before completion of PPU initialization (Global 1,
> register 0, bit 15) is signaled. Therefore, allow chips more time
> before giving up.
>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>  {
>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
> +	int err, i;
>  
> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
> +	for (i = 0; i < 20; i++) {
> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
> +		if (err != -ETIMEDOUT)
> +			break;
> +	}

The commit message does not indicate why it is necessary to swap to
_mv88e6xxx_wait_bit().

> +
> +	if (err) {
> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
> +		return err;
> +	}
> +
> +	if (i)
> +		dev_warn(chip->dev,
> +			 "PPU was slow to come online, retried %d times\n", i);

dev_dbg()? Does the user care if it took longer than one loop
iteration?

	Andrew

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>> In a daisy-chain of three 6393X devices, delays of up to 750ms are
>> sometimes observed before completion of PPU initialization (Global 1,
>> register 0, bit 15) is signaled. Therefore, allow chips more time
>> before giving up.
>>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>>  {
>>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
>> +	int err, i;
>>  
>> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
>> +	for (i = 0; i < 20; i++) {
>> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
>> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
>> +		if (err != -ETIMEDOUT)
>> +			break;
>> +	}
>
> The commit message does not indicate why it is necessary to swap to
> _mv88e6xxx_wait_bit().

It is not strictly necessary, I just wanted to avoid flooding the logs
with spurious timeout errors. Do you want me to update the message?

>> +
>> +	if (err) {
>> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	if (i)
>> +		dev_warn(chip->dev,
>> +			 "PPU was slow to come online, retried %d times\n", i);
>
> dev_dbg()? Does the user care if it took longer than one loop
> iteration?

My resoning was: While it does seem fine that the device takes this long
to initialize, if it turns out that this is an indication of some bigger
issue, it might be good to have it recorded in the log.

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Andrew Lunn]

On Fri, Dec 06, 2024 at 02:39:25PM +0100, Tobias Waldekranz wrote:
> On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
> >> In a daisy-chain of three 6393X devices, delays of up to 750ms are
> >> sometimes observed before completion of PPU initialization (Global 1,
> >> register 0, bit 15) is signaled. Therefore, allow chips more time
> >> before giving up.
> >>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
> >>  {
> >>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
> >> +	int err, i;
> >>  
> >> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
> >> +	for (i = 0; i < 20; i++) {
> >> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
> >> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
> >> +		if (err != -ETIMEDOUT)
> >> +			break;
> >> +	}
> >
> > The commit message does not indicate why it is necessary to swap to
> > _mv88e6xxx_wait_bit().
> 
> It is not strictly necessary, I just wanted to avoid flooding the logs
> with spurious timeout errors. Do you want me to update the message?

Ah, the previous patch.

I wounder if the simpler fix is just to increase the timeout? I don't
think we have any code specifically wanting a timeout, so changing the
timeout should have no real effect.

	Andrew

Re: [PATCH net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[Chris Packham]

Hi Tobias,

On 07/12/2024 02:07, Tobias Waldekranz wrote:
> This series provides a set of bug fixes discovered while bringing up a
> new board using mv88e6393x chips.
>
> 1/4 adds logging of low-level I/O errors that where previously only
> logged at a much higher layer, e.g. "probe failed" or "failed to add
> VLAN", at which time the origin of the error was long gone. Not
> exactly a bugfix, though still suitable for -net IMHO; but I'm also
> happy to send it via net-next instead if that makes more sense.
>
> 2/4 fixes an issue I've never seen on any other board. At first I
> assumed that there was some board-specific issue, but we've not been
> able to find one. If you give the chip enough time, it will eventually
> signal "PPU Polling" and everything else will work as
> expected. Therefore I assume that all is in order, and that we simply
> need to increase the timeout.
>
> 3/4 just broadens Chris' original fix to apply to all chips. Though I
> have obviously not tested this on every supported device, I can't see
> how this could possibly be chip specific. Was there some specific
> reason for originally limiting the set of chips that this applied to?

I think it was mainly because I didn't have a 88e639xx to test with 
(much like you) so I kept the change isolated to the hardware I did have 
access to.

The original thread that kicked the original series off was 
https://lore.kernel.org/netdev/72e8e25a-db0d-275f-e80e-0b74bf112832@alliedtelesis.co.nz/

Since the only difference is the mode == MLO_AN_INBAND check I think 
your change is reasonably safe.

>
> 4/4 can only be supported on the Amethyst, which can control the
> ieee-multicast policy per-port, rather than via a global setting as
> it's done on the older families.
>
> Tobias Waldekranz (4):
>    net: dsa: mv88e6xxx: Improve I/O related error logging
>    net: dsa: mv88e6xxx: Give chips more time to activate their PPUs
>    net: dsa: mv88e6xxx: Never force link on in-band managed MACs
>    net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X
>
>   drivers/net/dsa/mv88e6xxx/chip.c    | 92 ++++++++++++++++-------------
>   drivers/net/dsa/mv88e6xxx/chip.h    |  6 +-
>   drivers/net/dsa/mv88e6xxx/global1.c | 19 +++++-
>   drivers/net/dsa/mv88e6xxx/port.c    | 48 +++++++--------
>   drivers/net/dsa/mv88e6xxx/port.h    |  1 -
>   5 files changed, 97 insertions(+), 69 deletions(-)
>

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

On lör, dec 07, 2024 at 16:38, Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Dec 06, 2024 at 02:39:25PM +0100, Tobias Waldekranz wrote:
>> On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
>> > On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>> >> In a daisy-chain of three 6393X devices, delays of up to 750ms are
>> >> sometimes observed before completion of PPU initialization (Global 1,
>> >> register 0, bit 15) is signaled. Therefore, allow chips more time
>> >> before giving up.
>> >>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>> >>  {
>> >>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
>> >> +	int err, i;
>> >>  
>> >> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
>> >> +	for (i = 0; i < 20; i++) {
>> >> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
>> >> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
>> >> +		if (err != -ETIMEDOUT)
>> >> +			break;
>> >> +	}
>> >
>> > The commit message does not indicate why it is necessary to swap to
>> > _mv88e6xxx_wait_bit().
>> 
>> It is not strictly necessary, I just wanted to avoid flooding the logs
>> with spurious timeout errors. Do you want me to update the message?
>
> Ah, the previous patch.
>
> I wounder if the simpler fix is just to increase the timeout? I don't

It would certainly be simpler. To me, it just felt a bit dangerous to
have a static 1s timeout buried that deep in the stack.

> think we have any code specifically wanting a timeout, so changing the
> timeout should have no real effect.

I imagine some teardown scenario, in which we typically ignore return
values. In that case, if we're trying to remove lots of objects from
hardware that require waiting on busy bits (ATU/VTU), we could end up
blocking for minutes rather than seconds.

But it is definitely more of a gut feeling - I don't have a concrete
example.

Re: [PATCH net 0/4] net: dsa: mv88e6xxx: Amethyst (6393X) fixes

[Tobias Waldekranz]

On mån, dec 09, 2024 at 09:23, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> Hi Tobias,
>
> On 07/12/2024 02:07, Tobias Waldekranz wrote:
>> This series provides a set of bug fixes discovered while bringing up a
>> new board using mv88e6393x chips.
>>
>> 1/4 adds logging of low-level I/O errors that where previously only
>> logged at a much higher layer, e.g. "probe failed" or "failed to add
>> VLAN", at which time the origin of the error was long gone. Not
>> exactly a bugfix, though still suitable for -net IMHO; but I'm also
>> happy to send it via net-next instead if that makes more sense.
>>
>> 2/4 fixes an issue I've never seen on any other board. At first I
>> assumed that there was some board-specific issue, but we've not been
>> able to find one. If you give the chip enough time, it will eventually
>> signal "PPU Polling" and everything else will work as
>> expected. Therefore I assume that all is in order, and that we simply
>> need to increase the timeout.
>>
>> 3/4 just broadens Chris' original fix to apply to all chips. Though I
>> have obviously not tested this on every supported device, I can't see
>> how this could possibly be chip specific. Was there some specific
>> reason for originally limiting the set of chips that this applied to?
>
> I think it was mainly because I didn't have a 88e639xx to test with 
> (much like you) so I kept the change isolated to the hardware I did have 
> access to.
>
> The original thread that kicked the original series off was 
> https://lore.kernel.org/netdev/72e8e25a-db0d-275f-e80e-0b74bf112832@alliedtelesis.co.nz/
>
> Since the only difference is the mode == MLO_AN_INBAND check I think 
> your change is reasonably safe.

Yeah exactly; and since that only applies when the user has explicitly
stated "the PHY will communicate the link information in-band", then I
don't see how forcing the link state could ever be the right thing to
do.

Thanks for providing the background!

>>
>> 4/4 can only be supported on the Amethyst, which can control the
>> ieee-multicast policy per-port, rather than via a global setting as
>> it's done on the older families.
>>
>> Tobias Waldekranz (4):
>>    net: dsa: mv88e6xxx: Improve I/O related error logging
>>    net: dsa: mv88e6xxx: Give chips more time to activate their PPUs
>>    net: dsa: mv88e6xxx: Never force link on in-band managed MACs
>>    net: dsa: mv88e6xxx: Limit rsvd2cpu policy to user ports on 6393X
>>
>>   drivers/net/dsa/mv88e6xxx/chip.c    | 92 ++++++++++++++++-------------
>>   drivers/net/dsa/mv88e6xxx/chip.h    |  6 +-
>>   drivers/net/dsa/mv88e6xxx/global1.c | 19 +++++-
>>   drivers/net/dsa/mv88e6xxx/port.c    | 48 +++++++--------
>>   drivers/net/dsa/mv88e6xxx/port.h    |  1 -
>>   5 files changed, 97 insertions(+), 69 deletions(-)
>>

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Paolo Abeni]

On 12/6/24 14:39, Tobias Waldekranz wrote:
> On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
>> On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>>> +
>>> +	if (err) {
>>> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
>>> +		return err;
>>> +	}
>>> +
>>> +	if (i)
>>> +		dev_warn(chip->dev,
>>> +			 "PPU was slow to come online, retried %d times\n", i);
>>
>> dev_dbg()? Does the user care if it took longer than one loop
>> iteration?
> 
> My resoning was: While it does seem fine that the device takes this long
> to initialize, if it turns out that this is an indication of some bigger
> issue, it might be good to have it recorded in the log.

What about dev_info()? Warn in the log message tend to be interpreted in
pretty drastic ways.

Thanks,

Paolo

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

On tis, dec 10, 2024 at 13:10, Paolo Abeni <pabeni@redhat.com> wrote:
> On 12/6/24 14:39, Tobias Waldekranz wrote:
>> On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
>>> On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>>>> +
>>>> +	if (err) {
>>>> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	if (i)
>>>> +		dev_warn(chip->dev,
>>>> +			 "PPU was slow to come online, retried %d times\n", i);
>>>
>>> dev_dbg()? Does the user care if it took longer than one loop
>>> iteration?
>> 
>> My resoning was: While it does seem fine that the device takes this long
>> to initialize, if it turns out that this is an indication of some bigger
>> issue, it might be good to have it recorded in the log.
>
> What about dev_info()? Warn in the log message tend to be interpreted in
> pretty drastic ways.

Sure, that seems fair. I'll lower it in v2.

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Tobias Waldekranz]

On sön, dec 08, 2024 at 22:23, Tobias Waldekranz <tobias@waldekranz.com> wrote:
> On lör, dec 07, 2024 at 16:38, Andrew Lunn <andrew@lunn.ch> wrote:
>> On Fri, Dec 06, 2024 at 02:39:25PM +0100, Tobias Waldekranz wrote:
>>> On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
>>> > On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>>> >> In a daisy-chain of three 6393X devices, delays of up to 750ms are
>>> >> sometimes observed before completion of PPU initialization (Global 1,
>>> >> register 0, bit 15) is signaled. Therefore, allow chips more time
>>> >> before giving up.
>>> >>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>>> >>  {
>>> >>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
>>> >> +	int err, i;
>>> >>  
>>> >> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
>>> >> +	for (i = 0; i < 20; i++) {
>>> >> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
>>> >> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
>>> >> +		if (err != -ETIMEDOUT)
>>> >> +			break;
>>> >> +	}
>>> >
>>> > The commit message does not indicate why it is necessary to swap to
>>> > _mv88e6xxx_wait_bit().
>>> 
>>> It is not strictly necessary, I just wanted to avoid flooding the logs
>>> with spurious timeout errors. Do you want me to update the message?
>>
>> Ah, the previous patch.
>>
>> I wounder if the simpler fix is just to increase the timeout? I don't
>
> It would certainly be simpler. To me, it just felt a bit dangerous to
> have a static 1s timeout buried that deep in the stack.
>
>> think we have any code specifically wanting a timeout, so changing the
>> timeout should have no real effect.
>
> I imagine some teardown scenario, in which we typically ignore return
> values. In that case, if we're trying to remove lots of objects from
> hardware that require waiting on busy bits (ATU/VTU), we could end up
> blocking for minutes rather than seconds.
>
> But it is definitely more of a gut feeling - I don't have a concrete
> example.

Andrew, have you had a chance to mull this over?

If you want to go with a global timeout then let's do that, but either
way I would really like to move this series forward.

Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

[Andrew Lunn]

> Andrew, have you had a chance to mull this over?
> 
> If you want to go with a global timeout then let's do that, but either
> way I would really like to move this series forward.

Please increase the global timeout.

       Andrew