Message-ID: <aNRG82biP9mA-rvm@shell.armlinux.org.uk>
Date: Wed, 24 Sep 2025 20:30:59 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Abhishek Chauhan <quic_abchauha@...cinc.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Alexis Lothore <alexis.lothore@...tlin.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Boon Khai Ng <boon.khai.ng@...era.com>,
	Choong Yong Liang <yong.liang.choong@...ux.intel.com>,
	Daniel Machon <daniel.machon@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Drew Fustini <dfustini@...storrent.com>,
	Emil Renner Berthing <emil.renner.berthing@...onical.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>,
	Furong Xu <0x1207@...il.com>, Inochi Amaoto <inochiama@...il.com>,
	Jakub Kicinski <kuba@...nel.org>,
	"Jan Petrous (OSS)" <jan.petrous@....nxp.com>,
	Jisheng Zhang <jszhang@...nel.org>, Kees Cook <kees@...nel.org>,
	Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
	Ley Foon Tan <leyfoon.tan@...rfivetech.com>,
	linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	Matthew Gerlach <matthew.gerlach@...era.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
	netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>,
	Paolo Abeni <pabeni@...hat.com>,
	Rohan G Thomas <rohan.g.thomas@...era.com>,
	Shenwei Wang <shenwei.wang@....com>,
	Simon Horman <horms@...nel.org>,
	Song Yoong Siang <yoong.siang.song@...el.com>,
	Swathi K S <swathi.ks@...sung.com>,
	Tiezhu Yang <yangtiezhu@...ngson.cn>, Vinod Koul <vkoul@...nel.org>,
	Vladimir Oltean <olteanv@...il.com>,
	Vladimir Oltean <vladimir.oltean@....com>,
	Yu-Chun Lin <eleanor15x@...il.com>
Subject: Re: [PATCH RFC net-next 0/9] net: stmmac: experimental PCS conversion

On Wed, Sep 24, 2025 at 12:13:18PM -0700, Jacob Keller wrote:
> 
> 
> On 9/24/2025 11:17 AM, Russell King (Oracle) wrote:
> > This series is radical - it takes the brave step of ripping out much of
> > the existing PCS support code and throwing it all away.
> > 
> > I have discussed the introduction of the STMMAC_FLAG_HAS_INTEGRATED_PCS
> > flag with Bartosz Golaszewski, and the conclusion I came to is that
> > this is to workaround the breakage that I've been going on about
> > concerning the phylink conversion for the last five or six years.
> > 
> > The problem is that the stmmac PCS code manipulates the netif carrier
> > state, which confuses phylink.
> > 
> > There is a way of testing this out on the Jetson Xavier NX platform as
> > the "PCS" code paths can be exercised while in RGMII mode - because
> > RGMII also has in-band status and the status register is shared with
> > SGMII. Testing this out confirms my long held theory: the interrupt
> > handler manipulates the netif carrier state before phylink gets a
> > look-in, which means that the mac_link_up() and mac_link_down() methods
> > are never called, resulting in the device being non-functional.
> > 
> > Moreover, on dwmac4 cores, ethtool reports incorrect information -
> > despite having a full-duplex link, ethtool reports that it is
> > half-dupex.
> > 
> > Thus, this code is completely broken - anyone using it will not have
> > a functional platform, and thus it doesn't deserve to live any longer,
> > especially as it's a thorn in phylink.
> > 
> > Rip all this out, leaving just the bare bones initialisation in place.
> > 
> > However, this is not the last of what's broken. We have this hw->ps
> > integer which is really not descriptive, and the DT property from
> > which it comes from does little to help understand what's going on.
> > Putting all the clues together:
> > 
> > - early configuration of the GMAC configuration register for the
> >   speed.
> > - setting the SGMII rate adapter layer to take its speed from the
> >   GMAC configuration register.
> > 
> > Lastly, setting the transmit enable (TE) bit, which is a typo that puts
> > the nail in the coffin of this code. It should be the transmit
> > configuration (TC) bit. Given that when the link comes up, phylink
> > will call mac_link_up() which will overwrite the speed in the GMAC
> > configuration register, the only part of this that is functional is
> > changing where the SGMII rate adapter layer gets its speed from,
> > which is a boolean.
> > 
> > From what I've found so far, everyone who sets the snps,ps-speed
> > property which configures this mode also configures a fixed link,
> > so the pre-configuration is unnecessary - the link will come up
> > anyway.
> > 
> > So, this series rips that out the preconfiguration as well, and
> > replaces hw->ps with a boolean hw->reverse_sgmii_enable flag.
> > 
> > We then move the sole PCS configuration into a phylink_pcs instance,
> > which configures the PCS control register in the same way as is done
> > during the probe function.
> > 
> > Thus, we end up with much easier and simpler conversion to phylink PCS
> > than previous attempts.
> > 
> > Even so, this still results in inband mode always being enabled at the
> > moment in the new .pcs_config() method to reflect what the probe
> > function was doing. The next stage will be to change that to allow
> > phylink to correctly configure the PCS. This needs fixing to allow
> > platform glue maintainers who are currently blocked to progress.
> > 
> > Please note, however, that this has not been tested with any SGMII
> > platform.
> > 
> > I've tried to get as many people into the Cc list with get_maintainers,
> > I hope that's sufficient to get enough eyeballs on this.
> > 
> 
> I'm no expert with this hardware or driver, but all of your explanations
> seem reasonable to me.
> 
> I'd guess the real step is to try and get this tested against the
> variety of hardware supported by stmmac?

Yes please, that would be very helpful, as I don't want to regress
anyone's setup. I'm hoping that this series is going to be the low-
risk change.

Thanks.

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

Powered by blists - more mailing lists