| Message ID | 20180220111057.14756-1-dietmar.eggemann@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 20-02-18, 11:10, Dietmar Eggemann wrote: > Commit 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") > changed the cpufreq driver on juno from arm_big_little to scpi. > > The scpi set_target function does not call the frequency-invariance > setter function arch_set_freq_scale() like the arm_big_little set_target > function does. As a result the task scheduler load and utilization > signals are not frequency-invariant on this platform anymore. > > Fix this by adding a call to arch_set_freq_scale() into > scpi_cpufreq_set_target(). > > Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/cpufreq/scpi-cpufreq.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > index c32a833e1b00..3101d4e9c2de 100644 > --- a/drivers/cpufreq/scpi-cpufreq.c > +++ b/drivers/cpufreq/scpi-cpufreq.c > @@ -51,13 +51,19 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) > static int > scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > { > + unsigned long freq = policy->freq_table[index].frequency; > struct scpi_data *priv = policy->driver_data; > - u64 rate = policy->freq_table[index].frequency * 1000; > + u64 rate = freq * 1000; > int ret; > > ret = clk_set_rate(priv->clk, rate); > - if (!ret && (clk_get_rate(priv->clk) != rate)) > - ret = -EIO; > + if (!ret) { > + if (clk_get_rate(priv->clk) != rate) > + ret = -EIO; > + > + arch_set_freq_scale(policy->related_cpus, freq, > + policy->cpuinfo.max_freq); > + } > > return ret; > } Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Tue, Feb 20, 2018 at 12:10 PM, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > Commit 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") > changed the cpufreq driver on juno from arm_big_little to scpi. > > The scpi set_target function does not call the frequency-invariance > setter function arch_set_freq_scale() like the arm_big_little set_target > function does. As a result the task scheduler load and utilization > signals are not frequency-invariant on this platform anymore. > > Fix this by adding a call to arch_set_freq_scale() into > scpi_cpufreq_set_target(). > > Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> > Acked-by: Sudeep Holla <sudeep.holla@arm.com> This is really minor, but I would reorder this slightly. > --- > drivers/cpufreq/scpi-cpufreq.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > index c32a833e1b00..3101d4e9c2de 100644 > --- a/drivers/cpufreq/scpi-cpufreq.c > +++ b/drivers/cpufreq/scpi-cpufreq.c > @@ -51,13 +51,19 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) > static int > scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > { > + unsigned long freq = policy->freq_table[index].frequency; > struct scpi_data *priv = policy->driver_data; > - u64 rate = policy->freq_table[index].frequency * 1000; > + u64 rate = freq * 1000; > int ret; > > ret = clk_set_rate(priv->clk, rate); > - if (!ret && (clk_get_rate(priv->clk) != rate)) > - ret = -EIO; > + if (!ret) { I would do: if (ret) return ret; arch_set_freq_scale(policy->related_cpus, freq, policy->cpuinfo.max_freq); if (clk_get_rate(priv->clk) != rate) return -EIO; return 0; That's somewhat easier to follow for me. > + if (clk_get_rate(priv->clk) != rate) > + ret = -EIO; > + > + arch_set_freq_scale(policy->related_cpus, freq, > + policy->cpuinfo.max_freq); > + } > > return ret; > } > -- I also am not sure why you want to call arch_set_freq_scale() even if the new clock rate didn't stick.
On 02/22/2018 11:27 PM, Rafael J. Wysocki wrote: > On Tue, Feb 20, 2018 at 12:10 PM, Dietmar Eggemann > <dietmar.eggemann@arm.com> wrote: [...] >> Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: Sudeep Holla <sudeep.holla@arm.com> >> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> >> Acked-by: Sudeep Holla <sudeep.holla@arm.com> > > This is really minor, but I would reorder this slightly. Tried to figure out what would be the better order. Not sure since I saw different examples. Can you tell what would be the best tag order? [...] >> diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c >> index c32a833e1b00..3101d4e9c2de 100644 >> --- a/drivers/cpufreq/scpi-cpufreq.c >> +++ b/drivers/cpufreq/scpi-cpufreq.c >> @@ -51,13 +51,19 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) >> static int >> scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) >> { >> + unsigned long freq = policy->freq_table[index].frequency; >> struct scpi_data *priv = policy->driver_data; >> - u64 rate = policy->freq_table[index].frequency * 1000; >> + u64 rate = freq * 1000; >> int ret; >> >> ret = clk_set_rate(priv->clk, rate); >> - if (!ret && (clk_get_rate(priv->clk) != rate)) >> - ret = -EIO; >> + if (!ret) { > > I would do: > > if (ret) > return ret; > > arch_set_freq_scale(policy->related_cpus, freq, policy->cpuinfo.max_freq); > > if (clk_get_rate(priv->clk) != rate) > return -EIO; > > return 0; > > That's somewhat easier to follow for me. Yes I can change this. > >> + if (clk_get_rate(priv->clk) != rate) >> + ret = -EIO; >> + >> + arch_set_freq_scale(policy->related_cpus, freq, >> + policy->cpuinfo.max_freq); >> + } >> >> return ret; >> } >> -- > > I also am not sure why you want to call arch_set_freq_scale() even if > the new clock rate didn't stick. Right, this is much better. static int scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) { + unsigned long freq = policy->freq_table[index].frequency; struct scpi_data *priv = policy->driver_data; - u64 rate = policy->freq_table[index].frequency * 1000; + u64 rate = freq * 1000; int ret; ret = clk_set_rate(priv->clk, rate); - if (!ret && (clk_get_rate(priv->clk) != rate)) - ret = -EIO; - return ret; + if (ret) + return ret; + + if (clk_get_rate(priv->clk) != rate) + return -EIO; + + arch_set_freq_scale(policy->related_cpus, freq, + policy->cpuinfo.max_freq); + + return 0; Will send out a v2 as soon as I know the preferred tag order.
On Mon, Feb 26, 2018 at 8:49 AM, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > On 02/22/2018 11:27 PM, Rafael J. Wysocki wrote: >> On Tue, Feb 20, 2018 at 12:10 PM, Dietmar Eggemann >> <dietmar.eggemann@arm.com> wrote: > > [...] > >>> Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Viresh Kumar <viresh.kumar@linaro.org> >>> Cc: Sudeep Holla <sudeep.holla@arm.com> >>> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> >>> Acked-by: Sudeep Holla <sudeep.holla@arm.com> >> >> This is really minor, but I would reorder this slightly. > > Tried to figure out what would be the better order. Not sure since I saw > different examples. Can you tell what would be the best tag order? I was talking about the patch, not about tags, sorry for the confusion. Frankly, I don't care about the ordering of the tags. :-)
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c index c32a833e1b00..3101d4e9c2de 100644 --- a/drivers/cpufreq/scpi-cpufreq.c +++ b/drivers/cpufreq/scpi-cpufreq.c @@ -51,13 +51,19 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) static int scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) { + unsigned long freq = policy->freq_table[index].frequency; struct scpi_data *priv = policy->driver_data; - u64 rate = policy->freq_table[index].frequency * 1000; + u64 rate = freq * 1000; int ret; ret = clk_set_rate(priv->clk, rate); - if (!ret && (clk_get_rate(priv->clk) != rate)) - ret = -EIO; + if (!ret) { + if (clk_get_rate(priv->clk) != rate) + ret = -EIO; + + arch_set_freq_scale(policy->related_cpus, freq, + policy->cpuinfo.max_freq); + } return ret; }
Commit 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") changed the cpufreq driver on juno from arm_big_little to scpi. The scpi set_target function does not call the frequency-invariance setter function arch_set_freq_scale() like the arm_big_little set_target function does. As a result the task scheduler load and utilization signals are not frequency-invariant on this platform anymore. Fix this by adding a call to arch_set_freq_scale() into scpi_cpufreq_set_target(). Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> Acked-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/cpufreq/scpi-cpufreq.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)