net: dsa: mv88e6xxx: prevent 2500BASEX mode override

Commit Message

Nathan Rossi Feb. 15, 2021, 6:15 a.m. UTC

From: Nathan Rossi <[e-mail]>

The mv88e6xxx devices cannot automatically select between 1000BASE-X and
2500BASE-X and rely on configuration (gpio pins Px_SMODE/S_MODE and/or
OF phy-mode) to select between the two modes.

However when configuring a cpu/dsa port as 1000BASE-X with a inband or
fixed link phy the mode is always overridden to 2500BASE-X by the call of
phylink_helper_basex_speed in phylink_validate due to the order of setup
with respect to advertised modes and auto negotiation being enabled.

During the initial setup of the phy the phy-mode property defined for
the port is configured before any calls to phylink_validate. The first
call to phylink_validate sets the advertised modes to all valid modes
and phylink_validate masks to supported modes, for the ports that
support 1000BASE-X/2500BASE-X both are advertised. At this stage the
speed is not yet configured and the phylink_helper_basex_speed function
overrides the mode to 2500BASE-X due to all modes being advertised and
auto negotiation being enabled. After the speed is configured
phylink_validate is called again, the same logic applies and the mode is
set to 2500BASE-X (due to auto negotiation).

As such it is not possible for a fixed link to be configured as
1000BASE-X, as the mode cannot be configured (e.g. via phy-mode
property) and the link cannot be automatically selected as 1000BASE-X.

This change prevents the advertising of 2500BASE-X when the port is
already configured for 1000BASE-X, which in turn prevents the
phylink_helper_basex_speed from always overriding to 2500BASE-X. This
allows for the mode to correctly propagate from the phy-mode property to
the port configuration.

Signed-off-by: Nathan Rossi <[e-mail]>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 7 +++++++
 1 file changed, 7 insertions(+)

---
2.30.0

Comments

Marek Behún Feb. 15, 2021, 1:47 p.m. UTC | #1

On Mon, 15 Feb 2021 06:15:59 +0000
Nathan Rossi <[e-mail]> wrote:

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 54aa942eed..5c52906b29 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
>  	if (chip->info->ops->phylink_validate)
>  		chip->info->ops->phylink_validate(chip, port, mask, state);
>  
> +	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
> +	 * prevents phylink_helper_basex_speed from always overriding the
> +	 * 1000BASEX mode since auto negotiation is always enabled.
> +	 */
> +	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> +		phylink_clear(mask, 2500baseX_Full);
> +

I don't quite like this. This problem should be either solved in
phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
the call to phylink_helper_basex_speed().

Putting a solution to the behaviour of phylink_helper_basex_speed() it
into the validate() method when phylink_helper_basex_speed() is called
from a different place will complicate debugging in the future. If
we start solving problems in this kind of way, the driver will become
totally unreadable, IMO.

Marek

Russell King (Oracle) Feb. 15, 2021, 2:57 p.m. UTC | #2

On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:
> On Mon, 15 Feb 2021 06:15:59 +0000
> Nathan Rossi <[e-mail]> wrote:
> 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index 54aa942eed..5c52906b29 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> >  	if (chip->info->ops->phylink_validate)
> >  		chip->info->ops->phylink_validate(chip, port, mask, state);
> >  
> > +	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
> > +	 * prevents phylink_helper_basex_speed from always overriding the
> > +	 * 1000BASEX mode since auto negotiation is always enabled.
> > +	 */
> > +	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > +		phylink_clear(mask, 2500baseX_Full);
> > +
> 
> I don't quite like this. This problem should be either solved in
> phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
> the call to phylink_helper_basex_speed().
> 
> Putting a solution to the behaviour of phylink_helper_basex_speed() it
> into the validate() method when phylink_helper_basex_speed() is called
> from a different place will complicate debugging in the future. If
> we start solving problems in this kind of way, the driver will become
> totally unreadable, IMO.

If we can't switch between 1000base-X and 2500base-X, then we should
not be calling phylink_helper_basex_speed() - and only one of those
two capabilities should be set in the validation callback. I thought
there were DSA switches where we could program the CMODE to switch
between these two...

Andrew Lunn Feb. 15, 2021, 3:11 p.m. UTC | #3

> If we can't switch between 1000base-X and 2500base-X, then we should
> not be calling phylink_helper_basex_speed() - and only one of those
> two capabilities should be set in the validation callback. I thought
> there were DSA switches where we could program the CMODE to switch
> between these two...

6390 family has a writable CMODE for ports 9 and 10, and you can
select various SERDES modes.

Nathan, what device are you using?

	Andrew

Marek Behún Feb. 15, 2021, 3:16 p.m. UTC | #4

On Mon, 15 Feb 2021 14:57:57 +0000
Russell King - ARM Linux admin <[e-mail]> wrote:

> On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:
> > On Mon, 15 Feb 2021 06:15:59 +0000
> > Nathan Rossi <[e-mail]> wrote:
> >   
> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > index 54aa942eed..5c52906b29 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> > >  	if (chip->info->ops->phylink_validate)
> > >  		chip->info->ops->phylink_validate(chip, port, mask, state);
> > >  
> > > +	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
> > > +	 * prevents phylink_helper_basex_speed from always overriding the
> > > +	 * 1000BASEX mode since auto negotiation is always enabled.
> > > +	 */
> > > +	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > > +		phylink_clear(mask, 2500baseX_Full);
> > > +  
> > 
> > I don't quite like this. This problem should be either solved in
> > phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
> > the call to phylink_helper_basex_speed().
> > 
> > Putting a solution to the behaviour of phylink_helper_basex_speed() it
> > into the validate() method when phylink_helper_basex_speed() is called
> > from a different place will complicate debugging in the future. If
> > we start solving problems in this kind of way, the driver will become
> > totally unreadable, IMO.  
> 
> If we can't switch between 1000base-X and 2500base-X, then we should
> not be calling phylink_helper_basex_speed() - and only one of those
> two capabilities should be set in the validation callback. I thought
> there were DSA switches where we could program the CMODE to switch
> between these two...

There are. At least Peridot, Topaz and Amethyst support switching
between these modes. But only on some ports.

This problem happnes on Peridot X, I think.

On Peridot X there are
- port 0: RGMII
- ports 9-10: capable of 1, 2.5 and 10G SerDes (10G via
  XAUI/RXAUI, so multiple lanes)
- ports 1-8: with copper PHYs
  - some of these can instead be set to use the unused SerDes lanes
    of ports 9-10, but only in 1000base-x mode

So the problem can happen if you set port 9 or 10 to only use one
SerDes lane, and use the spare lanes for the 1G ports.
On these ports 2500base-x is not supported, only 1000base-x (maybe
sgmii, I don't remember)

Marek

Russell King (Oracle) Feb. 15, 2021, 3:29 p.m. UTC | #5

On Mon, Feb 15, 2021 at 04:16:27PM +0100, Marek Behun wrote:
> On Mon, 15 Feb 2021 14:57:57 +0000
> Russell King - ARM Linux admin <[e-mail]> wrote:
> 
> > On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:
> > > On Mon, 15 Feb 2021 06:15:59 +0000
> > > Nathan Rossi <[e-mail]> wrote:
> > >   
> > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > index 54aa942eed..5c52906b29 100644
> > > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> > > >  	if (chip->info->ops->phylink_validate)
> > > >  		chip->info->ops->phylink_validate(chip, port, mask, state);
> > > >  
> > > > +	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
> > > > +	 * prevents phylink_helper_basex_speed from always overriding the
> > > > +	 * 1000BASEX mode since auto negotiation is always enabled.
> > > > +	 */
> > > > +	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > > > +		phylink_clear(mask, 2500baseX_Full);
> > > > +  
> > > 
> > > I don't quite like this. This problem should be either solved in
> > > phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
> > > the call to phylink_helper_basex_speed().
> > > 
> > > Putting a solution to the behaviour of phylink_helper_basex_speed() it
> > > into the validate() method when phylink_helper_basex_speed() is called
> > > from a different place will complicate debugging in the future. If
> > > we start solving problems in this kind of way, the driver will become
> > > totally unreadable, IMO.  
> > 
> > If we can't switch between 1000base-X and 2500base-X, then we should
> > not be calling phylink_helper_basex_speed() - and only one of those
> > two capabilities should be set in the validation callback. I thought
> > there were DSA switches where we could program the CMODE to switch
> > between these two...
> 
> There are. At least Peridot, Topaz and Amethyst support switching
> between these modes. But only on some ports.
> 
> This problem happnes on Peridot X, I think.
> 
> On Peridot X there are
> - port 0: RGMII
> - ports 9-10: capable of 1, 2.5 and 10G SerDes (10G via
>   XAUI/RXAUI, so multiple lanes)
> - ports 1-8: with copper PHYs
>   - some of these can instead be set to use the unused SerDes lanes
>     of ports 9-10, but only in 1000base-x mode
> 
> So the problem can happen if you set port 9 or 10 to only use one
> SerDes lane, and use the spare lanes for the 1G ports.
> On these ports 2500base-x is not supported, only 1000base-x (maybe
> sgmii, I don't remember)

It sounds like the modes are not reporting correctly then before calling
phylink_helper_basex_speed(). If the port can only be used at 1000base-X,
then it should not be allowing 2500base-X to be set prior to calling
phylink_helper_basex_speed().

Marek Behún Feb. 15, 2021, 3:58 p.m. UTC | #6

On Mon, 15 Feb 2021 15:29:44 +0000
Russell King - ARM Linux admin <[e-mail]> wrote:

> On Mon, Feb 15, 2021 at 04:16:27PM +0100, Marek Behun wrote:
> > On Mon, 15 Feb 2021 14:57:57 +0000
> > Russell King - ARM Linux admin <[e-mail]> wrote:
> >   
> > > On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:  
> > > > On Mon, 15 Feb 2021 06:15:59 +0000
> > > > Nathan Rossi <[e-mail]> wrote:
> > > >     
> > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > index 54aa942eed..5c52906b29 100644
> > > > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> > > > >  	if (chip->info->ops->phylink_validate)
> > > > >  		chip->info->ops->phylink_validate(chip, port, mask, state);
> > > > >  
> > > > > +	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
> > > > > +	 * prevents phylink_helper_basex_speed from always overriding the
> > > > > +	 * 1000BASEX mode since auto negotiation is always enabled.
> > > > > +	 */
> > > > > +	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > > > > +		phylink_clear(mask, 2500baseX_Full);
> > > > > +    
> > > > 
> > > > I don't quite like this. This problem should be either solved in
> > > > phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
> > > > the call to phylink_helper_basex_speed().
> > > > 
> > > > Putting a solution to the behaviour of phylink_helper_basex_speed() it
> > > > into the validate() method when phylink_helper_basex_speed() is called
> > > > from a different place will complicate debugging in the future. If
> > > > we start solving problems in this kind of way, the driver will become
> > > > totally unreadable, IMO.    
> > > 
> > > If we can't switch between 1000base-X and 2500base-X, then we should
> > > not be calling phylink_helper_basex_speed() - and only one of those
> > > two capabilities should be set in the validation callback. I thought
> > > there were DSA switches where we could program the CMODE to switch
> > > between these two...  
> > 
> > There are. At least Peridot, Topaz and Amethyst support switching
> > between these modes. But only on some ports.
> > 
> > This problem happnes on Peridot X, I think.
> > 
> > On Peridot X there are
> > - port 0: RGMII
> > - ports 9-10: capable of 1, 2.5 and 10G SerDes (10G via
> >   XAUI/RXAUI, so multiple lanes)
> > - ports 1-8: with copper PHYs
> >   - some of these can instead be set to use the unused SerDes lanes
> >     of ports 9-10, but only in 1000base-x mode
> > 
> > So the problem can happen if you set port 9 or 10 to only use one
> > SerDes lane, and use the spare lanes for the 1G ports.
> > On these ports 2500base-x is not supported, only 1000base-x (maybe
> > sgmii, I don't remember)  
> 
> It sounds like the modes are not reporting correctly then before calling
> phylink_helper_basex_speed(). If the port can only be used at 1000base-X,
> then it should not be allowing 2500base-X to be set prior to calling
> phylink_helper_basex_speed().
> 

Hmm. It doesn't seem that way. Ports 1-8 only set support for 1000baseT
and 1000baseX. And for lower modes if state->interface is not 8023z.

Nathan, what switch do you use and on which port does this happen?

Marek

Nathan Rossi Feb. 16, 2021, 8 a.m. UTC | #7

On Tue, 16 Feb 2021 at 01:58, Marek Behun <[e-mail]> wrote:
>
> On Mon, 15 Feb 2021 15:29:44 +0000
> Russell King - ARM Linux admin <[e-mail]> wrote:
>
> > On Mon, Feb 15, 2021 at 04:16:27PM +0100, Marek Behun wrote:
> > > On Mon, 15 Feb 2021 14:57:57 +0000
> > > Russell King - ARM Linux admin <[e-mail]> wrote:
> > >
> > > > On Mon, Feb 15, 2021 at 02:47:56PM +0100, Marek Behun wrote:
> > > > > On Mon, 15 Feb 2021 06:15:59 +0000
> > > > > Nathan Rossi <[e-mail]> wrote:
> > > > >
> > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > > index 54aa942eed..5c52906b29 100644
> > > > > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > > > > @@ -650,6 +650,13 @@ static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
> > > > > >       if (chip->info->ops->phylink_validate)
> > > > > >               chip->info->ops->phylink_validate(chip, port, mask, state);
> > > > > >
> > > > > > +     /* Advertise 2500BASEX only if 1000BASEX is not configured, this
> > > > > > +      * prevents phylink_helper_basex_speed from always overriding the
> > > > > > +      * 1000BASEX mode since auto negotiation is always enabled.
> > > > > > +      */
> > > > > > +     if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> > > > > > +             phylink_clear(mask, 2500baseX_Full);
> > > > > > +
> > > > >
> > > > > I don't quite like this. This problem should be either solved in
> > > > > phylink_helper_basex_speed() or somewhere in the mv88e6xxx code, but near
> > > > > the call to phylink_helper_basex_speed().
> > > > >
> > > > > Putting a solution to the behaviour of phylink_helper_basex_speed() it
> > > > > into the validate() method when phylink_helper_basex_speed() is called
> > > > > from a different place will complicate debugging in the future. If
> > > > > we start solving problems in this kind of way, the driver will become
> > > > > totally unreadable, IMO.
> > > >
> > > > If we can't switch between 1000base-X and 2500base-X, then we should
> > > > not be calling phylink_helper_basex_speed() - and only one of those
> > > > two capabilities should be set in the validation callback. I thought
> > > > there were DSA switches where we could program the CMODE to switch
> > > > between these two...
> > >
> > > There are. At least Peridot, Topaz and Amethyst support switching
> > > between these modes. But only on some ports.
> > >
> > > This problem happnes on Peridot X, I think.
> > >
> > > On Peridot X there are
> > > - port 0: RGMII
> > > - ports 9-10: capable of 1, 2.5 and 10G SerDes (10G via
> > >   XAUI/RXAUI, so multiple lanes)
> > > - ports 1-8: with copper PHYs
> > >   - some of these can instead be set to use the unused SerDes lanes
> > >     of ports 9-10, but only in 1000base-x mode
> > >
> > > So the problem can happen if you set port 9 or 10 to only use one
> > > SerDes lane, and use the spare lanes for the 1G ports.
> > > On these ports 2500base-x is not supported, only 1000base-x (maybe
> > > sgmii, I don't remember)
> >
> > It sounds like the modes are not reporting correctly then before calling
> > phylink_helper_basex_speed(). If the port can only be used at 1000base-X,
> > then it should not be allowing 2500base-X to be set prior to calling
> > phylink_helper_basex_speed().
> >
>
> Hmm. It doesn't seem that way. Ports 1-8 only set support for 1000baseT
> and 1000baseX. And for lower modes if state->interface is not 8023z.
>
> Nathan, what switch do you use and on which port does this happen?
>

After looking at this issue again in more depth, it seems I had
managed to confuse myself with the uplinks and their actual modes.
Specifically the fixed link uplink was being configured as a
1000base-x but should have been configured as sgmii phy-mode in the
device tree. This resolves the issues associated with auto negotiation
(not enabled for sgmii), on 88e6390 (port 9 uplink) and 88e6193x (port
0 uplink) switches.

Sorry for the noise, and any confusion. However it should be noted
that the issue mentioned in this patch might still be applicable if
fixed 1000base-x links are expected to work.

Thanks,
Nathan

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 54aa942eed..5c52906b29 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -650,6 +650,13 @@  static void mv88e6xxx_validate(struct dsa_switch *ds, int port,
 	if (chip->info->ops->phylink_validate)
 		chip->info->ops->phylink_validate(chip, port, mask, state);
 
+	/* Advertise 2500BASEX only if 1000BASEX is not configured, this
+	 * prevents phylink_helper_basex_speed from always overriding the
+	 * 1000BASEX mode since auto negotiation is always enabled.
+	 */
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_clear(mask, 2500baseX_Full);
+
 	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
 	bitmap_and(state->advertising, state->advertising, mask,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS);