Re: mv88e6240 configuration broken for B850v3

On Mon, Dec 06, 2021 at 09:37:30PM +0200, Vladimir Oltean wrote:

On Mon, Dec 06, 2021 at 07:24:56PM +0000, Martyn Welch wrote:

quoted

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.

It's needed because otherwise we end up reconfiguring a CPU link while
it is still "up". Phylink's initial mode is "link down" and the phylink
design is such that it guarantees that we will not call mac_link_down()
unless the link was previously up. This is true of all network drivers,
but was found to be false for some DSA drivers, and some DSA broke as
a result.

My conclusion from having read this thread is the CPU port is using PPU
polling, meaning that in mac_link_up():

        if ((!mv88e6xxx_phy_is_internal(ds, port) &&
             !mv88e6xxx_port_ppu_updates(chip, port)) ||
            mode == MLO_AN_FIXED) {

is false - because mv88e6xxx_port_ppu_updates() returns true, and
consequently we never undo this force-down. On Marvell hardware, the
PPU updating effectively makes it appear as an in-band link as the
hardware fetches the PHY status to the MAC by polling the PHY.

What we do elsewhere is we handle the link-up-down-forcing in
mac_config appropriate to the mode, and we already do that in Marvell
DSA:

        /* Undo the forced down state above after completing configuration
         * irrespective of its state on entry, which allows the link to come up.         */
        if (mode == MLO_AN_INBAND && p->interface != state->interface &&
            chip->info->ops->port_set_link)
                chip->info->ops->port_set_link(chip, port, LINK_UNFORCED);

I'm thinking this needs set LINK_UNFORCED here if the PPU is polling
and we're in MLO_AN_PHY mode:

	if (((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->info->ops->port_set_link(chip, port, LINK_UNFORCED);

What I don't know is whether that mv88e6xxx_port_ppu_updates() 
should be mv88e6xxx_phy_is_internal() || mv88e6xxx_port_ppu_updates(),
at which point introducing a helper for that would probably be a good
idea, or it might be simpler to use the new phylink_get_caps() ability
to set phylink_config.ovr_an_inband for the internal/ppu polled ports
if they're not operating in fixed link mode. This is essentially what
is going on here - the PPU polling is just a way of replicating SGMII's
propagation of the PHY status to the MAC.

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