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

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

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

This fix is for 4.10.

The driver needs to save the proper P-state on suspend, so it'll resume at
the same speed.

We also add the P-state and the mode as returned by the AVS firmware GET_PMAP
command to the existing brcm_avs_pmap sysfs entry. This will make it easier
to diagnose any issues.

Markus Mayer (2):
  cpufreq: brcmstb-avs-cpufreq: extend sysfs entry brcm_avs_pmap
  cpufreq: brcmstb-avs-cpufreq: properly retrieve P-state upon suspend

 drivers/cpufreq/brcmstb-avs-cpufreq.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: extend sysfs entry brcm_avs_pmap

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

We extend the brcm_avs_pmap sysfs entry (which issues the GET_PMAP
command to AVS) to include all fields from struct pmap. This means
adding mode (AVS, DVS, DVFS) and state (the P-state) to the output.

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

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 4fda623..2c6e325 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -954,9 +954,9 @@ static ssize_t show_brcm_avs_pmap(struct cpufreq_policy *policy, char *buf)
 	brcm_avs_parse_p1(pmap.p1, &mdiv_p0, &pdiv, &ndiv);
 	brcm_avs_parse_p2(pmap.p2, &mdiv_p1, &mdiv_p2, &mdiv_p3, &mdiv_p4);
 
-	return sprintf(buf, "0x%08x 0x%08x %u %u %u %u %u %u %u\n",
+	return sprintf(buf, "0x%08x 0x%08x %u %u %u %u %u %u %u %u %u\n",
 		pmap.p1, pmap.p2, ndiv, pdiv, mdiv_p0, mdiv_p1, mdiv_p2,
-		mdiv_p3, mdiv_p4);
+		mdiv_p3, mdiv_p4, pmap.mode, pmap.state);
 }
 
 static ssize_t show_brcm_avs_voltage(struct cpufreq_policy *policy, char *buf)
-- 
2.7.4

Re: [PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: extend sysfs entry brcm_avs_pmap

[Viresh Kumar]

On 19-12-16, 12:10, Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> We extend the brcm_avs_pmap sysfs entry (which issues the GET_PMAP
> command to AVS) to include all fields from struct pmap. This means
> adding mode (AVS, DVS, DVFS) and state (the P-state) to the output.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index 4fda623..2c6e325 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -954,9 +954,9 @@ static ssize_t show_brcm_avs_pmap(struct cpufreq_policy *policy, char *buf)
>  	brcm_avs_parse_p1(pmap.p1, &mdiv_p0, &pdiv, &ndiv);
>  	brcm_avs_parse_p2(pmap.p2, &mdiv_p1, &mdiv_p2, &mdiv_p3, &mdiv_p4);
>  
> -	return sprintf(buf, "0x%08x 0x%08x %u %u %u %u %u %u %u\n",
> +	return sprintf(buf, "0x%08x 0x%08x %u %u %u %u %u %u %u %u %u\n",
>  		pmap.p1, pmap.p2, ndiv, pdiv, mdiv_p0, mdiv_p1, mdiv_p2,
> -		mdiv_p3, mdiv_p4);
> +		mdiv_p3, mdiv_p4, pmap.mode, pmap.state);
>  }
>  
>  static ssize_t show_brcm_avs_voltage(struct cpufreq_policy *policy, char *buf)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

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

[Markus Mayer]

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

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

[Viresh Kumar]

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>

-- 
viresh

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

[Markus Mayer]

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

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

[Rafael J. Wysocki]

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

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

[Markus Mayer]

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

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

[Viresh Kumar]

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.

-- 
viresh

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

[Rafael J. Wysocki]

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