Date:   Fri, 8 Jul 2022 13:37:59 +0200
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     Biao Huang <biao.huang@...iatek.com>,
        David Miller <davem@...emloft.net>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, netdev@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, macpaul.lin@...iatek.com
Subject: Re: [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@...iatek.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[] = {
> 

Powered by blists - more mailing lists