[PATCH net-next 0/2] mv88e6xxx fixed link fixes

[PATCH net-next 0/2] mv88e6xxx fixed link fixes

[Andrew Lunn]

Recent changes for how the MAC is configured broke fixed links, as
used by CPU/DSA ports, and for SFPs when phylink cannot be used.

Andrew Lunn (2):
  net: dsa: mv88e6xxx: Configure MAC when using fixed link
  net: dsa: mv88e6xxx: Set link down when changing speed

 drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.26.0.rc2

[PATCH net-next 1/2] net: dsa: mv88e6xxx: Configure MAC when using fixed link

[Andrew Lunn]

The 88e6185 is reporting it has detected a PHY, when a port is
connected to an SFP. As a result, the fixed-phy configuration is not
being applied. That then breaks packet transfer, since the port is
reported as being down.

Add additional conditions to check the interface mode, and if it is
fixed always configure the port on link up/down, independent of the
PPU status.

Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 221593261e8f..dd8a5666a584 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -709,7 +709,8 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
 	ops = chip->info->ops;
 
 	mv88e6xxx_reg_lock(chip);
-	if (!mv88e6xxx_port_ppu_updates(chip, port) && ops->port_set_link)
+	if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
+	     mode == MLO_AN_FIXED) && ops->port_set_link)
 		err = ops->port_set_link(chip, port, LINK_FORCED_DOWN);
 	mv88e6xxx_reg_unlock(chip);
 
@@ -731,7 +732,7 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 	ops = chip->info->ops;
 
 	mv88e6xxx_reg_lock(chip);
-	if (!mv88e6xxx_port_ppu_updates(chip, port)) {
+	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
 		/* FIXME: for an automedia port, should we force the link
 		 * down here - what if the link comes up due to "other" media
 		 * while we're bringing the port up, how is the exclusivity
-- 
2.26.0.rc2

[PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Andrew Lunn]

The MAC control register must not be changed unless the link is down.
Add the necassary call into mv88e6xxx_mac_link_up. Without it, the MAC
does not change state, the link remains at the wrong speed.

Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dd8a5666a584..24ce17503950 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -733,6 +733,14 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 
 	mv88e6xxx_reg_lock(chip);
 	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
+		/* Port's MAC control must not be changed unless the
+		 * link is down
+		 */
+		err = chip->info->ops->port_set_link(chip, port,
+						     LINK_FORCED_DOWN);
+		if (err)
+			goto error;
+
 		/* FIXME: for an automedia port, should we force the link
 		 * down here - what if the link comes up due to "other" media
 		 * while we're bringing the port up, how is the exclusivity
-- 
2.26.0.rc2

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Russell King - ARM Linux admin]

On Mon, Mar 23, 2020 at 10:49:00PM +0100, Andrew Lunn wrote:
> The MAC control register must not be changed unless the link is down.
> Add the necassary call into mv88e6xxx_mac_link_up. Without it, the MAC
> does not change state, the link remains at the wrong speed.
> 
> Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index dd8a5666a584..24ce17503950 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -733,6 +733,14 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
>  
>  	mv88e6xxx_reg_lock(chip);
>  	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
> +		/* Port's MAC control must not be changed unless the
> +		 * link is down
> +		 */
> +		err = chip->info->ops->port_set_link(chip, port,
> +						     LINK_FORCED_DOWN);
> +		if (err)
> +			goto error;
> +

The port should be down at this point, otherwise the link state is not
matching phylink's idea of the state.  Your patch merely works around
that.  I think it needs solving properly.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Andrew Lunn]

On Mon, Mar 23, 2020 at 10:01:13PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Mar 23, 2020 at 10:49:00PM +0100, Andrew Lunn wrote:
> > The MAC control register must not be changed unless the link is down.
> > Add the necassary call into mv88e6xxx_mac_link_up. Without it, the MAC
> > does not change state, the link remains at the wrong speed.
> > 
> > Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index dd8a5666a584..24ce17503950 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -733,6 +733,14 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
> >  
> >  	mv88e6xxx_reg_lock(chip);
> >  	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
> > +		/* Port's MAC control must not be changed unless the
> > +		 * link is down
> > +		 */
> > +		err = chip->info->ops->port_set_link(chip, port,
> > +						     LINK_FORCED_DOWN);
> > +		if (err)
> > +			goto error;
> > +
> 
> The port should be down at this point, otherwise the link state is not
> matching phylink's idea of the state.  Your patch merely works around
> that.  I think it needs solving properly.

Hi Russell

So the problem here is that CPU and DSA ports should default to up and
at their fastest speed. During setup, the driver is setting the CPU
port to 1G and up. Later on, phylink finds the fixed-link node in DT,
and then sets the port to 100Mbps as requested.

How do you suggest fixing this? If we find a fixed-link, configure it
first down and then up?

     Andrew

Re: [PATCH net-next 0/2] mv88e6xxx fixed link fixes

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 23 Mar 2020 22:48:58 +0100

> Recent changes for how the MAC is configured broke fixed links, as
> used by CPU/DSA ports, and for SFPs when phylink cannot be used.

Please repost when you and Russell sort out the best way to handle the
fixed-link scenerio you described.

Thanks.

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Russell King - ARM Linux admin]

On Mon, Mar 23, 2020 at 11:39:34PM +0100, Andrew Lunn wrote:
> On Mon, Mar 23, 2020 at 10:01:13PM +0000, Russell King - ARM Linux admin wrote:
> > On Mon, Mar 23, 2020 at 10:49:00PM +0100, Andrew Lunn wrote:
> > > The MAC control register must not be changed unless the link is down.
> > > Add the necassary call into mv88e6xxx_mac_link_up. Without it, the MAC
> > > does not change state, the link remains at the wrong speed.
> > > 
> > > Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> > >  drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > index dd8a5666a584..24ce17503950 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -733,6 +733,14 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
> > >  
> > >  	mv88e6xxx_reg_lock(chip);
> > >  	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
> > > +		/* Port's MAC control must not be changed unless the
> > > +		 * link is down
> > > +		 */
> > > +		err = chip->info->ops->port_set_link(chip, port,
> > > +						     LINK_FORCED_DOWN);
> > > +		if (err)
> > > +			goto error;
> > > +
> > 
> > The port should be down at this point, otherwise the link state is not
> > matching phylink's idea of the state.  Your patch merely works around
> > that.  I think it needs solving properly.
> 
> Hi Russell
> 
> So the problem here is that CPU and DSA ports should default to up and
> at their fastest speed. During setup, the driver is setting the CPU
> port to 1G and up. Later on, phylink finds the fixed-link node in DT,
> and then sets the port to 100Mbps as requested.
> 
> How do you suggest fixing this? If we find a fixed-link, configure it
> first down and then up?

I think this is another example of DSA fighting phylink in terms of
what's expected.

The only suggestion I've come up so far with is to avoid calling
mv88e6xxx_port_setup_mac() with forced-link-up in
mv88e6xxx_setup_port() if we have phylink attached.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Andrew Lunn]

> > Hi Russell
> > 
> > So the problem here is that CPU and DSA ports should default to up and
> > at their fastest speed. During setup, the driver is setting the CPU
> > port to 1G and up. Later on, phylink finds the fixed-link node in DT,
> > and then sets the port to 100Mbps as requested.
> > 
> > How do you suggest fixing this? If we find a fixed-link, configure it
> > first down and then up?
> 
> I think this is another example of DSA fighting phylink in terms of
> what's expected.
> 
> The only suggestion I've come up so far with is to avoid calling
> mv88e6xxx_port_setup_mac() with forced-link-up in
> mv88e6xxx_setup_port() if we have phylink attached.

Hi Russell

Yes, that might work. But it is a solution specific to mv88e6xxx. I
guess other switches could have a similar issue.

Is it really that bad to add the link down as i proposed? Do we even
have a guarantee the port is down before phylink starts configuring
it, for all switch drivers?

    Andrew

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

[Russell King - ARM Linux admin]

On Fri, Mar 27, 2020 at 10:26:08PM +0100, Andrew Lunn wrote:
> > > Hi Russell
> > > 
> > > So the problem here is that CPU and DSA ports should default to up and
> > > at their fastest speed. During setup, the driver is setting the CPU
> > > port to 1G and up. Later on, phylink finds the fixed-link node in DT,
> > > and then sets the port to 100Mbps as requested.
> > > 
> > > How do you suggest fixing this? If we find a fixed-link, configure it
> > > first down and then up?
> > 
> > I think this is another example of DSA fighting phylink in terms of
> > what's expected.
> > 
> > The only suggestion I've come up so far with is to avoid calling
> > mv88e6xxx_port_setup_mac() with forced-link-up in
> > mv88e6xxx_setup_port() if we have phylink attached.
> 
> Hi Russell
> 
> Yes, that might work. But it is a solution specific to mv88e6xxx. I
> guess other switches could have a similar issue.
> 
> Is it really that bad to add the link down as i proposed? Do we even
> have a guarantee the port is down before phylink starts configuring
> it, for all switch drivers?

That's partly why I made the suggestion, as mac_config() could end
up being called with potential changes to the configuration with
the link already up.  If that can happen, then it's not just about
the link being down before mac_link_up() is called...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up