Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue

From: Matthias Brugger
Date: Fri Jul 08 2022 - 07:38:15 EST

Hi Biao Huang,

On 08/07/2022 11:46, Biao Huang wrote:
Dear Mattias,
Thanks for your comments.

On Fri, 2022-07-08 at 11:22 +0200, Matthias Brugger wrote:

On 08/07/2022 10:39, Biao Huang wrote:
Since clocks are handled in mediatek_dwmac_clks_config(),
remove the clocks configuration in init()/exit(), and
invoke mediatek_dwmac_clks_config instead.

This issue is found in suspend/resume test.

Commit message is rather confusing. Basically you are moving the
clock enable
into probe instead of init and remove it from exit. That means,
clocks get
enabled earlier and don't get disabled if the module gets unloaded.
That doesn't
sound correct, I think we would at least need to disable the clocks
in remove
there is pm_runtime support in driver, and clocks will be
disabled/enabled in stmmac_runtime_suspend/stmmac_runtime_resume.

stmmac_dvr_probe() invoke pm_runtime_put(device) at the end, and
disable clocks, but no clock enable at the beginning.
so vendor's probe entry should enable clocks to ensure normal behavior.

As to clocks in remove function, we did not test it
We should implement a vendor specified remove function who will take
care of clocks rather than invoke stmmac_pltfr_remove directly.

Anyway, we miss the clock handling case in remove function,
and will
test it and feed back.

Right, sorry I'm not familiar with the stmmac driver stack, yes suspend/resume is fine. Thanks for clarification.

stmmac_pltfr_remove will disable stmmac_clk and pclk but not the rest of the clocks. So I think you will need to have specific remove function to disable them.

I suppose that suspend calls exit and that there was a problem when
we disable
the clocks there. Is this a HW issue that has no other possible fix?
Not a HW issue. suspend/resume will disable/enable clocks by invoking
stmmac_pltfr_noirq_suspend/stmmac_pltfr_noirq_resume -->
mediatek_dwmac_clks_config, so old clock handling in init/exit is no
longer a proper choice.

Got it, thanks for clarification.

Best regards,

Best Regards!


Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform level
clocks management")
Signed-off-by: Biao Huang <biao.huang@xxxxxxxxxxxx>
.../ethernet/stmicro/stmmac/dwmac-mediatek.c | 36 +++++---------
1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
index 6ff88df58767..e86f3e125cb4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
@@ -576,32 +576,7 @@ static int mediatek_dwmac_init(struct
platform_device *pdev, void *priv)
- ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks);
- if (ret) {
- dev_err(plat->dev, "failed to enable clks, err = %d\n",
- return ret;
- }
- ret = clk_prepare_enable(plat->rmii_internal_clk);
- if (ret) {
- dev_err(plat->dev, "failed to enable rmii internal clk,
err = %d\n", ret);
- goto err_clk;
- }
return 0;
- clk_bulk_disable_unprepare(variant->num_clks, plat->clks);
- return ret;
-static void mediatek_dwmac_exit(struct platform_device *pdev, void
- struct mediatek_dwmac_plat_data *plat = priv;
- const struct mediatek_dwmac_variant *variant = plat->variant;
- clk_disable_unprepare(plat->rmii_internal_clk);
- clk_bulk_disable_unprepare(variant->num_clks, plat->clks);
static int mediatek_dwmac_clks_config(void *priv, bool enabled)
@@ -643,7 +618,6 @@ static int mediatek_dwmac_common_data(struct
platform_device *pdev,
plat->addr64 = priv_plat->variant->dma_bit_mask;
plat->bsp_priv = priv_plat;
plat->init = mediatek_dwmac_init;
- plat->exit = mediatek_dwmac_exit;
plat->clks_config = mediatek_dwmac_clks_config;
if (priv_plat->variant->dwmac_fix_mac_speed)
plat->fix_mac_speed = priv_plat->variant-
@@ -712,13 +686,21 @@ static int mediatek_dwmac_probe(struct
platform_device *pdev)
mediatek_dwmac_common_data(pdev, plat_dat, priv_plat);
mediatek_dwmac_init(pdev, priv_plat);
+ ret = mediatek_dwmac_clks_config(priv_plat, true);
+ if (ret)
+ return ret;
ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
if (ret) {
stmmac_remove_config_dt(pdev, plat_dat);
- return ret;
+ goto err_drv_probe;
return 0;
+ mediatek_dwmac_clks_config(priv_plat, false);
+ return ret;
static const struct of_device_id mediatek_dwmac_match[] = {