[U-Boot] [PATCH] Update s3c24x0 timer implementation

[Mark Norman]

Hi Marek Vasut,

>>
>> I have attached an updated patch below which hopefully addresses the
>> other issues you highlighted.
>
> Hehe, you should RTFM ;-) http://www.denx.de/wiki/U-Boot/Patches
>
> Generally, use git send-email to send the patch.
>

Thanks for your feedback.  I have submitted an updated patch using git
send-email.  I think I got the formatting right this time. :)


>>
>> The s3c24x0 timer has been updated to use the global_data struct.
>> Restructured code based on other timer.c files.
>> Updated comments and several parameters.
>>
>> Signed-off-by: Mark Norman <mpnorman at gmail.com>
>> ---
>>  arch/arm/cpu/arm920t/s3c24x0/timer.c |  155
>> +++++++++++++-------------------- arch/arm/include/asm/global_data.h   |
>>  4 +
>>  2 files changed, 65 insertions(+), 94 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm920t/s3c24x0/timer.c
>> b/arch/arm/cpu/arm920t/s3c24x0/timer.c
>> index 9571870..5695c62 100644
>> --- a/arch/arm/cpu/arm920t/s3c24x0/timer.c
>> +++ b/arch/arm/cpu/arm920t/s3c24x0/timer.c
>> @@ -35,142 +35,109 @@
>>  #include <asm/io.h>
>>  #include <asm/arch/s3c24x0_cpu.h>
>>
>> -int timer_load_val = 0;
>> -static ulong timer_clk;
>> +DECLARE_GLOBAL_DATA_PTR;
>>
>> -/* macro to read the 16 bit timer */
>> -static inline ulong READ_TIMER(void)
>> +/* Read the 16 bit timer */
>> +static inline ulong read_timer(void)
>>  {
>>       struct s3c24x0_timers *timers = s3c24x0_get_base_timers();
>>
>>       return readl(&timers->tcnto4) & 0xffff;
>>  }
>>
>> -static ulong timestamp;
>> -static ulong lastdec;
>> -
>>  int timer_init(void)
>>  {
>> +     /*
>> +      * PWM Timer 4 is used because it has no output.
>> +      * Prescaler is hard fixed at 250, divider at 2.
>> +      * This generates a Timer clock frequency of 100kHz (@PCLK=50MHz) and
>> +      * therefore 10us timer ticks.
>> +      */
>> +     const ulong prescaler = 250;
>>       struct s3c24x0_timers *timers = s3c24x0_get_base_timers();
>>       ulong tmr;
>>
>> -     /* use PWM Timer 4 because it has no output */
>> -     /* prescaler for Timer 4 is 16 */
>> -     writel(0x0f00, &timers->tcfg0);
>> -     if (timer_load_val == 0) {
>> -             /*
>> -              * for 10 ms clock period @ PCLK with 4 bit divider = 1/2
>> -              * (default) and prescaler = 16. Should be 10390
>> -              * @33.25MHz and 15625 @ 50 MHz
>> -              */
>> -             timer_load_val = get_PCLK() / (2 * 16 * 100);
>> -             timer_clk = get_PCLK() / (2 * 16);
>> -     }
>> -     /* load value for 10 ms timeout */
>> -     lastdec = timer_load_val;
>> -     writel(timer_load_val, &timers->tcntb4);
>> -     /* auto load, manual update of timer 4 */
>> +     /* Set prescaler for Timer 4 */
>> +     writel((prescaler-1) << 8, &timers->tcfg0);
>
> Can we get rid of the magic numbers please?

I have declared some bitfields to remove the magic numbers.

>
>> +
>> +     /* Calculate timer freq, approx 100kHz @ PCLK=50MHz. */
>> +     gd->timer_rate_hz = get_PCLK() / (2 * prescaler);
>
> Can get_PCLK() be renamed to be lower-case only?

I haven't addressed this in my latest patch as it would be quite a
large change that I though might be worthy of its own patch (there are
several related functions get_FCLK, get_HCLK, etc. which could be
updated at the same time).


>> diff --git a/arch/arm/include/asm/global_data.h
>> b/arch/arm/include/asm/global_data.h
>> index fac98d5..b836915 100644
>> --- a/arch/arm/include/asm/global_data.h
>> +++ b/arch/arm/include/asm/global_data.h
>> @@ -67,6 +67,10 @@ typedef    struct  global_data {
>>  #ifdef CONFIG_IXP425
>>       unsigned long   timestamp;
>>  #endif
>> +#ifdef CONFIG_S3C24X0
>> +     unsigned long   lastdec;
>> +     unsigned long   timestamp;
>> +#endif
>
> I wonder about this change ...
>

The latest patch no longer creates new variables in the global_data
struct and instead reuses some of the existing ones.

Any more feedback would be welcomed.

Cheers

Mark Norman