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

[Dirk Behme <dirk.behme@googlemail.com>]

Dear Ladis,

Ladislav Michl wrote:
> 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@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. 

Ok. Is this 150MHz defined in one of the configs you modify with this 
patch or do you use a custom config? Just curious ;)

> Perhaps I should modify those
> boards wich uses 12MHz clock to use smaller divisor, 

Yes, this should be easily doable by changing CONFIG_SYS_PTV.

> but let's wait for more
> comments first.

I hope there will be some ;)

>>> -/* 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.

Ok, understood. Thanks for the hint!

>> (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.

I hope I got it right with the updated patch sent some minutes ago.

Best regards

Dirk