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 <la...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
