Date:   Tue, 7 Dec 2021 01:27:35 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Martyn Welch <martyn.welch@...labora.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, kernel@...labora.com
Subject: Re: mv88e6240 configuration broken for B850v3

On Mon, Dec 06, 2021 at 09:49:37PM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 06, 2021 at 09:27:33PM +0000, Russell King (Oracle) wrote:
> > We used to just rely on the PPU bit for making the decision, but when
> > I introduced that helper, I forgot that the PPU bit doesn't exist on
> > the 6250 family, which resulted in commit 4a3e0aeddf09. Looking at
> > 4a3e0aeddf09, I now believe the fix there to be wrong. It should
> > have made mv88e6xxx_port_ppu_updates() follow
> > mv88e6xxx_phy_is_internal() for internal ports only for the 6250 family
> > that has the link status bit in that position, especially as one can
> > disable the PPU bit in DSA switches such as 6390, which for some ports
> > stops the PHY being used and switches the port to serdes mode.
> > "Internal" ports aren't always internal on these switches.
> 
> Here's the situation I'm concerned about. The 88E6390X has two serdes
> each with four lanes. Let's just think about one serdes. Lane 0 is
> assigned to port 9 and lane 1 to port 4. We don't need to consider
> any others.
> 
> If the PHY_DETECT bit (effectively PPU poll enable) is set for port 4,
> which is an "internal" port, then the port is in auto-media mode, and
> the PPU will poll the internal PHY and the serdes, and configure
> according to which has link.
>
> If the PPU bit is clear, then the port is forced to serdes mode.
> However, in this configuration, we end up with:
> 
> 	mv88e6xxx_phy_is_internal(ds, port) = true
> 	mv88e6xxx_port_ppu_updates(chip, port) = false
> 
> which results in:
> 
>         if ((!mv88e6xxx_phy_is_internal(ds, port) &&
>              !mv88e6xxx_port_ppu_updates(chip, port)) ||
>             mode == MLO_AN_FIXED) {
> 
> being false since we have (!true && !false) || false. So, in actual
> fact, when we have a PHY_DETECT bit, we _do_ need to take note of it
> whether the port is "internal" or not. Essentially, that means that
> for DSA switches that are not part of the 6250, we should be using
> the PHY_DETECT bit.
> 
> For the 6250 family, the problem is that there's no PHY_DETECT bit,
> and that's the link status. So I've started a separate discussion
> with Maarten to find out which Marvell switch is being used and
> whether an alterative approach would work for him.

I hope that you understand that it is getting very difficult for me to
follow, especially when faced with contradictory information about
hardware that I am not familiar with, and which may well be very
different from one family to another for all I know.

Commit 4a3e0aeddf09 ("net: dsa: mv88e6xxx: don't use PHY_DETECT on
internal PHY's") says nothing about the 6250, and I have nothing through
which I can cross-check your statement about the change only being
applicable/needed for 6250.

The documentation I happen to have access to, which is for the 6097,
says about "Forcing Link, Speed, Duplex in the MAC":

| These bits change the port's MAC mode only! It does not change the mode
| of the PHY for ports where a PHY is connected. These bits are intended
| to be used for the following situations only:
| 
| - When the PHY Polling Unit (PPU) is disabled on the port (PHYDetect
|   equal to zero in Port offset 0x00) and software needs to copy the
|   PHY’s Link, Speed and Duplex values to the port’s MAC (this is not
|   required for internal PHYs as this information is communicated between
|   the PHY and MAC even if the PPU is disabled on the port).
| 
| - When no PHY is connected to the port. This includes ports that connect
|   to a CPU (typically using a digital interface like MII or GMII) and
|   ports connected to another switch device (typically using a SERDES
|   interface).  SERDES ports connected to a fiber module will get their
|   Link from the port’s SDET pin and its Speed and Duplex is set to
|   1000BASE full-duplex (assuming the Px_MODE has been set correctly –
|   see the C_Mode bits, Port offset 0x00).

So the first paragraph is pretty clear to me: the PPU could be disabled
(PHY_DETECT bit unset) and yet there would still be no reason to force
the link for internal PHY ports. So the change still makes some level of
sense to me, even if not for the cited reason from the commit message.

As for your auto-media comment, you say that on 6390X, port 4 is an
auto-media port only if the PPU is enabled - otherwise it falls back to
SERDES mode and not to internal PHY mode. Again, no way to cross-check,
but so be it. The problem that you cite here is that we are in SERDES
mode with PPU disabled, and that we should* (should we?) force the link,
yet we don't, because the mv88e6xxx_phy_is_internal() function only
conveys static information that doesn't properly reflect the current
state of an auto-media port. The question is: did this use to work
properly before the commit 4a3e0aeddf09 ("net: dsa: mv88e6xxx: don't use
PHY_DETECT on internal PHY's") that you mention? This is the same as
asking: if the PPU is disabled on an auto media port, the old code (via
your commit 5d5b231da7ac ("net: dsa: mv88e6xxx: use PHY_DETECT in
mac_link_up/mac_link_down")) would always force the link. Is that ok for
an internal PHY port? Is it ok for a fiber port with clause 37 in-band
autoneg? More below.

* would it not matter whether this SERDES port is used in SGMII vs
  1000base-X mode? According to the documentation I have access to, only
  SGMII would need forcing without a PPU - see second quoted paragraph.

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.

Powered by blists - more mailing lists