mv88e6xxx: tx regression in v5.3

Hi Andrew, Vivien,

Since kernel v5.3 (tested v5.3.15), the 88E6141 switch on SolidRun Clearfog GT-8K stopped transmitting packets on switch connected ports. Kernel v5.2 works fine (tested v5.2.21).

Here are the relevant kernel v5.3 log lines:

[    2.867424] mv88e6085 f412a200.mdio-mii:04: switch 0x3400 detected: Marvell 88E6141, revision 0
[    2.927445] libphy: mdio: probed
[    3.578496] mv88e6085 f412a200.mdio-mii:04 lan2 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:11] driver [Marvell 88E6390]
[    3.595674] mv88e6085 f412a200.mdio-mii:04 lan1 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:12] driver [Marvell 88E6390]
[    3.612797] mv88e6085 f412a200.mdio-mii:04 lan4 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:13] driver [Marvell 88E6390]
[    3.629910] mv88e6085 f412a200.mdio-mii:04 lan3 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:14] driver [Marvell 88E6390]
[    3.646049] mv88e6085 f412a200.mdio-mii:04: configuring for phy/ link mode
[    3.654451] DSA: tree 0 setup
...
[   10.784521] mvpp2 f4000000.ethernet eth2: configuring for fixed/2500base-x link mode
[   10.792401] mvpp2 f4000000.ethernet eth2: Link is Up - 2.5Gbps/Full - flow control off
[   19.817981] mv88e6085 f412a200.mdio-mii:04 lan1: configuring for phy/ link mode
[   19.827083] 8021q: adding VLAN 0 to HW filter on device lan1
[   21.577276] mv88e6085 f412a200.mdio-mii:04 lan1: Link is Up - 100Mbps/Full - flow control rx/tx
[   21.586030] IPv6: ADDRCONF(NETDEV_CHANGE): lan1: link becomes ready

The Tx count on the lan1 interface increments, but the ARP packets don't show on the network.

Quoted 24 lines · Baruch Siach

Hi Andrew, Vivien,

Since kernel v5.3 (tested v5.3.15), the 88E6141 switch on SolidRun Clearfog GT-8K stopped transmitting packets on switch connected ports. Kernel v5.2 works fine (tested v5.2.21).

Here are the relevant kernel v5.3 log lines:

[    2.867424] mv88e6085 f412a200.mdio-mii:04: switch 0x3400 detected: Marvell 88E6141, revision 0
[    2.927445] libphy: mdio: probed
[    3.578496] mv88e6085 f412a200.mdio-mii:04 lan2 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:11] driver [Marvell 88E6390]
[    3.595674] mv88e6085 f412a200.mdio-mii:04 lan1 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:12] driver [Marvell 88E6390]
[    3.612797] mv88e6085 f412a200.mdio-mii:04 lan4 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:13] driver [Marvell 88E6390]
[    3.629910] mv88e6085 f412a200.mdio-mii:04 lan3 (uninitialized): PHY [!cp1!config-space@f4000000!mdio@12a200!switch0@4!mdio:14] driver [Marvell 88E6390]
[    3.646049] mv88e6085 f412a200.mdio-mii:04: configuring for phy/ link mode
[    3.654451] DSA: tree 0 setup
...
[   10.784521] mvpp2 f4000000.ethernet eth2: configuring for fixed/2500base-x link mode
[   10.792401] mvpp2 f4000000.ethernet eth2: Link is Up - 2.5Gbps/Full - flow control off
[   19.817981] mv88e6085 f412a200.mdio-mii:04 lan1: configuring for phy/ link mode
[   19.827083] 8021q: adding VLAN 0 to HW filter on device lan1
[   21.577276] mv88e6085 f412a200.mdio-mii:04 lan1: Link is Up - 100Mbps/Full - flow control rx/tx
[   21.586030] IPv6: ADDRCONF(NETDEV_CHANGE): lan1: link becomes ready

Quoted 2 lines

The Tx count on the lan1 interface increments, but the ARP packets don't show on the network.

Hi Baruch

I don't know of an issues.

If the MAC TX counter increases, it sounds like it is a PHY issue? Does 100Mbps/Full make sense for this link?

Probably your best bet is to do a git bisect to find which commit broke it.

Bisect points at 7fb5a711545d ("net: dsa: mv88e6xxx: drop adjust_link to enabled phylink"). Reverting this commit on top of v5.3.15 fixes the issue (and brings the warning back). As I understand, this basically reverts the driver migration to phylink. What might be the issue with phylink?

Quoted 5 lines

Bisect points at 7fb5a711545d ("net: dsa: mv88e6xxx: drop adjust_link to enabled phylink"). Reverting this commit on top of v5.3.15 fixes the issue (and brings the warning back). As I understand, this basically reverts the driver migration to phylink. What might be the issue with phylink?

That suggests the MAC is wrongly running at 1G, and the PHY at 100M, which is why it does not work.

You probably want to add #define DEBUG to the top of phylink.c, and scatter some debug prints in mv88e6xxx_mac_config(). My guess would be, either mv88e6xxx_mac_config() is existing before configuring the MAC, or mv88e6xxx_port_setup_mac() has wrongly decided nothing has changed and so has not configured the MAC.

As you guessed, mv88e6xxx_mac_config() exits early because mv88e6xxx_phy_is_internal() returns true for port number 2, and 'mode' is MLO_AN_PHY. What is the right MAC/PHY setup flow in this case?

Quoted 3 lines

As you guessed, mv88e6xxx_mac_config() exits early because mv88e6xxx_phy_is_internal() returns true for port number 2, and 'mode' is MLO_AN_PHY. What is the right MAC/PHY setup flow in this case?

So this goes back to

commit d700ec4118f9d5e88db8f678e7342f28c93037b9

Author: Marek Vasut <[email protected]>
Date:   Wed Sep 12 00:15:24 2018 +0200

net: dsa: mv88e6xxx: Make sure to configure ports with external PHYs

The MV88E6xxx can have external PHYs attached to certain ports and those PHYs could even be on different MDIO bus than the one within the switch. This patch makes sure that ports with such PHYs are configured correctly according to the information provided by the PHY.

(quoted diff) +1 -1

@@ -709,13 +717,17 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
         struct mv88e6xxx_chip *chip = ds->priv;
         int speed, duplex, link, pause, err;
 
-       if (mode == MLO_AN_PHY)
+       if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
                 return;
 

The idea being, that the MAC has direct knowledge of the PHY configuration because it is internal. There is no need to configure the MAC, it does it itself.

This assumption seems wrong for the switch you have.

I think it is just a optimisation. So we can probably remove this phy internal test.

And

         } else if (!mv88e6xxx_phy_is_internal(ds, port)) {

also needs to change.

It would be interesting to know if the MAC is completely wrongly configured, or it is just a subset of parameters.

Quoted 3 lines

As you guessed, mv88e6xxx_mac_config() exits early because mv88e6xxx_phy_is_internal() returns true for port number 2, and 'mode' is MLO_AN_PHY. What is the right MAC/PHY setup flow in this case?

So this goes back to

commit d700ec4118f9d5e88db8f678e7342f28c93037b9

Author: Marek Vasut <[email protected]>
Date:   Wed Sep 12 00:15:24 2018 +0200

net: dsa: mv88e6xxx: Make sure to configure ports with external PHYs

The MV88E6xxx can have external PHYs attached to certain ports and those PHYs could even be on different MDIO bus than the one within the switch. This patch makes sure that ports with such PHYs are configured correctly according to the information provided by the PHY.

(quoted diff) +1 -1

@@ -709,13 +717,17 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
        struct mv88e6xxx_chip *chip = ds->priv;
        int speed, duplex, link, pause, err;
 
-       if (mode == MLO_AN_PHY)
+       if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
                return;
 

The idea being, that the MAC has direct knowledge of the PHY configuration because it is internal. There is no need to configure the MAC, it does it itself.

This assumption seems wrong for the switch you have.

I think it is just a optimisation. So we can probably remove this phy internal test.

And

        } else if (!mv88e6xxx_phy_is_internal(ds, port)) {

also needs to change.

It would be interesting to know if the MAC is completely wrongly configured, or it is just a subset of parameters.

I compared phylib to phylink calls to mv88e6xxx_port_setup_mac(). It turns out that the phylink adds mv88e6xxx_port_setup_mac() call for the cpu port (port 5 in my case) with these parameters and call stack:

[    4.219148] mv88e6xxx_port_setup_mac: port: 5 link: 0 speed: -1 duplex: 255
[    4.226144] CPU: 2 PID: 21 Comm: kworker/2:0 Not tainted 5.3.15-00003-gb9bb09189d02-dirty #104
[    4.234795] Hardware name: SolidRun ClearFog GT 8K (DT)
[    4.240044] Workqueue: events deferred_probe_work_func
[    4.245205] Call trace:
[    4.247661]  dump_backtrace+0x0/0x128
[    4.251339]  show_stack+0x14/0x1c
[    4.254669]  dump_stack+0xa4/0xd0
[    4.257998]  mv88e6xxx_port_setup_mac+0x78/0x2a0
[    4.262635]  mv88e6xxx_mac_config+0xd0/0x154
[    4.266924]  dsa_port_phylink_mac_config+0x2c/0x38
[    4.271736]  phylink_mac_config+0xe0/0x1cc
[    4.275849]  phylink_start+0xc8/0x224
[    4.279527]  dsa_port_link_register_of+0xe8/0x1b0
[    4.284251]  dsa_register_switch+0x7fc/0x908
[    4.288539]  mv88e6xxx_probe+0x62c/0x66c
[    4.292478]  mdio_probe+0x30/0x5c
[    4.295806]  really_probe+0x1d0/0x280
[    4.299483]  driver_probe_device+0xd4/0xe4
[    4.303596]  __device_attach_driver+0x94/0xa0
[    4.307971]  bus_for_each_drv+0x94/0xb4
[    4.311823]  __device_attach+0xc0/0x12c
[    4.315674]  device_initial_probe+0x10/0x18
[    4.319875]  bus_probe_device+0x2c/0x8c
[    4.323726]  deferred_probe_work_func+0x84/0x98
[    4.328276]  process_one_work+0x19c/0x258
[    4.332303]  process_scheduled_works+0x3c/0x40
[    4.336765]  worker_thread+0x228/0x2f8
[    4.340530]  kthread+0x114/0x124
[    4.343771]  ret_from_fork+0x10/0x18

This hunk removed that mv88e6xxx_port_setup_mac() call, and fixed Tx for me on v5.3.15:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..f0457274b5a9 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

Is that the right solution?

Quoted 53 lines

I compared phylib to phylink calls to mv88e6xxx_port_setup_mac(). It turns out that the phylink adds mv88e6xxx_port_setup_mac() call for the cpu port (port 5 in my case) with these parameters and call stack:

[    4.219148] mv88e6xxx_port_setup_mac: port: 5 link: 0 speed: -1 duplex: 255
[    4.226144] CPU: 2 PID: 21 Comm: kworker/2:0 Not tainted 5.3.15-00003-gb9bb09189d02-dirty #104
[    4.234795] Hardware name: SolidRun ClearFog GT 8K (DT)
[    4.240044] Workqueue: events deferred_probe_work_func
[    4.245205] Call trace:
[    4.247661]  dump_backtrace+0x0/0x128
[    4.251339]  show_stack+0x14/0x1c
[    4.254669]  dump_stack+0xa4/0xd0
[    4.257998]  mv88e6xxx_port_setup_mac+0x78/0x2a0
[    4.262635]  mv88e6xxx_mac_config+0xd0/0x154
[    4.266924]  dsa_port_phylink_mac_config+0x2c/0x38
[    4.271736]  phylink_mac_config+0xe0/0x1cc
[    4.275849]  phylink_start+0xc8/0x224
[    4.279527]  dsa_port_link_register_of+0xe8/0x1b0
[    4.284251]  dsa_register_switch+0x7fc/0x908
[    4.288539]  mv88e6xxx_probe+0x62c/0x66c
[    4.292478]  mdio_probe+0x30/0x5c
[    4.295806]  really_probe+0x1d0/0x280
[    4.299483]  driver_probe_device+0xd4/0xe4
[    4.303596]  __device_attach_driver+0x94/0xa0
[    4.307971]  bus_for_each_drv+0x94/0xb4
[    4.311823]  __device_attach+0xc0/0x12c
[    4.315674]  device_initial_probe+0x10/0x18
[    4.319875]  bus_probe_device+0x2c/0x8c
[    4.323726]  deferred_probe_work_func+0x84/0x98
[    4.328276]  process_one_work+0x19c/0x258
[    4.332303]  process_scheduled_works+0x3c/0x40
[    4.336765]  worker_thread+0x228/0x2f8
[    4.340530]  kthread+0x114/0x124
[    4.343771]  ret_from_fork+0x10/0x18

This hunk removed that mv88e6xxx_port_setup_mac() call, and fixed Tx for me on v5.3.15:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..f0457274b5a9 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

Quoted 1 line

Is that the right solution?

What needs testing is:

                                        port@0 {
                                                reg = <0>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                fixed-link {
                                                        speed = <100>;
                                                        full-duplex;
                                                };

At some point, there is a call to configure the CPU port to 100Mbps, because the SoC Ethernet does not support 1G. We need to ensure this does not break with your change.

Quoted 67 lines · Andrew Lunn

Quoted 53 lines

I compared phylib to phylink calls to mv88e6xxx_port_setup_mac(). It turns out that the phylink adds mv88e6xxx_port_setup_mac() call for the cpu port (port 5 in my case) with these parameters and call stack:

[    4.219148] mv88e6xxx_port_setup_mac: port: 5 link: 0 speed: -1 duplex: 255
[    4.226144] CPU: 2 PID: 21 Comm: kworker/2:0 Not tainted 5.3.15-00003-gb9bb09189d02-dirty #104
[    4.234795] Hardware name: SolidRun ClearFog GT 8K (DT)
[    4.240044] Workqueue: events deferred_probe_work_func
[    4.245205] Call trace:
[    4.247661]  dump_backtrace+0x0/0x128
[    4.251339]  show_stack+0x14/0x1c
[    4.254669]  dump_stack+0xa4/0xd0
[    4.257998]  mv88e6xxx_port_setup_mac+0x78/0x2a0
[    4.262635]  mv88e6xxx_mac_config+0xd0/0x154
[    4.266924]  dsa_port_phylink_mac_config+0x2c/0x38
[    4.271736]  phylink_mac_config+0xe0/0x1cc
[    4.275849]  phylink_start+0xc8/0x224
[    4.279527]  dsa_port_link_register_of+0xe8/0x1b0
[    4.284251]  dsa_register_switch+0x7fc/0x908
[    4.288539]  mv88e6xxx_probe+0x62c/0x66c
[    4.292478]  mdio_probe+0x30/0x5c
[    4.295806]  really_probe+0x1d0/0x280
[    4.299483]  driver_probe_device+0xd4/0xe4
[    4.303596]  __device_attach_driver+0x94/0xa0
[    4.307971]  bus_for_each_drv+0x94/0xb4
[    4.311823]  __device_attach+0xc0/0x12c
[    4.315674]  device_initial_probe+0x10/0x18
[    4.319875]  bus_probe_device+0x2c/0x8c
[    4.323726]  deferred_probe_work_func+0x84/0x98
[    4.328276]  process_one_work+0x19c/0x258
[    4.332303]  process_scheduled_works+0x3c/0x40
[    4.336765]  worker_thread+0x228/0x2f8
[    4.340530]  kthread+0x114/0x124
[    4.343771]  ret_from_fork+0x10/0x18

This hunk removed that mv88e6xxx_port_setup_mac() call, and fixed Tx for me on v5.3.15:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..f0457274b5a9 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

Quoted 1 line

Is that the right solution?

What needs testing is:

                                        port@0 {
                                                reg = <0>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                fixed-link {
                                                        speed = <100>;
                                                        full-duplex;
                                                };

Quoted 3 lines

At some point, there is a call to configure the CPU port to 100Mbps, because the SoC Ethernet does not support 1G. We need to ensure this does not break with your change.

So maybe this:

drivers/net/dsa/mv88e6xxx/chip.c +0 -0

index d0a97eb73a37..84ca4f36a778 100644

This is not enough to fix v5.4, though. Commit 7a3007d22e8dc ("net: dsa:
mv88e6xxx: fully support SERDES on Topaz family") breaks switch Tx on SolidRun Clearfog GT-8K in much the same way. I could not easily revert 7a3007d22e8dc on top of v5.4, but this is enough to make Tx work again with v5.4:

drivers/net/dsa/mv88e6xxx/chip.c +0 -1

index 7b5b73499e37..e7e6400a994e 100644
@@ -3191,7 +3191,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
 	.port_link_state = mv88e6352_port_link_state,
 	.port_get_cmode = mv88e6352_port_get_cmode,
-	.port_set_cmode = mv88e6341_port_set_cmode,
 	.port_setup_message_port = mv88e6xxx_setup_message_port,
 	.stats_snapshot = mv88e6390_g1_stats_snapshot,
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,

Marek, do you have any idea how to properly fix this?

Thanks,
baruch

Quoted 18 lines · On Thu, 12 Dec 2019 20:17:29 +0200 Baruch Siach <[email protected]> wrote:

This is not enough to fix v5.4, though. Commit 7a3007d22e8dc ("net: dsa:
mv88e6xxx: fully support SERDES on Topaz family") breaks switch Tx on SolidRun Clearfog GT-8K in much the same way. I could not easily revert 7a3007d22e8dc on top of v5.4, but this is enough to make Tx work again with v5.4:

drivers/net/dsa/mv88e6xxx/chip.c +0 -1

index 7b5b73499e37..e7e6400a994e 100644
@@ -3191,7 +3191,6 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
 	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
 	.port_link_state = mv88e6352_port_link_state,
 	.port_get_cmode = mv88e6352_port_get_cmode,
-	.port_set_cmode = mv88e6341_port_set_cmode,
 	.port_setup_message_port = mv88e6xxx_setup_message_port,
 	.stats_snapshot = mv88e6390_g1_stats_snapshot,
 	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,

Quoted 1 line

Marek, do you have any idea how to properly fix this?

can you tell on which port the port_set_cmode is called when things break? Because if it is other than port 5, than method mv88e6341_port_set_cmode does nothing. If it is port 5, do you have cpu connected to the switch via port 5? What phy mode?

Hi Andrew,

Quoted 67 lines · Andrew Lunn

Quoted 53 lines

I compared phylib to phylink calls to mv88e6xxx_port_setup_mac(). It turns out that the phylink adds mv88e6xxx_port_setup_mac() call for the cpu port (port 5 in my case) with these parameters and call stack:

[    4.219148] mv88e6xxx_port_setup_mac: port: 5 link: 0 speed: -1 duplex: 255
[    4.226144] CPU: 2 PID: 21 Comm: kworker/2:0 Not tainted 5.3.15-00003-gb9bb09189d02-dirty #104
[    4.234795] Hardware name: SolidRun ClearFog GT 8K (DT)
[    4.240044] Workqueue: events deferred_probe_work_func
[    4.245205] Call trace:
[    4.247661]  dump_backtrace+0x0/0x128
[    4.251339]  show_stack+0x14/0x1c
[    4.254669]  dump_stack+0xa4/0xd0
[    4.257998]  mv88e6xxx_port_setup_mac+0x78/0x2a0
[    4.262635]  mv88e6xxx_mac_config+0xd0/0x154
[    4.266924]  dsa_port_phylink_mac_config+0x2c/0x38
[    4.271736]  phylink_mac_config+0xe0/0x1cc
[    4.275849]  phylink_start+0xc8/0x224
[    4.279527]  dsa_port_link_register_of+0xe8/0x1b0
[    4.284251]  dsa_register_switch+0x7fc/0x908
[    4.288539]  mv88e6xxx_probe+0x62c/0x66c
[    4.292478]  mdio_probe+0x30/0x5c
[    4.295806]  really_probe+0x1d0/0x280
[    4.299483]  driver_probe_device+0xd4/0xe4
[    4.303596]  __device_attach_driver+0x94/0xa0
[    4.307971]  bus_for_each_drv+0x94/0xb4
[    4.311823]  __device_attach+0xc0/0x12c
[    4.315674]  device_initial_probe+0x10/0x18
[    4.319875]  bus_probe_device+0x2c/0x8c
[    4.323726]  deferred_probe_work_func+0x84/0x98
[    4.328276]  process_one_work+0x19c/0x258
[    4.332303]  process_scheduled_works+0x3c/0x40
[    4.336765]  worker_thread+0x228/0x2f8
[    4.340530]  kthread+0x114/0x124
[    4.343771]  ret_from_fork+0x10/0x18

This hunk removed that mv88e6xxx_port_setup_mac() call, and fixed Tx for me on v5.3.15:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..f0457274b5a9 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

Quoted 1 line

Is that the right solution?

What needs testing is:

                                        port@0 {
                                                reg = <0>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                fixed-link {
                                                        speed = <100>;
                                                        full-duplex;
                                                };

Quoted 3 lines

At some point, there is a call to configure the CPU port to 100Mbps, because the SoC Ethernet does not support 1G. We need to ensure this does not break with your change.

So maybe this:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..84ca4f36a778 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (mode != MLO_AN_FIXED && dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

baruch

Quoted 83 lines · On Thu, 12 Dec 2019 17:23:55 +0200 Baruch Siach <[email protected]> wrote:

Quoted 67 lines · Andrew Lunn

Quoted 53 lines

I compared phylib to phylink calls to mv88e6xxx_port_setup_mac(). It turns out that the phylink adds mv88e6xxx_port_setup_mac() call for the cpu port (port 5 in my case) with these parameters and call stack:

[    4.219148] mv88e6xxx_port_setup_mac: port: 5 link: 0 speed: -1 duplex: 255
[    4.226144] CPU: 2 PID: 21 Comm: kworker/2:0 Not tainted 5.3.15-00003-gb9bb09189d02-dirty #104
[    4.234795] Hardware name: SolidRun ClearFog GT 8K (DT)
[    4.240044] Workqueue: events deferred_probe_work_func
[    4.245205] Call trace:
[    4.247661]  dump_backtrace+0x0/0x128
[    4.251339]  show_stack+0x14/0x1c
[    4.254669]  dump_stack+0xa4/0xd0
[    4.257998]  mv88e6xxx_port_setup_mac+0x78/0x2a0
[    4.262635]  mv88e6xxx_mac_config+0xd0/0x154
[    4.266924]  dsa_port_phylink_mac_config+0x2c/0x38
[    4.271736]  phylink_mac_config+0xe0/0x1cc
[    4.275849]  phylink_start+0xc8/0x224
[    4.279527]  dsa_port_link_register_of+0xe8/0x1b0
[    4.284251]  dsa_register_switch+0x7fc/0x908
[    4.288539]  mv88e6xxx_probe+0x62c/0x66c
[    4.292478]  mdio_probe+0x30/0x5c
[    4.295806]  really_probe+0x1d0/0x280
[    4.299483]  driver_probe_device+0xd4/0xe4
[    4.303596]  __device_attach_driver+0x94/0xa0
[    4.307971]  bus_for_each_drv+0x94/0xb4
[    4.311823]  __device_attach+0xc0/0x12c
[    4.315674]  device_initial_probe+0x10/0x18
[    4.319875]  bus_probe_device+0x2c/0x8c
[    4.323726]  deferred_probe_work_func+0x84/0x98
[    4.328276]  process_one_work+0x19c/0x258
[    4.332303]  process_scheduled_works+0x3c/0x40
[    4.336765]  worker_thread+0x228/0x2f8
[    4.340530]  kthread+0x114/0x124
[    4.343771]  ret_from_fork+0x10/0x18

This hunk removed that mv88e6xxx_port_setup_mac() call, and fixed Tx for me on v5.3.15:

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

index d0a97eb73a37..f0457274b5a9 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 
+	if (dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

Quoted 1 line

Is that the right solution?

What needs testing is:

                                        port@0 {
                                                reg = <0>;
                                                label = "cpu";
                                                ethernet = <&fec1>;

                                                fixed-link {
                                                        speed = <100>;
                                                        full-duplex;
                                                };

Quoted 3 lines

At some point, there is a call to configure the CPU port to 100Mbps, because the SoC Ethernet does not support 1G. We need to ensure this does not break with your change.

So maybe this:

drivers/net/dsa/mv88e6xxx/chip.c +0 -0

index d0a97eb73a37..84ca4f36a778 100644
@@ -611,6 +611,9 @@ static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
 	if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
 		return;
 

drivers/net/dsa/mv88e6xxx/chip.c static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port, · line 619

Quoted 6 lines

drivers/net/dsa/mv88e6xxx/chip.c +3 -0

+	if (mode != MLO_AN_FIXED && dsa_is_cpu_port(ds, port))
+		return;
+
 	if (mode == MLO_AN_FIXED) {
 		link = LINK_FORCED_UP;
 		speed = state->speed;

No, if your switch is connected to the cpu via 100mbps on the cpu side, you have to add fixed-link node as Andrew suggested. This is what we were trying to do with the Topaz patches, so that dsa port nodes support fixed-link via phylink.

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +0 -0

index bd881497b872..8f61cae9d3b0 100644

Quoted 5 lines

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +0 -0

index bd881497b872..8f61cae9d3b0 100644

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts port@5 { · line 417

Quoted 10 lines

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +5 -0

@@ -408,6 +408,11 @@ port@5 {
 				reg = <5>;
 				label = "cpu";
 				ethernet = <&cp1_eth2>;
+
+				fixed-link {
+					speed = <2500>;
+					full-duplex;
+				};
 			};
 		};  

The DSA driver is expected to configure the CPU port at its maximum speed. You should only add a fixed link if you need to slow it down. I expect 2500 is the maximum speed of this port.

Andrew

Baruch, if the cpu port is in 2500 base-x, remove the fixed-link and do this:

port@5 {
	reg = <5>;
	label = "cpu";
	ethernet = <&cp1_eth2>;
	phy-mode = "2500base-x";
	managed = "in-band-status";
};

Andrew, if the dsa driver is expected to do that, the code certainly does not do so. For example in mv88e6xxx_port_set_cmode you have:
/* Default to a slow mode, so freeing up SERDES interfaces for
* other ports which might use them for SFPs.
*/
if (mode == PHY_INTERFACE_MODE_NA)

         mode = PHY_INTERFACE_MODE_1000BASEX;

Quoted 11 lines · On Thu, 12 Dec 2019 20:31:29 +0100 Andrew Lunn <[email protected]> wrote:

Quoted 11 lines

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +3 -0

index bd881497b872..8f61cae9d3b0 100644
@@ -408,6 +408,11 @@ port@5 {
 				reg = <5>;
 				label = "cpu";
 				ethernet = <&cp1_eth2>;
+
+				fixed-link {
+					speed = <2500>;

Quoted 8 lines

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts port@5 { · line 417

Quoted 4 lines

arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +2 -0

+					full-duplex;
+				};
 			};
 		};

The DSA driver is expected to configure the CPU port at its maximum speed. You should only add a fixed link if you need to slow it down. I expect 2500 is the maximum speed of this port.

Baruch, if the cpu port is in 2500 base-x, remove the fixed-link and do this:

port@5 {
	reg = <5>;
	label = "cpu";
	ethernet = <&cp1_eth2>;
	phy-mode = "2500base-x";
	managed = "in-band-status";
};

The call to mv88e6341_port_set_cmode() introduced in commit 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family") still breaks port 5 (cpu) configuration. When called, its mode parameter is set to PHY_INTERFACE_MODE_2500BASEX (19).

Any idea?

This fixes cpu port configuration for me:

drivers/net/dsa/mv88e6xxx/port.c +0 -4

index 7fe256c5739d..a6c320978bcf 100644
@@ -427,10 +427,6 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
-		return 0;
-
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
 		if (chip->ports[port].serdes_irq) {

Does that make sense?

Quoted 18 lines

This fixes cpu port configuration for me:

drivers/net/dsa/mv88e6xxx/port.c +0 -4

index 7fe256c5739d..a6c320978bcf 100644
@@ -427,10 +427,6 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
-		return 0;
-
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
 		if (chip->ports[port].serdes_irq) {

Quoted 1 line

Does that make sense?

This needs testing on a 6390, with a port 9 or 10 using fixed link. We have had issues in the past where mac_config() has been called once per second, and each time it reconfigured the MAC, causing the link to go down/up. So we try to avoid doing work which is not requires and which could upset the link.

Hi Andrew,

Quoted 18 lines · Andrew Lunn

Quoted 18 lines

This fixes cpu port configuration for me:

drivers/net/dsa/mv88e6xxx/port.c +0 -4

index 7fe256c5739d..a6c320978bcf 100644
@@ -427,10 +427,6 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
-		return 0;
-
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
 		if (chip->ports[port].serdes_irq) {

Quoted 7 lines

Quoted 1 line

Does that make sense?

This needs testing on a 6390, with a port 9 or 10 using fixed link. We have had issues in the past where mac_config() has been called once per second, and each time it reconfigured the MAC, causing the link to go down/up. So we try to avoid doing work which is not requires and which could upset the link.

You refer to ed8fe20205ac ("net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode") that introduced this code.

The alternative is to call ->port_get_cmode() to refresh the cmode cache after mv88e6xxx_port_hidden_write().

Quoted 30 lines · Baruch Siach

Quoted 18 lines · Andrew Lunn

Quoted 18 lines

This fixes cpu port configuration for me:

drivers/net/dsa/mv88e6xxx/port.c +0 -4

index 7fe256c5739d..a6c320978bcf 100644
@@ -427,10 +427,6 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 		cmode = 0;
 	}
 
-	/* cmode doesn't change, nothing to do for us */
-	if (cmode == chip->ports[port].cmode)
-		return 0;
-
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
 		if (chip->ports[port].serdes_irq) {

Quoted 7 lines

Quoted 1 line

Does that make sense?

This needs testing on a 6390, with a port 9 or 10 using fixed link. We have had issues in the past where mac_config() has been called once per second, and each time it reconfigured the MAC, causing the link to go down/up. So we try to avoid doing work which is not requires and which could upset the link.

You refer to ed8fe20205ac ("net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode") that introduced this code.

Quoted 2 lines

The alternative is to call ->port_get_cmode() to refresh the cmode cache after mv88e6xxx_port_hidden_write().

Refreshing the cmode after mv88e6xxx_port_hidden_write() sounds like a good idea. But please limit it to just switches which need to make cmode writable. cmode is rather fragile and the 6390 family is easy to break in this area.

Thanks. That is enough to fix the phylink issue triggered by commit 7fb5a711545 ("net: dsa: mv88e6xxx: drop adjust_link to enabled phylink").

The Clearfog GT-8K DT has also this on the cpu side:

&cp1_eth2 {
        status = "okay";
        phy-mode = "2500base-x";
        phys = <&cp1_comphy5 2>;
        fixed-link {
                speed = <2500>;
                full-duplex;
        };
};

Should I drop fixed-link here as well?

The call to mv88e6341_port_set_cmode() introduced in commit 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family") still breaks port 5 (cpu) configuration. When called, its mode parameter is set to PHY_INTERFACE_MODE_2500BASEX (19).

Any idea?

Thanks,
baruch

--
http://baruch.siach.name/blog/ ~. .~ Tk Open
Systems =}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il
-

Quoted 16 lines · On Sun, 15 Dec 2019 12:13:25 +0200 Baruch Siach <[email protected]> wrote:

Thanks. That is enough to fix the phylink issue triggered by commit 7fb5a711545 ("net: dsa: mv88e6xxx: drop adjust_link to enabled phylink").

The Clearfog GT-8K DT has also this on the cpu side:

&cp1_eth2 {
        status = "okay";
        phy-mode = "2500base-x";
        phys = <&cp1_comphy5 2>;
        fixed-link {
                speed = <2500>;
                full-duplex;
        };
};

Quoted 1 line

Should I drop fixed-link here as well?

I would think yes. phy-mode = 2500base-x should already force 2500mbps, the fixed-link should be irrelevant. Whether this is truly the case I do not know, but on Turris Mox I do not use fixed-link with these.

Quoted 6 lines

The call to mv88e6341_port_set_cmode() introduced in commit 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family") still breaks port 5 (cpu) configuration. When called, its mode parameter is set to PHY_INTERFACE_MODE_2500BASEX (19).

Any idea?

I shall look into this. On Turris Mox this works, so I will have to do some experiments.

Quoted 16 lines · On Sun, 15 Dec 2019 12:13:25 +0200 Baruch Siach <[email protected]> wrote:

Thanks. That is enough to fix the phylink issue triggered by commit 7fb5a711545 ("net: dsa: mv88e6xxx: drop adjust_link to enabled phylink").

The Clearfog GT-8K DT has also this on the cpu side:

&cp1_eth2 {
        status = "okay";
        phy-mode = "2500base-x";
        phys = <&cp1_comphy5 2>;
        fixed-link {
                speed = <2500>;
                full-duplex;
        };
};

Quoted 1 line

Should I drop fixed-link here as well?

I would think yes. phy-mode = 2500base-x should already force 2500mbps, the fixed-link should be irrelevant. Whether this is truly the case I do not know, but on Turris Mox I do not use fixed-link with these.

Quoted 6 lines

The call to mv88e6341_port_set_cmode() introduced in commit 7a3007d22e8 ("net: dsa: mv88e6xxx: fully support SERDES on Topaz family") still breaks port 5 (cpu) configuration. When called, its mode parameter is set to PHY_INTERFACE_MODE_2500BASEX (19).

Any idea?

I shall look into this. On Turris Mox this works, so I will have to do some experiments.