[PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Ian Campbell]

From: Ian Campbell <ian.campbell@citrix.com>

u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
internal get_timer which is in ms, which isn't all that clearly documented but
is obvious from the use within u-boot and is mentioned in
http://www.denx.de/wiki/U-Boot/TaskTimerAPI.

This was put wrong in 4e13e84e56f7 "Fix timer units".

Without this it takes 5000s to count down to the automatic boot of the selected
option (or I assume it would, I never waited...)

Cc: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
Cc: Leif Lindholm <leif.lindholm@arm.com>
---
 grub-core/kern/uboot/init.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/kern/uboot/init.c b/grub-core/kern/uboot/init.c
index b108de3..2e9d382 100644
--- a/grub-core/kern/uboot/init.c
+++ b/grub-core/kern/uboot/init.c
@@ -66,8 +66,7 @@ uboot_timer_ms (void)
   if (cur < last)
     high++;
   last = cur;
-  return grub_divmod64 ((((grub_uint64_t) high) << 32) | cur,
-			1000, 0);
+  return (((grub_uint64_t) high) << 32) | cur;
 }
 
 void
-- 
1.7.10.4

Re: [PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

Committed, thanks
On 15.12.2013 16:23, Ian Campbell wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
> internal get_timer which is in ms, which isn't all that clearly documented but
> is obvious from the use within u-boot and is mentioned in
> http://www.denx.de/wiki/U-Boot/TaskTimerAPI.
> 
> This was put wrong in 4e13e84e56f7 "Fix timer units".
> 
> Without this it takes 5000s to count down to the automatic boot of the selected
> option (or I assume it would, I never waited...)
> 
> Cc: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> Cc: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  grub-core/kern/uboot/init.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/grub-core/kern/uboot/init.c b/grub-core/kern/uboot/init.c
> index b108de3..2e9d382 100644
> --- a/grub-core/kern/uboot/init.c
> +++ b/grub-core/kern/uboot/init.c
> @@ -66,8 +66,7 @@ uboot_timer_ms (void)
>    if (cur < last)
>      high++;
>    last = cur;
> -  return grub_divmod64 ((((grub_uint64_t) high) << 32) | cur,
> -			1000, 0);
> +  return (((grub_uint64_t) high) << 32) | cur;
>  }
>  
>  void
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On 15.12.2013 16:23, Ian Campbell wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
> internal get_timer which is in ms, which isn't all that clearly documented but
> is obvious from the use within u-boot and is mentioned in
> http://www.denx.de/wiki/U-Boot/TaskTimerAPI.
> 
During tests on my raspberry pi, I actually experienced the exact
opposite. On PI timer API is in microseconds.
Are you sure you made no mistake?

> This was put wrong in 4e13e84e56f7 "Fix timer units".
> 
> Without this it takes 5000s to count down to the automatic boot of the selected
> option (or I assume it would, I never waited...)
> 
> Cc: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> Cc: Leif Lindholm <leif.lindholm@arm.com>
> ---
>  grub-core/kern/uboot/init.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/grub-core/kern/uboot/init.c b/grub-core/kern/uboot/init.c
> index b108de3..2e9d382 100644
> --- a/grub-core/kern/uboot/init.c
> +++ b/grub-core/kern/uboot/init.c
> @@ -66,8 +66,7 @@ uboot_timer_ms (void)
>    if (cur < last)
>      high++;
>    last = cur;
> -  return grub_divmod64 ((((grub_uint64_t) high) << 32) | cur,
> -			1000, 0);
> +  return (((grub_uint64_t) high) << 32) | cur;
>  }
>  
>  void
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Ian Campbell]

On Sat, 2013-12-21 at 23:23 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
wrote:
> On 15.12.2013 16:23, Ian Campbell wrote:
> > From: Ian Campbell <ian.campbell@citrix.com>
> > 
> > u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
> > internal get_timer which is in ms, which isn't all that clearly documented but
> > is obvious from the use within u-boot and is mentioned in
> > http://www.denx.de/wiki/U-Boot/TaskTimerAPI.
> > 
> During tests on my raspberry pi, I actually experienced the exact
> opposite. On PI timer API is in microseconds.
> Are you sure you made no mistake?

I'm quite sure that on the Midway platform get_timer was returning ms
and the 5s grub countdown took 5s after my fix and some interminably
long time before it.

Sadly actual documentation of the u-boot API is a bit thin on the
ground, but get_timer==ms is also corroborated by some ad-hoc googling I
did (e.g. resulting in the above link) as well as inspection of some
random u-boot ports. Some ports have an explicit get_timer_us function,
which adds credence to the idea that get_timer is in ms instead.

Ian.

Re: [PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Ian Campbell]

On Sun, 2013-12-22 at 01:21 +0000, Ian Campbell wrote:
> On Sat, 2013-12-21 at 23:23 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
> wrote:
> > On 15.12.2013 16:23, Ian Campbell wrote:
> > > From: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
> > > internal get_timer which is in ms, which isn't all that clearly documented but
> > > is obvious from the use within u-boot and is mentioned in
> > > http://www.denx.de/wiki/U-Boot/TaskTimerAPI.
> > > 
> > During tests on my raspberry pi, I actually experienced the exact
> > opposite. On PI timer API is in microseconds.
> > Are you sure you made no mistake?
> 
> I'm quite sure that on the Midway platform get_timer was returning ms
> and the 5s grub countdown took 5s after my fix and some interminably
> long time before it.
> 
> Sadly actual documentation of the u-boot API is a bit thin on the
> ground, but get_timer==ms is also corroborated by some ad-hoc googling I
> did (e.g. resulting in the above link) as well as inspection of some
> random u-boot ports. Some ports have an explicit get_timer_us function,
> which adds credence to the idea that get_timer is in ms instead.

See also this patch to u-boot upstream:

commit 5eaa215607c8668bfa6a7183407eba8fec63d648
Author: Stephen Warren <swarren@wwwdotorg.org>
Date:   Wed Mar 27 18:43:23 2013 +0000

    ARM: bcm2835: fix get_timer() to return ms
    
    Apparently, CONFIG_SYS_HZ must be 1000. Change this, and fix the timer
    driver to conform to this.
    
    Have the timer implementation export a custom API get_timer_us() for use
    by the BCM2835 MMC API, which needs us resolution for a HW workaround.
    
    Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>

and from README:

- CPU timer options:
                CONFIG_SYS_HZ

                The frequency of the timer returned by get_timer().
                get_timer() must operate in milliseconds and this CONFIG
                option must be set to 1000.

Ian.

Re: [PATCH] * grub-core/kern/uboot/init.c (uboot_timer_ms) correct units

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On 22.12.2013 02:29, Ian Campbell wrote:
> On Sun, 2013-12-22 at 01:21 +0000, Ian Campbell wrote:
>> On Sat, 2013-12-21 at 23:23 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
>> wrote:
>>> On 15.12.2013 16:23, Ian Campbell wrote:
>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>>
>>>> u-boot's API_GET_TIMER returns the current time in ms by directly exposing the
>>>> internal get_timer which is in ms, which isn't all that clearly documented but
>>>> is obvious from the use within u-boot and is mentioned in
>>>> http://www.denx.de/wiki/U-Boot/TaskTimerAPI.
>>>>
>>> During tests on my raspberry pi, I actually experienced the exact
>>> opposite. On PI timer API is in microseconds.
>>> Are you sure you made no mistake?
>>
>> I'm quite sure that on the Midway platform get_timer was returning ms
>> and the 5s grub countdown took 5s after my fix and some interminably
>> long time before it.
>>
>> Sadly actual documentation of the u-boot API is a bit thin on the
>> ground, but get_timer==ms is also corroborated by some ad-hoc googling I
>> did (e.g. resulting in the above link) as well as inspection of some
>> random u-boot ports. Some ports have an explicit get_timer_us function,
>> which adds credence to the idea that get_timer is in ms instead.
> 
> See also this patch to u-boot upstream:
> 
Ok, I've kept current code as generic and added own timer implementation
for raspberry pie
> commit 5eaa215607c8668bfa6a7183407eba8fec63d648
> Author: Stephen Warren <swarren@wwwdotorg.org>
> Date:   Wed Mar 27 18:43:23 2013 +0000
> 
>     ARM: bcm2835: fix get_timer() to return ms
>     
>     Apparently, CONFIG_SYS_HZ must be 1000. Change this, and fix the timer
>     driver to conform to this.
>     
>     Have the timer implementation export a custom API get_timer_us() for use
>     by the BCM2835 MMC API, which needs us resolution for a HW workaround.
>     
>     Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> and from README:
> 
> - CPU timer options:
>                 CONFIG_SYS_HZ
> 
>                 The frequency of the timer returned by get_timer().
>                 get_timer() must operate in milliseconds and this CONFIG
>                 option must be set to 1000.
> 
> Ian.
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]