Patchwork [U-Boot,1/8] u8500: Correct unnecessary mathematical roll-over

login
register
mail settings
Submitter Lee Jones
Date Nov. 20, 2012, 2:33 p.m.
Message ID <1353422034-28107-2-git-send-email-lee.jones@linaro.org>
Download mbox | patch
Permalink /patch/200354/
State Rejected
Delegated to: Wolfgang Denk
Headers show

Comments

Lee Jones - Nov. 20, 2012, 2:33 p.m.
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.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/cpu/armv7/u8500/timer.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Wolfgang Denk - Nov. 20, 2012, 6:14 p.m.
Dear Lee Jones,

In message <1353422034-28107-2-git-send-email-lee.jones@linaro.org> you wrote:
> 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?

> --- 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.

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.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 10:02 a.m.
> > 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
Wolfgang Denk - Nov. 21, 2012, 1:51 p.m.
Dear Lee Jones,

In message <20121121100228.GJ28265@gmail.com> you wrote:
>
> > > -#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.

This makes no sense to me.  An overflow will not happen before
UINT_MAX/16 = 268435455 = 268 seconds.

Anyway.  If you have overflof problems, then use proper arithmetics.

> > 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?

There are routines like do_div() or lldiv() etc. that can be used when
32 bit arithmetics is really not good enough.

However, I fail to see why that should even be needed here.

Best regards,

Wolfgang Denk
Lee Jones - Nov. 21, 2012, 2:54 p.m.
> > > > -#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.
> 
> This makes no sense to me.  An overflow will not happen before
> UINT_MAX/16 = 268435455 = 268 seconds.

Right, but that's the timer.

The issue here is not that the timer is overflowing, it's the
arithmetics that takes place once the timer value is obtained.
If a value of >0x10000000 is read, then we carry out the
arithmetic above, then other registers overflow.

> Anyway.  If you have overflof problems, then use proper arithmetics.
> 
> > > 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?
> 
> There are routines like do_div() or lldiv() etc. that can be used when
> 32 bit arithmetics is really not good enough.

Ah ha, thanks.
 
> However, I fail to see why that should even be needed here.

As above.

Patch

diff --git a/arch/arm/cpu/armv7/u8500/timer.c b/arch/arm/cpu/armv7/u8500/timer.c
index 79aad99..40326d8 100644
--- 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)
 #define USEC_TO_COUNT(x)	((x) * 133 / 16)
 #define TICKS_PER_HZ		(TIMER_CLOCK / CONFIG_SYS_HZ)
 #define TICKS_TO_HZ(x)		((x) / TICKS_PER_HZ)