Re: [PATCH] ARM: Don't ever downscale loops_per_jiffy in SMP systems#

[Doug Anderson]

Hi,

On Tue, May 13, 2014 at 4:29 PM, Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
> On Tue, 13 May 2014, Nicolas Pitre wrote:
>
>> On Tue, 13 May 2014, Stephen Warren wrote:
>>
>> > On 05/13/2014 03:50 PM, Doug Anderson wrote:
>> > ...
>> > > ...but then I found the true problem shows up when we transition
>> > > between very low frequencies on exynos, like between 200MHz and
>> > > 300MHz.  While transitioning between frequencies the system
>> > > temporarily bumps over to the "switcher" PLL running at 800MHz while
>> > > waiting for the main PLL to stabilize.  No CPUFREQ notification is
>> > > sent for that.  That means there's a period of time when we're running
>> > > at 800MHz but loops_per_jiffy is calibrated at between 200MHz and
>> > > 300MHz.
>> > >
>> > >
>> > > I'm welcome to any suggestions for how to address this.  It sorta
>> > > feels like it would be a common thing to have a temporary PLL during
>> > > the transition, ...
>> >
>> > We definitely do that on Tegra for some cpufreq transitions.
>>
>> Ouch...  If this is a common strategy to use a third frequency during a
>> transition phase, especially if that frequency is way off (800MHz vs
>> 200-300MHz) then it is something the cpufreq layer must capture and
>> advertise.
>
> Of course if only the loops_per_jiffy scaling does care about frequency
> changes these days, and if in those cases udelay() can instead be moved
> to a timer source on those hick-up prone platforms, then all this is
> fairly theoretical and may not be worth pursuing.

Yup, I think I'm going to abandon this.  If anyone ever runs into this
problem later and can't use a bloody timer, hopefully they'll stumble
on this thread and at least end up with a good starting point.

For the record, the fix I made locally (which actually worked) was:

1. Add a new "temp" field to cpufreq_freqs:

--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -133,6 +133,7 @@ struct cpufreq_freqs {
        unsigned int cpu;       /* cpu nr */
        unsigned int old;
        unsigned int new;
+       unsigned int temp;
        u8 flags;               /* flags of cpufreq_driver, see below. */
 };

...depending on whether all users of this structure zero-initialize it
(I didn't check) you might also need to add a flag to say whether the
temp field is actually valid.


2. Set the "temp" to the switching PLL frequency in the cpufreq driver.


3. Change the ARM cpufreq_callback() to take the temp freq into account:

+       if (val == CPUFREQ_PRECHANGE) {
+               old_freq = freq->old;
+               new_freq = max(freq->new, freq->temp);
+       } else if (val == CPUFREQ_POSTCHANGE) {
+               old_freq = max(freq->old, freq->temp);
+               new_freq = freq->new;
+       }

...then use old_freq and new_freq in the place of freq->old and freq->new



-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/