[PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

Commit d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary
 on mac_config") disabled auto negotiation bypass completely, which breaks
platforms enabling bypass via firmware (not the best option, but it worked).
Since 1000BaseX/2500BaseX ports neither negotiate speed nor duplex mode
we could enable auto negotiation bypass to get back information about link
state.

Fixes: d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary on mac_config")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 2b5dad2ec650..ddcd781052e1 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5043,6 +5043,7 @@ static void mvpp2_gmac_config(struct mvpp2_port *port, unsigned int mode,
 			MVPP2_GMAC_CONFIG_GMII_SPEED |
 			MVPP2_GMAC_CONFIG_FULL_DUPLEX);
 		an |= MVPP2_GMAC_IN_BAND_AUTONEG |
+		      MVPP2_GMAC_IN_BAND_AUTONEG_BYPASS |
 		      MVPP2_GMAC_CONFIG_GMII_SPEED |
 		      MVPP2_GMAC_CONFIG_FULL_DUPLEX;
 
-- 
2.16.4

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Thu, May 28, 2020 at 02:11:21PM +0200, Thomas Bogendoerfer wrote:
> Commit d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary
>  on mac_config") disabled auto negotiation bypass completely, which breaks
> platforms enabling bypass via firmware (not the best option, but it worked).
> Since 1000BaseX/2500BaseX ports neither negotiate speed nor duplex mode
> we could enable auto negotiation bypass to get back information about link
> state.

Thanks, but your commit is missing some useful information.

Which platforms have broken?

Can you describe the situation where you require this bit to be set?

We should not be enabling bypass mode as a matter of course, it exists
to work around broken setups which do not send the control word.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Thu, 28 May 2020 14:07:38 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, May 28, 2020 at 02:11:21PM +0200, Thomas Bogendoerfer wrote:
> > Commit d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary
> >  on mac_config") disabled auto negotiation bypass completely, which breaks
> > platforms enabling bypass via firmware (not the best option, but it worked).
> > Since 1000BaseX/2500BaseX ports neither negotiate speed nor duplex mode
> > we could enable auto negotiation bypass to get back information about link
> > state.
> 
> Thanks, but your commit is missing some useful information.
> 
> Which platforms have broken?

it's an Ambedded MARS-400
 
> Can you describe the situation where you require this bit to be set?

as I have no exact design details I'm just talking about what I can see
on that platform. It looks like the switch connecting the internal nodes
doesn't run autoneg on the internal links. So the link to the internal
nodes will never come up. These links are running 2500BaseX so speed/duplex
is clean and by enabling bypass I'll get a proper link state, too.

> We should not be enabling bypass mode as a matter of course, it exists
> to work around broken setups which do not send the control word.

if you call it a broken setup I'm fine, but this doesn't solve the problem,
which exists now. What would be your solution ?

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Thu, May 28, 2020 at 03:17:33PM +0200, Thomas Bogendoerfer wrote:
> On Thu, 28 May 2020 14:07:38 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Thu, May 28, 2020 at 02:11:21PM +0200, Thomas Bogendoerfer wrote:
> > > Commit d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary
> > >  on mac_config") disabled auto negotiation bypass completely, which breaks
> > > platforms enabling bypass via firmware (not the best option, but it worked).
> > > Since 1000BaseX/2500BaseX ports neither negotiate speed nor duplex mode
> > > we could enable auto negotiation bypass to get back information about link
> > > state.
> > 
> > Thanks, but your commit is missing some useful information.
> > 
> > Which platforms have broken?
> 
> it's an Ambedded MARS-400
>  
> > Can you describe the situation where you require this bit to be set?
> 
> as I have no exact design details I'm just talking about what I can see
> on that platform. It looks like the switch connecting the internal nodes
> doesn't run autoneg on the internal links. So the link to the internal
> nodes will never come up. These links are running 2500BaseX so speed/duplex
> is clean and by enabling bypass I'll get a proper link state, too.
> 
> > We should not be enabling bypass mode as a matter of course, it exists
> > to work around broken setups which do not send the control word.
> 
> if you call it a broken setup I'm fine, but this doesn't solve the problem,
> which exists now. What would be your solution ?

What I was after was additional information about the problem, so
that we can start thinking about how to deal with the AN bypass bit
in a sensible way.

How is the connection between the switch and network interface
described?  I don't think I see a .dts file in mainline for this
platform.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Thu, 28 May 2020 14:56:08 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, May 28, 2020 at 03:17:33PM +0200, Thomas Bogendoerfer wrote:
> > On Thu, 28 May 2020 14:07:38 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > 
> > > On Thu, May 28, 2020 at 02:11:21PM +0200, Thomas Bogendoerfer wrote:
> > > > Commit d14e078f23cc ("net: marvell: mvpp2: only reprogram what is necessary
> > > >  on mac_config") disabled auto negotiation bypass completely, which breaks
> > > > platforms enabling bypass via firmware (not the best option, but it worked).
> > > > Since 1000BaseX/2500BaseX ports neither negotiate speed nor duplex mode
> > > > we could enable auto negotiation bypass to get back information about link
> > > > state.
> > > 
> > > Thanks, but your commit is missing some useful information.
> > > 
> > > Which platforms have broken?
> > 
> > it's an Ambedded MARS-400
> >  
> > > Can you describe the situation where you require this bit to be set?
> > 
> > as I have no exact design details I'm just talking about what I can see
> > on that platform. It looks like the switch connecting the internal nodes
> > doesn't run autoneg on the internal links. So the link to the internal
> > nodes will never come up. These links are running 2500BaseX so speed/duplex
> > is clean and by enabling bypass I'll get a proper link state, too.
> > 
> > > We should not be enabling bypass mode as a matter of course, it exists
> > > to work around broken setups which do not send the control word.
> > 
> > if you call it a broken setup I'm fine, but this doesn't solve the problem,
> > which exists now. What would be your solution ?
> 
> What I was after was additional information about the problem, so
> that we can start thinking about how to deal with the AN bypass bit
> in a sensible way.
> 
> How is the connection between the switch and network interface
> described?  I don't think I see a .dts file in mainline for this
> platform.

below is the dts part for the two network interfaces. The switch to
the outside has two ports, which correlate to the two internal ports.
And the switch propagates the link state of the external ports to
the internal ports.

Thomas.

&cp0_eth1 {
        status = "okay";
        phy-mode = "2500base-x";
        mac-address = [00 00 00 00 00 01];
        interrupts = <41 IRQ_TYPE_LEVEL_HIGH>,
        <45 IRQ_TYPE_LEVEL_HIGH>,
        <49 IRQ_TYPE_LEVEL_HIGH>,
        <53 IRQ_TYPE_LEVEL_HIGH>,
        <57 IRQ_TYPE_LEVEL_HIGH>,
        <61 IRQ_TYPE_LEVEL_HIGH>,
        <65 IRQ_TYPE_LEVEL_HIGH>,
        <69 IRQ_TYPE_LEVEL_HIGH>,
        <73 IRQ_TYPE_LEVEL_HIGH>,
        <127 IRQ_TYPE_LEVEL_HIGH>;
        interrupt-names = "hif0", "hif1", "hif2",
        "hif3", "hif4", "hif5", "hif6", "hif7",
        "hif8", "link";
        port-id = <2>;
        gop-port-id = <3>;
        managed = "in-band-status";
};

&cp0_eth2 {
        status = "okay";
        phy-mode = "2500base-x";
        mac-address = [00 00 00 00 00 02];
        interrupts = <40 IRQ_TYPE_LEVEL_HIGH>,
        <44 IRQ_TYPE_LEVEL_HIGH>,
        <48 IRQ_TYPE_LEVEL_HIGH>,
        <52 IRQ_TYPE_LEVEL_HIGH>,
        <56 IRQ_TYPE_LEVEL_HIGH>,
        <60 IRQ_TYPE_LEVEL_HIGH>,
        <64 IRQ_TYPE_LEVEL_HIGH>,
        <68 IRQ_TYPE_LEVEL_HIGH>,
        <72 IRQ_TYPE_LEVEL_HIGH>,
        <128 IRQ_TYPE_LEVEL_HIGH>;
        interrupt-names = "hif0", "hif1", "hif2",
        "hif3", "hif4", "hif5", "hif6", "hif7",
        "hif8", "link";
        port-id = <1>;
        gop-port-id = <2>;
        managed = "in-band-status";
};


-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> below is the dts part for the two network interfaces. The switch to
> the outside has two ports, which correlate to the two internal ports.
> And the switch propagates the link state of the external ports to
> the internal ports.

Okay, so this DTS hasn't been reviewed...

> &cp0_eth1 {
>         status = "okay";
>         phy-mode = "2500base-x";
>         mac-address = [00 00 00 00 00 01];
>         interrupts = <41 IRQ_TYPE_LEVEL_HIGH>,
>         <45 IRQ_TYPE_LEVEL_HIGH>,
>         <49 IRQ_TYPE_LEVEL_HIGH>,
>         <53 IRQ_TYPE_LEVEL_HIGH>,
>         <57 IRQ_TYPE_LEVEL_HIGH>,
>         <61 IRQ_TYPE_LEVEL_HIGH>,
>         <65 IRQ_TYPE_LEVEL_HIGH>,
>         <69 IRQ_TYPE_LEVEL_HIGH>,
>         <73 IRQ_TYPE_LEVEL_HIGH>,
>         <127 IRQ_TYPE_LEVEL_HIGH>;
>         interrupt-names = "hif0", "hif1", "hif2",
>         "hif3", "hif4", "hif5", "hif6", "hif7",
>         "hif8", "link";
>         port-id = <2>;
>         gop-port-id = <3>;

This seems to correlate with the eth2 node in armada-cp11x.dtsi -
you do not need to specify the interrupts, interrupt-names, port-id
and gop-port-id unless you need to change them just because you want
to add a few properties to this node - you can just list the new or
altered properties.  To delete a property, you need to prefix the
property name with /delete-property/.

>         managed = "in-band-status";

This isn't correct - you are requesting that in-band status is used
(i.o.w. the in-band control word, see commit 4cba5c210365), but your
bug report wants to enable AN bypass because there is no in-band
control word.  This seems to be rather contradictory.

May I suggest you use a fixed-link here, which will not have any
inband status, as there is no in-band control word being sent by
the switch?  That is also the conventional way of handling switch
links.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Thu, 28 May 2020 15:48:05 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> > below is the dts part for the two network interfaces. The switch to
> > the outside has two ports, which correlate to the two internal ports.
> > And the switch propagates the link state of the external ports to
> > the internal ports.
> 
> Okay, so this DTS hasn't been reviewed...

that's from our partner, I'm just using it. Stripping it down isn't
the point for my now.

> This isn't correct - you are requesting that in-band status is used
> (i.o.w. the in-band control word, see commit 4cba5c210365), but your
> bug report wants to enable AN bypass because there is no in-band
> control word.  This seems to be rather contradictory.
> 
> May I suggest you use a fixed-link here, which will not have any

afaik fixed-link will always be up, and we want to have the link state
from the switch external ports.

> inband status, as there is no in-band control word being sent by
> the switch?  That is also the conventional way of handling switch
> links.

again, we want to propagte the external link state inside to all
the internal nodes. So this will not work anymore with fixed-link.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

On Thu, May 28, 2020 at 08:43:12PM +0200, Thomas Bogendoerfer wrote:
> On Thu, 28 May 2020 15:48:05 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> > > below is the dts part for the two network interfaces. The switch to
> > > the outside has two ports, which correlate to the two internal ports.
> > > And the switch propagates the link state of the external ports to
> > > the internal ports.
> > 
> > Okay, so this DTS hasn't been reviewed...
> 
> that's from our partner, I'm just using it. Stripping it down isn't
> the point for my now.
> 
> > This isn't correct - you are requesting that in-band status is used
> > (i.o.w. the in-band control word, see commit 4cba5c210365), but your
> > bug report wants to enable AN bypass because there is no in-band
> > control word.  This seems to be rather contradictory.
> > 
> > May I suggest you use a fixed-link here, which will not have any
> 
> afaik fixed-link will always be up, and we want to have the link state
> from the switch external ports.
> 
> > inband status, as there is no in-band control word being sent by
> > the switch?  That is also the conventional way of handling switch
> > links.
> 
> again, we want to propagte the external link state inside to all
> the internal nodes. So this will not work anymore with fixed-link.

Just for my understanding...

https://www.ambedded.com/ARM_Server_platform.html

seems to suggest there are 4 external ports. You want to pass the link
status of these four external ports to the CPU module?

	Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Thu, May 28, 2020 at 08:43:12PM +0200, Thomas Bogendoerfer wrote:
> On Thu, 28 May 2020 15:48:05 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> > > below is the dts part for the two network interfaces. The switch to
> > > the outside has two ports, which correlate to the two internal ports.
> > > And the switch propagates the link state of the external ports to
> > > the internal ports.
> > 
> > Okay, so this DTS hasn't been reviewed...
> 
> that's from our partner, I'm just using it. Stripping it down isn't
> the point for my now.
> 
> > This isn't correct - you are requesting that in-band status is used
> > (i.o.w. the in-band control word, see commit 4cba5c210365), but your
> > bug report wants to enable AN bypass because there is no in-band
> > control word.  This seems to be rather contradictory.
> > 
> > May I suggest you use a fixed-link here, which will not have any
> 
> afaik fixed-link will always be up, and we want to have the link state
> from the switch external ports.
> 
> > inband status, as there is no in-band control word being sent by
> > the switch?  That is also the conventional way of handling switch
> > links.
> 
> again, we want to propagte the external link state inside to all
> the internal nodes. So this will not work anymore with fixed-link.

Can you explain this please?  Just as we think we understand what's
going on here, you throw in a new comment that makes us confused.

You said previously that the mvpp2 was connected to a switch, which
makes us think that you've got some DSA-like setup going on here.
Does your switch drop its serdes link when all the external links
(presumably the 10G SFP+ cages) fail?

Both Andrew and myself wish to have a complete picture before we
move forward with this.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Thu, 28 May 2020 23:04:20 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> Can you explain this please?  Just as we think we understand what's
> going on here, you throw in a new comment that makes us confused.

sorry about that.

> You said previously that the mvpp2 was connected to a switch, which
> makes us think that you've got some DSA-like setup going on here.
> Does your switch drop its serdes link when all the external links
> (presumably the 10G SFP+ cages) fail?
> 
> Both Andrew and myself wish to have a complete picture before we
> move forward with this.

full understandable, I'll try by a small picture, which just
covers one switch:

        external ports
      |  |          |  |
*-----------------------------*
|     1  1          2  2      |
|                             |
|           switch            |
|                             |
|   1   2            1   2    |
*-----------------------------*
    |   |            |   |
    |   |            |   |
*----------*     *----------*
|   1   2  |     |   1   2  |
|          |     |          |
|  node 1  | ... |  node 8  |
|          |     |          |
*----------*     *----------*

External ports a grouped in ports to network 1 and network 2. If one of the
external ports has an established link, this link state will be propagated
to the internal ports. Same when both external ports of a network are down.

I have no control over the software running on the switch, therefore I can't
enable autoneg on the internal links. The internal nodes are running our
enterprise distribution and everything is working with SLE15SP1. With 
SLE15SP2 we've moved to kernel 5.3 and are now facing the problem, that links
on der internal nodes aren't coming up anymore.

I hope this makes things a little bit clearer.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

On Fri, May 29, 2020 at 01:05:39PM +0200, Thomas Bogendoerfer wrote:
> On Thu, 28 May 2020 23:04:20 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > Can you explain this please?  Just as we think we understand what's
> > going on here, you throw in a new comment that makes us confused.
> 
> sorry about that.
> 
> > You said previously that the mvpp2 was connected to a switch, which
> > makes us think that you've got some DSA-like setup going on here.
> > Does your switch drop its serdes link when all the external links
> > (presumably the 10G SFP+ cages) fail?
> > 
> > Both Andrew and myself wish to have a complete picture before we
> > move forward with this.
> 
> full understandable, I'll try by a small picture, which just
> covers one switch:
> 
>         external ports
>       |  |          |  |
> *-----------------------------*
> |     1  1          2  2      |
> |                             |
> |           switch            |
> |                             |
> |   1   2            1   2    |
> *-----------------------------*
>     |   |            |   |
>     |   |            |   |
> *----------*     *----------*
> |   1   2  |     |   1   2  |
> |          |     |          |
> |  node 1  | ... |  node 8  |
> |          |     |          |
> *----------*     *----------*
> 
> External ports a grouped in ports to network 1 and network 2. If one of the
> external ports has an established link, this link state will be propagated
> to the internal ports. Same when both external ports of a network are down.

By propagated, you mean if the external link is down, the link between
the switch and node 1 will also be forced down, at the SERDES level?
And if external ports are down, the nodes cannot talk to each other?
External link down causes the whole in box network to fall apart? That
seems a rather odd design.

> I have no control over the software running on the switch, therefore I can't
> enable autoneg on the internal links.

O.K. So that means using in-band signalling in DT is clearly
wrong. There is no signalling....

What you are actually interested in is the sync state of the SERDES?
The link is up if the SERDES has sync.

    Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Fri, May 29, 2020 at 04:59:28PM +0200, Andrew Lunn wrote:
> On Fri, May 29, 2020 at 01:05:39PM +0200, Thomas Bogendoerfer wrote:
> > On Thu, 28 May 2020 23:04:20 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > 
> > > Can you explain this please?  Just as we think we understand what's
> > > going on here, you throw in a new comment that makes us confused.
> > 
> > sorry about that.
> > 
> > > You said previously that the mvpp2 was connected to a switch, which
> > > makes us think that you've got some DSA-like setup going on here.
> > > Does your switch drop its serdes link when all the external links
> > > (presumably the 10G SFP+ cages) fail?
> > > 
> > > Both Andrew and myself wish to have a complete picture before we
> > > move forward with this.
> > 
> > full understandable, I'll try by a small picture, which just
> > covers one switch:
> > 
> >         external ports
> >       |  |          |  |
> > *-----------------------------*
> > |     1  1          2  2      |
> > |                             |
> > |           switch            |
> > |                             |
> > |   1   2            1   2    |
> > *-----------------------------*
> >     |   |            |   |
> >     |   |            |   |
> > *----------*     *----------*
> > |   1   2  |     |   1   2  |
> > |          |     |          |
> > |  node 1  | ... |  node 8  |
> > |          |     |          |
> > *----------*     *----------*
> > 
> > External ports a grouped in ports to network 1 and network 2. If one of the
> > external ports has an established link, this link state will be propagated
> > to the internal ports. Same when both external ports of a network are down.
> 
> By propagated, you mean if the external link is down, the link between
> the switch and node 1 will also be forced down, at the SERDES level?
> And if external ports are down, the nodes cannot talk to each other?
> External link down causes the whole in box network to fall apart? That
> seems a rather odd design.
> 
> > I have no control over the software running on the switch, therefore I can't
> > enable autoneg on the internal links.
> 
> O.K. So that means using in-band signalling in DT is clearly
> wrong. There is no signalling....
> 
> What you are actually interested in is the sync state of the SERDES?
> The link is up if the SERDES has sync.

Right now we force the link up/down for fixed-link mode, depending on
the state that phylink is given (either via the callback, or the gpio,
or in the absence of either of those, we assume link up.)

In the case of a serdes link, where we also have the sync state to
consider, that doesn't sound sane - if in fixed link mode, phylink is
told that the link is up, yet we can clearly see that the serdes we
are attached to is not, we aren't going to receive anything and
anything we try to transmit isn't going to go anywhere.

I wonder how much risk there is to changing that, so we force the link
down if phylink says the link should be down, otherwise we force the
speed/duplex, disable AN, and allow the link to come up depending on
the serdes status.  It /sounds/ like something sane to do.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

> I wonder how much risk there is to changing that, so we force the link
> down if phylink says the link should be down, otherwise we force the
> speed/duplex, disable AN, and allow the link to come up depending on
> the serdes status.  It /sounds/ like something sane to do.

Hi Russell

I actually did this for mv88e6xxx in a patchset for ZII devel B. It
was determining link based on SFP LOS, which we know is unreliable. It
said there was link even when the SERDES had lost link.

I did it by making use of the fixed-link state call back, since it was
a quick and dirty patch. But it might make more sense for the MAC to
call phylink_mac_change() for change in PCS state? Or add a PCS
specific.

	Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Fri, May 29, 2020 at 06:25:04PM +0200, Andrew Lunn wrote:
> > I wonder how much risk there is to changing that, so we force the link
> > down if phylink says the link should be down, otherwise we force the
> > speed/duplex, disable AN, and allow the link to come up depending on
> > the serdes status.  It /sounds/ like something sane to do.
> 
> Hi Russell
> 
> I actually did this for mv88e6xxx in a patchset for ZII devel B. It
> was determining link based on SFP LOS, which we know is unreliable. It
> said there was link even when the SERDES had lost link.
> 
> I did it by making use of the fixed-link state call back, since it was
> a quick and dirty patch. But it might make more sense for the MAC to
> call phylink_mac_change() for change in PCS state? Or add a PCS
> specific.

Correct.  In the early phylink versions, I did have sync state reported
from the PCS back to phylink, but as phylink didn't make use of it, I
removed it as it wasn't obvious how it should be used.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Fri, 29 May 2020 16:59:28 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, May 29, 2020 at 01:05:39PM +0200, Thomas Bogendoerfer wrote:
> > On Thu, 28 May 2020 23:04:20 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > 
> > > Can you explain this please?  Just as we think we understand what's
> > > going on here, you throw in a new comment that makes us confused.
> > 
> > sorry about that.
> > 
> > > You said previously that the mvpp2 was connected to a switch, which
> > > makes us think that you've got some DSA-like setup going on here.
> > > Does your switch drop its serdes link when all the external links
> > > (presumably the 10G SFP+ cages) fail?
> > > 
> > > Both Andrew and myself wish to have a complete picture before we
> > > move forward with this.
> > 
> > full understandable, I'll try by a small picture, which just
> > covers one switch:
> > 
> >         external ports
> >       |  |          |  |
> > *-----------------------------*
> > |     1  1          2  2      |
> > |                             |
> > |           switch            |
> > |                             |
> > |   1   2            1   2    |
> > *-----------------------------*
> >     |   |            |   |
> >     |   |            |   |
> > *----------*     *----------*
> > |   1   2  |     |   1   2  |
> > |          |     |          |
> > |  node 1  | ... |  node 8  |
> > |          |     |          |
> > *----------*     *----------*
> > 
> > External ports a grouped in ports to network 1 and network 2. If one of the
> > external ports has an established link, this link state will be propagated
> > to the internal ports. Same when both external ports of a network are down.
> 
> By propagated, you mean if the external link is down, the link between
> the switch and node 1 will also be forced down, at the SERDES level?

yes

> And if external ports are down, the nodes cannot talk to each other?

correct

> External link down causes the whole in box network to fall apart? That
> seems a rather odd design.

as I'm not an expert in ceph, I can't judge. But I'll bring it up.

> > I have no control over the software running on the switch, therefore I can't
> > enable autoneg on the internal links.
> 
> O.K. So that means using in-band signalling in DT is clearly
> wrong. There is no signalling....
> 
> What you are actually interested in is the sync state of the SERDES?
> The link is up if the SERDES has sync.

yes, that's what I need. How can I do that ?

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

> > By propagated, you mean if the external link is down, the link between
> > the switch and node 1 will also be forced down, at the SERDES level?
> 
> yes
> 
> > And if external ports are down, the nodes cannot talk to each other?
> 
> correct
> 
> > External link down causes the whole in box network to fall apart? That
> > seems a rather odd design.
> 
> as I'm not an expert in ceph, I can't judge. But I'll bring it up.

I guess for a single use appliance this is O.K. But it makes the
hardware unusable as a general purpose server.

Is there a variant of the hardware to be used as a general purpose
server, rather than as a Ceph appliance? If so, does it share the same
DT files?

> > What you are actually interested in is the sync state of the SERDES?
> > The link is up if the SERDES has sync.
> 
> yes, that's what I need. How can I do that ?

Given the current code, you cannot. Now we understand the
requirements, we can come up with some ideas how to do this properly.

      Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Fri, 29 May 2020 18:33:40 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > By propagated, you mean if the external link is down, the link between
> > > the switch and node 1 will also be forced down, at the SERDES level?
> > 
> > yes
> > 
> > > And if external ports are down, the nodes cannot talk to each other?
> > 
> > correct
> > 
> > > External link down causes the whole in box network to fall apart? That
> > > seems a rather odd design.
> > 
> > as I'm not an expert in ceph, I can't judge. But I'll bring it up.
> 
> I guess for a single use appliance this is O.K. But it makes the
> hardware unusable as a general purpose server.
> 
> Is there a variant of the hardware to be used as a general purpose
> server, rather than as a Ceph appliance? If so, does it share the same
> DT files?

I don't know of any, but it would just need some way to change the switch
configuration. So it's independent from linux running on the nodes.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Fri, May 29, 2020 at 06:33:40PM +0200, Andrew Lunn wrote:
> Given the current code, you cannot. Now we understand the
> requirements, we can come up with some ideas how to do this properly.

Okay, I've been a little quiet because of sorting out the ARM tree
for merging with Linus (now done) and I've been working on a solution
to this problem.

The good news is, I have an implementation in phylink to use the sync
status reported from a PCS, and to appropriately enable sync status
reporting.  I'm quite nervous about having that enabled as a matter of
routine as I've seen some Marvell hardware end up with interrupt storms
from it - presumably due to noise pickup on the serdes lines being
interpreted as an intermittently valid signal.

I have mvneta using it, and partially tested on the SolidRun Clearfog
platform; for testing, I've stripped out everything for the SFP and
replaced it with a fixed link - that way, I can control whether the
serdes is in sync or not. It isn't mainline quality as I need to work
out how to properly handle the MVNETA_INTR_MISC_MASK which looks to me
like it's needlessly written in multiple places in the driver.

I have a partial implementation on mvpp2, but not complete yet, that
is to come - it will need mvpp2 converted to the new phylink pcs_ops
which has yet to happen, and may take a bit of time to sort.

So, some progress towards a solution, but not to the point where I'd
be happy to post some patches just yet.

However, I think we need to think about:
1) how we classify Thomas' problem - does it count as a regression
   given that support for his platform is not part of mainline, and
   the use of in-band-status in his unreviewed DT is clearly incorrect?

2) if we deem it to be a regression, then how do we intend to solve
   this for stable kernels?

3) re-enabling AN bypass for mvpp2 would create inconsistencies
   between different drivers for similar hardware from the same
   manufacturer, so should we propagate the "fix" to them as
   well (e.g. mvneta.)

4) what about when we have a proper solution to this, what do we
   then do with mvpp2 if we decide to change it's behaviour, and
   do we then recommend that Thomas switches to using this (I
   suppose so, otherwise there's not much point me developing a
   solution to this problem.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Tue, Jun 02, 2020 at 11:50:16PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, May 29, 2020 at 06:33:40PM +0200, Andrew Lunn wrote:
> > Given the current code, you cannot. Now we understand the
> > requirements, we can come up with some ideas how to do this properly.
> 
> Okay, I've been a little quiet because of sorting out the ARM tree
> for merging with Linus (now done) and I've been working on a solution
> to this problem.
> 
> The good news is, I have an implementation in phylink to use the sync
> status reported from a PCS, and to appropriately enable sync status
> reporting.  I'm quite nervous about having that enabled as a matter of
> routine as I've seen some Marvell hardware end up with interrupt storms
> from it - presumably due to noise pickup on the serdes lines being
> interpreted as an intermittently valid signal.

Yes, as expected - though not quite a storm - I'm seeing:

[root@buildroot ~]# dmesg |grep 'eno2:' | wc -l
1604
[root@buildroot ~]# dmesg |grep 'eno2: mac link down' | wc -l
1598
[root@buildroot ~]# cat /proc/uptime
68868.10 137231.62

Similar happens with mvpp2 hardware - for Marvell's older mvpp2x driver,
I had to disable the AN bypass bit:

    net: marvell: mvpp2x: avoid link status flood

    eth2 on the Macchiatobin board floods the system with link status
    interrupts whilethe link is down.  This appears to be caused by the
    AN bypass logic causing spurious link status change interrupts,
    despite the port status register indicating that the link remains
    down.

    Avoid this by not setting the AN bypass bit for SGMII links.

    Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

So, it looks like a different approach will be needed, since having a
system flooded with unnecessary interrupts is obviously bad.  This
isn't limited to just Marvell mvneta and mvpp2, I think Chris at ZII
has reported a similar behaviour on his boards with noise inducing
serdes sync/link events.

Hmm.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

On Tue, Jun 02, 2020 at 11:50:17PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, May 29, 2020 at 06:33:40PM +0200, Andrew Lunn wrote:
> > Given the current code, you cannot. Now we understand the
> > requirements, we can come up with some ideas how to do this properly.
> 
> Okay, I've been a little quiet because of sorting out the ARM tree
> for merging with Linus (now done) and I've been working on a solution
> to this problem.
> 
> The good news is, I have an implementation in phylink to use the sync
> status reported from a PCS, and to appropriately enable sync status
> reporting.  I'm quite nervous about having that enabled as a matter of
> routine as I've seen some Marvell hardware end up with interrupt storms
> from it - presumably due to noise pickup on the serdes lines being
> interpreted as an intermittently valid signal.

Hi Russell

I have seen similar with an SFP without link. I think squelch is
optional, so noise gets passed through, which is enough to get and
loose sync.

I think we probably need to only enable the interrupt when the LOS
signal indicates there is at least some power coming into the SFP.

> However, I think we need to think about:
> 1) how we classify Thomas' problem - does it count as a regression
>    given that support for his platform is not part of mainline, and
>    the use of in-band-status in his unreviewed DT is clearly incorrect?

I would say no, it is not a regression.

> 2) if we deem it to be a regression, then how do we intend to solve
>    this for stable kernels?

I think this new code should go into net-next, when it opens. I
suspect it is going to be a big change, once we consider LOS.

	Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Russell King - ARM Linux admin]

On Wed, Jun 03, 2020 at 03:21:47PM +0200, Andrew Lunn wrote:
> On Tue, Jun 02, 2020 at 11:50:17PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, May 29, 2020 at 06:33:40PM +0200, Andrew Lunn wrote:
> > > Given the current code, you cannot. Now we understand the
> > > requirements, we can come up with some ideas how to do this properly.
> > 
> > Okay, I've been a little quiet because of sorting out the ARM tree
> > for merging with Linus (now done) and I've been working on a solution
> > to this problem.
> > 
> > The good news is, I have an implementation in phylink to use the sync
> > status reported from a PCS, and to appropriately enable sync status
> > reporting.  I'm quite nervous about having that enabled as a matter of
> > routine as I've seen some Marvell hardware end up with interrupt storms
> > from it - presumably due to noise pickup on the serdes lines being
> > interpreted as an intermittently valid signal.
> 
> Hi Russell
> 
> I have seen similar with an SFP without link. I think squelch is
> optional, so noise gets passed through, which is enough to get and
> loose sync.
> 
> I think we probably need to only enable the interrupt when the LOS
> signal indicates there is at least some power coming into the SFP.

This doesn't help in Thomas' case, there is no LOS signal.  I was
proposing that Thomas uses fixed-link to describe the setup (because
that's exactly what it is) but use the serdes sync status as an
additional gate for "link up".

Given that we have platforms that hammer the CPU with interrupts
when AN bypass is enabled, I think that setup is also a non-starter
for the mainline kernel without some way for firmware to tell the
kernel that it's okay to use it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Andrew Lunn]

On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> below is the dts part for the two network interfaces. The switch to
> the outside has two ports, which correlate to the two internal ports.
> And the switch propagates the link state of the external ports to
> the internal ports.

Hi Thomas

Any plans to add mainline support for this board?
Contribute the DT files?

    Andrew

Re: [PATCH net] net: mvpp2: Enable autoneg bypass for 1000BaseX/2500BaseX ports

[Thomas Bogendoerfer]

On Thu, 28 May 2020 18:33:27 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, May 28, 2020 at 04:33:35PM +0200, Thomas Bogendoerfer wrote:
> > below is the dts part for the two network interfaces. The switch to
> > the outside has two ports, which correlate to the two internal ports.
> > And the switch propagates the link state of the external ports to
> > the internal ports.
> 
> Hi Thomas
> 
> Any plans to add mainline support for this board?
> Contribute the DT files?

I'll ask our partner, if they are interested in upstreaming it.

Thomas.

-- 
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Felix Imendörffer