Re: mv88e6240 configuration broken for B850v3

On Tue, Dec 07, 2021 at 03:24:13PM +0200, Vladimir Oltean wrote:

On Tue, Dec 07, 2021 at 12:58:20AM +0000, Russell King (Oracle) wrote:

quoted

quoted

Anyway, so be it. Essentially, what is the most frustrating is that I
haven't been doing anything else for the past 4 hours, and I still
haven't understood enough of what you're saying to even understand why
it is _relevant_ to Martyn's report. All I understand is that you're
also looking in that area of the code for a completely unrelated reason
which you've found adequate to mention here and now.

I hope you realise that _your_ attitude here is frustrating and
inflamatory. This is _not_ a "completely unrelated reason" - this
is about getting the right solution to Martyn's problem. I thought
about doing another detailed reply, but when I got to the part
about you wanting to check 6390X, I discarded that reply and
wrote this one instead. You clearly have a total distrust for
everything that I write in any email, so I just won't bother.

I think getting the right solution to Martyn's problem is the most
important part. I'd be very happy if I understood it too, although that
isn't mandatory, since it may be beyond me but still correct, and I hope
I'm not standing in the way if that is the case.
I've explained my current state of understanding, which is that I saw in
the manual for 6097 that forcing the link is unnecessary for internal
PHY ports regardless of whether the PPU is enabled or not. Yet, DSA will
still force these ports down with your proposed change (because DSA lies
that it is a MLO_AN_FIXED mode despite what's in the device tree), and
it relies on unforcing them in mv88e6xxx_mac_config(), which in itself
makes sense considering the other discussion about kexec and such. But
it appears that it may not unforce them again - because that condition
depends on mv88e6xxx_port_ppu_updates() which may be false. So if it is
false, things are still broken.
It isn't a matter of distrust and I'm sorry that you take it personal,
but your proposed solution doesn't appear to me to have a clear
correlation to the patch that introduced a regression.

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().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!