On STM32 MCUs, Ethernet fails to come up after resume and the following
errors appear in dmesg:
[ 17.451148] stm32-dwmac 40028000.ethernet: Failed to reset the dma
[ 17.451266] stm32-dwmac 40028000.ethernet eth0: stmmac_hw_setup: DMA engine initialization failed
This occurs because clk_rx is never re-enabled during resume. On the
STM32MP1, clk_rx is left running during suspend, and therefore doesn't
need to be enabled during resume, but this code was mistakenly applied
to the STM32 MCUs as well.
The first patch in this series applies a minimal fix for the bug, while
the second refactors the clock configuration to make it easier to spot
such bugs in the future.
I have tested that this series allows Ethernet to come back up correctly
after resuming from s2idle on an STM32F746. I don't have STM32MP1
hardware to test.
Ben Wolsieffer (2):
net: stmmac: dwmac-stm32: fix resume on STM32 MCU
net: stmmac: dwmac-stm32: refactor clock config
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 116 ++++++++----------
1 file changed, 48 insertions(+), 68 deletions(-)
--
2.42.0
Currently, clock configuration is spread throughout the driver and
partially duplicated for the STM32MP1 and STM32 MCU variants. This makes
it difficult to keep track of which clocks need to be enabled or disabled
in various scenarios.
This patch adds symmetric stm32_dwmac_clk_enable/disable() functions
that handle all clock configuration, including quirks required while
suspending or resuming. syscfg_clk and clk_eth_ck are not present on
STM32 MCUs, but it is fine to try to configure them anyway since NULL
clocks are ignored.
Signed-off-by: Ben Wolsieffer <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 113 +++++++-----------
1 file changed, 45 insertions(+), 68 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index a0e276783e65..e53ca4111cbe 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -98,7 +98,6 @@ struct stm32_dwmac {
struct stm32_ops {
int (*set_mode)(struct plat_stmmacenet_data *plat_dat);
- int (*clk_prepare)(struct stm32_dwmac *dwmac, bool prepare);
int (*suspend)(struct stm32_dwmac *dwmac);
void (*resume)(struct stm32_dwmac *dwmac);
int (*parse_data)(struct stm32_dwmac *dwmac,
@@ -107,62 +106,55 @@ struct stm32_ops {
bool clk_rx_enable_in_suspend;
};
-static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat)
+static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac, bool resume)
{
- struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
int ret;
- if (dwmac->ops->set_mode) {
- ret = dwmac->ops->set_mode(plat_dat);
- if (ret)
- return ret;
- }
-
ret = clk_prepare_enable(dwmac->clk_tx);
if (ret)
- return ret;
+ goto err_clk_tx;
- if (!dwmac->ops->clk_rx_enable_in_suspend ||
- !dwmac->dev->power.is_suspended) {
+ if (!dwmac->ops->clk_rx_enable_in_suspend || !resume) {
ret = clk_prepare_enable(dwmac->clk_rx);
- if (ret) {
- clk_disable_unprepare(dwmac->clk_tx);
- return ret;
- }
+ if (ret)
+ goto err_clk_rx;
}
- if (dwmac->ops->clk_prepare) {
- ret = dwmac->ops->clk_prepare(dwmac, true);
- if (ret) {
- clk_disable_unprepare(dwmac->clk_rx);
- clk_disable_unprepare(dwmac->clk_tx);
- }
+ ret = clk_prepare_enable(dwmac->syscfg_clk);
+ if (ret)
+ goto err_syscfg_clk;
+
+ if (dwmac->enable_eth_ck) {
+ ret = clk_prepare_enable(dwmac->clk_eth_ck);
+ if (ret)
+ goto err_clk_eth_ck;
}
return ret;
+
+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);
+err_clk_rx:
+ clk_disable_unprepare(dwmac->clk_tx);
+err_clk_tx:
+ return ret;
}
-static int stm32mp1_clk_prepare(struct stm32_dwmac *dwmac, bool prepare)
+static int stm32_dwmac_init(struct plat_stmmacenet_data *plat_dat, bool resume)
{
- int ret = 0;
+ struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
+ int ret;
- if (prepare) {
- ret = clk_prepare_enable(dwmac->syscfg_clk);
+ if (dwmac->ops->set_mode) {
+ ret = dwmac->ops->set_mode(plat_dat);
if (ret)
return ret;
- if (dwmac->enable_eth_ck) {
- ret = clk_prepare_enable(dwmac->clk_eth_ck);
- if (ret) {
- clk_disable_unprepare(dwmac->syscfg_clk);
- return ret;
- }
- }
- } else {
- clk_disable_unprepare(dwmac->syscfg_clk);
- if (dwmac->enable_eth_ck)
- clk_disable_unprepare(dwmac->clk_eth_ck);
}
- return ret;
+
+ return stm32_dwmac_clk_enable(dwmac, resume);
}
static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
@@ -252,13 +244,15 @@ static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
dwmac->ops->syscfg_eth_mask, val << 23);
}
-static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac)
+static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac, bool suspend)
{
clk_disable_unprepare(dwmac->clk_tx);
- clk_disable_unprepare(dwmac->clk_rx);
+ if (!dwmac->ops->clk_rx_enable_in_suspend || !suspend)
+ clk_disable_unprepare(dwmac->clk_rx);
- if (dwmac->ops->clk_prepare)
- dwmac->ops->clk_prepare(dwmac, false);
+ clk_disable_unprepare(dwmac->syscfg_clk);
+ if (dwmac->enable_eth_ck)
+ clk_disable_unprepare(dwmac->clk_eth_ck);
}
static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
@@ -400,7 +394,7 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
plat_dat->bsp_priv = dwmac;
- ret = stm32_dwmac_init(plat_dat);
+ ret = stm32_dwmac_init(plat_dat, false);
if (ret)
goto err_remove_config_dt;
@@ -411,7 +405,7 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
return 0;
err_clk_disable:
- stm32_dwmac_clk_disable(dwmac);
+ stm32_dwmac_clk_disable(dwmac, false);
err_remove_config_dt:
stmmac_remove_config_dt(pdev, plat_dat);
@@ -426,7 +420,7 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
stmmac_dvr_remove(&pdev->dev);
- stm32_dwmac_clk_disable(priv->plat->bsp_priv);
+ stm32_dwmac_clk_disable(dwmac, false);
if (dwmac->irq_pwr_wakeup >= 0) {
dev_pm_clear_wake_irq(&pdev->dev);
@@ -436,18 +430,7 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
{
- int ret = 0;
-
- ret = clk_prepare_enable(dwmac->clk_ethstp);
- if (ret)
- return ret;
-
- clk_disable_unprepare(dwmac->clk_tx);
- clk_disable_unprepare(dwmac->syscfg_clk);
- if (dwmac->enable_eth_ck)
- clk_disable_unprepare(dwmac->clk_eth_ck);
-
- return ret;
+ return clk_prepare_enable(dwmac->clk_ethstp);
}
static void stm32mp1_resume(struct stm32_dwmac *dwmac)
@@ -455,14 +438,6 @@ static void stm32mp1_resume(struct stm32_dwmac *dwmac)
clk_disable_unprepare(dwmac->clk_ethstp);
}
-static int stm32mcu_suspend(struct stm32_dwmac *dwmac)
-{
- clk_disable_unprepare(dwmac->clk_tx);
- clk_disable_unprepare(dwmac->clk_rx);
-
- return 0;
-}
-
#ifdef CONFIG_PM_SLEEP
static int stm32_dwmac_suspend(struct device *dev)
{
@@ -473,6 +448,10 @@ static int stm32_dwmac_suspend(struct device *dev)
int ret;
ret = stmmac_suspend(dev);
+ if (ret)
+ return ret;
+
+ stm32_dwmac_clk_disable(dwmac, true);
if (dwmac->ops->suspend)
ret = dwmac->ops->suspend(dwmac);
@@ -490,7 +469,7 @@ static int stm32_dwmac_resume(struct device *dev)
if (dwmac->ops->resume)
dwmac->ops->resume(dwmac);
- ret = stm32_dwmac_init(priv->plat);
+ ret = stm32_dwmac_init(priv->plat, true);
if (ret)
return ret;
@@ -505,13 +484,11 @@ static SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops,
static struct stm32_ops stm32mcu_dwmac_data = {
.set_mode = stm32mcu_set_mode,
- .suspend = stm32mcu_suspend,
.syscfg_eth_mask = SYSCFG_MCU_ETH_MASK
};
static struct stm32_ops stm32mp1_dwmac_data = {
.set_mode = stm32mp1_set_mode,
- .clk_prepare = stm32mp1_clk_prepare,
.suspend = stm32mp1_suspend,
.resume = stm32mp1_resume,
.parse_data = stm32mp1_parse_data,
--
2.42.0
On Tue, 2023-09-19 at 12:45 -0400, Ben Wolsieffer wrote:
> Currently, clock configuration is spread throughout the driver and
> partially duplicated for the STM32MP1 and STM32 MCU variants. This makes
> it difficult to keep track of which clocks need to be enabled or disabled
> in various scenarios.
>
> This patch adds symmetric stm32_dwmac_clk_enable/disable() functions
> that handle all clock configuration, including quirks required while
> suspending or resuming. syscfg_clk and clk_eth_ck are not present on
> STM32 MCUs, but it is fine to try to configure them anyway since NULL
> clocks are ignored.
>
> Signed-off-by: Ben Wolsieffer <[email protected]>
This patch is for net-next, while the previous one targets the -net
tree: you can't bundle them in a single series. Please re-post the
first one individually, specifying the target tree into the subj.
If there is a code dependency, you can post this one for net-next,
after -net is merged back into -net-next (Usually within a week since
the net patch is applied).
Cheers,
Paolo