| Message ID | 1412263338.2313.1.camel@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Lucas, > Lucas Stach <l.stach@pengutronix.de> hat am 2. Oktober 2014 um 17:22 > geschrieben: > > [...] > > > > > > > > Does it work if you just change this OPP to 454737 kHz? > > > > > > > Yes after that i can change it with userspace governor. But i get this > > warning > > during modprobe: > > > > root@duckbill:~# modprobe cpufreq-cpu0 > > [ 48.573808] cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: > > 454736 KHz > > [ 48.604871] cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency > > changed to: 454737 KHz > > > > I'm not sure if it's okay. > > > It's harmless in your case. But to get rid of it you might try the > attached patch which may be an acceptable solution. > thanks a lot. The warning disappear after apply this patch :) Stefan > ------------------------------>8----------------------------------- > From db9e708edf6451b619ee73f68c3fcde2eccd7b0c Mon Sep 17 00:00:00 2001 > From: Lucas Stach <l.stach@pengutronix.de> > Date: Thu, 2 Oct 2014 17:20:09 +0200 > Subject: [PATCH] cpufreq: generic: try preserve some accuracy while > converting > from Hz to kHz > > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d9fdeddcef96..459830c6576f 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -187,7 +187,7 @@ unsigned int cpufreq_generic_get(unsigned int cpu) > return 0; > } > > - return clk_get_rate(policy->clk) / 1000; > + return DIV_ROUND_CLOSEST(clk_get_rate(policy->clk), 1000); > } > EXPORT_SYMBOL_GPL(cpufreq_generic_get); > > -- > 2.1.0 > > -- > Pengutronix e.K. | Lucas Stach | > Industrial Linux Solutions | http://www.pengutronix.de/ | > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 October 2014 20:52, Lucas Stach <l.stach@pengutronix.de> wrote: > It's harmless in your case. But to get rid of it you might try the > attached patch which may be an acceptable solution. > > ------------------------------>8----------------------------------- > From db9e708edf6451b619ee73f68c3fcde2eccd7b0c Mon Sep 17 00:00:00 2001 > From: Lucas Stach <l.stach@pengutronix.de> > Date: Thu, 2 Oct 2014 17:20:09 +0200 > Subject: [PATCH] cpufreq: generic: try preserve some accuracy while > converting > from Hz to kHz > > --- > drivers/cpufreq/cpufreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d9fdeddcef96..459830c6576f 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -187,7 +187,7 @@ unsigned int cpufreq_generic_get(unsigned int cpu) > return 0; > } > > - return clk_get_rate(policy->clk) / 1000; > + return DIV_ROUND_CLOSEST(clk_get_rate(policy->clk), 1000); > } > EXPORT_SYMBOL_GPL(cpufreq_generic_get); Absolutely NOT.. This can't be an acceptable solution to the problem you have in your DT. This routine is used for getting the current frequency of a CPU and we can't play with that.. The problem that Stefan is getting is: The boot loader has programmed cpu to a frequency which isn't present in your DT table. And so kernel tries to adjust CPU frequency to one of the listed frequencies. Solutions: - Add another entry to your DT table to reflect the currently programmed frequency. - Just ignore the error, as that is just for information. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.10.2014 um 06:21 schrieb Viresh Kumar: > On 2 October 2014 20:52, Lucas Stach <l.stach@pengutronix.de> wrote: >> It's harmless in your case. But to get rid of it you might try the >> attached patch which may be an acceptable solution. >> >> ------------------------------>8----------------------------------- >> From db9e708edf6451b619ee73f68c3fcde2eccd7b0c Mon Sep 17 00:00:00 2001 >> From: Lucas Stach <l.stach@pengutronix.de> >> Date: Thu, 2 Oct 2014 17:20:09 +0200 >> Subject: [PATCH] cpufreq: generic: try preserve some accuracy while >> converting >> from Hz to kHz >> >> --- >> drivers/cpufreq/cpufreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index d9fdeddcef96..459830c6576f 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -187,7 +187,7 @@ unsigned int cpufreq_generic_get(unsigned int cpu) >> return 0; >> } >> >> - return clk_get_rate(policy->clk) / 1000; >> + return DIV_ROUND_CLOSEST(clk_get_rate(policy->clk), 1000); >> } >> EXPORT_SYMBOL_GPL(cpufreq_generic_get); > Absolutely NOT.. > > This can't be an acceptable solution to the problem you have in your DT. > This routine is used for getting the current frequency of a CPU and we > can't play with that.. I thought about it and agree with you. > The problem that Stefan is getting is: The boot loader has programmed > cpu to a frequency which isn't present in your DT table. And so kernel > tries to adjust CPU frequency to one of the listed frequencies. Here comes the real problem, i'm not able to specify the exact frequency of the cpu. The clock has a value in Hz, but in the DT we have kHz. So there are always rounding issues. The question is how to handle it. > Solutions: > - Add another entry to your DT table to reflect the currently programmed > frequency. > - Just ignore the error, as that is just for information. Ignoring errors isn't a solution. I have another idea. I've looked a little bit at the code in the cpufreq-(cpu0/dt). Usually the frequencies are rounded down, but in cpu0_set_target() with dev_pm_opp_find_freq_ceil(cpu_dev, &freq_Hz) they are rounded up. From my point of view that's wrong. I fixed the problem by decreasing frequency in DT and replacing dev_pm_opp_find_freq_ceil() with dev_pm_opp_find_freq_floor() in cpufreq-cpu0.c . Unfortunatelly this will break all other DTS files with roundings in OPPs. Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 October 2014 12:11, Stefan Wahren <stefan.wahren@i2se.com> wrote: > Ignoring errors isn't a solution. I have another idea. I've looked a > little bit at the code in the cpufreq-(cpu0/dt). Usually the frequencies > are rounded down, but in cpu0_set_target() with > dev_pm_opp_find_freq_ceil(cpu_dev, &freq_Hz) they are rounded up. From > my point of view that's wrong. @Shawn ?? > I fixed the problem by decreasing frequency in DT and replacing > dev_pm_opp_find_freq_ceil() with dev_pm_opp_find_freq_floor() in > cpufreq-cpu0.c . Unfortunatelly this will break all other DTS files with > roundings in OPPs. I am still not sure how this fixed your problem.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.10.2014 um 10:50 schrieb Viresh Kumar: > On 6 October 2014 12:11, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> Ignoring errors isn't a solution. I have another idea. I've looked a >> little bit at the code in the cpufreq-(cpu0/dt). Usually the frequencies >> are rounded down, but in cpu0_set_target() with >> dev_pm_opp_find_freq_ceil(cpu_dev, &freq_Hz) they are rounded up. From >> my point of view that's wrong. > @Shawn ?? > >> I fixed the problem by decreasing frequency in DT and replacing >> dev_pm_opp_find_freq_ceil() with dev_pm_opp_find_freq_floor() in >> cpufreq-cpu0.c . Unfortunatelly this will break all other DTS files with >> roundings in OPPs. > I am still not sure how this fixed your problem.. Sorry, about the confusion. The main problem about setting cpufreq to maximum has been fixed by DT settings. Above i talked about an idea to fix the confusing warning after fixing the main problem: root@duckbill:~# modprobe cpufreq-cpu0 [ 48.573808] cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 454736 KHz [ 48.604871] cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency changed to: 454737 KHz But the best solution would be a more intelligent function like dev_pm_opp_find_freq_nearest() which find the OPP with the smallest frequency distance. So nobody of the DTS users has to care about frequency rounding. Currently i'm trying to adapt the DTS file to the driver and that's not the intention of devicetree. Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 October 2014 14:43, Stefan Wahren <stefan.wahren@i2se.com> wrote: > Sorry, about the confusion. The main problem about setting cpufreq to > maximum has been fixed by DT settings. Above i talked about an idea to > fix the confusing warning after fixing the main problem: > > root@duckbill:~# modprobe cpufreq-cpu0 > [ 48.573808] cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted > freq: 454736 KHz > [ 48.604871] cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial > frequency changed to: 454737 KHz > > But the best solution would be a more intelligent function like > dev_pm_opp_find_freq_nearest() which find the OPP with the smallest > frequency distance. So nobody of the DTS users has to care about > frequency rounding. > > Currently i'm trying to adapt the DTS file to the driver and that's not > the intention of devicetree. It looks that one of us hasn't understood how things are working :) So here is my part of theory: - Search for CPUFREQ_NEED_INITIAL_FREQ_CHECK in cpufreq.c - When starting cpufreq core for every CPU we check if the current frequency at which is running is mentioned in DT. If not, we throw above warnings and fix the freq. - Until this time cpufreq-driver's ->target_index() routine hasn't come into picture and so NO opp routine will fix that.. Does this sound reasonable ? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.10.2014 um 11:26 schrieb Viresh Kumar: > On 6 October 2014 14:43, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> Sorry, about the confusion. The main problem about setting cpufreq to >> maximum has been fixed by DT settings. Above i talked about an idea to >> fix the confusing warning after fixing the main problem: >> >> root@duckbill:~# modprobe cpufreq-cpu0 >> [ 48.573808] cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted >> freq: 454736 KHz >> [ 48.604871] cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial >> frequency changed to: 454737 KHz >> >> But the best solution would be a more intelligent function like >> dev_pm_opp_find_freq_nearest() which find the OPP with the smallest >> frequency distance. So nobody of the DTS users has to care about >> frequency rounding. >> >> Currently i'm trying to adapt the DTS file to the driver and that's not >> the intention of devicetree. > It looks that one of us hasn't understood how things are working :) I think i know him ;-) > > So here is my part of theory: > - Search for CPUFREQ_NEED_INITIAL_FREQ_CHECK in cpufreq.c > - When starting cpufreq core for every CPU we check if the current > frequency at which is running is mentioned in DT. If not, we throw above > warnings and fix the freq. Okay i.MX28 cpu is running initial on 454736842 Hz. 454736 kHz in the DT will make cpufreq core happy, but cpufreq-cpu0 will select next lower frequency. 454737 kHz in the DT will make cpufreq sad, but it's selectable by cpufreq-cpu0. Is it possible cpufreq core round the clock frequency down? > - Until this time cpufreq-driver's ->target_index() routine hasn't come > into picture and so NO opp routine will fix that.. > > Does this sound reasonable ? Thanks for your explanation. Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 October 2014 15:35, Stefan Wahren <stefan.wahren@i2se.com> wrote: > Okay i.MX28 cpu is running initial on 454736842 Hz. 454736 kHz in the DT > will make cpufreq core happy, Probably you know why cpufreq-core would be happy but let me still write it down. policy->cur = cpufreq_generic_get(), i.e. clk_get_rate(policy->clk) / 1000 So, policy->cur = 454736842/1000 = 454736 This is == to the DT value and so cpufreq-core is happy. > but cpufreq-cpu0 will select next lower Where did this come from in the above theory ? > frequency. 454737 kHz in the DT will make cpufreq sad, but it's > selectable by cpufreq-cpu0. I couldn't understand why you mentioned this here. It has nothing to do with the warning you got.. > Is it possible cpufreq core round the clock frequency down? Even this, sorry :( -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.10.2014 um 12:16 schrieb Viresh Kumar: > On 6 October 2014 15:35, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> but cpufreq-cpu0 will select next lower > Where did this come from in the above theory ? > I was wrong. cpufreq-cpu0 will search for a higher OPP than 454736842 and fails: [ 1111.226006] cpu cpu0: failed to find OPP for 454736842 May be Shawn has an idea or solution. Sorry for that noise. Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6 October 2014 16:42, Stefan Wahren <stefan.wahren@i2se.com> wrote: > Am 06.10.2014 um 12:16 schrieb Viresh Kumar: >> On 6 October 2014 15:35, Stefan Wahren <stefan.wahren@i2se.com> wrote: >>> but cpufreq-cpu0 will select next lower >> Where did this come from in the above theory ? >> > > I was wrong. cpufreq-cpu0 will search for a higher OPP than 454736842 > and fails: > > [ 1111.226006] cpu cpu0: failed to find OPP for 454736842 I have been talking about this error all this time: [ 48.573808] cpufreq: __cpufreq_add_dev: CPU0: Running at unlisted freq: 454736 KHz [ 48.604871] cpufreq: __cpufreq_add_dev: CPU0: Unlisted initial frequency changed to: 454737 KHz And this has nothing to do with the error you just pasted. For the above two errors cpufreq-cpu0's ->target_index() isn't called at all and so it doesn't make a difference.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d9fdeddcef96..459830c6576f 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -187,7 +187,7 @@ unsigned int cpufreq_generic_get(unsigned int cpu) return 0; } - return clk_get_rate(policy->clk) / 1000; + return DIV_ROUND_CLOSEST(clk_get_rate(policy->clk), 1000); } EXPORT_SYMBOL_GPL(cpufreq_generic_get);