From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:56933)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1budVg-0002KO-Dt
	for qemu-devel@nongnu.org; Thu, 13 Oct 2016 06:45:13 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1budVf-0001xi-Eq
	for qemu-devel@nongnu.org; Thu, 13 Oct 2016 06:45:12 -0400
Received: from mail-vk0-x236.google.com ([2607:f8b0:400c:c05::236]:35626)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1budVf-0001xS-B2
	for qemu-devel@nongnu.org; Thu, 13 Oct 2016 06:45:11 -0400
Received: by mail-vk0-x236.google.com with SMTP id q126so19939915vkd.2
	for <qemu-devel@nongnu.org>; Thu, 13 Oct 2016 03:45:11 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <alpine.LFD.2.20.1610131141320.2125@wniryva>
References: <1475903058-729-1-git-send-email-ppandit@redhat.com>
	<CAFEAcA8LfjDEMtbT5GRryLvAEu9kzbEzykKXF9DP+dDLajT8BQ@mail.gmail.com>
	<alpine.LFD.2.20.1610131141320.2125@wniryva>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Thu, 13 Oct 2016 11:44:50 +0100
Message-ID: <CAFEAcA8Vn9F7FOYqU5ddLBKtn=3vnQc-eynsOoV_X-pU7S_C-Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH v4] timer: a9gtimer: remove loop to
 auto-increment comparator
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: P J P <ppandit@redhat.com>
Cc: Qemu Developers <qemu-devel@nongnu.org>, Li Qiang <liqiang6-s@360.cn>, qemu-arm <qemu-arm@nongnu.org>

On 13 October 2016 at 07:42, P J P <ppandit@redhat.com> 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