Messages in this thread | | | Date | Tue, 31 Jan 2023 14:19:11 +0100 | Subject | Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs | From | Matthias Brugger <> |
| |
On 11/01/2023 08:45, Roger Lu wrote: > In MediaTek HW design, svs and thermal both use the same clk source. > It means that svs clk reference count from CCF includes thermal control > counts. That makes svs driver confuse whether it disabled svs's main clk > or not from CCF's perspective and lead to turn off their shared clk > unexpectedly. Therefore, we add svs clk control APIs to make sure svs's > main clk is controlled well by svs driver itself. > > Here is a NG example. Rely on CCF's reference count and cause problem. > > thermal probe (clk ref = 1) > -> svs probe (clk ref = 2) > -> svs suspend (clk ref = 1) > -> thermal suspend (clk ref = 0) > -> thermal resume (clk ref = 1) > -> svs resume (encounter error, clk ref = 1) > -> svs suspend (clk ref = 0) > -> thermal suspend (Fail here, thermal HW control w/o clk) > > Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()") > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
That looks wrong. Although I don't out of my mind, there should be a way to tell the clock framework that this clock is shared between several devices.
I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't be enough.
Regards, Matthias
> --- > drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++-------- > 1 file changed, 44 insertions(+), 13 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c > index 9575aa645643..830263bad81e 100644 > --- a/drivers/soc/mediatek/mtk-svs.c > +++ b/drivers/soc/mediatek/mtk-svs.c > @@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = { > * @bank_max: total number of svs banks > * @efuse: svs efuse data received from NVMEM framework > * @tefuse: thermal efuse data received from NVMEM framework > + * @clk_cnt: clock count shows the clk enable/disable times by svs driver > */ > struct svs_platform { > char *name; > @@ -343,6 +344,7 @@ struct svs_platform { > u32 bank_max; > u32 *efuse; > u32 *tefuse; > + s32 clk_cnt; > }; > > struct svs_platform_data { > @@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp) > svs_writel_relaxed(svsp, svsb->core_sel, CORESEL); > } > > +static bool svs_is_clk_enabled(struct svs_platform *svsp) > +{ > + return svsp->clk_cnt > 0 ? true : false; > +} > + > +static int svs_clk_enable(struct svs_platform *svsp) > +{ > + int ret; > + > + ret = clk_prepare_enable(svsp->main_clk); > + if (ret) { > + dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret); > + return ret; > + } > + > + svsp->clk_cnt++; > + > + return 0; > +} > + > +static void svs_clk_disable(struct svs_platform *svsp) > +{ > + clk_disable_unprepare(svsp->main_clk); > + svsp->clk_cnt--; > +} > + > static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step, > u32 svsb_volt_base) > { > @@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev) > int ret; > u32 idx; > > + if (!svs_is_clk_enabled(svsp)) { > + dev_err(svsp->dev, "svs clk is disabled already (%d)\n", > + svsp->clk_cnt); > + return 0; > + } > + > for (idx = 0; idx < svsp->bank_max; idx++) { > svsb = &svsp->banks[idx]; > > @@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev) > return ret; > } > > - clk_disable_unprepare(svsp->main_clk); > + svs_clk_disable(svsp); > > return 0; > } > @@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev) > struct svs_platform *svsp = dev_get_drvdata(dev); > int ret; > > - ret = clk_prepare_enable(svsp->main_clk); > - if (ret) { > - dev_err(svsp->dev, "cannot enable main_clk, disable svs\n"); > + ret = svs_clk_enable(svsp); > + if (ret) > return ret; > - } > > ret = reset_control_deassert(svsp->rst); > if (ret) { > dev_err(svsp->dev, "cannot deassert reset %d\n", ret); > - goto out_of_resume; > + goto svs_resume_clk_disable; > } > > ret = svs_init02(svsp); > @@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev) > dev_err(svsp->dev, "assert reset: %d\n", > reset_control_assert(svsp->rst)); > > -out_of_resume: > - clk_disable_unprepare(svsp->main_clk); > +svs_resume_clk_disable: > + svs_clk_disable(svsp); > + > return ret; > } > > @@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev) > goto svs_probe_free_resource; > } > > - ret = clk_prepare_enable(svsp->main_clk); > - if (ret) { > - dev_err(svsp->dev, "cannot enable main clk: %d\n", ret); > + ret = svs_clk_enable(svsp); > + if (ret) > goto svs_probe_free_resource; > - } > > svsp->base = of_iomap(svsp->dev->of_node, 0); > if (IS_ERR_OR_NULL(svsp->base)) { > @@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev) > iounmap(svsp->base); > > svs_probe_clk_disable: > - clk_disable_unprepare(svsp->main_clk); > + svs_clk_disable(svsp); > > svs_probe_free_resource: > if (!IS_ERR_OR_NULL(svsp->efuse))
| |