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

[Pillai, Manikandan]

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


Regards
Mani

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