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

[Andrew Lunn <andrew@lunn.ch>]

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