Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 17 Oct 2021 22:07:21 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Maarten Zanders <maarten.zanders@mind.be>
Cc:     Maarten Zanders <m.zanders@televic.com>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: dsa: mv88e6xxx: don't use PHY_DETECT on internal
 PHY's
Message-ID: <YWyQiSejqGNOG6ES@shell.armlinux.org.uk>
References: <20211011142720.42642-1-maarten.zanders@mind.be>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211011142720.42642-1-maarten.zanders@mind.be>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Mon, Oct 11, 2021 at 04:27:20PM +0200, Maarten Zanders wrote:
> mv88e6xxx_port_ppu_updates() interpretes data in the PORT_STS
> register incorrectly for internal ports (ie no PPU). In these
> cases, the PHY_DETECT bit indicates link status. This results
> in forcing the MAC state whenever the PHY link goes down which
> is not intended. As a side effect, LED's configured to show
> link status stay lit even though the physical link is down.

I know this patch has been merged, but I'm going to say this anyway
for the record.

The description is not entirely correct. It is not true that internal
ports do not have the PHY_DETECT bit. 88E6176 and friends are documented
that bit 12 is always the PHY_DETECT bit even for internal ports. Bit 11
there is Link status.

Looking at the definitions in port.h, some switches are different
(88E6250 family) and do indeed use bit 12 as link status.

The point I'm making is that the commit description is not universally
true and only applies to a subset of mv88e6xxx supported switches. It is
a shame it wasn't better reviewed before merging.

At least the patch is harmless; we leave the PHY_DETECT bit set on the
internal ports, which means the PPU fetches the configuration and
configures the port appropriately. The change merely helps to ensure
that we don't force link status on the internal ports.

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