* [PATCH net v3 0/1] stmmac: dwmac-mediatek: fix clock issue @ 2022-07-08 8:39 Biao Huang 2022-07-08 8:39 ` [PATCH net v3] " Biao Huang 0 siblings, 1 reply; 8+ messages in thread From: Biao Huang @ 2022-07-08 8:39 UTC (permalink / raw) To: David Miller, Matthias Brugger Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, Biao Huang, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin changes in v3: 1. delete mediatek_dwmac_exit() since there is no operation in it, as Matthias's comments. changes in v2: 1. clock configuration is still needed in probe, and invoke mediatek_dwmac_clks_config() instead. 2. update commit message. v1: remove duplicated clock configuration in init/exit. Biao Huang (1): stmmac: dwmac-mediatek: fix clock issue .../ethernet/stmicro/stmmac/dwmac-mediatek.c | 36 +++++-------------- 1 file changed, 9 insertions(+), 27 deletions(-) -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 8:39 [PATCH net v3 0/1] stmmac: dwmac-mediatek: fix clock issue Biao Huang @ 2022-07-08 8:39 ` Biao Huang 2022-07-08 9:22 ` Matthias Brugger 2022-07-11 22:31 ` Jakub Kicinski 0 siblings, 2 replies; 8+ messages in thread From: Biao Huang @ 2022-07-08 8:39 UTC (permalink / raw) To: David Miller, Matthias Brugger Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, Biao Huang, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin 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. Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform level clocks management") Signed-off-by: Biao Huang <biao.huang@mediatek.com> --- .../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 b/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", ret); - 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; - -err_clk: - clk_bulk_disable_unprepare(variant->num_clks, plat->clks); - return ret; -} - -static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) -{ - 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->dwmac_fix_mac_speed; @@ -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; + +err_drv_probe: + mediatek_dwmac_clks_config(priv_plat, false); + return ret; } static const struct of_device_id mediatek_dwmac_match[] = { -- 2.25.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 8:39 ` [PATCH net v3] " Biao Huang @ 2022-07-08 9:22 ` Matthias Brugger 2022-07-08 9:46 ` Biao Huang 2022-07-11 22:31 ` Jakub Kicinski 1 sibling, 1 reply; 8+ messages in thread From: Matthias Brugger @ 2022-07-08 9:22 UTC (permalink / raw) To: Biao Huang, David Miller Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin 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 function. 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? Regards, Matthias > Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform level clocks management") > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > --- > .../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 b/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", ret); > - 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; > - > -err_clk: > - clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > - return ret; > -} > - > -static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) > -{ > - 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->dwmac_fix_mac_speed; > @@ -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; > + > +err_drv_probe: > + mediatek_dwmac_clks_config(priv_plat, false); > + return ret; > } > > static const struct of_device_id mediatek_dwmac_match[] = { _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 9:22 ` Matthias Brugger @ 2022-07-08 9:46 ` Biao Huang 2022-07-08 11:37 ` Matthias Brugger 0 siblings, 1 reply; 8+ messages in thread From: Biao Huang @ 2022-07-08 9:46 UTC (permalink / raw) To: Matthias Brugger, David Miller Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin 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 > function. 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. > > 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 --> pm_runtime_force_suspend/pm_runtime_force_resume--> mediatek_dwmac_clks_config, so old clock handling in init/exit is no longer a proper choice. Best Regards! Biao > > Regards, > Matthias > > > Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform level > > clocks management") > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > > --- > > .../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 > > b/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", > > ret); > > - 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; > > - > > -err_clk: > > - clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > > - return ret; > > -} > > - > > -static void mediatek_dwmac_exit(struct platform_device *pdev, void > > *priv) > > -{ > > - 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- > > >dwmac_fix_mac_speed; > > @@ -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; > > + > > +err_drv_probe: > > + mediatek_dwmac_clks_config(priv_plat, false); > > + return ret; > > } > > > > static const struct of_device_id mediatek_dwmac_match[] = { _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 9:46 ` Biao Huang @ 2022-07-08 11:37 ` Matthias Brugger 2022-07-12 1:10 ` Biao Huang 0 siblings, 1 reply; 8+ messages in thread From: Matthias Brugger @ 2022-07-08 11:37 UTC (permalink / raw) To: Biao Huang, David Miller Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin 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 >> function. > 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 --> > pm_runtime_force_suspend/pm_runtime_force_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, Matthias > Best Regards! > Biao > >> >> Regards, >> Matthias >> >>> Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform level >>> clocks management") >>> Signed-off-by: Biao Huang <biao.huang@mediatek.com> >>> --- >>> .../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 >>> b/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", >>> ret); >>> - 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; >>> - >>> -err_clk: >>> - clk_bulk_disable_unprepare(variant->num_clks, plat->clks); >>> - return ret; >>> -} >>> - >>> -static void mediatek_dwmac_exit(struct platform_device *pdev, void >>> *priv) >>> -{ >>> - 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- >>>> dwmac_fix_mac_speed; >>> @@ -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; >>> + >>> +err_drv_probe: >>> + mediatek_dwmac_clks_config(priv_plat, false); >>> + return ret; >>> } >>> >>> static const struct of_device_id mediatek_dwmac_match[] = { > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 11:37 ` Matthias Brugger @ 2022-07-12 1:10 ` Biao Huang 0 siblings, 0 replies; 8+ messages in thread From: Biao Huang @ 2022-07-12 1:10 UTC (permalink / raw) To: Matthias Brugger, David Miller Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin Dear Matthias, Thanks for your comments. On Fri, 2022-07-08 at 13:37 +0200, Matthias Brugger wrote: > 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 > > > function. > > > > 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. Yes, I'll rewrite the commit message and test remove function, if need any modification, will add them in next send. Best Regards! Biao > > > > > > > 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 --> > > pm_runtime_force_suspend/pm_runtime_force_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, > Matthias > > > Best Regards! > > Biao > > > > > > > > Regards, > > > Matthias > > > > > > > Fixes: 3186bdad97d5 ("stmmac: dwmac-mediatek: add platform > > > > level > > > > clocks management") > > > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > > > > --- > > > > .../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 > > > > b/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", > > > > ret); > > > > - 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; > > > > - > > > > -err_clk: > > > > - clk_bulk_disable_unprepare(variant->num_clks, plat- > > > > >clks); > > > > - return ret; > > > > -} > > > > - > > > > -static void mediatek_dwmac_exit(struct platform_device *pdev, > > > > void > > > > *priv) > > > > -{ > > > > - 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- > > > > > dwmac_fix_mac_speed; > > > > > > > > @@ -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; > > > > + > > > > +err_drv_probe: > > > > + mediatek_dwmac_clks_config(priv_plat, false); > > > > + return ret; > > > > } > > > > > > > > static const struct of_device_id mediatek_dwmac_match[] = { _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-08 8:39 ` [PATCH net v3] " Biao Huang 2022-07-08 9:22 ` Matthias Brugger @ 2022-07-11 22:31 ` Jakub Kicinski 2022-07-12 1:11 ` Biao Huang 1 sibling, 1 reply; 8+ messages in thread From: Jakub Kicinski @ 2022-07-11 22:31 UTC (permalink / raw) To: Biao Huang Cc: David Miller, Matthias Brugger, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin On Fri, 8 Jul 2022 16:39:37 +0800 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. Please improve the commit message so that it answers the questions Matthias had and repost. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue 2022-07-11 22:31 ` Jakub Kicinski @ 2022-07-12 1:11 ` Biao Huang 0 siblings, 0 replies; 8+ messages in thread From: Biao Huang @ 2022-07-12 1:11 UTC (permalink / raw) To: Jakub Kicinski Cc: David Miller, Matthias Brugger, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Eric Dumazet, Paolo Abeni, Maxime Coquelin, AngeloGioacchino Del Regno, netdev, linux-stm32, linux-arm-kernel, linux-kernel, linux-mediatek, macpaul.lin Dear Jakub, Thanks for your comments. On Mon, 2022-07-11 at 15:31 -0700, Jakub Kicinski wrote: > On Fri, 8 Jul 2022 16:39:37 +0800 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. > > Please improve the commit message so that it answers the questions > Matthias had and repost. OK, I'll improve the commit message in next send. Best Regards! Biao _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-07-12 1:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-08 8:39 [PATCH net v3 0/1] stmmac: dwmac-mediatek: fix clock issue Biao Huang 2022-07-08 8:39 ` [PATCH net v3] " Biao Huang 2022-07-08 9:22 ` Matthias Brugger 2022-07-08 9:46 ` Biao Huang 2022-07-08 11:37 ` Matthias Brugger 2022-07-12 1:10 ` Biao Huang 2022-07-11 22:31 ` Jakub Kicinski 2022-07-12 1:11 ` Biao Huang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).