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

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

[Marek Behún]

Hello,

I am preparing device tree for Turris Mox, and am encountering the
question of how to properly tell in DTS that a specific port is
connected to the cpu via 2500base-x mode.

CPU port is connected to eth1. In eth1, I simply have
  &eth1 {
    phy-mode = "2500base-x";
    managed = "in-band-status";
  };

This does not work for the CPU/DSA ports though, because of how phylink
and mv88e6xxx operate. CPU/DSA ports SERDES is enabled from
mv88e6xxx_setup(). SERDES irq is not enabled for there ports. Enabling
SERDES irq for there ports cannot be done from mv88e6xxx_setup(),
because the IRQ may fire immediately after enablement, and the phylink
structures do not exist yet for there ports (they are create by DSA only
after the .setup() method is called).

One way to make it work is to use fixed-link for there ports, with
speed = <2500>. But looking at the mv88e6xxx driver I discovered that
this works only because we are lucky (or because of my commit
65b034cf5c176, whatever you prefer. But when I was sending that
patch, fixed-link was not needed for the switch to work):
  - when first the mv88e6xxx_port_setup_mac is called from
    mv88e6xxx_setup_port, it is called with SPEED_MAX. The
    ->port_max_speed_mode() method is called to determine cmode for
    this port, which is 2500basex
  - afterwards, mv88e6xxx_port_setup_mac is called with parameters
    speed=2500 and mode=PHY_INTERFACE_MODE_NA. Because of commit
    65b034cf5c176, cmode is not set to something else

I think that the correct way to do this would be for CPU/DSA port nodes
to have the same setting in dts as the eth node (eg 2500base-x/inband).

For this to work, the mv88e6390_serdes_irq_link_sgmii has to be able
to determine between 2500base-x vs 1000base-x modes. This is done by
the first patch.

The second patch adds two new methods into the DSA operations structure,
.port_setup() and .port_teardown(). The .port_setup is called from
dsa_port_setup() after the port is registered and phylink structure
already exists, and .port_teardown() is called from dsa_port_teardown()
before the port is unregistered.

The third patch then utilizes these new methods to enable/disable
SERDESes and ther IRQs for CPU/DSA ports.

Please comment on this new code, whether it is acceptable to have these
new methods, if they should be called differently, and so on.

Thank you.

Marek

Marek Behún (3):
  net: dsa: mv88e6xxx: support 2500base-x in SGMII IRQ handler
  net: dsa: add port_setup/port_teardown methods to DSA ops
  net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

 drivers/net/dsa/mv88e6xxx/chip.c   | 54 ++++++++++++++++++++++++------
 drivers/net/dsa/mv88e6xxx/serdes.c |  6 +++-
 include/net/dsa.h                  |  2 ++
 net/dsa/dsa2.c                     | 10 +++++-
 4 files changed, 60 insertions(+), 12 deletions(-)

-- 
2.21.0

[PATCH RFC net-next 1/3] 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.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 20c526c2a9ee..17bb4a705d44 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -506,6 +506,7 @@ 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;
@@ -527,7 +528,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;
-- 
2.21.0

[PATCH RFC net-next 2/3] net: dsa: add port_setup/port_teardown methods to DSA ops

[Marek Behún]

Add two new methods into the DSA operations structure:
  - port_setup(), called from dsa_port_setup() after the DSA port
    already registered
  - port_teardown(), called from dsa_port_teardown() before the port is
    unregistered

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>
---
 include/net/dsa.h |  2 ++
 net/dsa/dsa2.c    | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 147b757ef8ea..848898e5d7c5 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -360,6 +360,8 @@ struct dsa_switch_ops {
 
 	int	(*setup)(struct dsa_switch *ds);
 	void	(*teardown)(struct dsa_switch *ds);
+	int	(*port_setup)(struct dsa_switch *ds, int port);
+	void	(*port_teardown)(struct dsa_switch *ds, int port);
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
 	/*
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3abd173ebacb..c891300a6d2c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -315,6 +315,9 @@ static int dsa_port_setup(struct dsa_port *dp)
 		break;
 	}
 
+	if (!err && ds->ops->port_setup)
+		err = ds->ops->port_setup(ds, dp->index);
+
 	if (err)
 		devlink_port_unregister(&dp->devlink_port);
 
@@ -323,8 +326,13 @@ 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)
+	struct dsa_switch *ds = dp->ds;
+
+	if (dp->type != DSA_PORT_TYPE_UNUSED) {
 		devlink_port_unregister(&dp->devlink_port);
+		if (ds->ops->port_teardown)
+			ds->ops->port_teardown(ds, dp->index);
+	}
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
-- 
2.21.0

[PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Marek Behún]

When CPU/DSA port is put into for example into 2500base-x mode, the
SERDES irq has to be enabled so that port's MAC is configured properly
after autonegotiation.

When SERDES irq is being enabled, the port's phylink structure already
has to exist. Otherwise if the IRQ fires immediately, the IRQ routine's
access to the nonexistent phylink structure results in an exception.

We therefore enable SERDES irqs for CPU/DSA ports in the .port_setup()
method, which is called by DSA from dsa_setup_port after the port is
registered and phylink structures exist.

We also move SERDES powering on for CPU/DSA ports to this method.

We also free the IRQ and power off SERDESes for these ports in the
.port_teardown() method.

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 | 54 ++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9b3ad22a5b98..23d3e39d2b9c 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
@@ -2557,6 +2547,48 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	return err;
 }
 
+static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int 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))) {
+		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;
+	}
+
+	return 0;
+}
+
+static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) {
+		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);
+	}
+}
+
 static int mv88e6xxx_mdio_read(struct mii_bus *bus, int phy, int reg)
 {
 	struct mv88e6xxx_mdio_bus *mdio_bus = bus->priv;
@@ -4692,6 +4724,8 @@ static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
 	.setup			= mv88e6xxx_setup,
+	.port_setup		= mv88e6xxx_port_setup,
+	.port_teardown		= mv88e6xxx_port_teardown,
 	.phylink_validate	= mv88e6xxx_validate,
 	.phylink_mac_link_state	= mv88e6xxx_link_state,
 	.phylink_mac_config	= mv88e6xxx_mac_config,
-- 
2.21.0

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Vivien Didelot]

On Fri, 16 Aug 2019 17:08:34 +0200, Marek Behún <marek.behun@nic.cz> wrote:
> @@ -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
> @@ -2557,6 +2547,48 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>  	return err;
>  }
>  
> +static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int 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))) {
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))) {
> +		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);
> +	}
> +}

So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both
setup a port, differently, at different time. This is definitely error prone.

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Marek Behun]

On Fri, 16 Aug 2019 12:25:52 -0400
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both
> setup a port, differently, at different time. This is definitely error prone.

Hmm. I don't know how much of mv88e6xxx_setup_port() could be moved to
this new port_setup(), since there are other setup functions called in
mv88e6xxx_setup() that can possibly depend on what was done by
mv88e6xxx_setup_port().

Maybe the new DSA operations should be called .after_setup()
and .before_teardown(), and be called just once for the whole switch,
not for each port?

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Vivien Didelot]

Hi Marek,

On Fri, 16 Aug 2019 19:05:20 +0200, Marek Behun <marek.behun@nic.cz> wrote:
> On Fri, 16 Aug 2019 12:25:52 -0400
> Vivien Didelot <vivien.didelot@gmail.com> wrote:
> 
> > So now we have mv88e6xxx_setup_port() and mv88e6xxx_port_setup(), which both
> > setup a port, differently, at different time. This is definitely error prone.
> 
> Hmm. I don't know how much of mv88e6xxx_setup_port() could be moved to
> this new port_setup(), since there are other setup functions called in
> mv88e6xxx_setup() that can possibly depend on what was done by
> mv88e6xxx_setup_port().
> 
> Maybe the new DSA operations should be called .after_setup()
> and .before_teardown(), and be called just once for the whole switch,
> not for each port?

I think the DSA switch port_setup/port_teardown operations are fine, but the
idea would be that the drivers must no longer setup their ports directly
in their .setup function. So for mv88e6xxx precisely, we should rename
mv88e6xxx_setup_port to mv88e6xxx_port_setup, and move all the port-related
code from mv88e6xxx_setup into mv88e6xxx_port_setup.

Also, the DSA stack must call ds->ops->port_setup() for all ports, regardless
their type, i.e. even if their are unused.

As a reminder, *setup/*teardown are more like typical probe/remove callbacks
found in drivers, while enable/disable are a runtime thing, switching a port
on and off (think ifconfig up/down).


Thanks,

	Vivien

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Marek Behun]

Hi Vivien,

On Fri, 16 Aug 2019 19:05:37 -0400
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> I think the DSA switch port_setup/port_teardown operations are fine, but the
> idea would be that the drivers must no longer setup their ports directly
> in their .setup function. So for mv88e6xxx precisely, we should rename
> mv88e6xxx_setup_port to mv88e6xxx_port_setup, and move all the port-related
> code from mv88e6xxx_setup into mv88e6xxx_port_setup.

I looked into the driver, and found out that mv88e6xxx_setup calls many
other setup functions after the calls to mv88e6xxx_setup_port for each
port:
   1. setup errata
   2. cache cmode
   3. for each port setup_port
   4. irl setup
   5. mac setup
   6. phy setup
   7. vtu setup
   8. pvt setup
   9. atu setup
  10. broadcast setuo
  11. pot setup
  12. rmu setup
  13. rsvd2cpu setup
  14. trunk setup
  15. devmap setup
  16. pri setup
  17. ptp setup
  18. hwtstamp setup
  19. stats setup

The problem is that some of these steps (after step 3) may depend on
some of the work done by step 3. Some of these functions iterate again
over the port array (mv88e6xxx_hwtstamp_setup, for example).
We cannot simply move step 3 to be called from DSA after
mv88e6xxx_setup.

I now do not know exactly what to do about the error prone naming of
setup_port vs port_setup.

One way would be to rename the mv88e6xxx_setup_port function to
mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something
like that. Would the names mv88e6xxx_port_setup and
mv88e6xxx_setup_port_regs still be very confusing and error prone?
I think maybe yes...

Other solution would be to, instead of the .port_setup()
and .port_teardown() DSA ops, create the .after_setup()
and .before_teardown() ops I mentioned in the previous mail.

And yet another (in my opinion very improper) solution could be that
the .setup() method could call dsa_port_setup() from within itself, to
ensure that the needed structres exist.

Please let me know what you think about this.

The first solution to me currently seems as the easiest.

Marek

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Marek Behun]

On Sat, 17 Aug 2019 20:03:42 +0200
Marek Behun <marek.behun@nic.cz> wrote:

> One way would be to rename the mv88e6xxx_setup_port function to
> mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something
> like that. Would the names mv88e6xxx_port_setup and
> mv88e6xxx_setup_port_regs still be very confusing and error prone?
> I think maybe yes...
> 
> Other solution would be to, instead of the .port_setup()
> and .port_teardown() DSA ops, create the .after_setup()
> and .before_teardown() ops I mentioned in the previous mail.
> 
> And yet another (in my opinion very improper) solution could be that
> the .setup() method could call dsa_port_setup() from within itself, to
> ensure that the needed structres exist.

I thought of another solution, one that does not need new DSA
operations. What if dsa_port_enable was called for CPU/DSA ports after
in dsa_port_setup_switches, after all ports are setup, and
dsa_port_disable called for CPU/DSA ports in dsa_port_teardown_switches?

This seems to me as cleaner solution.

Marek

Re: [PATCH RFC net-next 3/3] net: dsa: mv88e6xxx: setup SERDES irq also for CPU/DSA ports

[Vivien Didelot]

Hi Marek,

On Sat, 17 Aug 2019 20:15:52 +0200, Marek Behun <marek.behun@nic.cz> wrote:
> On Sat, 17 Aug 2019 20:03:42 +0200
> Marek Behun <marek.behun@nic.cz> wrote:
> 
> > One way would be to rename the mv88e6xxx_setup_port function to
> > mv88e6xxx_setup_port_regs, or mv88e6xxx_port_pre_setup, or something
> > like that. Would the names mv88e6xxx_port_setup and
> > mv88e6xxx_setup_port_regs still be very confusing and error prone?
> > I think maybe yes...
> > 
> > Other solution would be to, instead of the .port_setup()
> > and .port_teardown() DSA ops, create the .after_setup()
> > and .before_teardown() ops I mentioned in the previous mail.
> > 
> > And yet another (in my opinion very improper) solution could be that
> > the .setup() method could call dsa_port_setup() from within itself, to
> > ensure that the needed structres exist.
> 
> I thought of another solution, one that does not need new DSA
> operations. What if dsa_port_enable was called for CPU/DSA ports after
> in dsa_port_setup_switches, after all ports are setup, and
> dsa_port_disable called for CPU/DSA ports in dsa_port_teardown_switches?
> 
> This seems to me as cleaner solution.
> 
> Marek

Yes DSA needs to initialize a switch before its ports, but the driver may
need the opposite. Splitting the switch and ports setup and moving it up to
the DSA stack is a separate topic in fact that I'll handle soon.

I guess you meant dsa_tree_setup_switches instead of dsa_port_setup_switches.

Let's call dsa_port_enable/dsa_port_disable for the CPU and DSA ports as well.


Thanks,

	Vivien