[PATCH RFC v2 net-next 0/4] mv88e6xxx: setting 2500base-x mode for CPU/DSA port in dts

[PATCH RFC v2 net-next 0/4] mv88e6xxx: setting 2500base-x mode for CPU/DSA port in dts

[Marek Behún]

Hi,

here is another proposal for supporting setting 2500base-x mode for
CPU/DSA ports in device tree correctly.

The changes from v1 are that instead of adding .port_setup() and
.port_teardown() methods to the DSA operations struct we instead, for
CPU/DSA ports, call dsa_port_enable() from dsa_port_setup(), but only
after the port is registered (and required phylink/devlink structures
exist).

The .port_enable/.port_disable methods are now only meant to be used
for user ports, when the slave interface is brought up/down. This
proposal changes that in such a way that these methods are also called
for CPU/DSA ports, but only just after the switch is set up (and just
before the switch is tore down).

If we went this way, we would have to patch the other DSA drivers to
check if user port is being given in their respective .port_enable
and .port_disable implmentations.

What do you think about this?

Marek

Marek Behún (4):
  net: dsa: mv88e6xxx: support 2500base-x in SGMII IRQ handler
  net: dsa: call port_enable/port_disable for CPU/DSA ports
  net: dsa: mv88e6xxx: check for port type in port_disable
  net: dsa: mv88e6xxx: do not enable SERDESes in mv88e6xxx_setup

 drivers/net/dsa/mv88e6xxx/chip.c   | 15 +++------------
 drivers/net/dsa/mv88e6xxx/serdes.c | 23 +++++++++++++++++++++--
 net/dsa/dsa2.c                     | 21 ++++++++++++++++++++-
 net/dsa/port.c                     |  4 ++--
 4 files changed, 46 insertions(+), 17 deletions(-)

-- 
2.21.0

[PATCH RFC v2 net-next 1/4] net: dsa: mv88e6xxx: support 2500base-x in SGMII IRQ handler

[Marek Behún]

The mv88e6390_serdes_irq_link_sgmii IRQ handler reads the SERDES PHY
status register to determine speed, among other things. If cmode of the
port is set to 2500base-x, though, the PHY still reports 1000 Mbps (the
PHY register itself does not differentiate between 1000 Mbps and 2500
Mbps - it thinks it is running at 1000 Mbps, although clock is 2.5x
faster).
Look at the cmode and set SPEED_2500 if cmode is set to 2500base-x.
Also tell mv88e6xxx_port_setup_mac the PHY interface mode corresponding
to current cmode in terms of phy_interface_t.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 20c526c2a9ee..0f3d7cbb696b 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -506,9 +506,11 @@ static void mv88e6390_serdes_irq_link_sgmii(struct mv88e6xxx_chip *chip,
 					    int port, int lane)
 {
 	struct dsa_switch *ds = chip->ds;
+	u8 cmode = chip->ports[port].cmode;
 	int duplex = DUPLEX_UNKNOWN;
 	int speed = SPEED_UNKNOWN;
 	int link, err;
+	phy_interface_t mode;
 	u16 status;
 
 	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
@@ -527,7 +529,10 @@ static void mv88e6390_serdes_irq_link_sgmii(struct mv88e6xxx_chip *chip,
 
 		switch (status & MV88E6390_SGMII_PHY_STATUS_SPEED_MASK) {
 		case MV88E6390_SGMII_PHY_STATUS_SPEED_1000:
-			speed = SPEED_1000;
+			if (cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX)
+				speed = SPEED_2500;
+			else
+				speed = SPEED_1000;
 			break;
 		case MV88E6390_SGMII_PHY_STATUS_SPEED_100:
 			speed = SPEED_100;
@@ -541,8 +546,22 @@ static void mv88e6390_serdes_irq_link_sgmii(struct mv88e6xxx_chip *chip,
 		}
 	}
 
+	switch (cmode) {
+	case MV88E6XXX_PORT_STS_CMODE_SGMII:
+		mode = PHY_INTERFACE_MODE_SGMII;
+		break;
+	case MV88E6XXX_PORT_STS_CMODE_1000BASE_X:
+		mode = PHY_INTERFACE_MODE_1000BASEX;
+		break;
+	case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
+		mode = PHY_INTERFACE_MODE_2500BASEX;
+		break;
+	default:
+		mode = PHY_INTERFACE_MODE_NA;
+	}
+
 	err = mv88e6xxx_port_setup_mac(chip, port, link, speed, duplex,
-				       PAUSE_OFF, PHY_INTERFACE_MODE_NA);
+				       PAUSE_OFF, mode);
 	if (err)
 		dev_err(chip->dev, "can't propagate PHY settings to MAC: %d\n",
 			err);
-- 
2.21.0

[PATCH RFC v2 net-next 2/4] net: dsa: call port_enable/port_disable for CPU/DSA ports

[Marek Behún]

Call dsa_port_enable for CPU/DSA ports in dsa_port_setup, and
dsa_port_disable for CPU/DSA ports in dsa_port_teardown. This requires
changing all DSA drivers, since they expect the port_enable/port_disable
methods to be called only for user ports.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/dsa/dsa2.c | 21 ++++++++++++++++++++-
 net/dsa/port.c |  4 ++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3abd173ebacb..98ea5c158ee3 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -315,6 +315,16 @@ static int dsa_port_setup(struct dsa_port *dp)
 		break;
 	}
 
+	switch (dp->type) {
+	case DSA_PORT_TYPE_CPU:
+	case DSA_PORT_TYPE_DSA:
+		if (!err)
+			err = dsa_port_enable(dp, NULL);
+		break;
+	default:
+		break;
+	}
+
 	if (err)
 		devlink_port_unregister(&dp->devlink_port);
 
@@ -323,8 +333,17 @@ static int dsa_port_setup(struct dsa_port *dp)
 
 static void dsa_port_teardown(struct dsa_port *dp)
 {
-	if (dp->type != DSA_PORT_TYPE_UNUSED)
+	switch (dp->type) {
+	case DSA_PORT_TYPE_UNUSED:
+		break;
+	case DSA_PORT_TYPE_CPU:
+	case DSA_PORT_TYPE_DSA:
+		dsa_port_disable(dp);
+		/* fall-through */
+	case DSA_PORT_TYPE_USER:
 		devlink_port_unregister(&dp->devlink_port);
+		break;
+	}
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
diff --git a/net/dsa/port.c b/net/dsa/port.c
index f071acf2842b..0cadda57df1f 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -75,7 +75,7 @@ int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
 			return err;
 	}
 
-	if (!dp->bridge_dev)
+	if (dp->type == DSA_PORT_TYPE_USER && !dp->bridge_dev)
 		dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
 
 	return 0;
@@ -86,7 +86,7 @@ void dsa_port_disable(struct dsa_port *dp)
 	struct dsa_switch *ds = dp->ds;
 	int port = dp->index;
 
-	if (!dp->bridge_dev)
+	if (dp->type == DSA_PORT_TYPE_USER && !dp->bridge_dev)
 		dsa_port_set_state_now(dp, BR_STATE_DISABLED);
 
 	if (ds->ops->port_disable)
-- 
2.21.0

[PATCH RFC v2 net-next 3/4] net: dsa: mv88e6xxx: check for port type in port_disable

[Marek Behún]

The mv88e6xxx_port_disable method calls mv88e6xxx_port_set_state, which
should be called only for user ports.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9b3ad22a5b98..ad27f2fc5c33 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2285,8 +2285,9 @@ 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 (dsa_is_user_port(ds, port))
+		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.21.0

[PATCH RFC v2 net-next 4/4] net: dsa: mv88e6xxx: do not enable SERDESes in mv88e6xxx_setup

[Marek Behún]

CPU/DSA ports are now enabled/disabled in the .port_enable() and
.port_disable() methods. We do not need to enable SERDESes for these
ports in mv88e6xxx_setup.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index ad27f2fc5c33..cca9f1e2038f 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
-- 
2.21.0

Re: [PATCH RFC v2 net-next 0/4] mv88e6xxx: setting 2500base-x mode for CPU/DSA port in dts

[Vivien Didelot]

Hi Marek,

On Sat, 17 Aug 2019 21:14:48 +0200, Marek Behún <marek.behun@nic.cz> wrote:
> Hi,
> 
> here is another proposal for supporting setting 2500base-x mode for
> CPU/DSA ports in device tree correctly.
> 
> The changes from v1 are that instead of adding .port_setup() and
> .port_teardown() methods to the DSA operations struct we instead, for
> CPU/DSA ports, call dsa_port_enable() from dsa_port_setup(), but only
> after the port is registered (and required phylink/devlink structures
> exist).
> 
> The .port_enable/.port_disable methods are now only meant to be used
> for user ports, when the slave interface is brought up/down. This
> proposal changes that in such a way that these methods are also called
> for CPU/DSA ports, but only just after the switch is set up (and just
> before the switch is tore down).
> 
> If we went this way, we would have to patch the other DSA drivers to
> check if user port is being given in their respective .port_enable
> and .port_disable implmentations.
> 
> What do you think about this?

This looks much better. Let me pass through all patches of this RFC so that
I can include bits I would like to see in your next series.


Thanks,

	Vivien

Re: [PATCH RFC v2 net-next 0/4] mv88e6xxx: setting 2500base-x mode for CPU/DSA port in dts

[Vivien Didelot]

Hi Marek,

On Sat, 17 Aug 2019 15:50:25 -0400, Vivien Didelot <vivien.didelot@gmail.com> wrote:
> > here is another proposal for supporting setting 2500base-x mode for
> > CPU/DSA ports in device tree correctly.
> > 
> > The changes from v1 are that instead of adding .port_setup() and
> > .port_teardown() methods to the DSA operations struct we instead, for
> > CPU/DSA ports, call dsa_port_enable() from dsa_port_setup(), but only
> > after the port is registered (and required phylink/devlink structures
> > exist).
> > 
> > The .port_enable/.port_disable methods are now only meant to be used
> > for user ports, when the slave interface is brought up/down. This
> > proposal changes that in such a way that these methods are also called
> > for CPU/DSA ports, but only just after the switch is set up (and just
> > before the switch is tore down).
> > 
> > If we went this way, we would have to patch the other DSA drivers to
> > check if user port is being given in their respective .port_enable
> > and .port_disable implmentations.
> > 
> > What do you think about this?
> 
> This looks much better. Let me pass through all patches of this RFC so that
> I can include bits I would like to see in your next series.

I went ahead and sent a series which enables and disables all ports
in DSA, I hope you don't mind. You can now send a single patch on
top of it focusing on the 2500base-x issue with all the details.


Thank you,

	Vivien