[PATCH v2 net-next 0/7] net: dsa: mt7530: support EcoNet EN751221

[PATCH v2 net-next 0/7] net: dsa: mt7530: support EcoNet EN751221

[Caleb James DeLisle]

The EcoNet EN751221 has a unique switch configuration. Every chip made
with an EN751221 die has at least one on-die MT7530 that is MMIO based.
This switch has 3x 10/100 ports and 1x gigabit port.

Some popular chip packages such as the EN7513G and EN7526G have another
MT7530 implemented as a multi-chip-module that connects to port 5 of
the on-die switch and is controlled via the MDIO controller integrated
in the on-die MT7530.

In this configurations, the upstream switch is put into a "passthrough"
mode in which all traffic is forwarded between port 5 and port 6
without altering its DSA tags.

Furthermore, since the DSA core does not call mt753x_setup until all
switches are present, and the downstream switch only becomes available
when the upstream switch has configured its MDIO controller, the MDIO
controller setup on MMIO implementations must be done before
mt753x_setup, or else it will never complete.

Finally, the on-die switch and the MCM switch connect via a TRGMII link
which requires calibration. In order to calibrate this, PLL adjustment
must be possible on the on-die switch, necessitating changes to the
core_(set|clear|write|rmw) functions to support MMIO implementations.

Solve all of these issues, add the TRGMII calibration, and add support
for both switches.

Since this patchset will probably be subject to some discussion, please
consider accepting any patches that are non-controversial since it will
make future re-submissions easier.

Thanks!

Changes since v1:
* Move mt7530_setup_mdio_irq out of mt7530_setup_mdio because
mt7530_setup_mdio may be called before mt753x_setup

* Do not return error in mt7988_probe if mt7530_setup_mdio fails because
doing so leaks the DSA switch registration, and in any case
mt7530_setup_mdio will be called again.

* default_taps_ondie not being used en751221_trgmii_cal_ok unused args

* Use clk_drvp / clk_drvp instead of drvp / drvn (typo)

* en751221_set_tx_drive ext values too large for fields - values taken
from reference implementation, reduced from 16 to 15.

v1: https://lore.kernel.org/netdev/20260909140346.2861572-1-cjd@cjdns.fr/

Caleb James DeLisle (7):
  net: dsa: mt7530: get ctrl phy addr using a function
  dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode
  net: dsa: mediatek: add support for passthrough mode
  net: dsa: mediatek: support PLL setup on MMIO MT7530
  net: dsa: mediatek: support MDIO switch downstream of MMIO switch
  dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221
  net: dsa: mediatek: support EN751221 switch

 .../bindings/net/dsa/mediatek,mt7530.yaml     |  50 +-
 drivers/net/dsa/mt7530-mdio.c                 |   1 +
 drivers/net/dsa/mt7530-mmio.c                 |  10 +-
 drivers/net/dsa/mt7530.c                      | 579 ++++++++++++++++--
 drivers/net/dsa/mt7530.h                      |  46 +-
 5 files changed, 632 insertions(+), 54 deletions(-)

-- 
2.39.5

[PATCH v2 net-next 1/7] net: dsa: mt7530: get ctrl phy addr using a function

[Caleb James DeLisle]

The MDIO MT7530 has 5 integrated PHYs, of these, the first PHY has
the additional registers that are used for such things as configuring
the clock configuration for the switch. The switch is typically on
MDIO address 31 and the PHYs are 0, 1, 2, 3, and 4 with 0 being the
special one. So taking the address of mdiodev (the switch) add 1 and
modulo 31 finds the first PHY.

However there are a number of MMIO based implementations of MT7530
and on these, mdiodev is NULL though they often still have a control
PHY for the purpose. Move MT753X_CTRL_PHY_ADDR to a function which
handles the NULL condition somewhat more gracefully and allows a
place for MMIO implementations to add special case handling to
provide their control PHY address.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/dsa/mt7530.c | 54 +++++++++++++++++++++-------------------
 drivers/net/dsa/mt7530.h |  2 --
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 14692497e1a6..54d931002bc1 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -46,6 +46,16 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
 	MIB_DESC(1, MT7530_PORT_MIB_RX_ARL_DROP, "RxArlDrop"),
 };
 
+static int
+mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
+{
+	if (WARN_ON_ONCE(!priv->mdiodev))
+		return 0;
+
+	/* Default is 1st PHY */
+	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);
+}
+
 static void
 mt7530_mutex_lock(struct mt7530_priv *priv)
 {
@@ -63,32 +73,30 @@ mt7530_mutex_unlock(struct mt7530_priv *priv)
 static void
 core_write(struct mt7530_priv *priv, u32 reg, u32 val)
 {
+	int ctl_phy = mt753x_ctrl_phy_addr(priv);
 	struct mii_bus *bus = priv->bus;
 	int ret;
 
 	mt7530_mutex_lock(priv);
 
 	/* Write the desired MMD Devad */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_CTRL, MDIO_MMD_VEND2);
+	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
 	if (ret < 0)
 		goto err;
 
 	/* Write the desired MMD register address */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_DATA, reg);
+	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, reg);
 	if (ret < 0)
 		goto err;
 
 	/* Select the Function : DATA with no post increment */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
+	ret = bus->write(bus, ctl_phy,
 			 MII_MMD_CTRL, MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR);
 	if (ret < 0)
 		goto err;
 
 	/* Write the data into MMD's selected register */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_DATA, val);
+	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);
 err:
 	if (ret < 0)
 		dev_err(&bus->dev, "failed to write mmd register\n");
@@ -99,6 +107,7 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
 static void
 core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
 {
+	int ctl_phy = mt753x_ctrl_phy_addr(priv);
 	struct mii_bus *bus = priv->bus;
 	u32 val;
 	int ret;
@@ -106,26 +115,23 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
 	mt7530_mutex_lock(priv);
 
 	/* Write the desired MMD Devad */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_CTRL, MDIO_MMD_VEND2);
+	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
 	if (ret < 0)
 		goto err;
 
 	/* Write the desired MMD register address */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_DATA, reg);
+	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, reg);
 	if (ret < 0)
 		goto err;
 
 	/* Select the Function : DATA with no post increment */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
+	ret = bus->write(bus, ctl_phy,
 			 MII_MMD_CTRL, MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR);
 	if (ret < 0)
 		goto err;
 
 	/* Read the content of the MMD's selected register */
-	ret = bus->read(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			MII_MMD_DATA);
+	ret = bus->read(bus, ctl_phy, MII_MMD_DATA);
 	if (ret < 0)
 		goto err;
 	val = ret;
@@ -133,8 +139,7 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
 	val &= ~mask;
 	val |= set;
 	/* Write the data into MMD's selected register */
-	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-			 MII_MMD_DATA, val);
+	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);
 err:
 	if (ret < 0)
 		dev_err(&bus->dev, "failed to write mmd register\n");
@@ -2674,8 +2679,11 @@ mt7531_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 val, id;
+	int ctl_phy;
 	int ret, i;
 
+	ctl_phy = mt753x_ctrl_phy_addr(priv);
+
 	/* Reset whole chip through gpio pin or memory-mapped registers for
 	 * different type of hardware
 	 */
@@ -2743,25 +2751,21 @@ mt7531_setup(struct dsa_switch *ds)
 	 * phy_[read,write]_mmd_indirect is called, we provide our own
 	 * mt7531_ind_mmd_phy_[read,write] to complete this function.
 	 */
-	ret = mt7531_ind_c45_phy_read(priv,
-				      MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-				      MDIO_MMD_VEND2, CORE_PLL_GROUP4);
+	ret = mt7531_ind_c45_phy_read(priv, ctl_phy, MDIO_MMD_VEND2,
+				      CORE_PLL_GROUP4);
 	if (ret < 0)
 		return ret;
 
 	val = ret;
 	val |= MT7531_RG_SYSPLL_DMY2 | MT7531_PHY_PLL_BYPASS_MODE;
 	val &= ~MT7531_PHY_PLL_OFF;
-	ret = mt7531_ind_c45_phy_write(priv,
-				       MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
-				       MDIO_MMD_VEND2, CORE_PLL_GROUP4, val);
+	ret = mt7531_ind_c45_phy_write(priv, ctl_phy, MDIO_MMD_VEND2,
+				       CORE_PLL_GROUP4, val);
 	if (ret < 0)
 		return ret;
 
 	/* Disable EEE advertisement on the switch PHYs. */
-	for (i = MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr);
-	     i < MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr) + MT7530_NUM_PHYS;
-	     i++) {
+	for (i = ctl_phy; i < ctl_phy + MT7530_NUM_PHYS; i++) {
 		mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
 					 0);
 	}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 2bbbe617b52e..3dabbc99fbbc 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -747,8 +747,6 @@ enum mt7531_xtal_fsel {
 #define  MT7531_PHY_PLL_OFF		BIT(5)
 #define  MT7531_PHY_PLL_BYPASS_MODE	BIT(4)
 
-#define MT753X_CTRL_PHY_ADDR(addr)	(((addr) + 1) & (PHY_MAX_ADDR - 1))
-
 #define CORE_PLL_GROUP5			0x404
 #define  RG_LCDDS_PCW_NCPO1_MASK	GENMASK(15, 0)
 #define  RG_LCDDS_PCW_NCPO1(x)		FIELD_PREP(RG_LCDDS_PCW_NCPO1_MASK, x)
-- 
2.39.5

[PATCH v2 net-next 2/7] dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

[Caleb James DeLisle]

MT7530 switches can be configured to blindly pass packets between their
CPU/upstream port and a downstream DSA port, in particular, without
altering the MediaTek DSA Tag on the packets. This mode is used in some
integrations where an SoC integrated switch exists, but a more capable
MCM switch is added to effectively replace it.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 .../devicetree/bindings/net/dsa/mediatek,mt7530.yaml       | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index 90b3582b7619..e29fb8693498 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -73,6 +73,13 @@ description: |
 
       Check out example 7.
 
+  The MT7530 and MT7531 switches are capable of being operated in "passthrough"
+  mode. In this mode, traffic is unconditionally passed between the CPU port
+  and the DSA port which links to a second switch. To use passthrough mode, the
+  upstream switch is configured with a DSA link to the downstream switch. In
+  passthrough mode the upstream switch is restricted, with no other ports or
+  switch configuration allowed.
+
 properties:
   compatible:
     oneOf:
-- 
2.39.5

[PATCH v2 net-next 3/7] net: dsa: mediatek: add support for passthrough mode

[Caleb James DeLisle]

In some cases, hardware integrators add a second MT7530 switch to a
port of the CPU-connected MT7530 and configure the upstream switch to
pass all traffic to and from the downstream switch. Particularly,
without altering the MediaTek DSA tag. The typical reason for this is
because the downstream switch is more capable, but the upstream switch
is an irremovable part of the SoC.

To enable passthrough, the main switch has its CPU port, and the port
to the second switch, both configured as MT7530_VLAN_EG_DISABLED and
MT7530_VLAN_TRANSPARENT and it uses the port matrix to control
forwarding.

Support passing through to a downstream switch, and also support being
the downstream switch - meaning the upstream link is not a CPU port but
rather a DSA port.

As a point of information:

There exist features in the hardware which appear to enable full
two-switch hierarchies (i.e. ports on the upstream switch are also
available for use). The way that this is supposed to work is though a
PASSTHROUGH flag in the DSA tag on BIT(7). The downstream switch sets
a flag called "PT_OPTION" on the PVC register of its "CPU" port (the
port linking to the upstream switch). This causes the downstream switch
to set the PASSTHROUGH flag on all ingress traffic. The CPU then sets
the PASSTHROUGH flag on egress traffic meant for the downstream switch.

In this configuration, the upstream switch sets MT7530_VLAN_USER, and
PORT_SPEC_TAG on the (real) CPU port (as usual), but also sets it on
the port leading to the downstream switch. The PORT_SPEC_TAG flag on
the downstream link port causes incoming DSA tags to be copied over
rather than being replaced by tags identifying the traffic as having
come from the link port. Finally, an ACL is installed which causes the
upstream switch to forward all egress traffic with the PASSTHROUGH bit
to the link port.

However for reasons unknown, the PASSTHROUGH bit is cleared by the
upstream switch as it receives ingress traffic from the downstream
switch. So by the time the traffic reaches the CPU, it no longer bears
the PASSTHROUGH bit and its origin cannot be known.

Reference code defaults to passthrough mode as is implemented here, but
supports a limited hierarchical implementation for hardware that needs
it. In this mode it is not permitted to use the same port number on
both switches. This implementation does not implement that mode because
there is no known hardware using ports on both switches.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/dsa/mt7530.c | 164 +++++++++++++++++++++++++++++++++++----
 drivers/net/dsa/mt7530.h |   6 ++
 2 files changed, 153 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 54d931002bc1..b2344808457c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1286,13 +1286,41 @@ mt753x_trap_frames(struct mt7530_priv *priv)
 				   TO_CPU_FW_CPU_ONLY);
 }
 
+static int
+mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);
+
+/* If this switch is downstream of another switch that is in passthrough mode,
+ * the "CPU" port is actually a DSA port.
+ */
 static void
 mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
 {
 	struct mt7530_priv *priv = ds->priv;
 
-	/* Enable Mediatek header mode on the cpu port */
-	regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
+	if (priv->is_passthrough) {
+		/* Disable parsing of the DSA tag, it will be forwarded blindly
+		 * to the downstream switch.
+		 */
+		regmap_write(priv->regmap, MT7530_PVC_P(port),
+			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
+			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));
+
+		/* The port is not configured to parse DSA tags, so they are
+		 * liable to be confused for length fields, so length check is
+		 * disabled.
+		 */
+		regmap_clear_bits(priv->regmap, MT753X_AGC, AGC_L2LEN_CHK);
+
+		/* In passthrough mode, MTU is only enforced downstream */
+		mt7530_port_change_mtu(ds, port, MT7530_MAX_MTU);
+
+		/* Loop detection has no value in passthrough mode */
+		regmap_set_bits(priv->regmap, MT753X_MTRAP,
+				MT7530_LOOP_DET_DISABLE);
+	} else {
+		/* Not passthrough, enable DSA tag handling on CPU port. */
+		regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
+	}
 
 	/* Enable flooding on the CPU port */
 	regmap_set_bits(priv->regmap, MT753X_MFC,
@@ -1322,6 +1350,7 @@ static int
 mt7530_port_enable(struct dsa_switch *ds, int port,
 		   struct phy_device *phy)
 {
+	int upstream_pt = dsa_switch_upstream_port(ds);
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct mt7530_priv *priv = ds->priv;
 
@@ -1332,13 +1361,38 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 	 * bridge.
 	 */
 	if (dsa_port_is_user(dp)) {
-		struct dsa_port *cpu_dp = dp->cpu_dp;
+		priv->ports[port].pm |= PCR_MATRIX(BIT(upstream_pt));
+
+	} else if (dsa_port_is_dsa(dp) && dp->index != upstream_pt) {
+		priv->ports[port].pm |= PCR_MATRIX(BIT(upstream_pt));
+
+		/* Should not happen */
+		WARN_ON_ONCE(!priv->is_passthrough);
+
+		/* We are passing through to a downstream switch so we set both
+		 * CPU and downstream link to pass traffic untouched so that
+		 * the STAG from the downstream switch will pass to the upstream.
+		 */
+		regmap_write(priv->regmap, MT7530_PVC_P(port),
+			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
+			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));
+
+		/* We let the downstream switch flood */
+		regmap_set_bits(priv->regmap, MT753X_MFC, BC_FFP(BIT(port)) |
+				UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
 
-		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
+		/* Make the upstream port able to connect to the DSA port.
+		 * This must be explicit because PORT_SPEC_TAG is unset.
+		 */
+		regmap_write(priv->regmap, MT7530_PCR_P(upstream_pt),
+			     PCR_MATRIX(BIT(port)));
 	}
 	priv->ports[port].enable = true;
-	regmap_update_bits(priv->regmap, MT7530_PCR_P(port), PCR_MATRIX_MASK,
-			   priv->ports[port].pm);
+
+	/* In passthrough mode, CPU port mask is set above. */
+	if (!(priv->is_passthrough && dp->index == upstream_pt))
+		regmap_update_bits(priv->regmap, MT7530_PCR_P(port),
+				   PCR_MATRIX_MASK, priv->ports[port].pm);
 
 	mutex_unlock(&priv->reg_mutex);
 
@@ -1390,7 +1444,7 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 	 * largest MTU of the user ports. Because the switch only has a global
 	 * RX length register, only allowing CPU port here is enough.
 	 */
-	if (!dsa_is_cpu_port(ds, port))
+	if (!dsa_is_upstream_port(ds, port))
 		return 0;
 
 	regmap_read(priv->regmap, MT7530_GMACCR, &val);
@@ -2411,11 +2465,6 @@ mt7530_setup(struct dsa_switch *ds)
 		break;
 	}
 
-	if (!dn) {
-		dev_err(ds->dev, "parent OF node of DSA conduit not found");
-		return -EINVAL;
-	}
-
 	ds->assisted_learning_on_cpu_port = true;
 	ds->untag_vlan_aware_bridge_pvid = true;
 	ds->mtu_enforcement_ingress = true;
@@ -2517,7 +2566,7 @@ mt7530_setup(struct dsa_switch *ds)
 		/* Disable learning by default on all ports */
 		regmap_set_bits(priv->regmap, MT7530_PSC_P(i), SA_DIS);
 
-		if (dsa_is_cpu_port(ds, i)) {
+		if (dsa_is_upstream_port(ds, i)) {
 			mt753x_cpu_port_enable(ds, i);
 		} else {
 			mt7530_port_disable(ds, i);
@@ -2541,7 +2590,7 @@ mt7530_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Check for PHY muxing on port 5 */
-	if (dsa_is_unused_port(ds, 5)) {
+	if (dn && dsa_is_unused_port(ds, 5)) {
 		/* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
 		 * Set priv->p5_mode to the appropriate value if PHY muxing is
 		 * detected.
@@ -2641,7 +2690,7 @@ mt7531_setup_common(struct dsa_switch *ds)
 		regmap_set_bits(priv->regmap, MT7531_DBG_CNT(i),
 				MT7531_DIS_CLR);
 
-		if (dsa_is_cpu_port(ds, i)) {
+		if (dsa_is_upstream_port(ds, i)) {
 			mt753x_cpu_port_enable(ds, i);
 		} else {
 			mt7530_port_disable(ds, i);
@@ -3310,7 +3359,7 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
 
 	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
 	 * forwarded to the numerically smallest CPU port whose conduit
-	 * interface is up.
+	 * interface is up. NOTE: "CPU port" can also mean an upstream DSA link.
 	 */
 	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
 	    priv->id != ID_EN7528)
@@ -3405,6 +3454,76 @@ static int mt7988_setup(struct dsa_switch *ds)
 	return mt7531_setup_common(ds);
 }
 
+/* 1 if passthrough, negative if error. */
+static int mt753x_check_passthrough(struct device *dev)
+{
+	struct device_node *ports, *port;
+	int passthrough_ports = 0;
+	int enabled_ports = 0;
+
+	ports = of_get_child_by_name(dev->of_node, "ports");
+	if (!ports)
+		ports = of_get_child_by_name(dev->of_node, "ethernet-ports");
+
+	if (!ports) {
+		dev_err(dev, "no ports child node found\n");
+		return -EINVAL;
+	}
+
+	for_each_available_child_of_node(ports, port) {
+		struct device_node *link;
+
+		enabled_ports++;
+
+		link = of_parse_phandle(port, "ethernet", 0);
+		if (!link)
+			link = of_parse_phandle(port, "link", 0);
+
+		if (!link)
+			continue;
+
+		of_node_put(link);
+
+		passthrough_ports++;
+	}
+
+	of_node_put(ports);
+
+	/*
+	 * A switch is considered passthrough if exactly two available
+	 * ports have an "ethernet" or "link" phandle.
+	 */
+	if (passthrough_ports > 2 ||
+	    (passthrough_ports == 2 && enabled_ports != 2)
+	) {
+		dev_err(dev, "Only two ports allowed in passthrough mode\n");
+		return -EINVAL;
+	}
+
+	return passthrough_ports == 2;
+}
+
+/* No manipulation of forwarding rules allowed in passthrough mode */
+static const struct dsa_switch_ops mt7530_passthrough_switch_ops = {
+	.get_tag_protocol	= mtk_get_tag_protocol,
+	.setup			= mt753x_setup,
+	.teardown		= mt753x_teardown,
+	.preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port,
+	.get_strings		= mt7530_get_strings,
+	.get_ethtool_stats	= mt7530_get_ethtool_stats,
+	.get_sset_count		= mt7530_get_sset_count,
+	.get_eth_mac_stats	= mt7530_get_eth_mac_stats,
+	.get_rmon_stats		= mt7530_get_rmon_stats,
+	.get_eth_ctrl_stats	= mt7530_get_eth_ctrl_stats,
+	.get_stats64		= mt7530_get_stats64,
+	.port_enable		= mt7530_port_enable,
+	.port_disable		= mt7530_port_disable,
+	.phylink_get_caps	= mt753x_phylink_get_caps,
+	.support_eee		= dsa_supports_eee,
+	.set_mac_eee		= mt753x_set_mac_eee,
+	.conduit_state_change	= mt753x_conduit_state_change,
+};
+
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt753x_setup,
@@ -3537,8 +3656,12 @@ EXPORT_SYMBOL_GPL(mt753x_table);
 int
 mt7530_probe_common(struct mt7530_priv *priv)
 {
+	int passthrough = mt753x_check_passthrough(priv->dev);
 	struct device *dev = priv->dev;
 
+	if (passthrough < 0)
+		return passthrough;
+
 	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
 		return -ENOMEM;
@@ -3556,7 +3679,14 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	priv->id = priv->info->id;
 	priv->dev = dev;
 	priv->ds->priv = priv;
-	priv->ds->ops = &mt7530_switch_ops;
+
+	if (passthrough) {
+		priv->ds->ops = &mt7530_passthrough_switch_ops;
+		priv->is_passthrough = true;
+	} else {
+		priv->ds->ops = &mt7530_switch_ops;
+	}
+
 	priv->ds->phylink_mac_ops = &mt753x_phylink_mac_ops;
 	mutex_init(&priv->reg_mutex);
 	spin_lock_init(&priv->stats_lock);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 3dabbc99fbbc..108c831dd012 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -40,6 +40,7 @@ enum mt753x_id {
 /* Register for ARL global control */
 #define MT753X_AGC			0xc
 #define  LOCAL_EN			BIT(7)
+#define  AGC_L2LEN_CHK			BIT(4)
 
 /* Register for MAC forward control */
 #define MT753X_MFC			0x10
@@ -583,6 +584,7 @@ enum mt7531_clk_skew {
 #define MT753X_MTRAP			0x7804
 #define  MT7530_P5_PHY0_SEL		BIT(20)
 #define  MT7530_CHG_TRAP		BIT(16)
+#define  MT7530_LOOP_DET_DISABLE	BIT(14)
 #define  MT7530_P5_MAC_SEL		BIT(13)
 #define  MT7530_P6_DIS			BIT(8)
 #define  MT7530_P5_RGMII_MODE		BIT(7)
@@ -897,6 +899,9 @@ struct mt753x_info {
  * @p5_mode:		Holding the current mode of port 5 of the MT7530 switch
  * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
  *			has got SGMII
+ * @is_passthrough:	If this switch is to pass traffic between the upstream
+ *			(CPU) port and a second downstream switch, without
+ *			altering the DSA tag.
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @create_sgmii:	Pointer to function creating SGMII PCS instance(s)
  * @active_cpu_ports:	Holding the active CPU ports
@@ -921,6 +926,7 @@ struct mt7530_priv {
 	bool			p5_sgmii;
 	u8			mirror_rx;
 	u8			mirror_tx;
+	bool			is_passthrough;
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	struct mt753x_pcs	pcs[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
-- 
2.39.5

[PATCH v2 net-next 4/7] net: dsa: mediatek: support PLL setup on MMIO MT7530

[Caleb James DeLisle]

On MT7530 and MT7531, PHYs are an integral part of the switch. However,
on MT7530, certain configuration such as PLL setup is done via special
registers on one of the PHYs.

In an MMIO implementation, the MDIO bus is part of the switch itself.
This bus is already setup for PHY calibration so begin using it for
core switch PLL setup.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/dsa/mt7530.c | 15 +++++++++++++++
 drivers/net/dsa/mt7530.h |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b2344808457c..b152b73315d5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -77,6 +77,12 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
 	struct mii_bus *bus = priv->bus;
 	int ret;
 
+	if (!bus)
+		bus = priv->child_bus;
+
+	if (WARN_ON_ONCE(!bus))
+		return;
+
 	mt7530_mutex_lock(priv);
 
 	/* Write the desired MMD Devad */
@@ -112,6 +118,12 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
 	u32 val;
 	int ret;
 
+	if (!bus)
+		bus = priv->child_bus;
+
+	if (WARN_ON_ONCE(!bus))
+		return;
+
 	mt7530_mutex_lock(priv);
 
 	/* Write the desired MMD Devad */
@@ -2429,8 +2441,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	if (priv->irq_domain && !mnp)
 		mt7530_setup_mdio_irq(priv);
 
+	priv->child_bus = bus;
+
 	ret = devm_of_mdiobus_register(dev, bus, mnp);
 	if (ret) {
+		priv->child_bus = NULL;
 		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
 		if (priv->irq_domain && !mnp)
 			mt7530_free_mdio_irq(priv);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 108c831dd012..5b7394f6383c 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -886,6 +886,9 @@ struct mt753x_info {
  * @dev:		The device pointer
  * @ds:			The pointer to the dsa core structure
  * @bus:		The bus used for the device and built-in PHY
+ * @child_bus:		The bus created by the device, on MDIO implementations
+ *			this is bus indirectly accesses `bus`, on MMIO
+ *			implementations this bus is part of the switch.
  * @regmap:		The regmap instance representing all switch registers
  * @rstc:		The pointer to reset control used by MCM
  * @core_pwr:		The power supplied into the core
@@ -914,6 +917,7 @@ struct mt7530_priv {
 	struct device		*dev;
 	struct dsa_switch	*ds;
 	struct mii_bus		*bus;
+	struct mii_bus		*child_bus;
 	struct regmap		*regmap;
 	struct reset_control	*rstc;
 	struct regulator	*core_pwr;
-- 
2.39.5

[PATCH v2 net-next 5/7] net: dsa: mediatek: support MDIO switch downstream of MMIO switch

[Caleb James DeLisle]

The MT7530 / MT7531 can connect via either MDIO or MMIO. In MMIO mode
the switch provides its own MDIO bus which connects to its integrated
PHYs, but also can be connected to other things, such as another
switch.

If the MDIO bus contains another switch, then the upstream switch
will register but it will not setup the DSA tree because the
downstream switch is missing. Without the complete DSA tree,
mt753x_setup() will not be called on the switch and therefore
mt7530_setup_mdio() is never called so the downstream switch never
registers.

Update mt7530_setup_mdio() so that it can be called multiple times,
export the symbol, and call it again after dsa_register_switch()
from mt7988_probe().

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/dsa/mt7530-mmio.c |  9 ++++++++-
 drivers/net/dsa/mt7530.c      | 13 +++++++++----
 drivers/net/dsa/mt7530.h      | 10 ++++++++++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
index cf3bdb23a2d5..f387190a9539 100644
--- a/drivers/net/dsa/mt7530-mmio.c
+++ b/drivers/net/dsa/mt7530-mmio.c
@@ -61,7 +61,14 @@ mt7988_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regmap))
 		return PTR_ERR(priv->regmap);
 
-	return dsa_register_switch(priv->ds);
+	ret = dsa_register_switch(priv->ds);
+
+	if (ret)
+		return ret;
+
+	mt7530_setup_mdio(priv);
+
+	return 0;
 }
 
 static void mt7988_remove(struct platform_device *pdev)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b152b73315d5..fe93be1ece7d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2404,7 +2404,7 @@ mt7530_free_mdio_irq(struct mt7530_priv *priv)
 	}
 }
 
-static int
+int
 mt7530_setup_mdio(struct mt7530_priv *priv)
 {
 	struct device_node *mnp, *np = priv->dev->of_node;
@@ -2414,6 +2414,10 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	static int idx;
 	int ret = 0;
 
+	/* Already done */
+	if (priv->child_bus)
+		return 0;
+
 	mnp = of_get_child_by_name(np, "mdio");
 
 	if (mnp && !of_device_is_available(mnp))
@@ -2438,9 +2442,6 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	bus->parent = dev;
 	bus->phy_mask = ~ds->phys_mii_mask;
 
-	if (priv->irq_domain && !mnp)
-		mt7530_setup_mdio_irq(priv);
-
 	priv->child_bus = bus;
 
 	ret = devm_of_mdiobus_register(dev, bus, mnp);
@@ -2455,6 +2456,7 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	of_node_put(mnp);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mt7530_setup_mdio);
 
 static int
 mt7530_setup(struct dsa_switch *ds)
@@ -3285,6 +3287,9 @@ mt753x_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	if (priv->irq_domain && ds->user_mii_bus)
+		mt7530_setup_mdio_irq(priv);
+
 	/* Initialise the PCS devices */
 	for (i = 0; i < priv->ds->num_ports; i++) {
 		priv->pcs[i].pcs.ops = priv->info->pcs_ops;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5b7394f6383c..c86bc4bca29b 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -969,6 +969,16 @@ struct mt7530_hw_stats {
 int mt7530_probe_common(struct mt7530_priv *priv);
 void mt7530_remove_common(struct mt7530_priv *priv);
 
+/*
+ * When an MMIO based switch has an MDIO bus which contains a downstream MDIO
+ * switch, the MDIO bus is not normally setup until mt753x_setup() has been
+ * called. But mt753x_setup() is not called until DSA sees that every switch in
+ * the tree has been probed. Calling mt7530_setup_mdio() after at the end of
+ * the MMIO probe function allows the downstream switch to register, completing
+ * the tree.
+ */
+int mt7530_setup_mdio(struct mt7530_priv *priv);
+
 extern const struct mt753x_info mt753x_table[];
 
 #endif /* __MT7530_H */
-- 
2.39.5

[PATCH v2 net-next 6/7] dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221

[Caleb James DeLisle]

The EcoNet EN751221 MIPS SoC integrates an MT7530 switch memory-mapped
in the SoC register space. It is similar to the econet,en7528-switch
except that ports 0..3 are 10/100 only, port 4 has a gigabit PHY, and
port 5 has no PHY, but is in some implementations connected to a
downstream MT7530 multi-chip module via a TRGMII link.

The downstream MT7530 is similar to the MT7621 multi-chip module and
has all gigabit ports.

Add the econet,en751221-switch compatible with the same constraints as
the other built-in switches and also add the econet,en751221
multi-chip-module switch whose port 6 is configured as a DSA link port.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 .../bindings/net/dsa/mediatek,mt7530.yaml     | 43 ++++++++++++++++++-
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index e29fb8693498..106c4087528d 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -29,6 +29,12 @@ description: |
   and the switch registers are directly mapped into SoC's memory map rather than
   using MDIO. The DSA driver currently doesn't support MT7620 variants.
 
+  EcoNet EN751221 SoCs have a memory mapped MT7530 with 3x 10/100 PHYs and 1x
+  gigabit PHY. Some EN751221 chips such as EN7513G and EN7526G have a multi-chip
+  module implementation of the MT7530 as well as the on-die switch. In these
+  packages, the multi-chip module is connected to port 5 of the on-die switch
+  via a TRGMII link.
+
   There is only the standalone version of MT7531.
 
   Port 5 on MT7530 supports various configurations:
@@ -111,6 +117,14 @@ properties:
           Built-in switch of the EcoNet EN7528 SoC
         const: econet,en7528-switch
 
+      - description:
+          Built-in switch of EcoNet EN751221 SoC
+        const: econet,en751221-switch
+
+      - description:
+          Multi-chip module MT7530 in EN751221 SoC
+        const: econet,en751221
+
   reg:
     maxItems: 1
 
@@ -160,8 +174,8 @@ properties:
   mediatek,mcm:
     type: boolean
     description:
-      Used for MT7621AT, MT7621DAT, MT7621ST and MT7623AI SoCs which the MT7530
-      switch is a part of the multi-chip module.
+      Used for MT7621AT, MT7621DAT, MT7621ST, MT7623AI and EN751221 SoCs which
+      the MT7530 switch is a part of the multi-chip module.
 
   reset-gpios:
     description: |
@@ -223,6 +237,20 @@ $defs:
                 phy-mode:
                   const: internal
 
+  mt7530-passthrough-dsa-port:
+    patternProperties:
+      "^(ethernet-)?ports$":
+        patternProperties:
+          "^(ethernet-)?port@[0-6]$":
+            if:
+              required: [ link ]
+            then:
+              properties:
+                reg:
+                  const: 6
+                phy-mode:
+                  const: trgmii
+
   mt7530-dsa-port:
     patternProperties:
       "^(ethernet-)?ports$":
@@ -330,6 +358,7 @@ allOf:
             - airoha,en7581-switch
             - airoha,an7583-switch
             - econet,en7528-switch
+            - econet,en751221-switch
     then:
       $ref: "#/$defs/builtin-dsa-port"
       properties:
@@ -337,6 +366,16 @@ allOf:
         mediatek,mcm: false
         reset-names: false
 
+  - if:
+      properties:
+        compatible:
+          enum:
+            - econet,en751221
+    then:
+      $ref: "#/$defs/mt7530-passthrough-dsa-port"
+      required:
+        - mediatek,mcm
+
 unevaluatedProperties: false
 
 examples:
-- 
2.39.5

[PATCH v2 net-next 7/7] net: dsa: mediatek: support EN751221 switch

[Caleb James DeLisle]

The EcoNet EN751221 has either one, or two, onboard MT7530 switches.
Every implementation of this SoC has one switch on die with the CPU,
but some chips - notably those with a "G" in the name, have a second
MT7530 as an MCM module.

The on-die MT7530 has 4 FE ports and 1 GE port. The MCM switch has all
gigabit ports.

The MCM switch connects to the SoC switch via a TRGMII link from port
5 on the SoC switch to port 6 on the MCM switch. This link undergoes
calibration on startup.

All known devices with the MCM present do not make any use of the ports
on the on-die switch, it is put into "passthrough" mode in which all
traffic is shuttled between port 6 and port 5 without alteration of the
DSA tags.

Add support for both the on-die and the MCM switch, and calibration of
the TRGMII link between them.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/dsa/mt7530-mdio.c |   1 +
 drivers/net/dsa/mt7530-mmio.c |   1 +
 drivers/net/dsa/mt7530.c      | 335 +++++++++++++++++++++++++++++++++-
 drivers/net/dsa/mt7530.h      |  24 +++
 4 files changed, 357 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 24ef54f3db2a..2fd987ff8510 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -139,6 +139,7 @@ mt7531_create_sgmii(struct mt7530_priv *priv)
 }
 
 static const struct of_device_id mt7530_of_match[] = {
+	{ .compatible = "econet,en751221", .data = &mt753x_table[ID_EN751221_EXT], },
 	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
 	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
 	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
index f387190a9539..b77e3cb655aa 100644
--- a/drivers/net/dsa/mt7530-mmio.c
+++ b/drivers/net/dsa/mt7530-mmio.c
@@ -12,6 +12,7 @@
 static const struct of_device_id mt7988_of_match[] = {
 	{ .compatible = "airoha,an7583-switch", .data = &mt753x_table[ID_AN7583], },
 	{ .compatible = "airoha,en7581-switch", .data = &mt753x_table[ID_EN7581], },
+	{ .compatible = "econet,en751221-switch", .data = &mt753x_table[ID_EN751221], },
 	{ .compatible = "econet,en7528-switch", .data = &mt753x_table[ID_EN7528], },
 	{ .compatible = "mediatek,mt7988-switch", .data = &mt753x_table[ID_MT7988], },
 	{ /* sentinel */ }
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fe93be1ece7d..4f43454083b9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -49,6 +49,9 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
 static int
 mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
 {
+	if (priv->id == ID_EN751221)
+		return 12;
+
 	if (WARN_ON_ONCE(!priv->mdiodev))
 		return 0;
 
@@ -344,12 +347,19 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 	regmap_read(priv->regmap, MT753X_MTRAP, &xtal);
 	xtal &= MT7530_XTAL_MASK;
 
+	/* EN751221 on-die does not report clock speed in MTRAP */
+	if (priv->id == ID_EN751221)
+		xtal = MT7530_XTAL_25MHZ;
+
 	if (xtal == MT7530_XTAL_25MHZ)
 		ssc_delta = 0x57;
 	else
 		ssc_delta = 0x87;
 
-	if (priv->id == ID_MT7621) {
+	if (priv->id == ID_EN751221_EXT || priv->id == ID_EN751221) {
+		/* PLL frequency: 362.5Mhz */
+		ncpo1 = 0x1d00;
+	} else if (priv->id == ID_MT7621) {
 		/* PLL frequency: 125MHz: 1.0GBit */
 		if (xtal == MT7530_XTAL_40MHZ)
 			ncpo1 = 0x0640;
@@ -466,6 +476,251 @@ mt7531_pll_setup(struct mt7530_priv *priv)
 	usleep_range(25, 35);
 }
 
+static bool
+en751221_trgmii_cal_ok(struct mt7530_priv *rx, u32 reg)
+{
+	u32 val = 0;
+
+	regmap_read(rx->regmap, reg, &val);
+	regmap_write(rx->regmap, reg, val | EDGE_CHK);
+	regmap_write(rx->regmap, reg, val & ~EDGE_CHK);
+	regmap_read(rx->regmap, reg, &val);
+
+	return FIELD_GET(RD_VALUE_MASK, val) == TGMII_TD_PAT &&
+	       !FIELD_GET(RD_ERR_MASK, val);
+}
+
+static void
+en751221_trgmii_calibrate_direction(struct mt7530_priv *tx,
+				    struct mt7530_priv *rx,
+				    u8 *default_taps)
+{
+	int channel;
+
+	regmap_set_bits(tx->regmap, MT7530_TRGMII_TXCTRL, TRAIN_TXEN);
+
+	/* Put fail pattern on every channel */
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
+		regmap_update_bits(tx->regmap, EN7530_TRGMII_TD_CTRL(channel),
+				   TGMII_TD_PAT_MASK,
+				   FIELD_PREP(TGMII_TD_PAT_MASK,
+					      TGMII_TD_FAIL_PAT));
+
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++) {
+		u32 rx_reg = MT7530_TRGMII_RD(channel);
+		int best_tap = -1, best_range = -1;
+		int first = -1;
+		u8 dac;
+		char log[TD_TAP_MAX + 1];
+
+		/* Put test pattern on correct channel */
+		regmap_update_bits(tx->regmap, EN7530_TRGMII_TD_CTRL(channel),
+				   TGMII_TD_PAT_MASK,
+				   FIELD_PREP(TGMII_TD_PAT_MASK, TGMII_TD_PAT));
+
+		for (dac = 0; dac < TD_TAP_MAX; dac++) {
+			/* Set TAP */
+			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
+					   RD_TAP(dac));
+
+			/* Check if we can get a good read */
+			if (en751221_trgmii_cal_ok(rx, rx_reg)) {
+				log[dac] = '+';
+
+				if (first < 0)
+					first = dac;
+
+				if ((dac - first) > best_range) {
+					best_tap = (first + dac) / 2;
+					best_range = dac - first;
+				}
+			} else {
+				log[dac] = '.';
+				first = -1;
+			}
+		}
+
+		log[TD_TAP_MAX] = '\0';
+
+		if (best_tap > -1) {
+			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
+					   RD_TAP(best_tap));
+
+			log[best_tap] = 'X';
+
+			dev_info(rx->dev,
+				 "TRGMII lane %d: %s tap %d old %d\n",
+				 channel, log, best_tap, default_taps[channel]);
+		} else {
+			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
+					   RD_TAP(default_taps[channel]));
+
+			dev_warn(rx->dev,
+				 "TRGMII lane %d: %s calibration failed, preserving tap %u\n",
+				 channel, log, default_taps[channel]);
+		}
+
+		/* Return channel to failure pattern */
+		regmap_update_bits(tx->regmap, EN7530_TRGMII_TD_CTRL(channel),
+				   TGMII_TD_PAT_MASK,
+				   FIELD_PREP(TGMII_TD_PAT_MASK,
+					      TGMII_TD_FAIL_PAT));
+	}
+
+	regmap_clear_bits(tx->regmap, MT7530_TRGMII_TXCTRL, TRAIN_TXEN);
+}
+
+static void
+en751221_set_tx_drive(struct mt7530_priv *priv, u8 drvp, u8 drvn, u8 clk_drvp,
+		      u8 clk_drvn)
+{
+	int channel;
+
+	/* Tx driving for TRGMII path on SoC */
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
+		regmap_write(priv->regmap, MT7530_TRGMII_TD_ODT(channel),
+			     TD_DM_DRVP(drvp) | TD_DM_DRVN(drvn));
+
+	/* Undocumented 6th channel, probably drive strength for clock line. */
+	regmap_write(priv->regmap, MT7530_TRGMII_TD_ODT(5), TD_DM_DRVP(clk_drvp) |
+		     TD_DM_DRVN(clk_drvn));
+}
+
+static void
+en751221_trgmii_pair_setup(struct mt7530_priv *ext, int ext_port,
+			   struct mt7530_priv *ondie, int ondie_port)
+{
+	u8 default_taps_ondie[NUM_TRGMII_CTRL];
+	u8 default_taps_ext[NUM_TRGMII_CTRL];
+	u32 mcr_down, mcr_up;
+	int channel;
+	int reg_val;
+
+	/* BOTH: Put interfaces in a downed state */
+	mcr_down = PMCR_IFG_XMIT(PMCR_IFG_XMIT_64) | PMCR_MAC_MODE |
+		   MT7530_FORCE_MODE | PMCR_MAC_RX_EN | PMCR_BACKOFF_EN |
+		   PMCR_BACKPR_EN | PMCR_FORCE_SPEED_1000 | PMCR_FORCE_FDX;
+	regmap_write(ondie->regmap,  MT753X_PMCR_P(ondie_port), mcr_down);
+	regmap_write(ext->regmap, MT753X_PMCR_P(ext_port), mcr_down);
+	usleep_range(5000, 6000);
+
+	/* BOTH: Reset TX */
+	regmap_set_bits(ext->regmap, MT7530_TRGMII_TXCTRL, TX_RST);
+	regmap_set_bits(ondie->regmap, MT7530_TRGMII_TXCTRL, TX_RST);
+	usleep_range(5000, 6000);
+	regmap_clear_bits(ext->regmap, MT7530_TRGMII_TXCTRL, TX_RST);
+	regmap_clear_bits(ondie->regmap, MT7530_TRGMII_TXCTRL, TX_RST);
+
+	/* BOTH: Reset RX controllers down */
+	regmap_set_bits(ext->regmap, MT7530_TRGMII_RCK_CTRL, RX_RST);
+	regmap_set_bits(ondie->regmap, MT7530_TRGMII_RCK_CTRL, RX_RST);
+
+	/* BOTH: Set TX drive strength, MCM uses more */
+	en751221_set_tx_drive(ondie, 8, 8, 7, 7);
+	en751221_set_tx_drive(ext, 11, 11, 15, 15);
+
+	/* BOTH: Reset RX controllers up */
+	regmap_clear_bits(ext->regmap, MT7530_TRGMII_RCK_CTRL, RX_RST);
+	regmap_clear_bits(ondie->regmap, MT7530_TRGMII_RCK_CTRL, RX_RST);
+
+	/* BOTH: Ports up */
+	mcr_up = mcr_down | PMCR_MAC_TX_EN | PMCR_FORCE_LNK;
+	regmap_write(ondie->regmap, MT753X_PMCR_P(ondie_port), mcr_up);
+	regmap_write(ext->regmap, MT753X_PMCR_P(ext_port), mcr_up);
+
+	/* MCM: Set Ext->SoC TX delay to 0 */
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
+		regmap_clear_bits(ext->regmap, EN7530_TRGMII_TD_CTRL(channel),
+				  TGMII_TD_TAP_MASK);
+
+	/* SOC: ODT */
+	regmap_set_bits(ondie->regmap, MT7530_TRGMII_RCK_RTT,
+			DQS1_GATE | DQS0_GATE | EN751221_B17);
+
+	/* SOC: Undocumented */
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
+		regmap_write(ondie->regmap,  (0x7a14 + channel * 8),
+			     0x3227700);
+
+	/* MCM: Spread spectrum clock*/
+	core_clear(ext, CORE_PLL_GROUP8, RG_LCDDS_SSC_EN);
+
+	/* BOTH: Zero clock delay */
+	regmap_clear_bits(ext->regmap, MT7530_TRGMII_RCK_CTRL,
+			  DQSI0_TAP_MASK);
+	regmap_clear_bits(ondie->regmap, MT7530_TRGMII_RCK_CTRL,
+			  DQSI0_TAP_MASK);
+
+	/* BOTH: Collect and then zero every RX TAP */
+	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++) {
+		reg_val = 0;
+		regmap_read(ext->regmap, MT7530_TRGMII_RD(channel), &reg_val);
+		default_taps_ext[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
+		regmap_clear_bits(ext->regmap, MT7530_TRGMII_RD(channel),
+				  RD_TAP_MASK);
+
+		reg_val = 0;
+		regmap_read(ondie->regmap, MT7530_TRGMII_RD(channel), &reg_val);
+		default_taps_ondie[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
+		regmap_clear_bits(ondie->regmap, MT7530_TRGMII_RD(channel),
+				  RD_TAP_MASK);
+	}
+
+	en751221_trgmii_calibrate_direction(ondie, ext, default_taps_ondie);
+	en751221_trgmii_calibrate_direction(ext, ondie, default_taps_ext);
+
+	dev_info(ondie->dev, "TRGMII inter-switch link initialized\n");
+}
+
+static void
+mt7530_cal_interswitch_trgmii(struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+	struct dsa_switch *peer_ds;
+	struct dsa_port *peer_dp;
+	struct mt7530_priv *peer;
+	struct mt7530_priv *priv;
+	struct dsa_link *dl;
+	u32 pmsr = 0;
+
+	priv = ds->priv;
+
+	if (priv->id != ID_EN751221_EXT)
+		return;
+
+	if (dp->type != DSA_PORT_TYPE_DSA)
+		return;
+
+	regmap_read(priv->regmap, MT7530_PMSR_P(dp->index), &pmsr);
+
+	if (!(pmsr & PMSR_LINK))
+		return;
+
+	list_for_each_entry(dl, &ds->dst->rtable, list) {
+		if (dl->dp != dp)
+			continue;
+
+		peer_dp = dl->link_dp;
+
+		if (peer_dp->type != DSA_PORT_TYPE_DSA)
+			continue;
+
+		peer_ds = dl->link_dp->ds;
+
+		if (!of_device_is_compatible(peer_ds->dev->of_node,
+					     "econet,en751221-switch"))
+			continue;
+
+		peer = peer_ds->priv;
+
+		if (peer->id != ID_EN751221)
+			continue;
+
+		en751221_trgmii_pair_setup(priv, dp->index, peer,
+					   peer_dp->index);
+	}
+}
+
 static void
 mt7530_mib_reset(struct dsa_switch *ds)
 {
@@ -1408,7 +1663,8 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 
 	mutex_unlock(&priv->reg_mutex);
 
-	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
+	    priv->id != ID_EN751221 && priv->id != ID_EN751221_EXT)
 		return 0;
 
 	if (port == 5)
@@ -1435,7 +1691,8 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
 
 	mutex_unlock(&priv->reg_mutex);
 
-	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
+	    priv->id != ID_EN751221 && priv->id != ID_EN751221_EXT)
 		return;
 
 	/* Do not set MT7530_P5_DIS when port 5 is being used for PHY muxing. */
@@ -2560,6 +2817,12 @@ mt7530_setup(struct dsa_switch *ds)
 			   MT7530_CHG_TRAP | MT7530_PHY_INDIRECT_ACCESS,
 			   MT7530_CHG_TRAP);
 
+	/* EN751221 MCM starts on the wrong frequency */
+	if (priv->id == ID_EN751221_EXT)
+		regmap_update_bits(priv->regmap, MT753X_MTRAP,
+				   MT7530_CHG_TRAP | MT7530_CK_SEL,
+				   MT7530_CHG_TRAP);
+
 	if ((val & MT7530_XTAL_MASK) == MT7530_XTAL_40MHZ)
 		mt7530_pll_setup(priv);
 
@@ -2980,6 +3243,44 @@ static void en7528_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
+static void en751221_mac_port_get_caps(struct dsa_switch *ds, int port,
+				       struct phylink_config *config)
+{
+	switch (port) {
+	/* EN751221 ports 0..3 are connected to the integrated FE PHYs. */
+	case 0 ... 3:
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+
+		config->mac_capabilities |= MAC_10 | MAC_100;
+		break;
+
+	/* Port 4 is connected to the standalone EN7512/EN7521 GPHY. */
+	case 4:
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+
+		config->mac_capabilities |= MAC_10 | MAC_100 | MAC_1000FD;
+		break;
+
+	/* Port 5 is the 1 Gbit/s TRGMII cascade to the external MT7530. */
+	case 5:
+		__set_bit(PHY_INTERFACE_MODE_TRGMII,
+			  config->supported_interfaces);
+
+		config->mac_capabilities |= MAC_1000FD;
+		break;
+
+	/* Port 6 is the 1 Gbit/s CPU link to GDM1. */
+	case 6:
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+
+		config->mac_capabilities |= MAC_1000FD;
+		break;
+	}
+}
+
 static void
 mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -3129,6 +3430,9 @@ static void mt753x_phylink_mac_link_up(struct phylink_config *config,
 	}
 
 	regmap_set_bits(priv->regmap, MT753X_PMCR_P(dp->index), mcr);
+
+	if (interface == PHY_INTERFACE_MODE_TRGMII)
+		mt7530_cal_interswitch_trgmii(dp);
 }
 
 static void mt753x_phylink_mac_disable_tx_lpi(struct phylink_config *config)
@@ -3382,7 +3686,8 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
 	 * interface is up. NOTE: "CPU port" can also mean an upstream DSA link.
 	 */
 	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
-	    priv->id != ID_EN7528)
+	    priv->id != ID_EN7528 && priv->id != ID_EN751221 &&
+	    priv->id != ID_EN751221_EXT)
 		return;
 
 	mask = BIT(cpu_dp->index);
@@ -3670,6 +3975,28 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c45 = mt7531_ind_c45_phy_write,
 		.mac_port_get_caps = en7528_mac_port_get_caps,
 	},
+	[ID_EN751221] = {
+		.id = ID_EN751221,
+		.pcs_ops = &mt7530_pcs_ops,
+		.sw_setup = mt7988_setup,
+		.phy_read_c22 = mt7531_ind_c22_phy_read,
+		.phy_write_c22 = mt7531_ind_c22_phy_write,
+		.phy_read_c45 = mt7531_ind_c45_phy_read,
+		.phy_write_c45 = mt7531_ind_c45_phy_write,
+		.mac_port_get_caps = en751221_mac_port_get_caps,
+		.mac_port_config = mt7530_mac_config,
+	},
+	[ID_EN751221_EXT] = {
+		.id = ID_EN751221_EXT,
+		.pcs_ops = &mt7530_pcs_ops,
+		.sw_setup = mt7530_setup,
+		.phy_read_c22 = mt7530_phy_read_c22,
+		.phy_write_c22 = mt7530_phy_write_c22,
+		.phy_read_c45 = mt7530_phy_read_c45,
+		.phy_write_c45 = mt7530_phy_write_c45,
+		.mac_port_get_caps = mt7530_mac_port_get_caps,
+		.mac_port_config = mt7530_mac_config,
+	},
 };
 EXPORT_SYMBOL_GPL(mt753x_table);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index c86bc4bca29b..1a4329749c0f 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -24,6 +24,8 @@ enum mt753x_id {
 	ID_EN7581 = 4,
 	ID_AN7583 = 5,
 	ID_EN7528 = 6,
+	ID_EN751221 = 7,
+	ID_EN751221_EXT = 8,
 };
 
 #define	NUM_TRGMII_CTRL			5
@@ -356,6 +358,9 @@ enum mt7530_vlan_port_acc_frm {
 #define MT753X_PMCR_P(x)		(0x3000 + ((x) * 0x100))
 #define  PMCR_IFG_XMIT_MASK		GENMASK(19, 18)
 #define  PMCR_IFG_XMIT(x)		FIELD_PREP(PMCR_IFG_XMIT_MASK, x)
+#define    PMCR_IFG_XMIT_96		0
+#define    PMCR_IFG_XMIT_RAND		1
+#define    PMCR_IFG_XMIT_64		2
 #define  PMCR_EXT_PHY			BIT(17)
 #define  PMCR_MAC_MODE			BIT(16)
 #define  MT7530_FORCE_MODE		BIT(15)
@@ -584,6 +589,7 @@ enum mt7531_clk_skew {
 #define MT753X_MTRAP			0x7804
 #define  MT7530_P5_PHY0_SEL		BIT(20)
 #define  MT7530_CHG_TRAP		BIT(16)
+#define  MT7530_CK_SEL			BIT(15)
 #define  MT7530_LOOP_DET_DISABLE	BIT(14)
 #define  MT7530_P5_MAC_SEL		BIT(13)
 #define  MT7530_P6_DIS			BIT(8)
@@ -601,6 +607,8 @@ enum mt7531_xtal_fsel {
 /* Register for TOP signal control */
 #define MT7530_TOP_SIG_CTRL		0x7808
 #define  TOP_SIG_CTRL_NORMAL		(BIT(17) | BIT(16))
+/* Undocumented */
+#define  TOP_SIG_CTRL_B0		BIT(0)
 
 #define MT7531_TOP_SIG_SR		0x780c
 #define  PAD_DUAL_SGMII_EN		BIT(1)
@@ -647,18 +655,31 @@ enum mt7531_xtal_fsel {
 #define MT7530_TRGMII_RCK_RTT		0x7a04
 #define  DQS1_GATE			BIT(31)
 #define  DQS0_GATE			BIT(30)
+/* Undocumented */
+#define  EN751221_B17			BIT(17)
 
 #define MT7530_TRGMII_RD(x)		(0x7a10 + (x) * 8)
 #define  BSLIP_EN			BIT(31)
 #define  EDGE_CHK			BIT(30)
+#define  RD_VALUE_MASK			GENMASK(23, 16)
+#define  RD_ERR_MASK			GENMASK(11, 8)
 #define  RD_TAP_MASK			GENMASK(6, 0)
 #define  RD_TAP(x)			FIELD_PREP(RD_TAP_MASK, x)
+/* Training does not try anything beyond this */
+#define  TD_TAP_MAX			64
 
 #define MT7530_TRGMII_TXCTRL		0x7a40
 #define  TRAIN_TXEN			BIT(31)
 #define  TXC_INV			BIT(30)
 #define  TX_RST				BIT(28)
 
+#define EN7530_TRGMII_TD_CTRL(x)	(0x7a50 + (x) * 8)
+#define   TGMII_TD_TAP_MASK		GENMASK(11, 8)
+#define   TGMII_TD_PAT_MASK		GENMASK(7, 0)
+/* Use a 01010101 bit pattern */
+#define   TGMII_TD_PAT			0x55
+#define   TGMII_TD_FAIL_PAT		0xaa
+
 #define MT7530_TRGMII_TD_ODT(i)		(0x7a54 + 8 * (i))
 #define  TD_DM_DRVP_MASK		GENMASK(3, 0)
 #define  TD_DM_DRVP(x)			FIELD_PREP(TD_DM_DRVP_MASK, x)
@@ -764,6 +785,9 @@ enum mt7531_xtal_fsel {
 #define  RG_LCCDS_C(x)			FIELD_PREP(RG_LCCDS_C_MASK, x)
 #define  RG_LCDDS_PCW_NCPO_CHG		BIT(3)
 
+#define CORE_PLL_GROUP8			0x407
+#define  RG_LCDDS_SSC_EN		BIT(10)
+
 #define CORE_PLL_GROUP10		0x409
 #define  RG_LCDDS_SSC_DELTA_MASK	GENMASK(11, 0)
 #define  RG_LCDDS_SSC_DELTA(x)		FIELD_PREP(RG_LCDDS_SSC_DELTA_MASK, x)
-- 
2.39.5

Re: [PATCH v2 net-next 1/7] net: dsa: mt7530: get ctrl phy addr using a function

[netdev-bot+sashiko]

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

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

- [Low] mt753x_ctrl_phy_addr() (drivers/net/dsa/mt7530.c:49-57) reports
  the !priv->mdiodev case only via WARN_ON_ONCE() and then returns 0 — a…
- [Low] The commit message states the control-PHY address is found by
  'taking the address of mdiodev (the switch) add 1 and modulo 31', but…

--

net: dsa: mt7530: get ctrl phy addr using a function

This replaces the MT753X_CTRL_PHY_ADDR() macro with a new helper,
mt753x_ctrl_phy_addr(), which warns and falls back when priv->mdiodev is
NULL. The intent is to give MMIO based MT7530 implementations a place to
supply their own control PHY address.

A couple of questions below.

>     The MDIO MT7530 has 5 integrated PHYs, of these, the first PHY has
>     the additional registers that are used for such things as configuring
>     the clock configuration for the switch. The switch is typically on
>     MDIO address 31 and the PHYs are 0, 1, 2, 3, and 4 with 0 being the
>     special one. So taking the address of mdiodev (the switch) add 1 and
>     modulo 31 finds the first PHY.

[Severity: Low]
Should "modulo 31" read "modulo 32" here?  The helper computes:

	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);

and include/linux/phy.h has:

	#define PHY_MAX_ADDR	32

so the mask is 31 and the arithmetic is modulo 32.  For the typical strap
described in the message, (31 + 1) % 32 == 0, which is the first integrated
PHY, whereas (31 + 1) % 31 == 1.  Perhaps "modulo 32", or "masked with 31",
would describe it better.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 14692497e1a61..54d931002bc14 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -46,6 +46,16 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
>  	MIB_DESC(1, MT7530_PORT_MIB_RX_ARL_DROP, "RxArlDrop"),
>  };
>  
> +static int
> +mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
> +{
> +	if (WARN_ON_ONCE(!priv->mdiodev))
> +		return 0;
> +
> +	/* Default is 1st PHY */
> +	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);
> +}
> +

[Severity: Low]
Is 0 a usable way to signal the !priv->mdiodev case?  0 is also the normal
result for a switch strapped at MDIO address 31, so callers cannot tell the
two apart, and none of the three call sites checks the return value.

There is a related question about what happens after the warning fires.
priv->bus and priv->mdiodev are only assigned together in mt7530-mdio.c,
while mt7530-mmio.c mt7988_probe() does:

	priv->bus = NULL;

and never sets priv->mdiodev.  So the WARN_ON_ONCE() branch and a NULL
priv->bus are the same condition, and in core_write()/core_rmw() at this
commit the warning falls through into the bus dereference below rather than
aborting.  Note mt7530_mutex_lock()/mt7530_mutex_unlock() in the same path
do guard on priv->bus.

I could not find an in-tree path that reaches the warning at this commit:
core_write()/core_rmw() are reached from mt7530_pll_setup() and
mt7530_setup_port5/6() (ID_MT7530/ID_MT7621), and mt7531_setup() runs for
ID_MT7531, all of which are probed through mt7530-mdio.c.  Later in the
series, "net: dsa: mediatek: support PLL setup on MMIO MT7530" adds

	if (!bus)
		bus = priv->child_bus;

	if (WARN_ON_ONCE(!bus))
		return;

to both functions, and mt753x_ctrl_phy_addr() gains an ID_EN751221 case
returning 12, so both aspects are handled inside the same series.  Would it
still be preferable to return a negative errno here and check it at the call
sites, so the fallback value is not a valid PHY address?

>  static void
>  mt7530_mutex_lock(struct mt7530_priv *priv)
>  {
> @@ -63,32 +73,30 @@ mt7530_mutex_unlock(struct mt7530_priv *priv)
>  static void
>  core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  {
> +	int ctl_phy = mt753x_ctrl_phy_addr(priv);
>  	struct mii_bus *bus = priv->bus;
>  	int ret;
>  
>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */
> -	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
> -			 MII_MMD_CTRL, MDIO_MMD_VEND2);
> +	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
>  	if (ret < 0)
>  		goto err;

[ ... ]

> @@ -99,6 +107,7 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  static void
>  core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
>  {
> +	int ctl_phy = mt753x_ctrl_phy_addr(priv);
>  	struct mii_bus *bus = priv->bus;
>  	u32 val;
>  	int ret;

[ ... ]

> @@ -2674,8 +2679,11 @@ mt7531_setup(struct dsa_switch *ds)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  	u32 val, id;
> +	int ctl_phy;
>  	int ret, i;
>  
> +	ctl_phy = mt753x_ctrl_phy_addr(priv);
> +

Same question here: ctl_phy is used unchecked as the address for the
CORE_PLL_GROUP4 read/write and as the base of the EEE advertisement loop,
so a fallback of 0 would silently target PHY address 0 through 4.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 2/7] dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

[netdev-bot+sashiko]

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

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

- [Low] The added `description` paragraph in
  Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml (lines…
- [Low] Concern (retained for policy combination, assessed as a false
  positive): the binding advertises a passthrough mode that…

--

dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

This adds a paragraph to the mediatek,mt7530 binding description explaining
that MT7530/MT7531 switches can be operated in a "passthrough" mode where
traffic moves unconditionally between the CPU port and a DSA link port to a
second switch. The paragraph also states that in this mode the upstream
switch may have no other ports or switch configuration.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index 90b3582b76190..e29fb86934981 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -73,6 +73,13 @@ description: |
>  
>        Check out example 7.
>  
> +  The MT7530 and MT7531 switches are capable of being operated in "passthrough"
> +  mode. In this mode, traffic is unconditionally passed between the CPU port
> +  and the DSA port which links to a second switch. To use passthrough mode, the
> +  upstream switch is configured with a DSA link to the downstream switch. In
> +  passthrough mode the upstream switch is restricted, with no other ports or
> +  switch configuration allowed.
> +

[Severity: Low]
The last sentence states a hard device tree constraint, but it only exists
as prose.  Should this restriction also be expressed in the
machine-checkable part of the schema?

As it stands there is no new property, no if/then under allOf, and no $defs
entry that limits the port set when a port carries a link phandle.  The port
container in this same file is permissive:

  patternProperties:
    "^(ethernet-)?ports$":
      type: object
      additionalProperties: true

      patternProperties:
        "^(ethernet-)?port@[0-6]$":
          type: object
          additionalProperties: true

so a device tree describing an upstream MT7530/MT7531 with a CPU port, a DSA
link port and extra user ports, plus gpio-controller/interrupt-controller,
still passes dt_binding_check.  The layout is instead rejected at probe time
by mt753x_check_passthrough() in drivers/net/dsa/mt7530.c, added by the
following patch in the series:

	if (passthrough_ports > 2 ||
	    (passthrough_ports == 2 && enabled_ports != 2)
	) {
		dev_err(dev, "Only two ports allowed in passthrough mode\n");
		return -EINVAL;

Two smaller points on the same paragraph:

Every other topology description in this description block ends with a
pointer to a validated example ("Check out example 7." just above).  Could
this one gain an example too, so the passthrough layout is actually exercised
by dt_binding_check?

The per-port reg description further down still reads:

          reg:
            description:
              Port address described must be 5 or 6 for CPU port and from 0 to 5
              for user ports.

which never mentions the DSA link port this new mode depends on.  The later
patch in the series constrains that link port to reg = 6 via the
mt7530-passthrough-dsa-port $def, which does not match the text above.
Should the reg description be updated at the same time?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 3/7] net: dsa: mediatek: add support for passthrough mode

[netdev-bot+sashiko]

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

Critical: 0 · High: 5 · Medium: 4 · Low: 4

- [High] mt753x_check_passthrough() counts any available port node that
  has either an "ethernet" (CPU/conduit) or a "link" (DSA) phandle into…
- [High] mt7530_passthrough_switch_ops omits .port_change_mtu,
  .port_max_mtu, .port_vlan_add/del, .port_fdb_add/del,…
- [High] The patch fixes only mt7530_port_enable() to route user ports
  via the *local* upstream port (dsa_switch_upstream_port()); the other…
- [High] The passthrough PVC programming of the upstream/CPU port is dead
  code: mt753x_cpu_port_enable() writes MT7530_PVC_P(port) =…
- [High] net: dsa: passthrough switch unintentionally traps link-local
  frames, breaking transparency
- [Medium] The MTK DSA tag carries no switch identifier and mtk_tag_rcv()
  resolves the source port with a hardcoded switch index of 0 (`skb->dev…
- [Medium] mt7530_port_enable() now derives the CPU/upstream port from
  the switch-global helper dsa_switch_upstream_port(ds) instead of the…
- [Medium] MT7530_LOOP_DET_DISABLE (BIT(14) of MT753X_MTRAP, 0x7804) is
  set unconditionally in the shared mt753x_cpu_port_enable() passthrough…
- [Medium] In the new DSA-link branch of mt7530_port_enable(), the
  upstream port's forwarding matrix is programmed with a blind full-…
- [Low] The patch silently deletes the `if (!dn) { dev_err(ds->dev,
  "parent OF node of DSA conduit not found"); return -EINVAL; }` check…
- [Low] Concern as filed ("passthrough mode is undocumented; no update to
  Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml; no…
- [Low] mt7530_port_change_mtu() still carries the comment "Because the
  switch only has a global RX length register, only allowing CPU port…
- [Low] Style defects in the newly added mt753x_check_passthrough(): a
  multi-line comment whose first line is empty (`/*` alone), which…

--

net: dsa: mediatek: add support for passthrough mode

This adds a "passthrough" mode to the mt7530 driver where an upstream
MT7530 forwards all traffic between its CPU port and a port connected to
a second, downstream MT7530 without altering the MediaTek DSA tag.  The
mode is detected from the device tree topology, and a reduced
dsa_switch_ops table is used for the passthrough switch.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 54d931002bc14..b2344808457cb 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1286,13 +1286,41 @@ mt753x_trap_frames(struct mt7530_priv *priv)
>  				   TO_CPU_FW_CPU_ONLY);
>  }
>  
> +static int
> +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);
> +
> +/* If this switch is downstream of another switch that is in passthrough mode,
> + * the "CPU" port is actually a DSA port.
> + */
>  static void
>  mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  
> -	/* Enable Mediatek header mode on the cpu port */
> -	regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
> +	if (priv->is_passthrough) {
> +		/* Disable parsing of the DSA tag, it will be forwarded blindly
> +		 * to the downstream switch.
> +		 */
> +		regmap_write(priv->regmap, MT7530_PVC_P(port),
> +			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
> +			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));

[Severity: High]
While mt753x_cpu_port_enable() is being taught to leave the tag
alone in passthrough mode, the rest of mt7530_setup() /
mt7531_setup_common() is unchanged, and both still call
mt753x_trap_frames(priv) unconditionally.  That helper programs
MT753X_BPC, MT753X_RGAC1 and MT753X_RGAC2 so that 802.1X PAE frames,
BPDUs and the 01:80:c2:00:00:0{1,2,3,e} group DAs are forwarded with
TO_CPU_FW_CPU_ONLY, i.e. redirected to the CPU port by the ARL
independently of the port matrix.

On a switch whose only job is to be a transparent pipe between its
CPU port and the downstream switch, is that what we want?  Two
concerns:

- Link-local frames arriving on the downstream DSA link are diverted
  to the CPU port by the trap rules rather than following
  priv->ports[].pm, so a frame that the downstream switch tagged for
  a specific user port can reach the conduit through a path this
  patch does not configure; and link-local frames the CPU sends
  toward the downstream switch can be matched by the same rules and
  bounced back to the CPU instead of egressing on the link port.

- The trap rules also force the egress tag state
  (PAE_EG_TAG(MT7530_VLAN_EG_UNTAGGED) / BPDU_EG_TAG(...)), which
  directly contradicts the VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
  PVC_EG_TAG(MT7530_VLAN_EG_DISABLED) configuration this hunk
  installs on the upstream port, so for exactly the frames that STP
  and 802.1X depend on, the passthrough switch is no longer
  transparent.

Since the passthrough switch has no user ports and no
.port_stp_state_set in mt7530_passthrough_switch_ops, nothing in this
driver consumes those trapped frames locally either.

Should mt753x_trap_frames() be skipped entirely when
priv->is_passthrough, or should the forwarding be changed so the
frames still follow the port matrix?  If the current behaviour is
intentional and harmless on the EN751221 topology, could the commit
message explain why global link-local trapping is safe on a switch
that is supposed to pass all traffic through untouched?

[Severity: High]
Does this PVC_EG_TAG value survive?  Both callers of
mt753x_cpu_port_enable() are inside the per-port setup loop, and in the same
iteration, right after the call, the loop unconditionally rewrites the same
field:

drivers/net/dsa/mt7530.c:mt7530_setup() {
    ...
        if (dsa_is_upstream_port(ds, i)) {
                mt753x_cpu_port_enable(ds, i);
        } else {
                mt7530_port_disable(ds, i);
                ...
        }

        /* Enable consistent egress tag */
        regmap_update_bits(priv->regmap, MT7530_PVC_P(i),
                           PVC_EG_TAG_MASK,
                           PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
    ...
}

mt7531_setup_common() has the identical sequence.  PVC_EG_TAG_MASK is
GENMASK(10, 8) and MT7530_VLAN_EG_DISABLED is 0, so the field that was just
cleared is set back to EG_CONSISTENT on the upstream port.  VLAN_ATTR lives
in GENMASK(7, 6) and survives, which makes this silent.

The downstream DSA link keeps EG_DISABLED because it is programmed later from
mt7530_port_enable(), so the two ends of the passthrough path end up
asymmetric, unlike the commit message description:

  "the main switch has its CPU port, and the port to the second switch, both
  configured as MT7530_VLAN_EG_DISABLED and MT7530_VLAN_TRANSPARENT"

Should the EG_CONSISTENT update skip upstream ports in passthrough mode?

> +
> +		/* The port is not configured to parse DSA tags, so they are
> +		 * liable to be confused for length fields, so length check is
> +		 * disabled.
> +		 */
> +		regmap_clear_bits(priv->regmap, MT753X_AGC, AGC_L2LEN_CHK);
> +
> +		/* In passthrough mode, MTU is only enforced downstream */
> +		mt7530_port_change_mtu(ds, port, MT7530_MAX_MTU);
> +
> +		/* Loop detection has no value in passthrough mode */
> +		regmap_set_bits(priv->regmap, MT753X_MTRAP,
> +				MT7530_LOOP_DET_DISABLE);

[Severity: Medium]
Is this MTRAP write safe on the MT7531 generation?  mt753x_cpu_port_enable()
is also called from mt7531_setup_common() (MT7531/MT7988/EN7581/AN7583/
EN7528), and priv->is_passthrough comes purely from the DT topology, so there
is no priv->id guard here.

Every other MT7530_*-prefixed access to this register in the driver is gated,
for example in mt7530_port_enable()/mt7530_port_disable():

	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
		regmap_clear_bits(priv->regmap, MT753X_MTRAP, ...);

and the bit definitions for 0x7804 collide across generations:

  #define  MT7530_P6_DIS			BIT(8)
  #define  MT7531_CHG_STRAP		BIT(8)

Should MT7530_LOOP_DET_DISABLE only be set when priv->id is ID_MT7530 or
ID_MT7621?

> +	} else {
> +		/* Not passthrough, enable DSA tag handling on CPU port. */
> +		regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
> +	}
>  
>  	/* Enable flooding on the CPU port */
>  	regmap_set_bits(priv->regmap, MT753X_MFC,
> @@ -1322,6 +1350,7 @@ static int
>  mt7530_port_enable(struct dsa_switch *ds, int port,
>  		   struct phy_device *phy)
>  {
> +	int upstream_pt = dsa_switch_upstream_port(ds);
>  	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct mt7530_priv *priv = ds->priv;
>  
> @@ -1332,13 +1361,38 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  	 * bridge.
>  	 */
>  	if (dsa_port_is_user(dp)) {
> -		struct dsa_port *cpu_dp = dp->cpu_dp;
> +		priv->ports[port].pm |= PCR_MATRIX(BIT(upstream_pt));
> +

[Severity: Medium]
Can switching from dp->cpu_dp->index to dsa_switch_upstream_port(ds) lose
per-port conduit affinity?  The helper answers for the switch, not for this
port:

include/net/dsa.h:dsa_switch_upstream_port() {
	dsa_switch_for_each_available_port(dp, ds) {
		return dsa_upstream_port(ds, dp->index);
	}
	return ds->num_ports;
}

On MT7531 this driver supports per-port conduits via
mt753x_port_change_conduit(), which only clears the old per-port bit:

	priv->ports[port].pm &= ~PCR_MATRIX(BIT(dp->cpu_dp->index));
	priv->ports[port].pm |= PCR_MATRIX(BIT(new_cpu_dp->index));

So after "ip link set swp0 type dsa conduit eth1" and then bringing swp1 up,
does swp1 get the bit of swp0's new conduit ORed in permanently, leaving two
CPU-port bits in priv->ports[swp1].pm?  Would dsa_upstream_port(ds, port)
work for passthrough while keeping the per-port affinity?

> +	} else if (dsa_port_is_dsa(dp) && dp->index != upstream_pt) {
> +		priv->ports[port].pm |= PCR_MATRIX(BIT(upstream_pt));
> +
> +		/* Should not happen */
> +		WARN_ON_ONCE(!priv->is_passthrough);
> +
> +		/* We are passing through to a downstream switch so we set both
> +		 * CPU and downstream link to pass traffic untouched so that
> +		 * the STAG from the downstream switch will pass to the upstream.
> +		 */
> +		regmap_write(priv->regmap, MT7530_PVC_P(port),
> +			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
> +			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));
> +
> +		/* We let the downstream switch flood */
> +		regmap_set_bits(priv->regmap, MT753X_MFC, BC_FFP(BIT(port)) |
> +				UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
>  
> -		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
> +		/* Make the upstream port able to connect to the DSA port.
> +		 * This must be explicit because PORT_SPEC_TAG is unset.
> +		 */
> +		regmap_write(priv->regmap, MT7530_PCR_P(upstream_pt),
> +			     PCR_MATRIX(BIT(port)));

[Severity: Medium]
Does this full-register write clobber the upstream port's VLAN mode?
mt753x_cpu_port_enable() programmed the same register a moment earlier:

	regmap_update_bits(priv->regmap, MT7530_PCR_P(port),
			   PCR_PORT_VLAN_MASK, MT7530_PORT_FALLBACK_MODE);

A regmap_write() of PCR_P(upstream_pt) with only PCR_MATRIX() set zeroes
PCR_PORT_VLAN_MASK (and PORT_PRI/mirror bits) on that port.

It also bypasses priv->ports[upstream_pt].pm, which every other matrix
writer in this driver treats as the authoritative shadow
(mt7530_port_enable, mt7530_port_disable, mt7530_update_port_member,
mt753x_port_change_conduit).

Would regmap_update_bits(..., PCR_MATRIX_MASK, ...) plus updating
priv->ports[upstream_pt].pm keep the shadow and hardware in sync here?

>  	}
>  	priv->ports[port].enable = true;
> -	regmap_update_bits(priv->regmap, MT7530_PCR_P(port), PCR_MATRIX_MASK,
> -			   priv->ports[port].pm);
> +
> +	/* In passthrough mode, CPU port mask is set above. */
> +	if (!(priv->is_passthrough && dp->index == upstream_pt))
> +		regmap_update_bits(priv->regmap, MT7530_PCR_P(port),
> +				   PCR_MATRIX_MASK, priv->ports[port].pm);

[Severity: Medium]
This is the same shadow concern as above: with this branch suppressed, is
there any remaining path that pushes priv->ports[upstream_pt].pm to
hardware?  mt7530_port_disable() is still in
mt7530_passthrough_switch_ops and does:

	regmap_update_bits(priv->regmap, MT7530_PCR_P(port), PCR_MATRIX_MASK,
			   PCR_MATRIX_CLR);

so once the upstream port's matrix is cleared, can it be restored from a
shadow that was never updated?

[Severity: High]
Only mt7530_port_enable() is converted to the local upstream port; do the
other consumers still treat a remote CPU port index as a local port number?

mt7530_update_port_member() runs on every bridge join/leave and is part of
the full ops table used by the downstream switch:

drivers/net/dsa/mt7530.c:mt7530_update_port_member() {
	struct dsa_port *cpu_dp = dp->cpu_dp;
	u32 port_bitmap = BIT(cpu_dp->index);
	...
	p->pm = PCR_MATRIX(port_bitmap);
}

That is an assignment, not an OR, so does "ip link set swpN master br0" on a
downstream user port replace the local upstream-link bit installed here with
the bit of the upstream switch's CPU port number?

mt753x_conduit_state_change() has the same shape, programming the local
MT753X_MFC CPU-port selector from a foreign index:

	mask = BIT(cpu_dp->index);
	...
	val = MT7530_CPU_EN | MT7530_CPU_PORT(__ffs(priv->active_cpu_ports));

The two indices happen to coincide on the EN751221 topology added later in
the series, but the commit message says "also support being the downstream
switch" generally.  Should these sites use dsa_upstream_port() too?

>  
>  	mutex_unlock(&priv->reg_mutex);
>  
> @@ -1390,7 +1444,7 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	 * largest MTU of the user ports. Because the switch only has a global
>  	 * RX length register, only allowing CPU port here is enough.
>  	 */
> -	if (!dsa_is_cpu_port(ds, port))
> +	if (!dsa_is_upstream_port(ds, port))
>  		return 0;

[Severity: Low]
The comment just above still says "only allowing CPU port here is enough",
but dsa_is_upstream_port() also matches a DSA link:

include/net/dsa.h:
	/* Return true if this is the local port used to reach the CPU port */
	static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)

Should the comment be updated to mention the upstream DSA port case?

>  
>  	regmap_read(priv->regmap, MT7530_GMACCR, &val);
> @@ -2411,11 +2465,6 @@ mt7530_setup(struct dsa_switch *ds)
>  		break;
>  	}
>  
> -	if (!dn) {
> -		dev_err(ds->dev, "parent OF node of DSA conduit not found");
> -		return -EINVAL;
> -	}
> -

[Severity: Low]
The commit message does not mention removing this diagnostic.  With the
check gone, a malformed conduit OF hierarchy (conduit device node without a
parent) is now accepted silently, and the port-5 PHY-mux scan is skipped via
the new "if (dn && ...)" guard below.

Could the commit message describe this change, or could the error be kept
for the case where a CPU port exists but its conduit OF node has no parent?

>  	ds->assisted_learning_on_cpu_port = true;
>  	ds->untag_vlan_aware_bridge_pvid = true;
>  	ds->mtu_enforcement_ingress = true;
> @@ -2517,7 +2566,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		/* Disable learning by default on all ports */
>  		regmap_set_bits(priv->regmap, MT7530_PSC_P(i), SA_DIS);
>  
> -		if (dsa_is_cpu_port(ds, i)) {
> +		if (dsa_is_upstream_port(ds, i)) {
>  			mt753x_cpu_port_enable(ds, i);
>  		} else {
>  			mt7530_port_disable(ds, i);

[ ... ]

> @@ -3310,7 +3359,7 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
>  
>  	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
>  	 * forwarded to the numerically smallest CPU port whose conduit
> -	 * interface is up.
> +	 * interface is up. NOTE: "CPU port" can also mean an upstream DSA link.
>  	 */

[Severity: Medium]
Related to the tagger side of this: the MTK tag carries no switch id and
mtk_tag_rcv() resolves the source port with a hardcoded switch index 0:

net/dsa/tag_mtk.c:mtk_tag_rcv() {
	skb->dev = dsa_conduit_find_user(dev, 0, port);
	if (!skb->dev) {
		kfree_skb(skb);
		return NULL;
	}
}

net/dsa/tag.h:dsa_conduit_find_user() {
	if (dp->ds->index == device && dp->index == port &&
	    dp->type == DSA_PORT_TYPE_USER)
		return dp->user;
}

The passthrough switch has no user ports, so if the DT numbers the
CPU-attached passthrough switch as dsa,member index 0 and the downstream
switch as 1, does every received frame get silently freed here?

Would it be worth enforcing or documenting that the downstream switch must
be index 0?

>  	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
>  	    priv->id != ID_EN7528)
> @@ -3405,6 +3454,76 @@ static int mt7988_setup(struct dsa_switch *ds)
>  	return mt7531_setup_common(ds);
>  }
>  
> +/* 1 if passthrough, negative if error. */
> +static int mt753x_check_passthrough(struct device *dev)
> +{

[Severity: Low]
This isn't a bug, but the banner comment omits the 0 (non-passthrough)
return, which is the case mt7530_probe_common() actually branches on.

> +	struct device_node *ports, *port;
> +	int passthrough_ports = 0;
> +	int enabled_ports = 0;
> +
> +	ports = of_get_child_by_name(dev->of_node, "ports");
> +	if (!ports)
> +		ports = of_get_child_by_name(dev->of_node, "ethernet-ports");
> +
> +	if (!ports) {
> +		dev_err(dev, "no ports child node found\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_available_child_of_node(ports, port) {
> +		struct device_node *link;
> +
> +		enabled_ports++;
> +
> +		link = of_parse_phandle(port, "ethernet", 0);
> +		if (!link)
> +			link = of_parse_phandle(port, "link", 0);
> +
> +		if (!link)
> +			continue;
> +
> +		of_node_put(link);
> +
> +		passthrough_ports++;
> +	}
> +
> +	of_node_put(ports);
> +
> +	/*
> +	 * A switch is considered passthrough if exactly two available
> +	 * ports have an "ethernet" or "link" phandle.
> +	 */
> +	if (passthrough_ports > 2 ||
> +	    (passthrough_ports == 2 && enabled_ports != 2)
> +	) {
> +		dev_err(dev, "Only two ports allowed in passthrough mode\n");
> +		return -EINVAL;
> +	}
> +
> +	return passthrough_ports == 2;
> +}

[Severity: High]
Can this heuristic break existing in-tree device trees that declare two CPU
ports?  "ethernet" and "link" phandles share one counter, so a second CPU
port looks the same as a downstream DSA link.

arch/arm/boot/dts/mediatek/mt7623n-bananapi-bpi-r2.dts has 5 enabled user
ports plus:

	port@5 { reg = <5>; label = "cpu"; ethernet = <&gmac1>; ... };
	port@6 { reg = <6>; label = "cpu"; ethernet = <&gmac0>; ... };

That gives passthrough_ports == 2 and enabled_ports == 7, so this returns
-EINVAL and mt7530_probe_common() aborts probe, leaving the board without a
switch.

arch/arm/boot/dts/mediatek/mt7623a.dtsi is the mirror case: ports 0..4 are
status = "disabled" and only the two CPU ports remain, so
passthrough_ports == 2 and enabled_ports == 2, this returns 1, and an
ordinary dual-CPU-port MT7530 is programmed as a passthrough bridge
(reduced ops table, both CPU ports set to VLAN_TRANSPARENT/EG_DISABLED,
global L2 length check and loop detection off, MTU forced to
MT7530_MAX_MTU).

The driver supports multiple CPU ports elsewhere
(mt753x_preferred_default_local_cpu_port(), mt753x_port_change_conduit()).
Could the detection distinguish "link" phandles (DSA links) from "ethernet"
phandles (CPU ports) instead of counting them together?

[Severity: Low]
This isn't a bug: the preceding series patch "dt-bindings: net: dsa:
mediatek,mt7530: add passthrough mode" does document the mode in
Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml, including
"In passthrough mode the upstream switch is restricted, with no other ports
or switch configuration allowed."

One observation remains: since the mode is inferred from topology rather
than an explicit property, dt_binding_check/dtbs_check cannot validate the
"exactly two available ports" rule that this function turns into -EINVAL,
and mt7530_probe_common() applies the heuristic to every mt753x variant
while the binding prose describes MT7530/MT7531.  Would an explicit DT
property be preferable?

[Severity: Low]
This isn't a bug, but two style points in the new code: the multi-line
comment starts with an empty "/*" line, which netdev style disallows, and
the "if" condition's closing ")" sits alone on its own line.

> +
> +/* No manipulation of forwarding rules allowed in passthrough mode */
> +static const struct dsa_switch_ops mt7530_passthrough_switch_ops = {
> +	.get_tag_protocol	= mtk_get_tag_protocol,
> +	.setup			= mt753x_setup,
> +	.teardown		= mt753x_teardown,
> +	.preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port,
> +	.get_strings		= mt7530_get_strings,
> +	.get_ethtool_stats	= mt7530_get_ethtool_stats,
> +	.get_sset_count		= mt7530_get_sset_count,
> +	.get_eth_mac_stats	= mt7530_get_eth_mac_stats,
> +	.get_rmon_stats		= mt7530_get_rmon_stats,
> +	.get_eth_ctrl_stats	= mt7530_get_eth_ctrl_stats,
> +	.get_stats64		= mt7530_get_stats64,
> +	.port_enable		= mt7530_port_enable,
> +	.port_disable		= mt7530_port_disable,
> +	.phylink_get_caps	= mt753x_phylink_get_caps,
> +	.support_eee		= dsa_supports_eee,
> +	.set_mac_eee		= mt753x_set_mac_eee,
> +	.conduit_state_change	= mt753x_conduit_state_change,
> +};

[Severity: High]
Do the omitted ops break operations that originate on the downstream
switch?  DSA notifiers are broadcast to every switch in the tree and abort
on the first error, and the handlers start with an unconditional test:

net/dsa/switch.c:dsa_switch_mtu() {
	if (!ds->ops->port_change_mtu)
		return -EOPNOTSUPP;
}

net/dsa/user.c:dsa_user_change_mtu() {
	err = dsa_port_mtu_change(cpu_dp, cpu_mtu);
	if (err)
		goto out_cpu_failed;
}

So does "ip link set <downstream user port> mtu N" now fail and roll back,
because this table has no .port_change_mtu?

The same shape applies to .port_vlan_add/del, .port_fdb_add/del and
.port_mdb_add/del:

net/dsa/switch.c:
	if (!ds->ops->port_fdb_add)
		return -EOPNOTSUPP;
	...
	if (!ds->ops->port_vlan_add)
		return -EOPNOTSUPP;

Does that break VLAN uppers on downstream ports, and defeat
ds->assisted_learning_on_cpu_port which mt7530_setup() still sets for the
passthrough switch?

Also, with no .port_stp_state_set, are the passthrough switch's ports ever
programmed into FID_PST forwarding state, given dsa_port_set_state_now()
swallows -EOPNOTSUPP?

> +
>  static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.get_tag_protocol	= mtk_get_tag_protocol,
>  	.setup			= mt753x_setup,
> @@ -3537,8 +3656,12 @@ EXPORT_SYMBOL_GPL(mt753x_table);
>  int
>  mt7530_probe_common(struct mt7530_priv *priv)
>  {
> +	int passthrough = mt753x_check_passthrough(priv->dev);
>  	struct device *dev = priv->dev;
>  
> +	if (passthrough < 0)
> +		return passthrough;
> +

[Severity: High]
This is where the heuristic's -EINVAL becomes fatal: probe returns before
anything is set up, so on mt7623n-bananapi-bpi-r2 the switch is never
registered and the board comes up with no network interfaces.

>  	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
>  	if (!priv->ds)
>  		return -ENOMEM;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 4/7] net: dsa: mediatek: support PLL setup on MMIO MT7530

[netdev-bot+sashiko]

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

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

- [Medium] On the path this patch newly enables (priv->bus == NULL, so
  bus = priv->child_bus), core_write() and core_rmw() execute a 4-5…
- [Low] Ordering of the new priv->child_bus producer vs. its consumers:
  priv->child_bus is assigned only in mt7530_setup_mdio()…
- [Low] The commit subject/message ('net: dsa: mediatek: support PLL
  setup on MMIO MT7530', 'begin using it for core switch PLL setup')…
- [Low] core_write() and core_rmw() compute `int ctl_phy =
  mt753x_ctrl_phy_addr(priv);` in the declaration initializer, before…
- [Low] The new kernel-doc for the @child_bus member of struct
  mt7530_priv is ungrammatical and does not describe the field's actual…

Pre-existing issues:
- [High] Pre-existing race on the MDIO path that this patch extends
  without addressing: core_write()/core_rmw() drive the control PHY's…

--

net: dsa: mediatek: support PLL setup on MMIO MT7530

This patch adds a new priv->child_bus pointer that records the MDIO bus
the switch driver itself creates in mt7530_setup_mdio(), and makes
core_write() and core_rmw() fall back to it when priv->bus is NULL, so
core (PLL) register access can work on MMIO-probed switches.

No Link: tags are present in the commit header.

The commit message says:

  In an MMIO implementation, the MDIO bus is part of the switch itself.
  This bus is already setup for PHY calibration so begin using it for
  core switch PLL setup.

[Severity: Low]
Is there any device in the tree at this commit that can reach the new
fallback?  mt7530-mmio.c matches airoha,an7583-switch,
airoha,en7581-switch, econet,en7528-switch and mediatek,mt7988-switch,
and all of those IDs use .sw_setup = mt7988_setup, which contains no
core_write()/core_rmw() call, and none of them has a .mac_port_config
that reaches mt7530_setup_port5()/mt7530_setup_port6().

The first MMIO consumer appears only with the later patch
"net: dsa: mediatek: support EN751221 switch", which adds ID_EN751221
with .mac_port_config = mt7530_mac_config and the control PHY address
for MMIO (if (priv->id == ID_EN751221) return 12;).

Would it be clearer to state in the message that this is a prerequisite
for the EN751221 MMIO variant, and that the MMIO control PHY address is
still missing here?

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b2344808457cb..b152b73315d50 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -77,6 +77,12 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  	struct mii_bus *bus = priv->bus;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +

[Severity: Low]
On the instances this fallback newly enables, what PHY address does the
MMD sequence end up using?  core_write() computes ctl_phy in the
declaration initializer, before the fallback runs:

	int ctl_phy = mt753x_ctrl_phy_addr(priv);
	struct mii_bus *bus = priv->bus;

and at this commit mt753x_ctrl_phy_addr() has no MMIO case:

	if (WARN_ON_ONCE(!priv->mdiodev))
		return 0;

	/* Default is 1st PHY */
	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);

mt7988_probe() in mt7530-mmio.c sets priv->bus = NULL and never sets
priv->mdiodev, so an MMIO caller would hit that WARN and then issue the
whole vendor MMD sequence to child bus address 0, which is an ordinary
port PHY rather than the control PHY.

This is not reachable at this commit (no MMIO ID calls core_write() or
core_rmw()), and the later patch "net: dsa: mediatek: support EN751221
switch" adds the missing case for the only MMIO consumer.  Would it be
worth adding the MMIO control PHY handling in the same patch that adds
the bus fallback, so the two never appear apart?

[Severity: High]
This isn't a bug introduced by this patch, but since the same helper is
now extended to a second bus, would it make sense to fix the locking
once for both paths?

On the existing MDIO path, core_write() drives the control PHY MMD
window while holding only the parent bus lock:

	mt7530_mutex_lock(priv);   /* priv->bus->mdio_lock */
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, reg);
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL,
			 MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR);
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);

That address, (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1), is the
same physical PHY that phylib manages as user port 0.  phylib's own
indirect MMD access in mmd_phy_indirect() holds the child bus lock, and
picks up the parent lock only per transfer, because the child bus ops go
through mt7530_phy_read_c22()/mt7530_phy_write_c22() ->
mdiobus_read_nested()/mdiobus_write_nested(priv->bus, ...).

Can the whole core_write() sequence then land between two steps of
phylib's sequence?  It leaves MII_MMD_CTRL set to
MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR with a CORE_* register latched in
MII_MMD_DATA, so phylib's next MII_MMD_DATA access would write its own
value into that CORE_* register (CORE_PLL_GROUP7 or
CORE_TRGMII_GSW_CLK_CG, for example), or read switch PLL state instead
of EEE ability.

Both contexts run concurrently: mt7530_setup_port6() is reached from
mt753x_phylink_mac_config() in the phylink resolve worker, which holds
only pl->phydev_mutex and pl->state_mutex, while phy_check_link_status()
issues genphy_c45_eee_is_active() -> phy_read_mmd() on link up.

Would taking the PHY's own bus lock (the child bus mdio_lock) around the
MMD sequence, before the nested parent lock, be the right fix here?

>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */
> @@ -112,6 +118,12 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
>  	u32 val;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +
>  	mt7530_mutex_lock(priv);

[Severity: Medium]
When the child bus is used, is anything serializing the multi-step MMD
window?  mt7530_mutex_lock()/mt7530_mutex_unlock() are no-ops in exactly
that case:

static void
mt7530_mutex_lock(struct mt7530_priv *priv)
{
	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
}

So with priv->bus == NULL the four writes in core_write(), and the five
transactions of the core_rmw() read-modify-write:

	ret = bus->read(bus, ctl_phy, MII_MMD_DATA);
	...
	val &= ~mask;
	val |= set;
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);

run with no lock covering the sequence, and the direct bus->write() /
bus->read() calls also bypass child_bus->mdio_lock, so the
lockdep_assert_held_once(&bus->mdio_lock) in __mdiobus_write() cannot
flag it.  The atomicity the MDIO path got from holding the parent bus
lock across the whole sequence is dropped for the MMIO path.

Individual transfers do stay atomic on MMIO (mt7531_ind_c22_phy_read()
and mt7531_ind_c22_phy_write() take priv->reg_mutex), so what is exposed
is the multi-step window on the control PHY: a second accessor of that
PHY address would see a read return, or a write land in, the wrong
CORE_* register.  I could not find such a second accessor for the
in-series EN751221 consumer, so the corruption itself is speculative,
but the dropped lock is not.

Would locking the bus that was actually selected work here, that is
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED) plus
__mdiobus_read()/__mdiobus_write(), the way other DSA drivers do it?

>  
>  	/* Write the desired MMD Devad */
> @@ -2429,8 +2441,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	if (priv->irq_domain && !mnp)
>  		mt7530_setup_mdio_irq(priv);
>  
> +	priv->child_bus = bus;
> +
>  	ret = devm_of_mdiobus_register(dev, bus, mnp);
>  	if (ret) {
> +		priv->child_bus = NULL;
>  		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
>  		if (priv->irq_domain && !mnp)
>  			mt7530_free_mdio_irq(priv);

[Severity: Low]
Is the producer of priv->child_bus ordered ahead of its consumers?
mt753x_setup() runs the switch specific setup first and only afterwards
creates the bus:

	int ret = priv->info->sw_setup(ds);
	...
	ret = mt7530_setup_mdio(priv);

So any core_write()/core_rmw() issued from an sw_setup callback on an
instance where priv->bus is NULL (mt7988_probe() sets priv->bus = NULL)
finds both pointers NULL, trips the new WARN_ON_ONCE(!bus) and returns
without performing the MMD sequence.  Should the bus be created before
sw_setup(), or should core_* return an error the caller can act on?

There is also a case where the pointer stays NULL for the whole device
lifetime.  mt7530_setup_mdio() returns 0 without allocating a bus when a
disabled mdio child node is present:

	if (mnp && !of_device_is_available(mnp))
		goto out;

After that, every runtime core_* access (the CORE_PLL_GROUP* and
CORE_TRGMII_GSW_CLK_CG programming in mt7530_setup_port6(), for example)
becomes a warning plus a no-op rather than a probe failure.  Is silently
skipping the clock programming the intended behaviour there?

For completeness, the path named in the commit message,
mt7530_setup() -> mt7530_pll_setup() -> core_clear()/core_write(), is
not reachable on a bus-less instance in this tree: mt7530_setup() is
.sw_setup only for the MDIO probed IDs, where priv->bus is a real parent
bus.  So this reads as a latent ordering trap plus the disabled mdio node
corner case rather than a failure any current board hits.

> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 108c831dd0120..5b7394f6383c7 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -886,6 +886,9 @@ struct mt753x_info {
>   * @dev:		The device pointer
>   * @ds:			The pointer to the dsa core structure
>   * @bus:		The bus used for the device and built-in PHY
> + * @child_bus:		The bus created by the device, on MDIO implementations
> + *			this is bus indirectly accesses `bus`, on MMIO
> + *			implementations this bus is part of the switch.

[Severity: Low]
The middle clause, "this is bus indirectly accesses `bus`", looks like it
is missing a word or two.

Also, "The bus created by the device" does not match the code:
mt7530_setup_mdio() returns 0 without allocating a bus when a disabled
mdio child node exists, and it resets the pointer back to NULL on
registration failure, so the field can legitimately be NULL for the
whole device lifetime.

Could the doc state when the field may be NULL, and which of priv->bus,
priv->child_bus and ds->user_mii_bus the core_* helpers are expected to
use?  The selection rule (if (!bus) bus = priv->child_bus;) is currently
copy-pasted into core_write() and core_rmw(), so every future core_*
helper has to remember to repeat it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 5/7] net: dsa: mediatek: support MDIO switch downstream of MMIO switch

[netdev-bot+sashiko]

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

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

- [High] Moving mt7530_setup_mdio_irq() from mt7530_setup_mdio() (where
  it ran *before* devm_of_mdiobus_register()) to mt753x_setup()…
- [High] mt7988_probe() (drivers/net/dsa/mt7530-mmio.c:69-71) calls
  mt7530_setup_mdio(priv) and discards its int return value, then…
- [High] The early MDIO enumeration added to mt7988_probe() still cannot
  reach a downstream switch when any preceding PHY child of the switch's…
- [Medium] mt7530_setup_mdio() is now invoked from mt7988_probe() right
  after dsa_register_switch().
- [Medium] mt7530_setup_mdio() is now reachable from mt7988_probe() with
  no lock held, whereas every previous invocation ran from…
- [Medium] mt7988_probe() now registers the switch's MDIO bus - and
  therefore probes the PHYs/devices on it - before mt753x_setup() ->…
- [Low] The submission has no Fixes: tag although it removes a user-
  visible limitation of the MMIO switch driver introduced by commit…

--

net: dsa: mediatek: support MDIO switch downstream of MMIO switch

This makes mt7530_setup_mdio() idempotent, exports it, and calls it from
mt7988_probe() right after dsa_register_switch() so that a switch sitting
on the MMIO switch's own MDIO bus can register and complete the DSA tree.
The PHY interrupt setup previously done inside mt7530_setup_mdio() is moved
to mt753x_setup().

No Link: tags in the commit header.

[Severity: Low]
This isn't a bug, but there is no Fixes: tag on the submission even though
it lifts a user-visible limitation of the MMIO path added by commit
110c18bfed41 ("net: dsa: mt7530: introduce driver for MT7988 built-in
switch").  Is a Fixes: tag intended here, or is this considered new
functionality rather than a fix (the MMIO driver never claimed to support a
switch behind the switch's own MDIO bus)?

> diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
> index cf3bdb23a2d54..f387190a95391 100644
> --- a/drivers/net/dsa/mt7530-mmio.c
> +++ b/drivers/net/dsa/mt7530-mmio.c
> @@ -61,7 +61,14 @@ mt7988_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->regmap))
>  		return PTR_ERR(priv->regmap);
>  
> -	return dsa_register_switch(priv->ds);
> +	ret = dsa_register_switch(priv->ds);
> +
> +	if (ret)
> +		return ret;
> +
> +	mt7530_setup_mdio(priv);
> +
> +	return 0;
>  }

[Severity: High]
Should the return value of mt7530_setup_mdio() be checked here?

mt7530_setup_mdio() returns -ENOMEM from devm_mdiobus_alloc() and
propagates devm_of_mdiobus_register() failures, including -EPROBE_DEFER,
after clearing priv->child_bus:

drivers/net/dsa/mt7530.c:mt7530_setup_mdio() {
	ret = devm_of_mdiobus_register(dev, bus, mnp);
	if (ret) {
		priv->child_bus = NULL;
		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
		...
	}
}

The other caller, mt753x_setup(), propagates that value:

	ret = mt7530_setup_mdio(priv);
	if (ret)
		return ret;

With the error dropped, mt7988_probe() returns 0 while no child MDIO bus
exists.  Since the DSA tree can only be completed once that bus registers
the downstream switch, mt753x_setup() never runs and nothing retries the
bus, so the board stays without networking.  Note also that the failure
path clears priv->child_bus but leaves ds->user_mii_bus pointing at the
never-registered devm bus, so a later mt753x_setup() would allocate a
second mii_bus.

[Severity: High]
Can this call actually reach the downstream switch node on a DT shaped like
arch/arm64/boot/dts/mediatek/mt7988a.dtsi, where the PHY children of the
switch's mdio node use the switch itself as their interrupt provider?

	switch@15020000 {
		interrupt-controller;
		#interrupt-cells = <1>;
		mdio {
			gsw_phy0: ethernet-phy@0 {
				reg = <0>;
				interrupts = <0>;

At probe time priv->irq_domain does not exist yet, because
mt7530_setup_irq() only runs from mt753x_setup():

mt753x_setup()
    mt7530_setup_irq()		<- creates priv->irq_domain
    mt7530_setup_mdio()

so of_irq_get() on gsw_phy0 finds no host for the switch node and returns
-EPROBE_DEFER, which is forwarded during boot:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
	rc = fwnode_irq_get(child, 0);
	if (rc == -EPROBE_DEFER)
		rc = driver_deferred_probe_check_state(&phy->mdio.dev);
	if (rc == -EPROBE_DEFER)
		return rc;
}

and __of_mdiobus_parse_phys() then aborts child enumeration on that error,
after which __of_mdiobus_register() unregisters the bus.  Wouldn't that
mean the downstream switch node is never registered, the tree never
completes, and the IRQ domain that the PHY children need is never created,
i.e. the prerequisite loop the patch aims to break is still there?

[Severity: Medium]
Is it safe to probe the MDIO bus here, before the switch bring-up in
mt7988_setup() has run?

In the incomplete-tree case this patch targets, dsa_register_switch()
returns without invoking ds->ops->setup, so the reset and PHY power-up
sequence has not executed when devm_of_mdiobus_register() synchronously
probes the MDIO children:

drivers/net/dsa/mt7530.c:mt7988_setup() {
	/* Reset the switch */
	reset_control_assert(priv->rstc);
	usleep_range(20, 50);
	reset_control_deassert(priv->rstc);
	usleep_range(20, 50);
	...
	/* Reset the switch PHYs */
	regmap_write(priv->regmap, MT7530_SYS_CTRL, SYS_CTRL_PHY_RST);
}

Can the PHY ID reads issued from probe through mt7531_ind_c22_phy_read()
return 0xffff or time out in the MT7531_PHY_IAC poll loop on an un-reset
and (on AN7583, before the AN7583_CSR_PHY_CORE_REG_CLK_SEL /
AN7583_CSR_ETHER_AFE_PWD handling) unclocked block?  And when the reads do
succeed, does the later reset_control_assert()/deassert() plus
SYS_CTRL_PHY_RST reset the MDIO master and integrated PHYs underneath the
mdio/phy devices that were already bound during probe, including any
transaction in flight from the freshly probed downstream switch?

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b152b73315d50..fe93be1ece7d7 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> @@ -2414,6 +2414,10 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	static int idx;
>  	int ret = 0;
>  
> +	/* Already done */
> +	if (priv->child_bus)
> +		return 0;
> +
>  	mnp = of_get_child_by_name(np, "mdio");

[Severity: Medium]
What serializes this check now that mt7530_setup_mdio() is also reachable
from mt7988_probe()?

Every previous invocation came from mt753x_setup() under dsa2_mutex, but
the new call runs after the lock has been dropped:

net/dsa/dsa.c:dsa_register_switch() {
	mutex_lock(&dsa2_mutex);
	err = dsa_switch_probe(ds);
	dsa_tree_put(ds->dst);
	mutex_unlock(&dsa2_mutex);
}

So the probe thread can run mt7530_setup_mdio(priv) unlocked while another
switch of the same tree drives, under dsa2_mutex:

dsa_register_switch()
    dsa_tree_setup()
        dsa_tree_setup_switches()
            dsa_switch_setup(this ds)
                mt753x_setup()
                    mt7530_setup_mdio(same priv)

Can both observe priv->child_bus == NULL and both allocate and register an
mii_bus for the same switch?  priv->child_bus is only published just before
devm_of_mdiobus_register() and unconditionally cleared on failure, so a
losing racer can clear a pointer that mt753x_phy_read_c22() and
mt753x_phy_write_c22() dereference.  The non-atomic "static int idx"
increment used for bus->id is also no longer under any lock, so can two
buses end up with the same KBUILD_MODNAME-%d id?

Related, in the single-threaded case: the downstream switch probed from
inside devm_of_mdiobus_register() completes the tree and re-enters
mt753x_setup() for this same priv, where this guard returns 0 while the
outer registration is still in flight further down the stack.  Is that
intended?

> @@ -2438,9 +2442,6 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	bus->parent = dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;
        ^^^^^^^^^^^^^^

[Severity: Medium]
When called from mt7988_probe(), is ds->phys_mii_mask initialized yet?

The mask is set by the DSA core, and it is skipped entirely for an
incomplete tree:

net/dsa/dsa.c:dsa_tree_setup() {
	complete = dsa_tree_setup_routing_table(dst);
	if (!complete)
		return 0;
	...
}

net/dsa/dsa.c:dsa_switch_setup() {
	/* Initialize ds->phys_mii_mask before registering the user MDIO bus
	 * driver and before ops->setup() has run, ...
	 */
	ds->phys_mii_mask |= dsa_user_ports(ds);
}

With phys_mii_mask still 0, bus->phy_mask becomes ~0.  If the switch node
has no "mdio" child, devm_of_mdiobus_register() does not override the mask:

drivers/net/mdio/of_mdio.c:__of_mdiobus_register() {
	if (!np)
		return __mdiobus_register(mdio, owner);
	...
}

so does __mdiobus_register() end up skipping every address, leaving
ds->user_mii_bus pointing at a bus with no PHYs?  The new
"if (priv->child_bus) return 0;" guard then makes the later call from
mt753x_setup(), where phys_mii_mask is correct, a no-op, so the ~0 mask
looks permanent, and dsa_switch_setup() will not build its own user mii bus
either because ds->user_mii_bus is already set.

> -	if (priv->irq_domain && !mnp)
> -		mt7530_setup_mdio_irq(priv);
> -
>  	priv->child_bus = bus;
>  
>  	ret = devm_of_mdiobus_register(dev, bus, mnp);

[Severity: High]
Does moving this to mt753x_setup() make the per-PHY IRQ assignment take
effect too late?

Before the change the order was:

mt7530_setup_mdio()
    mt7530_setup_mdio_irq()		<- fills ds->user_mii_bus->irq[p]
    devm_of_mdiobus_register()
        __mdiobus_register()
            phy_device_create()
                dev->irq = bus->irq[addr];

After the change, mt753x_setup() calls mt7530_setup_mdio() first and only
then mt7530_setup_mdio_irq(), so the writes in

	irq = irq_create_mapping(priv->irq_domain, p);
	ds->user_mii_bus->irq[p] = irq;

happen after phy_device_create() has already latched phydev->irq from
bus->irq[addr], which mdiobus_alloc_size() initialized to PHY_POLL.
Nothing re-reads bus->irq[] afterwards, so do the internal PHYs silently
fall back to polling while the virqs from irq_create_mapping() end up
referenced by nothing?

This is the case the old "priv->irq_domain && !mnp" condition selected, and
it matches in-tree DTs that have "interrupt-controller" on the switch node
and no "mdio" child, for example switch@1f in
arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts and switch@31 in
arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts.  The commit
message does not mention this behaviour change; is it intended?

> @@ -3285,6 +3287,9 @@ mt753x_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	if (priv->irq_domain && ds->user_mii_bus)
> +		mt7530_setup_mdio_irq(priv);
> +
>  	/* Initialise the PCS devices */
>  	for (i = 0; i < priv->ds->num_ports; i++) {
>  		priv->pcs[i].pcs.ops = priv->info->pcs_ops;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 6/7] dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221

[netdev-bot+sashiko]

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

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

- [High] net: dsa: lockless child_bus access causes MDIO race condition
- [Medium] The new switch-device compatible `econet,en751221`
  (mediatek,mt7530.yaml line 126) is the identical string already…
- [Medium] The new `econet,en751221` conditional (mediatek,mt7530.yaml
  lines 369-377) applies only `$ref: "#/$defs/mt7530-passthrough-dsa-…
- [Medium] The series patch "net: dsa: mediatek: support MDIO switch
  downstream of MMIO switch" moved `mt7530_setup_mdio_irq()` out of…
- [Medium] In passthrough mode `mt753x_cpu_port_enable()` (added by the
  series patch "net: dsa: mediatek: add support for passthrough mode")…
- [Low] The paragraph added to the binding description
  (mediatek,mt7530.yaml lines 32-36) states that "EcoNet EN751221 SoCs…
- [Low] Concern that neither `econet,en751221-switch` nor
  `econet,en751221` has any consumer: at this commit…

--

dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221

This adds two compatible strings to the MT7530 binding: econet,en751221-switch
for the memory mapped switch built into the EcoNet EN751221 SoC, and
econet,en751221 for the MT7530 multi-chip module that some EN751221 packages
attach to port 5 of the on-die switch via TRGMII. A new $defs block constrains
the DSA link port of the multi-chip module.

A few questions on the schema below, plus two observations about other
patches in the same series.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e29fb86934981..106c4087528d7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -29,6 +29,12 @@ description: |
>    and the switch registers are directly mapped into SoC's memory map rather than
>    using MDIO. The DSA driver currently doesn't support MT7620 variants.
>  
> +  EcoNet EN751221 SoCs have a memory mapped MT7530 with 3x 10/100 PHYs and 1x
                                                          ^^^^

[Severity: Low]
Should this be 4x 10/100 PHYs?  The commit message of this patch says
"ports 0..3 are 10/100 only, port 4 has a gigabit PHY", which is four fast
ethernet PHYs, and en751221_mac_port_get_caps() in the later driver patch of
this series agrees:

	/* EN751221 ports 0..3 are connected to the integrated FE PHYs. */
	case 0 ... 3:
		__set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces);
		config->mac_capabilities |= MAC_10 | MAC_100;

Since there is no in-tree EN751221 switch DTS yet, this paragraph is the only
port reference a DT author has.

> +  gigabit PHY. Some EN751221 chips such as EN7513G and EN7526G have a multi-chip
> +  module implementation of the MT7530 as well as the on-die switch. In these
> +  packages, the multi-chip module is connected to port 5 of the on-die switch
> +  via a TRGMII link.
> +
>    There is only the standalone version of MT7531.
>  
>    Port 5 on MT7530 supports various configurations:
> @@ -111,6 +117,14 @@ properties:
>            Built-in switch of the EcoNet EN7528 SoC
>          const: econet,en7528-switch
>  
> +      - description:
> +          Built-in switch of EcoNet EN751221 SoC
> +        const: econet,en751221-switch
> +
> +      - description:
> +          Multi-chip module MT7530 in EN751221 SoC
> +        const: econet,en751221

[Severity: Medium]
Can this string collide with the existing SoC compatible?  econet,en751221 is
already documented as the EN751221 root node compatible in
Documentation/devicetree/bindings/mips/econet.yaml:

  properties:
    $nodename:
      const: '/'

    compatible:
      oneOf:
        - description: Boards with EcoNet EN751221 family SoC
          items:
            - enum:
                - smartfiber,xp8421-b
            - const: econet,en751221

and arch/mips/boot/dts/econet/en751221.dtsi uses it on the root node:

  / {
	compatible = "econet,en751221";

dtschema derives the select clause of a binding from the strings enumerated
under properties:compatible, so after this patch mediatek,mt7530.yaml is also
selected for that root node.  The switch schema has required: [compatible,
reg], unevaluatedProperties: false, and via the new conditional below also
required: [mediatek,mcm] (which in turn pulls in resets and reset-names).  Does
make dtbs_check on arch/mips/boot/dts/econet/en751221_smartfiber_xp8421-b.dtb
now report the root node as an invalid MT7530 switch node?

The reverse direction looks affected too: a switch@... node carrying
compatible = "econet,en751221" is selected by mips/econet.yaml and fails its
$nodename: const: '/' and its two-entry board/SoC compatible list.

The analogous MT7621 case does not collide because the SoC root uses
mediatek,mt7621-soc.  Since the SoC compatible is already released, would a
distinct string for the switch, e.g. econet,en751221-mcm-switch, work here?

> +
>    reg:
>      maxItems: 1
>  

[ ... ]

> @@ -223,6 +237,20 @@ $defs:
>                  phy-mode:
>                    const: internal
>  
> +  mt7530-passthrough-dsa-port:
> +    patternProperties:
> +      "^(ethernet-)?ports$":
> +        patternProperties:
> +          "^(ethernet-)?port@[0-6]$":
> +            if:
> +              required: [ link ]
> +            then:
> +              properties:
> +                reg:
> +                  const: 6
> +                phy-mode:
> +                  const: trgmii
> +
>    mt7530-dsa-port:
>      patternProperties:
>        "^(ethernet-)?ports$":
> @@ -330,6 +358,7 @@ allOf:
>              - airoha,en7581-switch
>              - airoha,an7583-switch
>              - econet,en7528-switch
> +            - econet,en751221-switch
>      then:
>        $ref: "#/$defs/builtin-dsa-port"
>        properties:
> @@ -337,6 +366,16 @@ allOf:
>          mediatek,mcm: false
>          reset-names: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - econet,en751221
> +    then:
> +      $ref: "#/$defs/mt7530-passthrough-dsa-port"
> +      required:
> +        - mediatek,mcm

[Severity: High]
One more observation about another patch in this series, so there is
no hunk to quote for it either.

In "net: dsa: mediatek: support PLL setup on MMIO MT7530", is the MDIO
bus lock lost for the new child_bus path?  core_write() and core_rmw()
now do:

	if (!bus)
		bus = priv->child_bus;

	if (WARN_ON_ONCE(!bus))
		return;

	mt7530_mutex_lock(priv);

	/* Write the desired MMD Devad */
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);

but mt7530_mutex_lock() only knows about priv->bus:

	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);

On an MMIO switch priv->bus is NULL, so the guard makes
mt7530_mutex_lock()/mt7530_mutex_unlock() a no-op while the following
bus->write()/bus->read() calls are raw driver callbacks issued on
child_bus without child_bus->mdio_lock held.  The indirect MMD sequence
(MII_MMD_CTRL devad, MII_MMD_DATA address, then the data phase) is not
atomic on the wire, so a concurrent mdiobus_read()/mdiobus_write() on
the same child bus - phylib polling the downstream MT7530 PHYs, or a
regmap access from the mt7530 MDIO path - can interleave and leave the
MMD devad/address latch pointing somewhere else, giving a read or write
to the wrong core register.  This is reachable from mt7530_setup_port6()
and the PLL setup path whenever a port changes state, i.e. during normal
operation, and the missing lock is invisible to lockdep because nothing
is taken at all.

Should mt7530_mutex_lock()/mt7530_mutex_unlock() fall back to
priv->child_bus->mdio_lock the same way core_write()/core_rmw() do, or
would it be cleaner for the child_bus path to call mdiobus_write()/
mdiobus_read() (which take the lock themselves) instead of the raw
bus->write()/bus->read() callbacks?  Either way the two places that
pick the bus should agree on which lock protects it.

[Severity: Medium]
Are the MT7530 per-port interface-mode constraints intentionally dropped for
this compatible?  The commit message says the multi-chip module "is similar to
the MT7621 multi-chip module", and the mediatek,mt7621 conditional in the same
allOf applies:

      $ref: "#/$defs/mt7530-dsa-port"
      required:
        - mediatek,mcm

A JSON-Schema object can only carry one $ref, so referencing
mt7530-passthrough-dsa-port here means $defs/mt7530-dsa-port (port 5:
gmii/mii/rgmii, other ports: rgmii/trgmii) is never evaluated.  A DT giving
this multi-chip module an ethernet (CPU) port with phy-mode = "sgmii",
"2500base-x" or "internal" would then pass dtbs_check and only fail later in
mt7530_mac_config()/phylink.

Related to that, the new $defs/mt7530-passthrough-dsa-port rule is gated on:

            if:
              required: [ link ]

so a node with no DSA link port gets no port validation at all, and ports
carrying ethernet are unconstrained.

On the on-die side, econet,en751221-switch is only added to the builtin enum,
whose $defs/builtin-dsa-port rule keys off required: [ ethernet ].  Nothing
pins the cascade port to port 5 with trgmii as the added description states, so
link can be declared on port@0..port@4, which en751221_mac_port_get_caps()
wires to the internal FE/GE PHYs and which offers TRGMII on port 5 only.

Should an example be added for the new on-die plus multi-chip module topology?
Without one, the new $def, the required mediatek,mcm and the implied
resets/reset-names combination are never exercised by dt_binding_check.

> +
>  unevaluatedProperties: false
>  
>  examples:

Two further observations concern other patches in the same series, so there are
no hunks to quote here.

[Severity: Medium]
In "net: dsa: mediatek: support MDIO switch downstream of MMIO switch", is the
new call site of mt7530_setup_mdio_irq() too late?  It used to run inside
mt7530_setup_mdio() before devm_of_mdiobus_register(), and now runs in
mt753x_setup() after mt7530_setup_mdio() has returned:

	ret = mt7530_setup_mdio(priv);
	if (ret)
		return ret;

	if (priv->irq_domain && ds->user_mii_bus)
		mt7530_setup_mdio_irq(priv);

Bus registration creates the phy_devices, and phy_device_create() copies the
IRQ once at creation time:

	dev->irq = bus->irq[addr];

mdiobus_alloc_size() pre-fills bus->irq[i] = PHY_POLL, so the internal PHYs are
created with phydev->irq == PHY_POLL and the later
ds->user_mii_bus->irq[p] = irq_create_mapping(...) writes an array that is not
read again.  Does this leave every MT7530/MT7531 board that declares
interrupt-controller on the switch node and has no mdio child node (binding
examples 3 to 7, MT7621 boards) on polled link detection instead of the
switch PHY link-change interrupt?  The ordering is unchanged at the end of the
series.

[Severity: Medium]
In "net: dsa: mediatek: add support for passthrough mode", is the egress tag
setting for the upstream port immediately overwritten?
mt753x_cpu_port_enable() programs:

	if (priv->is_passthrough) {
		/* Disable parsing of the DSA tag ... */
		regmap_write(priv->regmap, MT7530_PVC_P(port),
			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));

but both callers, mt7530_setup() and mt7531_setup_common(), do this in the same
loop iteration right after the call:

		if (dsa_is_upstream_port(ds, i)) {
			mt753x_cpu_port_enable(ds, i);
		} else {
			...
		}
		/* Enable consistent egress tag */
		regmap_update_bits(priv->regmap, MT7530_PVC_P(i),
				   PVC_EG_TAG_MASK,
				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));

MT7530_VLAN_EG_DISABLED is 0 and MT7530_VLAN_EG_CONSISTENT is 1 in
drivers/net/dsa/mt7530.h, so the field really changes.  The downstream DSA link
port is re-programmed to PVC_EG_TAG(MT7530_VLAN_EG_DISABLED) later in
mt7530_port_enable(), but with priv->is_passthrough set the upstream port takes
neither the user port branch nor the dsa_port_is_dsa(dp) && dp->index !=
upstream_pt branch there, so there is no later write for it.  Is the write in
mt753x_cpu_port_enable() then dead, or should the EG_CONSISTENT update be
skipped for the upstream port in passthrough mode?  The commit message of that
patch states both ports end up at MT7530_VLAN_EG_DISABLED.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr

Re: [PATCH v2 net-next 7/7] net: dsa: mediatek: support EN751221 switch

[netdev-bot+sashiko]

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

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

- [High] This patch makes the core_write()/core_rmw() MMD indirect-access
  path reachable for an MMIO-attached switch (ID_EN751221 gets…
- [Medium] en751221_trgmii_pair_setup() passes the wrong default_taps
  array to each en751221_trgmii_calibrate_direction() call.
- [Medium] On the EN751221 on-die switch the TRGMII cascade is port 5,
  but mt7530_mac_config() dispatches port 5 to mt7530_setup_port5()…
- [Medium] The MDIO of_match table now claims the compatible string
  "econet,en751221" for the multi-chip-module MT7530, but that exact…
- [Medium] mt7530_setup_irq() writes TOP_SIG_CTRL_NORMAL ("This register
  must be set for MT7530 to properly fire interrupts") only for…
- [Medium] en751221_trgmii_pair_setup() overwrites MT753X_PMCR_P() with
  hardcoded full-register values (mcr_down, then mcr_up) on both the…
- [Medium] The new TRGMII reset and training sequence ignores every
  regmap return value, including accesses to the external switch which…
- [Medium] en751221_trgmii_pair_setup() reaches into a different DSA
  switch instance (the on-die switch's mt7530_priv, obtained from the…
- [Medium] Connecting the MCM switch through the on-die switch's MDIO
  child bus nests two mt7530_priv::reg_mutex instances of the same lock…
- [Medium] For the on-die switch (ID_EN751221) the only place the TRGMII
  transmit clock/DDS is programmed is mt7530_setup_port6(), reached from…
- [Medium] net: dsa: mt7530: immediate PMSR read may cause spurious
  calibration failure
- [Low] The patch adds register/field definitions that no code uses
  (TOP_SIG_CTRL_B0 with comment "Undocumented", and PMCR_IFG_XMIT_96 /…
- [Low] The in-tree binding text for the EN751221 on-die switch says "3x
  10/100 PHYs and 1x gigabit PHY", while the new…
- [Low] For `econet,en751221-switch` the binding applies the generic
  `builtin-dsa-port` definition, which only constrains ports carrying…

--

net: dsa: mediatek: support EN751221 switch

This adds two new mt753x IDs for the EcoNet EN751221: the on-die MT7530
(memory mapped) and an optional second MT7530 attached as an MCM module over
MDIO.  It also adds a TRGMII RX tap calibration routine that is run over the
inter-switch link when it comes up.

> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 24ef54f3db2a6..2fd987ff85107 100644
> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -139,6 +139,7 @@ mt7531_create_sgmii(struct mt7530_priv *priv)
>  }
>  
>  static const struct of_device_id mt7530_of_match[] = {
> +	{ .compatible = "econet,en751221", .data = &mt753x_table[ID_EN751221_EXT], },

[Severity: Medium]
Is it intentional to reuse the SoC root node compatible string for the MCM
switch?  "econet,en751221" is already defined as the root node compatible:

Documentation/devicetree/bindings/mips/econet.yaml
  $nodename:
    const: '/'

  compatible:
    oneOf:
      - description: Boards with EcoNet EN751221 family SoC
        items:
          - enum:
              - smartfiber,xp8421-b
          - const: econet,en751221

and arch/mips/boot/dts/econet/en751221.dtsi already uses it there:

  / {
        compatible = "econet,en751221";

With the same string also declared in mediatek,mt7530.yaml as "Multi-chip
module MT7530 in EN751221 SoC", one string now names two different device
classes.

Since dt-schema derives select from the compatible const/enum values, won't
both schemas now select on it, so the switch node gets validated against
mips/econet.yaml (failing $nodename and the two item compatible list) and
every EN751221 board root node gets validated against mediatek,mt7530.yaml?

Every other MCM/on-die variant in these tables uses a device specific
string.  Would something like econet,en751221-mcm-switch work here, in both
the driver and the binding?

> 	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> diff --git a/drivers/net/dsa/mt7530-mmio.c b/drivers/net/dsa/mt7530-mmio.c
> index f387190a95391..b77e3cb655aa9 100644
> --- a/drivers/net/dsa/mt7530-mmio.c
> +++ b/drivers/net/dsa/mt7530-mmio.c
> @@ -12,6 +12,7 @@
>  static const struct of_device_id mt7988_of_match[] = {
>  	{ .compatible = "airoha,an7583-switch", .data = &mt753x_table[ID_AN7583], },
>  	{ .compatible = "airoha,en7581-switch", .data = &mt753x_table[ID_EN7581], },
> +	{ .compatible = "econet,en751221-switch", .data = &mt753x_table[ID_EN751221], },

[Severity: Low]
For this compatible the binding applies $defs/builtin-dsa-port, which only
constrains ports that carry ethernet:

  builtin-dsa-port:
    patternProperties:
      "^(ethernet-)?ports$":
        patternProperties:
          "^(ethernet-)?port@[0-6]$":
            if:
              required: [ ethernet ]
            then:
              properties:
                phy-mode:
                  const: internal

Ports carrying link are left unconstrained, while en751221_mac_port_get_caps()
advertises PHY_INTERFACE_MODE_TRGMII on port 5 only and INTERNAL everywhere
else.

Doesn't that let a DSA link on a port other than 5, a link on port 5 with a
non-trgmii phy-mode, or a CPU port at reg = 5 with phy-mode = internal pass
dt_binding_check and then get rejected by phylink at runtime?

The sibling $defs/mt7530-passthrough-dsa-port used for the MCM side does
constrain its link port to reg 6 and phy-mode trgmii - should the built-in
side get an equivalent constraint?

>  	{ .compatible = "econet,en7528-switch", .data = &mt753x_table[ID_EN7528], },
>  	{ .compatible = "mediatek,mt7988-switch", .data = &mt753x_table[ID_MT7988], },
>  	{ /* sentinel */ }
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fe93be1ece7d7..4f43454083b95 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -49,6 +49,9 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
>  static int
>  mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
>  {
> +	if (priv->id == ID_EN751221)
> +		return 12;
> +

[Severity: Low]
This isn't a bug, but could the bare 12 get a comment or a #define?  Every
other ID derives the control PHY address from the MDIO device address, and
all core_write()/core_rmw() PLL programming for the on-die switch depends on
this value, yet the commit message doesn't mention it.

In the same spirit, en751221_trgmii_pair_setup() programs hardware with raw
literals rather than the register names this header otherwise uses:

	/* SOC: Undocumented */
	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
		regmap_write(ondie->regmap,  (0x7a14 + channel * 8),
			     0x3227700);

That address range overlaps the MT7530_TRGMII_RD() block - can it be
expressed with a named macro?

Finally, TOP_SIG_CTRL_B0, PMCR_IFG_XMIT_96 and PMCR_IFG_XMIT_RAND are added
but never referenced anywhere in the tree (only PMCR_IFG_XMIT_64 is used).
Was a register write hunk dropped when the patch was prepared?

>  	if (WARN_ON_ONCE(!priv->mdiodev))
>  		return 0;
>  
> @@ -344,12 +347,19 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  	regmap_read(priv->regmap, MT753X_MTRAP, &xtal);
>  	xtal &= MT7530_XTAL_MASK;
>  
> +	/* EN751221 on-die does not report clock speed in MTRAP */
> +	if (priv->id == ID_EN751221)
> +		xtal = MT7530_XTAL_25MHZ;
> +
>  	if (xtal == MT7530_XTAL_25MHZ)
>  		ssc_delta = 0x57;
>  	else
>  		ssc_delta = 0x87;
>  
> -	if (priv->id == ID_MT7621) {
> +	if (priv->id == ID_EN751221_EXT || priv->id == ID_EN751221) {
> +		/* PLL frequency: 362.5Mhz */
> +		ncpo1 = 0x1d00;

[Severity: Medium]
For the on-die switch, is this the only place the TRGMII clock and DDS get
programmed, and is it ordered against the tap calibration?

mt753x_phylink_mac_config() calls mac_port_config only for ports 5 and 6, and
mt7530_mac_config() sends port 6 here.  mt7530_setup_port6() starts with:

	/* Disable the MT7530 TRGMII clocks */
	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

and then rewrites CORE_PLL_GROUP2/4/5/6/7/10/11 for any interface that is not
PHY_INTERFACE_MODE_RGMII, including the INTERNAL CPU link that
en751221_mac_port_get_caps() declares for port 6.

The calibration itself is triggered from the other switch's mac_link_up, and
each dsa_port has its own phylink instance and resolve work, so the two are
unordered.  Can a later port 6 mac_config (conduit down/up, resume,
renegotiation) gate the clock off and relock the DDS underneath an already
calibrated live link?  A fixed DSA link gets no further mac_link_up, so is
there anything that re-runs the calibration afterwards?

>  	} else if (priv->id == ID_MT7621) {
>  		/* PLL frequency: 125MHz: 1.0GBit */
>  		if (xtal == MT7530_XTAL_40MHZ)
>  			ncpo1 = 0x0640;
> @@ -466,6 +476,251 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	usleep_range(25, 35);
>  }
>  
> +static bool
> +en751221_trgmii_cal_ok(struct mt7530_priv *rx, u32 reg)
> +{
> +	u32 val = 0;
> +
> +	regmap_read(rx->regmap, reg, &val);
> +	regmap_write(rx->regmap, reg, val | EDGE_CHK);
> +	regmap_write(rx->regmap, reg, val & ~EDGE_CHK);
> +	regmap_read(rx->regmap, reg, &val);

[Severity: Medium]
What happens here if the first regmap_read() fails?  For the MCM switch these
accesses go over MDIO (mt7530_regmap_read()/mt7530_regmap_write() in
mt7530-mdio.c), where transactions can genuinely fail.

val stays 0 and the two writes are full register writes, so the whole
MT7530_TRGMII_RD(x) register for that lane is zeroed - clearing BSLIP_EN and
RD_TAP - and the pass/fail verdict is computed from a value that never came
from hardware.

The rest of the sequence has the same property: a failed TX_RST/RX_RST
deassert can leave a block in reset, a failed final

	regmap_clear_bits(tx->regmap, MT7530_TRGMII_TXCTRL, TRAIN_TXEN);

can leave the transmitter emitting training patterns, and a failed saved-tap
read yields a fallback tap of 0, yet en751221_trgmii_pair_setup() still ends
with an unconditional "TRGMII inter-switch link initialized".  Should these
return values be checked and propagated?

> +
> +	return FIELD_GET(RD_VALUE_MASK, val) == TGMII_TD_PAT &&
> +	       !FIELD_GET(RD_ERR_MASK, val);
> +}
> +
> +static void
> +en751221_trgmii_calibrate_direction(struct mt7530_priv *tx,
> +				    struct mt7530_priv *rx,
> +				    u8 *default_taps)
> +{

[ ... ]

> +		if (best_tap > -1) {
> +			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
> +					   RD_TAP(best_tap));
> +
> +			log[best_tap] = 'X';
> +
> +			dev_info(rx->dev,
> +				 "TRGMII lane %d: %s tap %d old %d\n",
> +				 channel, log, best_tap, default_taps[channel]);
> +		} else {
> +			regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
> +					   RD_TAP(default_taps[channel]));
> +
> +			dev_warn(rx->dev,
> +				 "TRGMII lane %d: %s calibration failed, preserving tap %u\n",
> +				 channel, log, default_taps[channel]);
> +		}

[ ... ]

> +static void
> +en751221_set_tx_drive(struct mt7530_priv *priv, u8 drvp, u8 drvn, u8 clk_drvp,
> +		      u8 clk_drvn)
> +{

[ ... ]

> +static void
> +en751221_trgmii_pair_setup(struct mt7530_priv *ext, int ext_port,
> +			   struct mt7530_priv *ondie, int ondie_port)
> +{
> +	u8 default_taps_ondie[NUM_TRGMII_CTRL];
> +	u8 default_taps_ext[NUM_TRGMII_CTRL];
> +	u32 mcr_down, mcr_up;
> +	int channel;
> +	int reg_val;
> +
> +	/* BOTH: Put interfaces in a downed state */
> +	mcr_down = PMCR_IFG_XMIT(PMCR_IFG_XMIT_64) | PMCR_MAC_MODE |
> +		   MT7530_FORCE_MODE | PMCR_MAC_RX_EN | PMCR_BACKOFF_EN |
> +		   PMCR_BACKPR_EN | PMCR_FORCE_SPEED_1000 | PMCR_FORCE_FDX;
> +	regmap_write(ondie->regmap,  MT753X_PMCR_P(ondie_port), mcr_down);
> +	regmap_write(ext->regmap, MT753X_PMCR_P(ext_port), mcr_down);
> +	usleep_range(5000, 6000);
> +

[ ... ]

> +	/* BOTH: Ports up */
> +	mcr_up = mcr_down | PMCR_MAC_TX_EN | PMCR_FORCE_LNK;
> +	regmap_write(ondie->regmap, MT753X_PMCR_P(ondie_port), mcr_up);
> +	regmap_write(ext->regmap, MT753X_PMCR_P(ext_port), mcr_up);

[Severity: Medium]
These are full register writes rather than regmap_update_bits(), and this
function runs from the tail of mt753x_phylink_mac_link_up().  Does that
discard the PMCR bits phylink just programmed?

mt753x_phylink_mac_link_up() builds:

	if (duplex == DUPLEX_FULL) {
		mcr |= PMCR_FORCE_FDX;
		if (tx_pause)
			mcr |= PMCR_FORCE_TX_FC_EN;
		if (rx_pause)
			mcr |= PMCR_FORCE_RX_FC_EN;
	}

	regmap_set_bits(priv->regmap, MT753X_PMCR_P(dp->index), mcr);

and then calls into here, where mcr_down/mcr_up contain no flow control bits,
no PMCR_FORCE_EEE1G/PMCR_FORCE_EEE100 and no PMCR_EXT_PHY.  Nothing appears
to re-apply them afterwards, and the peer switch's port is force programmed
behind its own phylink instance's back.  Should this use regmap_update_bits()
with an explicit mask, or re-apply the phylink derived value?

> +
> +	/* MCM: Set Ext->SoC TX delay to 0 */
> +	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++)
> +		regmap_clear_bits(ext->regmap, EN7530_TRGMII_TD_CTRL(channel),
> +				  TGMII_TD_TAP_MASK);
> +

[ ... ]

> +	/* BOTH: Collect and then zero every RX TAP */
> +	for (channel = 0; channel < NUM_TRGMII_CTRL; channel++) {
> +		reg_val = 0;
> +		regmap_read(ext->regmap, MT7530_TRGMII_RD(channel), &reg_val);
> +		default_taps_ext[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
> +		regmap_clear_bits(ext->regmap, MT7530_TRGMII_RD(channel),
> +				  RD_TAP_MASK);
> +
> +		reg_val = 0;
> +		regmap_read(ondie->regmap, MT7530_TRGMII_RD(channel), &reg_val);
> +		default_taps_ondie[channel] = FIELD_GET(RD_TAP_MASK, reg_val);
> +		regmap_clear_bits(ondie->regmap, MT7530_TRGMII_RD(channel),
> +				  RD_TAP_MASK);
> +	}
> +
> +	en751221_trgmii_calibrate_direction(ondie, ext, default_taps_ondie);
> +	en751221_trgmii_calibrate_direction(ext, ondie, default_taps_ext);

[Severity: Medium]
Are the two default_taps arrays swapped here?

The loop above stores each switch's own saved taps into its own array:
default_taps_ext[] comes from ext->regmap, default_taps_ondie[] from
ondie->regmap.

In the calls, the second argument is rx:

  en751221_trgmii_calibrate_direction(ondie, ext, default_taps_ondie);
      rx = ext, taps = ondie's
  en751221_trgmii_calibrate_direction(ext, ondie, default_taps_ext);
      rx = ondie, taps = ext's

and the callee only ever writes the array into rx:

	regmap_update_bits(rx->regmap, rx_reg, RD_TAP_MASK,
			   RD_TAP(default_taps[channel]));

So when a lane finds no passing tap, doesn't the receiver's RD_TAP get
programmed with the peer chip's saved value, and doesn't the dev_warn() report
a tap that was never in that register?  The values are not the same on both
sides - mt7530_setup() programs RD_TAP(16) on the MDIO attached switch while
the on-die switch keeps its reset/bootloader value.

> +
> +	dev_info(ondie->dev, "TRGMII inter-switch link initialized\n");
> +}
> +
> +static void
> +mt7530_cal_interswitch_trgmii(struct dsa_port *dp)
> +{

[ ... ]

> +		peer = peer_ds->priv;
> +
> +		if (peer->id != ID_EN751221)
> +			continue;
> +
> +		en751221_trgmii_pair_setup(priv, dp->index, peer,
> +					   peer_dp->index);

[Severity: Medium]
What serializes this against the peer switch's own driver paths?

en751221_trgmii_pair_setup() performs a long sleeping sequence on the peer
instance - full PMCR overwrites, TX/RX resets, drive strength changes,
RCK/RTT and per-lane RD_TAP read-modify-writes, plus core PLL access - but
neither ondie->reg_mutex nor rtnl is held.  It runs from the ext switch's
phylink resolve worker, which holds only that phylink instance's state_mutex.

Meanwhile the on-die switch has its own per-port phylink workers, which can
run concurrently:

mt753x_phylink_mac_link_down()
    regmap_clear_bits(priv->regmap, MT753X_PMCR_P(dp->index), mcr);

mt7530_mac_config() -> mt7530_setup_port6()
    core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

Can a concurrent link event on the on-die switch clobber the PMCR bits this
sequence is writing, or gate the TRGMII clock off in the middle of the tap
sweep so every lane "fails" and falls back to the default tap?

> +	}
> +}
> +
>  static void
>  mt7530_mib_reset(struct dsa_switch *ds)
>  {
> @@ -1408,7 +1663,8 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  
>  	mutex_unlock(&priv->reg_mutex);
>  
> -	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
> +	    priv->id != ID_EN751221 && priv->id != ID_EN751221_EXT)
>  		return 0;

[Severity: Medium]
Does the new MCM-behind-on-die topology nest two reg_mutex instances of the
same lock class?

mt7530_port_enable() takes mutex_lock(&priv->reg_mutex) and issues regmap
accesses while holding it.  For the MCM switch those accesses go through the
on-die switch's MDIO child bus, whose read/write ops are the on-die
info->phy_read_c22/phy_write_c22, i.e. mt7531_ind_c22_phy_read() and
mt7531_ind_c22_phy_write(), and those take mutex_lock(&priv->reg_mutex) on the
on-die priv.

Both mutexes are initialised at the same mutex_init(&priv->reg_mutex) site in
mt7530_probe_common(), so they share one lock class and both acquisitions use
the default subclass.  Won't lockdep report possible recursive locking on
every boot of a board with the MCM present, and disable further validation for
the rest of the boot?  Would a mutex_lock_nested() annotation or separate lock
classes be appropriate?

>  
>  	if (port == 5)
> @@ -2980,6 +3243,44 @@ static void en7528_mac_port_get_caps(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> +static void en751221_mac_port_get_caps(struct dsa_switch *ds, int port,
> +				       struct phylink_config *config)
> +{
> +	switch (port) {
> +	/* EN751221 ports 0..3 are connected to the integrated FE PHYs. */
> +	case 0 ... 3:

[Severity: Low]
This advertises four 10/100 ports and the commit message says "4 FE ports and
1 GE port", but the binding text added by the preceding series patch says:

  EcoNet EN751221 SoCs have a memory mapped MT7530 with 3x 10/100 PHYs and 1x
  gigabit PHY.

That binding patch's own changelog says "ports 0..3 are 10/100 only", so is
the description text in
Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml the part that
needs fixing?  As it stands a DT author cannot tell whether port@3 is valid.

> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +
> +		config->mac_capabilities |= MAC_10 | MAC_100;
> +		break;
> +
> +	/* Port 4 is connected to the standalone EN7512/EN7521 GPHY. */
> +	case 4:
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +
> +		config->mac_capabilities |= MAC_10 | MAC_100 | MAC_1000FD;
> +		break;
> +
> +	/* Port 5 is the 1 Gbit/s TRGMII cascade to the external MT7530. */
> +	case 5:
> +		__set_bit(PHY_INTERFACE_MODE_TRGMII,
> +			  config->supported_interfaces);

[Severity: Medium]
Port 5 is declared TRGMII here and port 6 INTERNAL, but mt7530_mac_config()
dispatches by fixed port number:

	if (port == 5)
		mt7530_setup_port5(priv->ds, interface);
	else if (port == 6)
		mt7530_setup_port6(priv->ds, interface);

mt7530_setup_port5() contains no TRGMII or PLL handling, only MTRAP muxing and
RGMII pad/delay programming, and mt7530_setup_port6() only returns early for
PHY_INTERFACE_MODE_RGMII.

So for this variant, doesn't the INTERNAL CPU link on port 6 fall into the
TRGMII branch - setting MT7530_P6ECR to P6_INTF_MODE(1) and reprogramming the
DDS with the new xtal override and ncpo1 = 0x1d00 - while the port that
actually reports TRGMII never reaches any TRGMII specific setup?  If the
fallthrough is what the hardware wants, could that be spelled out in a
comment?

> +
> +		config->mac_capabilities |= MAC_1000FD;
> +		break;
> +
> +	/* Port 6 is the 1 Gbit/s CPU link to GDM1. */
> +	case 6:
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +
> +		config->mac_capabilities |= MAC_1000FD;
> +		break;
> +	}
> +}
> +
>  static void
>  mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		  phy_interface_t interface)
> @@ -3129,6 +3430,9 @@ static void mt753x_phylink_mac_link_up(struct phylink_config *config,
>  	}
>  
>  	regmap_set_bits(priv->regmap, MT753X_PMCR_P(dp->index), mcr);
> +
> +	if (interface == PHY_INTERFACE_MODE_TRGMII)
> +		mt7530_cal_interswitch_trgmii(dp);
>  }
>  
> @@ -3670,6 +3975,28 @@ const struct mt753x_info mt753x_table[] = {
>  		.phy_write_c45 = mt7531_ind_c45_phy_write,
>  		.mac_port_get_caps = en7528_mac_port_get_caps,
>  	},
> +	[ID_EN751221] = {
> +		.id = ID_EN751221,
> +		.pcs_ops = &mt7530_pcs_ops,
> +		.sw_setup = mt7988_setup,
> +		.phy_read_c22 = mt7531_ind_c22_phy_read,
> +		.phy_write_c22 = mt7531_ind_c22_phy_write,
> +		.phy_read_c45 = mt7531_ind_c45_phy_read,
> +		.phy_write_c45 = mt7531_ind_c45_phy_write,
> +		.mac_port_get_caps = en751221_mac_port_get_caps,
> +		.mac_port_config = mt7530_mac_config,

[Severity: Medium]
Is this PMSR read guaranteed to observe the link state that was
just forced a few instructions earlier?

mt753x_phylink_mac_link_up() does

	regmap_set_bits(priv->regmap, MT753X_PMCR_P(dp->index), mcr);

with mcr containing PMCR_FORCE_LNK, and then calls straight into
mt7530_cal_interswitch_trgmii(), which reads MT7530_PMSR_P(dp->index)
and bails out unless PMSR_LINK is set. There is no delay and no
regmap_read_poll_timeout() between the write and the read, so if the
MAC needs any propagation time before the forced link shows up in the
status register, the check fails and the whole calibration is skipped
silently - no message is emitted on this path, unlike the dev_info()
at the end of en751221_trgmii_pair_setup().

Since this is a fixed DSA link that gets no further mac_link_up, a
single early read would leave the inter-switch TRGMII running on
whatever taps the bootloader left behind for the rest of the boot,
which is exactly the failure mode this patch is meant to avoid.

What does the hardware actually guarantee here? If PMSR is expected to
reflect the forced link immediately, could that be stated in a comment?
Otherwise, would a regmap_read_poll_timeout() on PMSR_LINK (with a
dev_warn() on timeout so the skip is visible) be more robust? And is
the PMSR check even needed - the caller is already in mac_link_up for
a TRGMII interface, so what additional condition is it trying to
rule out?

[Severity: High]
This is the first memory mapped ID to set .mac_port_config, which makes the
indirect MMD core access path reachable for a switch where priv->bus is NULL:

mt753x_phylink_mac_config() -> mt7530_mac_config() -> mt7530_setup_port6()
    -> core_clear()/core_write()/core_set() -> core_rmw()

Is the MDIO bus lock held on that path?  mt7530_mutex_lock() is:

	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);

and for an MMIO probed switch priv->bus is NULL, so it is a no-op, while
core_rmw() falls back to the child bus:

	struct mii_bus *bus = priv->bus;
	...
	if (!bus)
		bus = priv->child_bus;
	...
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);

The four transaction "select MMD devad, select MMD register, select function,
read/write data" sequence is then issued on priv->child_bus without holding
child_bus->mdio_lock, which every other user of that bus takes - phylib via
mdiobus_read()/mdiobus_write(), and the MCM MT7530's regmap, whose lock is
bus->mdio_lock.

Can a concurrent transaction on that child bus interleave inside the MMD
select/data sequence, so a core PLL value lands in whatever register the other
user selected, or the read-modify-write picks up a foreign value?

> +	},
> +	[ID_EN751221_EXT] = {
> +		.id = ID_EN751221_EXT,
> +		.pcs_ops = &mt7530_pcs_ops,
> +		.sw_setup = mt7530_setup,
> +		.phy_read_c22 = mt7530_phy_read_c22,
> +		.phy_write_c22 = mt7530_phy_write_c22,
> +		.phy_read_c45 = mt7530_phy_read_c45,
> +		.phy_write_c45 = mt7530_phy_write_c45,
> +		.mac_port_get_caps = mt7530_mac_port_get_caps,
> +		.mac_port_config = mt7530_mac_config,
> +	},
>  };
>  EXPORT_SYMBOL_GPL(mt753x_table);
>  
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index c86bc4bca29b9..1a4329749c0f1 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -356,6 +358,9 @@ enum mt7530_vlan_port_acc_frm {
>  #define MT753X_PMCR_P(x)		(0x3000 + ((x) * 0x100))
>  #define  PMCR_IFG_XMIT_MASK		GENMASK(19, 18)
>  #define  PMCR_IFG_XMIT(x)		FIELD_PREP(PMCR_IFG_XMIT_MASK, x)
> +#define    PMCR_IFG_XMIT_96		0
> +#define    PMCR_IFG_XMIT_RAND		1
> +#define    PMCR_IFG_XMIT_64		2
>  #define  PMCR_EXT_PHY			BIT(17)
>  #define  PMCR_MAC_MODE			BIT(16)
>  #define  MT7530_FORCE_MODE		BIT(15)
> @@ -601,6 +607,8 @@ enum mt7531_xtal_fsel {
>  /* Register for TOP signal control */
>  #define MT7530_TOP_SIG_CTRL		0x7808
>  #define  TOP_SIG_CTRL_NORMAL		(BIT(17) | BIT(16))
> +/* Undocumented */
> +#define  TOP_SIG_CTRL_B0		BIT(0)

[Severity: Medium]
This new bit has no user, and the .c side of the same register was not
touched.  Should ID_EN751221_EXT be added to the condition in
mt7530_setup_irq()?

	/* This register must be set for MT7530 to properly fire interrupts */
	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
		regmap_set_bits(priv->regmap, MT7530_TOP_SIG_CTRL,
				TOP_SIG_CTRL_NORMAL);

ID_EN751221_EXT is an MT7530 die probed through the MDIO driver and uses
mt7530_setup(), and this patch extends three other "ID_MT7530 || ID_MT7621"
predicates (mt7530_port_enable, mt7530_port_disable,
mt753x_conduit_state_change) with the new IDs but not this one.

If the MCM switch node declares interrupt-controller, the irq chip and domain
are registered while the register that enables interrupt generation is never
set - would that leave PHY/link interrupts undelivered?

>  
>  #define MT7531_TOP_SIG_SR		0x780c
>  #define  PAD_DUAL_SGMII_EN		BIT(1)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr