Re: mv88e6240 configuration broken for B850v3

Andrew Lunn
2021-12-06T19:53:49Z

> > > Just out of curiosity, can you try this change? It looks like a
> > > simple
> > > case of mismatched conditions inside the mv88e6xxx driver between
> > > what
> > > is supposed to force the link down and what is supposed to force it
> > > up:
> > >
> > > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > > index 20f183213cbc..d235270babf7 100644
> > > --- a/net/dsa/port.c
> > > +++ b/net/dsa/port.c
> > > @@ -1221,7 +1221,7 @@ int dsa_port_link_register_of(struct dsa_port
> > > *dp)
> > >                 if (of_phy_is_fixed_link(dp->dn) || phy_np) {
> > >                         if (ds->ops->phylink_mac_link_down)
> > >                                 ds->ops->phylink_mac_link_down(ds, port,
> > > -                                       MLO_AN_FIXED, PHY_INTERFACE_MODE_NA);
> > > +                                       MLO_AN_PHY, PHY_INTERFACE_MODE_NA);
> > >                         return dsa_port_phylink_register(dp);
> > >                 }
> > >                 return 0;
> >
> > Yes, that appears to also make it work.
> >
> > Martyn
>
> Well, I just pointed out what the problem is, I don't know how to solve
> it, honest! :)
>
> It's clear that the code is wrong, because it's in an "if" block that
> checks for "of_phy_is_fixed_link(dp->dn) || phy_np" but then it omits
> the "phy_np" part of it. On the other hand we can't just go ahead and
> say "if (phy_np) mode = MLO_AN_PHY; else mode = MLO_AN_FIXED;" because
> MLO_AN_INBAND is also a valid option that we may be omitting. So we'd
> have to duplicate part of the logic from phylink_parse_mode(), which
> does not appear ideal at all. What would be ideal is if this fabricated
> phylink call would not be done at all, but I don't know enough about the
> systems that need it, I expect Andrew knows more.

phylink assumes interfaces start in the down state. It will then
configure them and bring them up as needed. This is not always true
with mv88e6xxx, the interface can already by up, and then the hardware
and phylink have different state information. So this call was added
to force the link down before phylink took control of it.

So i suspect something is missing when phylink sometime later does
bring the interface up. It is not fully undoing what this down
does. Maybe enable the dev_dbg() in mv88e6xxx_port_set_link() and see
what value it has in both the good and bad case?

Andrew