[U-Boot] [PATCH] ARM: S3C64XX: fix timer broken by relocation

[Albert ARIBAUD <albert.aribaud@free.fr>]

Le 12/11/2010 19:18, Darius Augulis a ?crit :
> Dear Minkyu Kang,
>
> On 11/12/2010 05:10 AM, Minkyu Kang wrote:
>> Dear Darius Augulis,
>>
>> On 2 November 2010 05:49, Darius Augulis<augulis.darius@gmail.com>   wrote:
>>> S3C64XX timer uses static variables which
>>> are initialized before relocation and used after it.
>>> Move them to global data structure.
>>>
>>> Signed-off-by: Darius Augulis<augulis.darius@gmail.com>
>>> ---
>>>    arch/arm/cpu/arm1176/s3c64xx/timer.c |   36 ++++++++++++++--------------------
>>>    arch/arm/include/asm/global_data.h   |    6 ++++++
>>>    2 files changed, 21 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/arm1176/s3c64xx/timer.c b/arch/arm/cpu/arm1176/s3c64xx/timer.c
>>> index 9768319..4e420cb 100644
>>> --- a/arch/arm/cpu/arm1176/s3c64xx/timer.c
>>> +++ b/arch/arm/cpu/arm1176/s3c64xx/timer.c
>>> @@ -43,7 +43,7 @@
>>>    #include<asm/arch/s3c6400.h>
>>>    #include<div64.h>
>>>
>>> -static ulong timer_load_val;
>>> +DECLARE_GLOBAL_DATA_PTR;
>>>
>>>    #define PRESCALER      167
>>>
>>> @@ -60,12 +60,6 @@ static inline ulong read_timer(void)
>>>          return timers->TCNTO4;
>>>    }
>>>
>>> -/* Internal tick units */
>>> -/* Last decremneter snapshot */
>>> -static unsigned long lastdec;
>>> -/* Monotonic incrementing timer */
>>> -static unsigned long long timestamp;
>>> -
>>>    int timer_init(void)
>>>    {
>>>          s3c64xx_timers *const timers = s3c64xx_get_base_timers();
>>> @@ -83,20 +77,20 @@ int timer_init(void)
>>>           * the prescaler automatically for other PCLK frequencies.
>>>           */
>>>          timers->TCFG0 = PRESCALER<<   8;
>>> -       if (timer_load_val == 0) {
>>> -               timer_load_val = get_PCLK() / PRESCALER * (100 / 4); /* 100s */
>>> +       if (gd->timer_load_val == 0) {
>>> +               gd->timer_load_val = get_PCLK() / PRESCALER * (100 / 4); /* 100s */
>>>                  timers->TCFG1 = (timers->TCFG1&   ~0xf0000) | 0x20000;
>>>          }
>>>
>>>          /* load value for 10 ms timeout */
>>> -       lastdec = timers->TCNTB4 = timer_load_val;
>>> +       gd->lastdec = timers->TCNTB4 = gd->timer_load_val;
>>>          /* auto load, manual update of Timer 4 */
>>>          timers->TCON = (timers->TCON&   ~0x00700000) | TCON_4_AUTO |
>>>                  TCON_4_UPDATE;
>>>
>>>          /* auto load, start Timer 4 */
>>>          timers->TCON = (timers->TCON&   ~0x00700000) | TCON_4_AUTO | COUNT_4_ON;
>>> -       timestamp = 0;
>>> +       gd->timestamp = 0;
>>>
>>>          return 0;
>>>    }
>>> @@ -113,16 +107,16 @@ unsigned long long get_ticks(void)
>>>    {
>>>          ulong now = read_timer();
>>>
>>> -       if (lastdec>= now) {
>>> +       if (gd->lastdec>= now) {
>>>                  /* normal mode */
>>> -               timestamp += lastdec - now;
>>> +               gd->timestamp += gd->lastdec - now;
>>>          } else {
>>>                  /* we have an overflow ... */
>>> -               timestamp += lastdec + timer_load_val - now;
>>> +               gd->timestamp += gd->lastdec + gd->timer_load_val - now;
>>>          }
>>> -       lastdec = now;
>>> +       gd->lastdec = now;
>>>
>>> -       return timestamp;
>>> +       return gd->timestamp;
>>>    }
>>>
>>>    /*
>>> @@ -132,14 +126,14 @@ unsigned long long get_ticks(void)
>>>    ulong get_tbclk(void)
>>>    {
>>>          /* We overrun in 100s */
>>> -       return (ulong)(timer_load_val / 100);
>>> +       return (ulong)(gd->timer_load_val / 100);
>>>    }
>>>
>>>    void reset_timer_masked(void)
>>>    {
>>>          /* reset time */
>>> -       lastdec = read_timer();
>>> -       timestamp = 0;
>>> +       gd->lastdec = read_timer();
>>> +       gd->timestamp = 0;
>>>    }
>>>
>>>    void reset_timer(void)
>>> @@ -150,7 +144,7 @@ void reset_timer(void)
>>>    ulong get_timer_masked(void)
>>>    {
>>>          unsigned long long res = get_ticks();
>>> -       do_div (res, (timer_load_val / (100 * CONFIG_SYS_HZ)));
>>> +       do_div (res, (gd->timer_load_val / (100 * CONFIG_SYS_HZ)));
>>>          return res;
>>>    }
>>>
>>> @@ -161,7 +155,7 @@ ulong get_timer(ulong base)
>>>
>>>    void set_timer(ulong t)
>>>    {
>>> -       timestamp = t * (timer_load_val / (100 * CONFIG_SYS_HZ));
>>> +       gd->timestamp = t * (gd->timer_load_val / (100 * CONFIG_SYS_HZ));
>>>    }
>>>
>>>    void __udelay(unsigned long usec)
>>> diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
>>> index ada3fbb..69fb7b6 100644
>>> --- a/arch/arm/include/asm/global_data.h
>>> +++ b/arch/arm/include/asm/global_data.h
>>> @@ -61,6 +61,12 @@ typedef      struct  global_data {
>>>          unsigned long   tbu;
>>>          unsigned long long      timer_reset_value;
>>>    #endif
>>> +#ifdef CONFIG_S3C64XX
>>> +       /* "static data" needed by s3c64xx's timer.c */
>>> +       unsigned long           timer_load_val;
>>> +       unsigned long           lastdec;
>>> +       unsigned long long      timestamp;
>>> +#endif
>>>          unsigned long   relocaddr;      /* Start address of U-Boot in RAM */
>>>          phys_size_t     ram_size;       /* RAM size */
>>>          unsigned long   mon_len;        /* monitor len */
>>>
>>
>> I think it's not good way.
>> We just need to fix the timer_init function.
>> Because we can use the static variables after relocation.
>
> at least it's working one and not new - at91 does it the same way. But
> please send your patch, if it's better solution then I will be happy to
> have it instead of mine.
> Please remember, that these static variables are used in almost all
> timer functions and timer is initialised before relocation. So you
> should not change them during relocation to have correct ones after it.
> Looking forward for your solution.
>
> regards,
> Darius.

FWIW, there already is a solution based on statics and post-relocation 
initialization for orion5x. The principle there is that the timer is not 
used before calling board_init_r, so we don't need initializing 
timestamp before relocation.

I'm not saying that's how it should be done, mind; it's just how I did 
it then.

Amicalement,
-- 
Albert.