[RFC v1 0/3] Add MT7531 switch to BPI-R2Pro Board

[RFC v1 0/3] Add MT7531 switch to BPI-R2Pro Board

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

The Rockchip-Board Bananapi-R2-Pro has a Mediatek MT7531 connected to
the GMAC0.
This series changes DSA driver where needed to work on the Board and
adds necessary Devicetree node.

Frank Wunderlich (3):
  net: dsa: mt753x: make reset optional
  net: dsa: mt753x: make CPU-Port dynamic
  arm64: dts: rockchip: Add mt7531 dsa node to BPI-R2-Pro board

 .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts   | 49 +++++++++++++++++
 drivers/net/dsa/mt7530.c                      | 53 ++++++++++---------
 drivers/net/dsa/mt7530.h                      |  2 +-
 3 files changed, 78 insertions(+), 26 deletions(-)

-- 
2.25.1

[RFC v1 1/3] net: dsa: mt753x: make reset optional

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Currently a reset line is required, but on BPI-R2-Pro board
this reset is shared with the gmac and prevents the switch to
be initialized because mdio is not ready fast enough after
the reset.

So make the reset optional to allow shared reset lines.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/net/dsa/mt7530.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 19f0035d4410..ccf4cb944167 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2134,7 +2134,7 @@ mt7530_setup(struct dsa_switch *ds)
 		reset_control_assert(priv->rstc);
 		usleep_range(1000, 1100);
 		reset_control_deassert(priv->rstc);
-	} else {
+	} else if (priv->reset) {
 		gpiod_set_value_cansleep(priv->reset, 0);
 		usleep_range(1000, 1100);
 		gpiod_set_value_cansleep(priv->reset, 1);
@@ -2276,7 +2276,7 @@ mt7531_setup(struct dsa_switch *ds)
 		reset_control_assert(priv->rstc);
 		usleep_range(1000, 1100);
 		reset_control_deassert(priv->rstc);
-	} else {
+	} else if (priv->reset) {
 		gpiod_set_value_cansleep(priv->reset, 0);
 		usleep_range(1000, 1100);
 		gpiod_set_value_cansleep(priv->reset, 1);
@@ -3272,8 +3272,7 @@ mt7530_probe(struct mdio_device *mdiodev)
 		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
 						      GPIOD_OUT_LOW);
 		if (IS_ERR(priv->reset)) {
-			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
-			return PTR_ERR(priv->reset);
+			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");
 		}
 	}
 
-- 
2.25.1

Re: [RFC v1 1/3] net: dsa: mt753x: make reset optional

[Florian Fainelli]

On 4/26/22 06:49, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently a reset line is required, but on BPI-R2-Pro board
> this reset is shared with the gmac and prevents the switch to
> be initialized because mdio is not ready fast enough after
> the reset.
> 
> So make the reset optional to allow shared reset lines.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>

This looks fine however 
Documentation/devicetree/bindings/net/dsa/mt7530.txt still has some 
verbiage that suggests that the 'reset' property is mandatory, so you 
might need to update the binding (and as a separate patch we should make 
it YAML).
-- 
Florian

Re: [RFC v1 1/3] net: dsa: mt753x: make reset optional

[Vladimir Oltean]

On Tue, Apr 26, 2022 at 03:49:22PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently a reset line is required, but on BPI-R2-Pro board
> this reset is shared with the gmac and prevents the switch to
> be initialized because mdio is not ready fast enough after
> the reset.
> 
> So make the reset optional to allow shared reset lines.

What does it mean "to allow shared reset lines"? Allow as in "allow them
to sit there, unused"?

> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/net/dsa/mt7530.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 19f0035d4410..ccf4cb944167 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2134,7 +2134,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		reset_control_assert(priv->rstc);
>  		usleep_range(1000, 1100);
>  		reset_control_deassert(priv->rstc);
> -	} else {
> +	} else if (priv->reset) {

I don't really understand this patch. gpiod_set_value_cansleep() can
tolerate NULL GPIO descriptors.

>  		gpiod_set_value_cansleep(priv->reset, 0);
>  		usleep_range(1000, 1100);
>  		gpiod_set_value_cansleep(priv->reset, 1);
> @@ -2276,7 +2276,7 @@ mt7531_setup(struct dsa_switch *ds)
>  		reset_control_assert(priv->rstc);
>  		usleep_range(1000, 1100);
>  		reset_control_deassert(priv->rstc);
> -	} else {
> +	} else if (priv->reset) {
>  		gpiod_set_value_cansleep(priv->reset, 0);
>  		usleep_range(1000, 1100);
>  		gpiod_set_value_cansleep(priv->reset, 1);
> @@ -3272,8 +3272,7 @@ mt7530_probe(struct mdio_device *mdiodev)
>  		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
>  						      GPIOD_OUT_LOW);
>  		if (IS_ERR(priv->reset)) {
> -			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
> -			return PTR_ERR(priv->reset);
> +			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");

I certainly don't understand why you're suppressing the pointer-encoded
errors here. The function used is devm_gpiod_get_optional(), which
returns NULL for a missing reset-gpios, not IS_ERR(something). The
IS_ERR(something) is actually important to not ignore, maybe it's
IS_ERR(-EPROBE_DEFER). And this change breaks waiting for the descriptor
to become available.

>  		}
>  	}
>  
> -- 
> 2.25.1
> 

So what doesn't work without this patch, exactly?

Aw: Re: [RFC v1 1/3] net: dsa: mt753x: make reset optional

[Frank Wunderlich]

Hi,

you're right, reset is already optional...

idk why it was failing before the patch...i guess i had always defined the reset in dts on switch-side and dropped it same time with this patch.

Reset on both nodes (gmac+switch) blocks in switch driver because of exclusive (error message "could not get our reset line") and after dropping the reset on gmac-side the mdio-bus does not come up after switch driver resets gmac+switch (in loop with edefer).

> Gesendet: Mittwoch, 27. April 2022 um 01:57 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> On Tue, Apr 26, 2022 at 03:49:22PM +0200, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>
> >
> > Currently a reset line is required, but on BPI-R2-Pro board
> > this reset is shared with the gmac and prevents the switch to
> > be initialized because mdio is not ready fast enough after
> > the reset.
> >
> > So make the reset optional to allow shared reset lines.
>
> What does it mean "to allow shared reset lines"? Allow as in "allow them
> to sit there, unused"?

for switch part unused right, but reset-line is used by gmac. If switch does the reset, it resets the gmac too and so the mdio goes down. It took longer to get up than the wait-poll after the reset and so i got mdio read errors.

> > -	} else {
> > +	} else if (priv->reset) {
>
> I don't really understand this patch. gpiod_set_value_cansleep() can
> tolerate NULL GPIO descriptors.

had not looked for NULL-tolerance, so i precautionary added the check.

> >  		gpiod_set_value_cansleep(priv->reset, 0);
> >  		usleep_range(1000, 1100);
> >  		gpiod_set_value_cansleep(priv->reset, 1);

> > @@ -3272,8 +3272,7 @@ mt7530_probe(struct mdio_device *mdiodev)
> >  		priv->reset = devm_gpiod_get_optional(&mdiodev->dev, "reset",
> >  						      GPIOD_OUT_LOW);
> >  		if (IS_ERR(priv->reset)) {
> > -			dev_err(&mdiodev->dev, "Couldn't get our reset line\n");
> > -			return PTR_ERR(priv->reset);
> > +			dev_warn(&mdiodev->dev, "Couldn't get our reset line\n");
>
> I certainly don't understand why you're suppressing the pointer-encoded
> errors here. The function used is devm_gpiod_get_optional(), which
> returns NULL for a missing reset-gpios, not IS_ERR(something). The
> IS_ERR(something) is actually important to not ignore, maybe it's
> IS_ERR(-EPROBE_DEFER). And this change breaks waiting for the descriptor
> to become available.

you're right...the intention was to not leave the probe function if not reset was defined...but yes, devm_gpiod_get_optional is called so reset is already optional.

> So what doesn't work without this patch, exactly?

reverted the Patch in my repo and it is still working :)

just ignore it. something went wrong during my tests...

sorry for the inconvenience.

regards Frank

[RFC v1 2/3] net: dsa: mt753x: make CPU-Port dynamic

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Currently CPU-Port is hardcoded to Port 6.

On BPI-R2-Pro board this port is not connected and only Port 5 is
connected to gmac of SoC.

Replace this hardcoded CPU-Port with a member in mt7530_priv struct
which is set in mt753x_cpu_port_enable to the right port.

I defined a default in probe (in case no CPU-Port will be setup) and
if both cpu-port were setup port 6 will be used like the const prior
this patch.

In mt7531_setup first access is before we know which port should be used
(mt753x_cpu_port_enable) so section "BPDU to CPU port" needs to be moved
down.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/net/dsa/mt7530.c | 46 ++++++++++++++++++++++------------------
 drivers/net/dsa/mt7530.h |  2 +-
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ccf4cb944167..4789105b8137 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1004,6 +1004,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
 			return ret;
 	}
 
+	priv->cpu_port = port;
 	/* Enable Mediatek header mode on the cpu port */
 	mt7530_write(priv, MT7530_PVC_P(port),
 		     PORT_SPEC_TAG);
@@ -1041,7 +1042,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 	 * restore the port matrix if the port is the member of a certain
 	 * bridge.
 	 */
-	priv->ports[port].pm |= PCR_MATRIX(BIT(MT7530_CPU_PORT));
+	priv->ports[port].pm |= PCR_MATRIX(BIT(priv->cpu_port));
 	priv->ports[port].enable = true;
 	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
 		   priv->ports[port].pm);
@@ -1190,8 +1191,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 			struct netlink_ext_ack *extack)
 {
 	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
-	u32 port_bitmap = BIT(MT7530_CPU_PORT);
 	struct mt7530_priv *priv = ds->priv;
+	u32 port_bitmap = BIT(priv->cpu_port);
 
 	mutex_lock(&priv->reg_mutex);
 
@@ -1267,9 +1268,9 @@ mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
 	 * the CPU port get out of VLAN filtering mode.
 	 */
 	if (all_user_ports_removed) {
-		mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
+		mt7530_write(priv, MT7530_PCR_P(priv->cpu_port),
 			     PCR_MATRIX(dsa_user_ports(priv->ds)));
-		mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT), PORT_SPEC_TAG
+		mt7530_write(priv, MT7530_PVC_P(priv->cpu_port), PORT_SPEC_TAG
 			     | PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
 	}
 }
@@ -1335,8 +1336,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 	 */
 	if (priv->ports[port].enable)
 		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
-			   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
-	priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
+			   PCR_MATRIX(BIT(priv->cpu_port)));
+	priv->ports[port].pm = PCR_MATRIX(BIT(priv->cpu_port));
 
 	/* When a port is removed from the bridge, the port would be set up
 	 * back to the default as is at initial boot which is a VLAN-unaware
@@ -1503,6 +1504,7 @@ static int
 mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			   struct netlink_ext_ack *extack)
 {
+	struct mt7530_priv *priv = ds->priv;
 	if (vlan_filtering) {
 		/* The port is being kept as VLAN-unaware port when bridge is
 		 * set up with vlan_filtering not being set, Otherwise, the
@@ -1510,7 +1512,7 @@ mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 		 * for becoming a VLAN-aware port.
 		 */
 		mt7530_port_set_vlan_aware(ds, port);
-		mt7530_port_set_vlan_aware(ds, MT7530_CPU_PORT);
+		mt7530_port_set_vlan_aware(ds, priv->cpu_port);
 	} else {
 		mt7530_port_set_vlan_unaware(ds, port);
 	}
@@ -1526,7 +1528,7 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
 	u32 val;
 
 	new_members = entry->old_members | BIT(entry->port) |
-		      BIT(MT7530_CPU_PORT);
+		      BIT(priv->cpu_port);
 
 	/* Validate the entry with independent learning, create egress tag per
 	 * VLAN and joining the port as one of the port members.
@@ -1550,8 +1552,8 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
 	 * DSA tag.
 	 */
 	mt7530_rmw(priv, MT7530_VAWD2,
-		   ETAG_CTRL_P_MASK(MT7530_CPU_PORT),
-		   ETAG_CTRL_P(MT7530_CPU_PORT,
+		   ETAG_CTRL_P_MASK(priv->cpu_port),
+		   ETAG_CTRL_P(priv->cpu_port,
 			       MT7530_VLAN_EGRESS_STACK));
 }
 
@@ -1575,7 +1577,7 @@ mt7530_hw_vlan_del(struct mt7530_priv *priv,
 	 * the entry would be kept valid. Otherwise, the entry is got to be
 	 * disabled.
 	 */
-	if (new_members && new_members != BIT(MT7530_CPU_PORT)) {
+	if (new_members && new_members != BIT(priv->cpu_port)) {
 		val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) |
 		      VLAN_VALID;
 		mt7530_write(priv, MT7530_VAWD1, val);
@@ -2105,7 +2107,7 @@ mt7530_setup(struct dsa_switch *ds)
 	 * controller also is the container for two GMACs nodes representing
 	 * as two netdev instances.
 	 */
-	dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
+	dn = dsa_to_port(ds, priv->cpu_port)->master->dev.of_node->parent;
 	ds->assisted_learning_on_cpu_port = true;
 	ds->mtu_enforcement_ingress = true;
 
@@ -2337,15 +2339,6 @@ mt7531_setup(struct dsa_switch *ds)
 	mt7531_ind_c45_phy_write(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2,
 				 CORE_PLL_GROUP4, val);
 
-	/* BPDU to CPU port */
-	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
-		   BIT(MT7530_CPU_PORT));
-	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
-		   MT753X_BPDU_CPU_ONLY);
-
-	/* Enable and reset MIB counters */
-	mt7530_mib_reset(ds);
-
 	for (i = 0; i < MT7530_NUM_PORTS; i++) {
 		/* Disable forwarding by default on all ports */
 		mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
@@ -2373,6 +2366,15 @@ mt7531_setup(struct dsa_switch *ds)
 			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
 	}
 
+	/* BPDU to CPU port */
+	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
+		   BIT(priv->cpu_port));
+	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+		   MT753X_BPDU_CPU_ONLY);
+
+	/* Enable and reset MIB counters */
+	mt7530_mib_reset(ds);
+
 	/* Setup VLAN ID 0 for VLAN-unaware bridges */
 	ret = mt7530_setup_vlan0(priv);
 	if (ret)
@@ -3213,6 +3215,8 @@ mt7530_probe(struct mdio_device *mdiodev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->cpu_port = 6;
+
 	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
 	if (!priv->ds)
 		return -ENOMEM;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 91508e2feef9..62df8d10f6d4 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -8,7 +8,6 @@
 
 #define MT7530_NUM_PORTS		7
 #define MT7530_NUM_PHYS			5
-#define MT7530_CPU_PORT			6
 #define MT7530_NUM_FDB_RECORDS		2048
 #define MT7530_ALL_MEMBERS		0xff
 
@@ -823,6 +822,7 @@ struct mt7530_priv {
 	u8			mirror_tx;
 
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
+	int			cpu_port;
 	/* protect among processes for registers access*/
 	struct mutex reg_mutex;
 	int irq;
-- 
2.25.1

Re: [RFC v1 2/3] net: dsa: mt753x: make CPU-Port dynamic

[Florian Fainelli]

On 4/26/22 06:49, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently CPU-Port is hardcoded to Port 6.
> 
> On BPI-R2-Pro board this port is not connected and only Port 5 is
> connected to gmac of SoC.
> 
> Replace this hardcoded CPU-Port with a member in mt7530_priv struct
> which is set in mt753x_cpu_port_enable to the right port.
> 
> I defined a default in probe (in case no CPU-Port will be setup) and
> if both cpu-port were setup port 6 will be used like the const prior
> this patch.
> 
> In mt7531_setup first access is before we know which port should be used
> (mt753x_cpu_port_enable) so section "BPDU to CPU port" needs to be moved
> down.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>   drivers/net/dsa/mt7530.c | 46 ++++++++++++++++++++++------------------
>   drivers/net/dsa/mt7530.h |  2 +-
>   2 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ccf4cb944167..4789105b8137 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1004,6 +1004,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>   			return ret;
>   	}
>   
> +	priv->cpu_port = port;
>   	/* Enable Mediatek header mode on the cpu port */
>   	mt7530_write(priv, MT7530_PVC_P(port),
>   		     PORT_SPEC_TAG);
> @@ -1041,7 +1042,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>   	 * restore the port matrix if the port is the member of a certain
>   	 * bridge.
>   	 */
> -	priv->ports[port].pm |= PCR_MATRIX(BIT(MT7530_CPU_PORT));
> +	priv->ports[port].pm |= PCR_MATRIX(BIT(priv->cpu_port));
>   	priv->ports[port].enable = true;
>   	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
>   		   priv->ports[port].pm);
> @@ -1190,8 +1191,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
>   			struct netlink_ext_ack *extack)
>   {
>   	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
> -	u32 port_bitmap = BIT(MT7530_CPU_PORT);
>   	struct mt7530_priv *priv = ds->priv;
> +	u32 port_bitmap = BIT(priv->cpu_port);

No need to re-order these two lines.

>   
>   	mutex_lock(&priv->reg_mutex);
>   
> @@ -1267,9 +1268,9 @@ mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
>   	 * the CPU port get out of VLAN filtering mode.
>   	 */
>   	if (all_user_ports_removed) {
> -		mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
> +		mt7530_write(priv, MT7530_PCR_P(priv->cpu_port),
>   			     PCR_MATRIX(dsa_user_ports(priv->ds)));
> -		mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT), PORT_SPEC_TAG
> +		mt7530_write(priv, MT7530_PVC_P(priv->cpu_port), PORT_SPEC_TAG
>   			     | PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>   	}
>   }
> @@ -1335,8 +1336,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
>   	 */
>   	if (priv->ports[port].enable)
>   		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
> -			   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
> -	priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
> +			   PCR_MATRIX(BIT(priv->cpu_port)));
> +	priv->ports[port].pm = PCR_MATRIX(BIT(priv->cpu_port));
>   
>   	/* When a port is removed from the bridge, the port would be set up
>   	 * back to the default as is at initial boot which is a VLAN-unaware
> @@ -1503,6 +1504,7 @@ static int
>   mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>   			   struct netlink_ext_ack *extack)
>   {
> +	struct mt7530_priv *priv = ds->priv;

Add a space to separate declaration from code.

>   	if (vlan_filtering) {
>   		/* The port is being kept as VLAN-unaware port when bridge is
>   		 * set up with vlan_filtering not being set, Otherwise, the
> @@ -1510,7 +1512,7 @@ mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>   		 * for becoming a VLAN-aware port.
>   		 */
>   		mt7530_port_set_vlan_aware(ds, port);
> -		mt7530_port_set_vlan_aware(ds, MT7530_CPU_PORT);
> +		mt7530_port_set_vlan_aware(ds, priv->cpu_port);
>   	} else {
>   		mt7530_port_set_vlan_unaware(ds, port);
>   	}
> @@ -1526,7 +1528,7 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
>   	u32 val;
>   
>   	new_members = entry->old_members | BIT(entry->port) |
> -		      BIT(MT7530_CPU_PORT);
> +		      BIT(priv->cpu_port);
>   
>   	/* Validate the entry with independent learning, create egress tag per
>   	 * VLAN and joining the port as one of the port members.
> @@ -1550,8 +1552,8 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
>   	 * DSA tag.
>   	 */
>   	mt7530_rmw(priv, MT7530_VAWD2,
> -		   ETAG_CTRL_P_MASK(MT7530_CPU_PORT),
> -		   ETAG_CTRL_P(MT7530_CPU_PORT,
> +		   ETAG_CTRL_P_MASK(priv->cpu_port),
> +		   ETAG_CTRL_P(priv->cpu_port,
>   			       MT7530_VLAN_EGRESS_STACK));
>   }
>   
> @@ -1575,7 +1577,7 @@ mt7530_hw_vlan_del(struct mt7530_priv *priv,
>   	 * the entry would be kept valid. Otherwise, the entry is got to be
>   	 * disabled.
>   	 */
> -	if (new_members && new_members != BIT(MT7530_CPU_PORT)) {
> +	if (new_members && new_members != BIT(priv->cpu_port)) {
>   		val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) |
>   		      VLAN_VALID;
>   		mt7530_write(priv, MT7530_VAWD1, val);
> @@ -2105,7 +2107,7 @@ mt7530_setup(struct dsa_switch *ds)
>   	 * controller also is the container for two GMACs nodes representing
>   	 * as two netdev instances.
>   	 */
> -	dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
> +	dn = dsa_to_port(ds, priv->cpu_port)->master->dev.of_node->parent;
>   	ds->assisted_learning_on_cpu_port = true;
>   	ds->mtu_enforcement_ingress = true;
>   
> @@ -2337,15 +2339,6 @@ mt7531_setup(struct dsa_switch *ds)
>   	mt7531_ind_c45_phy_write(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2,
>   				 CORE_PLL_GROUP4, val);
>   
> -	/* BPDU to CPU port */
> -	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> -		   BIT(MT7530_CPU_PORT));
> -	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> -
> -	/* Enable and reset MIB counters */
> -	mt7530_mib_reset(ds);
> -
>   	for (i = 0; i < MT7530_NUM_PORTS; i++) {
>   		/* Disable forwarding by default on all ports */
>   		mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> @@ -2373,6 +2366,15 @@ mt7531_setup(struct dsa_switch *ds)
>   			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>   	}
>   
> +	/* BPDU to CPU port */
> +	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> +		   BIT(priv->cpu_port));
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +
> +	/* Enable and reset MIB counters */
> +	mt7530_mib_reset(ds);
> +
>   	/* Setup VLAN ID 0 for VLAN-unaware bridges */
>   	ret = mt7530_setup_vlan0(priv);
>   	if (ret)
> @@ -3213,6 +3215,8 @@ mt7530_probe(struct mdio_device *mdiodev)
>   	if (!priv)
>   		return -ENOMEM;
>   
> +	priv->cpu_port = 6;
> +
>   	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
>   	if (!priv->ds)
>   		return -ENOMEM;
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 91508e2feef9..62df8d10f6d4 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -8,7 +8,6 @@
>   
>   #define MT7530_NUM_PORTS		7
>   #define MT7530_NUM_PHYS			5
> -#define MT7530_CPU_PORT			6

We could have kept this define and rename it MT7530_DEFAULT_CPU_PORT or 
something and in m7530_probe() use that newly renamed constant to 
illustrate that we have a default value assigned, just in case.

>   #define MT7530_NUM_FDB_RECORDS		2048
>   #define MT7530_ALL_MEMBERS		0xff
>   
> @@ -823,6 +822,7 @@ struct mt7530_priv {
>   	u8			mirror_tx;
>   
>   	struct mt7530_port	ports[MT7530_NUM_PORTS];
> +	int			cpu_port;

This can be an unsigned integer since you do not assign negative values. 
With that fixes, this looks good to me.
-- 
Florian

Aw: Re: [RFC v1 2/3] net: dsa: mt753x: make CPU-Port dynamic

[Frank Wunderlich]

Hi

thanks for fast review.

> Gesendet: Dienstag, 26. April 2022 um 17:45 Uhr
> Von: "Florian Fainelli" <f.fainelli@gmail.com>
> On 4/26/22 06:49, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w@public-files.de>

> > @@ -1190,8 +1191,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
> >   			struct netlink_ext_ack *extack)
> >   {
> >   	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
> > -	u32 port_bitmap = BIT(MT7530_CPU_PORT);
> >   	struct mt7530_priv *priv = ds->priv;
> > +	u32 port_bitmap = BIT(priv->cpu_port);
>
> No need to re-order these two lines.

imho it is needed as i now access priv-struct now ;) which is not available at the "old" position

> >
> >   	mutex_lock(&priv->reg_mutex);

> > @@ -1503,6 +1504,7 @@ static int
> >   mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
> >   			   struct netlink_ext_ack *extack)
> >   {
> > +	struct mt7530_priv *priv = ds->priv;
>
> Add a space to separate declaration from code.

OK

> > --- a/drivers/net/dsa/mt7530.h
> > +++ b/drivers/net/dsa/mt7530.h
> > @@ -8,7 +8,6 @@
> >
> >   #define MT7530_NUM_PORTS		7
> >   #define MT7530_NUM_PHYS			5
> > -#define MT7530_CPU_PORT			6
>
> We could have kept this define and rename it MT7530_DEFAULT_CPU_PORT or
> something and in m7530_probe() use that newly renamed constant to
> illustrate that we have a default value assigned, just in case.

i do

> >   #define MT7530_NUM_FDB_RECORDS		2048
> >   #define MT7530_ALL_MEMBERS		0xff
> >
> > @@ -823,6 +822,7 @@ struct mt7530_priv {
> >   	u8			mirror_tx;
> >
> >   	struct mt7530_port	ports[MT7530_NUM_PORTS];
> > +	int			cpu_port;
>
> This can be an unsigned integer since you do not assign negative values.
> With that fixes, this looks good to me.

ok, i change to unsigned int

regards Frank

Re: Aw: Re: [RFC v1 2/3] net: dsa: mt753x: make CPU-Port dynamic

[Florian Fainelli]

On 4/26/22 08:54, Frank Wunderlich wrote:
> Hi
> 
> thanks for fast review.
> 
>> Gesendet: Dienstag, 26. April 2022 um 17:45 Uhr
>> Von: "Florian Fainelli" <f.fainelli@gmail.com>
>> On 4/26/22 06:49, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@public-files.de>
> 
>>> @@ -1190,8 +1191,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
>>>    			struct netlink_ext_ack *extack)
>>>    {
>>>    	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
>>> -	u32 port_bitmap = BIT(MT7530_CPU_PORT);
>>>    	struct mt7530_priv *priv = ds->priv;
>>> +	u32 port_bitmap = BIT(priv->cpu_port);
>>
>> No need to re-order these two lines.
> 
> imho it is needed as i now access priv-struct now ;) which is not available at the "old" position

My bad, yes, I was concerned with preserving the reserve christmas tree 
style, never mind.
-- 
Florian

Re: [RFC v1 2/3] net: dsa: mt753x: make CPU-Port dynamic

[Vladimir Oltean]

On Tue, Apr 26, 2022 at 03:49:23PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Currently CPU-Port is hardcoded to Port 6.
> 
> On BPI-R2-Pro board this port is not connected and only Port 5 is
> connected to gmac of SoC.
> 
> Replace this hardcoded CPU-Port with a member in mt7530_priv struct
> which is set in mt753x_cpu_port_enable to the right port.
> 
> I defined a default in probe (in case no CPU-Port will be setup) and
> if both cpu-port were setup port 6 will be used like the const prior
> this patch.
> 
> In mt7531_setup first access is before we know which port should be used
> (mt753x_cpu_port_enable) so section "BPDU to CPU port" needs to be moved
> down.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  drivers/net/dsa/mt7530.c | 46 ++++++++++++++++++++++------------------
>  drivers/net/dsa/mt7530.h |  2 +-
>  2 files changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ccf4cb944167..4789105b8137 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1004,6 +1004,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  			return ret;
>  	}
>  
> +	priv->cpu_port = port;
>  	/* Enable Mediatek header mode on the cpu port */
>  	mt7530_write(priv, MT7530_PVC_P(port),
>  		     PORT_SPEC_TAG);
> @@ -1041,7 +1042,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  	 * restore the port matrix if the port is the member of a certain
>  	 * bridge.
>  	 */
> -	priv->ports[port].pm |= PCR_MATRIX(BIT(MT7530_CPU_PORT));
> +	priv->ports[port].pm |= PCR_MATRIX(BIT(priv->cpu_port));

This is called with an "int port" argument, so could you please do:

	struct dsa_port *dp = dsa_to_port(ds, port);

	if (dsa_port_is_user(dp)) {
		struct dsa_port *cpu_dp = dp->cpu_dp;

		priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
	}

>  	priv->ports[port].enable = true;
>  	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
>  		   priv->ports[port].pm);
> @@ -1190,8 +1191,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
>  			struct netlink_ext_ack *extack)
>  {
>  	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
> -	u32 port_bitmap = BIT(MT7530_CPU_PORT);
>  	struct mt7530_priv *priv = ds->priv;
> +	u32 port_bitmap = BIT(priv->cpu_port);

Here we know for certain that "port" is a user port, so could you please:

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

>  
>  	mutex_lock(&priv->reg_mutex);
>  
> @@ -1267,9 +1268,9 @@ mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
>  	 * the CPU port get out of VLAN filtering mode.
>  	 */
>  	if (all_user_ports_removed) {
> -		mt7530_write(priv, MT7530_PCR_P(MT7530_CPU_PORT),
> +		mt7530_write(priv, MT7530_PCR_P(priv->cpu_port),
>  			     PCR_MATRIX(dsa_user_ports(priv->ds)));
> -		mt7530_write(priv, MT7530_PVC_P(MT7530_CPU_PORT), PORT_SPEC_TAG
> +		mt7530_write(priv, MT7530_PVC_P(priv->cpu_port), PORT_SPEC_TAG
>  			     | PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>  	}
>  }

Same idea here, go through dp->cpu_dp.

> @@ -1335,8 +1336,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
>  	 */
>  	if (priv->ports[port].enable)
>  		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
> -			   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
> -	priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
> +			   PCR_MATRIX(BIT(priv->cpu_port)));
> +	priv->ports[port].pm = PCR_MATRIX(BIT(priv->cpu_port));

Same.

>  
>  	/* When a port is removed from the bridge, the port would be set up
>  	 * back to the default as is at initial boot which is a VLAN-unaware
> @@ -1503,6 +1504,7 @@ static int
>  mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>  			   struct netlink_ext_ack *extack)
>  {
> +	struct mt7530_priv *priv = ds->priv;
>  	if (vlan_filtering) {
>  		/* The port is being kept as VLAN-unaware port when bridge is
>  		 * set up with vlan_filtering not being set, Otherwise, the
> @@ -1510,7 +1512,7 @@ mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
>  		 * for becoming a VLAN-aware port.
>  		 */
>  		mt7530_port_set_vlan_aware(ds, port);
> -		mt7530_port_set_vlan_aware(ds, MT7530_CPU_PORT);
> +		mt7530_port_set_vlan_aware(ds, priv->cpu_port);
>  	} else {
>  		mt7530_port_set_vlan_unaware(ds, port);
>  	}

Same.

> @@ -1526,7 +1528,7 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
>  	u32 val;
>  
>  	new_members = entry->old_members | BIT(entry->port) |
> -		      BIT(MT7530_CPU_PORT);
> +		      BIT(priv->cpu_port);
>  
>  	/* Validate the entry with independent learning, create egress tag per
>  	 * VLAN and joining the port as one of the port members.
> @@ -1550,8 +1552,8 @@ mt7530_hw_vlan_add(struct mt7530_priv *priv,
>  	 * DSA tag.
>  	 */
>  	mt7530_rmw(priv, MT7530_VAWD2,
> -		   ETAG_CTRL_P_MASK(MT7530_CPU_PORT),
> -		   ETAG_CTRL_P(MT7530_CPU_PORT,
> +		   ETAG_CTRL_P_MASK(priv->cpu_port),
> +		   ETAG_CTRL_P(priv->cpu_port,
>  			       MT7530_VLAN_EGRESS_STACK));
>  }

This one is interesting. For some reason, the driver author felt the
need to explicitly add BIT(MT7530_CPU_PORT) to new_members, even though
mt7530_port_vlan_add() will be called on the CPU port too

bridge vlan add dev swp0 vid 100
bridge vlan add dev br0 vid 100 self		# here

I would first make a patch that leaves it up to DSA to call
port_vlan_add for the CPU port, rather than doing it implicitly.
There is a certain advantage to that too. You can do autonomous
forwarding in a certain VLAN, but not add br0 to that VLAN, and then,
you could avoid flooding the CPU with those packets, if you know that
software doesn't need to process them.

The modified function would look like this:

static void
mt7530_hw_vlan_add(struct mt7530_priv *priv,
		   struct mt7530_hw_vlan_entry *entry)
{
	struct dsa_port *dp = dsa_to_port(priv->ds, entry->port);
	u8 new_members;
	u32 val;

	new_members = entry->old_members | BIT(entry->port);

	/* Validate the entry with independent learning, create egress tag per
	 * VLAN and joining the port as one of the port members.
	 */
	val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) | FID(FID_BRIDGED) |
	      VLAN_VALID;
	mt7530_write(priv, MT7530_VAWD1, val);

	/* Decide whether adding tag or not for those outgoing packets from the
	 * port inside the VLAN.
	 * CPU port is always taken as a tagged port for serving more than one
	 * VLANs across and also being applied with egress type stack mode for
	 * that VLAN tags would be appended after hardware special tag used as
	 * DSA tag.
	 */
	if (dsa_port_is_cpu(dp))
		val = MT7530_VLAN_EGRESS_STACK;
	else if (entry->untagged)
		val = MT7530_VLAN_EGRESS_UNTAG;
	else
		val = MT7530_VLAN_EGRESS_TAG;
	mt7530_rmw(priv, MT7530_VAWD2,
		   ETAG_CTRL_P_MASK(entry->port),
		   ETAG_CTRL_P(entry->port, val));
}

Then please confirm that there aren't regressions in local traffic
termination for a VLAN-aware bridge. Pinging over a bridge with
vlan_filtering=1 should be sufficient, since that is done in the default
pvid of 1.

With this change, you no longer need to concern yourself about the fixed
value of MT7530_CPU_PORT.

>  
> @@ -1575,7 +1577,7 @@ mt7530_hw_vlan_del(struct mt7530_priv *priv,
>  	 * the entry would be kept valid. Otherwise, the entry is got to be
>  	 * disabled.
>  	 */
> -	if (new_members && new_members != BIT(MT7530_CPU_PORT)) {
> +	if (new_members && new_members != BIT(priv->cpu_port)) {
>  		val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) |
>  		      VLAN_VALID;
>  		mt7530_write(priv, MT7530_VAWD1, val);

Here the logic is again too convoluted for its own good. Let's think
before changing.

What I think was happening was this. DSA used to be super blind when
adding a VLAN to the CPU port. Since there is no netdev for the CPU
port, then what DSA used to do was to implicitly program a bridge VLAN to
the CPU port whenever that VLAN was programmed on a user port. The
downside is that you never know when you can remove that VLAN, since the
CPU port is shared between multiple user ports and refcounting was
impossible due to technical switchdev API details.

Here, the mt7530 driver tries to outsmart DSA by saying "hey, if the CPU
port is the only remaining member of the VLAN, this VLAN is garbage,
let's remove it". Which, I mean, fair point.

But since commit 134ef2388e7f ("net: dsa: add explicit support for host
bridge VLANs"), DSA does a better job of keeping track of VLANs added on
CPU ports, and it doesn't leave dangling VLANs behind. You need to trust it.

So in this case, you would have to change the code like this:

	if (new_members) {
		val = IVL_MAC | VTAG_EN | PORT_MEM(new_members) |
		      VLAN_VALID;
		mt7530_write(priv, MT7530_VAWD1, val);
	} else {
		mt7530_write(priv, MT7530_VAWD1, 0);
		mt7530_write(priv, MT7530_VAWD2, 0);
	}

and then delete the comment, it isn't relevant any longer.

> @@ -2105,7 +2107,7 @@ mt7530_setup(struct dsa_switch *ds)
>  	 * controller also is the container for two GMACs nodes representing
>  	 * as two netdev instances.
>  	 */
> -	dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
> +	dn = dsa_to_port(ds, priv->cpu_port)->master->dev.of_node->parent;

Holy ####, this is convoluted. This gets us the OF node of the container
of the DSA master, alright.

Could you do something more generic? Something like this:

	struct device_node *dn = NULL;
	struct dsa_port *cpu_dp;

	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
		dn = cpu_dp->master->dev.of_node->parent;
		/* It doesn't matter which CPU port is found first,
		 * their masters should share the same parent OF node
		 */
		break;
	}

	if (!dn) {
		ERROR ERROR ERROR, no CPU port
	}

>  	ds->assisted_learning_on_cpu_port = true;
>  	ds->mtu_enforcement_ingress = true;
>  
> @@ -2337,15 +2339,6 @@ mt7531_setup(struct dsa_switch *ds)
>  	mt7531_ind_c45_phy_write(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2,
>  				 CORE_PLL_GROUP4, val);
>  
> -	/* BPDU to CPU port */
> -	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> -		   BIT(MT7530_CPU_PORT));
> -	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> -
> -	/* Enable and reset MIB counters */
> -	mt7530_mib_reset(ds);
> -
>  	for (i = 0; i < MT7530_NUM_PORTS; i++) {
>  		/* Disable forwarding by default on all ports */
>  		mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> @@ -2373,6 +2366,15 @@ mt7531_setup(struct dsa_switch *ds)
>  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>  	}
>  
> +	/* BPDU to CPU port */
> +	mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> +		   BIT(priv->cpu_port));
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +
> +	/* Enable and reset MIB counters */
> +	mt7530_mib_reset(ds);
> +
>  	/* Setup VLAN ID 0 for VLAN-unaware bridges */
>  	ret = mt7530_setup_vlan0(priv);
>  	if (ret)
> @@ -3213,6 +3215,8 @@ mt7530_probe(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->cpu_port = 6;
> +
>  	priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
>  	if (!priv->ds)
>  		return -ENOMEM;
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 91508e2feef9..62df8d10f6d4 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -8,7 +8,6 @@
>  
>  #define MT7530_NUM_PORTS		7
>  #define MT7530_NUM_PHYS			5
> -#define MT7530_CPU_PORT			6
>  #define MT7530_NUM_FDB_RECORDS		2048
>  #define MT7530_ALL_MEMBERS		0xff
>  
> @@ -823,6 +822,7 @@ struct mt7530_priv {
>  	u8			mirror_tx;
>  
>  	struct mt7530_port	ports[MT7530_NUM_PORTS];
> +	int			cpu_port;

And this should no longer be needed.

>  	/* protect among processes for registers access*/
>  	struct mutex reg_mutex;
>  	int irq;
> -- 
> 2.25.1
> 

[RFC v1 3/3] arm64: dts: rockchip: Add mt7531 dsa node to BPI-R2-Pro board

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Add Device Tree node for mt7531 switch connected to gmac0.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts   | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
index e091f0407460..ea5b01a90ee0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
@@ -437,6 +437,55 @@ &i2c5 {
 	status = "disabled";
 };
 
+&mdio0 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	switch@0 {
+		compatible = "mediatek,mt7531";
+		reg = <0>;
+		status = "disabled";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@1 {
+				reg = <1>;
+				label = "lan0";
+			};
+
+			port@2 {
+				reg = <2>;
+				label = "lan1";
+			};
+
+			port@3 {
+				reg = <3>;
+				label = "lan2";
+			};
+
+			port@4 {
+				reg = <4>;
+				label = "lan3";
+			};
+
+			port@5 {
+				reg = <5>;
+				label = "cpu";
+				ethernet = <&gmac0>;
+				phy-mode = "rgmii";
+
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+					pause;
+				};
+			};
+		};
+	};
+};
+
 &mdio1 {
 	rgmii_phy1: ethernet-phy@0 {
 		compatible = "ethernet-phy-ieee802.3-c22";
-- 
2.25.1

Aw: [RFC v1 3/3] arm64: dts: rockchip: Add mt7531 dsa node to BPI-R2-Pro board

[Frank Wunderlich]

> Gesendet: Dienstag, 26. April 2022 um 15:49 Uhr
> Von: "Frank Wunderlich" <linux@fw-web.de>

> +&mdio0 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	switch@0 {
> +		compatible = "mediatek,mt7531";
> +		reg = <0>;
> +		status = "disabled";

seems i had missed to delete this, but it looks like it was ignored as switch was probed

> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;