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