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

[Matthias Brugger]

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@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
> > > 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[] = {