Message-ID: <aS3F36UAkeLfFeHx@shell.armlinux.org.uk>
Date: Mon, 1 Dec 2025 16:44:15 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Heiko Stuebner <heiko@...ech.de>, Jakub Kicinski <kuba@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC net-next 00/15] net: stmmac: rk: cleanups galore

On Mon, Dec 01, 2025 at 02:49:56PM +0000, Russell King (Oracle) wrote:
> This is work in progress, cleaning up the excessively large Rockchip
> glue driver somewhat. This series as it currently stands removes
> approximately 200 lines from this file, while adding slightly to its
> flexibility.
> 
> A brief overview of the changes:
> 
> - similar to previous commits, it seems the RGMII clock field follows
>   a common pattern irrespective of the SoC.
> - update rk3328 to use the ->id mechanism rather than guessing from
>   the PHY interface mode and whether the PHY is integrated.
> - switch to wm16 based masking, providing the lower-16 bits of the
>   mask to indicate appropriate fields, and use this to construct the
>   values to write to the registers.
> - convert px30 to these methods.
> - since many set_to_rmii() methods are now empty, add flags to indicate
>   whether RMII / RGMII are supported.
> - clear RGMII where the specific SoC's GMAC instance doesn't support
>   this.
> 
> I've spent quite a while mulling over how to deal with these "wm16"
> registers, none of the kernel bitfield macros (not even the
> hw_bitfield.h macros) allow for what I present here, because the
> masks are not constant.
> 
> One of the interesting things is that this appears to deal with RGMII
> delays at the MAC end of the link, but there's no way to tell phylib
> that's the case. I've not looked deeply into what is going on there,
> but it is surprising that the driver insists that the delays (in
> register values?) are provided, but then ignores them depending on the
> exact RGMII mode selected.
> 
> One outstanding issue with these patches: RK3528_GMAC0_CLK_RMII_DIV2
> remains, although I deleted its definition, so there's build errors
> in this series. Before I do anything about that, I would like to hear
> from the Rockchip guys whether it is necessary for rk3528_set_to_rmii()
> to set the clock rate, given that rk_set_clk_tx_rate() will do this
> when the link comes up. Does it matter whether it was set to 2.5MHz
> (/ 20) or 25MHz (/ 2) when we switch to RMII mode?

Another issue has come up while looking at this driver -
gmac_clk_enable() is buggy.

If clk_bulk_prepare_enable() succeeds, but then the following
clk_prepare_enable() fails, we simply return its error code, failing
gmac_clk_enable(, true), leaving the bulk clocks prepared and enabled.

Calling this with "false" to disable clocks won't - because we never
get as far as setting bsp_priv->clk_enabled, and even if we did, we'd
disable and unprepare clk_phy which failed to prepare/enable.

Again, I don't like this foo_enable() / foo_power_on() pattern with
a true/false argument - when false, the function is not enabling
nor "on"-ing, but disabling or "off"-ing. So, gmac_clk_enable() is
going to get split up and renamed.

-- 
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