Message-ID: <1f9d856f-4cfb-472c-abec-96ce55f4db81@bootlin.com>
Date: Fri, 17 Oct 2025 09:04:55 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: 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>,
 Jacob Keller <jacob.e.keller@...el.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 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 net-next v2 00/14] net: stmmac: phylink PCS conversion

Hello,

On 16/10/2025 16:35, 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.
> 
> Changes since RFC:
> - new patch (7) to remove RGMII "pcs" mode
> - new patch (8) to move reverse "pcs" mode to stmmac_check_pcs_mode()
> - new patch (9) to simplify the code moved in the previous patch
> - new patch (10) to rename the confusing hw->ps to something more
>   understandable.
> - new patch (11) to shut up inappropriate complaints about
>   "snps,ps-speed" being invalid.
> - new patch (13) to add a MAC .pcs_init method, which will only be
>   called when core has PCS present.
> - modify patch 14 to use this new pcs_init method.
> 
> Despite getting a couple of responses to the RFC series posted in
> September, I have had nothing testing this on hardware. I have tested
> this on the Jetson Xavier NX, which included trial runs with enabling
> the RGMII "pcs" mode, hence the new patches that rip out this mode. I
> have come to the conclusion that the only way to get stmmac changes
> tested is to get them merged into net-next, thereby forcing people to
> have to run with them... and we'll deal with any fallout later.

I have tested this on :
 - dwmac-socfpga w/ Lynx, SGMII and 1000BaseX mode
 - dwmac-socfpga w/ RGMII mode
 - stm32-dwmac (on stm32mp157a), w/ RGMII mode

I don't have any device available with stmmac using the internal PCS
implementation, but at least the 2 platforms above don't regress with
this series.

Tested-by: Maxime Chevallier <maxime.chevallier@...tlin.com>

Maxime

Powered by blists - more mailing lists