cpufreq: brcmstb-avs-cpufreq: properly retrieve P-state upon suspend

PATCH In review Markus Mayer 5 participants 10 messages Dec 19, 2016 – Feb 2, 2017

Subsystems (3)

CPU frequency scaling framework Broadcom BCM7XXX Arm architecture Broadcom stb avs cpufreq driver

lore

Share & tools

lore

Markus Mayer

author

2016-12-19 20:10 UTC #0 lore raw

cover cpufreq: brcmstb-avs-cpufreq: properly retrieve P-state upon suspend

From: Markus Mayer <[email protected]>

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

Signature · 1 line

2.7.4

Markus Mayer

author

2016-12-19 20:10 UTC #1 lore raw

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

From: Markus Mayer <[email protected]>

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 <[email protected]>

---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/cpufreq/brcmstb-avs-cpufreq.c +2 -2

index 4fda623..2c6e325 100644
@@ -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)

Signature · 1 line

2.7.4

Viresh Kumar

maintainer Acked-by on v1 1/2

2017-01-06 04:04 UTC #2 lore raw

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

Quoted 17 lines · Markus Mayer

From: Markus Mayer <[email protected]>
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 <[email protected]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/cpufreq/brcmstb-avs-cpufreq.c +0 -0
index 4fda623..2c6e325 100644
@@ -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);

drivers/cpufreq/brcmstb-avs-cpufreq.c static ssize_t show_brcm_avs_pmap(struct cpufreq_policy *policy, char *buf) · line 962

Quoted 10 lines

drivers/cpufreq/brcmstb-avs-cpufreq.c +2 -2

 	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 <[email protected]>

Signature · 1 line

viresh

Markus Mayer

author

2016-12-19 20:10 UTC #3 lore raw

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

In reply to Markus Mayer

From: Markus Mayer <[email protected]>

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 <[email protected]>

---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/cpufreq/brcmstb-avs-cpufreq.c +12 -1

index 2c6e325..c943606 100644
@@ -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)

Signature · 1 line

2.7.4

Viresh Kumar

maintainer Acked-by on v1 2/2

2017-01-06 04:11 UTC #4 lore raw

Quoted 29 lines · Markus Mayer

From: Markus Mayer <[email protected]>
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 <[email protected]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/cpufreq/brcmstb-avs-cpufreq.c +5 -1
index 2c6e325..c943606 100644
@@ -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);

drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802

Quoted 10 lines

drivers/cpufreq/brcmstb-avs-cpufreq.c +7 -0

+	/*
+	 * 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 <[email protected]>

Signature · 1 line

viresh

Markus Mayer

2017-01-31 18:53 UTC #5 lore raw

Quoted 39 lines · Viresh Kumar

Quoted 38 lines · Markus Mayer

From: Markus Mayer <[email protected]>

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

Signed-off-by Markus Mayer <[email protected]>

---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/cpufreq/brcmstb-avs-cpufreq.c +12 -1

index 2c6e325..c943606 100644
@@ -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);
 }

Quoted 3 lines

drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802
Quoted 1 line
drivers/cpufreq/brcmstb-avs-cpufreq.c +0 -0
 static int brcm_avs_resume(struct cpufreq_policy *policy)
Acked-by Viresh Kumar <[email protected]>

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

Rafael J. Wysocki

2017-02-01 23:44 UTC #6 lore raw

Quoted 40 lines · Markus Mayer

Quoted 39 lines · Viresh Kumar

Quoted 38 lines · Markus Mayer

From: Markus Mayer <[email protected]>

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

Signed-off-by Markus Mayer <[email protected]>

---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/cpufreq/brcmstb-avs-cpufreq.c +12 -1

index 2c6e325..c943606 100644
@@ -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);
 }

Quoted 6 lines

Quoted 3 lines
drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802
Quoted 1 line
drivers/cpufreq/brcmstb-avs-cpufreq.c +0 -0
 static int brcm_avs_resume(struct cpufreq_policy *policy)
Acked-by Viresh Kumar <[email protected]>
Just wanted to follow up to see if this has been or will be picked up for 4.10?

For 4.10? No way.

Quoted 2 lines

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

Markus Mayer

2017-02-01 23:59 UTC #7 lore raw

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Markus Mayer
Quoted 39 lines · Viresh Kumar
Quoted 38 lines · Markus Mayer
From: Markus Mayer <[email protected]>
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 <[email protected]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/cpufreq/brcmstb-avs-cpufreq.c +12 -1
index 2c6e325..c943606 100644
@@ -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);
 }
 

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802
Quoted 1 line
drivers/cpufreq/brcmstb-avs-cpufreq.c +0 -0
 static int brcm_avs_resume(struct cpufreq_policy *policy)
Acked-by Viresh Kumar <[email protected]>
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.

Quoted 4 lines

Quoted 2 lines
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

Viresh Kumar

maintainer

2017-02-02 03:33 UTC #8 lore raw

In reply to Rafael J. Wysocki

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Markus Mayer
Quoted 39 lines · Viresh Kumar
Quoted 38 lines · Markus Mayer
From: Markus Mayer <[email protected]>
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 <[email protected]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/cpufreq/brcmstb-avs-cpufreq.c +12 -1
index 2c6e325..c943606 100644
@@ -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);
 }
 

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802
Quoted 1 line
drivers/cpufreq/brcmstb-avs-cpufreq.c +0 -0
 static int brcm_avs_resume(struct cpufreq_policy *policy)
Acked-by Viresh Kumar <[email protected]>
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.

Signature · 1 line

viresh

Rafael J. Wysocki

maintainer

2017-02-02 11:22 UTC #9 lore raw

Quoted 46 lines · Viresh Kumar

Quoted 45 lines · Rafael J. Wysocki
Quoted 44 lines · Markus Mayer
Quoted 42 lines · Viresh Kumar
Quoted 29 lines · Markus Mayer
From: Markus Mayer <[email protected]>
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 <[email protected]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/cpufreq/brcmstb-avs-cpufreq.c +5 -1
index 2c6e325..c943606 100644
@@ -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);
drivers/cpufreq/brcmstb-avs-cpufreq.c static int brcm_avs_target_index(struct cpufreq_policy *policy, · line 802
Quoted 10 lines
drivers/cpufreq/brcmstb-avs-cpufreq.c +7 -0
+     /*
+      * 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 <[email protected]>

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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