[PATCH net-next 00/13] net: dsa: lantiq_gswip: code improvements

[PATCH net-next 00/13] net: dsa: lantiq_gswip: code improvements

[Martin Schiller]

This patchset for the lantiq_gswip driver is a collection of minor fixes
and coding improvements by Martin Blumenstingl without any real changes
in the actual functionality.

Martin Blumenstingl (12):
  dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and
    fixed-link
  net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU
    port
  net: dsa: lantiq_gswip: Use dev_err_probe where appropriate
  net: dsa: lantiq_gswip: Don't manually call gswip_port_enable()
  net: dsa: lantiq_gswip: Use dsa_is_cpu_port() in
    gswip_port_change_mtu()
  net: dsa: lantiq_gswip: Change literal 6 to ETH_ALEN
  net: dsa: lantiq_gswip: Consistently use macros for the mac bridge
    table
  net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU
    port
  net: dsa: lantiq_gswip: Fix error message in
    gswip_add_single_port_br()
  net: dsa: lantiq_gswip: Fix comments in gswip_port_vlan_filtering()
  net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro
  net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()

Martin Schiller (1):
  net: dsa: lantiq_gswip: do also enable or disable cpu port

 .../bindings/net/dsa/lantiq-gswip.txt         |   6 +
 drivers/net/dsa/lantiq_gswip.c                | 110 +++++++++---------
 2 files changed, 58 insertions(+), 58 deletions(-)

-- 
2.39.2

[PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The CPU port has to specify a phy-mode and either a phy or a fixed-link.
Since GSWIP is connected using a SoC internal protocol there's no PHY
involved. Add phy-mode = "internal" and a fixed-link to describe the
communication between the PMAC (Ethernet controller) and GSWIP switch.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt b/Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt
index 8bb1eff21cb1..e81ba0e0da0f 100644
--- a/Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt
+++ b/Documentation/devicetree/bindings/net/dsa/lantiq-gswip.txt
@@ -96,7 +96,13 @@ switch@e108000 {
 
 		port@6 {
 			reg = <0x6>;
+			phy-mode = "internal";
 			ethernet = <&eth0>;
+
+			fixed-link {
+				speed = <1000>;
+				full-duplex;
+			};
 		};
 	};
 
-- 
2.39.2

[PATCH net-next 02/13] net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU port

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Add the CPU port to gswip_xrx200_phylink_get_caps() and
gswip_xrx300_phylink_get_caps(). It connects through a SoC-internal bus,
so the only allowed phy-mode is PHY_INTERFACE_MODE_INTERNAL.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index a557049e34f5..b9c7076ce32f 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1516,6 +1516,7 @@ static void gswip_xrx200_phylink_get_caps(struct dsa_switch *ds, int port,
 	case 2:
 	case 3:
 	case 4:
+	case 6:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
@@ -1547,6 +1548,7 @@ static void gswip_xrx300_phylink_get_caps(struct dsa_switch *ds, int port,
 	case 2:
 	case 3:
 	case 4:
+	case 6:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
-- 
2.39.2

[PATCH net-next 03/13] net: dsa: lantiq_gswip: Use dev_err_probe where appropriate

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

dev_err_probe() can be used to simplify the existing code. Also it means
we get rid of the following warning which is seen whenever the PMAC
(Ethernet controller which connects to GSWIP's CPU port) has not been
probed yet:
  gswip 1e108000.switch: dsa switch register failed: -517

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 53 ++++++++++++++++------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index b9c7076ce32f..fcb5929c9c88 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1931,11 +1931,9 @@ static int gswip_gphy_fw_load(struct gswip_priv *priv, struct gswip_gphy_fw *gph
 	msleep(200);
 
 	ret = request_firmware(&fw, gphy_fw->fw_name, dev);
-	if (ret) {
-		dev_err(dev, "failed to load firmware: %s, error: %i\n",
-			gphy_fw->fw_name, ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to load firmware: %s\n",
+				     gphy_fw->fw_name);
 
 	/* GPHY cores need the firmware code in a persistent and contiguous
 	 * memory area with a 16 kB boundary aligned start address.
@@ -1948,9 +1946,9 @@ static int gswip_gphy_fw_load(struct gswip_priv *priv, struct gswip_gphy_fw *gph
 		dev_addr = ALIGN(dma_addr, XRX200_GPHY_FW_ALIGN);
 		memcpy(fw_addr, fw->data, fw->size);
 	} else {
-		dev_err(dev, "failed to alloc firmware memory\n");
 		release_firmware(fw);
-		return -ENOMEM;
+		return dev_err_probe(dev, -ENOMEM,
+				     "failed to alloc firmware memory\n");
 	}
 
 	release_firmware(fw);
@@ -1977,8 +1975,8 @@ static int gswip_gphy_fw_probe(struct gswip_priv *priv,
 
 	gphy_fw->clk_gate = devm_clk_get(dev, gphyname);
 	if (IS_ERR(gphy_fw->clk_gate)) {
-		dev_err(dev, "Failed to lookup gate clock\n");
-		return PTR_ERR(gphy_fw->clk_gate);
+		return dev_err_probe(dev, PTR_ERR(gphy_fw->clk_gate),
+				     "Failed to lookup gate clock\n");
 	}
 
 	ret = of_property_read_u32(gphy_fw_np, "reg", &gphy_fw->fw_addr_offset);
@@ -1998,8 +1996,8 @@ static int gswip_gphy_fw_probe(struct gswip_priv *priv,
 		gphy_fw->fw_name = priv->gphy_fw_name_cfg->ge_firmware_name;
 		break;
 	default:
-		dev_err(dev, "Unknown GPHY mode %d\n", gphy_mode);
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Unknown GPHY mode %d\n",
+				     gphy_mode);
 	}
 
 	gphy_fw->reset = of_reset_control_array_get_exclusive(gphy_fw_np);
@@ -2050,8 +2048,9 @@ static int gswip_gphy_fw_list(struct gswip_priv *priv,
 			priv->gphy_fw_name_cfg = &xrx200a2x_gphy_data;
 			break;
 		default:
-			dev_err(dev, "unknown GSWIP version: 0x%x", version);
-			return -ENOENT;
+			return dev_err_probe(dev, -ENOENT,
+					     "unknown GSWIP version: 0x%x",
+					     version);
 		}
 	}
 
@@ -2059,10 +2058,9 @@ static int gswip_gphy_fw_list(struct gswip_priv *priv,
 	if (match && match->data)
 		priv->gphy_fw_name_cfg = match->data;
 
-	if (!priv->gphy_fw_name_cfg) {
-		dev_err(dev, "GPHY compatible type not supported");
-		return -ENOENT;
-	}
+	if (!priv->gphy_fw_name_cfg)
+		return dev_err_probe(dev, -ENOENT,
+				     "GPHY compatible type not supported");
 
 	priv->num_gphy_fw = of_get_available_child_count(gphy_fw_list_np);
 	if (!priv->num_gphy_fw)
@@ -2163,8 +2161,8 @@ static int gswip_probe(struct platform_device *pdev)
 			return -EINVAL;
 		break;
 	default:
-		dev_err(dev, "unknown GSWIP version: 0x%x", version);
-		return -ENOENT;
+		return dev_err_probe(dev, -ENOENT,
+				     "unknown GSWIP version: 0x%x", version);
 	}
 
 	/* bring up the mdio bus */
@@ -2172,28 +2170,27 @@ static int gswip_probe(struct platform_device *pdev)
 	if (gphy_fw_np) {
 		err = gswip_gphy_fw_list(priv, gphy_fw_np, version);
 		of_node_put(gphy_fw_np);
-		if (err) {
-			dev_err(dev, "gphy fw probe failed\n");
-			return err;
-		}
+		if (err)
+			return dev_err_probe(dev, err,
+					     "gphy fw probe failed\n");
 	}
 
 	/* bring up the mdio bus */
 	err = gswip_mdio(priv);
 	if (err) {
-		dev_err(dev, "mdio probe failed\n");
+		dev_err_probe(dev, err, "mdio probe failed\n");
 		goto gphy_fw_remove;
 	}
 
 	err = dsa_register_switch(priv->ds);
 	if (err) {
-		dev_err(dev, "dsa switch register failed: %i\n", err);
+		dev_err_probe(dev, err, "dsa switch registration failed\n");
 		goto gphy_fw_remove;
 	}
 	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
-		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
-			priv->hw_info->cpu_port);
-		err = -EINVAL;
+		err = dev_err_probe(dev, -EINVAL,
+				    "wrong CPU port defined, HW only supports port: %i",
+				    priv->hw_info->cpu_port);
 		goto disable_switch;
 	}
 
-- 
2.39.2

[PATCH net-next 04/13] net: dsa: lantiq_gswip: Don't manually call gswip_port_enable()

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

We don't need to manually call gswip_port_enable() from within
gswip_setup() for the CPU port. DSA does this automatically for us.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index fcb5929c9c88..3fd5599fca52 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -898,8 +898,6 @@ static int gswip_setup(struct dsa_switch *ds)
 
 	ds->mtu_enforcement_ingress = true;
 
-	gswip_port_enable(ds, cpu_port, NULL);
-
 	ds->configure_vlan_while_not_filtering = false;
 
 	return 0;
-- 
2.39.2

[PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or disable cpu port

[Martin Schiller]

Before commit 74be4babe72f ("net: dsa: do not enable or disable non user
ports"), gswip_port_enable/disable() were also executed for the cpu port
in gswip_setup() which disabled the cpu port during initialization.

Let's restore this by removing the dsa_is_user_port checks. Also, let's
clean up the gswip_port_enable() function so that we only have to check
for the cpu port once.

Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user ports")
Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 drivers/net/dsa/lantiq_gswip.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 3fd5599fca52..38b5f743e5ee 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
 	struct gswip_priv *priv = ds->priv;
 	int err;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	if (!dsa_is_cpu_port(ds, port)) {
+		u32 mdio_phy = 0;
+
 		err = gswip_add_single_port_br(priv, port, true);
 		if (err)
 			return err;
+
+		if (phydev)
+			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
+
+		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
+				GSWIP_MDIO_PHYp(port));
 	}
 
 	/* RMON Counter Enable for port */
@@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
 	gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
 			  GSWIP_SDMA_PCTRLp(port));
 
-	if (!dsa_is_cpu_port(ds, port)) {
-		u32 mdio_phy = 0;
-
-		if (phydev)
-			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
-
-		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
-				GSWIP_MDIO_PHYp(port));
-	}
-
 	return 0;
 }
 
@@ -731,9 +726,6 @@ static void gswip_port_disable(struct dsa_switch *ds, int port)
 {
 	struct gswip_priv *priv = ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return;
-
 	gswip_switch_mask(priv, GSWIP_FDMA_PCTRL_EN, 0,
 			  GSWIP_FDMA_PCTRLp(port));
 	gswip_switch_mask(priv, GSWIP_SDMA_PCTRL_EN, 0,
-- 
2.39.2

[PATCH net-next 06/13] net: dsa: lantiq_gswip: Use dsa_is_cpu_port() in gswip_port_change_mtu()

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Make the check for the CPU port in gswip_port_change_mtu() consistent
with other areas of the driver by using dsa_is_cpu_port().

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 38b5f743e5ee..789b8a1076f1 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1464,12 +1464,11 @@ static int gswip_port_max_mtu(struct dsa_switch *ds, int port)
 static int gswip_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct gswip_priv *priv = ds->priv;
-	int cpu_port = priv->hw_info->cpu_port;
 
 	/* CPU port always has maximum mtu of user ports, so use it to set
 	 * switch frame size, including 8 byte special header.
 	 */
-	if (port == cpu_port) {
+	if (dsa_is_cpu_port(ds, port)) {
 		new_mtu += 8;
 		gswip_switch_w(priv, VLAN_ETH_HLEN + new_mtu + ETH_FCS_LEN,
 			       GSWIP_MAC_FLEN);
-- 
2.39.2

[PATCH net-next 07/13] net: dsa: lantiq_gswip: Change literal 6 to ETH_ALEN

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The addr variable in gswip_port_fdb_dump() stores a mac address. Use
ETH_ALEN to make this consistent across other drivers.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 789b8a1076f1..c049f505fcc7 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1413,7 +1413,7 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
 {
 	struct gswip_priv *priv = ds->priv;
 	struct gswip_pce_table_entry mac_bridge = {0,};
-	unsigned char addr[6];
+	unsigned char addr[ETH_ALEN];
 	int i;
 	int err;
 
-- 
2.39.2

[PATCH net-next 08/13] net: dsa: lantiq_gswip: Consistently use macros for the mac bridge table

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Introduce a new GSWIP_TABLE_MAC_BRIDGE_PORT macro and use it throughout
the driver. Also update GSWIP_TABLE_MAC_BRIDGE_STATIC to use the BIT()
macro. This makes the driver code easier to understand.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index c049f505fcc7..ee8296d5b901 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -236,7 +236,8 @@
 #define GSWIP_TABLE_ACTIVE_VLAN		0x01
 #define GSWIP_TABLE_VLAN_MAPPING	0x02
 #define GSWIP_TABLE_MAC_BRIDGE		0x0b
-#define  GSWIP_TABLE_MAC_BRIDGE_STATIC	0x01	/* Static not, aging entry */
+#define  GSWIP_TABLE_MAC_BRIDGE_STATIC	BIT(0)		/* Static not, aging entry */
+#define  GSWIP_TABLE_MAC_BRIDGE_PORT	GENMASK(7, 4)	/* Port on learned entries */
 
 #define XRX200_GPHY_FW_ALIGN	(16 * 1024)
 
@@ -1307,7 +1308,8 @@ static void gswip_port_fast_age(struct dsa_switch *ds, int port)
 		if (mac_bridge.val[1] & GSWIP_TABLE_MAC_BRIDGE_STATIC)
 			continue;
 
-		if (((mac_bridge.val[0] & GENMASK(7, 4)) >> 4) != port)
+		if (port != FIELD_GET(GSWIP_TABLE_MAC_BRIDGE_PORT,
+				      mac_bridge.val[0]))
 			continue;
 
 		mac_bridge.valid = false;
@@ -1445,7 +1447,8 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
 					return err;
 			}
 		} else {
-			if (((mac_bridge.val[0] & GENMASK(7, 4)) >> 4) == port) {
+			if (port == FIELD_GET(GSWIP_TABLE_MAC_BRIDGE_PORT,
+					      mac_bridge.val[0])) {
 				err = cb(addr, 0, false, data);
 				if (err)
 					return err;
-- 
2.39.2

[PATCH net-next 09/13] net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU port

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Calling gswip_add_single_port_br() with the CPU port would be a bug
because then only the CPU port could talk to itself. Add the CPU port to
the validation at the beginning of gswip_add_single_port_br().

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index ee8296d5b901..d2195271ffe9 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add)
 	unsigned int max_ports = priv->hw_info->max_ports;
 	int err;
 
-	if (port >= max_ports) {
+	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
 		dev_err(priv->dev, "single port for %i supported\n", port);
 		return -EIO;
 	}
-- 
2.39.2

[PATCH net-next 10/13] net: dsa: lantiq_gswip: Fix error message in gswip_add_single_port_br()

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The error message is printed when the port cannot be used. Update the
error message to reflect that.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index d2195271ffe9..3c96a62b8e0a 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -658,7 +658,8 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add)
 	int err;
 
 	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
-		dev_err(priv->dev, "single port for %i supported\n", port);
+		dev_err(priv->dev, "single port for %i is not supported\n",
+			port);
 		return -EIO;
 	}
 
-- 
2.39.2

[PATCH net-next 11/13] net: dsa: lantiq_gswip: Fix comments in gswip_port_vlan_filtering()

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Update the comments in gswip_port_vlan_filtering() so it's clear that
there are two separate cases, one for "tag based VLAN" and another one
for "port based VLAN".

Suggested-by: Martin Schiller <ms@dev.tdt.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 3c96a62b8e0a..f2faee112e33 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -786,7 +786,7 @@ static int gswip_port_vlan_filtering(struct dsa_switch *ds, int port,
 	}
 
 	if (vlan_filtering) {
-		/* Use port based VLAN tag */
+		/* Use tag based VLAN */
 		gswip_switch_mask(priv,
 				  GSWIP_PCE_VCTRL_VSR,
 				  GSWIP_PCE_VCTRL_UVR | GSWIP_PCE_VCTRL_VIMR |
@@ -795,7 +795,7 @@ static int gswip_port_vlan_filtering(struct dsa_switch *ds, int port,
 		gswip_switch_mask(priv, GSWIP_PCE_PCTRL_0_TVM, 0,
 				  GSWIP_PCE_PCTRL_0p(port));
 	} else {
-		/* Use port based VLAN tag */
+		/* Use port based VLAN */
 		gswip_switch_mask(priv,
 				  GSWIP_PCE_VCTRL_UVR | GSWIP_PCE_VCTRL_VIMR |
 				  GSWIP_PCE_VCTRL_VEMR,
-- 
2.39.2

[PATCH net-next 12/13] net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Only bits [5:0] in mac_bridge.key[3] are reserved for the FID. Add a
macro so this becomes obvious when reading the driver code.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index f2faee112e33..4bb894e75b81 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -238,6 +238,7 @@
 #define GSWIP_TABLE_MAC_BRIDGE		0x0b
 #define  GSWIP_TABLE_MAC_BRIDGE_STATIC	BIT(0)		/* Static not, aging entry */
 #define  GSWIP_TABLE_MAC_BRIDGE_PORT	GENMASK(7, 4)	/* Port on learned entries */
+#define  GSWIP_TABLE_MAC_BRIDGE_FID	GENMASK(5, 0)	/* Filtering identifier */
 
 #define XRX200_GPHY_FW_ALIGN	(16 * 1024)
 
@@ -1385,7 +1386,7 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 	mac_bridge.key[0] = addr[5] | (addr[4] << 8);
 	mac_bridge.key[1] = addr[3] | (addr[2] << 8);
 	mac_bridge.key[2] = addr[1] | (addr[0] << 8);
-	mac_bridge.key[3] = fid;
+	mac_bridge.key[3] = FIELD_PREP(GSWIP_TABLE_MAC_BRIDGE_FID, fid);
 	mac_bridge.val[0] = add ? BIT(port) : 0; /* port map */
 	mac_bridge.val[1] = GSWIP_TABLE_MAC_BRIDGE_STATIC;
 	mac_bridge.valid = add;
-- 
2.39.2

[PATCH net-next 13/13] net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()

[Martin Schiller]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Print the port which is not found to be part of a bridge so it's easier
to investigate the underlying issue.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 4bb894e75b81..69035598e8a4 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1377,7 +1377,8 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 	}
 
 	if (fid == -1) {
-		dev_err(priv->dev, "Port not part of a bridge\n");
+		dev_err(priv->dev,
+			"Port %d is not known to be part of bridge\n", port);
 		return -EINVAL;
 	}
 
-- 
2.39.2

Re: [PATCH net-next 00/13] net: dsa: lantiq_gswip: code improvements

[Hauke Mehrtens]

On 6/6/24 10:52, Martin Schiller wrote:
> This patchset for the lantiq_gswip driver is a collection of minor fixes
> and coding improvements by Martin Blumenstingl without any real changes
> in the actual functionality.
> 
> Martin Blumenstingl (12):
>    dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and
>      fixed-link
>    net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU
>      port
>    net: dsa: lantiq_gswip: Use dev_err_probe where appropriate
>    net: dsa: lantiq_gswip: Don't manually call gswip_port_enable()
>    net: dsa: lantiq_gswip: Use dsa_is_cpu_port() in
>      gswip_port_change_mtu()
>    net: dsa: lantiq_gswip: Change literal 6 to ETH_ALEN
>    net: dsa: lantiq_gswip: Consistently use macros for the mac bridge
>      table
>    net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU
>      port
>    net: dsa: lantiq_gswip: Fix error message in
>      gswip_add_single_port_br()
>    net: dsa: lantiq_gswip: Fix comments in gswip_port_vlan_filtering()
>    net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro
>    net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()
> 
> Martin Schiller (1):
>    net: dsa: lantiq_gswip: do also enable or disable cpu port
> 
>   .../bindings/net/dsa/lantiq-gswip.txt         |   6 +
>   drivers/net/dsa/lantiq_gswip.c                | 110 +++++++++---------
>   2 files changed, 58 insertions(+), 58 deletions(-)
> 
Thanks for sending this upstream. I had this on my list for a long time 
but never started it.

For all patches:
Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

Hauke

Re: [PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:22AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> The CPU port has to specify a phy-mode and either a phy or a fixed-link.
> Since GSWIP is connected using a SoC internal protocol there's no PHY
> involved. Add phy-mode = "internal" and a fixed-link to describe the
> communication between the PMAC (Ethernet controller) and GSWIP switch.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Long-term it would be good to also see a dt-schema conversion.

Re: [PATCH net-next 02/13] net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU port

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:23AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Add the CPU port to gswip_xrx200_phylink_get_caps() and
> gswip_xrx300_phylink_get_caps(). It connects through a SoC-internal bus,
> so the only allowed phy-mode is PHY_INTERFACE_MODE_INTERNAL.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

This is for the case where those CPU port device tree properties are
present, right? In the device trees in current circulation they are not,
and DSA skips phylink registration.

Re: [PATCH net-next 03/13] net: dsa: lantiq_gswip: Use dev_err_probe where appropriate

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:24AM +0200, Martin Schiller wrote:
> @@ -2050,8 +2048,9 @@ static int gswip_gphy_fw_list(struct gswip_priv *priv,
>  			priv->gphy_fw_name_cfg = &xrx200a2x_gphy_data;
>  			break;
>  		default:
> -			dev_err(dev, "unknown GSWIP version: 0x%x", version);
> -			return -ENOENT;
> +			return dev_err_probe(dev, -ENOENT,
> +					     "unknown GSWIP version: 0x%x",
> +					     version);
>  		}
>  	}
>  
> @@ -2059,10 +2058,9 @@ static int gswip_gphy_fw_list(struct gswip_priv *priv,
>  	if (match && match->data)
>  		priv->gphy_fw_name_cfg = match->data;
>  
> -	if (!priv->gphy_fw_name_cfg) {
> -		dev_err(dev, "GPHY compatible type not supported");
> -		return -ENOENT;
> -	}
> +	if (!priv->gphy_fw_name_cfg)
> +		return dev_err_probe(dev, -ENOENT,
> +				     "GPHY compatible type not supported");
>  
>  	priv->num_gphy_fw = of_get_available_child_count(gphy_fw_list_np);
>  	if (!priv->num_gphy_fw)
> @@ -2163,8 +2161,8 @@ static int gswip_probe(struct platform_device *pdev)
>  			return -EINVAL;
>  		break;
>  	default:
> -		dev_err(dev, "unknown GSWIP version: 0x%x", version);
> -		return -ENOENT;
> +		return dev_err_probe(dev, -ENOENT,
> +				     "unknown GSWIP version: 0x%x", version);
>  	}
>  
>  	/* bring up the mdio bus */
> @@ -2172,28 +2170,27 @@ static int gswip_probe(struct platform_device *pdev)
>  	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> -		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
> -			priv->hw_info->cpu_port);
> -		err = -EINVAL;
> +		err = dev_err_probe(dev, -EINVAL,
> +				    "wrong CPU port defined, HW only supports port: %i",
> +				    priv->hw_info->cpu_port);
>  		goto disable_switch;
>  	}

Nitpick: there is no terminating \n here.

Re: [PATCH net-next 04/13] net: dsa: lantiq_gswip: Don't manually call gswip_port_enable()

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:25AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> We don't need to manually call gswip_port_enable() from within
> gswip_setup() for the CPU port. DSA does this automatically for us.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Not to mention the first thing in gswip_port_enable(), which is:

	if (!dsa_is_user_port(ds, port))
		return 0;

So the call is dead code anyway.

Re: [PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or disable cpu port

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:26AM +0200, Martin Schiller wrote:
> Before commit 74be4babe72f ("net: dsa: do not enable or disable non user
> ports"), gswip_port_enable/disable() were also executed for the cpu port
> in gswip_setup() which disabled the cpu port during initialization.

Ah, you also noticed this.

> 
> Let's restore this by removing the dsa_is_user_port checks. Also, let's
> clean up the gswip_port_enable() function so that we only have to check
> for the cpu port once.
> 
> Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user ports")

Fixes tags shouldn't be taken lightly. If you think there's a functional
user-visible problem caused by that change, you need to explain what
that problem is and what it affects. Additionally, bug fix patches are
sent out to the 'net' tree, not bundled up with 'net-next' material
(unless they fix a change that's also exclusive to net-next).
Otherwise, just drop the 'Fixes' tag.

> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 3fd5599fca52..38b5f743e5ee 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
>  	struct gswip_priv *priv = ds->priv;
>  	int err;
>  
> -	if (!dsa_is_user_port(ds, port))
> -		return 0;
> -
>  	if (!dsa_is_cpu_port(ds, port)) {
> +		u32 mdio_phy = 0;
> +
>  		err = gswip_add_single_port_br(priv, port, true);
>  		if (err)
>  			return err;
> +
> +		if (phydev)
> +			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
> +
> +		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
> +				GSWIP_MDIO_PHYp(port));
>  	}
>  
>  	/* RMON Counter Enable for port */
> @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch *ds, int port,
>  	gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
>  			  GSWIP_SDMA_PCTRLp(port));
>  
> -	if (!dsa_is_cpu_port(ds, port)) {
> -		u32 mdio_phy = 0;
> -
> -		if (phydev)
> -			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
> -
> -		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
> -				GSWIP_MDIO_PHYp(port));
> -	}
> -
>  	return 0;
>  }

It would be good to state in the commit message that the operation
reordering is safe. The commit seems to be concerned mainly with code
cleanliness, which does not always take side effects into account.

Re: [PATCH net-next 06/13] net: dsa: lantiq_gswip: Use dsa_is_cpu_port() in gswip_port_change_mtu()

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:27AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Make the check for the CPU port in gswip_port_change_mtu() consistent
> with other areas of the driver by using dsa_is_cpu_port().
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 07/13] net: dsa: lantiq_gswip: Change literal 6 to ETH_ALEN

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:28AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> The addr variable in gswip_port_fdb_dump() stores a mac address. Use
> ETH_ALEN to make this consistent across other drivers.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 08/13] net: dsa: lantiq_gswip: Consistently use macros for the mac bridge table

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:29AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Introduce a new GSWIP_TABLE_MAC_BRIDGE_PORT macro and use it throughout
> the driver. Also update GSWIP_TABLE_MAC_BRIDGE_STATIC to use the BIT()
> macro. This makes the driver code easier to understand.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 09/13] net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU port

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Calling gswip_add_single_port_br() with the CPU port would be a bug
> because then only the CPU port could talk to itself. Add the CPU port to
> the validation at the beginning of gswip_add_single_port_br().
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index ee8296d5b901..d2195271ffe9 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add)
>  	unsigned int max_ports = priv->hw_info->max_ports;
>  	int err;
>  
> -	if (port >= max_ports) {
> +	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
>  		dev_err(priv->dev, "single port for %i supported\n", port);
>  		return -EIO;
>  	}
> -- 
> 2.39.2
> 

Isn't the new check effectively dead code?

Re: [PATCH net-next 10/13] net: dsa: lantiq_gswip: Fix error message in gswip_add_single_port_br()

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:31AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> The error message is printed when the port cannot be used. Update the
> error message to reflect that.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index d2195271ffe9..3c96a62b8e0a 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -658,7 +658,8 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add)
>  	int err;
>  
>  	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
> -		dev_err(priv->dev, "single port for %i supported\n", port);
> +		dev_err(priv->dev, "single port for %i is not supported\n",
> +			port);
>  		return -EIO;
>  	}
>  
> -- 
> 2.39.2
> 

Isn't even the original condition (port >= max_ports) dead code? Why not
remove the condition altogether?

Re: [PATCH net-next 12/13] net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:33AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Only bits [5:0] in mac_bridge.key[3] are reserved for the FID. Add a
> macro so this becomes obvious when reading the driver code.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index f2faee112e33..4bb894e75b81 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -238,6 +238,7 @@
>  #define GSWIP_TABLE_MAC_BRIDGE		0x0b
>  #define  GSWIP_TABLE_MAC_BRIDGE_STATIC	BIT(0)		/* Static not, aging entry */
>  #define  GSWIP_TABLE_MAC_BRIDGE_PORT	GENMASK(7, 4)	/* Port on learned entries */
> +#define  GSWIP_TABLE_MAC_BRIDGE_FID	GENMASK(5, 0)	/* Filtering identifier */
>  
>  #define XRX200_GPHY_FW_ALIGN	(16 * 1024)
>  
> @@ -1385,7 +1386,7 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
>  	mac_bridge.key[0] = addr[5] | (addr[4] << 8);
>  	mac_bridge.key[1] = addr[3] | (addr[2] << 8);
>  	mac_bridge.key[2] = addr[1] | (addr[0] << 8);
> -	mac_bridge.key[3] = fid;
> +	mac_bridge.key[3] = FIELD_PREP(GSWIP_TABLE_MAC_BRIDGE_FID, fid);
>  	mac_bridge.val[0] = add ? BIT(port) : 0; /* port map */
>  	mac_bridge.val[1] = GSWIP_TABLE_MAC_BRIDGE_STATIC;
>  	mac_bridge.valid = add;
> -- 
> 2.39.2

On second thought, I disagree with the naming scheme of the
GSWIP_TABLE_MAC_BRIDGE_* macros. It is completely non obvious that they
are non-overlapping, because they have the same name prefix, but:
_STATIC applies to gswip_pce_table_entry :: val[1]
_PORT applies to gswip_pce_table_entry :: val[0]
_FID applies to gswip_pce_table_entry :: key[3]

I think it's all too easy to use the wrong macro on the wrong register field.
If the macros incorporated names like VAL1, KEY3 etc, it would be much
more obvious. Could you please do that?

Re: [PATCH net-next 13/13] net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:34AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Print the port which is not found to be part of a bridge so it's easier
> to investigate the underlying issue.

Was there an actual issue which was investigated here? More details?

> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 4bb894e75b81..69035598e8a4 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1377,7 +1377,8 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
>  	}
>  
>  	if (fid == -1) {
> -		dev_err(priv->dev, "Port not part of a bridge\n");
> +		dev_err(priv->dev,
> +			"Port %d is not known to be part of bridge\n", port);
>  		return -EINVAL;
>  	}

Actually I would argue this is entirely confusing. There is an earlier
check:

	if (!bridge)
		return -EINVAL;

which did _not_ trigger if we're executing this. So the port _is_ a part
of a bridge. Just say that no FID is found for bridge %s (bridge->name),
which technically _is_ what happened.

Re: [PATCH net-next 11/13] net: dsa: lantiq_gswip: Fix comments in gswip_port_vlan_filtering()

[Vladimir Oltean]

On Thu, Jun 06, 2024 at 10:52:32AM +0200, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> Update the comments in gswip_port_vlan_filtering() so it's clear that
> there are two separate cases, one for "tag based VLAN" and another one
> for "port based VLAN".
> 
> Suggested-by: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Here and in whatever the previous patch turns into: please make more
careful use of the word "fix". It carries connotations of addressing
bugs which must be backported. Various automated tools scan the git tree
for bug fixes which were apparently "not properly submitted" and mark
them for auto-selection to stable. You don't want to cause that for a
minor comment.

Re: [PATCH net-next 02/13] net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU port

[Martin Schiller]

On 2024-06-07 13:03, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:23AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> Add the CPU port to gswip_xrx200_phylink_get_caps() and
>> gswip_xrx300_phylink_get_caps(). It connects through a SoC-internal 
>> bus,
>> so the only allowed phy-mode is PHY_INTERFACE_MODE_INTERNAL.
>> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
> 
> This is for the case where those CPU port device tree properties are
> present, right? In the device trees in current circulation they are 
> not,
> and DSA skips phylink registration.

Yes, as far as I know, this driver is mainly, if not exclusively, used 
in the
openWrt environment. These functions were already added here in Oct. 
2022 [1].

[1] 
https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2683cca5927844594f7835aa983e2690d1e343c6

Re: [PATCH net-next 02/13] net: dsa: lantiq_gswip: Only allow phy-mode = "internal" on the CPU port

[Vladimir Oltean]

On Fri, Jun 07, 2024 at 02:01:57PM +0200, Martin Schiller wrote:
> On 2024-06-07 13:03, Vladimir Oltean wrote:
> > On Thu, Jun 06, 2024 at 10:52:23AM +0200, Martin Schiller wrote:
> > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > 
> > > Add the CPU port to gswip_xrx200_phylink_get_caps() and
> > > gswip_xrx300_phylink_get_caps(). It connects through a SoC-internal
> > > bus,
> > > so the only allowed phy-mode is PHY_INTERFACE_MODE_INTERNAL.
> > > 
> > > Signed-off-by: Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com>
> > > ---
> > 
> > This is for the case where those CPU port device tree properties are
> > present, right? In the device trees in current circulation they are not,
> > and DSA skips phylink registration.
> 
> Yes, as far as I know, this driver is mainly, if not exclusively, used in
> the
> openWrt environment. These functions were already added here in Oct. 2022
> [1].
> 
> [1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2683cca5927844594f7835aa983e2690d1e343c6

Ok. You can add my

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next 03/13] net: dsa: lantiq_gswip: Use dev_err_probe where appropriate

[Martin Schiller]

On 2024-06-07 13:07, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:24AM +0200, Martin Schiller wrote:
>> @@ -2050,8 +2048,9 @@ static int gswip_gphy_fw_list(struct gswip_priv 
>> *priv,
>>  			priv->gphy_fw_name_cfg = &xrx200a2x_gphy_data;
>>  			break;
>>  		default:
>> -			dev_err(dev, "unknown GSWIP version: 0x%x", version);
>> -			return -ENOENT;
>> +			return dev_err_probe(dev, -ENOENT,
>> +					     "unknown GSWIP version: 0x%x",
>> +					     version);
>>  		}
>>  	}
>> 
>> @@ -2059,10 +2058,9 @@ static int gswip_gphy_fw_list(struct gswip_priv 
>> *priv,
>>  	if (match && match->data)
>>  		priv->gphy_fw_name_cfg = match->data;
>> 
>> -	if (!priv->gphy_fw_name_cfg) {
>> -		dev_err(dev, "GPHY compatible type not supported");
>> -		return -ENOENT;
>> -	}
>> +	if (!priv->gphy_fw_name_cfg)
>> +		return dev_err_probe(dev, -ENOENT,
>> +				     "GPHY compatible type not supported");
>> 
>>  	priv->num_gphy_fw = of_get_available_child_count(gphy_fw_list_np);
>>  	if (!priv->num_gphy_fw)
>> @@ -2163,8 +2161,8 @@ static int gswip_probe(struct platform_device 
>> *pdev)
>>  			return -EINVAL;
>>  		break;
>>  	default:
>> -		dev_err(dev, "unknown GSWIP version: 0x%x", version);
>> -		return -ENOENT;
>> +		return dev_err_probe(dev, -ENOENT,
>> +				     "unknown GSWIP version: 0x%x", version);
>>  	}
>> 
>>  	/* bring up the mdio bus */
>> @@ -2172,28 +2170,27 @@ static int gswip_probe(struct platform_device 
>> *pdev)
>>  	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
>> -		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
>> -			priv->hw_info->cpu_port);
>> -		err = -EINVAL;
>> +		err = dev_err_probe(dev, -EINVAL,
>> +				    "wrong CPU port defined, HW only supports port: %i",
>> +				    priv->hw_info->cpu_port);
>>  		goto disable_switch;
>>  	}
> 
> Nitpick: there is no terminating \n here.

Oh, thanks for the hint. I'll correct that (and also check the complete 
source
file for that kind of mistakes).

Re: [PATCH net-next 04/13] net: dsa: lantiq_gswip: Don't manually call gswip_port_enable()

[Martin Schiller]

On 2024-06-07 13:11, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:25AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> We don't need to manually call gswip_port_enable() from within
>> gswip_setup() for the CPU port. DSA does this automatically for us.
>> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
> 
> Not to mention the first thing in gswip_port_enable(), which is:
> 
> 	if (!dsa_is_user_port(ds, port))
> 		return 0;
> 
> So the call is dead code anyway.

As you will have noticed, this code will be brought back to life in the 
next
patch.

Re: [PATCH net-next 05/13] net: dsa: lantiq_gswip: do also enable or disable cpu port

[Martin Schiller]

On 2024-06-07 13:18, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:26AM +0200, Martin Schiller wrote:
>> Before commit 74be4babe72f ("net: dsa: do not enable or disable non 
>> user
>> ports"), gswip_port_enable/disable() were also executed for the cpu 
>> port
>> in gswip_setup() which disabled the cpu port during initialization.
> 
> Ah, you also noticed this.
> 
>> 
>> Let's restore this by removing the dsa_is_user_port checks. Also, 
>> let's
>> clean up the gswip_port_enable() function so that we only have to 
>> check
>> for the cpu port once.
>> 
>> Fixes: 74be4babe72f ("net: dsa: do not enable or disable non user 
>> ports")
> 
> Fixes tags shouldn't be taken lightly. If you think there's a 
> functional
> user-visible problem caused by that change, you need to explain what
> that problem is and what it affects. Additionally, bug fix patches are
> sent out to the 'net' tree, not bundled up with 'net-next' material
> (unless they fix a change that's also exclusive to net-next).
> Otherwise, just drop the 'Fixes' tag.

OK, I will drop the 'Fixes' tag.

> 
>> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 24 ++++++++----------------
>>  1 file changed, 8 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/lantiq_gswip.c 
>> b/drivers/net/dsa/lantiq_gswip.c
>> index 3fd5599fca52..38b5f743e5ee 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -695,13 +695,18 @@ static int gswip_port_enable(struct dsa_switch 
>> *ds, int port,
>>  	struct gswip_priv *priv = ds->priv;
>>  	int err;
>> 
>> -	if (!dsa_is_user_port(ds, port))
>> -		return 0;
>> -
>>  	if (!dsa_is_cpu_port(ds, port)) {
>> +		u32 mdio_phy = 0;
>> +
>>  		err = gswip_add_single_port_br(priv, port, true);
>>  		if (err)
>>  			return err;
>> +
>> +		if (phydev)
>> +			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
>> +
>> +		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
>> +				GSWIP_MDIO_PHYp(port));
>>  	}
>> 
>>  	/* RMON Counter Enable for port */
>> @@ -714,16 +719,6 @@ static int gswip_port_enable(struct dsa_switch 
>> *ds, int port,
>>  	gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
>>  			  GSWIP_SDMA_PCTRLp(port));
>> 
>> -	if (!dsa_is_cpu_port(ds, port)) {
>> -		u32 mdio_phy = 0;
>> -
>> -		if (phydev)
>> -			mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
>> -
>> -		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_ADDR_MASK, mdio_phy,
>> -				GSWIP_MDIO_PHYp(port));
>> -	}
>> -
>>  	return 0;
>>  }
> 
> It would be good to state in the commit message that the operation
> reordering is safe. The commit seems to be concerned mainly with code
> cleanliness, which does not always take side effects into account.

Thanks for the hint. I will take it into account in the commit message.

Re: [PATCH net-next 09/13] net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU port

[Martin Schiller]

On 2024-06-07 13:26, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> Calling gswip_add_single_port_br() with the CPU port would be a bug
>> because then only the CPU port could talk to itself. Add the CPU port 
>> to
>> the validation at the beginning of gswip_add_single_port_br().
>> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/dsa/lantiq_gswip.c 
>> b/drivers/net/dsa/lantiq_gswip.c
>> index ee8296d5b901..d2195271ffe9 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct 
>> gswip_priv *priv, int port, bool add)
>>  	unsigned int max_ports = priv->hw_info->max_ports;
>>  	int err;
>> 
>> -	if (port >= max_ports) {
>> +	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
>>  		dev_err(priv->dev, "single port for %i supported\n", port);
>>  		return -EIO;
>>  	}
>> --
>> 2.39.2
>> 
> 
> Isn't the new check effectively dead code?

As long as the dsa_switch_ops .port_bridge_join and .port_bridge_leave 
are not
executed for the cpu port, I agree with you.

Re: [PATCH net-next 10/13] net: dsa: lantiq_gswip: Fix error message in gswip_add_single_port_br()

[Martin Schiller]

On 2024-06-07 13:27, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:31AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> The error message is printed when the port cannot be used. Update the
>> error message to reflect that.
>> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/dsa/lantiq_gswip.c 
>> b/drivers/net/dsa/lantiq_gswip.c
>> index d2195271ffe9..3c96a62b8e0a 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -658,7 +658,8 @@ static int gswip_add_single_port_br(struct 
>> gswip_priv *priv, int port, bool add)
>>  	int err;
>> 
>>  	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
>> -		dev_err(priv->dev, "single port for %i supported\n", port);
>> +		dev_err(priv->dev, "single port for %i is not supported\n",
>> +			port);
>>  		return -EIO;
>>  	}
>> 
>> --
>> 2.39.2
>> 
> 
> Isn't even the original condition (port >= max_ports) dead code? Why 
> not
> remove the condition altogether?

I also agree here if we can be sure, that .port_enable, 
.port_bridge_join and
.port_bridge_leave are only called for "valid" (<= max_ports) ports.

Re: [PATCH net-next 11/13] net: dsa: lantiq_gswip: Fix comments in gswip_port_vlan_filtering()

[Martin Schiller]

On 2024-06-07 13:44, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:32AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> Update the comments in gswip_port_vlan_filtering() so it's clear that
>> there are two separate cases, one for "tag based VLAN" and another one
>> for "port based VLAN".
>> 
>> Suggested-by: Martin Schiller <ms@dev.tdt.de>
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
> 
> Here and in whatever the previous patch turns into: please make more
> careful use of the word "fix". It carries connotations of addressing
> bugs which must be backported. Various automated tools scan the git 
> tree
> for bug fixes which were apparently "not properly submitted" and mark
> them for auto-selection to stable. You don't want to cause that for a
> minor comment.

OK, I will eliminate the term "fix" wherever it is not appropriate.

Re: [PATCH net-next 09/13] net: dsa: lantiq_gswip: Forbid gswip_add_single_port_br on the CPU port

[Vladimir Oltean]

On Fri, Jun 07, 2024 at 03:31:57PM +0200, Martin Schiller wrote:
> On 2024-06-07 13:26, Vladimir Oltean wrote:
> > On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote:
> > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > 
> > > Calling gswip_add_single_port_br() with the CPU port would be a bug
> > > because then only the CPU port could talk to itself. Add the CPU
> > > port to
> > > the validation at the beginning of gswip_add_single_port_br().
> > > 
> > > Signed-off-by: Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com>
> > > ---
> > >  drivers/net/dsa/lantiq_gswip.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/dsa/lantiq_gswip.c
> > > b/drivers/net/dsa/lantiq_gswip.c
> > > index ee8296d5b901..d2195271ffe9 100644
> > > --- a/drivers/net/dsa/lantiq_gswip.c
> > > +++ b/drivers/net/dsa/lantiq_gswip.c
> > > @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct
> > > gswip_priv *priv, int port, bool add)
> > >  	unsigned int max_ports = priv->hw_info->max_ports;
> > >  	int err;
> > > 
> > > -	if (port >= max_ports) {
> > > +	if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) {
> > >  		dev_err(priv->dev, "single port for %i supported\n", port);
> > >  		return -EIO;
> > >  	}
> > > --
> > > 2.39.2
> > > 
> > 
> > Isn't the new check effectively dead code?
> 
> As long as the dsa_switch_ops .port_bridge_join and .port_bridge_leave are not
> executed for the cpu port, I agree with you.

They aren't. The primary trigger for dsa_port_bridge_join() is dsa_user_changeupper(),
along with other code paths that replay the operation in certain circumstances,
again only for user ports.

Re: [PATCH net-next 10/13] net: dsa: lantiq_gswip: Fix error message in gswip_add_single_port_br()

[Vladimir Oltean]

On Fri, Jun 07, 2024 at 03:34:13PM +0200, Martin Schiller wrote:
> On 2024-06-07 13:27, Vladimir Oltean wrote:
> > Isn't even the original condition (port >= max_ports) dead code? Why not
> > remove the condition altogether?
> 
> I also agree here if we can be sure, that .port_enable, .port_bridge_join and
> .port_bridge_leave are only called for "valid" (<= max_ports) ports.

dsa_switch_parse_ports_of() has:

		if (reg >= ds->num_ports) {
			dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%u)\n",
				port, reg, ds->num_ports);
			of_node_put(port);
			err = -EINVAL;
			goto out_put_node;
		}

Re: [PATCH net-next 13/13] net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()

[Martin Schiller]

On 2024-06-07 13:41, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:34AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> Print the port which is not found to be part of a bridge so it's 
>> easier
>> to investigate the underlying issue.
> 
> Was there an actual issue which was investigated here? More details?

Well, there are probably still several problems with this driver. Martin
Blumenstingl is probably referring to the problem discussed in [1] and 
[2].

[1] https://github.com/openwrt/openwrt/pull/13200
[2] https://github.com/openwrt/openwrt/pull/13638

> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/dsa/lantiq_gswip.c 
>> b/drivers/net/dsa/lantiq_gswip.c
>> index 4bb894e75b81..69035598e8a4 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -1377,7 +1377,8 @@ static int gswip_port_fdb(struct dsa_switch *ds, 
>> int port,
>>  	}
>> 
>>  	if (fid == -1) {
>> -		dev_err(priv->dev, "Port not part of a bridge\n");
>> +		dev_err(priv->dev,
>> +			"Port %d is not known to be part of bridge\n", port);
>>  		return -EINVAL;
>>  	}
> 
> Actually I would argue this is entirely confusing. There is an earlier
> check:
> 
> 	if (!bridge)
> 		return -EINVAL;
> 
> which did _not_ trigger if we're executing this. So the port _is_ a 
> part
> of a bridge. Just say that no FID is found for bridge %s 
> (bridge->name),
> which technically _is_ what happened.

Yes, you are right. I'll change that.

Re: [PATCH net-next 13/13] net: dsa: lantiq_gswip: Improve error message in gswip_port_fdb()

[Vladimir Oltean]

On Fri, Jun 07, 2024 at 03:51:19PM +0200, Martin Schiller wrote:
> On 2024-06-07 13:41, Vladimir Oltean wrote:
> > On Thu, Jun 06, 2024 at 10:52:34AM +0200, Martin Schiller wrote:
> > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > 
> > > Print the port which is not found to be part of a bridge so it's
> > > easier
> > > to investigate the underlying issue.
> > 
> > Was there an actual issue which was investigated here? More details?
> 
> Well, there are probably still several problems with this driver. Martin
> Blumenstingl is probably referring to the problem discussed in [1] and [2].
> 
> [1] https://github.com/openwrt/openwrt/pull/13200
> [2] https://github.com/openwrt/openwrt/pull/13638

Ok, but that doesn't technically exercise this error path.

Re: [PATCH net-next 10/13] net: dsa: lantiq_gswip: Fix error message in gswip_add_single_port_br()

[Martin Schiller]

On 2024-06-07 15:50, Vladimir Oltean wrote:
> On Fri, Jun 07, 2024 at 03:34:13PM +0200, Martin Schiller wrote:
>> On 2024-06-07 13:27, Vladimir Oltean wrote:
>> > Isn't even the original condition (port >= max_ports) dead code? Why not
>> > remove the condition altogether?
>> 
>> I also agree here if we can be sure, that .port_enable, 
>> .port_bridge_join and
>> .port_bridge_leave are only called for "valid" (<= max_ports) ports.
> 
> dsa_switch_parse_ports_of() has:
> 
> 		if (reg >= ds->num_ports) {
> 			dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%u)\n",
> 				port, reg, ds->num_ports);
> 			of_node_put(port);
> 			err = -EINVAL;
> 			goto out_put_node;
> 		}

OK, so I will remove this check altogether.

Re: [PATCH net-next 12/13] net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro

[Martin Schiller]

On 2024-06-07 13:36, Vladimir Oltean wrote:
> On Thu, Jun 06, 2024 at 10:52:33AM +0200, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> Only bits [5:0] in mac_bridge.key[3] are reserved for the FID. Add a
>> macro so this becomes obvious when reading the driver code.
>> 
>> Signed-off-by: Martin Blumenstingl 
>> <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/dsa/lantiq_gswip.c 
>> b/drivers/net/dsa/lantiq_gswip.c
>> index f2faee112e33..4bb894e75b81 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -238,6 +238,7 @@
>>  #define GSWIP_TABLE_MAC_BRIDGE		0x0b
>>  #define  GSWIP_TABLE_MAC_BRIDGE_STATIC	BIT(0)		/* Static not, aging 
>> entry */
>>  #define  GSWIP_TABLE_MAC_BRIDGE_PORT	GENMASK(7, 4)	/* Port on learned 
>> entries */
>> +#define  GSWIP_TABLE_MAC_BRIDGE_FID	GENMASK(5, 0)	/* Filtering 
>> identifier */
>> 
>>  #define XRX200_GPHY_FW_ALIGN	(16 * 1024)
>> 
>> @@ -1385,7 +1386,7 @@ static int gswip_port_fdb(struct dsa_switch *ds, 
>> int port,
>>  	mac_bridge.key[0] = addr[5] | (addr[4] << 8);
>>  	mac_bridge.key[1] = addr[3] | (addr[2] << 8);
>>  	mac_bridge.key[2] = addr[1] | (addr[0] << 8);
>> -	mac_bridge.key[3] = fid;
>> +	mac_bridge.key[3] = FIELD_PREP(GSWIP_TABLE_MAC_BRIDGE_FID, fid);
>>  	mac_bridge.val[0] = add ? BIT(port) : 0; /* port map */
>>  	mac_bridge.val[1] = GSWIP_TABLE_MAC_BRIDGE_STATIC;
>>  	mac_bridge.valid = add;
>> --
>> 2.39.2
> 
> On second thought, I disagree with the naming scheme of the
> GSWIP_TABLE_MAC_BRIDGE_* macros. It is completely non obvious that they
> are non-overlapping, because they have the same name prefix, but:
> _STATIC applies to gswip_pce_table_entry :: val[1]
> _PORT applies to gswip_pce_table_entry :: val[0]
> _FID applies to gswip_pce_table_entry :: key[3]
> 
> I think it's all too easy to use the wrong macro on the wrong register 
> field.
> If the macros incorporated names like VAL1, KEY3 etc, it would be much
> more obvious. Could you please do that?

OK, so I'll change the macro names to

GSWIP_TABLE_MAC_BRIDGE_KEY3_FID
GSWIP_TABLE_MAC_BRIDGE_VAL0_PORT
GSWIP_TABLE_MAC_BRIDGE_VAL1_STATIC

Also the comment of GSWIP_TABLE_MAC_BRIDGE_VAL1_STATIC should be changed 
to
/* Static, not aging entry */

While looking again at this diff above, I noticed that val[0] is set
incorrectly. Shouldn't it be either "port << 4" or (after the previous 
patch)
"FIELD_PREP(GSWIP_TABLE_MAC_BRIDGE_PORT, port);" instead of "BIT(port)"?

Re: [PATCH net-next 12/13] net: dsa: lantiq_gswip: Add and use a GSWIP_TABLE_MAC_BRIDGE_FID macro

[Martin Schiller]

On 2024-06-07 16:27, Martin Schiller wrote:
> While looking again at this diff above, I noticed that val[0] is set
> incorrectly. Shouldn't it be either "port << 4" or (after the previous 
> patch)
> "FIELD_PREP(GSWIP_TABLE_MAC_BRIDGE_PORT, port);" instead of 
> "BIT(port)"?

Please ignore this comment. The format of the port specification differs
for static and dynamic (learned) entries.

Re: [PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Krzysztof Kozlowski]

On 06/06/2024 10:52, Martin Schiller wrote:
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> The CPU port has to specify a phy-mode and either a phy or a fixed-link.
> Since GSWIP is connected using a SoC internal protocol there's no PHY
> involved. Add phy-mode = "internal" and a fixed-link to describe the
> communication between the PMAC (Ethernet controller) and GSWIP switch.

You did nothing in the binding to describe them. You only extended
example, which does not really matter if there is DTS with it.

Best regards,
Krzysztof

Re: [PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Martin Schiller]

On 2024-06-10 10:55, Krzysztof Kozlowski wrote:
> On 06/06/2024 10:52, Martin Schiller wrote:
>> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> 
>> The CPU port has to specify a phy-mode and either a phy or a 
>> fixed-link.
>> Since GSWIP is connected using a SoC internal protocol there's no PHY
>> involved. Add phy-mode = "internal" and a fixed-link to describe the
>> communication between the PMAC (Ethernet controller) and GSWIP switch.
> 
> You did nothing in the binding to describe them. You only extended
> example, which does not really matter if there is DTS with it.
> 
> Best regards,
> Krzysztof

OK, so I'll update subject and commit message to signal that we only
update the example code.

Re: [PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Rob Herring]

On Mon, Jun 10, 2024 at 11:07:15AM +0200, Martin Schiller wrote:
> On 2024-06-10 10:55, Krzysztof Kozlowski wrote:
> > On 06/06/2024 10:52, Martin Schiller wrote:
> > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > 
> > > The CPU port has to specify a phy-mode and either a phy or a
> > > fixed-link.
> > > Since GSWIP is connected using a SoC internal protocol there's no PHY
> > > involved. Add phy-mode = "internal" and a fixed-link to describe the
> > > communication between the PMAC (Ethernet controller) and GSWIP switch.
> > 
> > You did nothing in the binding to describe them. You only extended
> > example, which does not really matter if there is DTS with it.
> > 
> > Best regards,
> > Krzysztof
> 
> OK, so I'll update subject and commit message to signal that we only
> update the example code.

Either convert it or leave it alone. If you are worried about users' DTs 
being wrong due to copying a bad example, then you should care enough to 
do the conversion. Given the errors we find in examples, it's likely 
not the only problem.

Rob

Re: [PATCH net-next 01/13] dt-bindings: net: dsa: lantiq_gswip: Add missing phy-mode and fixed-link

[Martin Schiller]

On 2024-06-11 00:05, Rob Herring wrote:
> On Mon, Jun 10, 2024 at 11:07:15AM +0200, Martin Schiller wrote:
>> On 2024-06-10 10:55, Krzysztof Kozlowski wrote:
>> > On 06/06/2024 10:52, Martin Schiller wrote:
>> > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> > >
>> > > The CPU port has to specify a phy-mode and either a phy or a
>> > > fixed-link.
>> > > Since GSWIP is connected using a SoC internal protocol there's no PHY
>> > > involved. Add phy-mode = "internal" and a fixed-link to describe the
>> > > communication between the PMAC (Ethernet controller) and GSWIP switch.
>> >
>> > You did nothing in the binding to describe them. You only extended
>> > example, which does not really matter if there is DTS with it.
>> >
>> > Best regards,
>> > Krzysztof
>> 
>> OK, so I'll update subject and commit message to signal that we only
>> update the example code.
> 
> Either convert it or leave it alone. If you are worried about users' 
> DTs
> being wrong due to copying a bad example, then you should care enough 
> to
> do the conversion. Given the errors we find in examples, it's likely
> not the only problem.
> 
> Rob

Then I will convert the bindings to the new YAML format and send another
update of this patch-set.

Thanks,
Martin