[PATCH net-next 0/6] net: dsa: enable and disable all ports

[PATCH net-next 0/6] net: dsa: enable and disable all ports

[Vivien Didelot]

The DSA stack currently calls the .port_enable and .port_disable switch
callbacks for slave ports only. However, it is useful to call them for all
port types. For example this allows some drivers to delay the optimization
of power consumption after the switch is setup. This can also help reducing
the setup code of drivers a bit.

The first DSA core patches enable and disable all ports of a switch, regardless
their type. The last mv88e6xxx patches remove redundant code from the driver
setup and the said callbacks, now that they handle SERDES power for all ports.

Vivien Didelot (6):
  net: dsa: use a single switch statement for port setup
  net: dsa: do not enable or disable non user ports
  net: dsa: enable and disable all ports
  net: dsa: mv88e6xxx: do not change STP state on port disabling
  net: dsa: mv88e6xxx: enable SERDES after setup
  net: dsa: mv88e6xxx: wrap SERDES IRQ in power function

 drivers/net/dsa/b53/b53_common.c       | 10 ++-
 drivers/net/dsa/bcm_sf2.c              |  6 ++
 drivers/net/dsa/lan9303-core.c         |  6 ++
 drivers/net/dsa/lantiq_gswip.c         |  6 ++
 drivers/net/dsa/microchip/ksz_common.c |  6 ++
 drivers/net/dsa/mt7530.c               |  6 ++
 drivers/net/dsa/mv88e6xxx/chip.c       | 64 ++++++-----------
 net/dsa/dsa2.c                         | 98 +++++++++++++-------------
 8 files changed, 112 insertions(+), 90 deletions(-)

-- 
2.22.0

[PATCH net-next 1/6] net: dsa: use a single switch statement for port setup

[Vivien Didelot]

It is currently difficult to read the different steps involved in the
setup and teardown of ports in the DSA code. Keep it simple with a
single switch statement for each port type: UNUSED, CPU, DSA, or USER.

Also no need to call devlink_port_unregister from within dsa_port_setup
as this step is inconditionally handled by dsa_port_teardown on error.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/dsa2.c | 87 ++++++++++++++++++++++----------------------------
 1 file changed, 39 insertions(+), 48 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3abd173ebacb..405552ac4c08 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -254,88 +254,79 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst)
 
 static int dsa_port_setup(struct dsa_port *dp)
 {
-	enum devlink_port_flavour flavour;
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_switch_tree *dst = ds->dst;
-	int err = 0;
-
-	if (dp->type == DSA_PORT_TYPE_UNUSED)
-		return 0;
-
-	memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
-	dp->mac = of_get_mac_address(dp->dn);
-
-	switch (dp->type) {
-	case DSA_PORT_TYPE_CPU:
-		flavour = DEVLINK_PORT_FLAVOUR_CPU;
-		break;
-	case DSA_PORT_TYPE_DSA:
-		flavour = DEVLINK_PORT_FLAVOUR_DSA;
-		break;
-	case DSA_PORT_TYPE_USER: /* fall-through */
-	default:
-		flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
-		break;
-	}
-
-	/* dp->index is used now as port_number. However
-	 * CPU and DSA ports should have separate numbering
-	 * independent from front panel port numbers.
-	 */
-	devlink_port_attrs_set(&dp->devlink_port, flavour,
-			       dp->index, false, 0,
-			       (const char *) &dst->index, sizeof(dst->index));
-	err = devlink_port_register(ds->devlink, &dp->devlink_port,
-				    dp->index);
-	if (err)
-		return err;
+	const unsigned char *id = (const unsigned char *)&dst->index;
+	const unsigned char len = sizeof(dst->index);
+	struct devlink_port *dlp = &dp->devlink_port;
+	struct devlink *dl = ds->devlink;
+	int err;
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
 		break;
 	case DSA_PORT_TYPE_CPU:
+		memset(dlp, 0, sizeof(*dlp));
+		devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_CPU,
+				       dp->index, false, 0, id, len);
+		err = devlink_port_register(dl, dlp, dp->index);
+		if (err)
+			return err;
+
 		err = dsa_port_link_register_of(dp);
 		if (err)
-			dev_err(ds->dev, "failed to setup link for port %d.%d\n",
-				ds->index, dp->index);
+			return err;
 		break;
 	case DSA_PORT_TYPE_DSA:
+		memset(dlp, 0, sizeof(*dlp));
+		devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_DSA,
+				       dp->index, false, 0, id, len);
+		err = devlink_port_register(dl, dlp, dp->index);
+		if (err)
+			return err;
+
 		err = dsa_port_link_register_of(dp);
 		if (err)
-			dev_err(ds->dev, "failed to setup link for port %d.%d\n",
-				ds->index, dp->index);
+			return err;
 		break;
 	case DSA_PORT_TYPE_USER:
+		memset(dlp, 0, sizeof(*dlp));
+		devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_PHYSICAL,
+				       dp->index, false, 0, id, len);
+		err = devlink_port_register(dl, dlp, dp->index);
+		if (err)
+			return err;
+
+		dp->mac = of_get_mac_address(dp->dn);
 		err = dsa_slave_create(dp);
 		if (err)
-			dev_err(ds->dev, "failed to create slave for port %d.%d\n",
-				ds->index, dp->index);
-		else
-			devlink_port_type_eth_set(&dp->devlink_port, dp->slave);
+			return err;
+
+		devlink_port_type_eth_set(dlp, dp->slave);
 		break;
 	}
 
-	if (err)
-		devlink_port_unregister(&dp->devlink_port);
-
-	return err;
+	return 0;
 }
 
 static void dsa_port_teardown(struct dsa_port *dp)
 {
-	if (dp->type != DSA_PORT_TYPE_UNUSED)
-		devlink_port_unregister(&dp->devlink_port);
+	struct devlink_port *dlp = &dp->devlink_port;
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
 		break;
 	case DSA_PORT_TYPE_CPU:
 		dsa_tag_driver_put(dp->tag_ops);
-		/* fall-through */
+		devlink_port_unregister(dlp);
+		dsa_port_link_unregister_of(dp);
+		break;
 	case DSA_PORT_TYPE_DSA:
+		devlink_port_unregister(dlp);
 		dsa_port_link_unregister_of(dp);
 		break;
 	case DSA_PORT_TYPE_USER:
+		devlink_port_unregister(dlp);
 		if (dp->slave) {
 			dsa_slave_destroy(dp->slave);
 			dp->slave = NULL;
-- 
2.22.0

[PATCH net-next 2/6] net: dsa: do not enable or disable non user ports

[Vivien Didelot]

The .port_enable and .port_disable operations are currently only
called for user ports, hence assuming they have a slave device. In
preparation for using these operations for other port types as well,
simply guard all implementations against non user ports and return
directly in such case.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c       | 10 +++++++++-
 drivers/net/dsa/bcm_sf2.c              |  6 ++++++
 drivers/net/dsa/lan9303-core.c         |  6 ++++++
 drivers/net/dsa/lantiq_gswip.c         |  6 ++++++
 drivers/net/dsa/microchip/ksz_common.c |  6 ++++++
 drivers/net/dsa/mt7530.c               |  6 ++++++
 drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++++++
 7 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 907af62846ba..1639ea7b7dab 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -510,10 +510,15 @@ EXPORT_SYMBOL(b53_imp_vlan_setup);
 int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	struct b53_device *dev = ds->priv;
-	unsigned int cpu_port = ds->ports[port].cpu_dp->index;
+	unsigned int cpu_port;
 	int ret = 0;
 	u16 pvlan;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
+	cpu_port = ds->ports[port].cpu_dp->index;
+
 	if (dev->ops->irq_enable)
 		ret = dev->ops->irq_enable(dev, port);
 	if (ret)
@@ -547,6 +552,9 @@ void b53_disable_port(struct dsa_switch *ds, int port)
 	struct b53_device *dev = ds->priv;
 	u8 reg;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	/* Disable Tx/Rx for the port */
 	b53_read8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(port), &reg);
 	reg |= PORT_CTRL_RX_DISABLE | PORT_CTRL_TX_DISABLE;
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 49f99436018a..3d06262817bd 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -157,6 +157,9 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 	unsigned int i;
 	u32 reg;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
 	/* Clear the memory power down */
 	reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
 	reg &= ~P_TXQ_PSM_VDD(port);
@@ -222,6 +225,9 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	u32 reg;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	/* Disable learning while in WoL mode */
 	if (priv->wol_ports_mask & (1 << port)) {
 		reg = core_readl(priv, CORE_DIS_LEARN);
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 7a2063e7737a..bbec86b9418e 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1079,6 +1079,9 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 {
 	struct lan9303 *chip = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
 	return lan9303_enable_processing_port(chip, port);
 }
 
@@ -1086,6 +1089,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port)
 {
 	struct lan9303 *chip = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	lan9303_disable_processing_port(chip, port);
 	lan9303_phy_write(ds, chip->phy_addr_base + port, MII_BMCR, BMCR_PDOWN);
 }
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 2175ec13bb2c..a69c9b9878b7 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -642,6 +642,9 @@ 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)) {
 		err = gswip_add_single_port_br(priv, port, true);
 		if (err)
@@ -678,6 +681,9 @@ 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;
+
 	if (!dsa_is_cpu_port(ds, port)) {
 		gswip_mdio_mask(priv, GSWIP_MDIO_PHY_LINK_DOWN,
 				GSWIP_MDIO_PHY_LINK_MASK,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b45c7b972cec..b0b870f0c252 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -361,6 +361,9 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	struct ksz_device *dev = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
 	if (dev->dev_ops->phy_setup)
@@ -378,6 +381,9 @@ void ksz_disable_port(struct dsa_switch *ds, int port)
 {
 	struct ksz_device *dev = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	dev->on_ports &= ~(1 << port);
 	dev->live_ports &= ~(1 << port);
 
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3181e95586d6..c48e29486b10 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -726,6 +726,9 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 
 	/* Setup the MAC for the user port */
@@ -751,6 +754,9 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	mutex_lock(&priv->reg_mutex);
 
 	/* Clear up all port matrix which could be restored in the next
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9b3ad22a5b98..5e557545df6d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2267,6 +2267,9 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
 	mv88e6xxx_reg_lock(chip);
 
 	err = mv88e6xxx_serdes_power(chip, port, true);
@@ -2283,6 +2286,9 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	if (!dsa_is_user_port(ds, port))
+		return;
+
 	mv88e6xxx_reg_lock(chip);
 
 	if (mv88e6xxx_port_set_state(chip, port, BR_STATE_DISABLED))
-- 
2.22.0

[PATCH net-next 3/6] net: dsa: enable and disable all ports

[Vivien Didelot]

Call the .port_enable and .port_disable functions for all ports,
not only the user ports, so that drivers may optimize the power
consumption of all ports after a successful setup.

Unused ports are now disabled on setup. CPU and DSA ports are now
enabled on setup and disabled on teardown. User ports were already
enabled at slave creation and disabled at slave destruction.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/dsa2.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 405552ac4c08..8c4eccb0cfe6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -264,6 +264,7 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
+		dsa_port_disable(dp);
 		break;
 	case DSA_PORT_TYPE_CPU:
 		memset(dlp, 0, sizeof(*dlp));
@@ -274,6 +275,10 @@ static int dsa_port_setup(struct dsa_port *dp)
 			return err;
 
 		err = dsa_port_link_register_of(dp);
+		if (err)
+			return err;
+
+		err = dsa_port_enable(dp, NULL);
 		if (err)
 			return err;
 		break;
@@ -286,6 +291,10 @@ static int dsa_port_setup(struct dsa_port *dp)
 			return err;
 
 		err = dsa_port_link_register_of(dp);
+		if (err)
+			return err;
+
+		err = dsa_port_enable(dp, NULL);
 		if (err)
 			return err;
 		break;
@@ -317,11 +326,13 @@ static void dsa_port_teardown(struct dsa_port *dp)
 	case DSA_PORT_TYPE_UNUSED:
 		break;
 	case DSA_PORT_TYPE_CPU:
+		dsa_port_disable(dp);
 		dsa_tag_driver_put(dp->tag_ops);
 		devlink_port_unregister(dlp);
 		dsa_port_link_unregister_of(dp);
 		break;
 	case DSA_PORT_TYPE_DSA:
+		dsa_port_disable(dp);
 		devlink_port_unregister(dlp);
 		dsa_port_link_unregister_of(dp);
 		break;
-- 
2.22.0

[PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Vivien Didelot]

When disabling a port, that is not for the driver to decide what to
do with the STP state. This is already handled by the DSA layer.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5e557545df6d..27e1622bb03b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2291,9 +2291,6 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 
 	mv88e6xxx_reg_lock(chip);
 
-	if (mv88e6xxx_port_set_state(chip, port, BR_STATE_DISABLED))
-		dev_err(chip->dev, "failed to disable port\n");
-
 	if (chip->info->ops->serdes_irq_free)
 		chip->info->ops->serdes_irq_free(chip, port);
 
-- 
2.22.0

[PATCH net-next 5/6] net: dsa: mv88e6xxx: enable SERDES after setup

[Vivien Didelot]

SERDES is powered on for CPU and DSA ports and powered down for unused
ports at setup time. But now that DSA calls mv88e6xxx_port_enable
and mv88e6xxx_port_disable for all ports, the SERDES power can now
be handled after setup inconditionally for all ports.

Using the port enable and disable callbacks also have the benefit to
handle the SERDES IRQ for non user ports as well.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 35 ++++----------------------------
 1 file changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 27e1622bb03b..c72b3db75c54 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2151,16 +2151,6 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
-	/* Enable the SERDES interface for DSA and CPU ports. Normal
-	 * ports SERDES are enabled when the port is enabled, thus
-	 * saving a bit of power.
-	 */
-	if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) {
-		err = mv88e6xxx_serdes_power(chip, port, true);
-		if (err)
-			return err;
-	}
-
 	/* Port Control 2: don't force a good FCS, set the maximum frame size to
 	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
 	 * untagged frames on this port, do a destination address lookup on all
@@ -2267,9 +2257,6 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	mv88e6xxx_reg_lock(chip);
 
 	err = mv88e6xxx_serdes_power(chip, port, true);
@@ -2286,9 +2273,6 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return;
-
 	mv88e6xxx_reg_lock(chip);
 
 	if (chip->info->ops->serdes_irq_free)
@@ -2461,27 +2445,16 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 
 	/* Setup Switch Port Registers */
 	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
+		if (dsa_is_unused_port(ds, i))
+			continue;
+
 		/* Prevent the use of an invalid port. */
-		if (mv88e6xxx_is_invalid_port(chip, i) &&
-		    !dsa_is_unused_port(ds, i)) {
+		if (mv88e6xxx_is_invalid_port(chip, i)) {
 			dev_err(chip->dev, "port %d is invalid\n", i);
 			err = -EINVAL;
 			goto unlock;
 		}
 
-		if (dsa_is_unused_port(ds, i)) {
-			err = mv88e6xxx_port_set_state(chip, i,
-						       BR_STATE_DISABLED);
-			if (err)
-				goto unlock;
-
-			err = mv88e6xxx_serdes_power(chip, i, false);
-			if (err)
-				goto unlock;
-
-			continue;
-		}
-
 		err = mv88e6xxx_setup_port(chip, i);
 		if (err)
 			goto unlock;
-- 
2.22.0

[PATCH net-next 6/6] net: dsa: mv88e6xxx: wrap SERDES IRQ in power function

[Vivien Didelot]

Now that mv88e6xxx_serdes_power is only called after driver setup,
we can wrap the SERDES IRQ code directly within it for clarity.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c72b3db75c54..d0bf98c10b2b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2057,10 +2057,26 @@ static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 static int mv88e6xxx_serdes_power(struct mv88e6xxx_chip *chip, int port,
 				  bool on)
 {
-	if (chip->info->ops->serdes_power)
-		return chip->info->ops->serdes_power(chip, port, on);
+	int err;
 
-	return 0;
+	if (!chip->info->ops->serdes_power)
+		return 0;
+
+	if (on) {
+		err = chip->info->ops->serdes_power(chip, port, true);
+		if (err)
+			return err;
+
+		if (chip->info->ops->serdes_irq_setup)
+			err = chip->info->ops->serdes_irq_setup(chip, port);
+	} else {
+		if (chip->info->ops->serdes_irq_free)
+			chip->info->ops->serdes_irq_free(chip, port);
+
+		err = chip->info->ops->serdes_power(chip, port, false);
+	}
+
+	return err;
 }
 
 static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
@@ -2258,12 +2274,7 @@ static int mv88e6xxx_port_enable(struct dsa_switch *ds, int port,
 	int err;
 
 	mv88e6xxx_reg_lock(chip);
-
 	err = mv88e6xxx_serdes_power(chip, port, true);
-
-	if (!err && chip->info->ops->serdes_irq_setup)
-		err = chip->info->ops->serdes_irq_setup(chip, port);
-
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
@@ -2274,13 +2285,8 @@ static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port)
 	struct mv88e6xxx_chip *chip = ds->priv;
 
 	mv88e6xxx_reg_lock(chip);
-
-	if (chip->info->ops->serdes_irq_free)
-		chip->info->ops->serdes_irq_free(chip, port);
-
 	if (mv88e6xxx_serdes_power(chip, port, false))
 		dev_err(chip->dev, "failed to power off SERDES\n");
-
 	mv88e6xxx_reg_unlock(chip);
 }
 
-- 
2.22.0

Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Andrew Lunn]

On Sun, Aug 18, 2019 at 01:35:46PM -0400, Vivien Didelot wrote:
> When disabling a port, that is not for the driver to decide what to
> do with the STP state. This is already handled by the DSA layer.

Hi Vivien

Putting the port into STP disabled state is how you actually disable
it, for the mv88e6xxx. So this is not really about STP, it is about
powering off the port. Maybe a comment is needed, rather than removing
the code?

    Thanks
	Andrew

Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Vivien Didelot]

Hi Andrew,

On Mon, 19 Aug 2019 15:40:57 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Aug 18, 2019 at 01:35:46PM -0400, Vivien Didelot wrote:
> > When disabling a port, that is not for the driver to decide what to
> > do with the STP state. This is already handled by the DSA layer.
> 
> Hi Vivien
> 
> Putting the port into STP disabled state is how you actually disable
> it, for the mv88e6xxx. So this is not really about STP, it is about
> powering off the port. Maybe a comment is needed, rather than removing
> the code?

This is not for the driver to decide, the stack already handles that.
Otherwise, calling dsa_port_disable on a bridged port would result in
mv88e6xxx forcing the STP state to Disabled while this is not expected.


Thanks,

	Vivien

Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Andrew Lunn]

On Mon, Aug 19, 2019 at 11:27:33AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> On Mon, 19 Aug 2019 15:40:57 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Sun, Aug 18, 2019 at 01:35:46PM -0400, Vivien Didelot wrote:
> > > When disabling a port, that is not for the driver to decide what to
> > > do with the STP state. This is already handled by the DSA layer.
> > 
> > Hi Vivien
> > 
> > Putting the port into STP disabled state is how you actually disable
> > it, for the mv88e6xxx. So this is not really about STP, it is about
> > powering off the port. Maybe a comment is needed, rather than removing
> > the code?
> 
> This is not for the driver to decide, the stack already handles that.
> Otherwise, calling dsa_port_disable on a bridged port would result in
> mv88e6xxx forcing the STP state to Disabled while this is not expected.

Hi Vivien

Lets look at this from a different angle.

The chip powers up. The older generation, the ports are enabled by
default. For newer generations the NO_CPU strapping determines the
power up state of a port.

What we want is that unused ports get powered off. The previous change
in the set does this. It calls mv88e6xxx_port_disable for all unused
ports. How do we disable the port? For the mv88ex666, we set the port
state to disabled. "The switch port is disabled and it will not
receive or transmit and frames." For other switches, there might be
other control registers to play with, other than STP.

Are you saying the core already sets the STP to disabled, for ports
which are unused? I did not spot that in your previous patch?

Thanks
	Andrew

Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Vivien Didelot]

On Mon, 19 Aug 2019 18:10:18 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Mon, 19 Aug 2019 15:40:57 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Sun, Aug 18, 2019 at 01:35:46PM -0400, Vivien Didelot wrote:
> > > > When disabling a port, that is not for the driver to decide what to
> > > > do with the STP state. This is already handled by the DSA layer.
> > > 
> > > Putting the port into STP disabled state is how you actually disable
> > > it, for the mv88e6xxx. So this is not really about STP, it is about
> > > powering off the port. Maybe a comment is needed, rather than removing
> > > the code?
> > 
> > This is not for the driver to decide, the stack already handles that.
> > Otherwise, calling dsa_port_disable on a bridged port would result in
> > mv88e6xxx forcing the STP state to Disabled while this is not expected.

[...]

> Are you saying the core already sets the STP to disabled, for ports
> which are unused? I did not spot that in your previous patch?

Just look at dsa_port_disable Andrew:


    void dsa_port_disable(struct dsa_port *dp)
    {
    	struct dsa_switch *ds = dp->ds;
    	int port = dp->index;
    
    	if (!dp->bridge_dev)
    		dsa_port_set_state_now(dp, BR_STATE_DISABLED);
    
    	if (ds->ops->port_disable)
    		ds->ops->port_disable(ds, port);
    }


The only thing worth arguing here is whether it makes sense to call
ds->ops->disable for a bridged port, or should we simply return right
away in this case. But this would be an independent patch anyway.


Thank you,

	Vivien

Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling

[Andrew Lunn]

On Mon, Aug 19, 2019 at 12:27:37PM -0400, Vivien Didelot wrote:
> On Mon, 19 Aug 2019 18:10:18 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Mon, 19 Aug 2019 15:40:57 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > On Sun, Aug 18, 2019 at 01:35:46PM -0400, Vivien Didelot wrote:
> > > > > When disabling a port, that is not for the driver to decide what to
> > > > > do with the STP state. This is already handled by the DSA layer.
> > > > 
> > > > Putting the port into STP disabled state is how you actually disable
> > > > it, for the mv88e6xxx. So this is not really about STP, it is about
> > > > powering off the port. Maybe a comment is needed, rather than removing
> > > > the code?
> > > 
> > > This is not for the driver to decide, the stack already handles that.
> > > Otherwise, calling dsa_port_disable on a bridged port would result in
> > > mv88e6xxx forcing the STP state to Disabled while this is not expected.
> 
> [...]
> 
> > Are you saying the core already sets the STP to disabled, for ports
> > which are unused? I did not spot that in your previous patch?
> 
> Just look at dsa_port_disable Andrew:
> 
> 
>     void dsa_port_disable(struct dsa_port *dp)
>     {
>     	struct dsa_switch *ds = dp->ds;
>     	int port = dp->index;
>     
>     	if (!dp->bridge_dev)
>     		dsa_port_set_state_now(dp, BR_STATE_DISABLED);
>     
>     	if (ds->ops->port_disable)
>     		ds->ops->port_disable(ds, port);
>     }
> 

Ah, cool. I completely missed that.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup

[Florian Fainelli]

On 8/18/19 10:35 AM, Vivien Didelot wrote:
> It is currently difficult to read the different steps involved in the
> setup and teardown of ports in the DSA code. Keep it simple with a
> single switch statement for each port type: UNUSED, CPU, DSA, or USER.
> 
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---

[snip]

>  	case DSA_PORT_TYPE_CPU:
> +		memset(dlp, 0, sizeof(*dlp));
> +		devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_CPU,
> +				       dp->index, false, 0, id, len);
> +		err = devlink_port_register(dl, dlp, dp->index);
> +		if (err)
> +			return err;

This is shared between all port flavors with the exception that the
flavor type is different, maybe we should create a helper function and
factor out even more code. I don't feel great about repeating 3 times t
the same code without making use of a fall through.
-- 
Florian

Re: [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports

[Florian Fainelli]

On 8/18/19 10:35 AM, Vivien Didelot wrote:
> The .port_enable and .port_disable operations are currently only
> called for user ports, hence assuming they have a slave device. In
> preparation for using these operations for other port types as well,
> simply guard all implementations against non user ports and return
> directly in such case.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---

[snip]

>
diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
> index 907af62846ba..1639ea7b7dab 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -510,10 +510,15 @@ EXPORT_SYMBOL(b53_imp_vlan_setup);
>  int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
>  {
>  	struct b53_device *dev = ds->priv;
> -	unsigned int cpu_port = ds->ports[port].cpu_dp->index;
> +	unsigned int cpu_port;
>  	int ret = 0;
>  	u16 pvlan;
>  
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	cpu_port = ds->ports[port].cpu_dp->index;
> +
>  	if (dev->ops->irq_enable)
>  		ret = dev->ops->irq_enable(dev, port);
>  	if (ret)
> @@ -547,6 +552,9 @@ void b53_disable_port(struct dsa_switch *ds, int port)
>  	struct b53_device *dev = ds->priv;
>  	u8 reg;
>  
> +	if (!dsa_is_user_port(ds, port))
> +		return;
> +
>  	/* Disable Tx/Rx for the port */
>  	b53_read8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(port), &reg);
>  	reg |= PORT_CTRL_RX_DISABLE | PORT_CTRL_TX_DISABLE;

For b53, the changes are correct, for bcm_sf2, see comments below:

> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 49f99436018a..3d06262817bd 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -157,6 +157,9 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
>  	unsigned int i;
>  	u32 reg;
>  
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
>  	/* Clear the memory power down */
>  	reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
>  	reg &= ~P_TXQ_PSM_VDD(port);
> @@ -222,6 +225,9 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>  	u32 reg;
>  
> +	if (!dsa_is_user_port(ds, port))
> +		return;

in bcm_sf2_sw_suspend(), we do call bcm_sf2_port_disable() against the
user and CPU port.
-- 
Florian

Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports

[Florian Fainelli]

On 8/18/19 10:35 AM, Vivien Didelot wrote:
> Call the .port_enable and .port_disable functions for all ports,
> not only the user ports, so that drivers may optimize the power
> consumption of all ports after a successful setup.
> 
> Unused ports are now disabled on setup. CPU and DSA ports are now
> enabled on setup and disabled on teardown. User ports were already
> enabled at slave creation and disabled at slave destruction.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup

[Vivien Didelot]

Hi Florian,

On Mon, 19 Aug 2019 10:14:24 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 8/18/19 10:35 AM, Vivien Didelot wrote:
> > It is currently difficult to read the different steps involved in the
> > setup and teardown of ports in the DSA code. Keep it simple with a
> > single switch statement for each port type: UNUSED, CPU, DSA, or USER.
> > 
> > Also no need to call devlink_port_unregister from within dsa_port_setup
> > as this step is inconditionally handled by dsa_port_teardown on error.
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > ---
> 
> [snip]
> 
> >  	case DSA_PORT_TYPE_CPU:
> > +		memset(dlp, 0, sizeof(*dlp));
> > +		devlink_port_attrs_set(dlp, DEVLINK_PORT_FLAVOUR_CPU,
> > +				       dp->index, false, 0, id, len);
> > +		err = devlink_port_register(dl, dlp, dp->index);
> > +		if (err)
> > +			return err;
> 
> This is shared between all port flavors with the exception that the
> flavor type is different, maybe we should create a helper function and
> factor out even more code. I don't feel great about repeating 3 times t
> the same code without making use of a fall through.

Nah I did not want to use an helper because the code is still too
noodly, if you look at user ports setup, we continue to setup devlink
after the slave creation, so here I prefered to keep things readable
and expose all steps first, before writing yet another helper.


Thanks,

	Vivien

Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports

[Marek Behun]

On Sun, 18 Aug 2019 13:35:45 -0400
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> Call the .port_enable and .port_disable functions for all ports,
> not only the user ports, so that drivers may optimize the power
> consumption of all ports after a successful setup.
> 
> Unused ports are now disabled on setup. CPU and DSA ports are now
> enabled on setup and disabled on teardown. User ports were already
> enabled at slave creation and disabled at slave destruction.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>

My original reason for enabling CPU and DSA ports is that enabling
serdes irq could not be done in .setup in mv88e6xxx, since the required
phylink structures did not yet exists for those ports.

The case after this patch would be that .port_enable is called for
CPU/DSA ports right after these required phylink structures are created
for this port. A thought came to me while reading this that some driver
in the future can expect, in their implementation of
port_enable/port_disable, that phylink structures already exist for all
ports, not just the one being currently enabled/disabled.

Wouldn't it be safer if CPU/DSA ports were enabled in setup after all
ports are registered, and disabled in teardown before ports are
unregistered?

Current:
  ->setup()
  for each port
      dsa_port_link_register_of()
      dsa_port_enable()

Proposed:
  ->setup()
  for each port
      dsa_port_link_register_of()
  for each port
      dsa_port_enable()

Marek

Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports

[Vivien Didelot]

Hi Marek,

On Mon, 19 Aug 2019 19:32:46 +0200, Marek Behun <marek.behun@nic.cz> wrote:
> > Call the .port_enable and .port_disable functions for all ports,
> > not only the user ports, so that drivers may optimize the power
> > consumption of all ports after a successful setup.
> > 
> > Unused ports are now disabled on setup. CPU and DSA ports are now
> > enabled on setup and disabled on teardown. User ports were already
> > enabled at slave creation and disabled at slave destruction.
> 
> My original reason for enabling CPU and DSA ports is that enabling
> serdes irq could not be done in .setup in mv88e6xxx, since the required
> phylink structures did not yet exists for those ports.
> 
> The case after this patch would be that .port_enable is called for
> CPU/DSA ports right after these required phylink structures are created
> for this port. A thought came to me while reading this that some driver
> in the future can expect, in their implementation of
> port_enable/port_disable, that phylink structures already exist for all
> ports, not just the one being currently enabled/disabled.
> 
> Wouldn't it be safer if CPU/DSA ports were enabled in setup after all
> ports are registered, and disabled in teardown before ports are
> unregistered?
> 
> Current:
>   ->setup()
>   for each port
>       dsa_port_link_register_of()
>       dsa_port_enable()
> 
> Proposed:
>   ->setup()
>   for each port
>       dsa_port_link_register_of()
>   for each port
>       dsa_port_enable()

I understand what you mean, but the scope of .port_enable really is the
port itself, so I do not expect a driver to configure something else. Also,
I prefer to keep it simple at the moment and not speculate about what a
driver may need in the future, as we may also wonder which switch must be
enabled first in a tree, etc. If that case happens in the future, we can
definitely isolate the ports enabling code after the tree is setup.

So if this series meets your requirement for now, I'd keep it like that.


Thanks,

	Vivien

Re: [PATCH net-next 0/6] net: dsa: enable and disable all ports

[David Miller]

From: Vivien Didelot <vivien.didelot@gmail.com>
Date: Sun, 18 Aug 2019 13:35:42 -0400

> The DSA stack currently calls the .port_enable and .port_disable switch
> callbacks for slave ports only. However, it is useful to call them for all
> port types. For example this allows some drivers to delay the optimization
> of power consumption after the switch is setup. This can also help reducing
> the setup code of drivers a bit.
> 
> The first DSA core patches enable and disable all ports of a switch, regardless
> their type. The last mv88e6xxx patches remove redundant code from the driver
> setup and the said callbacks, now that they handle SERDES power for all ports.

Looks like the bcm_sf2 parts need some adjustments.