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

[Darius Augulis <augulis.darius@gmail.com>]

Dear Albert,

On 11/12/2010 09:25 PM, Albert ARIBAUD wrote:
> 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.

in this case it seems like timer must be initialised after relocation.
But I don't know if it could be correct for all ARM architectures. Maybe 
some of them use timer before relocation.
Global data is good place to store important static variables because 
they are valid before and after relocation and it could be common for 
all architectures. Since we have automatic size calculation of global 
data structure there should not be a problem to add several additional 
bytes specific to every CPU.
That's why I did like this and this is my opinion about this.

Darius.

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