public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 1/8] u8500: Correct unnecessary mathematical roll-over
Date: Wed, 21 Nov 2012 10:02:28 +0000	[thread overview]
Message-ID: <20121121100228.GJ28265@gmail.com> (raw)
In-Reply-To: <20121120181453.568D620009C@gemini.denx.de>

> > If we attempt to take a 32bit timer value and multiply it by a
> > significant number, the core will not be able to handle it. This
> > gives the illusion that the timer is rolling over, when in fact
> > this is not the case. If we ensure the division in this instance
> > is carried out before the multiplication, the issue vanishes.
> 
> Are you sure this is a good idea?

Not now I'm not. :)

> > --- a/arch/arm/cpu/armv7/u8500/timer.c
> > +++ b/arch/arm/cpu/armv7/u8500/timer.c
> > @@ -70,7 +70,7 @@ struct u8500_mtu {
> >   * The MTU is clocked at 133 MHz by default. (V1 and later)
> >   */
> >  #define TIMER_CLOCK		(133 * 1000 * 1000 / 16)
> > -#define COUNT_TO_USEC(x)	((x) * 16 / 133)
> > +#define COUNT_TO_USEC(x)	((x) / 133 * 16)
> 
> Before the change, the result is useful for all values of x in the
> interval from 0 through UINT_MAX/16 = 268435455 = 268 seconds, i. e.
> for all values that make sense to be measured in microseconds.

We're actually discussing this elsewhere. I don't have
permission to paste the other side of the conversation, but
I can show you my calculations:

> The original implementation is fine until we reach 32.28 seconds, which
> as you predicted is 0x1000_0000:

> 0x10000000 * PRESCALER) / (CLOCK_SPEED_133_MHZ)
> (268435456 * 16       ) / (133  * 1000  * 1000) == 32.28 seconds

> If we spend >30 seconds at the u-boot commandline, which is not
> unreasonable by any stretch, then the kernel assumes responsibility for
> the remaining of the time spent at the prompt, which is obviously not
> acceptable for this usecase.

> I doubt x will never be < 133, as that will be 16 micro-seconds
> post timer-start or role-over. Do we really need that degree of
> resolution?

Am assuming by your response that I'm wrong somewhere, and the
resolution loss will be >16us?

> After the change, the result is changed to the worse for all low
> values of X, especially for the ranges from 16 through 132.
> 
> You lose accuracy here, and win nothing.
> 
> This makes no sense to me.
> 
> If you need a bigger number range, then use proper arithmetics. It';s
> available, just use it.

Would you be able to lend a hand here, as I'm no mathematician?

What are the correct arithmetics?

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

  reply	other threads:[~2012-11-21 10:02 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-20 14:33 [U-Boot] [PATCH 0/8] Adding boottime support Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 1/8] u8500: Correct unnecessary mathematical roll-over Lee Jones
2012-11-20 18:14   ` Wolfgang Denk
2012-11-21 10:02     ` Lee Jones [this message]
2012-11-21 13:51       ` Wolfgang Denk
2012-11-21 14:54         ` Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 2/8] u8500: Add utimer support Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 3/8] boottime: Add core boottime measurement support Lee Jones
2012-11-20 18:20   ` Wolfgang Denk
2012-11-21  9:50     ` Lee Jones
2012-11-21 13:44       ` Wolfgang Denk
2012-11-21 15:03         ` Lee Jones
2012-11-21 16:14           ` Wolfgang Denk
2012-11-21 17:26             ` Lee Jones
2012-11-21 19:04               ` Wolfgang Denk
2012-11-26  6:08   ` Simon Glass
2012-11-26  9:00     ` Lee Jones
2012-11-26 19:57       ` Simon Glass
2012-11-27  8:55         ` Lee Jones
2012-11-27 13:46           ` Wolfgang Denk
2012-11-27 14:28             ` Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 4/8] boottime: Apply some key boottime tags into common code Lee Jones
2012-11-20 18:22   ` Wolfgang Denk
2012-11-21  9:36     ` Lee Jones
2012-11-21 13:40       ` Wolfgang Denk
2012-11-21 15:07         ` Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 5/8] arm: Add boottime support for the ARM architecture Lee Jones
2012-11-20 15:11   ` Otavio Salvador
2012-11-20 15:52     ` Lee Jones
2012-11-20 18:24   ` Wolfgang Denk
2012-11-21  9:17     ` Lee Jones
2012-11-21  9:30       ` Wolfgang Denk
2012-11-21 10:13         ` Lee Jones
2012-11-21 13:58           ` Wolfgang Denk
2012-11-21 14:39             ` Lee Jones
2012-11-21 16:05               ` Wolfgang Denk
2012-11-21 17:48                 ` Lee Jones
2012-11-21 19:18                   ` Wolfgang Denk
2012-11-22 10:14                     ` Lee Jones
2012-11-22 13:04                       ` Wolfgang Denk
2012-11-22 16:08                         ` Lee Jones
2012-11-22 17:40                           ` Wolfgang Denk
2012-11-23 10:08                             ` Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 6/8] arm: Add some boottime tags into prime booting locations Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 7/8] href: Enable boottime functionality Lee Jones
2012-11-20 14:33 ` [U-Boot] [PATCH 8/8] snowball: " Lee Jones
2012-11-20 18:08 ` [U-Boot] [PATCH 0/8] Adding boottime support Wolfgang Denk
2012-11-21 10:03   ` Lee Jones

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=20121121100228.GJ28265@gmail.com \
    --to=lee.jones@linaro.org \
    --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