clk: qcom: rpmh: Set wake/sleep state for BCM clks

[PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Stephen Boyd 4 years ago

Set the wake and sleep state for BCM clks here, not just the active
state, as the active only state is dropped when CPUs go to deep idle.
This ensures the clk is always on when the driver thinks it is on.

This was found by inspection, and could very well be incorrect if the
RPMh hardware copies over the active only state to the sleep and wake
states.

Cc: Alex Elder <elder@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Fixes: 04053f4d23a4 ("clk: qcom: clk-rpmh: Add IPA clock support")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/clk/qcom/clk-rpmh.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
index aed907982344..29da1ffd10cf 100644
--- a/drivers/clk/qcom/clk-rpmh.c
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -260,6 +260,7 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
 	struct tcs_cmd cmd = { 0 };
 	u32 cmd_state;
 	int ret = 0;
+	enum rpmh_state state;
 
 	mutex_lock(&rpmh_clk_lock);
 	if (enable) {
@@ -274,15 +275,19 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
 		cmd.addr = c->res_addr;
 		cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
 
-		ret = clk_rpmh_send(c, RPMH_ACTIVE_ONLY_STATE, &cmd, enable);
-		if (ret) {
-			dev_err(c->dev, "set active state of %s failed: (%d)\n",
-				c->res_name, ret);
-		} else {
-			c->last_sent_aggr_state = cmd_state;
+		for (state = RPMH_SLEEP_STATE; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
+			ret = clk_rpmh_send(c, state, &cmd, enable);
+			if (ret) {
+				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
+					!state ? "sleep" :
+					state == RPMH_WAKE_ONLY_STATE	?
+					"wake" : "active", c->res_name, ret);
+				goto out;
+			}
 		}
+		c->last_sent_aggr_state = cmd_state;
 	}
-
+out:
 	mutex_unlock(&rpmh_clk_lock);
 
 	return ret;

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
https://chromeos.dev

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Bjorn Andersson 4 years ago

On Tue 12 Apr 14:45 CDT 2022, Stephen Boyd wrote:

> Set the wake and sleep state for BCM clks here, not just the active
> state, as the active only state is dropped when CPUs go to deep idle.
> This ensures the clk is always on when the driver thinks it is on.
> 
> This was found by inspection, and could very well be incorrect if the
> RPMh hardware copies over the active only state to the sleep and wake
> states.
> 

Taking another look at this patch and now it makes perfect sense to me.
Sorry for not grasping the problem earlier.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Will you take this in fixes, or do you want me to pick it for 5.19?

> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Fixes: 04053f4d23a4 ("clk: qcom: clk-rpmh: Add IPA clock support")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/clk/qcom/clk-rpmh.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index aed907982344..29da1ffd10cf 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -260,6 +260,7 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>  	struct tcs_cmd cmd = { 0 };
>  	u32 cmd_state;
>  	int ret = 0;
> +	enum rpmh_state state;
>  
>  	mutex_lock(&rpmh_clk_lock);
>  	if (enable) {
> @@ -274,15 +275,19 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>  		cmd.addr = c->res_addr;
>  		cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
>  
> -		ret = clk_rpmh_send(c, RPMH_ACTIVE_ONLY_STATE, &cmd, enable);
> -		if (ret) {
> -			dev_err(c->dev, "set active state of %s failed: (%d)\n",
> -				c->res_name, ret);
> -		} else {
> -			c->last_sent_aggr_state = cmd_state;
> +		for (state = RPMH_SLEEP_STATE; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
> +			ret = clk_rpmh_send(c, state, &cmd, enable);

Nit. We only need to pass the positive enable on the last iteration here...

Regards,
Bjorn

> +			if (ret) {
> +				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> +					!state ? "sleep" :
> +					state == RPMH_WAKE_ONLY_STATE	?
> +					"wake" : "active", c->res_name, ret);
> +				goto out;
> +			}
>  		}
> +		c->last_sent_aggr_state = cmd_state;
>  	}
> -
> +out:
>  	mutex_unlock(&rpmh_clk_lock);
>  
>  	return ret;
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> -- 
> https://chromeos.dev
>

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Stephen Boyd 4 years ago

Quoting Bjorn Andersson (2022-05-04 09:53:54)
> On Tue 12 Apr 14:45 CDT 2022, Stephen Boyd wrote:
>
> > Set the wake and sleep state for BCM clks here, not just the active
> > state, as the active only state is dropped when CPUs go to deep idle.
> > This ensures the clk is always on when the driver thinks it is on.
> >
> > This was found by inspection, and could very well be incorrect if the
> > RPMh hardware copies over the active only state to the sleep and wake
> > states.
> >
>
> Taking another look at this patch and now it makes perfect sense to me.
> Sorry for not grasping the problem earlier.
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
>
> Will you take this in fixes, or do you want me to pick it for 5.19?
>

I'm waiting for Taniya to reply. For all I know this has no effect
because there's some sort of copy/paste from one state to another. Until
then it doesn't seem like we should do anything.

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Stephen Boyd 3 years, 12 months ago

Quoting Stephen Boyd (2022-05-06 16:24:15)
> Quoting Bjorn Andersson (2022-05-04 09:53:54)
> > On Tue 12 Apr 14:45 CDT 2022, Stephen Boyd wrote:
> >
> > > Set the wake and sleep state for BCM clks here, not just the active
> > > state, as the active only state is dropped when CPUs go to deep idle.
> > > This ensures the clk is always on when the driver thinks it is on.
> > >
> > > This was found by inspection, and could very well be incorrect if the
> > > RPMh hardware copies over the active only state to the sleep and wake
> > > states.
> > >
> >
> > Taking another look at this patch and now it makes perfect sense to me.
> > Sorry for not grasping the problem earlier.
> >
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
> >
> > Will you take this in fixes, or do you want me to pick it for 5.19?
> >
>
> I'm waiting for Taniya to reply. For all I know this has no effect
> because there's some sort of copy/paste from one state to another. Until
> then it doesn't seem like we should do anything.

Taniya told me that if there's no sleep or wake state set then active
state remains even when the subsystem is in sleep. Not exactly
copy/paste but at least it is consistent. We need a comment here so this
doesn't come up again.

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Alex Elder 3 years, 12 months ago

On 5/11/22 5:04 PM, Stephen Boyd wrote:
>>> Will you take this in fixes, or do you want me to pick it for 5.19?
>>>
>> I'm waiting for Taniya to reply. For all I know this has no effect
>> because there's some sort of copy/paste from one state to another. Until
>> then it doesn't seem like we should do anything.
> Taniya told me that if there's no sleep or wake state set then active
> state remains even when the subsystem is in sleep. Not exactly
> copy/paste but at least it is consistent. We need a comment here so this
> doesn't come up again.

If I understand what you're saying here, your original patch is not
necessary, but there should be a comment in the code that explains
why that is the case.  Is that right?  And am I right to assume you
will be sending out a patch with such a comment?

					-Alex

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Stephen Boyd 3 years, 11 months ago

Quoting Alex Elder (2022-05-12 08:15:31)
> On 5/11/22 5:04 PM, Stephen Boyd wrote:
> >>> Will you take this in fixes, or do you want me to pick it for 5.19?
> >>>
> >> I'm waiting for Taniya to reply. For all I know this has no effect
> >> because there's some sort of copy/paste from one state to another. Until
> >> then it doesn't seem like we should do anything.
> > Taniya told me that if there's no sleep or wake state set then active
> > state remains even when the subsystem is in sleep. Not exactly
> > copy/paste but at least it is consistent. We need a comment here so this
> > doesn't come up again.
>
> If I understand what you're saying here, your original patch is not
> necessary, but there should be a comment in the code that explains
> why that is the case.  Is that right?  And am I right to assume you
> will be sending out a patch with such a comment?
>

Yes.

Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

Posted by Alex Elder 4 years ago

On 4/12/22 2:45 PM, Stephen Boyd wrote:
> Set the wake and sleep state for BCM clks here, not just the active
> state, as the active only state is dropped when CPUs go to deep idle.
> This ensures the clk is always on when the driver thinks it is on.
> 
> This was found by inspection, and could very well be incorrect if the
> RPMh hardware copies over the active only state to the sleep and wake
> states.

I ran with this code and found no obvious errors.  And
it looks reasonable to me.

However someone (probably Taniya) needs to comment on
whether it's the right thing to do (based on Stephen's
second paragraph above), or if not, why not.  It might
warrant an additional comment also.

I can add a "Tested-by" but I'm going to wait for
some more input.

					-Alex

> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Fixes: 04053f4d23a4 ("clk: qcom: clk-rpmh: Add IPA clock support")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   drivers/clk/qcom/clk-rpmh.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index aed907982344..29da1ffd10cf 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -260,6 +260,7 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>   	struct tcs_cmd cmd = { 0 };
>   	u32 cmd_state;
>   	int ret = 0;
> +	enum rpmh_state state;
>   
>   	mutex_lock(&rpmh_clk_lock);
>   	if (enable) {
> @@ -274,15 +275,19 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>   		cmd.addr = c->res_addr;
>   		cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
>   
> -		ret = clk_rpmh_send(c, RPMH_ACTIVE_ONLY_STATE, &cmd, enable);
> -		if (ret) {
> -			dev_err(c->dev, "set active state of %s failed: (%d)\n",
> -				c->res_name, ret);
> -		} else {
> -			c->last_sent_aggr_state = cmd_state;
> +		for (state = RPMH_SLEEP_STATE; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
> +			ret = clk_rpmh_send(c, state, &cmd, enable);
> +			if (ret) {
> +				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> +					!state ? "sleep" :
> +					state == RPMH_WAKE_ONLY_STATE	?
> +					"wake" : "active", c->res_name, ret);
> +				goto out;
> +			}
>   		}
> +		c->last_sent_aggr_state = cmd_state;
>   	}
> -
> +out:
>   	mutex_unlock(&rpmh_clk_lock);
>   
>   	return ret;
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17