diff mbox

[2/2] cpufreq: brcmstb-avs-cpufreq: properly retrieve P-state upon suspend

Message ID 20161219201028.18885-3-code@mmayer.net (mailing list archive)
State Accepted, archived
Delegated to: Rafael Wysocki
Headers show

Commit Message

Markus Mayer Dec. 19, 2016, 8:10 p.m. UTC
From: Markus Mayer <mmayer@broadcom.com>

The AVS GET_PMAP command does return a P-state along with the P-map
information. However, that P-state is the initial P-state when the
P-map was first downloaded to AVS. It is *not* the current P-state.

Therefore, we explicitly retrieve the P-state using the GET_PSTATE
command.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Viresh Kumar Jan. 6, 2017, 4:11 a.m. UTC | #1
On 19-12-16, 12:10, Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> The AVS GET_PMAP command does return a P-state along with the P-map
> information. However, that P-state is the initial P-state when the
> P-map was first downloaded to AVS. It is *not* the current P-state.
> 
> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
> command.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index 2c6e325..c943606 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
>  {
>  	struct private_data *priv = policy->driver_data;
> +	int ret;
> +
> +	ret = brcm_avs_get_pmap(priv, &priv->pmap);
> +	if (ret)
> +		return ret;
>  
> -	return brcm_avs_get_pmap(priv, &priv->pmap);
> +	/*
> +	 * We can't use the P-state returned by brcm_avs_get_pmap(), since
> +	 * that's the initial P-state from when the P-map was downloaded to the
> +	 * AVS co-processor, not necessarily the P-state we are running at now.
> +	 * So, we get the current P-state explicitly.
> +	 */
> +	return brcm_avs_get_pstate(priv, &priv->pmap.state);
>  }
>  
>  static int brcm_avs_resume(struct cpufreq_policy *policy)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Markus Mayer Jan. 31, 2017, 6:53 p.m. UTC | #2
On 5 January 2017 at 20:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 19-12-16, 12:10, Markus Mayer wrote:
>> From: Markus Mayer <mmayer@broadcom.com>
>>
>> The AVS GET_PMAP command does return a P-state along with the P-map
>> information. However, that P-state is the initial P-state when the
>> P-map was first downloaded to AVS. It is *not* the current P-state.
>>
>> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
>> command.
>>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index 2c6e325..c943606 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
>>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
>>  {
>>       struct private_data *priv = policy->driver_data;
>> +     int ret;
>> +
>> +     ret = brcm_avs_get_pmap(priv, &priv->pmap);
>> +     if (ret)
>> +             return ret;
>>
>> -     return brcm_avs_get_pmap(priv, &priv->pmap);
>> +     /*
>> +      * We can't use the P-state returned by brcm_avs_get_pmap(), since
>> +      * that's the initial P-state from when the P-map was downloaded to the
>> +      * AVS co-processor, not necessarily the P-state we are running at now.
>> +      * So, we get the current P-state explicitly.
>> +      */
>> +     return brcm_avs_get_pstate(priv, &priv->pmap.state);
>>  }
>>
>>  static int brcm_avs_resume(struct cpufreq_policy *policy)
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Just wanted to follow up to see if this has been or will be picked up
for 4.10? I had a quick poke around some trees and did not see it
there.

Thanks,
-Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Feb. 1, 2017, 11:44 p.m. UTC | #3
On Tuesday, January 31, 2017 10:53:01 AM Markus Mayer wrote:
> On 5 January 2017 at 20:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 19-12-16, 12:10, Markus Mayer wrote:
> >> From: Markus Mayer <mmayer@broadcom.com>
> >>
> >> The AVS GET_PMAP command does return a P-state along with the P-map
> >> information. However, that P-state is the initial P-state when the
> >> P-map was first downloaded to AVS. It is *not* the current P-state.
> >>
> >> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
> >> command.
> >>
> >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> >> ---
> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> index 2c6e325..c943606 100644
> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
> >>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
> >>  {
> >>       struct private_data *priv = policy->driver_data;
> >> +     int ret;
> >> +
> >> +     ret = brcm_avs_get_pmap(priv, &priv->pmap);
> >> +     if (ret)
> >> +             return ret;
> >>
> >> -     return brcm_avs_get_pmap(priv, &priv->pmap);
> >> +     /*
> >> +      * We can't use the P-state returned by brcm_avs_get_pmap(), since
> >> +      * that's the initial P-state from when the P-map was downloaded to the
> >> +      * AVS co-processor, not necessarily the P-state we are running at now.
> >> +      * So, we get the current P-state explicitly.
> >> +      */
> >> +     return brcm_avs_get_pstate(priv, &priv->pmap.state);
> >>  }
> >>
> >>  static int brcm_avs_resume(struct cpufreq_policy *policy)
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Just wanted to follow up to see if this has been or will be picked up
> for 4.10?

For 4.10?  No way.

> I had a quick poke around some trees and did not see it
> there.

I'm not sure which trees you checked, but it is there in my linux-next branch.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Mayer Feb. 1, 2017, 11:59 p.m. UTC | #4
On 1 February 2017 at 15:44, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, January 31, 2017 10:53:01 AM Markus Mayer wrote:
>> On 5 January 2017 at 20:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 19-12-16, 12:10, Markus Mayer wrote:
>> >> From: Markus Mayer <mmayer@broadcom.com>
>> >>
>> >> The AVS GET_PMAP command does return a P-state along with the P-map
>> >> information. However, that P-state is the initial P-state when the
>> >> P-map was first downloaded to AVS. It is *not* the current P-state.
>> >>
>> >> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
>> >> command.
>> >>
>> >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> >> ---
>> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> >> index 2c6e325..c943606 100644
>> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> >> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
>> >>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
>> >>  {
>> >>       struct private_data *priv = policy->driver_data;
>> >> +     int ret;
>> >> +
>> >> +     ret = brcm_avs_get_pmap(priv, &priv->pmap);
>> >> +     if (ret)
>> >> +             return ret;
>> >>
>> >> -     return brcm_avs_get_pmap(priv, &priv->pmap);
>> >> +     /*
>> >> +      * We can't use the P-state returned by brcm_avs_get_pmap(), since
>> >> +      * that's the initial P-state from when the P-map was downloaded to the
>> >> +      * AVS co-processor, not necessarily the P-state we are running at now.
>> >> +      * So, we get the current P-state explicitly.
>> >> +      */
>> >> +     return brcm_avs_get_pstate(priv, &priv->pmap.state);
>> >>  }
>> >>
>> >>  static int brcm_avs_resume(struct cpufreq_policy *policy)
>> >
>> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> Just wanted to follow up to see if this has been or will be picked up
>> for 4.10?
>
> For 4.10?  No way.

I was only thinking 4.10, because it's a fix for an existing driver.
But I am not complaining if it goes into 4.11.

>> I had a quick poke around some trees and did not see it
>> there.
>
> I'm not sure which trees you checked, but it is there in my linux-next branch.

Clearly, I didn't check there, my apologies.

Regards,
-Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 2, 2017, 3:33 a.m. UTC | #5
On 02-02-17, 00:44, Rafael J. Wysocki wrote:
> On Tuesday, January 31, 2017 10:53:01 AM Markus Mayer wrote:
> > On 5 January 2017 at 20:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 19-12-16, 12:10, Markus Mayer wrote:
> > >> From: Markus Mayer <mmayer@broadcom.com>
> > >>
> > >> The AVS GET_PMAP command does return a P-state along with the P-map
> > >> information. However, that P-state is the initial P-state when the
> > >> P-map was first downloaded to AVS. It is *not* the current P-state.
> > >>
> > >> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
> > >> command.
> > >>
> > >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> > >> ---
> > >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
> > >>  1 file changed, 12 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > >> index 2c6e325..c943606 100644
> > >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> > >> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
> > >>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
> > >>  {
> > >>       struct private_data *priv = policy->driver_data;
> > >> +     int ret;
> > >> +
> > >> +     ret = brcm_avs_get_pmap(priv, &priv->pmap);
> > >> +     if (ret)
> > >> +             return ret;
> > >>
> > >> -     return brcm_avs_get_pmap(priv, &priv->pmap);
> > >> +     /*
> > >> +      * We can't use the P-state returned by brcm_avs_get_pmap(), since
> > >> +      * that's the initial P-state from when the P-map was downloaded to the
> > >> +      * AVS co-processor, not necessarily the P-state we are running at now.
> > >> +      * So, we get the current P-state explicitly.
> > >> +      */
> > >> +     return brcm_avs_get_pstate(priv, &priv->pmap.state);
> > >>  }
> > >>
> > >>  static int brcm_avs_resume(struct cpufreq_policy *policy)
> > >
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > Just wanted to follow up to see if this has been or will be picked up
> > for 4.10?
> 
> For 4.10?  No way.

I also thought it might get into 4.10 as these were fixes. Else he would be
required to push them via the 4.10 stable kernel.
Rafael J. Wysocki Feb. 2, 2017, 11:22 a.m. UTC | #6
On Thu, Feb 2, 2017 at 4:33 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-17, 00:44, Rafael J. Wysocki wrote:
>> On Tuesday, January 31, 2017 10:53:01 AM Markus Mayer wrote:
>> > On 5 January 2017 at 20:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > > On 19-12-16, 12:10, Markus Mayer wrote:
>> > >> From: Markus Mayer <mmayer@broadcom.com>
>> > >>
>> > >> The AVS GET_PMAP command does return a P-state along with the P-map
>> > >> information. However, that P-state is the initial P-state when the
>> > >> P-map was first downloaded to AVS. It is *not* the current P-state.
>> > >>
>> > >> Therefore, we explicitly retrieve the P-state using the GET_PSTATE
>> > >> command.
>> > >>
>> > >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> > >> ---
>> > >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
>> > >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> > >>
>> > >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> > >> index 2c6e325..c943606 100644
>> > >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> > >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> > >> @@ -784,8 +784,19 @@ static int brcm_avs_target_index(struct cpufreq_policy *policy,
>> > >>  static int brcm_avs_suspend(struct cpufreq_policy *policy)
>> > >>  {
>> > >>       struct private_data *priv = policy->driver_data;
>> > >> +     int ret;
>> > >> +
>> > >> +     ret = brcm_avs_get_pmap(priv, &priv->pmap);
>> > >> +     if (ret)
>> > >> +             return ret;
>> > >>
>> > >> -     return brcm_avs_get_pmap(priv, &priv->pmap);
>> > >> +     /*
>> > >> +      * We can't use the P-state returned by brcm_avs_get_pmap(), since
>> > >> +      * that's the initial P-state from when the P-map was downloaded to the
>> > >> +      * AVS co-processor, not necessarily the P-state we are running at now.
>> > >> +      * So, we get the current P-state explicitly.
>> > >> +      */
>> > >> +     return brcm_avs_get_pstate(priv, &priv->pmap.state);
>> > >>  }
>> > >>
>> > >>  static int brcm_avs_resume(struct cpufreq_policy *policy)
>> > >
>> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> >
>> > Just wanted to follow up to see if this has been or will be picked up
>> > for 4.10?
>>
>> For 4.10?  No way.
>
> I also thought it might get into 4.10 as these were fixes. Else he would be
> required to push them via the 4.10 stable kernel.

We'll see.

There are other fixes I may want to push for 4.10 and I can bundle
this thing with them if I decide to send one more pull request for
4.10 after all.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 2c6e325..c943606 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -784,8 +784,19 @@  static int brcm_avs_target_index(struct cpufreq_policy *policy,
 static int brcm_avs_suspend(struct cpufreq_policy *policy)
 {
 	struct private_data *priv = policy->driver_data;
+	int ret;
+
+	ret = brcm_avs_get_pmap(priv, &priv->pmap);
+	if (ret)
+		return ret;
 
-	return brcm_avs_get_pmap(priv, &priv->pmap);
+	/*
+	 * We can't use the P-state returned by brcm_avs_get_pmap(), since
+	 * that's the initial P-state from when the P-map was downloaded to the
+	 * AVS co-processor, not necessarily the P-state we are running at now.
+	 * So, we get the current P-state explicitly.
+	 */
+	return brcm_avs_get_pstate(priv, &priv->pmap.state);
 }
 
 static int brcm_avs_resume(struct cpufreq_policy *policy)