Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next 0/6] net: dsa: enable and disable all ports
@ 2019-08-18 17:35 Vivien Didelot
  2019-08-18 17:35 ` [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup Vivien Didelot
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-19 17:14   ` Florian Fainelli
  2019-08-18 17:35 ` [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports Vivien Didelot
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
  2019-08-18 17:35 ` [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-19 17:16   ` Florian Fainelli
  2019-08-18 17:35 ` [PATCH net-next 3/6] net: dsa: enable and disable all ports Vivien Didelot
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 3/6] net: dsa: enable and disable all ports
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
  2019-08-18 17:35 ` [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup Vivien Didelot
  2019-08-18 17:35 ` [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-19 17:17   ` Florian Fainelli
  2019-08-19 17:32   ` Marek Behun
  2019-08-18 17:35 ` [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling Vivien Didelot
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
                   ` (2 preceding siblings ...)
  2019-08-18 17:35 ` [PATCH net-next 3/6] net: dsa: enable and disable all ports Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-19 13:40   ` Andrew Lunn
  2019-08-18 17:35 ` [PATCH net-next 5/6] net: dsa: mv88e6xxx: enable SERDES after setup Vivien Didelot
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 5/6] net: dsa: mv88e6xxx: enable SERDES after setup
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
                   ` (3 preceding siblings ...)
  2019-08-18 17:35 ` [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-18 17:35 ` [PATCH net-next 6/6] net: dsa: mv88e6xxx: wrap SERDES IRQ in power function Vivien Didelot
  2019-08-19 20:11 ` [PATCH net-next 0/6] net: dsa: enable and disable all ports David Miller
  6 siblings, 0 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next 6/6] net: dsa: mv88e6xxx: wrap SERDES IRQ in power function
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
                   ` (4 preceding siblings ...)
  2019-08-18 17:35 ` [PATCH net-next 5/6] net: dsa: mv88e6xxx: enable SERDES after setup Vivien Didelot
@ 2019-08-18 17:35 ` Vivien Didelot
  2019-08-19 20:11 ` [PATCH net-next 0/6] net: dsa: enable and disable all ports David Miller
  6 siblings, 0 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-18 17:35 UTC (permalink / raw)
  To: netdev; +Cc: marek.behun, davem, f.fainelli, andrew, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-18 17:35 ` [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling Vivien Didelot
@ 2019-08-19 13:40   ` Andrew Lunn
  2019-08-19 15:27     ` Vivien Didelot
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2019-08-19 13:40 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, marek.behun, davem, f.fainelli

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-19 13:40   ` Andrew Lunn
@ 2019-08-19 15:27     ` Vivien Didelot
  2019-08-19 16:10       ` Andrew Lunn
  0 siblings, 1 reply; 19+ messages in thread
From: Vivien Didelot @ 2019-08-19 15:27 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, marek.behun, davem, f.fainelli

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-19 15:27     ` Vivien Didelot
@ 2019-08-19 16:10       ` Andrew Lunn
  2019-08-19 16:27         ` Vivien Didelot
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Lunn @ 2019-08-19 16:10 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, marek.behun, davem, f.fainelli

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-19 16:10       ` Andrew Lunn
@ 2019-08-19 16:27         ` Vivien Didelot
  2019-08-19 16:44           ` Andrew Lunn
  0 siblings, 1 reply; 19+ messages in thread
From: Vivien Didelot @ 2019-08-19 16:27 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, marek.behun, davem, f.fainelli

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling
  2019-08-19 16:27         ` Vivien Didelot
@ 2019-08-19 16:44           ` Andrew Lunn
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Lunn @ 2019-08-19 16:44 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, marek.behun, davem, f.fainelli

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup
  2019-08-18 17:35 ` [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup Vivien Didelot
@ 2019-08-19 17:14   ` Florian Fainelli
  2019-08-19 17:20     ` Vivien Didelot
  0 siblings, 1 reply; 19+ messages in thread
From: Florian Fainelli @ 2019-08-19 17:14 UTC (permalink / raw)
  To: Vivien Didelot, netdev; +Cc: marek.behun, davem, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports
  2019-08-18 17:35 ` [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports Vivien Didelot
@ 2019-08-19 17:16   ` Florian Fainelli
  0 siblings, 0 replies; 19+ messages in thread
From: Florian Fainelli @ 2019-08-19 17:16 UTC (permalink / raw)
  To: Vivien Didelot, netdev; +Cc: marek.behun, davem, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports
  2019-08-18 17:35 ` [PATCH net-next 3/6] net: dsa: enable and disable all ports Vivien Didelot
@ 2019-08-19 17:17   ` Florian Fainelli
  2019-08-19 17:32   ` Marek Behun
  1 sibling, 0 replies; 19+ messages in thread
From: Florian Fainelli @ 2019-08-19 17:17 UTC (permalink / raw)
  To: Vivien Didelot, netdev; +Cc: marek.behun, davem, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup
  2019-08-19 17:14   ` Florian Fainelli
@ 2019-08-19 17:20     ` Vivien Didelot
  0 siblings, 0 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-19 17:20 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev, marek.behun, davem, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports
  2019-08-18 17:35 ` [PATCH net-next 3/6] net: dsa: enable and disable all ports Vivien Didelot
  2019-08-19 17:17   ` Florian Fainelli
@ 2019-08-19 17:32   ` Marek Behun
  2019-08-19 18:03     ` Vivien Didelot
  1 sibling, 1 reply; 19+ messages in thread
From: Marek Behun @ 2019-08-19 17:32 UTC (permalink / raw)
  To: Vivien Didelot; +Cc: netdev, davem, f.fainelli, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 3/6] net: dsa: enable and disable all ports
  2019-08-19 17:32   ` Marek Behun
@ 2019-08-19 18:03     ` Vivien Didelot
  0 siblings, 0 replies; 19+ messages in thread
From: Vivien Didelot @ 2019-08-19 18:03 UTC (permalink / raw)
  To: Marek Behun; +Cc: netdev, davem, f.fainelli, andrew

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next 0/6] net: dsa: enable and disable all ports
  2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
                   ` (5 preceding siblings ...)
  2019-08-18 17:35 ` [PATCH net-next 6/6] net: dsa: mv88e6xxx: wrap SERDES IRQ in power function Vivien Didelot
@ 2019-08-19 20:11 ` David Miller
  6 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2019-08-19 20:11 UTC (permalink / raw)
  To: vivien.didelot; +Cc: netdev, marek.behun, f.fainelli, andrew

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-18 17:35 [PATCH net-next 0/6] net: dsa: enable and disable all ports Vivien Didelot
2019-08-18 17:35 ` [PATCH net-next 1/6] net: dsa: use a single switch statement for port setup Vivien Didelot
2019-08-19 17:14   ` Florian Fainelli
2019-08-19 17:20     ` Vivien Didelot
2019-08-18 17:35 ` [PATCH net-next 2/6] net: dsa: do not enable or disable non user ports Vivien Didelot
2019-08-19 17:16   ` Florian Fainelli
2019-08-18 17:35 ` [PATCH net-next 3/6] net: dsa: enable and disable all ports Vivien Didelot
2019-08-19 17:17   ` Florian Fainelli
2019-08-19 17:32   ` Marek Behun
2019-08-19 18:03     ` Vivien Didelot
2019-08-18 17:35 ` [PATCH net-next 4/6] net: dsa: mv88e6xxx: do not change STP state on port disabling Vivien Didelot
2019-08-19 13:40   ` Andrew Lunn
2019-08-19 15:27     ` Vivien Didelot
2019-08-19 16:10       ` Andrew Lunn
2019-08-19 16:27         ` Vivien Didelot
2019-08-19 16:44           ` Andrew Lunn
2019-08-18 17:35 ` [PATCH net-next 5/6] net: dsa: mv88e6xxx: enable SERDES after setup Vivien Didelot
2019-08-18 17:35 ` [PATCH net-next 6/6] net: dsa: mv88e6xxx: wrap SERDES IRQ in power function Vivien Didelot
2019-08-19 20:11 ` [PATCH net-next 0/6] net: dsa: enable and disable all ports David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox