[PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[Srinivas Kandagatla]

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.0

Re: [PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[Stephen Boyd]

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);
>

Re: [PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[Srinivas Kandagatla]

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);
>>

Re: [PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[Stephen Boyd]

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.

Re: [PATCH] clk: qcom: gfm-mux: use runtime pm while accessing registers

[Amit Pundir]

On 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
>