[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Ladislav Michl]

On Mon, Apr 20, 2009 at 08:27:34PM +0200, Dirk Behme wrote:
> Dear Ladis,
>
> ah, and some remarks on the patch itself ;)

Thanks, I'm glad someone still cares about ancient stuff.

> Ladislav Michl wrote:
>> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
>> get_timer.
>>
>> Signed-off-by: Ladislav Michl <ladis at linux-mips.org>
>>
>> diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
>> index e5c77f7..a22be66 100644
>> --- a/cpu/arm925t/interrupts.c
>> +++ b/cpu/arm925t/interrupts.c
> ...
>> -#define TIMER_LOAD_VAL 0xffffffff
>> +#define TIMER_LOAD_VAL	0xffffffff
>> +#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))
>
> Just to get an idea of the math:
>
> CONFIG_SYS_CLK_FREQ is 12000000 (12MHz)? This is divided by 256, so  
> TIMER_CLOCK is 46875Hz? A free running 32-bit count down timer is used  
> starting at 0xffffffff? Underflow (0) is reached after ~91626s ==  
> ~25hours with this?
>
> Please correct if something is wrong ;)

Math is perfectly correct, except in my case CONFIG_SYS_CLK_FREQ is 150MHz,
so resolution is actually 12.5 times better. Perhaps I should modify those
boards wich uses 12MHz clock to use smaller divisor, but let's wait for more
comments first.

>> -/* delay x useconds AND preserve advance timestamp value */
>> +/* delay usec microseconds preserving timestamp value */
>
> Hmm, 'usec microseconds' sounds somehow confusing?

It depends. 'usec' is obviously variable name and 'microsecond' is a time
unit, while 'x' is unreferenced variable and 'usec' is abbreviation.
And I prefer former (or deleting that part of comment entirely).

>>  void udelay (unsigned long usec)
>>  {
> ...
>> +	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
>> +	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>
> The first '1000' should be CONFIG_SYS_HZ? I.e.

No. Actually it should read 'usec * (TIMER_CLOCK / (1000 * 1000))', where
one '1000' is to get miliseconds and other brings you to microseconds
digit place. But integer math needs former writing.

> (TIMER_CLOCK / CONFIG_SYS_HZ) / 1000;
>
> ?
>
> In my udelay patch, I use
>
> +	tmo = usec * (TIMER_CLOCK / CONFIG_SYS_HZ);
> +	tmo /= 1000;
>
> From some printf debugging, for OMAP3 there was a difference doing it in 
> one or two lines. If I remember correctly due to integer vs floating 
> point math and rounding. What do you think?

I think all that udelay code is pointless once CONFIG_SYS_HZ always
_have_ to be 1000 and can be simplyfied.

> Running OMAP3 counter with 1.625MHz, max udelay resolution is ~1.62us.  
> If you run with 46875Hz, you have max udelay resolution of ~22us?

See above, it is ~1.7us.

>> +	while (tmo > 0) {
>> +		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>> +		if (last < now) /* count down timer underflow */
>> +			tmo -= TIMER_LOAD_VAL - now + last;
>> +		else
>> +			tmo -= last - now;
>> +		last = now;
>
> I will think about this, I always need some time for this clock math ;)
>
> In contrast to OMAP3 your timer here counts down, right? So while OMAP1 
> has to deal with underflow, OMAP3 will need overflow handling, right?

Right, but the key point here is to unbind udelay from get_timer as now
get_timer works with miliseconds resolution.

	ladis