[PATCH v3 0/7] net: dsa: mt7530: support EcoNet EN751221

[PATCH v3 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 v2:
* The MCM switch previously identified as econet,en751221, similarly to
mediatek,mt7621, however since econet,en751221 is already used for the
SoC compatible, is it changed to econet,en751221-gswitch. The name
"gswitch" is chosen as there's apparently no standard and for EN751221
this is intuitive because it is the gigabit switch used in
implementations with a "G" in the chip name.

* Do not use a special mt7530_switch_ops for passthrough mode, instead
detect passthrough mode in each function and exit early so as to avoid
errors when trying to make a configuration switch-tree wide.

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                      | 609 ++++++++++++++++--
 drivers/net/dsa/mt7530.h                      |  46 +-
 5 files changed, 663 insertions(+), 53 deletions(-)


base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.39.5

[PATCH v3 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 7781a63b4e6f..d10194488bde 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");
@@ -2668,8 +2673,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
 	 */
@@ -2737,25 +2745,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 v3 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>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../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 v3 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 | 194 +++++++++++++++++++++++++++++++++++----
 drivers/net/dsa/mt7530.h |   6 ++
 2 files changed, 184 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index d10194488bde..3eb2e11d6985 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -199,6 +199,9 @@ static void mt7530_port_fast_age(struct dsa_switch *ds, int port)
 	u32 val;
 	int ret;
 
+	if (priv->is_passthrough)
+		return;
+
 	mutex_lock(&priv->reg_mutex);
 
 	/* Flush all non-static MAC address entries */
@@ -970,6 +973,9 @@ mt7530_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
 	unsigned int age_count;
 	unsigned int age_unit;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	/* Applied timer is (AGE_CNT + 1) * (AGE_UNIT + 1) seconds.
 	 * The DSA core has already validated the range using
 	 * ds->ageing_time_min and ds->ageing_time_max.
@@ -1286,13 +1292,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 +1356,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 +1367,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));
 
-		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
+	} 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)));
+
+		/* 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);
 
@@ -1386,11 +1446,14 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 	int length;
 	u32 val;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	/* When a new MTU is set, DSA always set the CPU port's MTU to the
 	 * 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);
@@ -1427,6 +1490,9 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 	struct mt7530_priv *priv = ds->priv;
 	u32 stp_state;
 
+	if (priv->is_passthrough)
+		return;
+
 	switch (state) {
 	case BR_STATE_DISABLED:
 		stp_state = MT7530_STP_DISABLED;
@@ -1521,6 +1587,9 @@ mt7530_port_bridge_flags(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	if (flags.mask & BR_LEARNING)
 		regmap_update_bits(priv->regmap, MT7530_PSC_P(port), SA_DIS,
 				   flags.val & BR_LEARNING ? 0 : SA_DIS);
@@ -1561,6 +1630,9 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	mt7530_update_port_member(priv, port, bridge.dev, true);
@@ -1714,6 +1786,9 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return;
+
 	mutex_lock(&priv->reg_mutex);
 
 	mt7530_update_port_member(priv, port, bridge.dev, false);
@@ -1737,6 +1812,9 @@ mt7530_port_fdb_add(struct dsa_switch *ds, int port,
 	int ret;
 	u8 port_mask = BIT(port);
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
@@ -1754,6 +1832,9 @@ mt7530_port_fdb_del(struct dsa_switch *ds, int port,
 	int ret;
 	u8 port_mask = BIT(port);
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_EMP);
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
@@ -1809,6 +1890,9 @@ mt7530_port_mdb_add(struct dsa_switch *ds, int port,
 	u32 val;
 	int ret;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	mt7530_fdb_write(priv, vid, 0, addr, 0, STATIC_EMP);
@@ -1838,6 +1922,9 @@ mt7530_port_mdb_del(struct dsa_switch *ds, int port,
 	u32 val;
 	int ret;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	mt7530_fdb_write(priv, vid, 0, addr, 0, STATIC_EMP);
@@ -1862,6 +1949,10 @@ mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 {
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct mt7530_priv *priv = ds->priv;
+
+	if (priv->is_passthrough)
+		return 0;
 
 	if (vlan_filtering) {
 		/* The port is being kept as VLAN-unaware port when bridge is
@@ -1970,6 +2061,9 @@ mt7530_port_vlan_add(struct dsa_switch *ds, int port,
 	struct mt7530_hw_vlan_entry new_entry;
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	/* VID 0 is managed exclusively by mt7530_setup_vlan0() for
@@ -2022,6 +2116,9 @@ mt7530_port_vlan_del(struct dsa_switch *ds, int port,
 	struct mt7530_hw_vlan_entry target_entry;
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	/* VID 0 is managed exclusively by mt7530_setup_vlan0(). */
@@ -2063,6 +2160,9 @@ static int mt753x_port_mirror_add(struct dsa_switch *ds, int port,
 	int monitor_port;
 	u32 val;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	/* Check for existent entry */
 	if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
 		return -EEXIST;
@@ -2099,6 +2199,9 @@ static void mt753x_port_mirror_del(struct dsa_switch *ds, int port,
 	struct mt7530_priv *priv = ds->priv;
 	u32 val;
 
+	if (priv->is_passthrough)
+		return;
+
 	regmap_read(priv->regmap, MT7530_PCR_P(port), &val);
 	if (mirror->ingress) {
 		val &= ~PORT_RX_MIR;
@@ -2405,11 +2508,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;
@@ -2511,7 +2609,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);
@@ -2535,7 +2633,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.
@@ -2635,7 +2733,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);
@@ -3264,6 +3362,9 @@ mt753x_port_change_conduit(struct dsa_switch *ds, int port,
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct mt7530_priv *priv = ds->priv;
 
+	if (priv->is_passthrough)
+		return 0;
+
 	if (priv->id != ID_MT7531) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "Changing DSA conduit is only supported on MT7531");
@@ -3304,7 +3405,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)
@@ -3365,6 +3466,11 @@ static int mt753x_tc_setup_qdisc_tbf(struct dsa_switch *ds, int port,
 static int mt753x_setup_tc(struct dsa_switch *ds, int port,
 			   enum tc_setup_type type, void *type_data)
 {
+	struct mt7530_priv *priv = ds->priv;
+
+	if (priv->is_passthrough)
+		return 0;
+
 	switch (type) {
 	case TC_SETUP_QDISC_TBF:
 		return mt753x_tc_setup_qdisc_tbf(ds, port, type_data);
@@ -3399,6 +3505,55 @@ 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;
+}
+
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt753x_setup,
@@ -3531,8 +3686,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;
@@ -3550,7 +3709,10 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	priv->id = priv->info->id;
 	priv->dev = dev;
 	priv->ds->priv = priv;
+
+	priv->is_passthrough = passthrough;
 	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 v3 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 3eb2e11d6985..d09ff207e2df 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 */
@@ -2472,8 +2484,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 v3 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 d09ff207e2df..dbe3887097db 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2447,7 +2447,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;
@@ -2457,6 +2457,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))
@@ -2481,9 +2485,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);
@@ -2498,6 +2499,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)
@@ -3328,6 +3330,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 v3 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-gswitch
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..b73732d28d8c 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 and allows for 5 gigabit ports.
+
   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 gigabit MT7530 in EN751221 SoC
+        const: econet,en751221-gswitch
+
   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-gswitch
+    then:
+      $ref: "#/$defs/mt7530-passthrough-dsa-port"
+      required:
+        - mediatek,mcm
+
 unevaluatedProperties: false
 
 examples:
-- 
2.39.5

[PATCH v3 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 0be107dc6759..03ef6a9f2084 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 dbe3887097db..c44f2084b173 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;
 
@@ -347,12 +350,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;
@@ -469,6 +479,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)
 {
@@ -1414,7 +1669,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)
@@ -1441,7 +1697,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. */
@@ -2603,6 +2860,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);
 
@@ -3023,6 +3286,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)
@@ -3172,6 +3473,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)
@@ -3428,7 +3732,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);
@@ -3700,6 +4005,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 v3 2/7] dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

[Andrew Lunn]

On Wed, Oct 28, 2026 at 01:13:12AM +0000, Caleb James DeLisle wrote:
> 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>
> Acked-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  .../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:

Looking at just what is here, all i see is a comment. There is no new
property? Yet the subject is "add passthrough mode"?

	Andrew

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

[Andrew Lunn]

> +static int
> +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);

Please don't add forward declaration, move the code around.

       Andrew

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

[Andrew Lunn]

> @@ -199,6 +199,9 @@ static void mt7530_port_fast_age(struct dsa_switch *ds, int port)
>  	u32 val;
>  	int ret;
>  
> +	if (priv->is_passthrough)
> +		return;
> +
>  	mutex_lock(&priv->reg_mutex);
>  
>  	/* Flush all non-static MAC address entries */
> @@ -970,6 +973,9 @@ mt7530_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
>  	unsigned int age_count;
>  	unsigned int age_unit;
>  
> +	if (priv->is_passthrough)
> +		return 0;
> +
> @@ -1386,11 +1446,14 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	int length;
>  	u32 val;
>  
> +	if (priv->is_passthrough)
> +		return 0;
> +

What is actually left?

Is there sufficient code reuse that this makes sense?

Would it be simpler to have a much simpler DSA driver just for pass
through? Or the mt7530 driver has a second dsa_switch_ops instance
with all these functions set to NULL?

     Andrew

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

[Andrew Lunn]

On Wed, Oct 28, 2026 at 01:13:15AM +0000, Caleb James DeLisle wrote:
> 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().

I would suggest just moving it to probe. Don't also have it in setup.
KISS.

	Andrew

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

[Andrew Lunn]

On Wed, Oct 28, 2026 at 01:13:16AM +0000, Caleb James DeLisle wrote:
> 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-gswitch
> multi-chip-module switch whose port 6 is configured as a DSA link port.

Is there a use case of the econet,en751221-switch not in pass through
mode? This is where you could have a different dsa_switch_ops
structure, which probably makes the changes less invasive.

Even if there is a use case for not pass through, you can still look
for the property in probe, and decide which of two different
dsa_switch_ops to pass to dsa_register_switch.

	   Andrew

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

[Caleb James DeLisle]

Hello,

Thanks for the review.


On 28/10/2026 03:15, Andrew Lunn wrote:
>> @@ -199,6 +199,9 @@ static void mt7530_port_fast_age(struct dsa_switch *ds, int port)
>>   	u32 val;
>>   	int ret;
>>   
>> +	if (priv->is_passthrough)
>> +		return;
>> +
>>   	mutex_lock(&priv->reg_mutex);
>>   
>>   	/* Flush all non-static MAC address entries */
>> @@ -970,6 +973,9 @@ mt7530_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
>>   	unsigned int age_count;
>>   	unsigned int age_unit;
>>   
>> +	if (priv->is_passthrough)
>> +		return 0;
>> +
>> @@ -1386,11 +1446,14 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>>   	int length;
>>   	u32 val;
>>   
>> +	if (priv->is_passthrough)
>> +		return 0;
>> +
> What is actually left?


In v2 I was using special switch ops, and it looked like this:


/* 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,
};


>
> Is there sufficient code reuse that this makes sense?
>
> Would it be simpler to have a much simpler DSA driver just for pass
> through? Or the mt7530 driver has a second dsa_switch_ops instance
> with all these functions set to NULL?


After sending v2, I discovered that some switch config runs across the 
whole switch tree, and NULL configurators causes those ops to log an 
error and exit early.


As far as making a "passthrough driver", there are a few things going 
against it. As you can see there is still quite a bit of shared code, 
also it would be inconvenient to use a different compatible because some 
chip packages exclude the MCM switch and have 3x 10/100 ports and 1x 
gigabit port, so its better for the DT to pretend that the mt7530 can 
simply do full hierarchies, though (per the big commit message) that 
doesn't actually work and we're relying on the fact that hardware 
integrators will not require something that can't be done.

Lastly, the TRGMII calibration requires poking registers on one switch 
and reading values off the other (patch 7) so I think that mess is best 
kept all inside of one C file.


Thanks,

Caleb


>
>       Andrew

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

[Caleb James DeLisle]

On 28/10/2026 03:26, Andrew Lunn wrote:
> On Wed, Oct 28, 2026 at 01:13:16AM +0000, Caleb James DeLisle wrote:
>> 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-gswitch
>> multi-chip-module switch whose port 6 is configured as a DSA link port.
> Is there a use case of the econet,en751221-switch not in pass through
> mode? This is where you could have a different dsa_switch_ops
> structure, which probably makes the changes less invasive.
>
> Even if there is a use case for not pass through, you can still look
> for the property in probe, and decide which of two different
> dsa_switch_ops to pass to dsa_register_switch.


I think this was pretty much answered in my reply to your patch 3 
review. Without insider knowledge, my understanding is EN751221 is a 
common die, and it's packaged in a variety of different EN751x (DSL) and 
EN752x (Fiber) chip modules. The die has a switch with 3x 10/100 ports 
and 1x gigabit port. Chips with a 'G' in the name (EN7513G, EN7526G that 
I know) have the MCM switch and use the on-die switch as a passthrough. 
So yes, in short there are real (and quite popular) devices with both 
MCM and non-MCM implementations.


Thanks,

Caleb


>
> 	   Andrew

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

[Caleb James DeLisle]

On 28/10/2026 03:04, Andrew Lunn wrote:
> On Wed, Oct 28, 2026 at 01:13:12AM +0000, Caleb James DeLisle wrote:
>> 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>
>> Acked-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>>   .../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:
> Looking at just what is here, all i see is a comment. There is no new
> property? Yet the subject is "add passthrough mode"?


Indeed, I used "add" to mean "add documentation of". Per my reply on 
patch 3, I think we really prefer to avoid special properties and let 
this constitute (very) limited support for a switch hierarchy.

If there's another verb you think makes a better commit line, I'd be 
happy to adjust.


Thanks,

Caleb


>
> 	Andrew

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

[Caleb James DeLisle]

On 28/10/2026 03:20, Andrew Lunn wrote:
> On Wed, Oct 28, 2026 at 01:13:15AM +0000, Caleb James DeLisle wrote:
>> 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().
> I would suggest just moving it to probe. Don't also have it in setup.
> KISS.


I don't think there's a way to do that which will work. And I guess I 
need to add a few comments to explain why. Basically 
when dsa_register_switch() is called, one of three things can happen:

1. return error (often without calling mt753x_setup), typically this is 
EPROBE_DEFER because the ethernet driver is not there yet.

2. return 0 without calling mt753x_setup (because not all switches in 
the tree are yet probed)

3. call mt753x_setup, complete setup, and return 0


In case 1, mt7530_setup_mdio must not be called, because it would be 
setting up an MDIO on a driver that's about to get deleted.

In case 2, we have to call mt7530_setup_mdio because otherwise we may 
not discover downstream switches (this is the patch)

In case 3, mt7530_setup_mdio must be called before mt753x_setup returns 
because after it returns, the DSA core synchronously enables ports and 
it will expect PHYs to be operational.


Only the DSA core really knows what it's going to do, so I think the 
only way to solve this without going inside of it is this patch where 
calls are made from two sites.


That said, I think it's pretty clear that I should put a couple lines of 
comment in there so that nobody is tempted to "streamline" this code 
into one of the things that doesn't work.


Thanks,

Caleb


>
> 	Andrew

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

[Caleb James DeLisle]

On 28/10/2026 03:07, Andrew Lunn wrote:
>> +static int
>> +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);
> Please don't add forward declaration, move the code around.


Sure thing.


Thanks,

Caleb


>
>         Andrew

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

[Andrew Lunn]

On Wed, Oct 28, 2026 at 12:27:18PM +0100, Caleb James DeLisle wrote:
> 
> On 28/10/2026 03:04, Andrew Lunn wrote:
> > On Wed, Oct 28, 2026 at 01:13:12AM +0000, Caleb James DeLisle wrote:
> > > 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>
> > > Acked-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > >   .../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:
> > Looking at just what is here, all i see is a comment. There is no new
> > property? Yet the subject is "add passthrough mode"?
> 
> 
> Indeed, I used "add" to mean "add documentation of". Per my reply on patch
> 3, I think we really prefer to avoid special properties and let this
> constitute (very) limited support for a switch hierarchy.
> 
> If there's another verb you think makes a better commit line, I'd be happy
> to adjust.

[Goes and looks at mediatek,mt7530.yaml]

So pass through is not an existing property. You are also not adding a
new property 'passthrough'.

What you appear to be saying is that the MT7530 and MT7531 are hard
coded by the driver into pass through mode? So it is in effect a
property of the compatible? So maybe put this comment next to the
compatible? And make the subject/commit message talk about the
compatible being hard coded for pass through?

Looking forward, what happens when somebody wants to use the switch as
a switch? Or you figure out how to do proper D in DSA? It becomes
messy because you need to add a property indicating the compatible
should actually be a switch?

So maybe you really do need a property?

But DT describes hardware, and pass through is configuration. So maybe
it should not be in DT at all, Maybe the driver needs to look at the
DT tree, figure out a port is used as a DSA port, and put the switch
into pass through all on its own? Once D in DSA is supported, the
addition ports become available, but no other changes are needed in
the DT blob?

Sorry, going around in circles, but hopefully it helps you think about
the big picture.

	   Andrew

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

[Andrew Lunn]

> > What is actually left?
> 
> 
> In v2 I was using special switch ops, and it looked like this:

Sorry, i did not look at v2.

> /* 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,
> };

I think you should be doing something like this. Either two
dsa_switch_ops, or you add generic implementations for all the ops,
which then call device specific implementations dependent on the
compatible. This is how many DSA drivers handle different families of
devices in one driver.

I would avoid putting "if (priv->passthrough)" everywhere.

	Andrew

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

[Andrew Lunn]

On Wed, Oct 28, 2026 at 12:59:23PM +0100, Caleb James DeLisle wrote:
> 
> On 28/10/2026 03:20, Andrew Lunn wrote:
> > On Wed, Oct 28, 2026 at 01:13:15AM +0000, Caleb James DeLisle wrote:
> > > 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().
> > I would suggest just moving it to probe. Don't also have it in setup.
> > KISS.
> 
> 
> I don't think there's a way to do that which will work. And I guess I need
> to add a few comments to explain why. Basically when dsa_register_switch()
> is called, one of three things can happen:
> 
> 1. return error (often without calling mt753x_setup), typically this is
> EPROBE_DEFER because the ethernet driver is not there yet.
> 
> 2. return 0 without calling mt753x_setup (because not all switches in the
> tree are yet probed)
> 
> 3. call mt753x_setup, complete setup, and return 0
> 
> 
> In case 1, mt7530_setup_mdio must not be called, because it would be setting
> up an MDIO on a driver that's about to get deleted.

Which is fine, because when the probe fails, you unregister the MDIO
bus. That should work. The second time around, you do have the netdev,
and it should work.

It is not great, you are doing extra work, but many DSA drivers are
structured like this, the MDIO bus is registered in probe. The bus is
scanned, PHY are found, and then the probe fails, and the PHYs are all
thrown away etc.

Hanging one switch off another switch like this using MDIO, kind of
pushes you in this direction. Typical DSA setups don't do this, there
is one MDIO bus which all switches hang off, sometimes with a
multiplexor to make one MDIO bus act like many.

Something to look at which might be interesting, Vladimir had some
patches which allowed for partial switch topologies, where you could
take a switch in a DSA setup off line. Maybe there is something in
those patches which help, you can bring the switches up independently?

	    Andrew

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

[Caleb James DeLisle]

On 28/10/2026 13:28, Andrew Lunn wrote:
> On Wed, Oct 28, 2026 at 12:27:18PM +0100, Caleb James DeLisle wrote:
>> On 28/10/2026 03:04, Andrew Lunn wrote:
>>> On Wed, Oct 28, 2026 at 01:13:12AM +0000, Caleb James DeLisle wrote:
>>>> 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>
>>>> Acked-by: Rob Herring (Arm) <robh@kernel.org>
>>>> ---
>>>>    .../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:
>>> Looking at just what is here, all i see is a comment. There is no new
>>> property? Yet the subject is "add passthrough mode"?
>>
>> Indeed, I used "add" to mean "add documentation of". Per my reply on patch
>> 3, I think we really prefer to avoid special properties and let this
>> constitute (very) limited support for a switch hierarchy.
>>
>> If there's another verb you think makes a better commit line, I'd be happy
>> to adjust.
> [Goes and looks at mediatek,mt7530.yaml]
>
> So pass through is not an existing property. You are also not adding a
> new property 'passthrough'.
>
> What you appear to be saying is that the MT7530 and MT7531 are hard
> coded by the driver into pass through mode? So it is in effect a
> property of the compatible? So maybe put this comment next to the
> compatible? And make the subject/commit message talk about the
> compatible being hard coded for pass through?


Perhaps I should rephrase as follows:


"""

The MT7530 and MT7531 switches do not support switch hierarchies except in
the case that a switch is configured with only one CPU port and one DSA port
that links to a downstream switch. In this configuration, the upstream switch
acts as a blind pass-through to the downstream switch.
"""


> Looking forward, what happens when somebody wants to use the switch as
> a switch? Or you figure out how to do proper D in DSA? It becomes
> messy because you need to add a property indicating the compatible
> should actually be a switch?


Full hierarchy (using DSA tag) is impossible because of what seems to be 
a hardware bug where switches don't distinguish themselves in the tag. 
However it is possible (albeit ugly) to enable ports on both switches 
providing the same numbered port isn't enabled on both switches.


If somebody ever implements that, even if they remove this passthrough 
support, users of the current passthrough capability will not notice a 
difference because that will do the same as this.


>
> So maybe you really do need a property?
>
> But DT describes hardware, and pass through is configuration. So maybe
> it should not be in DT at all, Maybe the driver needs to look at the
> DT tree, figure out a port is used as a DSA port, and put the switch
> into pass through all on its own? Once D in DSA is supported, the
> addition ports become available, but no other changes are needed in
> the DT blob?


Previously there was no documentation regarding hierarchies, and any 
hierarchy would fail at runtime. I think a brief note that this one case 
of hierarchy is allowed is nice for the users, though it does bend the 
definition of "describing the hardware" so it could also be excluded. I 
may be persuaded either way.


Thanks,

Caleb


>
> Sorry, going around in circles, but hopefully it helps you think about
> the big picture.
>
> 	   Andrew

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

[Caleb James DeLisle]

On 28/10/2026 13:50, Andrew Lunn wrote:
> On Wed, Oct 28, 2026 at 12:59:23PM +0100, Caleb James DeLisle wrote:
>> On 28/10/2026 03:20, Andrew Lunn wrote:
>>> On Wed, Oct 28, 2026 at 01:13:15AM +0000, Caleb James DeLisle wrote:
>>>> 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().
>>> I would suggest just moving it to probe. Don't also have it in setup.
>>> KISS.
>>
>> I don't think there's a way to do that which will work. And I guess I need
>> to add a few comments to explain why. Basically when dsa_register_switch()
>> is called, one of three things can happen:
>>
>> 1. return error (often without calling mt753x_setup), typically this is
>> EPROBE_DEFER because the ethernet driver is not there yet.
>>
>> 2. return 0 without calling mt753x_setup (because not all switches in the
>> tree are yet probed)
>>
>> 3. call mt753x_setup, complete setup, and return 0
>>
>>
>> In case 1, mt7530_setup_mdio must not be called, because it would be setting
>> up an MDIO on a driver that's about to get deleted.
> Which is fine, because when the probe fails, you unregister the MDIO
> bus. That should work. The second time around, you do have the netdev,
> and it should work.


When I tried doing this it did not work. The first issue is that 
mt7530_setup_mdio() uses a static int and increments it to make its ID. 
And normally this would create higher numbered busses but still work, 
however from what I remember there's something (not mt7530) that 
searches for mt7530-0 anyway and is unable to find it. I don't remember 
now what was doing it but I remember thinking I would need a global ID 
generator with reuse of released IDs.


Furthermore, I think I'd have to test all of the PHYs that are 
downstream of this switch because this is the first time we would 
actually be asking them actually register and unregister multiple times 
each startup, so if there are bugs, then they're kinda my bugs.


So if this is a requirement, then I think this patchset will need to go 
on the back burner and that support would probably be its own 
patch/patchset. So please let me know how you're feeling on this because 
I don't want to put too much time into the rest of this patchset if it 
is going to have to get shelved for a while anyway.


Thanks,

Caleb


>
> It is not great, you are doing extra work, but many DSA drivers are
> structured like this, the MDIO bus is registered in probe. The bus is
> scanned, PHY are found, and then the probe fails, and the PHYs are all
> thrown away etc.
>
> Hanging one switch off another switch like this using MDIO, kind of
> pushes you in this direction. Typical DSA setups don't do this, there
> is one MDIO bus which all switches hang off, sometimes with a
> multiplexor to make one MDIO bus act like many.
>
> Something to look at which might be interesting, Vladimir had some
> patches which allowed for partial switch topologies, where you could
> take a switch in a DSA setup off line. Maybe there is something in
> those patches which help, you can bring the switches up independently?
>
> 	    Andrew

Re: [PATCH v3 0/7] net: dsa: mt7530: support EcoNet EN751221

[Cristian Papa]

Hi Caleb,

I tested v3 on a TP-Link Archer XR500v v1 (EN7526G: on-die switch plus the
MCM gigabit switch).

Setup: OpenWrt with kernel 6.18.44. I took the four mt7530 files from
econet_eth at 3a8bcd2e6398 (they contain every line added by 1/7, 3/7,
4/7, 5/7 and 7/7 of v3) and booted from RAM over TFTP, nothing written to
flash.

With the DT from en751221g.dtsi in openwrt#25330 (MCM "econet,en751221",
no "mediatek,mcm", dsa,member <0 1> on the on-die switch and <0 0> on the
MCM):

- 18/18 boots: all 10 TRGMII lanes found a window, no "calibration
  failed", "TRGMII inter-switch link initialized", DSA tree set up, ping
  over lan4.
- The chosen taps were stable across boots (+/-1): SoC->MCM 15-22,
  MCM->SoC 27-29. In the SoC->MCM direction the window always starts at
  tap 0.
- 6 of those boots ran iperf3 for 6 s each way through lan4: 0 RX CRC
  errors on on-die port 5 (~1.05M frames) and on MCM port 6 (~1.29M
  frames). In 4 of 10 boots MCM port 6 already had 1 CRC error before any
  traffic, probably from the calibration; it did not grow.

Following the binding in 6/7, the MCM does not come up on this board:

- "econet,en751221-gswitch": mt7530-mdio.c in 7/7 still matches
  "econet,en751221", so nothing probes the MCM and there are no LAN
  ports.
- "mediatek,mcm", which the binding requires for -gswitch: the probe
  fails with "Couldn't get our reset line", since nothing provides an
  "mcm" reset yet (the TODO in en751221g.dtsi).

So I think 7/7 should match the new compatible, and the binding should not
require "mediatek,mcm" while there is no "mcm" reset for the EN751221.

Not covered yet: cold boots, and the other LAN ports beyond link-up on
lan3/lan4. I'm happy to give a Tested-by once the compatible strings line
up, and to rerun anything you'd like; the full boot logs are available.

Thanks,
Cristian

Re: [PATCH v3 0/7] net: dsa: mt7530: support EcoNet EN751221

[Caleb James DeLisle]

Hello Christian,


Thank you for testing!


On 28/10/2026 17:03, Cristian Papa wrote:
> Hi Caleb,
>
> I tested v3 on a TP-Link Archer XR500v v1 (EN7526G: on-die switch plus the
> MCM gigabit switch).
>
> Setup: OpenWrt with kernel 6.18.44. I took the four mt7530 files from
> econet_eth at 3a8bcd2e6398 (they contain every line added by 1/7, 3/7,
> 4/7, 5/7 and 7/7 of v3) and booted from RAM over TFTP, nothing written to
> flash.
>
> With the DT from en751221g.dtsi in openwrt#25330 (MCM "econet,en751221",
> no "mediatek,mcm", dsa,member <0 1> on the on-die switch and <0 0> on the
> MCM):
>
> - 18/18 boots: all 10 TRGMII lanes found a window, no "calibration
>    failed", "TRGMII inter-switch link initialized", DSA tree set up, ping
>    over lan4.
> - The chosen taps were stable across boots (+/-1): SoC->MCM 15-22,
>    MCM->SoC 27-29. In the SoC->MCM direction the window always starts at
>    tap 0.
> - 6 of those boots ran iperf3 for 6 s each way through lan4: 0 RX CRC
>    errors on on-die port 5 (~1.05M frames) and on MCM port 6 (~1.29M
>    frames). In 4 of 10 boots MCM port 6 already had 1 CRC error before any
>    traffic, probably from the calibration; it did not grow.
>
> Following the binding in 6/7, the MCM does not come up on this board:
>
> - "econet,en751221-gswitch": mt7530-mdio.c in 7/7 still matches
>    "econet,en751221", so nothing probes the MCM and there are no LAN
>    ports.


Whoops, I see I made an error on patch 7 and did not update the name to 
"gswitch", I will send a v4 soon.


> - "mediatek,mcm", which the binding requires for -gswitch: the probe
>    fails with "Couldn't get our reset line", since nothing provides an
>    "mcm" reset yet (the TODO in en751221g.dtsi).


Also expected as setting "mediatek,mcm" requires specifying the reset 
line, and reset for the MCM is on its own unique register and is not yet 
supported by en7523-clk.c reset controller. The OpenWrt implementation 
is a bit cheating by not bumping the reset line at all right now but in 
my testing it was not different when I manually bumped it so it seems 
okay for now


>
> So I think 7/7 should match the new compatible, and the binding should not
> require "mediatek,mcm" while there is no "mcm" reset for the EN751221.
>
> Not covered yet: cold boots, and the other LAN ports beyond link-up on
> lan3/lan4. I'm happy to give a Tested-by once the compatible strings line
> up, and to rerun anything you'd like; the full boot logs are available.


I'll send a v4 and I will synchronize the PR on OpenWrt to it so you can 
test easily.


Thanks,

Caleb


>
> Thanks,
> Cristian

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

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The new helper mt753x_ctrl_phy_addr()
  (drivers/net/dsa/mt7530.c:49-57) handles a NULL priv->mdiodev by…
- [Low] The commit message says the control PHY is found by taking the
  switch's MDIO address, adding 1, and taking it "modulo 31".

--

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

This commit replaces the MT753X_CTRL_PHY_ADDR() macro with a
mt753x_ctrl_phy_addr() helper. The helper warns and returns 0 when
priv->mdiodev is NULL. It is also meant to give MMIO implementations a
place to supply their own control PHY address.

> 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]
Is "modulo 31" right here? The code computes:

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

PHY_MAX_ADDR is 32, so this masks with 31, which is modulo 32. For a
switch at address 31, (31 + 1) % 31 would give 1. The code gives 0,
which is the PHY the message calls the special one.

Should this say modulo 32, or say that it wraps around the 32-address
MDIO space?

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7781a63b4e6fd..d10194488bdee 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;

[Severity: Low]
When priv->mdiodev is NULL, this returns 0. That is also a valid PHY
address, so callers can't tell the fallback from a real result.

At this commit, core_write() and core_rmw() then use priv->bus without
checking it:

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

On MMIO devices, mt7988_probe() sets priv->bus = NULL and never sets
mdiodev. If the fallback were hit there, could the WARN be followed by a
NULL bus dereference in core_write() or core_rmw()?

In mt7531_setup(), the same fallback would send the CORE_PLL_GROUP4
write and the EEE advertisement writes to PHYs 0 to 4.

This doesn't look reachable at this commit. Every caller runs only for
MT7530/MT7621/MT7531, which are probed through mt7530-mdio.c and always
have both priv->bus and priv->mdiodev set.

Two later patches in the series also cover this:

  - "net: dsa: mediatek: support PLL setup on MMIO MT7530" adds a
    child_bus fallback and a WARN_ON_ONCE(!bus) check to core_write()
    and core_rmw().
  - "net: dsa: mediatek: support EN751221 switch" adds the EN751221
    special case to this helper.

That leaves a design question. Should the helper return an error for a
missing mdiodev, so callers can bail out instead of using address 0?

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

[ ... ]

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

Re: [PATCH v3 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: 1 · Low: 1

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

- [Medium] The binding tells DT authors to model passthrough as a two-
  switch DSA cascade ('the upstream switch is configured with a DSA link…
- [Low] The passthrough paragraph added to the description of
  Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml (lines…

--

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

This commit documents a "passthrough" mode for MT7530/MT7531 switches in
the mediatek,mt7530 binding description. In this mode the upstream switch
forwards traffic between its CPU port and a DSA link to a downstream
switch, and it does not alter the MediaTek tag.

> 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

[Severity: Medium]
Should this also say which DSA switch index each switch has to use?

The MediaTek tag carries only a 3-bit source port and no switch ID.
mtk_tag_rcv() always looks the port up on switch index 0:

net/dsa/tag_mtk.c:mtk_tag_rcv() {
    ...
	port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);

	skb->dev = dsa_conduit_find_user(dev, 0, port);
	if (!skb->dev) {
		kfree_skb(skb);
		return NULL;
	}
    ...
}

dsa_conduit_find_user() only matches DSA_PORT_TYPE_USER ports on the
given switch index. In passthrough mode the upstream switch forwards the
downstream switch's tag unchanged, so port is a downstream port number.

The two switches can't share an index, because
dsa_switch_parse_member_of() returns -EEXIST for a duplicate.

Take a DT that uses the natural ordering: the CPU-facing upstream switch
at dsa,member switch index 0 and the downstream switch at index 1. Would
every received frame then be looked up on the upstream switch?

That switch has no user ports in passthrough mode. The lookup would
return NULL and the frame would be freed, so all RX traffic on the
downstream user ports would be lost.

No later patch in the series seems to change this. tag_mtk.c is
unchanged, the driver doesn't touch ds->index, and the final binding has
no passthrough example or dsa,member guidance.

Could the text say that the downstream switch must be switch index 0 in
dsa,member and the upstream switch must use a non-zero index? An example
showing this would also work.

> +  passthrough mode the upstream switch is restricted, with no other ports or
> +  switch configuration allowed.

[Severity: Low]
These topology rules are stated only in prose. Could they be encoded in
the schema as well?

Ports match ^(ethernet-)?port@[0-6]$ with additionalProperties: true.
The per-port reg and phy-mode rules apply only to ports that have an
ethernet property. Those rules are the top-level if: required:
[ ethernet ] block and the builtin-dsa-port, mt7530-dsa-port and
mt7531-dsa-port $defs.

As a result, a port with a link phandle on the upstream switch gets no
reg or phy-mode check. A node with extra user ports next to the CPU and
link ports also passes dt_binding_check.

mt753x_check_passthrough() in the driver does reject that case at probe
with "Only two ports allowed in passthrough mode". The schema does not.

The port reg description still reads:

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

Should it mention DSA link ports too?

Later in the series, "dt-bindings: net: dsa: mediatek,mt7530: add
econet,en751221" adds mt7530-passthrough-dsa-port, which requires the
link port to be reg 6 and trgmii. That $def applies only to the
downstream econet,en751221-gswitch. The upstream econet,en751221-switch
uses builtin-dsa-port, so its link port is still unconstrained.

There is also a small wording mismatch. The commit message says:

    MT7530 switches can be configured to blindly pass packets between their

but the binding text says "The MT7530 and MT7531 switches". Could these
be made to match?

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

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

[netdev-bot+sashiko]

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

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

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

- [High] mt753x_check_passthrough() uses one counter for every available
  port that has an `ethernet` (CPU) phandle or a `link` (DSA) phandle.
- [High] net: dsa: mt7530: NULL pointer dereference in
  mt7531_mac_config() on DSA port
- [Medium] In passthrough mode, mt753x_cpu_port_enable() calls
  mt7530_port_change_mtu(ds, port, MT7530_MAX_MTU) to raise the global…
- [Medium] The patch adds support for this switch being the downstream
  switch, with a DSA port as its upstream link.
- [Medium] Passthrough receive works only if the downstream switch is DSA
  switch index 0.
- [Medium] In the user-port branch, mt7530_port_enable() replaces the
  per-port `dp->cpu_dp->index` with the switch-wide…
- [Medium] In passthrough mode, mt753x_cpu_port_enable() writes
  MT7530_PVC_P(port) = VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |…
- [Medium] In passthrough mode the upstream switch's CPU port no longer
  has PORT_SPEC_TAG; mt753x_cpu_port_enable() writes…
- [Low] In passthrough mode, mt753x_cpu_port_enable() unconditionally
  sets MT7530_LOOP_DET_DISABLE (BIT(14)) in MT753X_MTRAP (0x7804) with a…
- [Low] When the downstream DSA link port is enabled,
  mt7530_port_enable() does a full `regmap_write(priv->regmap,…

--

net: dsa: mediatek: add support for passthrough mode

This adds a passthrough mode to the mt7530 driver. In this mode a switch
forwards all traffic between its CPU port and a link to a second,
downstream MT7530 without changing the MediaTek tag, and most switchdev
ops become no-ops. Passthrough is detected at probe time from the device
tree, and the switch can also act as the downstream switch, with a DSA
link as its upstream port.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index d10194488bdee..3eb2e11d69856 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> @@ -1286,13 +1292,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: Medium]
Does this EG_DISABLED setting survive setup? As soon as
mt753x_cpu_port_enable() returns, the same loop iteration in mt7530_setup()
runs this:

	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));

mt7531_setup_common() does the same update. So the upstream port ends up
with EG_TAG set to consistent. The downstream DSA port is programmed later
from mt7530_port_enable() and keeps EG_DISABLED.

That doesn't match the commit message, which says both ports are
"configured as MT7530_VLAN_EG_DISABLED and MT7530_VLAN_TRANSPARENT".

Should the egress tag update be skipped for the upstream port in
passthrough mode?

[Severity: Medium]
Without PORT_SPEC_TAG on the CPU port, frames from the host go through the
normal ARL and trap logic. mt753x_trap_frames() still installs the BPC,
RGAC1 and RGAC2 rules unconditionally. Those rules use TO_CPU_FW_CPU_ONLY
for PAE, BPDU and 01:80:C2:00:00:01/02/03/0E.

Say the host sends an STP BPDU, LACPDU, EAPOL or LLDP frame on a downstream
user port. The real DA is still at the front of the frame, because the MTK
tag sits after the SA.

Would that frame match the trap rules and be sent to the CPU port? That is
its own ingress port, so the frame would never reach the downstream switch.

Control frames from the downstream switch to the host would still work, so
the failure would be one-way. This depends on the switch applying trap
rules to untagged frames that enter the CPU port, which I could not confirm
without the datasheet.

> +
> +		/* 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);

[Severity: Medium]
Does this call do anything? This branch runs only when priv->is_passthrough
is true. This patch also adds the following as the first check in
mt7530_port_change_mtu():

	if (priv->is_passthrough)
		return 0;

So MT7530_GMACCR is never written, and MAX_RX_PKT_LEN on the passthrough
switch keeps its reset default. Later DSA .port_change_mtu calls for this
switch return early in the same way.

The upstream switch no longer parses the MTK tag, so it counts those 4
bytes as payload. Meanwhile .port_max_mtu still reports MT7530_MAX_MTU and
.port_change_mtu reports success.

Can the downstream switch and the DSA core then accept MTUs whose frames
the upstream switch drops? Clearing AGC_L2LEN_CHK turns off the length field
check, but it does not change the MAC's maximum frame length.

> +
> +		/* Loop detection has no value in passthrough mode */
> +		regmap_set_bits(priv->regmap, MT753X_MTRAP,
> +				MT7530_LOOP_DET_DISABLE);

[Severity: Low]
Is BIT(14) of MT753X_MTRAP valid on every chip that can reach this code?
mt753x_cpu_port_enable() is also called from mt7531_setup_common(), which
covers MT7531, MT7988, EN7581 and AN7583.

The MTRAP layout differs between chip families. For example, BIT(8) is
MT7530_P6_DIS on MT7530 but MT7531_CHG_STRAP on MT7531. Every other MTRAP
access in the driver is limited to MT7530-family code paths.

On those other chips, could this set an undocumented strap bit instead of
disabling loop detection? Should the write depend on priv->id?

> +	} else {
> +		/* Not passthrough, enable DSA tag handling on CPU port. */
> +		regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
> +	}

[ ... ]

> @@ -1322,6 +1356,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 +1367,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));
>  
> -		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));

[Severity: Medium]
Does this change behaviour on switches with more than one CPU port?
dsa_switch_upstream_port() returns the upstream port of the first available
port on the switch, not of the port being enabled:

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

mt753x_port_change_conduit() and mt7530_update_port_member() still base
priv->ports[port].pm on the port's own dp->cpu_dp.

Take an MT7531 with CPU ports 5 and 6. lan0, the first available port, is
moved to CPU port 6 with .port_change_conduit, and lan1 stays on CPU
port 5. If lan1 is then taken down and up, this code ORs BIT(6) into lan1's
pm, which already has BIT(5). mt753x_port_change_conduit() clears only the
old cpu_dp bit, so the extra bit is never removed.

Would lan1 then forward to both CPU ports?

Right now this is hidden, because the new probe check rejects dual-CPU
device trees that have user ports (see below). It becomes reachable once
that check is fixed.

> +	} 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);
> +

[Severity: High]
Can a DSA link port now reach mt7531_mac_config() and oops there?

This patch makes DSA links a supported setup on MT7531 as well. That
happens through this branch, where the passthrough switch programs its
downstream link, and through the dsa_is_upstream_port() change in
mt7531_setup_common(), where a downstream switch has a DSA link as its
upstream port.

However, mt7531_mac_config() still assumes that every RGMII port has a
user netdev:

	if (phy_interface_mode_is_rgmii(interface)) {
		dp = dsa_to_port(ds, port);
		phydev = dp->user->phydev;
		mt7531_rgmii_setup(priv, interface, phydev);
	}

In struct dsa_port, user and conduit share a union. CPU ports set
conduit and user ports set user. DSA ports set neither, so dp->user is
NULL for them.

Take an MT7531BE without SGMII on port 5 (!priv->p5_sgmii), where port
5 is a DSA link with an RGMII phy-mode. This can be the downstream link
of a passthrough MT7531BE or the uplink of a downstream one. The call
chain is dsa_port_setup() -> dsa_port_enable_rt() -> phylink_start() ->
phylink_major_config() -> mt753x_phylink_mac_config() ->
mt7531_mac_config(). Would that dereference NULL during switch setup?

Should mt7531_mac_config() look up the phydev only for user ports and
pass NULL otherwise? mt7531_rgmii_setup() already handles !phydev by
applying the delays implied by the phy-mode, which seems to be what a
DSA link needs.

[ ... ]

> +		/* 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: Low]
Is a full register write intended here? It clears every other field in the
upstream port's PCR. That includes the fallback mode that
mt753x_cpu_port_enable() set just before:

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

It also clears PORT_PRI, the PCR EG_TAG field and the mirror bits.

The write also bypasses priv->ports[upstream_pt].pm, which stays 0. Every
other PCR writer uses regmap_update_bits(..., PCR_MATRIX_MASK, pm). As a
result, the forwarding bit is protected only by the is_passthrough early
returns and the check below.

mt7530_port_disable() also has nothing to undo this PCR write or the MFC
flood bits set for the DSA port.

Would regmap_update_bits() with PCR_MATRIX_MASK, plus an update of the pm
cache, be safer?

>  	}
>  	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);

[ ... ]

> @@ -1862,6 +1949,10 @@ mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>  {
>  	struct dsa_port *dp = dsa_to_port(ds, port);
>  	struct dsa_port *cpu_dp = dp->cpu_dp;
> +	struct mt7530_priv *priv = ds->priv;
> +
> +	if (priv->is_passthrough)
> +		return 0;

[Severity: Medium]
When this switch is the downstream one, is cpu_dp->index still a local port
number? dsa_tree_setup_default_cpu() sets dp->cpu_dp for the downstream
switch's ports to the CPU port on the upstream switch, so the index belongs
to another switch. is_passthrough is false on the downstream switch, so the
new early returns don't apply.

The patch moves mt7530_port_enable(), the setup loops and
mt7530_port_change_mtu() to the local upstream port. Three places still
treat cpu_dp->index as a local port:

mt7530_port_vlan_filtering():
		mt7530_port_set_vlan_aware(ds, port);
		mt7530_port_set_vlan_aware(ds, cpu_dp->index);

mt7530_update_port_member():
	struct dsa_port *cpu_dp = dp->cpu_dp;
	u32 port_bitmap = BIT(cpu_dp->index);

mt753x_conduit_state_change():
	mask = BIT(cpu_dp->index);

Suppose the downstream uplink port number differs from the upstream CPU
port number. Would a bridge join, a bridge leave or a BR_ISOLATED change
then overwrite p->pm and drop the uplink bit that mt7530_port_enable() set?
If that index happens to be a local user port, frames could also be
forwarded to it without the isolation check.

In the same setup, would VLAN filtering make an unrelated local port
VLAN-aware instead of the uplink? Would trapped frames go to the wrong
local CPU_PORT in MT753X_MFC? The patch only updates the comment in
mt753x_conduit_state_change().

The EN751221 hardware in this series seems to work only because both
indices are 6.

[ ... ]

> @@ -3399,6 +3505,55 @@ 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)
> +{

[ ... ]

> +	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;
> +	}

[Severity: High]
Will this break probe on existing boards that have two CPU ports? CPU ports
(ethernet) and DSA links (link) share one counter. Two of them plus any
user port returns -EINVAL, and mt7530_probe_common() then fails the whole
probe.

Several in-tree device trees have user ports plus port@5 with
ethernet = <&gmac1> and port@6 with ethernet = <&gmac0>. Examples are
mt7622-bananapi-bpi-r64.dts, mt7623n-bananapi-bpi-r2.dts, mt7986a/b-rfb.dts
and the MT7621 boards based on mt7621.dtsi.

On BPI-R64 this gives passthrough_ports == 2 and enabled_ports == 7, since
of_parse_phandle() returns the gmac node whatever its status. Would all of
these boards lose switch networking with "Only two ports allowed in
passthrough mode"?

The check is also wrong in the other direction. mt7621.dtsi with swp0-4
disabled has two CPU ports and nothing else, so it would be treated as
passthrough without any DSA link. A normal cascade with a CPU port, a DSA
link and user ports would also be rejected.

Later patches in the series don't seem to change this function.

Should the check require exactly one ethernet port and one link port, and
leave other layouts to the existing code paths?

[Severity: Medium]
Does passthrough receive work only if the downstream switch has DSA switch
index 0? The MTK tag has no switch ID, and mtk_tag_rcv() hardcodes
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;
	}
	...
}

dsa_conduit_find_user() requires an exact match on dp->ds->index. The
natural numbering gives the CPU-attached upstream switch
dsa,member = <0 0>. With that numbering, every frame from the downstream
switch is looked up on the upstream switch, finds no user port, and is
freed without any message.

Could this requirement be checked here, or at least documented in the
binding?

> +
> +	return passthrough_ports == 2;
> +}

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

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

[netdev-bot+sashiko]

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

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

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

- [Medium] core_write()/core_rmw() in drivers/net/dsa/mt7530.c now fall
  back to priv->child_bus when priv->bus is NULL.
- [Low] The new @child_bus kernel-doc in struct mt7530_priv
  (drivers/net/dsa/mt7530.h) reads: 'The bus created by the device, on…
- [Low] Claim: mt753x_setup() calls priv->info->sw_setup() (mt7530_setup
  -> mt7530_pll_setup -> core_clear/core_write/core_set) before…
- [Low] Claim: on MMIO, core_write()/core_rmw() call
  mt753x_ctrl_phy_addr() while priv->mdiodev is NULL, so they WARN and…

--

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

This commit adds a child_bus pointer to struct mt7530_priv. It is set to
the MDIO bus registered in mt7530_setup_mdio(). When priv->bus is NULL,
which is the MMIO case, core_write() and core_rmw() now use child_bus
instead, so core PLL registers can be reached on MMIO switches.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3eb2e11d69856..d09ff207e2dfc 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);

[Severity: Medium]
Is anything serializing the MMD indirect sequence when the code falls
back to child_bus here?

mt7530_mutex_lock() only takes a lock when priv->bus is set:

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

On MMIO, mt7988_probe() sets priv->bus = NULL, so this lock does nothing.
The steps then call bus->write() and bus->read() directly, so
child_bus->mdio_lock is never taken either.

Each step ends up in mt7531_ind_c22_phy_write(), which holds
priv->reg_mutex for a single IAC transfer and releases it between steps.
The sequence is MII_MMD_CTRL, then MII_MMD_DATA=reg, then
MII_MMD_CTRL|NOINCR, then MII_MMD_DATA=val, with a read in core_rmw().

Other users of the same PHY address on child_bus hold
child_bus->mdio_lock. That includes phylib's phy_read_mmd() and
phy_write_mmd(), e.g. genphy_c45_read_eee_abilities() from phy_probe() or
genphy_c45_eee_is_active() from the state machine, and SIOCSMIIREG via
phy_mii_ioctl().

Can one of them re-target the MMD address latch partway through
core_write() or core_rmw()? If so, the PLL value would land in the wrong
MMD register, or core_rmw() would read one register and write the merged
value back to it.

In the MDIO variant, priv->bus->mdio_lock is held for the whole sequence,
and child bus accesses take that same lock, so this cannot happen there.

Later in the series, "net: dsa: mediatek: support EN751221 switch" makes
the on-die EN751221 (MMIO) reach this path:

mt7530_mac_config()->mt7530_setup_port6()->core_write()/core_rmw()

mt7530_mutex_lock() is still unchanged at the end of the series. Should
core_write() and core_rmw() lock child_bus->mdio_lock when they fall back
to child_bus?

>  
>  	/* 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;
> +

[Severity: Low]
On the MMIO path priv->mdiodev is NULL as well. At this commit,
core_write() and core_rmw() get ctl_phy from:

static int
mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
{
	if (WARN_ON_ONCE(!priv->mdiodev))
		return 0;
	...
}

Wouldn't a core_* call through child_bus therefore WARN and target PHY
address 0?

No MMIO variant reaches core_* at this commit. The later commit "net: dsa:
mediatek: support EN751221 switch" adds an ID_EN751221 case that returns
12 before the WARN. So this looks resolved by the end of the series. Is
it worth noting that dependency in this commit message?

>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */

[ ... ]

> @@ -2472,8 +2484,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	if (priv->irq_domain && !mnp)
>  		mt7530_setup_mdio_irq(priv);
>  
> +	priv->child_bus = bus;
> +

[Severity: Low]
This is the only place child_bus is assigned. mt753x_setup() calls
priv->info->sw_setup() before mt7530_setup_mdio(). Could a core_* call
from sw_setup run before child_bus is set?

That ordering would only matter if an MMIO variant used mt7530_setup(),
which calls mt7530_pll_setup(). None does: the MMIO entries all use
mt7988_setup, and ID_EN751221_EXT is probed through mt7530-mdio.c. So the
ordering on its own does not look like a problem.

There is still a corner case. If the DT has an mdio node that is present
but disabled, mt7530_setup_mdio() returns early:

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

That path returns success and leaves child_bus NULL. For EN751221 later
in the series, mt7530_setup_port6() would then hit WARN_ON_ONCE(!bus) and
skip the TRGMII PLL writes. Is that the intended behaviour for that
configuration?

>  	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 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]
This isn't a bug, but "this is bus indirectly accesses" reads garbled.

The comment also suggests child_bus is always valid on MMIO. It is NULL
until mt7530_setup_mdio() runs, when the mdio node is disabled, and after
devm_of_mdiobus_register() fails. Could the comment mention those cases,
and that core_write() and core_rmw() only use child_bus as a fallback
when bus is NULL?

>   * @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

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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

- [High] net: dsa: mt7530: recursive reg_mutex lockdep class on
  downstream switch
- [Medium] The patch moves mt7530_setup_mdio_irq() out of
  mt7530_setup_mdio(), where it ran just before…
- [Medium] In exactly the case the patch targets (the DSA tree is
  incomplete when dsa_register_switch() returns), mt7988_probe() calls…
- [Medium] mt7988_probe() ignores the return value of the newly exported
  mt7530_setup_mdio() and always returns 0.
- [Medium] mt7530_setup_mdio() can now run from mt7988_probe() before DSA
  has set the switch up.
- [Medium] The early MDIO registration cannot complete when the upstream
  switch's 'mdio' node has PHY children whose interrupts come from the…
- [Medium] The new 'Already done' check (if (priv->child_bus) return 0;)
  in mt7530_setup_mdio() is an unlocked flag used as an initialisation…
- [Medium] Because the MDIO bus is now registered in probe, before the
  tree is set up, the later mt7988_setup() (run from mt753x_setup()…
- [Low] The commit message says "In MMIO mode the switch provides its own
  MDIO bus which connects to its integrated PHYs".

--

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

This patch lets an MDIO-attached switch sit on the child MDIO bus of an
MMIO MT753x switch. mt7530_setup_mdio() is made safe to call more than
once, it is exported, and mt7988_probe() now calls it right after
dsa_register_switch(). The mt7530_setup_mdio_irq() call also moves out of
mt7530_setup_mdio() and into mt753x_setup().

> 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);

[Severity: High]
Will the cascade this enables trip lockdep on the shared reg_mutex
class?

Both switches initialize priv->reg_mutex at the same mutex_init() call
site in mt7530_probe_common(), so the upstream MMIO switch and the
downstream MDIO switch share one lockdep class.

Once an MDIO-attached MT7530/MT7531 sits on the upstream switch's
child_bus, every regmap access on the downstream switch first goes
through mt7530_mdio_regmap_lock(). That takes
up->child_bus->mdio_lock with MDIO_MUTEX_NESTED. It then calls the
upstream bus callbacks, which take the upstream priv->reg_mutex at
subclass 0.

The downstream driver also holds its own reg_mutex across regmap
accesses. The first such access during setup is mt7530_port_disable():

mt7530_port_disable()               down->reg_mutex (0)
  regmap_update_bits(priv->regmap, MT7530_PCR_P(port), ...)
    mt7530_mdio_regmap_lock()       up->child_bus->mdio_lock (1)
    bus->write()
      mt753x_phy_write_c22()
        mt7531_ind_c22_phy_write()  up->reg_mutex (0)

The unguarded regmap accesses earlier in mt7530_setup() and
mt7531_setup() already record mdio_lock:1 -> reg_mutex:0. Taking the
upstream reg_mutex while the downstream one is held then looks to
lockdep like recursive locking of a single class.

The instances are different, and the order is always downstream
before upstream, so this is probably not a real deadlock. Under
CONFIG_PROVE_LOCKING, though, the splat would fire deterministically
during setup. Lockdep would then be disabled for everything else on
the system. The same pattern repeats on the port, VLAN and FDB paths
that hold reg_mutex around regmap calls.

Before this patch the tree never completed in this topology, so this
pairing was never reachable.

How would you like to annotate this? One option is a per-instance
lock class key for reg_mutex, using lockdep_register_key() with
__mutex_init(). Another is mutex_lock_nested() on the upstream
indirect PHY access path. Or is there a different approach you would
prefer?

[Severity: Medium]
Should the return value of mt7530_setup_mdio() be passed back here?

mt7530_setup_mdio() can fail with -ENOMEM from devm_mdiobus_alloc(). It
can also return any error from devm_of_mdiobus_register(). That includes
-EPROBE_DEFER, which fwnode_mdiobus_phy_device_register() passes up when a
PHY's interrupt provider is not ready yet.

On failure priv->child_bus goes back to NULL, but probe still returns 0,
so the driver core never re-probes this device. In the cascade topology
the only other caller is mt753x_setup(). That runs only once the tree is
complete, and completing the tree needs the downstream switch on this
same bus.

Wouldn't the tree then never complete, with nothing logged except the
dev_err()?

In the !mnp case, ds->user_mii_bus is also left pointing at a bus that
was allocated but never registered.

[Severity: Medium]
Is there a dependency cycle when the PHYs under the "mdio" node get their
interrupts from this switch?

arch/arm64/boot/dts/mediatek/mt7988a.dtsi uses that layout. The switch
node is the interrupt-controller, and the PHYs use "interrupts = <N>"
with the switch as their interrupt parent.

mt7530_setup_irq() is still only called from mt753x_setup(), so
priv->irq_domain does not exist yet at this point. The registration path
would be:

mt7988_probe()
  mt7530_setup_mdio()
    devm_of_mdiobus_register()
      __of_mdiobus_parse_phys()
        fwnode_mdiobus_phy_device_register()
          fwnode_irq_get() -> -EPROBE_DEFER

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
    ...
	if (rc == -EPROBE_DEFER)
		return rc;
    ...
}

__of_mdiobus_parse_phys() aborts on that error. __of_mdiobus_register()
then unregisters the whole bus, including the downstream switch device.

The IRQ domain cannot exist until the tree completes, and the tree cannot
complete until this bus registers. Propagating the error would not break
that cycle by itself. Does the IRQ domain need to be created before this
early registration?

[Severity: Medium]
Does this probe the PHYs before the switch has been reset and set up?

This patch targets the case where the tree is still incomplete after
dsa_register_switch(). There, the "mdio" bus is registered and every PHY
child is probed before mt753x_setup() and its sw_setup callback,
mt7988_setup(), have run. When the downstream switch completes the tree,
mt7988_setup() does this:

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);
    ...
}

For the MT7988/EN7581 class GEPHYs, mt7988_phy_probe() in
drivers/net/phy/mediatek/mtk-ge-soc.c applies three things only at probe
time: the LED polarity fixups, the MTK_PHY_RXADC_CTRL_RG7 TX power-saving
disable, and mt798x_phy_calibration().

If SYS_CTRL_PHY_RST clears those registers, does anything apply them
again?

On AN7583, the early PHY ID reads and the probe also run before
mt7988_setup() programs AN7583_GEPHY_CONN_CFG (PHY core register clock
select and the ETHER_AFE power-down clear).

All the MMIO variants use mt7988_setup. That includes EN751221, which the
later patch "net: dsa: mediatek: support EN751221 switch" adds, and that
patch does not change this ordering.

[Severity: Medium]
Related to the above, what keeps the reset in mt7988_setup() from
overlapping with MDIO users of child_bus?

The bus is now registered at probe time. mt7988_setup() runs later, from
mt753x_setup() during tree setup on the downstream switch's probe thread.
By then child_bus and its PHY and downstream devices are already
registered.

mt7988_setup() does not take child_bus->mdio_lock. For MMIO,
mt7530_mutex_lock() takes no lock either, because priv->bus is NULL:

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

Two MDIO users could be active at that moment: a PHY driver probing
asynchronously after module autoload, or userspace touching the PHY LED
class devices. Could reset_control_assert() or SYS_CTRL_PHY_RST cut
their transactions short?

Before this patch the reset always ran before the bus existed.

> +
> +	return 0;
>  }

[ ... ]

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

[ ... ]

> @@ -2457,6 +2457,10 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	static int idx;
>  	int ret = 0;
>  
> +	/* Already done */
> +	if (priv->child_bus)
> +		return 0;
> +

[Severity: Medium]
Is this unlocked check enough to make mt7530_setup_mdio() safe to call
from two places?

The new caller in mt7988_probe() runs after dsa_register_switch() has
dropped dsa2_mutex. The other caller, mt753x_setup(), runs from
dsa_tree_setup() with dsa2_mutex held, triggered by whichever tree member
completes the tree.

If that member is not on this switch's own MDIO bus, it can probe at the
same time on another thread, such as the deferred-probe worker or a
module load:

CPU1 (mt7988_probe)           CPU2 (dsa_tree_setup->mt753x_setup)
mt7530_setup_mdio()           mt7530_setup_mdio()
  priv->child_bus == NULL       priv->child_bus == NULL
  devm_mdiobus_alloc()          devm_mdiobus_alloc()
  devm_of_mdiobus_register()    devm_of_mdiobus_register()

Both would register a bus for the same OF "mdio" node and create
duplicate mdio and phy devices. priv->child_bus and ds->user_mii_bus
could also end up pointing at different buses.

priv->child_bus is also set before devm_of_mdiobus_register() finishes,
and cleared again on failure. A concurrent mt753x_setup() could therefore
treat a bus that later fails to register as already done. On MMIO,
core_write() and core_rmw() use child_bus whenever priv->bus is NULL.

Would a real lock, or doing the early registration in one well-defined
place, be more robust than using child_bus as a gate?

>  	mnp = of_get_child_by_name(np, "mdio");
>  
>  	if (mnp && !of_device_is_available(mnp))
> @@ -2481,9 +2485,6 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	bus->parent = dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;

[Severity: Medium]
When this runs from mt7988_probe(), is ds->phys_mii_mask set yet?

The DSA core fills it in only in dsa_switch_setup(), once the tree is
complete:

net/dsa/dsa.c:dsa_switch_setup() {
    ...
	ds->phys_mii_mask |= dsa_user_ports(ds);
    ...
}

mt7530.c never sets it itself.

Take an MMIO switch with no "mdio" DT node whose tree is still incomplete
when dsa_register_switch() returns, for example because another tree
member probes later. The probe-time call sets ds->user_mii_bus and
registers the bus with phy_mask = ~0, so none of the internal PHYs are
scanned.

When the tree later completes, mt753x_setup() calls mt7530_setup_mdio()
again. That call returns 0 straight away because priv->child_bus is set.

Wouldn't dsa_user_phy_connect() then fail with "no phy at %d" for user
ports that have no phy-handle?

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

[ ... ]

> @@ -3328,6 +3330,9 @@ mt753x_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	if (priv->irq_domain && ds->user_mii_bus)
> +		mt7530_setup_mdio_irq(priv);
> +

[Severity: Medium]
Is mt7530_setup_mdio_irq() now called too late to have any effect?

In the !mnp case, mt7530_setup_mdio() has already registered and scanned
ds->user_mii_bus by the time this runs. mdiobus_alloc_size() set every
bus->irq[] entry to PHY_POLL, and phy_device_create() copied that value
into each PHY during the scan:

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

mt7530_setup_mdio_irq() then updates only the bus array, which no
existing phy_device reads again:

	ds->user_mii_bus->irq[p] = irq;

Wouldn't the internal PHYs keep phydev->irq == PHY_POLL? Then the PHYn_LC
link-change interrupts would never be requested, and link detection would
fall back to polling.

This looks like it affects every MT7530/MT7531/MMIO switch with
"interrupt-controller" and no "mdio" subnode. That includes the ordinary
single-switch MDIO-attached setup, not just the cascade case. The commit
message does not mention this move.

>  	/* 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 5b7394f6383c7..c86bc4bca29b9 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

[Severity: Low]
This isn't a bug, but there's a typo here: "after at the end of".

The commit message also says:

  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.

mt7530_setup_mdio() is common code, and it also creates a child bus for
MDIO-attached MT7530/MT7531 (mt7530-mdio.c). An MDIO-attached MT753x with
a switch on its child bus would hit the same problem, but only
mt7988_probe() gets the workaround.

Could the commit message explain why this is MMIO-specific?

> + * the MMIO probe function allows the downstream switch to register, completing
> + * the tree.
> + */
> +int mt7530_setup_mdio(struct mt7530_priv *priv);

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

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

[netdev-bot+sashiko]

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

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

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

- [High] The binding documents `econet,en751221-gswitch` for the MCM
  switch, but even at the end of the series no driver matches that…
- [Low] In Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
  the new `$defs/mt7530-passthrough-dsa-port` (lines 240-252) is applied…
- [Low] The binding text gives the wrong number of on-die PHYs.
- [Low] The `econet,en751221-gswitch` conditional in mediatek,mt7530.yaml
  (lines 369-377) only requires `mediatek,mcm` and references…

--

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

This patch adds two compatibles to the MT7530 DSA binding.
econet,en751221-switch is the on-die switch of the EcoNet EN751221 SoC.
econet,en751221-gswitch is the multi-chip module MT7530 that some packages
connect to port 5 of the on-die switch over TRGMII. The gswitch gets a new
mt7530-passthrough-dsa-port definition that limits its link port to reg 6
and trgmii.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e29fb86934981..b73732d28d8cb 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 and allows for 5 gigabit ports.

[Severity: Low]
Is the PHY count here right? The commit message says:

  ports 0..3 are 10/100 only, port 4 has a gigabit PHY

That is four 10/100 PHYs, not three. The later driver patch in this series
("net: dsa: mediatek: support EN751221 switch") also says "The on-die
MT7530 has 4 FE ports and 1 GE port". Its en751221_mac_port_get_caps()
handles case 0 ... 3 as MAC_10 | MAC_100.

The mediatek,mcm description further down has a related wording problem.
It now reads:

  Used for MT7621AT, MT7621DAT, MT7621ST, MT7623AI and EN751221 SoCs which
  the MT7530 switch is a part of the multi-chip module.

However, this patch adds econet,en751221-switch to the builtin block, which
sets "mediatek,mcm: false". Only econet,en751221-gswitch in the EN7513G and
EN7526G packages can use the property.

Could that text name the multi-chip module packages instead of EN751221
SoCs in general?

[ ... ]

> @@ -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 gigabit MT7530 in EN751221 SoC
> +        const: econet,en751221-gswitch

[Severity: High]
Will any driver bind to econet,en751221-gswitch? Neither new compatible
has a driver match in this tree yet.

The later commit "net: dsa: mediatek: support EN751221 switch" adds
econet,en751221-switch to mt7988_of_match in mt7530-mmio.c. For the
multi-chip module, though, it adds this entry to mt7530_of_match in
drivers/net/dsa/mt7530-mdio.c:

	{ .compatible = "econet,en751221", .data = &mt753x_table[ID_EN751221_EXT], },

econet,en751221 is also the SoC root compatible in
arch/mips/boot/dts/econet/en751221.dtsi.

So with a DT that follows this binding, mt7530_probe() never binds to the
gswitch node. The on-die switch's port 5 link then points at a switch that
never registers. The DSA tree never completes, and EN7513G and EN7526G
boards have no switch networking.

Using "econet,en751221" in the DT to get the driver to probe would fail
dt_binding_check instead, because that string is not in the compatible
list here.

The binding requires mediatek,mcm and reset-names = "mcm" for the gswitch.
Those properties are only read in mt7530_probe() in mt7530-mdio.c:

	priv->rstc = devm_reset_control_get(&mdiodev->dev, "mcm");

That path is never reached for the documented compatible.

Should the binding and the driver use the same string here?

[ ... ]

> @@ -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

[Severity: Low]
Is "passthrough" the right name for this definition? It is only applied to
econet,en751221-gswitch, the downstream multi-chip module. By the driver's
definition, that switch is not in passthrough mode.

In this series, mt753x_check_passthrough() in drivers/net/dsa/mt7530.c
treats a switch as passthrough only when exactly two enabled ports carry
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;

The gswitch has user ports 0..4 plus one link, so this returns 0 for it.

The switch that really is in passthrough mode is the on-die
econet,en751221-switch, with CPU port 6 on ethernet and port 5 on link.
The "add passthrough mode" patch in this series describes it as: "the
upstream switch is restricted, with no other ports".

That switch only gets builtin-dsa-port, which only checks ports that have
ethernet. Doesn't that let the on-die port 5 link port use any reg and any
phy-mode, and allow extra user ports next to the link, while still passing
dt_binding_check?

At runtime, mt753x_check_passthrough() rejects the extra-ports case with
-EINVAL. A non-trgmii port 5 is refused by phylink, because
en751221_mac_port_get_caps() only allows TRGMII on port 5.

The reg 6 / trgmii rule does match the gswitch's upstream port. Would a
different name for this definition work, plus separate rules for
econet,en751221-switch?

[ ... ]

> @@ -337,6 +366,16 @@ allOf:
>          mediatek,mcm: false
>          reset-names: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - econet,en751221-gswitch
> +    then:
> +      $ref: "#/$defs/mt7530-passthrough-dsa-port"
> +      required:
> +        - mediatek,mcm

[Severity: Low]
Does this branch constrain the gswitch ports enough?
mt7530-passthrough-dsa-port uses "if: required: [ link ]", so it only
checks link ports that already exist. It never requires a link port, and
it never matches ports that have ethernet.

The commit message says the gswitch is "similar to the MT7621 multi-chip
module" and that its "port 6 is configured as a DSA link port". The
mediatek,mt7621 branch applies mt7530-dsa-port:

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

Nothing in this branch constrains a CPU (ethernet) port or requires the
port 6 link.

The gswitch only connects to on-die port 5 over TRGMII. Wouldn't a
gswitch node with an ethernet port of any phy-mode, or with no link port
at all, still pass dt_binding_check?

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

Re: [PATCH v3 7/7] net: dsa: mediatek: support EN751221 switch

[netdev-bot+sashiko]

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

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

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

- [High] The MCM (external) MT7530 is matched in mt7530-mdio.c with
  compatible "econet,en751221" (ID_EN751221_EXT).
- [High] net: dsa: mt7530: missing MT7530_CHG_TRAP when modifying
  MT753X_MTRAP on ID_EN751221
- [Medium] mt7530_setup_irq() still sets TOP_SIG_CTRL_NORMAL only for
  ID_MT7530/ID_MT7621.
- [Medium] en751221_trgmii_pair_setup() passes each switch's saved RX
  taps to the calibration of the other switch's receiver.
- [Medium] mt753x_phylink_mac_link_up() sets the resolved bits (including
  PMCR_FORCE_TX_FC_EN / PMCR_FORCE_RX_FC_EN when tx_pause/rx_pause are…
- [Medium] This commit makes core_write()/core_rmw() reachable on the
  MMIO on-die EN751221 switch.
- [Medium] This patch enables the topology described in commit "net: dsa:
  mediatek: support MDIO switch downstream of MMIO switch": an MDIO…
- [Low] The patch adds `TOP_SIG_CTRL_B0` (commented 'Undocumented') to
  mt7530.h, but nothing in drivers/net/dsa/ uses it.
- [Low] Concern: on the on-die EN751221 the TRGMII cascade is port 5, but
  the TRGMII PLL/clock is programmed only from port 6's mac_config…
- [Low] The binding description (from commit "dt-bindings: net: dsa:
  mediatek,mt7530: add econet,en751221" in this series) says "EcoNet…
- [Low] en751221_trgmii_pair_setup() runs from the MCM's port-6 phylink
  resolve work.
- [Low] en751221_trgmii_cal_ok() reads the RD register, writes
  val|EDGE_CHK, immediately writes val&~EDGE_CHK, and only then reads…
- [Low] Concern: in en751221_trgmii_pair_setup(), RX_RST has no explicit
  hold time (unlike TX_RST's usleep_range(5000, 6000)), so the pulse…

--

net: dsa: mediatek: support EN751221 switch

This adds support for two switches. The first is the EcoNet EN751221
on-die MT7530 (MMIO). The second is the optional MCM MT7530 (MDIO), which
sits on the on-die switch's child MDIO bus. It also adds a calibration
routine for the TRGMII link between port 5 of the on-die switch and port 6
of the MCM switch, which mt753x_phylink_mac_link_up() triggers.

> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 0be107dc6759a..03ef6a9f2084e 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: High]
Should this compatible be "econet,en751221-gswitch"?

The binding patch earlier in this series ("dt-bindings: net: dsa:
mediatek,mt7530: add econet,en751221") documents the MCM switch as:

    const: econet,en751221-gswitch

It also attaches mt7530-passthrough-dsa-port and the required
mediatek,mcm property to that string. "econet,en751221" is already the SoC
root compatible in arch/mips/boot/dts/econet/en751221.dtsi and in
Documentation/devicetree/bindings/mips/econet.yaml.

With a DT that follows the binding, the MCM switch never binds. The
on-die switch's port 5 has a link to the MCM, so the DSA tree never
completes. Neither switch is set up, and mt7530_cal_interswitch_trgmii()
never runs.

A DT that uses the driver's string instead puts the SoC top-level
compatible on a switch node, which fails dtbs_check.

The on-die side uses "econet,en751221-switch" in both mt7530-mmio.c and
the peer check in mt7530_cal_interswitch_trgmii().

[Severity: Medium]
With an MDIO MT7530 on the child MDIO bus of the MMIO on-die switch, can
lockdep report recursive locking on reg_mutex?

On the MCM, register accesses made under reg_mutex also take the on-die
switch's reg_mutex:

mt7530_port_enable()                        /* MCM */
    mutex_lock(&priv->reg_mutex);
    regmap_update_bits()
        mt7530_mdio_regmap_lock()           /* child_bus->mdio_lock */
        mt7530_regmap_write()
            bus->write() == mt753x_phy_write_c22()
                mt7531_ind_c22_phy_write()  /* on-die */
                    mutex_lock(&priv->reg_mutex);

Both reg_mutex instances come from the single mutex_init() in
mt7530_probe_common(), so they share one lockdep class. The downstream
switch has no mutex_lock_nested() or separate lock_class_key.

These are distinct instances, so this is not a real deadlock. With
CONFIG_PROVE_LOCKING, though, would the first such access during tree
setup trigger a "possible recursive locking detected" splat and turn
lockdep off for the rest of the boot?

[ ... ]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index dbe3887097dbc..c44f2084b1730 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: Medium]
This makes core_write() and core_rmw() reachable on the MMIO on-die
switch. ID_EN751221 sets .mac_port_config = mt7530_mac_config, so port 6
goes through mt7530_mac_config()->mt7530_setup_port6()->core_clear(),
core_write() and core_set().

On this switch priv->bus is NULL, so core_rmw() falls back to child_bus.
The lock helper only does something when priv->bus is set:

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

As a result, the MMD sequence on child_bus address 12 (MII_MMD_CTRL,
MII_MMD_DATA, MII_MMD_CTRL | NOINCR, MII_MMD_DATA) runs through direct
bus->write() and bus->read() calls without child_bus->mdio_lock. Each op
takes the on-die reg_mutex, but the transaction as a whole is not atomic.

Could a concurrent phylib or PHY driver access to address 12 change the
MMD devad or address between steps, so the PLL write lands in the wrong
register? The MDIO-attached variant holds bus->mdio_lock for the whole
sequence.

The child_bus fallback came from "net: dsa: mediatek: support PLL setup
on MMIO MT7530" earlier in this series. This patch is the first to make
it reachable.

> @@ -347,12 +350,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: Low]
This isn't a bug, but on the on-die EN751221 the TRGMII cascade is port
5, while the TRGMII PLL is programmed only from port 6's mac_config. Port
6 is PHY_INTERFACE_MODE_INTERNAL, and it also gets P6_INTF_MODE(1) here.

This works today for two reasons. dsa_tree_setup_ports() sets up the
on-die ports before the MCM, and phylink never does a major reconfig of
this fixed-link CPU port.

If port 6 were reconfigured later, would the TRGMII clock be gated and
reprogrammed without recalibrating the port 5 link? Would it be clearer
to tie the PLL setup to port 5?

[ ... ]

> @@ -469,6 +479,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: Low]
Does the hardware keep RD_VALUE and RD_ERR latched after EDGE_CHK is
cleared?

EDGE_CHK is set and cleared back to back, and the result is read only
after it is deasserted. en751221_trgmii_calibrate_direction() also calls
this right after changing RD_TAP, with no settle time. When the receiver
is the MMIO on-die switch, EDGE_CHK stays asserted for only a few bus
cycles.

If the results are not latched, the pass/fail map would show bus timing
rather than the data eye, and the chosen taps could be wrong.

> +
> +	return FIELD_GET(RD_VALUE_MASK, val) == TGMII_TD_PAT &&
> +	       !FIELD_GET(RD_ERR_MASK, val);
> +}

[ ... ]

> +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);

[Severity: Low]
Is anything serializing this against the on-die switch's own phylink
instance for port 5?

This runs from the MCM's port 6 phylink resolve work. It reprograms the
on-die MT753X_PMCR_P(ondie_port) and TRGMII block in several steps: down,
reset (with 5ms sleeps), up, then train.

The on-die port 5 mt753x_phylink_mac_link_up() writes the same PMCR
register from a separate resolve work item, under a different
pl->state_mutex. That work is queued earlier, but nothing guarantees it
has run before this sequence starts.

If it is delayed, its regmap_set_bits() of PMCR_MAC_TX_EN |
PMCR_FORCE_LNK could land inside the TX/RX reset window. On-die teardown
through mt753x_phylink_mac_link_down() is also not serialized against a
calibration in progress.

> +	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);

[Severity: Low]
This isn't a bug, but RX_RST has no explicit hold time here, unlike
TX_RST. The pulse width depends on how long the en751221_set_tx_drive()
writes in between take.

The MCM writes go over MDIO through the on-die indirect accessor, so the
pulse is long in practice. Would an explicit delay be more robust in case
this ordering changes?

> +
> +	/* 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]
Does this drop the flow control settings that phylink just programmed?

When tx_pause or rx_pause is set, mt753x_phylink_mac_link_up() sets
PMCR_FORCE_TX_FC_EN and PMCR_FORCE_RX_FC_EN with regmap_set_bits(). It
then calls mt7530_cal_interswitch_trgmii().

The full regmap_write() calls of mcr_down and mcr_up here replace PMCR on
both the MCM port and the on-die port, and mcr_up never includes the FC
bits.

Any pause configuration phylink resolved, such as a fixed-link with
pause, would be cleared on both ends. The on-die port's PMCR, which its
own phylink programmed, is overwritten too, so the hardware state no
longer matches what phylink reported.

[ ... ]

> +	/* 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 default tap arrays swapped here?

default_taps_ext[] is read from ext->regmap and default_taps_ondie[] from
ondie->regmap. But the first call passes ext as rx with
default_taps_ondie, and the second passes ondie as rx with
default_taps_ext.

The fallback in en751221_trgmii_calibrate_direction() writes default_taps
into the rx switch:

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

When a lane fails calibration, the MCM receiver would get the on-die
switch's original tap, and the reverse, instead of keeping its own.

The two sets can differ: mt7530_setup() programs RD_TAP(16) on the MCM,
but mt7988_setup() does not set RD_TAP on the on-die switch. The "old %d"
value in the success message would also come from the wrong switch.

Should these be:

	en751221_trgmii_calibrate_direction(ondie, ext, default_taps_ext);
	en751221_trgmii_calibrate_direction(ext, ondie, default_taps_ondie);

[ ... ]

> @@ -3023,6 +3286,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 isn't a bug, but the binding description added in "dt-bindings: net:
dsa: mediatek,mt7530: add econet,en751221" says:

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

This function, this commit message ("4 FE ports and 1 GE port") and the
binding commit message ("ports 0..3 are 10/100 only") all describe four FE
ports. Should the binding text say 4x 10/100 PHYs?

[ ... ]

> @@ -3428,7 +3732,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;

[Severity: High]
Does the on-die switch ever get MT7530_CHG_TRAP set?

On MT7530, values written to MT753X_MTRAP only override the hardware
straps in MT753X_TRAP while MT7530_CHG_TRAP is set. The driver relies
on mt7530_setup() setting it once during setup. After that,
mt7530_port_enable(), mt7530_port_disable() and mt7530_setup_port5()
update MTRAP without setting it again.

ID_EN751221 uses mt7988_setup() as its sw_setup. That resets the switch
through priv->rstc and then calls mt7531_setup_common(). Neither of them
touches MT753X_MTRAP.

This patch still sends ID_EN751221 down the MTRAP paths:

  - It adds ID_EN751221 to the MT7530_P5_DIS and MT7530_P6_DIS handling
    in mt7530_port_enable() and mt7530_port_disable().
  - Through .mac_port_config = mt7530_mac_config, port 5 reaches
    mt7530_setup_port5(), which writes MT7530_P5_MAC_SEL into MTRAP.

Without MT7530_CHG_TRAP, would those updates be silently ignored? Ports
5 and 6 would then stay at whatever the straps select. Port 5 is the
TRGMII cascade to the MCM, so if the straps disagree, the inter-switch
link would not come up. The same would apply to any other MTRAP bits the
series programs on the on-die switch, such as MT7530_LOOP_DET_DISABLE
for passthrough mode.

This may work on the tested boards because the EN751221 straps already
match, or because the on-die block does not gate MTRAP writes on
CHG_TRAP. If so, could that be stated in a comment? Otherwise, should
the EN751221 setup path set MT7530_CHG_TRAP the way mt7530_setup() does?

[Severity: Medium]
The MT7530-family ID lists here and in mt7530_port_enable() and
mt7530_port_disable() were extended. mt7530_setup_irq() still has only
the old IDs:

	/* 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 MCM MT7530 that uses mt7530_setup(), like the
MT7621 MCM. If the MCM DT node has interrupt-controller, the IRQ chip is
registered without TOP_SIG_CTRL being set.

Would switch and PHY interrupts then never fire, leaving phylib waiting
for link changes on the MCM user ports?

The unused TOP_SIG_CTRL_B0 define added for this register suggests some
TOP_SIG_CTRL handling may have been dropped.

[ ... ]

> 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

[ ... ]

> @@ -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: Low]
This isn't a bug, but nothing in drivers/net/dsa/ uses TOP_SIG_CTRL_B0.
Is it a leftover, or is some MT7530_TOP_SIG_CTRL programming for
EN751221 missing?

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