diff mbox series

opp: fix dev_pm_opp_set_rate for different frequency at the same opp level

Message ID 20210216201030.19152-1-jonathan@marek.ca (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series opp: fix dev_pm_opp_set_rate for different frequency at the same opp level | expand

Commit Message

Jonathan Marek Feb. 16, 2021, 8:10 p.m. UTC
There is not "nothing to do" when the opp is the same. The frequency can
be different from opp->rate.

Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/opp/core.c | 7 +++++--
 drivers/opp/opp.h  | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Viresh Kumar Feb. 17, 2021, 4:53 a.m. UTC | #1
On 16-02-21, 15:10, Jonathan Marek wrote:
> There is not "nothing to do" when the opp is the same. The frequency can
> be different from opp->rate.

I am sorry but I am not sure what are you trying to fix here and what exactly is
broken here. Can you provide a usecase for your platform where this doesn't work
like it used to ?

> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/opp/core.c | 7 +++++--
>  drivers/opp/opp.h  | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
Jonathan Marek Feb. 17, 2021, 1:29 p.m. UTC | #2
On 2/16/21 11:53 PM, Viresh Kumar wrote:
> On 16-02-21, 15:10, Jonathan Marek wrote:
>> There is not "nothing to do" when the opp is the same. The frequency can
>> be different from opp->rate.
> 
> I am sorry but I am not sure what are you trying to fix here and what exactly is
> broken here. Can you provide a usecase for your platform where this doesn't work
> like it used to ?
> 

The specific case is this opp table:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sm8250.dtsi#n439

It does not define every possible clock frequency, it only defines the 
rates at which a higher rpmhpd level must be used. Which is the intended 
use of opp.

Your change broke this completely: the clock rate change can be silently 
ignored because the opp level is the same. In particular it breaks 
bluetooth for this platform.

>> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/opp/core.c | 7 +++++--
>>   drivers/opp/opp.h  | 1 +
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>
Viresh Kumar Feb. 18, 2021, 7:11 a.m. UTC | #3
On 16-02-21, 15:10, Jonathan Marek wrote:
> There is not "nothing to do" when the opp is the same. The frequency can
> be different from opp->rate.
> 
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/opp/core.c | 7 +++++--
>  drivers/opp/opp.h  | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)

Thanks, I have updated it a bit and applied, thanks.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c3f3d9249cc5..f82cf72f433e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -986,6 +986,7 @@  static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		    struct dev_pm_opp *opp, unsigned long freq)
 {
 	struct dev_pm_opp *old_opp;
+	unsigned long old_freq;
 	int scaling_down, ret;
 
 	if (unlikely(!opp))
@@ -996,15 +997,16 @@  static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		_find_current_opp(dev, opp_table);
 
 	old_opp = opp_table->current_opp;
+	old_freq = opp_table->current_freq;
 
 	/* Return early if nothing to do */
-	if (opp_table->enabled && old_opp == opp) {
+	if (opp_table->enabled && old_opp == opp && old_freq == freq) {
 		dev_dbg(dev, "%s: OPPs are same, nothing to do\n", __func__);
 		return 0;
 	}
 
 	dev_dbg(dev, "%s: switching OPP: Freq %lu -> %lu Hz, Level %u -> %u, Bw %u -> %u\n",
-		__func__, old_opp->rate, freq, old_opp->level, opp->level,
+		__func__, old_freq, freq, old_opp->level, opp->level,
 		old_opp->bandwidth ? old_opp->bandwidth[0].peak : 0,
 		opp->bandwidth ? opp->bandwidth[0].peak : 0);
 
@@ -1061,6 +1063,7 @@  static int _set_opp(struct device *dev, struct opp_table *opp_table,
 	/* Make sure current_opp doesn't get freed */
 	dev_pm_opp_get(opp);
 	opp_table->current_opp = opp;
+	opp_table->current_freq = freq;
 
 	return ret;
 }
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9b9daf83b074..9f1d9c877380 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -186,6 +186,7 @@  struct opp_table {
 	enum opp_table_access shared_opp;
 	struct dev_pm_opp *current_opp;
 	struct dev_pm_opp *suspend_opp;
+	unsigned long current_freq;
 
 	struct mutex genpd_virt_dev_lock;
 	struct device **genpd_virt_devs;