[PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Russell King (Oracle)]

Hi,

This series cleans up the thead clock rate setting to use the
rgmii_clock() helper function added to phylib.

The first patch switches over to using the rgmii_clock() helper,
and the second patch cleans up the verification that the desired
clock rate is achievable, allowing the private clock rate
definitions to be removed.

 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 28 ++++++++---------------
 1 file changed, 9 insertions(+), 19 deletions(-)

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

[PATCH net-next 1/2] net: stmmac: thead: use rgmii_clock() for RGMII clock rate

[Russell King (Oracle)]

Switch to using rgmii_clock() to get the RGMII TXC rate, and calculate
the divisor from the parent clock rate and the rate indicated by
rgmii_clock().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/dwmac-thead.c    | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
index ddb1d8aba321..f16fa341aadb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
@@ -109,6 +109,7 @@ static void thead_dwmac_fix_speed(void *priv, int speed, unsigned int mode)
 	struct plat_stmmacenet_data *plat;
 	struct thead_dwmac *dwmac = priv;
 	unsigned long rate;
+	long tx_rate;
 	u32 div, reg;
 
 	plat = dwmac->plat;
@@ -131,21 +132,14 @@ static void thead_dwmac_fix_speed(void *priv, int speed, unsigned int mode)
 
 		writel(0, dwmac->apb_base + GMAC_PLLCLK_DIV);
 
-		switch (speed) {
-		case SPEED_1000:
-			div = rate / GMAC_GMII_RGMII_RATE;
-			break;
-		case SPEED_100:
-			div = rate / GMAC_MII_RATE;
-			break;
-		case SPEED_10:
-			div = rate * 10 / GMAC_MII_RATE;
-			break;
-		default:
+		tx_rate = rgmii_clock(speed);
+		if (tx_rate < 0) {
 			dev_err(dwmac->dev, "invalid speed %d\n", speed);
 			return;
 		}
 
+		div = rate / tx_rate;
+
 		reg = FIELD_PREP(GMAC_PLLCLK_DIV_EN, 1) |
 		      FIELD_PREP(GMAC_PLLCLK_DIV_NUM, div);
 		writel(reg, dwmac->apb_base + GMAC_PLLCLK_DIV);
-- 
2.30.2

Re: [PATCH net-next 1/2] net: stmmac: thead: use rgmii_clock() for RGMII clock rate

[Andrew Lunn]

On Fri, Feb 21, 2025 at 02:15:12PM +0000, Russell King (Oracle) wrote:
> Switch to using rgmii_clock() to get the RGMII TXC rate, and calculate
> the divisor from the parent clock rate and the rate indicated by
> rgmii_clock().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 2/2] net: stmmac: thead: ensure divisor gives proper rate

[Russell King (Oracle)]

thead was checking that the stmmac_clk rate was a multiple of the
RGMII rates for 1G and 100M, but didn't check for 10M. Rather than
use this with hard-coded speeds, check that the calculated divisor
gives the required rate by multplying the transmit clock rate back
up to the stmmac clock rate and checking that it agrees.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
index f16fa341aadb..f9f2bd65959f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
@@ -45,9 +45,6 @@
 #define  TXCLK_DIR_OUTPUT		FIELD_PREP(TXCLK_DIR_MASK, 0)
 #define  TXCLK_DIR_INPUT		FIELD_PREP(TXCLK_DIR_MASK, 1)
 
-#define GMAC_GMII_RGMII_RATE	125000000
-#define GMAC_MII_RATE		25000000
-
 struct thead_dwmac {
 	struct plat_stmmacenet_data *plat;
 	void __iomem *apb_base;
@@ -124,11 +121,6 @@ static void thead_dwmac_fix_speed(void *priv, int speed, unsigned int mode)
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		rate = clk_get_rate(plat->stmmac_clk);
-		if (!rate || rate % GMAC_GMII_RGMII_RATE != 0 ||
-		    rate % GMAC_MII_RATE != 0) {
-			dev_err(dwmac->dev, "invalid gmac rate %ld\n", rate);
-			return;
-		}
 
 		writel(0, dwmac->apb_base + GMAC_PLLCLK_DIV);
 
@@ -139,6 +131,10 @@ static void thead_dwmac_fix_speed(void *priv, int speed, unsigned int mode)
 		}
 
 		div = rate / tx_rate;
+		if (rate != tx_rate * div) {
+			dev_err(dwmac->dev, "invalid gmac rate %lu\n", rate);
+			return;
+		}
 
 		reg = FIELD_PREP(GMAC_PLLCLK_DIV_EN, 1) |
 		      FIELD_PREP(GMAC_PLLCLK_DIV_NUM, div);
-- 
2.30.2

Re: [PATCH net-next 2/2] net: stmmac: thead: ensure divisor gives proper rate

[Andrew Lunn]

On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> thead was checking that the stmmac_clk rate was a multiple of the
> RGMII rates for 1G and 100M, but didn't check for 10M. Rather than
> use this with hard-coded speeds, check that the calculated divisor
> gives the required rate by multplying the transmit clock rate back
> up to the stmmac clock rate and checking that it agrees.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Russell King (Oracle)]

Adding Joe Perches.

On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> This series cleans up the thead clock rate setting to use the
> rgmii_clock() helper function added to phylib.
> 
> The first patch switches over to using the rgmii_clock() helper,
> and the second patch cleans up the verification that the desired
> clock rate is achievable, allowing the private clock rate
> definitions to be removed.
> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 28 ++++++++---------------
>  1 file changed, 9 insertions(+), 19 deletions(-)

I've been investigating why the NIPA bot complains about maintainers
not being Cc'd, such as for patch 1 of this series:

https://netdev.bots.linux.dev/static/nipa/936447/13985595/cc_maintainers/stdout

I think this is a bug in either get_maintainers.pl or the NIPA bot.

On the bare patch without any Cc: header added:

$ scripts/get_maintainer.pl 0001-net-stmmac-thead-use-rgmii_clock-for-RGMII-clock-rat.patch
Drew Fustini <drew@pdp7.com> (maintainer:RISC-V THEAD SoC SUPPORT)
Guo Ren <guoren@kernel.org> (maintainer:RISC-V THEAD SoC SUPPORT)
Fu Wei <wefu@redhat.com> (maintainer:RISC-V THEAD SoC SUPPORT)
Andrew Lunn <andrew+netdev@lunn.ch> (maintainer:NETWORKING DRIVERS)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
Maxime Coquelin <mcoquelin.stm32@gmail.com> (maintainer:ARM/STM32 ARCHITECTURE)
Alexandre Torgue <alexandre.torgue@foss.st.com> (maintainer:ARM/STM32 ARCHITECTURE)
linux-riscv@lists.infradead.org (open list:RISC-V THEAD SoC SUPPORT)
netdev@vger.kernel.org (open list:STMMAC ETHERNET DRIVER)
linux-stm32@st-md-mailman.stormreply.com (moderated list:ARM/STM32 ARCHITECTURE)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/STM32 ARCHITECTURE)

If I add those maintainers to a Cc header in the patch file (as it would
be if NIPA runs on the unmodified received email), and then re-run
get_maintainer.pl, then:

$ scripts/get_maintainer.pl 0001-net-stmmac-thead-use-rgmii_clock-for-RGMII-clock-rat.patch
Drew Fustini <drew@pdp7.com> (maintainer:RISC-V THEAD SoC SUPPORT)
Guo Ren <guoren@kernel.org> (maintainer:RISC-V THEAD SoC SUPPORT)
Fu Wei <wefu@redhat.com> (maintainer:RISC-V THEAD SoC SUPPORT)
Andrew Lunn <andrew+netdev@lunn.ch> (maintainer:NETWORKING DRIVERS)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
Maxime Coquelin <mcoquelin.stm32@gmail.com> (maintainer:ARM/STM32 ARCHITECTURE)
Alexandre Torgue <alexandre.torgue@foss.st.com> (maintainer:ARM/STM32 ARCHITECTURE)
Paul Walmsley <paul.walmsley@sifive.com> (supporter:RISC-V ARCHITECTURE:Keyword:riscv)
Palmer Dabbelt <palmer@dabbelt.com> (supporter:RISC-V ARCHITECTURE:Keyword:riscv)
Albert Ou <aou@eecs.berkeley.edu> (supporter:RISC-V ARCHITECTURE:Keyword:riscv)
linux-riscv@lists.infradead.org (open list:RISC-V THEAD SoC SUPPORT)
netdev@vger.kernel.org (open list:STMMAC ETHERNET DRIVER)
linux-stm32@st-md-mailman.stormreply.com (moderated list:ARM/STM32 ARCHITECTURE)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/STM32 ARCHITECTURE)
linux-kernel@vger.kernel.org (open list)

Note the addition of Paul Walmsley, Palmer Dabbelt and Albert Ou that
was not in the original.

It seems that K: is fed everything in the file, including all headers.
In the second run, the addition of the linux-riscv@lists.infradead.org
mailing list in the headers then causes a subseqent run of
get_maintainer.pl (and nipa's run of get_maintainer.pl) to then match
using K: riscv in the "RISC-V ARCHITECTURE" entry.

So, it seems running get_maintainer.pl on an email received from
mailing lists without first stripping many of the email headers can
lead to false K: matches.

This makes NIPA's cc_maintainers test unreliable.

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

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Joe Perches]

On Sun, 2025-02-23 at 11:40 +0000, Russell King (Oracle) wrote:
> Adding Joe Perches.
> 
> On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
[]
> I've been investigating why the NIPA bot complains about maintainers
> not being Cc'd, such as for patch 1 of this series:
> 
> https://netdev.bots.linux.dev/static/nipa/936447/13985595/cc_maintainers/stdout

Additional maintainers added or missing?

> So, it seems running get_maintainer.pl on an email received from
> mailing lists without first stripping many of the email headers can
> lead to false K: matches.

My guess is the nipa bot should exec get_maintainer.pl with the
--nokeywords option on some pass it uses after cc's are added.

Or are you suggesting that lines that start with "to:" or "cc:"
should be ignored by the get_maintainer keyword test?

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Russell King (Oracle)]

On Sun, Feb 23, 2025 at 06:33:44AM -0800, Joe Perches wrote:
> On Sun, 2025-02-23 at 11:40 +0000, Russell King (Oracle) wrote:
> > Adding Joe Perches.
> > 
> > On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> []
> > I've been investigating why the NIPA bot complains about maintainers
> > not being Cc'd, such as for patch 1 of this series:
> > 
> > https://netdev.bots.linux.dev/static/nipa/936447/13985595/cc_maintainers/stdout
> 
> Additional maintainers added or missing?

Let me be clear - NIPA is not something under my control. It is a bot
run by Jakub on netdev patches that are received by patchwork - so
patches that have been emailed out, and thus contain at least the
To:, Cc: and Subject: header lines, possibly all header lines that
have been added such as Received: etc. I don't know what it actually
does.

Now let me restate the problem, because the answer to your question
is in the problem description. Here's the short version:

	K: entries match email headers.

Here's the long version:

If one runs get_maintainers.pl on a patch produced from git, it
comes out with a list of maintainers. In the case of dwmac-thead.c,
this includes an email address that contains "riscv".

If one adds this list of maintainers to email headers in the patch
prior to sending it out and then re-runs get_maintainers.pl on it,
or if one receives the patch after it having been emailed out, and
then runs get_maintainers.pl to validate that all appropriate
maintainers were sent a copy of the patch, then get_maintainers.pl
comes out with *extra* *additional* maintainers because the "K: riscv"
line matches *email* *headers*.

In this exact case of dwmac-thead.c, the first run prior to sending
out reports an email address containing "riscv". On these subsequent
runs with the maintainers added to email headers, the presence of
"K: riscv" in MAINTAINERS causes get_maintainers.pl to report the
three maintains for the "RISCV ARCHITECTURE" entry.

This is an issue for you as get_maintainers.pl maintainer and Jakub
as NIPA bot author to hash out - either get_maintainers.pl is
acting incorrectly and needs to be fixed, or NIPA is abusing
get_maintainers.pl in a way that it's not designed to be used.

I'm merely an observer of this behaviour and am merely reporting the
problem - that NIPA's cc_maintainers claim that maintainers were not
copied in my patch submission is incorrect and I've done the research
to identify _why_ it's incorrect. It's now up to you two to decide
where the problem lies and what the solution should be.

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

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Joe Perches]

On Mon, 2025-02-24 at 09:09 +0000, Russell King (Oracle) wrote:
> On Sun, Feb 23, 2025 at 06:33:44AM -0800, Joe Perches wrote:
> > On Sun, 2025-02-23 at 11:40 +0000, Russell King (Oracle) wrote:
> > > Adding Joe Perches.
> > > 
> > > On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> > []
> > > I've been investigating why the NIPA bot complains about maintainers
> > > not being Cc'd, such as for patch 1 of this series:
> > > 
> > > https://netdev.bots.linux.dev/static/nipa/936447/13985595/cc_maintainers/stdout
> > 
> > Additional maintainers added or missing?
> 
> Let me be clear - NIPA is not something under my control. It is a bot
> run by Jakub on netdev patches that are received by patchwork - so
> patches that have been emailed out, and thus contain at least the
> To:, Cc: and Subject: header lines, possibly all header lines that
> have been added such as Received: etc. I don't know what it actually
> does.
> 
> Now let me restate the problem, because the answer to your question
> is in the problem description. Here's the short version:
> 
> 	K: entries match email headers.
> 
> Here's the long version:
> 
> If one runs get_maintainers.pl on a patch produced from git, it
> comes out with a list of maintainers. In the case of dwmac-thead.c,
> this includes an email address that contains "riscv".

Yeah, I got all that from your first cc, thanks.

Which is why I suggested that the nipa bot use
get_maintainer.pl's --nokeywords option somewhere.

I don't use/control/read/write/care_about the nipa bot either.

cheers, Joe

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Russell King (Oracle)]

On Mon, Feb 24, 2025 at 01:34:36AM -0800, Joe Perches wrote:
> On Mon, 2025-02-24 at 09:09 +0000, Russell King (Oracle) wrote:
> > On Sun, Feb 23, 2025 at 06:33:44AM -0800, Joe Perches wrote:
> > > On Sun, 2025-02-23 at 11:40 +0000, Russell King (Oracle) wrote:
> > > > Adding Joe Perches.
> > > > 
> > > > On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> > > []
> > > > I've been investigating why the NIPA bot complains about maintainers
> > > > not being Cc'd, such as for patch 1 of this series:
> > > > 
> > > > https://netdev.bots.linux.dev/static/nipa/936447/13985595/cc_maintainers/stdout
> > > 
> > > Additional maintainers added or missing?
> > 
> > Let me be clear - NIPA is not something under my control. It is a bot
> > run by Jakub on netdev patches that are received by patchwork - so
> > patches that have been emailed out, and thus contain at least the
> > To:, Cc: and Subject: header lines, possibly all header lines that
> > have been added such as Received: etc. I don't know what it actually
> > does.
> > 
> > Now let me restate the problem, because the answer to your question
> > is in the problem description. Here's the short version:
> > 
> > 	K: entries match email headers.
> > 
> > Here's the long version:
> > 
> > If one runs get_maintainers.pl on a patch produced from git, it
> > comes out with a list of maintainers. In the case of dwmac-thead.c,
> > this includes an email address that contains "riscv".
> 
> Yeah, I got all that from your first cc, thanks.
> 
> Which is why I suggested that the nipa bot use
> get_maintainer.pl's --nokeywords option somewhere.

That's no solution. K: exists so that maintainers get Cc'd on patches
that match keywords - for example a subsystem maintainer wants to be
Cc'd on patches that make use of the subsystem interfaces would include
a K: line to pick up on function names that appear in patches.

Disabling K: means that these will be missed.

NIPA has caught several instances where I should have been Cc'd but
haven't because of this facility, and thus patches have not been
properly reviewed. So, disabling K: is detrimental.

IMHO, it's crazy that keywords match any of the email headers present
in a patch that is presented to it. As I've already said - either
get_maintainers should restrict to a limited number of headers used
for matching, or NIPA needs to present to get_maintainers what would
be an original patch as generated by git without lots of email headers.

> I don't use/control/read/write/care_about the nipa bot either.

Sigh. Why is this so damn difficult. I wasn't expecting you to do
anything about it immediately. It's something that _you_ /and/
_Jakub_ as the author of NIPA need to come to some agreement on.
That's why my original email was also sent _to_ Jakub as well.

Sheesh. Again, why is this so difficult????

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

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[Drew Fustini]

On Fri, Feb 21, 2025 at 02:15:17PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> This series cleans up the thead clock rate setting to use the
> rgmii_clock() helper function added to phylib.
> 
> The first patch switches over to using the rgmii_clock() helper,
> and the second patch cleans up the verification that the desired
> clock rate is achievable, allowing the private clock rate
> definitions to be removed.
> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 28 ++++++++---------------
>  1 file changed, 9 insertions(+), 19 deletions(-)

Thanks for improving the dwmac-thead driver. I've applied it on top of
next-20250221 and booted on the LPi4a board. Ethernet networking is
still working correctly.

Tested-by: Drew Fustini <drew@pdp7.com>

Re: [PATCH net-next 0/2] net: stmmac: thead: clean up clock rate setting

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 21 Feb 2025 14:15:17 +0000 you wrote:
> Hi,
> 
> This series cleans up the thead clock rate setting to use the
> rgmii_clock() helper function added to phylib.
> 
> The first patch switches over to using the rgmii_clock() helper,
> and the second patch cleans up the verification that the desired
> clock rate is achievable, allowing the private clock rate
> definitions to be removed.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: stmmac: thead: use rgmii_clock() for RGMII clock rate
    https://git.kernel.org/netdev/net-next/c/171fd7cb153c
  - [net-next,2/2] net: stmmac: thead: ensure divisor gives proper rate
    https://git.kernel.org/netdev/net-next/c/8bfff0481d91

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html