All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jens Scharsig <js_at_ng@scharsoft.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
Date: Sun, 08 Aug 2010 12:52:15 +0200	[thread overview]
Message-ID: <4C5E8C5F.6000001@scharsoft.de> (raw)
In-Reply-To: <4C5DA0FD.5020709@emk-elektronik.de>

Dear Reinhard
> I think the timer.c is more broken that just that return value:
> 
You are right, but the patch fixes only the single small problem.
> example 1:
> /*
>  * timer without interrupts
>  */
> unsigned long long get_ticks(void)
> {
>     at91_pit_t *pit = (at91_pit_t *) AT91_PIT_BASE;
> 
>     ulong now = readl(&pit->piir);
> 
>     if (now >= lastinc)    /* normal mode (non roll) */
>         /* move stamp forward with absolut diff ticks */
>         timestamp += (now - lastinc);
>     else            /* we have rollover of incrementer */
>         timestamp += (0xFFFFFFFF - lastinc) + now;
>     lastinc = now;
>     return timestamp;
> }
> 
> observation: timestamp, lastinc, now are all ulong.
>         timestamp += (0xFFFFFFFF - lastinc) + now;
> is the same as
>         timestamp += (now - lastinc) - 1;
> so in case of a "rollover" timestamp is incremented just by one less.
> I think the if is superfluous. ulong will handle the rollover automatically

But this is only true, if we are using ulong variables. I think the idea behind
this code is use unsigned long long for variables. (especially timestamp)

> 
> example 2:
> void __udelay(unsigned long usec)
> {
>     unsigned long long tmp;
>     ulong tmo;
> 
>     tmo = usec_to_tick(usec);
>     tmp = get_ticks() + tmo;    /* get current timestamp */
> 
>     while (get_ticks() < tmp)    /* loop till event */
>          /*NOP*/;
> }
> 
> observation: tmp being unsigned long long, get_ticks returning
> the unsigned long timestamp, tmp being a sum of two ulongs,
> the while might NEVER end. In practice that is very unlikely,
> however the code should be corrected.

Right, but this isn't only a problem of AT91 arch. Is should be fixed global.
The code is correct, if get_ticks returns real long long values.
> 
> I was going to rework that timer sooner or later to address all
> those issues, but you are welcome to go ahead, too. Just we
> should avoid double work :)

I have no time enough at the moment to do that.
> 
> Greetings, Reinhard

Best regards

Jens Scharsig

  reply	other threads:[~2010-08-08 10:52 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-07 17:49 [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk Jens Scharsig
2010-08-07 18:07 ` Reinhard Meyer
2010-08-08 10:52   ` Jens Scharsig [this message]
2010-08-09  6:34 ` Alexander Stein
2010-08-18  7:17   ` Xu, Hong
2010-08-30  6:52     ` Alexander Stein
2010-08-31  7:36       ` Reinhard Meyer
2010-08-31 16:22         ` Jens Scharsig
2010-09-01  7:30           ` Reinhard Meyer
2010-09-01 17:37             ` Jens Scharsig
2010-09-02  1:55               ` Xu, Hong
2010-09-02  6:58               ` Andreas Bießmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C5E8C5F.6000001@scharsoft.de \
    --to=js_at_ng@scharsoft.de \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.