[PATCH net 0/6 RESEND] net: dsa: b53: PCS fixes

[PATCH net 0/6 RESEND] net: dsa: b53: PCS fixes

[Florian Fainelli]

This patch series fixes a number of issues found with a LLM and while
testing on a Broadcom Northstar Plus reference board (BCM958625HR) using
a SFP connected to a fiber converter:

- there was no serialization of the registers across multiple lanes
- the SerDes powering down was incorrectly managed and should have used
  .pcs_{enable,disable}
- the interfaces advertisement should have been confined to successfully
  initialized PCSes
- the negotation and forced link parameters were incorrectly supported
  and should have used pcs_link_up and pcs_inband_caps
- link state should have been reported through the PCS

With these patches disconnecting the fiber cables triggers a link down,
as does disconnecting the fiber converter from its cable or powering it
down, ip link up/down also work properly as a result as well.

Florian Fainelli (6):
  net: dsa: b53: serialize SerDes register transactions
  net: dsa: b53: manage SerDes power through the PCS callbacks
  net: dsa: b53: constrain SerDes interfaces to initialized PCSes
  net: dsa: b53: configure PCS negotiation and forced link parameters
  net: dsa: b53: report link state through the PCS
  net: dsa: b53: apply resolved link settings to serial port MACs

 drivers/net/dsa/b53/b53_common.c |  69 ++----
 drivers/net/dsa/b53/b53_priv.h   |   4 -
 drivers/net/dsa/b53/b53_serdes.c | 407 +++++++++++++++++++++++--------
 drivers/net/dsa/b53/b53_serdes.h |  13 +-
 drivers/net/dsa/b53/b53_srab.c   |  12 +-
 5 files changed, 345 insertions(+), 160 deletions(-)

-- 
2.53.0

[PATCH net 1/6 RESEND] net: dsa: b53: serialize SerDes register transactions

[Florian Fainelli]

The lane and block selectors are shared by both SerDes lanes. Taking
reg_mutex separately for each register access allows another port to
change either selector before the data access. Hold the mutex across the
complete transaction and use the raw bus operations while it is held.

Propagate bus errors instead of returning uninitialized register contents,
only cache a successfully selected lane, and invalidate the cache after
failed accesses. Make the PCS callbacks fail configuration or report link
down on read errors, and reject out-of-range lane indices at discovery.

Fixes: 0e01491de646 ("net: dsa: b53: Add SerDes support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c |   1 +
 drivers/net/dsa/b53/b53_serdes.c | 148 ++++++++++++++++++++++++-------
 2 files changed, 115 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 0880310c9ce3..3f8f956aab82 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -3203,6 +3203,7 @@ struct b53_device *b53_switch_alloc(struct device *base,
 	dev->ds = ds;
 	dev->priv = priv;
 	dev->ops = ops;
+	dev->serdes_lane = B53_INVALID_LANE;
 	ds->ops = &b53_switch_ops;
 	ds->phylink_mac_ops = &b53_phylink_mac_ops;
 	dev->vlan_enabled = true;
diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index 7460122f6abc..c90894877626 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -22,47 +22,96 @@ static inline struct b53_pcs *pcs_to_b53_pcs(struct phylink_pcs *pcs)
 	return container_of(pcs, struct b53_pcs, pcs);
 }
 
-static void b53_serdes_write_blk(struct b53_device *dev, u8 offset, u16 block,
-				 u16 value)
+/* The caller holds reg_mutex across lane, block and data accesses. */
+static int b53_serdes_write_blk(struct b53_device *dev, u8 offset, u16 block,
+				u16 value)
 {
-	b53_write16(dev, B53_SERDES_PAGE, B53_SERDES_BLKADDR, block);
-	b53_write16(dev, B53_SERDES_PAGE, offset, value);
+	int ret;
+
+	lockdep_assert_held(&dev->reg_mutex);
+
+	ret = dev->ops->write16(dev, B53_SERDES_PAGE, B53_SERDES_BLKADDR,
+				block);
+	if (ret)
+		return ret;
+
+	return dev->ops->write16(dev, B53_SERDES_PAGE, offset, value);
 }
 
-static u16 b53_serdes_read_blk(struct b53_device *dev, u8 offset, u16 block)
+static int b53_serdes_read_blk(struct b53_device *dev, u8 offset, u16 block)
 {
 	u16 value;
+	int ret;
+
+	lockdep_assert_held(&dev->reg_mutex);
 
-	b53_write16(dev, B53_SERDES_PAGE, B53_SERDES_BLKADDR, block);
-	b53_read16(dev, B53_SERDES_PAGE, offset, &value);
+	ret = dev->ops->write16(dev, B53_SERDES_PAGE, B53_SERDES_BLKADDR,
+				block);
+	if (ret)
+		return ret;
 
-	return value;
+	ret = dev->ops->read16(dev, B53_SERDES_PAGE, offset, &value);
+	return ret ? ret : value;
 }
 
-static void b53_serdes_set_lane(struct b53_device *dev, u8 lane)
+static int b53_serdes_set_lane(struct b53_device *dev, u8 lane)
 {
+	int ret;
+
+	lockdep_assert_held(&dev->reg_mutex);
+
+	if (lane >= B53_N_PCS)
+		return -EINVAL;
+
 	if (dev->serdes_lane == lane)
-		return;
+		return 0;
 
-	WARN_ON(lane > 1);
+	ret = b53_serdes_write_blk(dev, B53_SERDES_LANE,
+				   SERDES_XGXSBLK0_BLOCKADDRESS, lane);
+	if (ret)
+		return ret;
 
-	b53_serdes_write_blk(dev, B53_SERDES_LANE,
-			     SERDES_XGXSBLK0_BLOCKADDRESS, lane);
 	dev->serdes_lane = lane;
+	return 0;
 }
 
-static void b53_serdes_write(struct b53_device *dev, u8 lane,
-			     u8 offset, u16 block, u16 value)
+static int b53_serdes_write(struct b53_device *dev, u8 lane,
+			    u8 offset, u16 block, u16 value)
 {
-	b53_serdes_set_lane(dev, lane);
-	b53_serdes_write_blk(dev, offset, block, value);
+	int ret;
+
+	mutex_lock(&dev->reg_mutex);
+	ret = b53_serdes_set_lane(dev, lane);
+	if (!ret)
+		ret = b53_serdes_write_blk(dev, offset, block, value);
+	if (ret)
+		dev->serdes_lane = B53_INVALID_LANE;
+	mutex_unlock(&dev->reg_mutex);
+
+	return ret;
 }
 
-static u16 b53_serdes_read(struct b53_device *dev, u8 lane,
+static int b53_serdes_read(struct b53_device *dev, u8 lane,
 			   u8 offset, u16 block)
 {
-	b53_serdes_set_lane(dev, lane);
-	return b53_serdes_read_blk(dev, offset, block);
+	int ret;
+
+	mutex_lock(&dev->reg_mutex);
+	ret = b53_serdes_set_lane(dev, lane);
+	if (!ret)
+		ret = b53_serdes_read_blk(dev, offset, block);
+	if (ret < 0)
+		dev->serdes_lane = B53_INVALID_LANE;
+	mutex_unlock(&dev->reg_mutex);
+
+	return ret;
+}
+
+static void b53_serdes_error(struct b53_device *dev, u8 lane, int ret)
+{
+	if (ret < 0)
+		dev_err_ratelimited(dev->dev, "SerDes lane %u access failed: %pe\n",
+				    lane, ERR_PTR(ret));
 }
 
 static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
@@ -72,31 +121,37 @@ static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 {
 	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
 	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	u16 reg;
+	int reg;
 
 	reg = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
 			      SERDES_DIGITAL_BLK);
+	if (reg < 0)
+		return reg;
+
 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
 		reg |= FIBER_MODE_1000X;
 	else
 		reg &= ~FIBER_MODE_1000X;
-	b53_serdes_write(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
-			 SERDES_DIGITAL_BLK, reg);
-
-	return 0;
+	return b53_serdes_write(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
+				SERDES_DIGITAL_BLK, reg);
 }
 
 static void b53_serdes_an_restart(struct phylink_pcs *pcs)
 {
 	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
 	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	u16 reg;
+	int reg;
 
 	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
 			      SERDES_MII_BLK);
+	if (reg < 0)
+		goto out;
+
 	reg |= BMCR_ANRESTART;
-	b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			 SERDES_MII_BLK, reg);
+	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
+			       SERDES_MII_BLK, reg);
+out:
+	b53_serdes_error(dev, lane, reg);
 }
 
 static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
@@ -104,12 +159,23 @@ static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
 {
 	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
 	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	u16 dig, bmsr;
+	int dig, bmsr;
 
 	dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS,
 			      SERDES_DIGITAL_BLK);
+	if (dig < 0) {
+		state->link = false;
+		b53_serdes_error(dev, lane, dig);
+		return;
+	}
+
 	bmsr = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMSR),
 			       SERDES_MII_BLK);
+	if (bmsr < 0) {
+		state->link = false;
+		b53_serdes_error(dev, lane, bmsr);
+		return;
+	}
 
 	switch ((dig >> SPEED_STATUS_SHIFT) & SPEED_STATUS_MASK) {
 	case SPEED_STATUS_10:
@@ -140,19 +206,24 @@ void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
 			 phy_interface_t interface, bool link_up)
 {
 	u8 lane = b53_serdes_map_lane(dev, port);
-	u16 reg;
+	int reg;
 
 	if (lane == B53_INVALID_LANE)
 		return;
 
 	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
 			      SERDES_MII_BLK);
+	if (reg < 0)
+		goto out;
+
 	if (link_up)
 		reg &= ~BMCR_PDOWN;
 	else
 		reg |= BMCR_PDOWN;
-	b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			 SERDES_MII_BLK, reg);
+	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
+			       SERDES_MII_BLK, reg);
+out:
+	b53_serdes_error(dev, lane, reg);
 }
 EXPORT_SYMBOL(b53_serdes_link_set);
 
@@ -213,16 +284,25 @@ int b53_serdes_init(struct b53_device *dev, int port)
 {
 	u8 lane = b53_serdes_map_lane(dev, port);
 	struct b53_pcs *pcs;
-	u16 id0, msb, lsb;
+	int id0, msb, lsb;
 
-	if (lane == B53_INVALID_LANE)
+	if (lane >= B53_N_PCS)
 		return -EINVAL;
 
 	id0 = b53_serdes_read(dev, lane, B53_SERDES_ID0, SERDES_ID0);
+	if (id0 < 0)
+		return id0;
+
 	msb = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_PHYSID1),
 			      SERDES_MII_BLK);
+	if (msb < 0)
+		return msb;
+
 	lsb = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_PHYSID2),
 			      SERDES_MII_BLK);
+	if (lsb < 0)
+		return lsb;
+
 	if (id0 == 0 || id0 == 0xffff) {
 		dev_err(dev->dev, "SerDes not initialized, check settings\n");
 		return -ENODEV;
-- 
2.53.0

[PATCH net 2/6 RESEND] net: dsa: b53: manage SerDes power through the PCS callbacks

[Florian Fainelli]

Powering down the SerDes from mac_link_down() prevents the PCS from
observing link recovery. This also ties PCS power management to in-band
802.3z mode, leaving SGMII and other negotiation modes inconsistent.

Implement pcs_enable() and pcs_disable() so that phylink controls the
lane lifetime independently of individual link transitions. Remove the
MAC-to-SerDes power callback and perform read-modify-write operations
under the register mutex.

Fixes: 0e01491de646 ("net: dsa: b53: Add SerDes support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c |  8 ----
 drivers/net/dsa/b53/b53_priv.h   |  3 --
 drivers/net/dsa/b53/b53_serdes.c | 70 +++++++++++++++++++++++---------
 drivers/net/dsa/b53/b53_serdes.h |  2 -
 drivers/net/dsa/b53/b53_srab.c   |  1 -
 5 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 3f8f956aab82..09de3520e71a 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1630,10 +1630,6 @@ static void b53_phylink_mac_link_down(struct phylink_config *config,
 		b53_force_link(dev, port, false);
 		return;
 	}
-
-	if (phy_interface_mode_is_8023z(interface) &&
-	    dev->ops->serdes_link_set)
-		dev->ops->serdes_link_set(dev, port, mode, interface, false);
 }
 
 static void b53_phylink_mac_link_up(struct phylink_config *config,
@@ -1672,10 +1668,6 @@ static void b53_phylink_mac_link_up(struct phylink_config *config,
 		b53_force_link(dev, port, true);
 		return;
 	}
-
-	if (phy_interface_mode_is_8023z(interface) &&
-	    dev->ops->serdes_link_set)
-		dev->ops->serdes_link_set(dev, port, mode, interface, true);
 }
 
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 29cca7945df7..cebbebe4ae0f 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -54,9 +54,6 @@ struct b53_io_ops {
 						      int port,
 						      phy_interface_t interface);
 	u8 (*serdes_map_lane)(struct b53_device *dev, int port);
-	void (*serdes_link_set)(struct b53_device *dev, int port,
-				unsigned int mode, phy_interface_t interface,
-				bool link_up);
 };
 
 struct b53_arl_entry;
diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index c90894877626..87771e3788f7 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -107,6 +107,39 @@ static int b53_serdes_read(struct b53_device *dev, u8 lane,
 	return ret;
 }
 
+/* Return one if the register changed, zero if unchanged, or an error. */
+static int b53_serdes_modify(struct b53_device *dev, u8 lane, u8 offset,
+			     u16 block, u16 mask, u16 set)
+{
+	int ret, old;
+	u16 new;
+
+	mutex_lock(&dev->reg_mutex);
+	ret = b53_serdes_set_lane(dev, lane);
+	if (ret)
+		goto out;
+
+	old = b53_serdes_read_blk(dev, offset, block);
+	if (old < 0) {
+		ret = old;
+		goto out;
+	}
+
+	new = (old & ~mask) | set;
+	if (new == old)
+		goto out;
+
+	ret = b53_serdes_write_blk(dev, offset, block, new);
+	if (!ret)
+		ret = 1;
+out:
+	if (ret < 0)
+		dev->serdes_lane = B53_INVALID_LANE;
+	mutex_unlock(&dev->reg_mutex);
+
+	return ret;
+}
+
 static void b53_serdes_error(struct b53_device *dev, u8 lane, int ret)
 {
 	if (ret < 0)
@@ -202,32 +235,31 @@ static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
 		state->pause |= MLO_PAUSE_TX;
 }
 
-void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
-			 phy_interface_t interface, bool link_up)
+static int b53_serdes_enable(struct phylink_pcs *pcs)
 {
-	u8 lane = b53_serdes_map_lane(dev, port);
-	int reg;
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	int ret;
 
-	if (lane == B53_INVALID_LANE)
-		return;
+	ret = b53_serdes_modify(priv->dev, priv->lane,
+				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
+				BMCR_PDOWN, 0);
+	return ret < 0 ? ret : 0;
+}
 
-	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			      SERDES_MII_BLK);
-	if (reg < 0)
-		goto out;
+static void b53_serdes_disable(struct phylink_pcs *pcs)
+{
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	int ret;
 
-	if (link_up)
-		reg &= ~BMCR_PDOWN;
-	else
-		reg |= BMCR_PDOWN;
-	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			       SERDES_MII_BLK, reg);
-out:
-	b53_serdes_error(dev, lane, reg);
+	ret = b53_serdes_modify(priv->dev, priv->lane,
+				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
+				BMCR_PDOWN, BMCR_PDOWN);
+	b53_serdes_error(priv->dev, priv->lane, ret);
 }
-EXPORT_SYMBOL(b53_serdes_link_set);
 
 static const struct phylink_pcs_ops b53_pcs_ops = {
+	.pcs_enable = b53_serdes_enable,
+	.pcs_disable = b53_serdes_disable,
 	.pcs_get_state = b53_serdes_get_state,
 	.pcs_config = b53_serdes_config,
 	.pcs_an_restart = b53_serdes_an_restart,
diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h
index 3d367c4df4d9..6c851838457e 100644
--- a/drivers/net/dsa/b53/b53_serdes.h
+++ b/drivers/net/dsa/b53/b53_serdes.h
@@ -107,8 +107,6 @@ static inline u8 b53_serdes_map_lane(struct b53_device *dev, int port)
 	return dev->ops->serdes_map_lane(dev, port);
 }
 
-void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
-			 phy_interface_t interface, bool link_up);
 struct phylink_pcs *b53_serdes_phylink_mac_select_pcs(struct b53_device *dev,
 						      int port,
 						      phy_interface_t interface);
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 83a03fca142a..9988a83fa7b7 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -493,7 +493,6 @@ static const struct b53_io_ops b53_srab_ops = {
 #if IS_ENABLED(CONFIG_B53_SERDES)
 	.phylink_mac_select_pcs = b53_serdes_phylink_mac_select_pcs,
 	.serdes_map_lane = b53_srab_serdes_map_lane,
-	.serdes_link_set = b53_serdes_link_set,
 #endif
 };
 
-- 
2.53.0

Re: [PATCH net 2/6 RESEND] net: dsa: b53: manage SerDes power through the PCS callbacks

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Baseline decision: discard.

Pre-existing issues:
- [Medium] Baseline decision: discard (pre-existing, medium).

--

net: dsa: b53: manage SerDes power through the PCS callbacks

This patch moves SerDes power management out of the b53 mac_link_down()
and mac_link_up() handlers and into new pcs_enable() and pcs_disable()
callbacks. It removes the serdes_link_set io op. It also adds a
b53_serdes_modify() helper that does read-modify-write of SerDes
registers under reg_mutex.

> diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> index c90894877626e..87771e3788f7f 100644
> --- a/drivers/net/dsa/b53/b53_serdes.c
> +++ b/drivers/net/dsa/b53/b53_serdes.c

[ ... ]

> @@ -202,32 +235,31 @@ static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  		state->pause |= MLO_PAUSE_TX;
>  }
>  
> -void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
> -			 phy_interface_t interface, bool link_up)
> +static int b53_serdes_enable(struct phylink_pcs *pcs)
>  {
> -	u8 lane = b53_serdes_map_lane(dev, port);
> -	int reg;
> +	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
> +	int ret;
>  
> -	if (lane == B53_INVALID_LANE)
> -		return;
> +	ret = b53_serdes_modify(priv->dev, priv->lane,
> +				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
> +				BMCR_PDOWN, 0);
> +	return ret < 0 ? ret : 0;
> +}

[Severity: Low]
Should b53_serdes_enable() also pass the error to b53_serdes_error(), as
b53_serdes_disable() does below?

The phylink core ignores the return value of pcs_enable() in
phylink_major_config():

drivers/net/phy/phylink.c:phylink_major_config() {
    ...
	if (pl->pcs_state == PCS_STATE_STARTING || pcs_changed)
		phylink_pcs_enable(pl->pcs);
    ...
}

If clearing BMCR_PDOWN fails, the lane stays powered down and the link
won't come up until the interface is cycled.

b53_srab_op() and b53_srab_request_grant() already hit WARN_ON() before
they return -EIO, so the failure does show up in the log. This is mostly
about keeping the enable and disable paths consistent.

>  
> -	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> -			      SERDES_MII_BLK);
> -	if (reg < 0)
> -		goto out;
> +static void b53_serdes_disable(struct phylink_pcs *pcs)
> +{
> +	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
> +	int ret;
>  
> -	if (link_up)
> -		reg &= ~BMCR_PDOWN;
> -	else
> -		reg |= BMCR_PDOWN;
> -	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> -			       SERDES_MII_BLK, reg);
> -out:
> -	b53_serdes_error(dev, lane, reg);
> +	ret = b53_serdes_modify(priv->dev, priv->lane,
> +				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
> +				BMCR_PDOWN, BMCR_PDOWN);
> +	b53_serdes_error(priv->dev, priv->lane, ret);
>  }

[ ... ]

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index b9939bbd2cd51..9818b3922a377 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -493,7 +493,6 @@ static const struct b53_io_ops b53_srab_ops = {
>  #if IS_ENABLED(CONFIG_B53_SERDES)
>  	.phylink_mac_select_pcs = b53_serdes_phylink_mac_select_pcs,
>  	.serdes_map_lane = b53_srab_serdes_map_lane,
> -	.serdes_link_set = b53_serdes_link_set,
>  #endif
>  };

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The new
SerDes power paths go through the SRAB accessors, and the timeout check
in b53_srab_request_grant() does not match its loop bound:

drivers/net/dsa/b53/b53_srab.c:b53_srab_request_grant() {
    ...
	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}
	if (WARN_ON(i == 5))
		return -EIO;
    ...
}

If all 20 polls time out, i ends up as 20 and the function returns 0. The
caller then goes ahead with the SRAB access without having seen RCAGNT.

If the grant shows up on the sixth poll, i is 5. The function then WARNs
and returns -EIO even though the grant was obtained.

Was the check meant to be i == 20? It dates back to commit 967dd82ffc52
("net: dsa: b53: Add support for Broadcom RoboSwitch"). It is reachable
from this patch's code through:

b53_serdes_enable()->b53_serdes_modify()->b53_serdes_write_blk()->
    b53_srab_write16()->b53_srab_request_grant()

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com

[PATCH net 3/6 RESEND] net: dsa: b53: constrain SerDes interfaces to initialized PCSes

[Florian Fainelli]

Publish the supported interface bitmap on each successfully discovered
PCS and use it for capability reporting and PCS selection. Clear the
parallel interfaces inherited from the common switch code on ports whose
mux selects a SerDes. A failed discovery must not leave a usable serial
interface advertised without its PCS.

Stop advertising 2500BASE-X: the driver neither selects its baud rate nor
supports 2.5G in the MAC override code. The downstream robo SerDes speed
setter likewise only implements 10/100/1000. Keep SGMII and 1000BASE-X on
both lanes until a complete 2.5G programming path is available.

Report discovery failures at the SRAB port to make missing capabilities
diagnosable.

Fixes: 0e01491de646 ("net: dsa: b53: Add SerDes support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_serdes.c | 28 ++++++++--------------------
 drivers/net/dsa/b53/b53_srab.c   |  9 +++++++--
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index 87771e3788f7..d521050d7c8f 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -273,24 +273,11 @@ void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
 	if (lane == B53_INVALID_LANE)
 		return;
 
-	switch (lane) {
-	case 0:
-		/* It appears lane 0 supports 2500base-X and 1000base-X */
-		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
-			  config->supported_interfaces);
-		config->mac_capabilities |= MAC_2500FD;
-		fallthrough;
-	case 1:
-		/* It appears lane 1 only supports 1000base-X and SGMII */
-		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
-			  config->supported_interfaces);
-		__set_bit(PHY_INTERFACE_MODE_SGMII,
-			  config->supported_interfaces);
-		config->mac_capabilities |= MAC_1000FD;
-		break;
-	default:
-		break;
-	}
+	if (lane >= B53_N_PCS || !dev->pcs[lane].dev)
+		return;
+
+	phy_interface_copy(config->supported_interfaces,
+			   dev->pcs[lane].pcs.supported_interfaces);
 }
 EXPORT_SYMBOL(b53_serdes_phylink_get_caps);
 
@@ -304,8 +291,7 @@ struct phylink_pcs *b53_serdes_phylink_mac_select_pcs(struct b53_device *dev,
 	    !dev->pcs[lane].dev)
 		return NULL;
 
-	if (!phy_interface_mode_is_8023z(interface) &&
-	    interface != PHY_INTERFACE_MODE_SGMII)
+	if (!test_bit(interface, dev->pcs[lane].pcs.supported_interfaces))
 		return NULL;
 
 	return &dev->pcs[lane].pcs;
@@ -351,6 +337,8 @@ int b53_serdes_init(struct b53_device *dev, int port)
 	pcs->dev = dev;
 	pcs->lane = lane;
 	pcs->pcs.ops = &b53_pcs_ops;
+	__set_bit(PHY_INTERFACE_MODE_SGMII, pcs->pcs.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, pcs->pcs.supported_interfaces);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 9988a83fa7b7..77e81e1a414a 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -451,9 +451,11 @@ static void b53_srab_phylink_get_caps(struct b53_device *dev, int port,
 
 	switch (p->mode) {
 	case PHY_INTERFACE_MODE_SGMII:
+		/* The mux connects this port to a SerDes, not a parallel bus. */
+		phy_interface_zero(config->supported_interfaces);
 #if IS_ENABLED(CONFIG_B53_SERDES)
 		/* If p->mode indicates SGMII mode, that essentially means we
-		 * are using a serdes. As the serdes for the capabilities.
+		 * are using a SerDes. Ask it for the supported interfaces.
 		 */
 		b53_serdes_phylink_get_caps(dev, port, config);
 #endif
@@ -588,8 +590,11 @@ static void b53_srab_mux_init(struct platform_device *pdev)
 		case MUX_CONFIG_SGMII:
 			p->mode = PHY_INTERFACE_MODE_SGMII;
 			ret = b53_serdes_init(dev, port);
-			if (ret)
+			if (ret) {
+				dev_err(dev->dev, "port %d SerDes unavailable: %pe\n",
+					port, ERR_PTR(ret));
 				continue;
+			}
 			break;
 		case MUX_CONFIG_MII_LITE:
 			p->mode = PHY_INTERFACE_MODE_MII;
-- 
2.53.0

Re: [PATCH net 3/6 RESEND] net: dsa: b53: constrain SerDes interfaces to initialized PCSes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Ports muxed to SGMII now fail phylink creation when SerDes
  discovery fails or CONFIG_B53_SERDES=n.

--

net: dsa: b53: constrain SerDes interfaces to initialized PCSes

This patch stores a supported interface bitmap on each b53 PCS that is
discovered, and uses that bitmap for capability reporting and PCS
selection. On SRAB ports whose mux selects a SerDes, it clears the
parallel interfaces inherited from the common code. It also stops
advertising 2500BASE-X.

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index 9818b3922a377..3fabae80dc1a8 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -451,9 +451,11 @@ static void b53_srab_phylink_get_caps(struct b53_device *dev, int port,
>  
>  	switch (p->mode) {
>  	case PHY_INTERFACE_MODE_SGMII:
> +		/* The mux connects this port to a SerDes, not a parallel bus. */
> +		phy_interface_zero(config->supported_interfaces);
>  #if IS_ENABLED(CONFIG_B53_SERDES)
>  		/* If p->mode indicates SGMII mode, that essentially means we
> -		 * are using a serdes. As the serdes for the capabilities.
> +		 * are using a SerDes. Ask it for the supported interfaces.
>  		 */
>  		b53_serdes_phylink_get_caps(dev, port, config);
>  #endif

[Severity: Medium]
Could an SGMII-muxed port end up here with an empty supported_interfaces
mask, so that phylink_create() fails for it?

b53_srab_mux_init() sets p->mode before it tries discovery. If discovery
fails, the loop continues with p->mode still set to SGMII and
pcs[lane].dev still NULL:

    case MUX_CONFIG_SGMII:
        p->mode = PHY_INTERFACE_MODE_SGMII;
        ret = b53_serdes_init(dev, port);
        if (ret) {
            dev_err(...);
            continue;
        }

In that state b53_serdes_phylink_get_caps() returns early on
!dev->pcs[lane].dev, so nothing goes back into the mask.

With CONFIG_B53_SERDES=n, the b53_serdes_init() stub in b53_serdes.h
always returns -ENODEV and the refill call is compiled out. Every
SGMII-muxed port then ends up in this state.

phylink_create() then rejects the port:

    if (phy_interface_empty(config->supported_interfaces)) {
        dev_err(config->dev,
            "phylink: error: empty supported_interfaces\n");
        return ERR_PTR(-EINVAL);
    }

DSA then sets the user port up as unused. Before this patch, these ports
kept GMII/MII/REVMII from b53_phylink_get_caps() and registered normally.

Ports that never select the PCS are hit as well. In
bcm958625-meraki-alamo.dtsi, SRAB port@4 and port@5 (sw0/sw1) use a
1000 Mb/s fixed-link with no phy-mode, so DSA passes
PHY_INTERFACE_MODE_NA. b53_serdes_phylink_mac_select_pcs() returns NULL
for that mode both before and after this patch.

These two ports are the conduits for the cascaded QCA8337 switches
(ethernet = <&sgmii0> / <&sgmii1>). Wouldn't those switches lose their
upstream link too?

The commit message says:

    A failed discovery must not leave a usable serial interface
    advertised without its PCS.

It does not mention that the port then fails to register at all, or that
fixed-link/NA ports are affected. Is that the intended result?

Later patches in the series do not change b53_srab_phylink_get_caps() or
b53_srab_mux_init(), so this still applies at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com

[PATCH net 4/6 RESEND] net: dsa: b53: configure PCS negotiation and forced link parameters

[Florian Fainelli]

pcs_config() currently only selects fiber mode. Configure the Clause 37
or SGMII advertisement and BMCR autonegotiation enable from phylink's
negotiation mode, clear isolation, disable protocol auto-detection and
select the MAC side of SGMII explicitly. Report changes to phylink so it
can restart negotiation, and expose the supported in-band modes.

Implement pcs_link_up() for forced speed and duplex when in-band
negotiation is disabled. Use the receiver hold and TX FIFO reset sequence
from the downstream robo SerDes driver, clear its vendor speed override,
and restore the receiver and FIFO even if programming fails.

Leave analog and PLL initialization to the existing firmware setup.

Fixes: 0e01491de646 ("net: dsa: b53: Add SerDes support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_serdes.c | 187 ++++++++++++++++++++++++-------
 drivers/net/dsa/b53/b53_serdes.h |   6 +
 2 files changed, 151 insertions(+), 42 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index d521050d7c8f..fc20e211e67e 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -75,22 +75,6 @@ static int b53_serdes_set_lane(struct b53_device *dev, u8 lane)
 	return 0;
 }
 
-static int b53_serdes_write(struct b53_device *dev, u8 lane,
-			    u8 offset, u16 block, u16 value)
-{
-	int ret;
-
-	mutex_lock(&dev->reg_mutex);
-	ret = b53_serdes_set_lane(dev, lane);
-	if (!ret)
-		ret = b53_serdes_write_blk(dev, offset, block, value);
-	if (ret)
-		dev->serdes_lane = B53_INVALID_LANE;
-	mutex_unlock(&dev->reg_mutex);
-
-	return ret;
-}
-
 static int b53_serdes_read(struct b53_device *dev, u8 lane,
 			   u8 offset, u16 block)
 {
@@ -147,44 +131,161 @@ static void b53_serdes_error(struct b53_device *dev, u8 lane, int ret)
 				    lane, ERR_PTR(ret));
 }
 
+static unsigned int b53_serdes_inband_caps(struct phylink_pcs *pcs,
+					   phy_interface_t interface)
+{
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
+	default:
+		return 0;
+	}
+}
+
+static int b53_serdes_force_link(struct b53_pcs *priv, int speed, int duplex)
+{
+	struct b53_device *dev = priv->dev;
+	u8 lane = priv->lane;
+	u16 bmcr = mii_bmcr_encode_fixed(speed, duplex);
+	u16 mask = BMCR_SPEED100 | BMCR_SPEED1000 | BMCR_FULLDPLX;
+	int old, misc, rx, fifo, ret, err;
+
+	old = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
+			      SERDES_MII_BLK);
+	if (old < 0)
+		return old;
+	misc = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_MISC1,
+			       SERDES_DIGITAL_BLK);
+	if (misc < 0)
+		return misc;
+	rx = b53_serdes_read(dev, lane, B53_SERDES_RX_CONTROL, SERDES_RX_BLK);
+	if (rx < 0)
+		return rx;
+	fifo = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_CONTROL(3),
+			       SERDES_DIGITAL_BLK);
+	if (fifo < 0)
+		return fifo;
+
+	if ((old & mask) == bmcr && !(misc & FORCE_SPEED_MASK) &&
+	    !(rx & RX_SEQ_START) && !(fifo & TX_FIFO_RST))
+		return 0;
+
+	/* Hold the receiver and TX FIFO while switching the forced speed,
+	 * following the downstream robo SerDes programming sequence.
+	 */
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_RX_CONTROL,
+				SERDES_RX_BLK, RX_SEQ_START, RX_SEQ_START);
+	if (ret < 0)
+		goto release;
+
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_DIGITAL_CONTROL(3),
+				SERDES_DIGITAL_BLK, TX_FIFO_RST, TX_FIFO_RST);
+	if (ret < 0)
+		goto release;
+
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_DIGITAL_MISC1,
+				SERDES_DIGITAL_BLK, FORCE_SPEED_MASK, 0);
+	if (ret >= 0)
+		ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
+					SERDES_MII_BLK, mask, bmcr);
+release:
+	/* A failed write may have reached the device. Release both holds. */
+	err = b53_serdes_modify(dev, lane, B53_SERDES_RX_CONTROL,
+				SERDES_RX_BLK, RX_SEQ_START, 0);
+	if (ret >= 0)
+		ret = err;
+	err = b53_serdes_modify(dev, lane, B53_SERDES_DIGITAL_CONTROL(3),
+				SERDES_DIGITAL_BLK, TX_FIFO_RST, 0);
+	if (ret >= 0)
+		ret = err;
+
+	return ret < 0 ? ret : 1;
+}
+
 static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			     phy_interface_t interface,
 			     const unsigned long *advertising,
 			     bool permit_pause_to_mac)
 {
-	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
-	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	int reg;
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	struct b53_device *dev = priv->dev;
+	u8 lane = priv->lane;
+	int adv, ret, changed;
+	u16 ctrl, bmcr;
 
-	reg = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
-			      SERDES_DIGITAL_BLK);
-	if (reg < 0)
-		return reg;
-
-	if (interface == PHY_INTERFACE_MODE_1000BASEX)
-		reg |= FIBER_MODE_1000X;
-	else
-		reg &= ~FIBER_MODE_1000X;
-	return b53_serdes_write(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
-				SERDES_DIGITAL_BLK, reg);
+	if (!test_bit(interface, pcs->supported_interfaces))
+		return -EINVAL;
+
+	adv = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
+	if (adv < 0)
+		return adv;
+
+	ctrl = interface == PHY_INTERFACE_MODE_1000BASEX ? FIBER_MODE_1000X : 0;
+	/* Select the protocol explicitly and act as the MAC in SGMII mode. */
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_DIGITAL_CONTROL(1),
+				SERDES_DIGITAL_BLK,
+				FIBER_MODE_1000X | AUTODET_EN | SGMII_MASTER_MODE,
+				ctrl);
+	if (ret < 0)
+		return ret;
+	changed = ret;
+
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_ADVERTISE),
+				SERDES_MII_BLK, 0xffff, adv);
+	if (ret < 0)
+		return ret;
+	changed |= ret;
+
+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		/* The baud rate must be set before waiting for PCS link-up. */
+		ret = b53_serdes_force_link(priv, SPEED_1000, DUPLEX_FULL);
+	} else {
+		ret = b53_serdes_modify(dev, lane, B53_SERDES_DIGITAL_MISC1,
+					SERDES_DIGITAL_BLK, FORCE_SPEED_MASK, 0);
+	}
+	if (ret < 0)
+		return ret;
+	changed |= ret;
+
+	bmcr = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE : 0;
+	/* phylink requests Clause 37 restarts separately, but not SGMII. */
+	if (bmcr && changed && interface == PHY_INTERFACE_MODE_SGMII)
+		bmcr |= BMCR_ANRESTART;
+	ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
+				SERDES_MII_BLK,
+				BMCR_ANENABLE | BMCR_ISOLATE | BMCR_ANRESTART, bmcr);
+	if (ret < 0)
+		return ret;
+
+	/* The MAC applies phylink's resolved pause policy in mac_link_up(). */
+	return changed | ret;
 }
 
 static void b53_serdes_an_restart(struct phylink_pcs *pcs)
 {
-	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
-	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	int reg;
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	int ret;
 
-	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			      SERDES_MII_BLK);
-	if (reg < 0)
-		goto out;
+	ret = b53_serdes_modify(priv->dev, priv->lane,
+				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
+				BMCR_ANRESTART, BMCR_ANRESTART);
+	b53_serdes_error(priv->dev, priv->lane, ret);
+}
 
-	reg |= BMCR_ANRESTART;
-	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
-			       SERDES_MII_BLK, reg);
-out:
-	b53_serdes_error(dev, lane, reg);
+static void b53_serdes_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
+			       phy_interface_t interface, int speed, int duplex)
+{
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	int ret;
+
+	if (interface != PHY_INTERFACE_MODE_SGMII ||
+	    neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+		return;
+
+	ret = b53_serdes_force_link(priv, speed, duplex);
+	b53_serdes_error(priv->dev, priv->lane, ret);
 }
 
 static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
@@ -258,11 +359,13 @@ static void b53_serdes_disable(struct phylink_pcs *pcs)
 }
 
 static const struct phylink_pcs_ops b53_pcs_ops = {
+	.pcs_inband_caps = b53_serdes_inband_caps,
 	.pcs_enable = b53_serdes_enable,
 	.pcs_disable = b53_serdes_disable,
 	.pcs_get_state = b53_serdes_get_state,
 	.pcs_config = b53_serdes_config,
 	.pcs_an_restart = b53_serdes_an_restart,
+	.pcs_link_up = b53_serdes_link_up,
 };
 
 void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h
index 6c851838457e..2c2aa5eaa985 100644
--- a/drivers/net/dsa/b53/b53_serdes.h
+++ b/drivers/net/dsa/b53/b53_serdes.h
@@ -22,6 +22,11 @@
 #define B53_SERDES_MII_REG(x)		(0x20 + (x) * 2)
 #define B53_SERDES_DIGITAL_CONTROL(x)	(0x1e + (x) * 2)
 #define B53_SERDES_DIGITAL_STATUS	0x28
+#define B53_SERDES_DIGITAL_MISC1		0x30
+#define  FORCE_SPEED_MASK		GENMASK(4, 0)
+
+#define B53_SERDES_RX_CONTROL		0x22
+#define  RX_SEQ_START			BIT(15)
 
 /* SERDES_DIGITAL_CONTROL1 */
 #define  FIBER_MODE_1000X		BIT(0)
@@ -92,6 +97,7 @@
 #define  TXFIFO_ERR_DET			BIT(15)
 
 /* Block offsets */
+#define SERDES_RX_BLK			0x80b0
 #define SERDES_DIGITAL_BLK		0x8300
 #define SERDES_ID0			0x8310
 #define SERDES_MII_BLK			0xffe0
-- 
2.53.0

Re: [PATCH net 4/6 RESEND] net: dsa: b53: configure PCS negotiation and forced link parameters

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] b53_serdes_config() ignores permit_pause_to_mac.

--

net: dsa: b53: configure PCS negotiation and forced link parameters

This commit makes b53_serdes_config() program the advertisement, the BMCR
autonegotiation enable, the protocol selection and SGMII MAC mode based on
phylink's negotiation mode. It adds pcs_inband_caps(). It also adds a
pcs_link_up() handler that forces speed and duplex when in-band negotiation
is disabled.

> diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> index d521050d7c8f7..fc20e211e67e1 100644
> --- a/drivers/net/dsa/b53/b53_serdes.c
> +++ b/drivers/net/dsa/b53/b53_serdes.c

[ ... ]

>  static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  			     phy_interface_t interface,
>  			     const unsigned long *advertising,
>  			     bool permit_pause_to_mac)
>  {

[ ... ]

> +	bmcr = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE : 0;
> +	/* phylink requests Clause 37 restarts separately, but not SGMII. */
> +	if (bmcr && changed && interface == PHY_INTERFACE_MODE_SGMII)
> +		bmcr |= BMCR_ANRESTART;
> +	ret = b53_serdes_modify(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> +				SERDES_MII_BLK,
> +				BMCR_ANENABLE | BMCR_ISOLATE | BMCR_ANRESTART, bmcr);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The MAC applies phylink's resolved pause policy in mac_link_up(). */

[Severity: Low]
Is this comment accurate at this point in the series?

b53_serdes_config() ignores permit_pause_to_mac and relies on this comment
to justify that. However, b53_phylink_mac_link_up() in b53_common.c has no
MLO_AN_INBAND branch. Its MLO_AN_PHY branch only programs pause on 63xx
RGMII ports:

drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
    ...
	if (mode == MLO_AN_PHY) {
		/* Re-negotiate EEE if it was enabled already */
		p->eee_enabled = b53_eee_init(ds, port, phydev);

		if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4)) {
			b53_force_port_config(dev, port, speed, duplex,
					      tx_pause, rx_pause);
			b53_force_link(dev, port, true);
		}

		return;
	}
    ...
}

As a result, SGMII and 1000BASE-X SerDes ports in in-band or PHY mode never
get phylink's resolved tx_pause/rx_pause applied to the MAC. This is not a
change in behavior, since pause was not applied before this patch either.

A later patch in this series, "net: dsa: b53: apply resolved link settings
to serial port MACs", adds b53_phylink_mac_needs_override(). That helper
covers SGMII and 1000BASE-X in every mode and calls b53_force_port_config()
with tx_pause and rx_pause, which makes the comment correct.

Could the comment be added in that later patch, or could this commit
message note that the MAC side is handled later in the series?

> +	return changed | ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com

[PATCH net 5/6 RESEND] net: dsa: b53: report link state through the PCS

[Florian Fainelli]

Decode the Clause 22 BMSR and link partner word with the phylink helper.
This respects the negotiation mode, reports the partner advertisement,
and resolves Clause 37 pause from the configured advertisement instead
of unconditionally accepting the vendor status register's result.

Notify phylink through phylink_pcs_change() from the SerDes port interrupt,
using live PCS status rather than MAC link status, which can be overridden
by mac_link_up/down(). Remove the now unused MAC notification helper.

Enable PCS polling as a fallback for optional port interrupts and for
negotiation completion after the initial link-change interrupt.

Fixes: 79396934e289 ("net: dsa: b53: convert to phylink_pcs")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 12 ------
 drivers/net/dsa/b53/b53_priv.h   |  1 -
 drivers/net/dsa/b53/b53_serdes.c | 72 +++++++++++++++-----------------
 drivers/net/dsa/b53/b53_serdes.h |  5 +++
 drivers/net/dsa/b53/b53_srab.c   |  2 +-
 5 files changed, 40 insertions(+), 52 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 09de3520e71a..b04f9d5f0525 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1522,18 +1522,6 @@ static void b53_adjust_5325_mii(struct dsa_switch *ds, int port)
 	}
 }
 
-void b53_port_event(struct dsa_switch *ds, int port)
-{
-	struct b53_device *dev = ds->priv;
-	bool link;
-	u16 sts;
-
-	b53_read16(dev, B53_STAT_PAGE, B53_LINK_STAT, &sts);
-	link = !!(sts & BIT(port));
-	dsa_port_phylink_mac_change(ds, port, link);
-}
-EXPORT_SYMBOL(b53_port_event);
-
 static void b53_phylink_get_caps(struct dsa_switch *ds, int port,
 				 struct phylink_config *config)
 {
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index cebbebe4ae0f..6028440e1358 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -519,7 +519,6 @@ int b53_br_flags(struct dsa_switch *ds, int port,
 		 struct switchdev_brport_flags flags,
 		 struct netlink_ext_ack *extack);
 int b53_setup_devlink_resources(struct dsa_switch *ds);
-void b53_port_event(struct dsa_switch *ds, int port);
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 		       struct netlink_ext_ack *extack);
 int b53_vlan_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
index fc20e211e67e..c7f3e131f2de 100644
--- a/drivers/net/dsa/b53/b53_serdes.c
+++ b/drivers/net/dsa/b53/b53_serdes.c
@@ -291,50 +291,44 @@ static void b53_serdes_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
 				 struct phylink_link_state *state)
 {
-	struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev;
-	u8 lane = pcs_to_b53_pcs(pcs)->lane;
-	int dig, bmsr;
-
-	dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS,
-			      SERDES_DIGITAL_BLK);
-	if (dig < 0) {
-		state->link = false;
-		b53_serdes_error(dev, lane, dig);
-		return;
-	}
+	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
+	struct b53_device *dev = priv->dev;
+	u8 lane = priv->lane;
+	int bmsr, lpa;
 
 	bmsr = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMSR),
 			       SERDES_MII_BLK);
-	if (bmsr < 0) {
-		state->link = false;
-		b53_serdes_error(dev, lane, bmsr);
-		return;
-	}
+	if (bmsr < 0)
+		goto error;
 
-	switch ((dig >> SPEED_STATUS_SHIFT) & SPEED_STATUS_MASK) {
-	case SPEED_STATUS_10:
-		state->speed = SPEED_10;
-		break;
-	case SPEED_STATUS_100:
-		state->speed = SPEED_100;
-		break;
-	case SPEED_STATUS_1000:
-		state->speed = SPEED_1000;
-		break;
-	default:
-	case SPEED_STATUS_2500:
-		state->speed = SPEED_2500;
-		break;
-	}
+	lpa = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_LPA),
+			      SERDES_MII_BLK);
+	if (lpa < 0)
+		goto error;
+
+	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
+	return;
+error:
+	state->link = false;
+	state->an_complete = false;
+	b53_serdes_error(dev, lane, bmsr < 0 ? bmsr : lpa);
+}
+
+void b53_serdes_link_change(struct b53_device *dev, int port)
+{
+	u8 lane = b53_serdes_map_lane(dev, port);
+	int dig;
 
-	state->duplex = dig & DUPLEX_STATUS ? DUPLEX_FULL : DUPLEX_HALF;
-	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
-	state->link = !!(dig & LINK_STATUS);
-	if (dig & PAUSE_RESOLUTION_RX_SIDE)
-		state->pause |= MLO_PAUSE_RX;
-	if (dig & PAUSE_RESOLUTION_TX_SIDE)
-		state->pause |= MLO_PAUSE_TX;
+	if (lane >= B53_N_PCS || !dev->pcs[lane].dev)
+		return;
+
+	/* Use the live PCS status, not latched BMSR or MAC override status. */
+	dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS,
+			      SERDES_DIGITAL_BLK);
+	b53_serdes_error(dev, lane, dig);
+	phylink_pcs_change(&dev->pcs[lane].pcs, dig >= 0 && (dig & LINK_STATUS));
 }
+EXPORT_SYMBOL(b53_serdes_link_change);
 
 static int b53_serdes_enable(struct phylink_pcs *pcs)
 {
@@ -440,6 +434,8 @@ int b53_serdes_init(struct b53_device *dev, int port)
 	pcs->dev = dev;
 	pcs->lane = lane;
 	pcs->pcs.ops = &b53_pcs_ops;
+	/* Port interrupts are optional and may precede AN completion. */
+	pcs->pcs.poll = true;
 	__set_bit(PHY_INTERFACE_MODE_SGMII, pcs->pcs.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_1000BASEX, pcs->pcs.supported_interfaces);
 
diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h
index 2c2aa5eaa985..4016b24bcda4 100644
--- a/drivers/net/dsa/b53/b53_serdes.h
+++ b/drivers/net/dsa/b53/b53_serdes.h
@@ -119,8 +119,13 @@ struct phylink_pcs *b53_serdes_phylink_mac_select_pcs(struct b53_device *dev,
 void b53_serdes_phylink_get_caps(struct b53_device *dev, int port,
 				 struct phylink_config *config);
 #if IS_ENABLED(CONFIG_B53_SERDES)
+void b53_serdes_link_change(struct b53_device *dev, int port);
 int b53_serdes_init(struct b53_device *dev, int port);
 #else
+static inline void b53_serdes_link_change(struct b53_device *dev, int port)
+{
+}
+
 static inline int b53_serdes_init(struct b53_device *dev, int port)
 {
 	return -ENODEV;
diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 77e81e1a414a..56da3dbaf997 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -374,7 +374,7 @@ static irqreturn_t b53_srab_port_thread(int irq, void *dev_id)
 	struct b53_device *dev = port->dev;
 
 	if (port->mode == PHY_INTERFACE_MODE_SGMII)
-		b53_port_event(dev->ds, port->num);
+		b53_serdes_link_change(dev, port->num);
 
 	return IRQ_HANDLED;
 }
-- 
2.53.0

Re: [PATCH net 5/6 RESEND] net: dsa: b53: report link state through the PCS

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] b53_srab_port_thread()
  (drivers/net/dsa/b53/b53_srab.c:371-380) now calls…
- [Low] In b53_serdes_link_change()
  (drivers/net/dsa/b53/b53_serdes.c:317-330), if the…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  b53_srab_request_grant() (drivers/net/dsa/b53/b53_srab.c:91-112) has a…

--

net: dsa: b53: report link state through the PCS

This commit changes b53_serdes_get_state() to decode the Clause 22 BMSR
and LPA registers with phylink_mii_c22_pcs_decode_state(). The SRAB
port interrupt now notifies phylink through phylink_pcs_change(), using
the SerDes digital status LINK_STATUS bit instead of the MAC link status.
The patch also removes b53_port_event() and turns on PCS polling.

> diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> index fc20e211e67e1..c7f3e131f2def 100644
> --- a/drivers/net/dsa/b53/b53_serdes.c
> +++ b/drivers/net/dsa/b53/b53_serdes.c
> @@ -291,50 +291,44 @@ static void b53_serdes_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
>  static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  				 struct phylink_link_state *state)
>  {

[ ... ]

>  	bmsr = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMSR),
>  			       SERDES_MII_BLK);
> -	if (bmsr < 0) {
> -		state->link = false;
> -		b53_serdes_error(dev, lane, bmsr);
> -		return;
> -	}
> +	if (bmsr < 0)
> +		goto error;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
The SRAB accessors behind b53_serdes_read() have a mismatched timeout
check in b53_srab_request_grant() in drivers/net/dsa/b53/b53_srab.c:

	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}
	if (WARN_ON(i == 5))
		return -EIO;

If the grant never arrives, the loop exits with i == 20 and the function
returns 0. The register access then goes ahead without the RCA grant.

If the grant arrives on the sixth poll (i == 5), the function warns and
returns -EIO for an access that should have succeeded.
b53_serdes_get_state() then reports that error as link down.

Should this check be i == 20?

[ ... ]

> +void b53_serdes_link_change(struct b53_device *dev, int port)
> +{
> +	u8 lane = b53_serdes_map_lane(dev, port);
> +	int dig;
>  

[ ... ]

> +	if (lane >= B53_N_PCS || !dev->pcs[lane].dev)
> +		return;
> +
> +	/* Use the live PCS status, not latched BMSR or MAC override status. */
> +	dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS,
> +			      SERDES_DIGITAL_BLK);
> +	b53_serdes_error(dev, lane, dig);
> +	phylink_pcs_change(&dev->pcs[lane].pcs, dig >= 0 && (dig & LINK_STATUS));

[Severity: Medium]
Can this force a fixed-link SerDes port down?

b53_srab_port_thread() calls b53_serdes_link_change() for every port
whose mux mode is SGMII. Nothing on this path checks the phylink AN
mode. b53_serdes_phylink_mac_select_pcs() returns the PCS for SGMII and
1000BASE-X in every mode, so pcs->phylink is also set for fixed-link
ports.

When the SerDes has no sync, for example because the peer is not up
yet, this is the call chain:

b53_srab_port_thread()
  b53_serdes_link_change()
    phylink_pcs_change(pcs, false)
      phylink_link_changed()
        pl->link_failed = true;

phylink_resolve() checks link_failed before the fixed-link branch:

	} else if (pl->link_failed) {
		link_state = pl->link_config;
		link_state.link = false;
		retrigger = true;
	} else if (pl->act_link_an_mode == MLO_AN_FIXED) {

As a result, b53_phylink_mac_link_down() calls
b53_force_link(dev, port, false). The retriggered resolve then takes
the MLO_AN_FIXED path, and b53_phylink_mac_link_up() calls
b53_force_link(dev, port, true).

Before this patch, b53_port_event() read B53_LINK_STAT. That register
follows the forced override, so a fixed link always reported up=true
and this interrupt never took it down.

The commit message says MAC link status "can be overridden by
mac_link_up/down()". Does the link_state_pN interrupt also fire when
those override bits change? If so, each forced toggle would raise
another interrupt that reports SerDes down again. The forced MAC link
and the carrier would then keep flapping until the SerDes gets sync. I
can't confirm that hardware behaviour from the code.

Would it make sense to call phylink_pcs_change() only when the PCS
provides the link state (in-band), or to keep the previous MAC-status
notification for the other modes? The commit message also doesn't
mention this change in fixed-link behaviour.

[Severity: Low]
What happens here when the B53_SERDES_DIGITAL_STATUS read fails?

b53_serdes_read() can return a negative errno, for example -EIO from
b53_srab_request_grant() or a b53_srab_op() timeout. In that case the
expression evaluates to false and phylink_pcs_change(pcs, false) is
called. phylink_link_changed() then sets pl->link_failed. As a result,
phylink_resolve() forces a link down/up cycle while the real link state
is unknown.

The phylink_pcs_change() kernel-doc says:

  If in doubt about the link state at interrupt time, then pass true if
  pcs_get_state() returns the latched link-down state, otherwise pass
  false.

b53_serdes_get_state() now reads MII_BMSR, which latches link-down.
Should the error case pass true, so that a transient access failure in
the IRQ thread doesn't cause a link flap?

>  }
> +EXPORT_SYMBOL(b53_serdes_link_change);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com

[PATCH net 6/6 RESEND] net: dsa: b53: apply resolved link settings to serial port MACs

[Florian Fainelli]

The serial port MAC callbacks leave speed, duplex and pause under the
hardware's autonomous control. In particular, changing phylink's pause
policy cannot affect the MAC when Clause 37 negotiation is in use.

Use the existing MAC override registers for SGMII and 1000BASE-X links,
including PHY and fixed-link negotiation modes. Apply the settings passed
by phylink at link-up and force the MAC link down at link-down. PCS power
and link observation remain independent of the MAC override.

Preserve the existing parallel-port handling, EEE initialization and
BCM5301x CPU-port pause quirk.

Fixes: 0e01491de646 ("net: dsa: b53: Add SerDes support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 50 +++++++++++++++-----------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index b04f9d5f0525..172ada124cbf 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1600,6 +1600,17 @@ static void b53_phylink_mac_config(struct phylink_config *config,
 	}
 }
 
+static bool b53_phylink_mac_needs_override(struct b53_device *dev, int port,
+					   unsigned int mode,
+					   phy_interface_t interface)
+{
+	return mode == MLO_AN_FIXED ||
+	       interface == PHY_INTERFACE_MODE_SGMII ||
+	       interface == PHY_INTERFACE_MODE_1000BASEX ||
+	       (mode == MLO_AN_PHY && is63xx(dev) &&
+		in_range(port, B53_63XX_RGMII0, 4));
+}
+
 static void b53_phylink_mac_link_down(struct phylink_config *config,
 				      unsigned int mode,
 				      phy_interface_t interface)
@@ -1608,16 +1619,8 @@ static void b53_phylink_mac_link_down(struct phylink_config *config,
 	struct b53_device *dev = dp->ds->priv;
 	int port = dp->index;
 
-	if (mode == MLO_AN_PHY) {
-		if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4))
-			b53_force_link(dev, port, false);
-		return;
-	}
-
-	if (mode == MLO_AN_FIXED) {
+	if (b53_phylink_mac_needs_override(dev, port, mode, interface))
 		b53_force_link(dev, port, false);
-		return;
-	}
 }
 
 static void b53_phylink_mac_link_up(struct phylink_config *config,
@@ -1633,29 +1636,22 @@ static void b53_phylink_mac_link_up(struct phylink_config *config,
 	struct ethtool_keee *p = &dev->ports[dp->index].eee;
 	int port = dp->index;
 
-	if (mode == MLO_AN_PHY) {
-		/* Re-negotiate EEE if it was enabled already */
+	/* Re-negotiate EEE if it was enabled already */
+	if (mode == MLO_AN_PHY)
 		p->eee_enabled = b53_eee_init(ds, port, phydev);
 
-		if (is63xx(dev) && in_range(port, B53_63XX_RGMII0, 4)) {
-			b53_force_port_config(dev, port, speed, duplex,
-					      tx_pause, rx_pause);
-			b53_force_link(dev, port, true);
-		}
-
+	if (!b53_phylink_mac_needs_override(dev, port, mode, interface))
 		return;
-	}
 
-	if (mode == MLO_AN_FIXED) {
-		/* Force flow control on BCM5301x's CPU port */
-		if (is5301x(dev) && dsa_is_cpu_port(ds, port))
-			tx_pause = rx_pause = true;
-
-		b53_force_port_config(dev, port, speed, duplex,
-				      tx_pause, rx_pause);
-		b53_force_link(dev, port, true);
-		return;
+	/* Force flow control on BCM5301x's CPU port */
+	if (mode == MLO_AN_FIXED && is5301x(dev) && dsa_is_cpu_port(ds, port)) {
+		tx_pause = true;
+		rx_pause = true;
 	}
+
+	/* PCS negotiation supplies state; the MAC applies phylink's policy. */
+	b53_force_port_config(dev, port, speed, duplex, tx_pause, rx_pause);
+	b53_force_link(dev, port, true);
 }
 
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
-- 
2.53.0

Re: [PATCH net 6/6 RESEND] net: dsa: b53: apply resolved link settings to serial port MACs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] b53_force_port_config() uses the IMP-register bit layout when
  it writes the per-port GMII override register.
- [Medium] net: dsa: b53: b53_force_port_config() clears GMII_PO_EN on
  non-IMP ports

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  b53_srab_request_grant() in drivers/net/dsa/b53/b53_srab.c checks the…
- [Medium] This problem wasn't introduced by this patch, but
  b53_force_link() and b53_force_port_config() ignore the return value…

--

net: dsa: b53: apply resolved link settings to serial port MACs

With this commit, the b53 phylink link_up/link_down callbacks use the MAC
override registers for SGMII and 1000BASE-X ports in every negotiation
mode, not only for fixed-link. At link-up the MAC gets the speed, duplex
and pause that phylink resolved, and at link-down the MAC link is forced
down.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index b04f9d5f05256..172ada124cbf1 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1600,6 +1600,17 @@ static void b53_phylink_mac_config(struct phylink_config *config,
>  	}
>  }
>  
> +static bool b53_phylink_mac_needs_override(struct b53_device *dev, int port,
> +					   unsigned int mode,
> +					   phy_interface_t interface)
> +{
> +	return mode == MLO_AN_FIXED ||
> +	       interface == PHY_INTERFACE_MODE_SGMII ||
> +	       interface == PHY_INTERFACE_MODE_1000BASEX ||
> +	       (mode == MLO_AN_PHY && is63xx(dev) &&
> +		in_range(port, B53_63XX_RGMII0, 4));
> +}
> +

[ ... ]

> @@ -1633,29 +1636,22 @@ static void b53_phylink_mac_link_up(struct phylink_config *config,

[ ... ]

> +	/* Force flow control on BCM5301x's CPU port */
> +	if (mode == MLO_AN_FIXED && is5301x(dev) && dsa_is_cpu_port(ds, port)) {
> +		tx_pause = true;
> +		rx_pause = true;
>  	}
> +
> +	/* PCS negotiation supplies state; the MAC applies phylink's policy. */
> +	b53_force_port_config(dev, port, speed, duplex, tx_pause, rx_pause);
> +	b53_force_link(dev, port, true);

[Severity: Medium]
b53_phylink_mac_needs_override() now returns true for SGMII and
1000BASE-X in every mode. That means the BCM58xx SerDes ports (ports 4
and 5, see b53_srab_serdes_map_lane()) reach b53_force_port_config()
on every link-up. Those ports are never dev->imp_port, so the helper
programs B53_GMII_PORT_OVERRIDE_CTRL(port) with val = GMII_PO_EN.

Does the 2G handling in b53_force_port_config() use the right bit for
that register? In b53_regs.h, bits 6 and 7 are swapped between the IMP
override register and the GMII override register:

	PORT_OVERRIDE_SPEED_2000M	BIT(6)
	PORT_OVERRIDE_EN		BIT(7)

	GMII_PO_EN			BIT(6)
	GMII_PO_SPEED_2000M		BIT(7)

After reg |= GMII_PO_EN, the is5301x(dev) || is58xx(dev) branch does
reg &= ~PORT_OVERRIDE_SPEED_2000M. That clears BIT(6), which is the
override enable bit that was just set. GMII_PO_SPEED_2000M is never
cleared, and the case 2000 path sets BIT(6) rather than BIT(7).

As a result, the b53_write8() at the end of b53_force_port_config()
goes out with GMII_PO_EN clear. The MAC briefly returns to autonomous
hardware control until b53_force_link() sets the enable bit again in a
second write. If the bootloader left BIT(7) set, the MAC also stays
forced to 2G even though phylink resolved 1000. That contradicts the
commit message's claim that the settings passed by phylink are applied
at link-up.

The mask itself predates this patch and already affected non-IMP
fixed-link ports on 5301x/58xx. However, this patch is what routes the
SerDes ports through it in in-band and PHY modes, and it relies on the
helper to apply phylink's resolved state. Should the non-IMP path use
GMII_PO_SPEED_2000M here, for example by picking the 2G bit alongside
off and val?

[Severity: Medium]
b53_phylink_mac_needs_override() now returns true for SGMII and 1000BASE-X
in any mode. That means the BCM58xx SerDes ports (ports 5 and 4, mapped to
lanes 0 and 1 by b53_srab_serdes_map_lane()) now reach
b53_force_port_config() in in-band and PHY modes too.

These ports are not dev->imp_port, so they use the GMII override register.
Does b53_force_port_config() use the right bit layout for that register?

drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
    ...
	} else {
		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
		val = GMII_PO_EN;
	}

	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
	reg |= val;
    ...
	reg &= ~(0x3 << GMII_PO_SPEED_S);
	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;
    ...
}

In b53_regs.h the two registers have the enable bit and the 2G bit in
swapped positions:

#define   PORT_OVERRIDE_SPEED_2000M	BIT(6)
#define   PORT_OVERRIDE_EN		BIT(7)
...
#define   GMII_PO_EN			BIT(6)
#define   GMII_PO_SPEED_2000M		BIT(7)

So on the GMII register, clearing PORT_OVERRIDE_SPEED_2000M also clears
the GMII_PO_EN bit that was just set. GMII_PO_SPEED_2000M is never
cleared.

This seems to have three effects:

First, the write from b53_force_port_config() goes out with the override
disabled. The MAC briefly goes back to autonomous control until
b53_force_link() sets GMII_PO_EN again.

Second, if firmware or the bootloader left BIT(7) set, the MAC stays forced
to 2G while phylink resolved 1000. The driver never clears that bit, and
its case 2000 sets BIT(6) on this register. Would that contradict the
commit message's "Apply the settings passed by phylink at link-up"?

Third, for an unknown speed, b53_force_port_config() calls dev_err() and
returns without writing anything. b53_phylink_mac_link_up() still calls
b53_force_link(dev, port, true), which forces the link up with the old
speed, duplex and pause bits. For SGMII and 1000BASE-X this may not be
reachable in practice.

The wrong mask in b53_force_port_config() is older code and already
affected non-IMP fixed-link ports on 5301x/58xx. This patch is what sends
the SerDes ports through it and relies on it to apply phylink's settings.

[Severity: Medium]
This isn't a bug introduced by this patch, but b53_force_port_config() and
b53_force_link() both ignore the return value of b53_read8() before they
modify reg and write it back:

	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
	reg |= val;

b53_srab_read8() returns without setting *val when
b53_srab_request_grant() or b53_srab_op() fails.

Can reg then hold uninitialized stack data that gets written to
B53_GMII_PORT_OVERRIDE_CTRL(port) with the enable and link bits set?

Neither helper reports a failure. If the write in b53_force_port_config()
fails but the next b53_force_link(dev, port, true) succeeds, the MAC is
forced up with its previous settings. Likewise, a failed write at link-down
can leave the MAC forced up.

This patch adds the SerDes ports in in-band and PHY modes to the users of
these helpers.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. On SRAB
switches these override accesses go through b53_srab_read8() and
b53_srab_write8(), and both call b53_srab_request_grant() first. Is the
timeout check in that function using the right limit?

drivers/net/dsa/b53/b53_srab.c:b53_srab_request_grant() {
    ...
	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}
	if (WARN_ON(i == 5))
		return -EIO;
    ...
}

If the grant never arrives, the loop exits with i == 20 and the function
returns 0. The register access then goes ahead without the grant.

If the grant arrives on the sixth poll, i == 5. A WARN fires and -EIO is
returned even though the grant is held, so the access is skipped.

Should this check be against 20 instead?

>  }
>  
>  int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com

Re: [PATCH net 0/6 RESEND] net: dsa: b53: PCS fixes

[Chenguang Zhao]

On Thu, Oct 22, 2026 at 03:56:50PM -0700, Florian Fainelli wrote:
> This patch series fixes a number of issues found with a LLM and while
> testing on a Broadcom Northstar Plus reference board (BCM958625HR) using
> a SFP connected to a fiber converter:
> 
> - there was no serialization of the registers across multiple lanes
> - the SerDes powering down was incorrectly managed and should have used
>   .pcs_{enable,disable}
> - the interfaces advertisement should have been confined to successfully
>   initialized PCSes
> - the negotation and forced link parameters were incorrectly supported
>   and should have used pcs_link_up and pcs_inband_caps
> - link state should have been reported through the PCS
> 
> With these patches disconnecting the fiber cables triggers a link down,
> as does disconnecting the fiber converter from its cable or powering it
> down, ip link up/down also work properly as a result as well.
> 
> Florian Fainelli (6):
>   net: dsa: b53: serialize SerDes register transactions
>   net: dsa: b53: manage SerDes power through the PCS callbacks
>   net: dsa: b53: constrain SerDes interfaces to initialized PCSes
>   net: dsa: b53: configure PCS negotiation and forced link parameters
>   net: dsa: b53: report link state through the PCS
>   net: dsa: b53: apply resolved link settings to serial port MACs
> 
>  drivers/net/dsa/b53/b53_common.c |  69 ++----
>  drivers/net/dsa/b53/b53_priv.h   |   4 -
>  drivers/net/dsa/b53/b53_serdes.c | 407 +++++++++++++++++++++++--------
>  drivers/net/dsa/b53/b53_serdes.h |  13 +-
>  drivers/net/dsa/b53/b53_srab.c   |  12 +-
>  5 files changed, 345 insertions(+), 160 deletions(-)
> 
> -- 
> 2.53.0
> 

Hi Florian,

Please take a look at the issues Sashiko reported.

Thanks,
Chenguang