[PATCH net-next] net: stmmac: stm32: simplify clock handling

[PATCH net-next] net: stmmac: stm32: simplify clock handling

[Russell King (Oracle)]

Some stm32 implementations need the receive clock running in suspend,
as indicated by dwmac->ops->clk_rx_enable_in_suspend. The existing
code achieved this in a rather complex way, by passing a flag around.

However, the clk API prepare/enables are counted - which means that a
clock won't be stopped as long as there are more prepare and enables
than disables and unprepares, just like a reference count.

Therefore, we can simplify this logic by calling clk_prepare_enable()
an additional time in the probe function if this flag is set, and then
balancing that at remove time.

With this, we can avoid passing a "are we suspending" and "are we
resuming" flag to various functions in the driver.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
This patch has been only build tested, so I would be grateful if
someone with the hardware could run-test this change please.

 .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 57 ++++++++++++-------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index c3d321192581..1eb16eec9c0d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -119,7 +119,7 @@ struct stm32_ops {
 	u32 syscfg_clr_off;
 };
 
-static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac, bool resume)
+static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac)
 {
 	int ret;
 
@@ -127,11 +127,9 @@ static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac, bool resume)
 	if (ret)
 		goto err_clk_tx;
 
-	if (!dwmac->ops->clk_rx_enable_in_suspend || !resume) {
-		ret = clk_prepare_enable(dwmac->clk_rx);
-		if (ret)
-			goto err_clk_rx;
-	}
+	ret = clk_prepare_enable(dwmac->clk_rx);
+	if (ret)
+		goto err_clk_rx;
 
 	ret = clk_prepare_enable(dwmac->syscfg_clk);
 	if (ret)
@@ -148,15 +146,14 @@ static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac, bool resume)
 err_clk_eth_ck:
 	clk_disable_unprepare(dwmac->syscfg_clk);
 err_syscfg_clk:
-	if (!dwmac->ops->clk_rx_enable_in_suspend || !resume)
-		clk_disable_unprepare(dwmac->clk_rx);
+	clk_disable_unprepare(dwmac->clk_rx);
 err_clk_rx:
 	clk_disable_unprepare(dwmac->clk_tx);
 err_clk_tx:
 	return ret;
 }
 
-static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat, bool resume)
+static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat)
 {
 	struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
 	int ret;
@@ -167,7 +164,7 @@ static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat, bool resume)
 			return ret;
 	}
 
-	return stm32_dwmac_clk_enable(dwmac, resume);
+	return stm32_dwmac_clk_enable(dwmac);
 }
 
 static int stm32mp1_select_ethck_external(struct plat_stmmacenet_data *plat_dat)
@@ -382,12 +379,10 @@ static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
 				 SYSCFG_MCU_ETH_MASK, val << 23);
 }
 
-static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac, bool suspend)
+static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac)
 {
 	clk_disable_unprepare(dwmac->clk_tx);
-	if (!dwmac->ops->clk_rx_enable_in_suspend || !suspend)
-		clk_disable_unprepare(dwmac->clk_rx);
-
+	clk_disable_unprepare(dwmac->clk_rx);
 	clk_disable_unprepare(dwmac->syscfg_clk);
 	if (dwmac->enable_eth_ck)
 		clk_disable_unprepare(dwmac->clk_eth_ck);
@@ -541,18 +536,32 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
 	plat_dat->flags |= STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
 	plat_dat->bsp_priv = dwmac;
 
-	ret = stm32_dwmac_init(plat_dat, false);
+	ret = stm32_dwmac_init(plat_dat);
 	if (ret)
 		return ret;
 
+	/* If this platform requires the clock to be running in suspend,
+	 * prepare and enable the receive clock an additional time to keep
+	 * it running.
+	 */
+	if (dwmac->ops->clk_rx_enable_in_suspend) {
+		ret = clk_prepare_enable(dwmac->clk_rx);
+		if (ret)
+			goto err_clk_disable;
+	}
+
 	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 	if (ret)
-		goto err_clk_disable;
+		goto err_clk_disable_suspend;
 
 	return 0;
 
+err_clk_disable_suspend:
+	if (dwmac->ops->clk_rx_enable_in_suspend)
+		clk_disable_unprepare(dwmac->clk_rx);
+
 err_clk_disable:
-	stm32_dwmac_clk_disable(dwmac, false);
+	stm32_dwmac_clk_disable(dwmac);
 
 	return ret;
 }
@@ -565,7 +574,15 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
 
 	stmmac_dvr_remove(&pdev->dev);
 
-	stm32_dwmac_clk_disable(dwmac, false);
+	/* If this platform requires the clock to be running in suspend,
+	 * we need to disable and unprepare the receive clock an additional
+	 * time to balance the extra clk_prepare_enable() in the probe
+	 * function.
+	 */
+	if (dwmac->ops->clk_rx_enable_in_suspend)
+		clk_disable_unprepare(dwmac->clk_rx);
+
+	stm32_dwmac_clk_disable(dwmac);
 
 	if (dwmac->irq_pwr_wakeup >= 0) {
 		dev_pm_clear_wake_irq(&pdev->dev);
@@ -596,7 +613,7 @@ static int stm32_dwmac_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	stm32_dwmac_clk_disable(dwmac, true);
+	stm32_dwmac_clk_disable(dwmac);
 
 	if (dwmac->ops->suspend)
 		ret = dwmac->ops->suspend(dwmac);
@@ -614,7 +631,7 @@ static int stm32_dwmac_resume(struct device *dev)
 	if (dwmac->ops->resume)
 		dwmac->ops->resume(dwmac);
 
-	ret = stm32_dwmac_init(priv->plat, true);
+	ret = stm32_dwmac_init(priv->plat);
 	if (ret)
 		return ret;
 
-- 
2.30.2

Re: [PATCH net-next] net: stmmac: stm32: simplify clock handling

[Simon Horman]

On Mon, Apr 07, 2025 at 08:15:35PM +0100, Russell King (Oracle) wrote:
> Some stm32 implementations need the receive clock running in suspend,
> as indicated by dwmac->ops->clk_rx_enable_in_suspend. The existing
> code achieved this in a rather complex way, by passing a flag around.
> 
> However, the clk API prepare/enables are counted - which means that a
> clock won't be stopped as long as there are more prepare and enables
> than disables and unprepares, just like a reference count.
> 
> Therefore, we can simplify this logic by calling clk_prepare_enable()
> an additional time in the probe function if this flag is set, and then
> balancing that at remove time.
> 
> With this, we can avoid passing a "are we suspending" and "are we
> resuming" flag to various functions in the driver.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> This patch has been only build tested, so I would be grateful if
> someone with the hardware could run-test this change please.

Yes, agreed that would be nice.
But this is a very nice cleanup.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next] net: stmmac: stm32: simplify clock handling

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 07 Apr 2025 20:15:35 +0100 you wrote:
> Some stm32 implementations need the receive clock running in suspend,
> as indicated by dwmac->ops->clk_rx_enable_in_suspend. The existing
> code achieved this in a rather complex way, by passing a flag around.
> 
> However, the clk API prepare/enables are counted - which means that a
> clock won't be stopped as long as there are more prepare and enables
> than disables and unprepares, just like a reference count.
> 
> [...]

Here is the summary with links:
  - [net-next] net: stmmac: stm32: simplify clock handling
    https://git.kernel.org/netdev/net-next/c/61499764e5cc

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