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
next prev parent 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