drivers/clk/qcom/lpass-gfm-sm8250.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)
gfm mux driver does support runtime pm but we never use it while
accessing registers. Looks like this driver was getting lucky and
totally depending on other drivers to leave the clk on.
Fix this by doing runtime pm while accessing registers.
Fixes: a2d8f507803e ("clk: qcom: Add support to LPASS AUDIO_CC Glitch Free Mux clocks")
Cc: stable@vger.kernel.org
Reported-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/clk/qcom/lpass-gfm-sm8250.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/lpass-gfm-sm8250.c b/drivers/clk/qcom/lpass-gfm-sm8250.c
index 96f476f24eb2..bcf0ea534f7f 100644
--- a/drivers/clk/qcom/lpass-gfm-sm8250.c
+++ b/drivers/clk/qcom/lpass-gfm-sm8250.c
@@ -38,14 +38,37 @@ struct clk_gfm {
static u8 clk_gfm_get_parent(struct clk_hw *hw)
{
struct clk_gfm *clk = to_clk_gfm(hw);
+ int ret;
+ u8 parent;
+
+ ret = pm_runtime_resume_and_get(clk->priv->dev);
+ if (ret < 0 && ret != -EACCES) {
+ dev_err_ratelimited(clk->priv->dev,
+ "pm_runtime_resume_and_get failed in %s, ret %d\n",
+ __func__, ret);
+ return ret;
+ }
+
+ parent = readl(clk->gfm_mux) & clk->mux_mask;
+
+ pm_runtime_mark_last_busy(clk->priv->dev);
- return readl(clk->gfm_mux) & clk->mux_mask;
+ return parent;
}
static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
{
struct clk_gfm *clk = to_clk_gfm(hw);
unsigned int val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(clk->priv->dev);
+ if (ret < 0 && ret != -EACCES) {
+ dev_err_ratelimited(clk->priv->dev,
+ "pm_runtime_resume_and_get failed in %s, ret %d\n",
+ __func__, ret);
+ return ret;
+ }
val = readl(clk->gfm_mux);
@@ -57,6 +80,8 @@ static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
writel(val, clk->gfm_mux);
+ pm_runtime_mark_last_busy(clk->priv->dev);
+
return 0;
}
@@ -251,6 +276,8 @@ static int lpass_gfm_clk_driver_probe(struct platform_device *pdev)
if (IS_ERR(cc->base))
return PTR_ERR(cc->base);
+ cc->dev = dev;
+
err = devm_pm_runtime_enable(dev);
if (err)
return err;
--
2.21.0On Tue, 21 Mar 2023 at 23:28, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> gfm mux driver does support runtime pm but we never use it while
> accessing registers. Looks like this driver was getting lucky and
> totally depending on other drivers to leave the clk on.
>
> Fix this by doing runtime pm while accessing registers.
Thank you Srini, this fixes the boot regression I see on the RB5
booting v6.1.y and v6.3-rc kernel versions.
Tested-by: Amit Pundir <amit.pundir@linaro.org>
>
> Fixes: a2d8f507803e ("clk: qcom: Add support to LPASS AUDIO_CC Glitch Free Mux clocks")
> Cc: stable@vger.kernel.org
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/clk/qcom/lpass-gfm-sm8250.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/lpass-gfm-sm8250.c b/drivers/clk/qcom/lpass-gfm-sm8250.c
> index 96f476f24eb2..bcf0ea534f7f 100644
> --- a/drivers/clk/qcom/lpass-gfm-sm8250.c
> +++ b/drivers/clk/qcom/lpass-gfm-sm8250.c
> @@ -38,14 +38,37 @@ struct clk_gfm {
> static u8 clk_gfm_get_parent(struct clk_hw *hw)
> {
> struct clk_gfm *clk = to_clk_gfm(hw);
> + int ret;
> + u8 parent;
> +
> + ret = pm_runtime_resume_and_get(clk->priv->dev);
> + if (ret < 0 && ret != -EACCES) {
> + dev_err_ratelimited(clk->priv->dev,
> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + parent = readl(clk->gfm_mux) & clk->mux_mask;
> +
> + pm_runtime_mark_last_busy(clk->priv->dev);
>
> - return readl(clk->gfm_mux) & clk->mux_mask;
> + return parent;
> }
>
> static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
> {
> struct clk_gfm *clk = to_clk_gfm(hw);
> unsigned int val;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(clk->priv->dev);
> + if (ret < 0 && ret != -EACCES) {
> + dev_err_ratelimited(clk->priv->dev,
> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
> + __func__, ret);
> + return ret;
> + }
>
> val = readl(clk->gfm_mux);
>
> @@ -57,6 +80,8 @@ static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
>
> writel(val, clk->gfm_mux);
>
> + pm_runtime_mark_last_busy(clk->priv->dev);
> +
> return 0;
> }
>
> @@ -251,6 +276,8 @@ static int lpass_gfm_clk_driver_probe(struct platform_device *pdev)
> if (IS_ERR(cc->base))
> return PTR_ERR(cc->base);
>
> + cc->dev = dev;
> +
> err = devm_pm_runtime_enable(dev);
> if (err)
> return err;
> --
> 2.21.0
>
Quoting Srinivas Kandagatla (2023-03-21 10:57:58)
> gfm mux driver does support runtime pm but we never use it while
> accessing registers. Looks like this driver was getting lucky and
> totally depending on other drivers to leave the clk on.
>
> Fix this by doing runtime pm while accessing registers.
>
> Fixes: a2d8f507803e ("clk: qcom: Add support to LPASS AUDIO_CC Glitch Free Mux clocks")
> Cc: stable@vger.kernel.org
> Reported-by: Amit Pundir <amit.pundir@linaro.org>
Is there a link to the report?
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/clk/qcom/lpass-gfm-sm8250.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/lpass-gfm-sm8250.c b/drivers/clk/qcom/lpass-gfm-sm8250.c
> index 96f476f24eb2..bcf0ea534f7f 100644
> --- a/drivers/clk/qcom/lpass-gfm-sm8250.c
> +++ b/drivers/clk/qcom/lpass-gfm-sm8250.c
> @@ -38,14 +38,37 @@ struct clk_gfm {
> static u8 clk_gfm_get_parent(struct clk_hw *hw)
> {
> struct clk_gfm *clk = to_clk_gfm(hw);
> + int ret;
> + u8 parent;
> +
> + ret = pm_runtime_resume_and_get(clk->priv->dev);
> + if (ret < 0 && ret != -EACCES) {
> + dev_err_ratelimited(clk->priv->dev,
> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + parent = readl(clk->gfm_mux) & clk->mux_mask;
> +
> + pm_runtime_mark_last_busy(clk->priv->dev);
>
> - return readl(clk->gfm_mux) & clk->mux_mask;
> + return parent;
> }
>
> static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
> {
> struct clk_gfm *clk = to_clk_gfm(hw);
> unsigned int val;
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(clk->priv->dev);
Doesn't the clk framework already do this? Why do we need to do it
again?
> + if (ret < 0 && ret != -EACCES) {
> + dev_err_ratelimited(clk->priv->dev,
> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
> + __func__, ret);
> + return ret;
> + }
>
> val = readl(clk->gfm_mux);
>
On 21/03/2023 18:46, Stephen Boyd wrote:
> Quoting Srinivas Kandagatla (2023-03-21 10:57:58)
>> gfm mux driver does support runtime pm but we never use it while
>> accessing registers. Looks like this driver was getting lucky and
>> totally depending on other drivers to leave the clk on.
>>
>> Fix this by doing runtime pm while accessing registers.
>>
>> Fixes: a2d8f507803e ("clk: qcom: Add support to LPASS AUDIO_CC Glitch Free Mux clocks")
>> Cc: stable@vger.kernel.org
>> Reported-by: Amit Pundir <amit.pundir@linaro.org>
>
> Is there a link to the report?
https://www.spinics.net/lists/stable/msg638380.html
>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/clk/qcom/lpass-gfm-sm8250.c | 29 ++++++++++++++++++++++++++++-
>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/lpass-gfm-sm8250.c b/drivers/clk/qcom/lpass-gfm-sm8250.c
>> index 96f476f24eb2..bcf0ea534f7f 100644
>> --- a/drivers/clk/qcom/lpass-gfm-sm8250.c
>> +++ b/drivers/clk/qcom/lpass-gfm-sm8250.c
>> @@ -38,14 +38,37 @@ struct clk_gfm {
>> static u8 clk_gfm_get_parent(struct clk_hw *hw)
>> {
>> struct clk_gfm *clk = to_clk_gfm(hw);
>> + int ret;
>> + u8 parent;
>> +
>> + ret = pm_runtime_resume_and_get(clk->priv->dev);
>> + if (ret < 0 && ret != -EACCES) {
>> + dev_err_ratelimited(clk->priv->dev,
>> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>> +
>> + parent = readl(clk->gfm_mux) & clk->mux_mask;
>> +
>> + pm_runtime_mark_last_busy(clk->priv->dev);
>>
>> - return readl(clk->gfm_mux) & clk->mux_mask;
>> + return parent;
>> }
>>
>> static int clk_gfm_set_parent(struct clk_hw *hw, u8 index)
>> {
>> struct clk_gfm *clk = to_clk_gfm(hw);
>> unsigned int val;
>> + int ret;
>> +
>> + ret = pm_runtime_resume_and_get(clk->priv->dev);
>
> Doesn't the clk framework already do this? Why do we need to do it
> again?
You are right, clk core already does do pm_runtime_resume_and_get for
set_parent.
this looks redundant here.
so we need only need to add this for get_parent
--srini
>
>> + if (ret < 0 && ret != -EACCES) {
>> + dev_err_ratelimited(clk->priv->dev,
>> + "pm_runtime_resume_and_get failed in %s, ret %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>>
>> val = readl(clk->gfm_mux);
>>
Quoting Srinivas Kandagatla (2023-03-21 13:33:49)
>
>
> On 21/03/2023 18:46, Stephen Boyd wrote:
> > Quoting Srinivas Kandagatla (2023-03-21 10:57:58)
> >> gfm mux driver does support runtime pm but we never use it while
> >> accessing registers. Looks like this driver was getting lucky and
> >> totally depending on other drivers to leave the clk on.
> >>
> >> Fix this by doing runtime pm while accessing registers.
> >>
> >> Fixes: a2d8f507803e ("clk: qcom: Add support to LPASS AUDIO_CC Glitch Free Mux clocks")
> >> Cc: stable@vger.kernel.org
> >> Reported-by: Amit Pundir <amit.pundir@linaro.org>
> >
> > Is there a link to the report?
>
> https://www.spinics.net/lists/stable/msg638380.html
Please add a Link: after the reported-by and use a lore link instead of
spinics please.
> >> {
> >> struct clk_gfm *clk = to_clk_gfm(hw);
> >> unsigned int val;
> >> + int ret;
> >> +
> >> + ret = pm_runtime_resume_and_get(clk->priv->dev);
> >
> > Doesn't the clk framework already do this? Why do we need to do it
> > again?
>
> You are right, clk core already does do pm_runtime_resume_and_get for
> set_parent.
>
> this looks redundant here.
>
>
> so we need only need to add this for get_parent
>
The get_parent() clk op is called from a couple places in the clk
framework. I guess that you're getting called from
clk_core_reparent_orphans() where the runtime PM count isn't
incremented? Can you confirm? Either way, this should be fixed in the
framework and not in the driver.
© 2016 - 2026 Red Hat, Inc.