public inbox for u-boot@lists.denx.de
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox