[PATCH] net: mvpp2: add delay at the end of .mac_prepare

[PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

From: Baruch Siach <baruch.siach@siklu.com>

Without this delay PHY mode switch from XLG to SGMII fails in a weird
way. Rx side works. However, Tx appears to work as far as the MAC is
concerned, but packets don't show up on the wire.

Tested with Marvell 10G 88X3310 PHY.

Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
---

Not sure this is the right fix. Let me know if you have any better
suggestion for me to test.

The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 1a835b48791b..8823efe396b1 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
 		}
 	}
 
+	mdelay(10);
+
 	return 0;
 }
 
-- 
2.35.1

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Marcin Wojtas]

Hi Baruch,

śr., 27 kwi 2022 o 17:05 Baruch Siach <baruch@tkos.co.il> napisał(a):
>
> From: Baruch Siach <baruch.siach@siklu.com>
>
> Without this delay PHY mode switch from XLG to SGMII fails in a weird
> way. Rx side works. However, Tx appears to work as far as the MAC is
> concerned, but packets don't show up on the wire.
>
> Tested with Marvell 10G 88X3310 PHY.
>
> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> ---
>
> Not sure this is the right fix. Let me know if you have any better
> suggestion for me to test.
>
> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 1a835b48791b..8823efe396b1 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
>                 }
>         }
>
> +       mdelay(10);
> +
>         return 0;
>  }
>

Thank you for the patch and debug effort, however at first glance it
seems that adding delay may be a work-around and cover an actual root
cause (maybe Russell will have more input here).

Can you share exact reproduction steps?

Best regards,
Marcin

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

Hi Marcin,

On Thu, Apr 28 2022, Marcin Wojtas wrote:
> śr., 27 kwi 2022 o 17:05 Baruch Siach <baruch@tkos.co.il> napisał(a):
>>
>> From: Baruch Siach <baruch.siach@siklu.com>
>>
>> Without this delay PHY mode switch from XLG to SGMII fails in a weird
>> way. Rx side works. However, Tx appears to work as far as the MAC is
>> concerned, but packets don't show up on the wire.
>>
>> Tested with Marvell 10G 88X3310 PHY.
>>
>> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
>> ---
>>
>> Not sure this is the right fix. Let me know if you have any better
>> suggestion for me to test.
>>
>> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
>> ---
>>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>>n index 1a835b48791b..8823efe396b1 100644
>> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> @@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
>>                 }
>>         }
>>
>> +       mdelay(10);
>> +
>>         return 0;
>>  }
>
> Thank you for the patch and debug effort, however at first glance it
> seems that adding delay may be a work-around and cover an actual root
> cause (maybe Russell will have more input here).

That's my suspicion as well.

> Can you share exact reproduction steps?

I think I covered all relevant details. Is there anything you find
missing?

The hardware setup is very similar to the Macchiatobin Doubleshot. I can
try to reproduce on that platform next week if it helps.

The PHY MAC type (MV_V2_33X0_PORT_CTRL_MACTYPE_MASK) is set to
MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER.

I can add that DT phy-mode is set to "10gbase-kr" (equivalent to
"10gbase-r" in this case). The port cp0_eth0 is connected to a 1G
Ethernet switch. Kernel messages indicate that on interface up the MAC
is first configured to XLG (10G), but after Ethernet (wire)
auto-negotiation that MAC switches to SGMII. If I set DT phy-mode to
"sgmii" the issue does not show. Same if I make a down/up cycle of the
interface.

Thanks for your review.

baruch

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

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Jakub Kicinski]

On Wed, 27 Apr 2022 18:05:36 +0300 Baruch Siach wrote:
> From: Baruch Siach <baruch.siach@siklu.com>
> 
> Without this delay PHY mode switch from XLG to SGMII fails in a weird
> way. Rx side works. However, Tx appears to work as far as the MAC is
> concerned, but packets don't show up on the wire.
> 
> Tested with Marvell 10G 88X3310 PHY.
> 
> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> ---
> 
> Not sure this is the right fix. Let me know if you have any better
> suggestion for me to test.
> 
> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.

Let me mark it as RFC in patchwork, then. If the discussion concludes
with an approval please repost as [PATCH net] and preferably with a
Fixes tag; failing that a stable tag, since you indicate 5.10 needs it.

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Marcin Wojtas]

Hi Jakub,

czw., 28 kwi 2022 o 16:28 Jakub Kicinski <kuba@kernel.org> napisał(a):
>
> On Wed, 27 Apr 2022 18:05:36 +0300 Baruch Siach wrote:
> > From: Baruch Siach <baruch.siach@siklu.com>
> >
> > Without this delay PHY mode switch from XLG to SGMII fails in a weird
> > way. Rx side works. However, Tx appears to work as far as the MAC is
> > concerned, but packets don't show up on the wire.
> >
> > Tested with Marvell 10G 88X3310 PHY.
> >
> > Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> > ---
> >
> > Not sure this is the right fix. Let me know if you have any better
> > suggestion for me to test.
> >
> > The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
>
> Let me mark it as RFC in patchwork, then. If the discussion concludes
> with an approval please repost as [PATCH net] and preferably with a
> Fixes tag; failing that a stable tag, since you indicate 5.10 needs it.

Please do, that's a good idea.

Thanks,
Marcin

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Marcin Wojtas]

Hi Baruch,

czw., 28 kwi 2022 o 13:16 Baruch Siach <baruch@tkos.co.il> napisał(a):
>
> Hi Marcin,
>
> On Thu, Apr 28 2022, Marcin Wojtas wrote:
> > śr., 27 kwi 2022 o 17:05 Baruch Siach <baruch@tkos.co.il> napisał(a):
> >>
> >> From: Baruch Siach <baruch.siach@siklu.com>
> >>
> >> Without this delay PHY mode switch from XLG to SGMII fails in a weird
> >> way. Rx side works. However, Tx appears to work as far as the MAC is
> >> concerned, but packets don't show up on the wire.
> >>
> >> Tested with Marvell 10G 88X3310 PHY.
> >>
> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> >> ---
> >>
> >> Not sure this is the right fix. Let me know if you have any better
> >> suggestion for me to test.
> >>
> >> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
> >> ---
> >>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> >>n index 1a835b48791b..8823efe396b1 100644
> >> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> >> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> >> @@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
> >>                 }
> >>         }
> >>
> >> +       mdelay(10);
> >> +
> >>         return 0;
> >>  }
> >
> > Thank you for the patch and debug effort, however at first glance it
> > seems that adding delay may be a work-around and cover an actual root
> > cause (maybe Russell will have more input here).
>
> That's my suspicion as well.
>
> > Can you share exact reproduction steps?
>
> I think I covered all relevant details. Is there anything you find
> missing?
>
> The hardware setup is very similar to the Macchiatobin Doubleshot. I can
> try to reproduce on that platform next week if it helps.
>
> The PHY MAC type (MV_V2_33X0_PORT_CTRL_MACTYPE_MASK) is set to
> MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER.
>
> I can add that DT phy-mode is set to "10gbase-kr" (equivalent to
> "10gbase-r" in this case). The port cp0_eth0 is connected to a 1G
> Ethernet switch. Kernel messages indicate that on interface up the MAC
> is first configured to XLG (10G), but after Ethernet (wire)
> auto-negotiation that MAC switches to SGMII. If I set DT phy-mode to
> "sgmii" the issue does not show. Same if I make a down/up cycle of the
> interface.
>
> Thanks for your review.
>

I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
without your patch and the 3310 PHY is connected to 1G e1000 card.
After `ifconfig eth0 up` it properly drops to SGMII without any issue
in my setup:

# ifconfig eth0 up
[   62.006580] mvpp2 f2000000.ethernet eth0: PHY
[f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
[   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
# [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
- flow control rx/tx
[   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
# ifconfig eth0 192.168.1.1
# ping 192.168.1.2
PING 192.168.1.2 (192.168.1.2): 56 data bytes
64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms

Are you aware of the firmware version of the 3310 PHY in your setup?
In my case it's:
mv88x3310 f212a600.mdio-mii:00: Firmware version 0.3.3.0

Best regards,
Marcin

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

Hi Marcin,

On Thu, Apr 28 2022, Marcin Wojtas wrote:
> czw., 28 kwi 2022 o 13:16 Baruch Siach <baruch@tkos.co.il> napisał(a):
>> On Thu, Apr 28 2022, Marcin Wojtas wrote:
>> > śr., 27 kwi 2022 o 17:05 Baruch Siach <baruch@tkos.co.il> napisał(a):
>> >>
>> >> From: Baruch Siach <baruch.siach@siklu.com>
>> >>
>> >> Without this delay PHY mode switch from XLG to SGMII fails in a weird
>> >> way. Rx side works. However, Tx appears to work as far as the MAC is
>> >> concerned, but packets don't show up on the wire.
>> >>
>> >> Tested with Marvell 10G 88X3310 PHY.
>> >>
>> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
>> >> ---
>> >>
>> >> Not sure this is the right fix. Let me know if you have any better
>> >> suggestion for me to test.
>> >>
>> >> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
>> >> ---
>> >>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >>n index 1a835b48791b..8823efe396b1 100644
>> >> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >> @@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
>> >>                 }
>> >>         }
>> >>
>> >> +       mdelay(10);
>> >> +
>> >>         return 0;
>> >>  }
>> >
>> > Thank you for the patch and debug effort, however at first glance it
>> > seems that adding delay may be a work-around and cover an actual root
>> > cause (maybe Russell will have more input here).
>>
>> That's my suspicion as well.
>>
>> > Can you share exact reproduction steps?
>>
>> I think I covered all relevant details. Is there anything you find
>> missing?
>>
>> The hardware setup is very similar to the Macchiatobin Doubleshot. I can
>> try to reproduce on that platform next week if it helps.
>>
>> The PHY MAC type (MV_V2_33X0_PORT_CTRL_MACTYPE_MASK) is set to
>> MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER.
>>
>> I can add that DT phy-mode is set to "10gbase-kr" (equivalent to
>> "10gbase-r" in this case). The port cp0_eth0 is connected to a 1G
>> Ethernet switch. Kernel messages indicate that on interface up the MAC
>> is first configured to XLG (10G), but after Ethernet (wire)
>> auto-negotiation that MAC switches to SGMII. If I set DT phy-mode to
>> "sgmii" the issue does not show. Same if I make a down/up cycle of the
>> interface.
>>
>> Thanks for your review.
>
> I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
> without your patch and the 3310 PHY is connected to 1G e1000 card.
> After `ifconfig eth0 up` it properly drops to SGMII without any issue
> in my setup:
>
> # ifconfig eth0 up
> [   62.006580] mvpp2 f2000000.ethernet eth0: PHY
> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
> [   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
> # [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
> - flow control rx/tx
> [   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> # ifconfig eth0 192.168.1.1
> # ping 192.168.1.2
> PING 192.168.1.2 (192.168.1.2): 56 data bytes
> 64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
> 64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms

This is what I see here:

[   46.097184] mvpp2 f2000000.ethernet eth0: PHY [f212a600.mdio-mii:02] driver [mv88x3310] (irq=POLL)
[   46.115071] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
[   50.249513] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[   50.257539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

It is almost the same except from the link mode. Why does it try sgmii
even before auto-negotiation takes place?

> Are you aware of the firmware version of the 3310 PHY in your setup?
> In my case it's:
> mv88x3310 f212a600.mdio-mii:00: Firmware version 0.3.3.0

I have a newer version here:

mv88x3310 f212a600.mdio-mii:02: Firmware version 0.3.10.0

This is a timing sensitive issue. Slight change in firmware code might
be significant.

One more detail that might be important is that the PHY firmware is
loaded at run-time using this patch (rebased):

  https://lore.kernel.org/all/13177f5abf60215fb9c5c4251e6f487e4d0d7ff0.1587967848.git.baruch@tkos.co.il/

Thanks,
baruch

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

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

Hi Marcin,

On Thu, Apr 28 2022, Baruch Siach wrote:
> On Thu, Apr 28 2022, Marcin Wojtas wrote:
>> I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
>> without your patch and the 3310 PHY is connected to 1G e1000 card.
>> After `ifconfig eth0 up` it properly drops to SGMII without any issue
>> in my setup:
>>
>> # ifconfig eth0 up
>> [   62.006580] mvpp2 f2000000.ethernet eth0: PHY
>> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
>> [   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
>> # [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
>> - flow control rx/tx
>> [   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>> # ifconfig eth0 192.168.1.1
>> # ping 192.168.1.2
>> PING 192.168.1.2 (192.168.1.2): 56 data bytes
>> 64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
>> 64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms
>
> This is what I see here:
>
> [   46.097184] mvpp2 f2000000.ethernet eth0: PHY [f212a600.mdio-mii:02] driver [mv88x3310] (irq=POLL)
> [   46.115071] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
> [   50.249513] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> [   50.257539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>
> It is almost the same except from the link mode. Why does it try sgmii
> even before auto-negotiation takes place?

I have now tested on my Macchiatobin, and the issue does not
reproduce. PHY firmware version here:

[    1.074605] mv88x3310 f212a600.mdio-mii:00: Firmware version 0.2.1.0

But still I see that pl->link_config.interface is initially set to
PHY_INTERFACE_MODE_10GBASER:

[   13.518118] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode

This is set in phylink_create() based on DT phy-mode. After interface
down/up sequence pl->link_config.interface matches the 1G wire rate:

[   33.383971] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode

Do you have any idea where your initial PHY_INTERFACE_MODE_SGMII comes
from?

Thanks,
baruch

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

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Marcin Wojtas]

Hi Baruch,

niedz., 1 maj 2022 o 09:57 Baruch Siach <baruch@tkos.co.il> napisał(a):
>
> Hi Marcin,
>
> On Thu, Apr 28 2022, Baruch Siach wrote:
> > On Thu, Apr 28 2022, Marcin Wojtas wrote:
> >> I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
> >> without your patch and the 3310 PHY is connected to 1G e1000 card.
> >> After `ifconfig eth0 up` it properly drops to SGMII without any issue
> >> in my setup:
> >>
> >> # ifconfig eth0 up
> >> [   62.006580] mvpp2 f2000000.ethernet eth0: PHY
> >> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
> >> [   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
> >> # [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
> >> - flow control rx/tx
> >> [   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> >> # ifconfig eth0 192.168.1.1
> >> # ping 192.168.1.2
> >> PING 192.168.1.2 (192.168.1.2): 56 data bytes
> >> 64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
> >> 64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms
> >
> > This is what I see here:
> >
> > [   46.097184] mvpp2 f2000000.ethernet eth0: PHY [f212a600.mdio-mii:02] driver [mv88x3310] (irq=POLL)
> > [   46.115071] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
> > [   50.249513] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> > [   50.257539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> >
> > It is almost the same except from the link mode. Why does it try sgmii
> > even before auto-negotiation takes place?
>
> I have now tested on my Macchiatobin, and the issue does not
> reproduce. PHY firmware version here:
>
> [    1.074605] mv88x3310 f212a600.mdio-mii:00: Firmware version 0.2.1.0
>
> But still I see that pl->link_config.interface is initially set to
> PHY_INTERFACE_MODE_10GBASER:
>
> [   13.518118] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
>
> This is set in phylink_create() based on DT phy-mode. After interface
> down/up sequence pl->link_config.interface matches the 1G wire rate:
>
> [   33.383971] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
>
> Do you have any idea where your initial PHY_INTERFACE_MODE_SGMII comes
> from?
>

I have the same behavior, the link configured to 10GBASER and switches
to 1G by linux init scripts. When I do the first ifconfig up, it's
already at SGMII state:

# dmesg | grep eth1
[    2.071753] mvpp2 f2000000.ethernet eth1: Using random mac address
12:27:35:ff:2d:48
[    3.461338] mvpp2 f2000000.ethernet eth1: PHY
[f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
[    3.679714] mvpp2 f2000000.ethernet eth1: configuring for
phy/10gbase-r link mode
[    7.775483] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full -
flow control rx/tx
[    7.783455] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
[    8.801107] mvpp2 f2000000.ethernet eth1: Link is Down
# ifconfig eth1 up
[   37.498617] mvpp2 f2000000.ethernet eth1: PHY
[f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
[   37.508812] mvpp2 f2000000.ethernet eth1: configuring for phy/sgmii link mode
[   41.598331] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full -
flow control rx/tx
[   41.606309] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
#

Best regards,
Marcin

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

Hi Marcin,

On Sun, May 01 2022, Marcin Wojtas wrote:
> niedz., 1 maj 2022 o 09:57 Baruch Siach <baruch@tkos.co.il> napisał(a):
>> On Thu, Apr 28 2022, Baruch Siach wrote:
>> > On Thu, Apr 28 2022, Marcin Wojtas wrote:
>> >> I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
>> >> without your patch and the 3310 PHY is connected to 1G e1000 card.
>> >> After `ifconfig eth0 up` it properly drops to SGMII without any issue
>> >> in my setup:
>> >>
>> >> # ifconfig eth0 up
>> >> [   62.006580] mvpp2 f2000000.ethernet eth0: PHY
>> >> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
>> >> [   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
>> >> # [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
>> >> - flow control rx/tx
>> >> [   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>> >> # ifconfig eth0 192.168.1.1
>> >> # ping 192.168.1.2
>> >> PING 192.168.1.2 (192.168.1.2): 56 data bytes
>> >> 64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
>> >> 64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms
>> >
>> > This is what I see here:
>> >
>> > [   46.097184] mvpp2 f2000000.ethernet eth0: PHY [f212a600.mdio-mii:02] driver [mv88x3310] (irq=POLL)
>> > [   46.115071] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
>> > [   50.249513] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
>> > [   50.257539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>> >
>> > It is almost the same except from the link mode. Why does it try sgmii
>> > even before auto-negotiation takes place?
>>
>> I have now tested on my Macchiatobin, and the issue does not
>> reproduce. PHY firmware version here:
>>
>> [    1.074605] mv88x3310 f212a600.mdio-mii:00: Firmware version 0.2.1.0
>>
>> But still I see that pl->link_config.interface is initially set to
>> PHY_INTERFACE_MODE_10GBASER:
>>
>> [   13.518118] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
>>
>> This is set in phylink_create() based on DT phy-mode. After interface
>> down/up sequence pl->link_config.interface matches the 1G wire rate:
>>
>> [   33.383971] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
>>
>> Do you have any idea where your initial PHY_INTERFACE_MODE_SGMII comes
>> from?
>>
>
> I have the same behavior, the link configured to 10GBASER and switches
> to 1G by linux init scripts. When I do the first ifconfig up, it's
> already at SGMII state:
>
> # dmesg | grep eth1
> [    2.071753] mvpp2 f2000000.ethernet eth1: Using random mac address
> 12:27:35:ff:2d:48
> [    3.461338] mvpp2 f2000000.ethernet eth1: PHY
> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
> [    3.679714] mvpp2 f2000000.ethernet eth1: configuring for
> phy/10gbase-r link mode
> [    7.775483] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full -
> flow control rx/tx
> [    7.783455] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
> [    8.801107] mvpp2 f2000000.ethernet eth1: Link is Down
> # ifconfig eth1 up
> [   37.498617] mvpp2 f2000000.ethernet eth1: PHY
> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
> [   37.508812] mvpp2 f2000000.ethernet eth1: configuring for phy/sgmii link mode
> [   41.598331] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full -
> flow control rx/tx
> [   41.606309] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready

No wonder why you don't see this issue. Interface down/up sequence is
one of the "fixes" I tested.

Does it work for you if you remove the init script on first interface
up?

Thanks for testing,
baruch

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

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Russell King (Oracle)]

On Wed, Apr 27, 2022 at 06:05:36PM +0300, Baruch Siach wrote:
> From: Baruch Siach <baruch.siach@siklu.com>
> 
> Without this delay PHY mode switch from XLG to SGMII fails in a weird
> way. Rx side works. However, Tx appears to work as far as the MAC is
> concerned, but packets don't show up on the wire.
> 
> Tested with Marvell 10G 88X3310 PHY.

Which firmware do you have running on the 88x3310 PHY? Early versions
are buggy when switching between modes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Russell King (Oracle)]

On Thu, Apr 28, 2022 at 10:59:59AM +0200, Marcin Wojtas wrote:
> Hi Baruch,
> 
> Thank you for the patch and debug effort, however at first glance it
> seems that adding delay may be a work-around and cover an actual root
> cause (maybe Russell will have more input here).

Please note I'm on a boat suffering diesel bug (means I'm running off
limited battery power which has to last until Wednesday for more vital
services, so I'm only around sporadically as the situation permits.
It's basically exactly an Apollo 13 situation - everything's turned
off except when absolutely required!)

Early revisions of the 88x3310 firmware have problems when switching
between the different MAC-side link modes. When this occurs, the PHY
firmware flashes one (iirc yellow) LED rapidly and refuses to link.

The answer is not to add random delays to the MAC driver, but get
the firmware of the MAC updated to something way less buggy - and
those early firmwares do contain a lot of bugs.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Russell King (Oracle)]

On Thu, Apr 28, 2022 at 01:59:55PM +0300, Baruch Siach wrote:
> Hi Marcin,
> 
> I can add that DT phy-mode is set to "10gbase-kr" (equivalent to
> "10gbase-r" in this case).

Please do not use 10gbase-kr - this is for backplane support, and was an
early mistake with the 88x3310 nand Armada 8040. Both behave compatibly
with it for older DT, and that's only what it's there for. You should be
specifying "10gbase-r" in DT. Please update your DT.

> The port cp0_eth0 is connected to a 1G
> Ethernet switch. Kernel messages indicate that on interface up the MAC
> is first configured to XLG (10G), but after Ethernet (wire)
> auto-negotiation that MAC switches to SGMII. If I set DT phy-mode to
> "sgmii" the issue does not show. Same if I make a down/up cycle of the
> interface.

Sounds like old PHY firmware.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Russell King (Oracle)]

Please trim so I don't have to waste my vital limited power reading
paging through lots of unnecessary text. Thanks.

On Thu, Apr 28, 2022 at 11:03:08PM +0300, Baruch Siach wrote:
> mv88x3310 f212a600.mdio-mii:02: Firmware version 0.3.10.0
> 
> This is a timing sensitive issue. Slight change in firmware code might
> be significant.

That should be new enough to avoid the firmware problem - and it seems
that 0.3.10.0 works fine on the Macchiatobin DS.

> One more detail that might be important is that the PHY firmware is
> loaded at run-time using this patch (rebased):
> 
>   https://lore.kernel.org/all/13177f5abf60215fb9c5c4251e6f487e4d0d7ff0.1587967848.git.baruch@tkos.co.il/

Hmm, I wonder what difference that makes...

Can you confirm the md5sum of your firmware please?
95180414ba23f2c7c2fabd377fb4c1df ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

[Baruch Siach]

Hi Russell,

On Sun, May 01 2022, Russell King (Oracle) wrote:
> Please trim so I don't have to waste my vital limited power reading
> paging through lots of unnecessary text. Thanks.
>
> On Thu, Apr 28, 2022 at 11:03:08PM +0300, Baruch Siach wrote:
>> mv88x3310 f212a600.mdio-mii:02: Firmware version 0.3.10.0
>> 
>> This is a timing sensitive issue. Slight change in firmware code might
>> be significant.
>
> That should be new enough to avoid the firmware problem - and it seems
> that 0.3.10.0 works fine on the Macchiatobin DS.
>
>> One more detail that might be important is that the PHY firmware is
>> loaded at run-time using this patch (rebased):
>> 
>>   https://lore.kernel.org/all/13177f5abf60215fb9c5c4251e6f487e4d0d7ff0.1587967848.git.baruch@tkos.co.il/
>
> Hmm, I wonder what difference that makes...
>
> Can you confirm the md5sum of your firmware please?
> 95180414ba23f2c7c2fabd377fb4c1df ?

Precisely:

$ md5sum x3310fw_0_3_10_0_10860.hdr 
95180414ba23f2c7c2fabd377fb4c1df  x3310fw_0_3_10_0_10860.hdr

Thanks,
baruch

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