[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

On Wed, Jun 29, 2011 at 7:00 AM, Russell King - ARM Linux
<linux at arm.linux.org.uk> wrote:
> On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote:
>> On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
>> <linux at arm.linux.org.uk> wrote:
>> > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
>> >> I don't think it affects bogomips - loops_per_jiffy can still be
>> >> calibrated and updated by cpufreq, udelay just wont use them.
>> >
>> > No, you can't avoid it.  __delay(), udelay(), and the global
>> > loops_per_jiffy are all linked together - for instance this is how
>> > the spinlock code has a 1s timeout:
>> >
>> > static void __spin_lock_debug(raw_spinlock_t *lock)
>> > {
>> >        u64 loops = loops_per_jiffy * HZ;
>> >        int print_once = 1;
>> >
>> >        for (;;) {
>> >                for (i = 0; i < loops; i++) {
>> >                        if (arch_spin_trylock(&lock->raw_lock))
>> >                                return;
>> >                        __delay(1);
>> >                }
>> >
>> > which goes wrong for all the same reasons you're pointing out about
>> > udelay().  So just restricting your argument to udelay() is not
>> > realistic.
>> >
>>
>> True, there are a few other users of loops_per_jiffy and __delay, but
>> it looks like __spin_lock_debug is the only one worth worrying about,
>> and it's timing is not as critical as udelay - worst case here is that
>> the warning occurs after 250 ms instead of 1s.  Leaving
>> loops_per_jiffy and __delay intact, and fixing udelay, would still be
>> a net gain.
>
> Other users of loops_per_jiffy are incorrect in any case:

The same conclusion I came to on a quick scan of other uses of
loops_per_jiffy...

<snip>

> And strangely the major offender of this stuff are ARM drivers, not other
> peoples stuff and not stuff in drivers/staging.
>
> So I don't think its sane to ignore loops_per_jiffy and __delay, and just
> concentrate on udelay().

But this I don't understand.  Every other use I found of
loops_per_jiffy is incorrect and should be changed.  Fixing udelay now
would not make them any worse - they would stay just as broken as they
were, so there is no need to couple a fix to udelay with fixing other
users of loops_per_jiffy.  Why block a legitimate fix because some
other broken code uses a variable whose behavior would not change?

If you are requesting an alternate change that would allow __delay to
continue to be used to time loops while irqs are off and jiffies is
not being updated, but also allows loops_per_jiffy to change in the
middle of a loop, while not increasing the number of instructions
executed in __delay, I don't think that is possible.  You could
replace __delay with a function that tests against a timer, and
loops_per_jiffy with the frequency of the counter divided by HZ, but
that would limit your spinlock spins to the frequency of the counter -
1MHz on Tegra.

Are there ever other legitimate uses of loops_per_jiffy/__delay?  I
don't think even the spinlock_debug use is correct - the number of
instructions executed in the loop before the __delay call (I count 17)
is going to be much higher than the instructions in the __delay(1)
call (3 instructions).  The spinlock debug timeout is already going to
be much longer than expected.  It looks to me like the only legitimate
use of loops_per_jiffy is to calculate the number of loops and call
__delay(loops) (exactly what udelay does), the overhead of doing
anything else will swamp the __delay call.  spinlock debug can get
away with its incorrect usage, because it doesn't really care how long
the delay is, and must have a minimum overhead.