Re: mv88e6240 configuration broken for B850v3

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Dec 07, 2021 at 01:59:03PM +0000, Russell King (Oracle) wrote:
> The change that I have proposed is based on two patches:
> 
> 1) Change mv88e6xxx_port_ppu_updates() to behave sensibly for the 6250
>    family of DSA switches.
> 
> 2) Un-force the link-down in mv88e6xxx_mac_config()
> 
> This gives us more consistency. The checks become:
> 
> a) mac_link_down():
> 
>         if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
>              mode == MLO_AN_FIXED) && ops->port_sync_link)
>                 err = ops->port_sync_link(chip, port, mode, false);
> 
> b) mac_link_up():
> 
>         if (!mv88e6xxx_port_ppu_updates(chip, port) ||
>             mode == MLO_AN_FIXED) {
> ...
>                 if (ops->port_sync_link)
>                         err = ops->port_sync_link(chip, port, mode, true);
>         }
> 
> c) mac_config():
> 
>         if (chip->info->ops->port_set_link &&
>             ((mode == MLO_AN_INBAND && p->interface != state->interface) ||
>              (mode == MLO_AN_PHY && mv88e6xxx_port_ppu_updates(chip, port))))
>                 chip->info->ops->port_set_link(chip, port, LINK_UNFORCED);
> 
> The "(mode == MLO_AN_INBAND && p->interface != state->interface)"
> expression comes from the existing code, so isn't something that
> needs discussion.
> 
> We want to preserve the state of the link-force for MLO_AN_FIXED,
> so the only case for discussion is the new MLO_AN_PHY. It can be
> seen that all three methods above end up using the same decision,
> and essentially if mv88e6xxx_port_ppu_updates() is true, meaning
> there is a non-software method to handle the status updates, then
> we (a) don't do any forcing in the mac_link_*() methods and turn
> off the forcing in mac_config().

Ok, so if we are in:
- MLO_AN_PHY (internal or external) and PPU enabled, we're letting the link unforced in mac_config
- MLO_AN_PHY (internal or external) and PPU disabled, we're forcing the link up in mac_link_up,
  via the "!mv88e6xxx_port_ppu_updates()" check.
- MLO_AN_FIXED, we're also forcing the link up in mac_link_up, via the
  "mode == MLO_AN_FIXED" check.

So in Martyn's case, a CPU port internal PHY link which is forced down
in mac_link_down would either be forced up or unforced, but never be
left the way in which mac_link_down put it. That is good and appears
robust.

Although it isn't strictly true that "[ if ] there is a non-software
method to handle the status updates, then we don't do any forcing in
the mac_link_*() methods", because technically we still do in
mac_link_down, due to the " || mode == MLO_AN_FIXED" condition plus the
way in which we are called, it's just that we undo it later.