Re: [Qemu-arm] [PATCH v4] timer: a9gtimer: remove loop to auto-increment comparator

[Peter Maydell]

On 13 October 2016 at 07:42, P J P <address@hidden> wrote:
> +-- On Wed, 12 Oct 2016, Peter Maydell wrote --+
> | > -            while (gtb->compare < update.new) {
> | > +            if (gtb->compare < update.new) {
> | >
> | > +                    inc = update.new - gtb->compare - 1;
> |
> | Can you explain why the '- 1' here ?
>
>   I think I did that because while was running till gtb->compare < update.new.
> To contain gtb->compare under update.new.
>
> | Something still doesn't look right here. Consider the case where
> | update.new is only just bigger than gtb->compare (actually the
> | usual case, I think). In this case 'update.new - gtb->compare - 1'
> | is smaller than gtb->inc and so the QEMU_ALIGN_DOWN will produce
> | an inc value of 0. That would be wrong, because we should definitely
> | have done an auto-increment.
>
>    Yes, but in those cases 'gtb->compare += inc' would exceed update.new. Is
> that okay ?

Of course, and it's what you want. The point of the autoincrement
is that when the timer fires (because the current time reaches
or exceeds the comparator) the hardware automatically advances
the compare value so that it is again ahead of the current time.
That way at some future point the timer will fire again.

You can work this out also by looking at the existing code,
which reads "while (gtb->compare < update.new) { ... }"
so if the autoincrement is enabled we can't leave the while
loop until the compare has been moved forward beyond update.new.

I suggest you try putting in some sample values for the
various variables to confirm that your new code produces the
same answers that the old code did.

thanks
-- PMM