| Message ID | de5c3ff26e9d06a6e0b06f8ba3da6d8c3ff82304.1385017369.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote: > Sometimes boot loaders set CPU frequency to a value outside of frequency table > present with cpufreq core. In such cases CPU might be unstable if it has to run > on that frequency for long duration of time and so its better to set it to a > frequency which is specified in freq-table. This also makes cpufreq stats > inconsistent as cpufreq-stats would fail to register because current frequency > of CPU isn't found in freq-table. > > Because we don't want this change to effect boot process badly, we go for the > next freq which is >= policy->cur ('cur' must be set by now, otherwise we will > end up setting freq to lowest of the table as 'cur' is initialized to zero). > > In case where CPU is already running on one of the frequencies present in > freq-table, this would turn into a dummy call as __cpufreq_driver_target() would > return early. > > Reported-by: Carlos Hernandez <ceh@ti.com> > Reported-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > After lots of discussion with Nishanth and others, I feel something like this.. > > @Nishanth: Please see if this works for you and I hope we don't need any of > these patches anymore: > > - https://lkml.org/lkml/2013/11/15/569 : cpufreq: cpufreq-cpu0: Use a sane boot > frequency when booting with a mismatched bootloader configuration > - https://lkml.org/lkml/2013/11/15/503 : cpufreq: stats: Do not populate stats > when policy->cur has no exact match > - https://lkml.org/lkml/2013/11/19/16 : cpufreq/stats: Add "unknown" frequency > field in stats tables > > drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 02d534d..d55c843 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, > } > } > > + /* > + * Sometimes boot loaders set CPU frequency to a value outside of > + * frequency table present with cpufreq core. In such cases CPU might be > + * unstable if it has to run on that frequency for long duration of time > + * and so its better to set it to a frequency which is specified in > + * freq-table. This also makes cpufreq stats inconsistent as > + * cpufreq-stats would fail to register because current frequency of CPU > + * isn't found in freq-table. > + * > + * Because we don't want this change to effect boot process badly, we go > + * for the next freq which is >= policy->cur ('cur' must be set by now, > + * otherwise we will end up setting freq to lowest of the table as 'cur' > + * is initialized to zero). > + * > + * In case where CPU is already running on one of the frequencies > + * present in freq-table, this would turn into a dummy call as > + * __cpufreq_driver_target() would return early. > + */ > + if (has_target()) { > + ret = __cpufreq_driver_target(policy, policy->cur, > + CPUFREQ_RELATION_L); > + if (ret) > + pr_err("%s: Unable to set frequency from table: %d\n", > + __func__, ret); Should we continue in that case? > + } > + > /* related cpus should atleast have policy->cpus */ > cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); > >
On 21 November 2013 18:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote: >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, >> + if (has_target()) { >> + ret = __cpufreq_driver_target(policy, policy->cur, >> + CPUFREQ_RELATION_L); >> + if (ret) >> + pr_err("%s: Unable to set frequency from table: %d\n", >> + __func__, ret); > > Should we continue in that case? I wasn't sure. I thought maybe there are platforms which might not be ready for transitions so early and so an error message would be fine, as we will fail soon anyway in case there is a bug. -- viresh
On 11/21/2013 07:57 AM, Viresh Kumar wrote: > On 21 November 2013 18:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote: > >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, > >>> + if (has_target()) { >>> + ret = __cpufreq_driver_target(policy, policy->cur, >>> + CPUFREQ_RELATION_L); >>> + if (ret) >>> + pr_err("%s: Unable to set frequency from table: %d\n", >>> + __func__, ret); >> >> Should we continue in that case? > > I wasn't sure. I thought maybe there are platforms which might not be > ready for transitions so early and so an error message would be fine, > as we will fail soon anyway in case there is a bug. > The scaling driver for the CPU has already loaded and its .init procedure has been called so .target better be callable. Since the scaling driver is responsible maintaining the set of valid frequencies and setting policy->cur I think it is reasonable to have the scaling driver ensure that policy->cur returned from its .init and the operating frequency are in sync and match one of the values in its frequency table. --Dirk > -- > viresh > -- > To unsubscribe from this list: send the line "unsubscribe cpufreq" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thursday, November 21, 2013 09:56:32 AM Dirk Brandewie wrote: > On 11/21/2013 07:57 AM, Viresh Kumar wrote: > > On 21 November 2013 18:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> On Thursday, November 21, 2013 12:39:02 PM Viresh Kumar wrote: > > > >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >>> @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, > > > >>> + if (has_target()) { > >>> + ret = __cpufreq_driver_target(policy, policy->cur, > >>> + CPUFREQ_RELATION_L); > >>> + if (ret) > >>> + pr_err("%s: Unable to set frequency from table: %d\n", > >>> + __func__, ret); > >> > >> Should we continue in that case? > > > > I wasn't sure. I thought maybe there are platforms which might not be > > ready for transitions so early and so an error message would be fine, > > as we will fail soon anyway in case there is a bug. > > > > The scaling driver for the CPU has already loaded and its .init procedure has > been called so .target better be callable. > > Since the scaling driver is responsible maintaining the set of valid frequencies > and setting policy->cur I think it is reasonable to have the scaling driver > ensure that policy->cur returned from its .init and the operating frequency are > in sync and match one of the values in its frequency table. From that I infer that we should not continue on errors here. Which also is my opinion. Rafael
On Friday 22 November 2013 03:13 AM, Rafael J. Wysocki wrote: > On Thursday, November 21, 2013 09:56:32 AM Dirk Brandewie wrote: >> The scaling driver for the CPU has already loaded and its .init procedure has >> been called so .target better be callable. Yeah.. >> Since the scaling driver is responsible maintaining the set of valid frequencies >> and setting policy->cur I think it is reasonable to have the scaling driver >> ensure that policy->cur returned from its .init I agree.. >> and the operating frequency are >> in sync and match one of the values in its frequency table. Hmmm, that doesn't necessarily lie in driver's domain but maybe at a common place like core. That's why we had this patch.. > From that I infer that we should not continue on errors here. Which also is my > opinion. Okay.. Code modified to return error on failure.. Will send V2 as soon as patch is tested by Nishanth..
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..d55c843 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1038,6 +1038,32 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, } } + /* + * Sometimes boot loaders set CPU frequency to a value outside of + * frequency table present with cpufreq core. In such cases CPU might be + * unstable if it has to run on that frequency for long duration of time + * and so its better to set it to a frequency which is specified in + * freq-table. This also makes cpufreq stats inconsistent as + * cpufreq-stats would fail to register because current frequency of CPU + * isn't found in freq-table. + * + * Because we don't want this change to effect boot process badly, we go + * for the next freq which is >= policy->cur ('cur' must be set by now, + * otherwise we will end up setting freq to lowest of the table as 'cur' + * is initialized to zero). + * + * In case where CPU is already running on one of the frequencies + * present in freq-table, this would turn into a dummy call as + * __cpufreq_driver_target() would return early. + */ + if (has_target()) { + ret = __cpufreq_driver_target(policy, policy->cur, + CPUFREQ_RELATION_L); + if (ret) + pr_err("%s: Unable to set frequency from table: %d\n", + __func__, ret); + } + /* related cpus should atleast have policy->cpus */ cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
Sometimes boot loaders set CPU frequency to a value outside of frequency table present with cpufreq core. In such cases CPU might be unstable if it has to run on that frequency for long duration of time and so its better to set it to a frequency which is specified in freq-table. This also makes cpufreq stats inconsistent as cpufreq-stats would fail to register because current frequency of CPU isn't found in freq-table. Because we don't want this change to effect boot process badly, we go for the next freq which is >= policy->cur ('cur' must be set by now, otherwise we will end up setting freq to lowest of the table as 'cur' is initialized to zero). In case where CPU is already running on one of the frequencies present in freq-table, this would turn into a dummy call as __cpufreq_driver_target() would return early. Reported-by: Carlos Hernandez <ceh@ti.com> Reported-by: Nishanth Menon <nm@ti.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- After lots of discussion with Nishanth and others, I feel something like this.. @Nishanth: Please see if this works for you and I hope we don't need any of these patches anymore: - https://lkml.org/lkml/2013/11/15/569 : cpufreq: cpufreq-cpu0: Use a sane boot frequency when booting with a mismatched bootloader configuration - https://lkml.org/lkml/2013/11/15/503 : cpufreq: stats: Do not populate stats when policy->cur has no exact match - https://lkml.org/lkml/2013/11/19/16 : cpufreq/stats: Add "unknown" frequency field in stats tables drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)