lkml.org 
[lkml]   [2022]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net v3] stmmac: dwmac-mediatek: fix clock issue
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[] = {
>

\
 
 \ /
  Last update: 2022-07-08 13:39    [W:0.057 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site