[U-Boot] [PATCH 1/1] Changes for fixing the timer to 1 msec and

[Dirk Behme]

Dear Mani,

Pillai, Manikandan wrote:
> Hi,
> 
> I was kind of concerned that I am changing some of the common files.
> 
> Got into a loop while tring to fix some issue for OMAP3.
> 
> I have got some queries on timer.
> 
> 1. Is the timer interrupt enabled and tick incremented after every expiry.
>    In the patch, I have tried to return the ms count of the free running timer.
>    The timer 0 is a free running timer and it overflows at 0xffffffff. The count
>    read from the timer is right shifted by 6 to give the count in msec.
> 
> 2. Since the delay is given in msec, I add the delay with the get_timer(0) count
>    So that the approximately correct delay can be counted
> 
> 3. I would like to know the constraints for the get_timer()

I'm not sure if there is any documentation how U-Boot's timer code 
should work. If not, and if nobody else likes to give some hints, the 
best way would be to look how other boards do it. I don't know what 
the best example would be, but looking at OMAP1, OMAP2 or DaVinci 
could help. Sorry, but as already mentioned I'm not familiar with 
U-Boot's timer code, too. Looks like the timer code actually used is 
the more or less the initial port from TI.

Best regards

Dirk

>> -----Original Message-----
>> From: Dirk Behme [mailto:dirk.behme at googlemail.com]
>> Sent: Tuesday, March 10, 2009 11:27 PM
>> To: Pillai, Manikandan
>> Cc: u-boot at lists.denx.de
>> Subject: Re: [PATCH 1/1] Changes for fixing the timer to 1 msec and
>>
>> Hi Mani,
>>
>> Manikandan Pillai wrote:
>>> The cmd_misc.c was changed to fix sleep.
>>> nand_base.c was fixed to fix the wait.
>> Should this be the fix for
>>
>> http://lists.denx.de/pipermail/u-boot/2009-February/048107.html
>>
>> ?
>>
>> I'm not familiar with U-Boot timer handling, so just some questions.
>> Maybe the U-Boot experts like to comment, too.
>>
>>> Signed-off-by: Manikandan Pillai <mani.pillai at ti.com>
>>> ---
>>>  common/cmd_misc.c                   |    1 +
>>>  cpu/arm_cortexa8/omap3/interrupts.c |   13 ++++---------
>>>  drivers/mtd/nand/nand_base.c        |    2 ++
>>>  include/configs/omap3_evm.h         |    2 +-
>>>  4 files changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/common/cmd_misc.c b/common/cmd_misc.c
>>> index 024299a..e3e64c0 100644
>>> --- a/common/cmd_misc.c
>>> +++ b/common/cmd_misc.c
>>> @@ -38,6 +38,7 @@ int do_sleep (cmd_tbl_t *cmdtp, int flag, int argc, char
>> *argv[])
>>>  	}
>>>
>>>  	delay = simple_strtoul(argv[1], NULL, 10) * CONFIG_SYS_HZ;
>>> +	delay += get_timer(0);
>> As I understand above error report there is an issue with OMAP3 timer
>> not returning ms. Why do we have to touch cmd_misc.c for *all*
>> architectures then?
>>
>>>  	while (get_timer(start) < delay) {
>>>  		if (ctrlc ()) {
>>> diff --git a/cpu/arm_cortexa8/omap3/interrupts.c
>> b/cpu/arm_cortexa8/omap3/interrupts.c
>>> index 9e9817d..5bfe6fc 100644
>>> --- a/cpu/arm_cortexa8/omap3/interrupts.c
>>> +++ b/cpu/arm_cortexa8/omap3/interrupts.c
>>> @@ -193,7 +193,7 @@ void reset_timer(void)
>>>
>>>  ulong get_timer(ulong base)
>>>  {
>>> -	return get_timer_masked() - base;
>>> +	return get_timer_masked();
>> Why don't we have to deal with base here any more?
>>
>>>  }
>>>
>>>  void set_timer(ulong t)
>>> @@ -238,14 +238,9 @@ void reset_timer_masked(void)
>>>
>>>  ulong get_timer_masked(void)
>>>  {
>>> -	ulong now = readl(&timer_base->tcrr); /* current tick value */
>>> -
>>> -	if (now >= lastinc)	/* normal mode (non roll) */
>>> -		/* move stamp fordward with absoulte diff ticks */
>>> -		timestamp += (now - lastinc);
>>> -	else	/* we have rollover of incrementer */
>>> -		timestamp += (0xFFFFFFFF - lastinc) + now;
>>> -	lastinc = now;
>>> +	timestamp = readl(&timer_base->tcrr); /* current tick value */
>>> +	/* get_timer() has to return time in milliseconds */
>>> +	timestamp >>= 6;
>> Why don't we have to deal with timer overflow here any more?
>>
>> As mentioned I haven't looked into the details, but why wouldn't it be
>> sufficient to just change first line of get_timer_masked() to
>> something like
>>
>> ulong now = readl(&timer_base->tcrr) >> 6; /* current tick value */
>>
>> to get ms?
>>
>>>  	return timestamp;
>>>  }
>>>
>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>> index d33fee2..283b456 100644
>>> --- a/drivers/mtd/nand/nand_base.c
>>> +++ b/drivers/mtd/nand/nand_base.c
>>> @@ -509,6 +509,7 @@ void nand_wait_ready(struct mtd_info *mtd)
>>>  	struct nand_chip *chip = mtd->priv;
>>>  	u32 timeo = (CONFIG_SYS_HZ * 20) / 1000;
>>>
>>> +	timeo += get_timer(0);
>>>  	reset_timer();
>> Same as above here and below: Why do we have to touch global code for
>> *all* architectures?
>>
>>>  	/* wait until command is processed or timeout occures */
>>> @@ -854,6 +855,7 @@ static int nand_wait(struct mtd_info *mtd, struct
>> nand_chip *this)
>>>  	else
>>>  		timeo = (CONFIG_SYS_HZ * 20) / 1000;
>>>
>>> +	timeo += get_timer(0);
>>>  	if ((state == FL_ERASING) && (this->options & NAND_IS_AND))
>>>  		this->cmdfunc(mtd, NAND_CMD_STATUS_MULTI, -1, -1);
>>>  	else
>>> diff --git a/include/configs/omap3_evm.h b/include/configs/omap3_evm.h
>>> index bf5614e..8d6e5fe 100644
>>> --- a/include/configs/omap3_evm.h
>>> +++ b/include/configs/omap3_evm.h
>>> @@ -230,7 +230,7 @@
>>>
>>>  #define CONFIG_SYS_TIMERBASE		OMAP34XX_GPT2
>>>  #define CONFIG_SYS_PVT			V_PVT	/* 2^(pvt+1) */
>>> -#define CONFIG_SYS_HZ			((V_SCLK) / (2 << CONFIG_SYS_PVT))
>>> +#define CONFIG_SYS_HZ			1000
>> Would be nice if you could change this for all OMAP3 boards.
>>
>> Best regards
>>
>> Dirk
> 
>