[BUG] mv88e6xxx: tx regression in v5.3

[BUG] mv88e6xxx: tx regression in v5.3

[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

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

Do you have any idea?

Thanks,
baruch

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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

On Wed, Dec 11, 2019 at 11:48:57AM +0200, Baruch Siach wrote:
> 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.

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.

      Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Wed, Dec 11 2019, Andrew Lunn wrote:

> On Wed, Dec 11, 2019 at 11:48:57AM +0200, Baruch Siach wrote:
>> 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.
>
> 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?

100Mbps switch is what I have at the other side of the link. Works
perfectly with v5.2.

> 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?

Thanks,
baruch

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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> 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.

	Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Wed, Dec 11, 2019 at 06:49:38PM +0100, Andrew Lunn wrote:
> > 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.

I reproduced the same issue with 1Gb link as well:

[   63.510782] mv88e6085 f412a200.mdio-mii:04 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
[   63.519357] IPv6: ADDRCONF(NETDEV_CHANGE): lan1: link becomes ready

> 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?

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> 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 <marex@denx.de>
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.

@@ -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.

	    Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andreas Tobler]

On 12.12.19 14:14, Andrew Lunn wrote:
>> 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 <marex@denx.de>
> 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.
> 
> @@ -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.

Interesting that you mention the patch above.

I jump in because I have an issue since the above patch went in.

In my case (see below) the links are up and running fine, but when I 
disconnect the lan on port 8 or 9, I do not get the IP back. The same 
applies if I set the device down and up again. I loose the ip and do not 
get it again.

If I set the link in the above 'else if' clause to LINK_UNFORCED, I get 
my ip's back and the links are working again.

         } else if (!mv88e6xxx_phy_is_internal(ds, port)) {
-               link = state->link;
+               link = LINK_UNFORCED;
                 speed = state->speed;

I hope I did not hijack the thread an my situation is related. Otherwise 
please let me know.

Thanks,
Andreas

The lan0/1/2 are 100M, lan3/4 are 1G

my switch info from the log, 5.4.2:

mv88e6085 f1072004.mdio-mii:00: switch 0x990 detected: Marvell 
88E6097/88E6097F, revision 2
libphy: mv88e6xxx SMI: probed
mv88e6085 f1072004.mdio-mii:00 lan0 (uninitialized): PHY 
[mv88e6xxx-1:05] driver [Generic PHY]
mv88e6085 f1072004.mdio-mii:00 lan1 (uninitialized): PHY 
[mv88e6xxx-1:06] driver [Generic PHY]
mv88e6085 f1072004.mdio-mii:00 lan2 (uninitialized): PHY 
[mv88e6xxx-1:07] driver [Generic PHY]
mv88e6085 f1072004.mdio-mii:00 lan3 (uninitialized): PHY 
[mv88e6xxx-1:08] driver [Marvell 88E1112]
mv88e6085 f1072004.mdio-mii:00 lan4 (uninitialized): PHY 
[mv88e6xxx-1:09] driver [Marvell 88E1112]




-- 
onway ag
Andreas Tobler
Software Engineer

Stauffacherstrasse 16, CH-8004 Zürich
Tel: +41 55 214 18 42
andreas.tobler@onway.ch
www.onway.ch

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Thu, Dec 12, 2019 at 02:14:48PM +0100, Andrew Lunn wrote:
> > 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 <marex@denx.de>
> 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.
> 
> @@ -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:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d0a97eb73a37..f0457274b5a9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -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? Or maybe mv88e6xxx_port_setup_mac() should be 
called again when the cpu interface goes up?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> 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:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index d0a97eb73a37..f0457274b5a9 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -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?

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.

Thanks
	Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Thu, Dec 12, 2019 at 04:13:55PM +0100, Andrew Lunn wrote:
> > 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:
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index d0a97eb73a37..f0457274b5a9 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -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?
> 
> 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.

So maybe this:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d0a97eb73a37..84ca4f36a778 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew, Marek,

On Thu, Dec 12, 2019 at 05:23:57PM +0200, Baruch Siach wrote:
> On Thu, Dec 12, 2019 at 04:13:55PM +0100, Andrew Lunn wrote:
> > > 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:
> > > 
> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > index d0a97eb73a37..f0457274b5a9 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -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?
> > 
> > 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.
> 
> So maybe this:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index d0a97eb73a37..84ca4f36a778 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -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;

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:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7b5b73499e37..e7e6400a994e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Marek Behun]

On Thu, 12 Dec 2019 20:17:29 +0200
Baruch Siach <baruch@tkos.co.il> 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:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 7b5b73499e37..e7e6400a994e 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -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
> 

Hi Baruch,

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?

Marek

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Marek Behun]

On Thu, 12 Dec 2019 17:23:55 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Andrew,
> 
> On Thu, Dec 12, 2019 at 04:13:55PM +0100, Andrew Lunn wrote:
> > > 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:
> > > 
> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > index d0a97eb73a37..f0457274b5a9 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -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?  
> > 
> > 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.  
> 
> So maybe this:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index d0a97eb73a37..84ca4f36a778 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -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
> 

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.

Marek

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Thu, Dec 12, 2019 at 07:32:18PM +0100, Marek Behun wrote:
> On Thu, 12 Dec 2019 20:17:29 +0200
> Baruch Siach <baruch@tkos.co.il> 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:
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index 7b5b73499e37..e7e6400a994e 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -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?
> 
> 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?

This is indeed port 5. It is connected to the cpu (Armada 8040). PHY mode is 
19 (PHY_INTERFACE_MODE_2500BASEX).

I have now tested v5.5-rc1 which behaves just like v5.4. The same "fix" works 
with v5.5-rc1.

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Thu, Dec 12, 2019 at 07:36:11PM +0100, Marek Behun wrote:
> On Thu, 12 Dec 2019 17:23:55 +0200
> Baruch Siach <baruch@tkos.co.il> wrote:
> > 
> > On Thu, Dec 12, 2019 at 04:13:55PM +0100, Andrew Lunn wrote:
> > > > 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:
> > > > 
> > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > index d0a97eb73a37..f0457274b5a9 100644
> > > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > @@ -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?  
> > > 
> > > 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.  
> > 
> > So maybe this:
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index d0a97eb73a37..84ca4f36a778 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -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;
> 
> 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.

Thanks for the clarification. This also works without the hunk above:

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index bd881497b872..8f61cae9d3b0 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
@@ -408,6 +408,11 @@ port@5 {
 				reg = <5>;
 				label = "cpu";
 				ethernet = <&cp1_eth2>;
+
+				fixed-link {
+					speed = <2500>;
+					full-duplex;
+				};
 			};
 		};
 
With this I get calls to mv88e6341_port_set_cmode() with PHY mode 0. So the 
other "fix" is still needed.

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> index bd881497b872..8f61cae9d3b0 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> @@ -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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Marek Behun]

On Thu, 12 Dec 2019 20:31:29 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> > index bd881497b872..8f61cae9d3b0 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> > @@ -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;

Marek

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> 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;

Yah, Ports 9 and 10 of 6390X are a bit odd. They can do 10Gbps. But
only if you set the correct phy-mode. Then they will default to 10G.
If these ports are not doing 10Gbps they can lend there SERDES
interfaces to other ports. There are a few boards which want this
lending, connecting lots of SFPs using these SERDES interfaces. And
there are other boards which do use the ports at 10G as DSA links. DSA
links also default to the maximum speed of the port, and that does
work if you set the correct phy-mode.

So in general, if the port supports > 1Gbps, you need to set the
phy-mode for CPU and DSA ports. It will then default to the maximum
speed for that mode.

     Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Thu, Dec 12 2019, Marek Behun wrote:

> On Thu, 12 Dec 2019 20:31:29 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
>
>> > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
>> > index bd881497b872..8f61cae9d3b0 100644
>> > --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
>> > +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
>> > @@ -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.
>
> 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";
> };

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------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Sun, Dec 15 2019, Baruch Siach wrote:
> 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 dug a little further here. It turns out that
mv88e6xxx_port_set_cmode() does not do any register write, because it
exits early here with cmode == 0xb:

        /* cmode doesn't change, nothing to do for us */
        if (cmode == chip->ports[port].cmode)
                return 0;

mv88e6341_port_set_cmode_writable() breaks the port configuration with
its call to mv88e6xxx_port_hidden_write(). Before this call
mv88e6352_port_get_cmode() sets cmode to 0xb. After this call cmode is
6. This breaks the assumption that the equality test above relies on.

This fixes cpu port configuration for me:

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 7fe256c5739d..a6c320978bcf 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -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?

Thanks,
baruch

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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

> This fixes cpu port configuration for me:
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index 7fe256c5739d..a6c320978bcf 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -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?

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.

      Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Sun, Dec 15 2019, Andrew Lunn wrote:
>> This fixes cpu port configuration for me:
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> index 7fe256c5739d..a6c320978bcf 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -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?
>
> 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().

What do you think?

Thanks,
baruch

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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Andrew Lunn]

On Sun, Dec 15, 2019 at 05:08:01PM +0200, Baruch Siach wrote:
> Hi Andrew,
> 
> On Sun, Dec 15 2019, Andrew Lunn wrote:
> >> This fixes cpu port configuration for me:
> >>
> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> >> index 7fe256c5739d..a6c320978bcf 100644
> >> --- a/drivers/net/dsa/mv88e6xxx/port.c
> >> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> >> @@ -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?
> >
> > 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().

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
		Andrew

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Andrew,

On Sun, Dec 15, 2019 at 05:14:23PM +0100, Andrew Lunn wrote:
> On Sun, Dec 15, 2019 at 05:08:01PM +0200, Baruch Siach wrote:
> > On Sun, Dec 15 2019, Andrew Lunn wrote:
> > >> This fixes cpu port configuration for me:
> > >>
> > >> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> > >> index 7fe256c5739d..a6c320978bcf 100644
> > >> --- a/drivers/net/dsa/mv88e6xxx/port.c
> > >> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> > >> @@ -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?
> > >
> > > 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().
> 
> 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.

This turned out to be much harder than expected. cmode update after 
mv88e6xxx_port_hidden_write() breaks the serdes configuration. The link change 
irq does not trigger on serdes interface up. The strange thing is that if I 
add 10ms delay after cmode read (or anywhere before the mv88e6xxx_port_write() 
call in mv88e6xxx_port_set_cmode()) it works again. I have no idea what is 
going on here. The cmode cache is used in many places, so maybe setting it to 
an invalid value (6) is not a good idea.

I ended up with cmode write force in mv88e6341_port_set_cmode_writable() like 
this:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 8a8e38bfb161..70284f100d87 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -233,6 +233,7 @@ struct mv88e6xxx_port {
 	u64 vtu_member_violation;
 	u64 vtu_miss_violation;
 	u8 cmode;
+	bool force_cmode;
 	bool mirror_ingress;
 	bool mirror_egress;
 	unsigned int serdes_irq;
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 7fe256c5739d..8e8724eb5669 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -427,9 +427,10 @@ 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)
+	/* cmode doesn't change, nothing to do for us unless forced */
+	if (cmode == chip->ports[port].cmode && !chip->ports[port].force_cmode)
 		return 0;
+	chip->ports[port].force_cmode = false;
 
 	lane = mv88e6xxx_serdes_get_lane(chip, port);
 	if (lane) {
@@ -516,6 +517,8 @@ static int mv88e6341_port_set_cmode_writable(struct mv88e6xxx_chip *chip,
 	if (port != 5)
 		return -EOPNOTSUPP;
 
+	chip->ports[port].force_cmode = true;
+
 	addr = chip->info->port_base_addr + port;
 
 	err = mv88e6xxx_port_hidden_read(chip, 0x7, addr, 0, &reg);

Does that look right?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Marek Behún]

Hi Baruch,

On Sun, 15 Dec 2019 12:13:25 +0200
Baruch Siach <baruch@tkos.co.il> 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;
>         };
> };
> 
> 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.

> 
> 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.

> Thanks,
> baruch
> 
> --
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open
> Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il
> -

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Wed, Dec 18 2019, Marek Behún wrote:
> On Sun, 15 Dec 2019 12:13:25 +0200
> Baruch Siach <baruch@tkos.co.il> 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;
>>         };
>> };
>> 
>> 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.

OK. I'll test that.

>> 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.

I have made some progress on this issue. See my other messages on this
thread. My latest suggestion is here:

  https://lore.kernel.org/netdev/20191217182643.augknhx57pafnelv@sapphire.tkos.co.il/

I now think that the force_cmode field is not necessary. We should just
add a 'force' parameter to mv88e6xxx_port_set_cmode(), and set it true
in mv88e6341_port_set_cmode().

baruch

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

Re: [BUG] mv88e6xxx: tx regression in v5.3

[Baruch Siach]

Hi Marek,

On Wed, Dec 18, 2019 at 03:30:35PM +0100, Marek Behún wrote:
> On Sun, 15 Dec 2019 12:13:25 +0200
> Baruch Siach <baruch@tkos.co.il> 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;
> >         };
> > };
> > 
> > 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.

It doesn't work here without fixed-link. The link state remains down.

baruch

> > 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.

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -