[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Nishanth Menon]

Having a loop with a counter is no timing guarentee for timing
accuracy or compiler optimizations. For e.g. the same loop counter
which runs when the MPU is running at 600MHz will timeout in around
half the time when running at 1GHz. or the example where GCC 4.5
compiles with different optimization compared to GCC 4.4.
use a udelay(10) to ensure we have a predictable delay.
use an emperical number - 100000 uSec ~= 1sec for a worst case timeout.
This should never happen, and is adequate imaginary condition for us
to fail with timeout.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
V2: additional cleanups + made MAX_RETRY a macro for reuse throughout
the file. tested on PandaBoard with 1GHz boot frequency and GCC4.5 on
u-boot 2010.09 + mmc patches. Requesting testing on other platforms

V1: http://www.mail-archive.com/u-boot at lists.denx.de/msg40969.html

 drivers/mmc/omap_hsmmc.c |  114 +++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 97 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c
index 9271470..f7bdfe9 100644
--- a/drivers/mmc/omap_hsmmc.c
+++ b/drivers/mmc/omap_hsmmc.c
@@ -30,6 +30,7 @@
 #include <twl4030.h>
 #include <asm/io.h>
 #include <asm/arch/mmc_host_def.h>
+#define MAX_RETRY	100000
 
 static int mmc_read_data(hsmmc_t *mmc_base, char *buf, unsigned int size);
 static int mmc_write_data(hsmmc_t *mmc_base, const char *buf, unsigned int siz);
@@ -70,18 +71,32 @@ unsigned char mmc_board_init(hsmmc_t *mmc_base)
 
 void mmc_init_stream(hsmmc_t *mmc_base)
 {
+	int retry = MAX_RETRY;
 
 	writel(readl(&mmc_base->con) | INIT_INITSTREAM, &mmc_base->con);
 
 	writel(MMC_CMD0, &mmc_base->cmd);
-	while (!(readl(&mmc_base->stat) & CC_MASK))
-		;
+	while (!(readl(&mmc_base->stat) & CC_MASK)) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for cc!\n", __func__);
+			return;
+		}
+	}
 	writel(CC_MASK, &mmc_base->stat)
 		;
 	writel(MMC_CMD0, &mmc_base->cmd)
 		;
-	while (!(readl(&mmc_base->stat) & CC_MASK))
-		;
+	retry = MAX_RETRY;
+	while (!(readl(&mmc_base->stat) & CC_MASK)) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for cc2!\n", __func__);
+			return;
+		}
+	}
 	writel(readl(&mmc_base->con) & ~INIT_INITSTREAM, &mmc_base->con);
 }
 
@@ -91,16 +106,31 @@ static int mmc_init_setup(struct mmc *mmc)
 	hsmmc_t *mmc_base = (hsmmc_t *)mmc->priv;
 	unsigned int reg_val;
 	unsigned int dsor;
+	int retry = MAX_RETRY;
 
 	mmc_board_init(mmc_base);
 
 	writel(readl(&mmc_base->sysconfig) | MMC_SOFTRESET,
 		&mmc_base->sysconfig);
-	while ((readl(&mmc_base->sysstatus) & RESETDONE) == 0)
-		;
+	while ((readl(&mmc_base->sysstatus) & RESETDONE) == 0) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for cc2!\n", __func__);
+			return TIMEOUT;
+		}
+	}
 	writel(readl(&mmc_base->sysctl) | SOFTRESETALL, &mmc_base->sysctl);
-	while ((readl(&mmc_base->sysctl) & SOFTRESETALL) != 0x0)
-		;
+	retry = MAX_RETRY;
+	while ((readl(&mmc_base->sysctl) & SOFTRESETALL) != 0x0) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for softresetall!\n",
+				__func__);
+			return TIMEOUT;
+		}
+	}
 	writel(DTW_1_BITMODE | SDBP_PWROFF | SDVS_3V0, &mmc_base->hctl);
 	writel(readl(&mmc_base->capa) | VS30_3V0SUP | VS18_1V8SUP,
 		&mmc_base->capa);
@@ -116,8 +146,15 @@ static int mmc_init_setup(struct mmc *mmc)
 		(ICE_STOP | DTO_15THDTO | CEN_DISABLE));
 	mmc_reg_out(&mmc_base->sysctl, ICE_MASK | CLKD_MASK,
 		(dsor << CLKD_OFFSET) | ICE_OSCILLATE);
-	while ((readl(&mmc_base->sysctl) & ICS_MASK) == ICS_NOTREADY)
-		;
+	retry = MAX_RETRY;
+	while ((readl(&mmc_base->sysctl) & ICS_MASK) == ICS_NOTREADY) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for ics!\n", __func__);
+			return TIMEOUT;
+		}
+	}
 	writel(readl(&mmc_base->sysctl) | CEN_ENABLE, &mmc_base->sysctl);
 
 	writel(readl(&mmc_base->hctl) | SDBP_PWRON, &mmc_base->hctl);
@@ -137,14 +174,27 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 {
 	hsmmc_t *mmc_base = (hsmmc_t *)mmc->priv;
 	unsigned int flags, mmc_stat;
-	unsigned int retry = 0x100000;
+	int retry = MAX_RETRY;
 
 
-	while ((readl(&mmc_base->pstate) & DATI_MASK) == DATI_CMDDIS)
-		;
+	while ((readl(&mmc_base->pstate) & DATI_MASK) == DATI_CMDDIS) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for cmddis!\n", __func__);
+			return TIMEOUT;
+		}
+	}
 	writel(0xFFFFFFFF, &mmc_base->stat);
-	while (readl(&mmc_base->stat))
-		;
+	retry = MAX_RETRY;
+	while (readl(&mmc_base->stat)) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for stat!\n", __func__);
+			return TIMEOUT;
+		}
+	}
 	/*
 	 * CMDREG
 	 * CMDIDX[13:8]	: Command index
@@ -200,8 +250,11 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	writel(cmd->cmdarg, &mmc_base->arg);
 	writel((cmd->cmdidx << 24) | flags, &mmc_base->cmd);
 
+	retry = MAX_RETRY;
 	do {
 		mmc_stat = readl(&mmc_base->stat);
+		if (!mmc_stat)
+			udelay(10);
 		retry--;
 	} while ((mmc_stat == 0) && (retry > 0));
 
@@ -253,8 +306,18 @@ static int mmc_read_data(hsmmc_t *mmc_base, char *buf, unsigned int size)
 	count /= 4;
 
 	while (size) {
+		int retry = MAX_RETRY;
 		do {
 			mmc_stat = readl(&mmc_base->stat);
+			if (!mmc_stat) {
+				retry--;
+				udelay(10);
+			}
+			if (!retry) {
+				printf("%s: timedout waiting for status!\n",
+						__func__);
+				return TIMEOUT;
+			}
 		} while (mmc_stat == 0);
 
 		if ((mmc_stat & ERRI_MASK) != 0)
@@ -298,8 +361,18 @@ static int mmc_write_data(hsmmc_t *mmc_base, const char *buf, unsigned int size)
 	count /= 4;
 
 	while (size) {
+		int retry = MAX_RETRY;
 		do {
 			mmc_stat = readl(&mmc_base->stat);
+			if (!mmc_stat) {
+				retry--;
+				udelay(10);
+			}
+			if (!retry) {
+				printf("%s: timedout waiting for status!\n",
+						__func__);
+				return TIMEOUT;
+			}
 		} while (mmc_stat == 0);
 
 		if ((mmc_stat & ERRI_MASK) != 0)
@@ -334,6 +407,7 @@ static void mmc_set_ios(struct mmc *mmc)
 {
 	hsmmc_t *mmc_base = (hsmmc_t *)mmc->priv;
 	unsigned int dsor = 0;
+	int retry = MAX_RETRY;
 
 	/* configue bus width */
 	switch (mmc->bus_width) {
@@ -372,8 +446,14 @@ static void mmc_set_ios(struct mmc *mmc)
 	mmc_reg_out(&mmc_base->sysctl, ICE_MASK | CLKD_MASK,
 				(dsor << CLKD_OFFSET) | ICE_OSCILLATE);
 
-	while ((readl(&mmc_base->sysctl) & ICS_MASK) == ICS_NOTREADY)
-		;
+	while ((readl(&mmc_base->sysctl) & ICS_MASK) == ICS_NOTREADY) {
+		retry--;
+		udelay(10);
+		if (!retry) {
+			printf("%s: timedout waiting for ics!\n", __func__);
+			return;
+		}
+	}
 	writel(readl(&mmc_base->sysctl) | CEN_ENABLE, &mmc_base->sysctl);
 }
 
-- 
1.6.3.3

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Reinhard Meyer]

Dear Nishanth Menon,
> Having a loop with a counter is no timing guarentee for timing
> accuracy or compiler optimizations. For e.g. the same loop counter
> which runs when the MPU is running at 600MHz will timeout in around
> half the time when running at 1GHz. or the example where GCC 4.5
> compiles with different optimization compared to GCC 4.4.
> use a udelay(10) to ensure we have a predictable delay.
> use an emperical number - 100000 uSec ~= 1sec for a worst case timeout.
> This should never happen, and is adequate imaginary condition for us
> to fail with timeout.

In such cases I prefer to use:

	uint64_t etime;
...
	etime = get_ticks() + get_tbclk(); /* 1 second */
	do {
		whatever;
		udelay (xx);
	} while (condition && get_ticks() <= etime);

That is far more accurate than calling udelay() 100000 times.
(depending on implementation and granularity udelay of a few usecs
might be rounded significantly)
You can still call udelay inside the loop if you don't want
to poll the condition too tightly...

Best Regards,
Reinhard

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Nishanth Menon]

Reinhard Meyer had written, on 10/25/2010 08:14 PM, the following:
> Dear Nishanth Menon,
>> Having a loop with a counter is no timing guarentee for timing
>> accuracy or compiler optimizations. For e.g. the same loop counter
>> which runs when the MPU is running at 600MHz will timeout in around
>> half the time when running at 1GHz. or the example where GCC 4.5
>> compiles with different optimization compared to GCC 4.4.
>> use a udelay(10) to ensure we have a predictable delay.
>> use an emperical number - 100000 uSec ~= 1sec for a worst case timeout.
>> This should never happen, and is adequate imaginary condition for us
>> to fail with timeout.
> 
> In such cases I prefer to use:
> 
> 	uint64_t etime;
> ...
> 	etime = get_ticks() + get_tbclk(); /* 1 second */
> 	do {
> 		whatever;
> 		udelay (xx);
> 	} while (condition && get_ticks() <= etime);
> 
> That is far more accurate than calling udelay() 100000 times.
> (depending on implementation and granularity udelay of a few usecs
> might be rounded significantly)
> You can still call udelay inside the loop if you don't want
> to poll the condition too tightly...
hmmm.. almost like the jiffies in kernel ;).. timing wise, I see that 
the only benefit is that the approach gives a little more accuracy to 
the timeout delay - the other benefit is option to loop continuously 
instead of delaying with udelays - overall, at least the segments I saw 
had no reason to hit the registers so hard (alright we dont have much 
else to do.. but still).. I am very open to options from 
Sukumar(original author) as well..

-- 
Regards,
Nishanth Menon

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Wolfgang Denk]

Dear Nishanth Menon,

In message <1288054924-24989-1-git-send-email-nm@ti.com> you wrote:
> Having a loop with a counter is no timing guarentee for timing
> accuracy or compiler optimizations. For e.g. the same loop counter
> which runs when the MPU is running at 600MHz will timeout in around
> half the time when running at 1GHz. or the example where GCC 4.5
> compiles with different optimization compared to GCC 4.4.
> use a udelay(10) to ensure we have a predictable delay.
> use an emperical number - 100000 uSec ~= 1sec for a worst case timeout.

Hm... 100000 usec = 0.1 sec only... Guess you mean

100,000 x 10 usec = 1 sec ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anyone can count the seeds in an apple.
No one can count the apples in a seed.

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Ghorai, Sukumar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> On Behalf Of Wolfgang Denk
> Sent: Tuesday, October 26, 2010 10:06 AM
> To: Menon, Nishanth
> Cc: u-boot
> Subject: Re: [U-Boot] [PATCH v2] mmc: omap: timeout counter fix
> 
> Dear Nishanth Menon,
> 
> In message <1288054924-24989-1-git-send-email-nm@ti.com> you wrote:
> > Having a loop with a counter is no timing guarentee for timing
> > accuracy or compiler optimizations. For e.g. the same loop counter
> > which runs when the MPU is running at 600MHz will timeout in around
> > half the time when running at 1GHz. or the example where GCC 4.5
> > compiles with different optimization compared to GCC 4.4.
> > use a udelay(10) to ensure we have a predictable delay.
> > use an emperical number - 100000 uSec ~= 1sec for a worst case timeout.
> 
> Hm... 100000 usec = 0.1 sec only... Guess you mean
> 
> 100,000 x 10 usec = 1 sec ?
> 
> 
> Best regards,
> 
> Wolfgang Denk
>

[..snip..]
[Menon, Nishanth] - overall, at least the segments I saw had no reason to hit the registers so hard (alright we dont have much else to do.. but still).. I am very open to options from Sukumar(original author) as well..

[Ghorai] I am agreeing with the part where it's looks while(1) loop. And can add retry counter too. otherwise I feel I will increase the boot time. 

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC62B6C.30601@emk-elektronik.de> you wrote:
>
> In such cases I prefer to use:
> 
> 	uint64_t etime;
> ...
> 	etime = get_ticks() + get_tbclk(); /* 1 second */
> 	do {
> 		whatever;
> 		udelay (xx);
> 	} while (condition && get_ticks() <= etime);
> 
> That is far more accurate than calling udelay() 100000 times.

It may be more accuratre, but it may also be HORRIBLY WRONG!!

Do NOT do that!! NEVER implement such a delay loop as

	end = time() + delay;
	while (time() < end)
		...

It fails in case the timer wraps around.

Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
will compute end = 0x10, the while codition is immediately false, and
you don't have any delay at all, which most probably generates a
false error condition.


Correct implementation of a timeout like that should always look like
that:

	start = time();
	while ((time() - start) < delay)
		...

This works much better (assuming unsigned arithmetics).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Ein weiser Herrscher kann in einem gro?en Land  mehr  Gutes  bewirken
als  in  einem kleinen - ein dummer Herrscher aber auch viel mehr Un-
fug. Da weise Herrscher seltener sind als dumme, war ich schon  immer
gegen gro?e Reiche skeptisch.                   - Herbert Rosendorfer

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Wolfgang Denk]

Dear Nishanth Menon,

In message <4CC62C81.8000202@ti.com> you wrote:
>
> > You can still call udelay inside the loop if you don't want
> > to poll the condition too tightly...
> hmmm.. almost like the jiffies in kernel ;).. timing wise, I see that 

Yes, except for the bugs... ;-)


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
und das nennen sie ihren Standpunkt.

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Ghorai, Sukumar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de]
> On Behalf Of Wolfgang Denk
> Sent: Tuesday, October 26, 2010 10:59 AM
> To: Reinhard Meyer
> Cc: Menon, Nishanth; u-boot
> Subject: Re: [U-Boot] [PATCH v2] mmc: omap: timeout counter fix
> 
> Dear Reinhard Meyer,
> 
> In message <4CC62B6C.30601@emk-elektronik.de> you wrote:
> >
> > In such cases I prefer to use:
> >
> > 	uint64_t etime;
> > ...
> > 	etime = get_ticks() + get_tbclk(); /* 1 second */
> > 	do {
> > 		whatever;
> > 		udelay (xx);
> > 	} while (condition && get_ticks() <= etime);
> >
> > That is far more accurate than calling udelay() 100000 times.
> 
> It may be more accuratre, but it may also be HORRIBLY WRONG!!
> 
> Do NOT do that!! NEVER implement such a delay loop as
> 
> 	end = time() + delay;
> 	while (time() < end)
> 		...
> 
> It fails in case the timer wraps around.
> 
> Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
> will compute end = 0x10, the while codition is immediately false, and
> you don't have any delay at all, which most probably generates a
> false error condition.
> 
> 
> Correct implementation of a timeout like that should always look like
> that:
> 
> 	start = time();
> 	while ((time() - start) < delay)
> 		...
> 
> This works much better (assuming unsigned arithmetics).
[Ghorai] Thanks.. This is the best approach. 
Otherwise udelay() will increase the boot time. 

> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Ein weiser Herrscher kann in einem gro?en Land  mehr  Gutes  bewirken
> als  in  einem kleinen - ein dummer Herrscher aber auch viel mehr Un-
> fug. Da weise Herrscher seltener sind als dumme, war ich schon  immer
> gegen gro?e Reiche skeptisch.                   - Herbert Rosendorfer
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Reinhard Meyer]

Dear Wolfgang Denk,
> In message<4CC62B6C.30601@emk-elektronik.de>  you wrote:
>>
>> In such cases I prefer to use:
>>
>> 	uint64_t etime;
>> ...
>> 	etime = get_ticks() + get_tbclk(); /* 1 second */
>> 	do {
>> 		whatever;
>> 		udelay (xx);
>> 	} while (condition&&  get_ticks()<= etime);
>>
>> That is far more accurate than calling udelay() 100000 times.
>
> It may be more accuratre, but it may also be HORRIBLY WRONG!!
>
> Do NOT do that!! NEVER implement such a delay loop as
>
> 	end = time() + delay;
> 	while (time()<  end)
> 		...
>
> It fails in case the timer wraps around.
>
> Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
> will compute end = 0x10, the while codition is immediately false, and
> you don't have any delay at all, which most probably generates a
> false error condition.

I used and assumed a 64 bit counter, that will not wrap around while
our civilization still exists...

If get_ticks() is only 32 bits worth, both methods will misbehave
at a 32 bit wrap over.

>
>
> Correct implementation of a timeout like that should always look like
> that:
>
> 	start = time();
> 	while ((time() - start)<  delay)
> 		...
>
> This works much better (assuming unsigned arithmetics).

True, provided the underlying timer is really 64 bits, otherwise
this fails, too...

Best would be to assign get_ticks() to a 32 bit unsigned and use
32 bit vars for start and delay as well.

The original udelay() implementation in AT91 would have failed
at a 32 bit wrap over, too (fixed in mainline). I hope other
implementations are done right, too...

Best regards,

Reinhard

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC66A67.4000608@emk-elektronik.de> you wrote:
>
> > It fails in case the timer wraps around.
> >
> > Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
> > will compute end = 0x10, the while codition is immediately false, and
> > you don't have any delay at all, which most probably generates a
> > false error condition.
> 
> I used and assumed a 64 bit counter, that will not wrap around while
> our civilization still exists...


The code is still wrong, and as a simple correct implementation exists
there is no excuse for using such incorrect code.

Please fix that!

> If get_ticks() is only 32 bits worth, both methods will misbehave
> at a 32 bit wrap over.

No.

> > 	start = time();
> > 	while ((time() - start)<  delay)
> > 		...
> >
> > This works much better (assuming unsigned arithmetics).
> 
> True, provided the underlying timer is really 64 bits, otherwise
> this fails, too...

You are wrong. Try for example this:

--------------------- snip -------------------
#include <stdio.h>

int main(void)
{
	unsigned int time = 0xFFFFFFF0;
	unsigned int delay = 0x20;
	unsigned int start;

	start = time;

	printf("start=0x%X\n", start);

	while ((time - start) < delay) {
		printf("time=0x%X...\n", time);
		++time;
	}

	return 0;
}
--------------------- snip -------------------

> Best would be to assign get_ticks() to a 32 bit unsigned and use
> 32 bit vars for start and delay as well.

? But that's what I wrote?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Speculation is always more interesting than facts.
                                    - Terry Pratchett, _Making_Money_

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Reinhard Meyer]

Dear Wolfgang Denk,
> Dear Reinhard Meyer,
>
> In message<4CC66A67.4000608@emk-elektronik.de>  you wrote:
>>
>>> It fails in case the timer wraps around.
>>>
>>> Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
>>> will compute end = 0x10, the while codition is immediately false, and
>>> you don't have any delay at all, which most probably generates a
>>> false error condition.
>>
>> I used and assumed a 64 bit counter, that will not wrap around while
>> our civilization still exists...
>
>
> The code is still wrong, and as a simple correct implementation exists
> there is no excuse for using such incorrect code.
>
> Please fix that!

Agreed here. People are invited to dig through u-boot and find all
those places.

>
>> If get_ticks() is only 32 bits worth, both methods will misbehave
>> at a 32 bit wrap over.
>
> No.
>
>>> 	start = time();
>>> 	while ((time() - start)<   delay)
>>> 		...
>>>
>>> This works much better (assuming unsigned arithmetics).
>>
>> True, provided the underlying timer is really 64 bits, otherwise
>> this fails, too...
 > You are wrong. Try for example this:
 >
 > --------------------- snip -------------------
 > #include <stdio.h>
 >
 > int main(void)
 > {
 > 	unsigned int time = 0xFFFFFFF0;
 > 	unsigned int delay = 0x20;
 > 	unsigned int start;

You are wrong here, because you take it out of context.
My "demo" is using the (declared as) 64 bit function get_ticks().
I mentioned above that this function MUST be truly returning 64
bits worth of (incrementing) value to make any version work.
If get_ticks() just returns a 32 bit counter value neither method will work
reliably. Just check all implementations that this function is implemented
correctly.

[U-Boot] Timer implementations (was: Re: [PATCH v2] mmc: omap: timeout counter fix)

[Reinhard Meyer]

I just had a look at other ARM implementations of timer.c.

Some have a colourful mix of 32 and 64 bits values, resulting
in some 64 bit timer functions returning the upper 32 bits always
cleared.

Some implement udelay() in the "while (xxxtime() < endtime);" variant.

I will fix this for at91 and submit a patch.

I also see that:

void reset_timer(void)
{
	gd->timer_reset_value = get_ticks();
}

ulong get_timer(ulong base)
{
	return tick_to_time(get_ticks() - gd->timer_reset_value) - base;
}

If implemented with true 64 bits for get_ticks() that function is useable
for timeout programming:

	ulong timeval = get_timer (0);

	do {
		...
	} while (get_timer (timeval) < TIMEOUT);

It appears that the "base" parameter and return value is in CONFIG_SYS_HZ
units, and not in native ticks. That causes 64 bit mul/div every
time get_timer() is called. Won't hurt in a timeout loop, though.

I guess the theoretically unnecessary function reset_timer() might have been
invented to mask the issue of 32 bit wraparounds when get_timer() is not truly
64 bits???

Reinhard

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[J. William Campbell]

On 10/25/2010 11:01 PM, Reinhard Meyer wrote:
> Dear Wolfgang Denk,
>> Dear Reinhard Meyer,
>>
>> In message<4CC66A67.4000608@emk-elektronik.de>   you wrote:
>>>> It fails in case the timer wraps around.
>>>>
>>>> Assume 32 bit counters, start time = 0xFFFFFFF0, delay = 0x20. It
>>>> will compute end = 0x10, the while codition is immediately false, and
>>>> you don't have any delay at all, which most probably generates a
>>>> false error condition.
>>> I used and assumed a 64 bit counter, that will not wrap around while
>>> our civilization still exists...
>>
>> The code is still wrong, and as a simple correct implementation exists
>> there is no excuse for using such incorrect code.
>>
>> Please fix that!
> Agreed here. People are invited to dig through u-boot and find all
> those places.
>
>>> If get_ticks() is only 32 bits worth, both methods will misbehave
>>> at a 32 bit wrap over.
>> No.
>>
>>>> 	start = time();
>>>> 	while ((time() - start)<    delay)
>>>> 		...
>>>>
>>>> This works much better (assuming unsigned arithmetics).
>>> True, provided the underlying timer is really 64 bits, otherwise
>>> this fails, too...
>   >  You are wrong. Try for example this:
>   >
>   >  --------------------- snip -------------------
>   >  #include<stdio.h>
>   >
>   >  int main(void)
>   >  {
>   >  	unsigned int time = 0xFFFFFFF0;
>   >  	unsigned int delay = 0x20;
>   >  	unsigned int start;
>
> You are wrong here, because you take it out of context.
> My "demo" is using the (declared as) 64 bit function get_ticks().
> I mentioned above that this function MUST be truly returning 64
> bits worth of (incrementing) value to make any version work.
> If get_ticks() just returns a 32 bit counter value neither method will work
> reliably. Just check all implementations that this function is implemented
> correctly.
Hi All,
       I have wondered for quite some time about the rush to make 
get_ticks() return a 64 bit value. For any "reasonable" purpose, like 
waiting a few seconds for something to complete, a 32 bit timebase is 
plenty adequate. If the number of ticks per second is 1000000000, i.e. a 
1 GHz clock rate, the clock wraps in a 32 bit word about every five 
seconds.
The trick is that time always moves forward, so a current get_ticks() - 
a previous get_ticks() is ALWAYS a positive number. It is necessary to 
check the clock more often than (0X100000000 - your_timeout) times per 
second, but unless your timeout is very near the maximum time that fits 
into 32 bits, this won't be a problem. Most CPUs have a counter that 
count at a "reasonable" rate. Some CPUs also have a "cycle counter" that 
runs at the CPU clock rate. These counters are useful to determine 
exactly how many machine cycles a certain process took, and therefore 
they have high resolution. Timers for simple delays neither need nor 
want such resolution. If the only counter available on you CPU runs at 
several GHz, and is 64 bits long,  just shift it right a few bits to 
reduce the resolution to a "reasonable"  resolution and return a 32 bit 
value. There is no need for a bunch of long long variables and extra 
code running around to process simple timeouts. It may be that we need a 
different routine for precision timing measurements with high 
resolution, but it needn't, and probably shouldn't IMHO be get_ticks().

Best Regards,
Bill Campbell
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC66ECA.9000106@emk-elektronik.de> you wrote:
>
> Agreed here. People are invited to dig through u-boot and find all
> those places.

You know the ones you added best :-)

>  > int main(void)
>  > {
>  > 	unsigned int time = 0xFFFFFFF0;
>  > 	unsigned int delay = 0x20;
>  > 	unsigned int start;
> 
> You are wrong here, because you take it out of context.

I am not wrong. Feel free to replace "unsigned int" by "unsigned long
long", and 0xFFFFFFF0 by 0xFFFFFFFFFFFFFFF0ULL, and the "%X" by "%llX".

> My "demo" is using the (declared as) 64 bit function get_ticks().
> I mentioned above that this function MUST be truly returning 64
> bits worth of (incrementing) value to make any version work.

What's the difference?  It is inherently wrong to believe an overflow
would never occur just because the precision of a number is "long
enough". Remeber y2k issues. Remember the time overflog for UNIX
systems in 2038, etc. etc.

> If get_ticks() just returns a 32 bit counter value neither method will work
> reliably. Just check all implementations that this function is implemented
> correctly.

No, the code is still WRONG. It'ss just that the problem is less
likely to hit.

BTW - who says the timer starts counting at 0 ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Lack of skill dictates economy of style.                - Joey Ramone

[U-Boot] Timer implementations (was: Re: [PATCH v2] mmc: omap: timeout counter fix)

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC67CA1.9090302@emk-elektronik.de> you wrote:
> 
> If implemented with true 64 bits for get_ticks() that function is useable
> for timeout programming:
> 
> 	ulong timeval = get_timer (0);
> 
> 	do {
> 		...
> 	} while (get_timer (timeval) < TIMEOUT);
> 
> It appears that the "base" parameter and return value is in CONFIG_SYS_HZ
> units, and not in native ticks. That causes 64 bit mul/div every
> time get_timer() is called. Won't hurt in a timeout loop, though.

But it will hurt in othe rplaces.

Also, this code _is_ a bit different, as "get_timer(0)" makes sure
the counter starts ticking again at 0, and get_timer() is defined to
have millisecond resolution. So you have a guaranteed 2^32
milliseconds or 4294967 seconds or about 3.3 years available which
indeed should be sufficient to implement standard timeouts.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
(null cookie; hope that's ok)

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Reinhard Meyer]

Dear Wolfgang Denk,
> Dear Reinhard Meyer,
> 
> In message <4CC66ECA.9000106@emk-elektronik.de> you wrote:
>> Agreed here. People are invited to dig through u-boot and find all
>> those places.
> 
> You know the ones you added best :-)
> 
>>  > int main(void)
>>  > {
>>  > 	unsigned int time = 0xFFFFFFF0;
>>  > 	unsigned int delay = 0x20;
>>  > 	unsigned int start;
>>
>> You are wrong here, because you take it out of context.
> 
> I am not wrong. Feel free to replace "unsigned int" by "unsigned long
> long", and 0xFFFFFFF0 by 0xFFFFFFFFFFFFFFF0ULL, and the "%X" by "%llX".
> 
>> My "demo" is using the (declared as) 64 bit function get_ticks().
>> I mentioned above that this function MUST be truly returning 64
>> bits worth of (incrementing) value to make any version work.
> 
> What's the difference?  It is inherently wrong to believe an overflow
> would never occur just because the precision of a number is "long
> enough". Remeber y2k issues. Remember the time overflog for UNIX
> systems in 2038, etc. etc.
> 
>> If get_ticks() just returns a 32 bit counter value neither method will work
>> reliably. Just check all implementations that this function is implemented
>> correctly.
You should really try to understand what I am saying here:

IF get_timer() returns 0x0000000000000000 to 0x00000000ffffffff and wraps back
to 0x0000000000000000 (thats how it is or was implemented in SOME architectures)
neither mine (agreed to be wrong) nor your code would work if it uses 64 bits vars.

Reinhard

[U-Boot] Timer implementations

[Reinhard Meyer]

Wolfgang Denk schrieb:
> Dear Reinhard Meyer,
> 
> In message <4CC67CA1.9090302@emk-elektronik.de> you wrote:
>> If implemented with true 64 bits for get_ticks() that function is useable
>> for timeout programming:
>>
>> 	ulong timeval = get_timer (0);
>>
>> 	do {
>> 		...
>> 	} while (get_timer (timeval) < TIMEOUT);
>>
>> It appears that the "base" parameter and return value is in CONFIG_SYS_HZ
>> units, and not in native ticks. That causes 64 bit mul/div every
>> time get_timer() is called. Won't hurt in a timeout loop, though.
> 
> But it will hurt in othe rplaces.
> 
> Also, this code _is_ a bit different, as "get_timer(0)" makes sure
> the counter starts ticking again at 0

Nope, it does not reset the counter itself. It returns the current
counter value (recalculated into CONFIG_SYS_HZ units).
Maybe you mean reset_timer() instead?

In arm9226ejs/omap/timer.c udelay() is implemented with reset_timer()
and get_timer(). Since those functions are inherently not nestable,
beware of base=get_timer(0); do { ... udelay(xx); ... } while (get_timer(base) < TIMEOUT);
constructs!

> , and get_timer() is defined to
> have millisecond resolution.

Actually CONFIG_SYS_HZ (whatever that is).

> So you have a guaranteed 2^32
> milliseconds or 4294967 seconds or about 3.3 years available which
> indeed should be sufficient to implement standard timeouts.

I think it is necessary to summarize all implicit or explicit documented
"defined to have's" regarding the timer and then to verify that all
implementations adhere to them.

Reinhard

[U-Boot] Timer implementations

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC68A07.6050109@emk-elektronik.de> you wrote:
>
> > Also, this code _is_ a bit different, as "get_timer(0)" makes sure
> > the counter starts ticking again at 0
> 
> Nope, it does not reset the counter itself. It returns the current
> counter value (recalculated into CONFIG_SYS_HZ units).
> Maybe you mean reset_timer() instead?

Yes, you are right, I was wrong.

> > have millisecond resolution.
> 
> Actually CONFIG_SYS_HZ (whatever that is).

It is defined to be 1000.

> I think it is necessary to summarize all implicit or explicit documented
> "defined to have's" regarding the timer and then to verify that all
> implementations adhere to them.

Indeed - documentation is an are where U-Boot has a serious lack.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's certainly  convenient  the  way  the  crime  (or  condition)  of
stupidity   carries   with   it  its  own  punishment,  automatically
admisistered without remorse, pity, or prejudice. :-)
         -- Tom Christiansen in <559seq$ag1$1@csnews.cs.colorado.edu>

[U-Boot] Timer implementations

[Reinhard Meyer]

Dear Wolfgang Denk,

>> Actually CONFIG_SYS_HZ (whatever that is).
> 
> It is defined to be 1000.

Ok, game with that.

Then the define CONFIG_SYS_HZ should not be in every <board>.h since that
suggests that a board developer has some freedom there...

>> I think it is necessary to summarize all implicit or explicit documented
>> "defined to have's" regarding the timer and then to verify that all
>> implementations adhere to them.
> 
> Indeed - documentation is an are where U-Boot has a serious lack.

OK, to summarize:

1. reset_timer(), get_timer(base) operate with 1ms granularity.
An implementation MUST make that close to 1ms.
On second thought, it might be valueable to be able to set
CONFIG_SYS_HZ to the exact value of the actual granularity.
(for example 1024, if a 32kHz based timer is used).

It should be pointed out, that the pair reset_timer()/get_timer()
cannot be used nested,

and MOST IMPORTANT that some implementations of udelay() might
call reset_timer() and therefore break an outer timeout loop !!!

It is also open if reset_timer() does actually reset the hardware timer
(e.g. tbu/tbl at PPC) - which would be messing up any time difference
calculation using get_ticks() - or does emulate that by remembering
the hardware value and subtracting it later in every subsequent
get_timer() call?

2. get_ticks() and friends operate at a higher rate (tbu/tbl for PPC).
Since they are defined as having 64 bits they MUST not wrap at 32 bits,
i.e. if the hardware provides only 32 bits, the upper 32 bits must be
emulated by software.
Otherwise we have to document that get_ticks() cannot be used to get
64 bit time differences.

OR fall back to a 32 bit get_ticks() that should perhaps increment at
a microsecond rate (see Bill Campbell's post).

If you really closely look at the various implementations of those
timers, you will easyly see the wide variations implemented there.

Best Regards,
Reinhard

[U-Boot] Timer implementations

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CC6AADC.8050608@emk-elektronik.de> you wrote:
> 
> Then the define CONFIG_SYS_HZ should not be in every <board>.h since that
> suggests that a board developer has some freedom there...

Agreed - there are historical reasons this has ever been changable at
all.

> and MOST IMPORTANT that some implementations of udelay() might
> call reset_timer() and therefore break an outer timeout loop !!!

Such implementations are inherently broken and need to be fixed.

> It is also open if reset_timer() does actually reset the hardware timer
> (e.g. tbu/tbl at PPC) - which would be messing up any time difference
> calculation using get_ticks() - or does emulate that by remembering
> the hardware value and subtracting it later in every subsequent
> get_timer() call?

This is an implementation detail.

> 2. get_ticks() and friends operate at a higher rate (tbu/tbl for PPC).
> Since they are defined as having 64 bits they MUST not wrap at 32 bits,
> i.e. if the hardware provides only 32 bits, the upper 32 bits must be
> emulated by software.

Right.

> Otherwise we have to document that get_ticks() cannot be used to get
> 64 bit time differences.

No. Such an implementation is broken and needs fixing.

> If you really closely look at the various implementations of those
> timers, you will easyly see the wide variations implemented there.

Yes, I am aware of this :-(

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
...though his invention worked superbly -- his theory was a crock  of
sewage from beginning to end.         - Vernor Vinge, "The Peace War"

[U-Boot] Timer implementations

[Reinhard Meyer]

Dear Wolfgang Denk,
>> Then the define CONFIG_SYS_HZ should not be in every <board>.h since that
>> suggests that a board developer has some freedom there...
> 
> Agreed - there are historical reasons this has ever been changable at
> all.
> 
>> and MOST IMPORTANT that some implementations of udelay() might
>> call reset_timer() and therefore break an outer timeout loop !!!
> 
> Such implementations are inherently broken and need to be fixed.

Found such in arm926ejs/omap... But then, that timer is multiple-broken:
relocation broken (uses static data), returns 32 a bit value in get_ticks(),
returns CONFIG_SYS_HZ in get_tbclk() instead of the rate get_ticks()
increments...

PXA:
void udelay_masked (unsigned long usec)
{
	unsigned long long tmp;
	ulong tmo;

	tmo = us_to_tick(usec);
	tmp = get_ticks() + tmo;	/* get current timestamp */

	while (get_ticks() < tmp)
		/* loop till event _OR FOREVER is tmp happens to be > 32 bit_ */
		 /*NOP*/;

}

unsigned long long get_ticks(void)
{
	return readl(OSCR);
}
- not any better :( -- its the same code that AT91 had before I fixed it.

> 
>> It is also open if reset_timer() does actually reset the hardware timer
>> (e.g. tbu/tbl at PPC) - which would be messing up any time difference
>> calculation using get_ticks() - or does emulate that by remembering
>> the hardware value and subtracting it later in every subsequent
>> get_timer() call?
> 
> This is an implementation detail.

IF we require that get_ticks() and get_timer() shall not interfere with
each other and IF both are based on the same hardware timer only the
second method is available (same if the hardware timer is not easyly
resettable).

>> 2. get_ticks() and friends operate at a higher rate (tbu/tbl for PPC).
>> Since they are defined as having 64 bits they MUST not wrap at 32 bits,
>> i.e. if the hardware provides only 32 bits, the upper 32 bits must be
>> emulated by software.
> 
> Right.
> 
>> Otherwise we have to document that get_ticks() cannot be used to get
>> 64 bit time differences.
> 
> No. Such an implementation is broken and needs fixing.

Original AT91 timer.c was like that, and I think other ARMs where this was
copied around should be looked at... I don't know when get_timer() became
64 bits, but it seems that some implementations just did change the return
type: uint64 get_timer(void) {return (uint64)timer_val_32;}

> 
>> If you really closely look at the various implementations of those
>> timers, you will easyly see the wide variations implemented there.
> 
> Yes, I am aware of this :-(

I will start beautifying the AT91 timer - its already quite there,
except for a possible timer wrap problem in udelay() after a few billion
years :)

Best Regards,
Reinhard

[U-Boot] [PATCH v2] mmc: omap: timeout counter fix

[Nishanth Menon]

Ghorai, Sukumar had written, on 10/26/2010 12:34 AM, the following:
[...]
> [Ghorai] Thanks.. This is the best approach. 
> Otherwise udelay() will increase the boot time. 
Please define "increase the boot time" with the context to the patch 
where you think the increase of boot time will be? In my experience, it 
has not. the iterations are such that:
while (condition_not_met)
	udelay(10);
This is reasonable enough to wait till the condition is met.
a) u-boot boot logic is not invoked until "mmc part 0" is invoked -> 
u-boot boot time does not change
b) the actual fatload by itself will only incur minor latency addition - 
IMHO - necessary addition - in cases where the check conditions are not met.

would be great if you can illustrate within the patch areas which need 
continuous checks Vs the ones that do not need continuous checks.

-- 
Regards,
Nishanth Menon

[U-Boot] Timer implementations

[Nishanth Menon]

Reinhard Meyer had written, on 10/26/2010 02:57 AM, the following:
> Wolfgang Denk schrieb:
>> Dear Reinhard Meyer,
>>
>> In message <4CC67CA1.9090302@emk-elektronik.de> you wrote:
>>> If implemented with true 64 bits for get_ticks() that function is useable
>>> for timeout programming:
>>>
>>> 	ulong timeval = get_timer (0);
>>>
>>> 	do {
>>> 		...
>>> 	} while (get_timer (timeval) < TIMEOUT);
>>>
>>> It appears that the "base" parameter and return value is in CONFIG_SYS_HZ
>>> units, and not in native ticks. That causes 64 bit mul/div every
>>> time get_timer() is called. Won't hurt in a timeout loop, though.
>> But it will hurt in othe rplaces.
>>
>> Also, this code _is_ a bit different, as "get_timer(0)" makes sure
>> the counter starts ticking again at 0
> 
> Nope, it does not reset the counter itself. It returns the current
> counter value (recalculated into CONFIG_SYS_HZ units).
> Maybe you mean reset_timer() instead?
> 
> In arm9226ejs/omap/timer.c udelay() is implemented with reset_timer()
> and get_timer(). Since those functions are inherently not nestable,
> beware of base=get_timer(0); do { ... udelay(xx); ... } while (get_timer(base) < TIMEOUT);
> constructs!
At least the omap3+ code arch/arm/cpu/armv7/omap-common/timer.c
__udelay() does'nt seem to reset the timer
BUT,
unsigned long long get_ticks(void)
{
         return get_timer(0);
}
ulong get_timer(ulong base)
{        return get_timer_masked() - base;
}
ulong get_timer_masked(void)
{
         /* current tick value */
         ulong now = readl(&timer_base->tcrr) / (TIMER_CLOCK / 
CONFIG_SYS_HZ);

         if (now >= lastinc)     /* normal mode (non roll) */
                 /* move stamp fordward with absoulte diff ticks */
                 timestamp += (now - lastinc);
         else    /* we have rollover of incrementer */
                 timestamp += ((TIMER_LOAD_VAL / (TIMER_CLOCK / 
CONFIG_SYS_HZ))
                                 - lastinc) + now;
         lastinc = now;
         return timestamp;
}

if I am not mistaken, this is 32bit long.. but since we have as wdenk 
mentions below a 2^32 tick duration, we can safely go with an option 
such as:
/* If we fail after 1 second wait, something is really bad */
#define MAX_RETRY_US    (1 * 1000 * 1000)

uint64_t etime; /* actually this could be u32 */

etime = get_ticks() + usec2ticks(MAX_RETRY_US);
while (!(readl(&mmc_base->stat) & CC_MASK)) {
         if (get_ticks() <= etime) {
                 printf("%s: timedout waiting for cc2!\n", __func__);
                 return;
         }
}

sounds right?

> 
>> , and get_timer() is defined to
>> have millisecond resolution.
> 
> Actually CONFIG_SYS_HZ (whatever that is).
> 
>> So you have a guaranteed 2^32
>> milliseconds or 4294967 seconds or about 3.3 years available which
>> indeed should be sufficient to implement standard timeouts.
> 
> I think it is necessary to summarize all implicit or explicit documented
> "defined to have's" regarding the timer and then to verify that all
> implementations adhere to them.
> 
yes indeed.. :(

-- 
Regards,
Nishanth Menon

[U-Boot] Timer implementations

[Wolfgang Denk]

Dear Nishanth Menon,

In message <4CC6EFB1.9000701@ti.com> you wrote:
>
> uint64_t etime; /* actually this could be u32 */
> 
> etime = get_ticks() + usec2ticks(MAX_RETRY_US);
> while (!(readl(&mmc_base->stat) & CC_MASK)) {
>          if (get_ticks() <= etime) {
>                  printf("%s: timedout waiting for cc2!\n", __func__);
>                  return;
>          }
> }
> 
> sounds right?

No. This code is always wrong. Please fix it as described.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The first thing we do is kill all the lawyers.
(Shakespeare. II Henry VI, Act IV, scene ii)

[U-Boot] Timer implementations

[Nishanth Menon]

Wolfgang Denk had written, on 10/26/2010 10:17 AM, the following:
> Dear Nishanth Menon,
> 
> In message <4CC6EFB1.9000701@ti.com> you wrote:
>> uint64_t etime; /* actually this could be u32 */
>>
>> etime = get_ticks() + usec2ticks(MAX_RETRY_US);
>> while (!(readl(&mmc_base->stat) & CC_MASK)) {
>>          if (get_ticks() <= etime) {
>>                  printf("%s: timedout waiting for cc2!\n", __func__);
>>                  return;
>>          }
>> }
>>
>> sounds right?
> 
> No. This code is always wrong. Please fix it as described.
Apologies on being a dudhead, I suppose you mean the following:

ulong start;
start = get_timer(0);
while (!(readl(&mmc_base->stat) & CC_MASK)) {
         if (get_timer(start) > usec2ticks(MAX_RETRY_US)) {
                 printf("%s: timedout waiting for cc2!\n", __func__);
                 return;
         }
}

would this be better?

-- 
Regards,
Nishanth Menon

[U-Boot] Timer implementations

[Wolfgang Denk]

Dear Nishanth Menon,

In message <4CC6F23A.2040608@ti.com> you wrote:
>
> > No. This code is always wrong. Please fix it as described.
> Apologies on being a dudhead, I suppose you mean the following:
> 
> ulong start;
> start = get_timer(0);
> while (!(readl(&mmc_base->stat) & CC_MASK)) {
>          if (get_timer(start) > usec2ticks(MAX_RETRY_US)) {
>                  printf("%s: timedout waiting for cc2!\n", __func__);
>                  return;
>          }
> }
> 
> would this be better?

No, not at all, as get_timer() returns milliseconds, not ticks.

You probably want something like:

	ulong start = get_timer(0);

	while (!(readl(&mmc_base->stat) & CC_MASK)) {
		if (get_timer(0) - start >= MAX_RETRY_US/1000) {
			printf(...);
			return;
		}
		udelay(100);
	}

or such.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He had quite a powerful intellect, but it  was  as  powerful  like  a
locomotive,  and  ran on rails and was therefore almost impossible to
steer.                          - Terry Pratchett, _Lords and Ladies_

[U-Boot] Timer implementations

[Reinhard Meyer]

On 26.10.2010 18:26, Wolfgang Denk wrote:
> Dear Nishanth Menon,
>
> In message<4CC6F23A.2040608@ti.com>  you wrote:
>>
>>> No. This code is always wrong. Please fix it as described.
>> Apologies on being a dudhead, I suppose you mean the following:
>>
>> ulong start;
>> start = get_timer(0);
>> while (!(readl(&mmc_base->stat)&  CC_MASK)) {
>>           if (get_timer(start)>  usec2ticks(MAX_RETRY_US)) {
>>                   printf("%s: timedout waiting for cc2!\n", __func__);
>>                   return;
>>           }
>> }
>>
>> would this be better?
>
> No, not at all, as get_timer() returns milliseconds, not ticks.
>
> You probably want something like:
>
> 	ulong start = get_timer(0);
>
> 	while (!(readl(&mmc_base->stat)&  CC_MASK)) {
> 		if (get_timer(0) - start>= MAX_RETRY_US/1000) {
> 			printf(...);
> 			return;
> 		}
> 		udelay(100);
> 	}

This will work, of course, but the original semantics of get_timer()
seems to be:

ulong start = get_timer(0);

loop {
	if (get_timer(start) >= timeout_in_ms)
		we_have_timeout();
}

--> get_timer(x) returns (time - x)

the subtraction is done internally.

Reinhard

[U-Boot] Timer implementations

[J. William Campbell]

On 10/26/2010 6:33 AM, Reinhard Meyer wrote:
> Dear Wolfgang Denk,
>>> Then the define CONFIG_SYS_HZ should not be in every<board>.h since that
>>> suggests that a board developer has some freedom there...
>> Agreed - there are historical reasons this has ever been changable at
>> all.
>>
>>> and MOST IMPORTANT that some implementations of udelay() might
>>> call reset_timer() and therefore break an outer timeout loop !!!
>> Such implementations are inherently broken and need to be fixed.
> Found such in arm926ejs/omap... But then, that timer is multiple-broken:
> relocation broken (uses static data), returns 32 a bit value in get_ticks(),
> returns CONFIG_SYS_HZ in get_tbclk() instead of the rate get_ticks()
> increments...
>
> PXA:
> void udelay_masked (unsigned long usec)
> {
> 	unsigned long long tmp;
> 	ulong tmo;
>
> 	tmo = us_to_tick(usec);
> 	tmp = get_ticks() + tmo;	/* get current timestamp */
>
> 	while (get_ticks()<  tmp)
> 		/* loop till event _OR FOREVER is tmp happens to be>  32 bit_ */
> 		 /*NOP*/;
>
> }
>
> unsigned long long get_ticks(void)
> {
> 	return readl(OSCR);
> }
> - not any better :( -- its the same code that AT91 had before I fixed it.
>
>>> It is also open if reset_timer() does actually reset the hardware timer
>>> (e.g. tbu/tbl at PPC) - which would be messing up any time difference
>>> calculation using get_ticks() - or does emulate that by remembering
>>> the hardware value and subtracting it later in every subsequent
>>> get_timer() call?
>> This is an implementation detail.
> IF we require that get_ticks() and get_timer() shall not interfere with
> each other and IF both are based on the same hardware timer only the
> second method is available (same if the hardware timer is not easyly
> resettable).
>
>>> 2. get_ticks() and friends operate at a higher rate (tbu/tbl for PPC).
>>> Since they are defined as having 64 bits they MUST not wrap at 32 bits,
>>> i.e. if the hardware provides only 32 bits, the upper 32 bits must be
>>> emulated by software.
>> Right.
>>
>>> Otherwise we have to document that get_ticks() cannot be used to get
>>> 64 bit time differences.
>> No. Such an implementation is broken and needs fixing.
> Original AT91 timer.c was like that, and I think other ARMs where this was
> copied around should be looked at... I don't know when get_timer() became
> 64 bits, but it seems that some implementations just did change the return
> type: uint64 get_timer(void) {return (uint64)timer_val_32;}
Hi All,

      I am pretty sure the migration to 64 bits was caused by 1) people 
not understanding that the timer operating on time DIFFERENCES would 
work fine even if the underlying timer wrapped around (most probable 
problem) and possibly 2) broken timer functions causing bogus timeouts, 
improperly "fixed" by switching to 64 bits.

I think u-boot could get along just fine with only 2 time related 
functions,  uint_32 get_timer(uint_32 base) and udelay(uint 32 delay).  
udelay will only work on "small" values of delay, on the order of 
milliseconds. It is to be used when a "short" but "precise" delay in 
microsecond resolution is required.  Users of get_timer must understand 
that it is only valid if it is called "often enough", i.e. at least once 
per period of the underlying timer. This is required because u-boot  
does not want to rely on interrupts as a timer update method. Therefore, 
all uses of get_timer must 1) call it once initially to get a start 
value, and 2) call get_timer at least once per period  of the underlying 
hardware counter. This underlying period is guaranteed to be at least 
4.29 seconds (32 bit counter at 4 GHz). Note that this does NOT mean 
that the total wait must be less than 4.29 seconds, only that the rate 
at which the elapsed time is TESTED must be adequate.

In order to implement this functionality, at least one hardware timer of 
some kind is required. An additional software "timer" in 1 ms resolution 
may be useful in maintaining the software time. If the hardware timer 
update rate is programmable, u-boot MAY set the update rate on 
initialization On initialization, u-boot MAY reset the hardware timer 
and MAY reset any associated software timer. The hardware timer MAY be 
started on initialization. On each call to get_timer(), u-boot MUST 
start the hardware timer if it was not started already. On calls to 
get_timer, u-boot MUST NOT reset the hardware timer if it was already 
started. The software timer MAY be reset if u-boot can unambiguously 
determine that  more than 4.29 seconds has elapsed since the last call 
to get_timer.

The simplest case for implementing this scheme is if two programmable 
timers exist that can be set to 1ms and 1us.  The timers are initialized 
at start-up, get_timer just returns the 32 bit 1 ms timer and udelay 
just waits for the number of ticks required on the second timer to 
elapse. The most common harder case is where there is only one timer 
available, it is running at 1 us per tick or faster, and we cannot 
control the rate. udelay is still easy, because we can convert the 
(small) delay in us to a delay in ticks by a 32 bit multiply that will 
not overflow 32 bits even if we have quite a few fractional bits in the 
tics per microsecond value. The elapsed ticks required is the (delay in 
us * us/per tick) >>  # fractional bits in us/per tick. If that is not 
close enough for you, you can do it as (delay in us * (integer part of 
us/tick)) + ((delay in us * (fractional part)us/tick) >> # fraction 
bits). For "nice" numbers, like any integral number of MHz, there is no 
fractional part. Only numbers like 66 MHz, or 1.666 GHz require messing 
with the fractional part.
For get_timer, it is a bit harder. The program must keep two 32 bit 
global variables, the timer reading "last time" and the software timer 
in 1 ms resolution. Whenever get_timer is called, it must increase the 
software timer by the number of ms that have elapsed since the previous 
update and record the corresponding timer reading as the new "last 
time". Note that if the number of ms elapsed is not an integer (a common 
case), the value recorded as the "last time" must be decreased by the 
number of ticks not included in the 1 ms resolution software timer.  
There are many different ways to accomplish update, depending on what 
hardware math capabilities are available, and whether one thinks 
efficiency is important here. Conceptually, you convert the elapsed time 
in ticks into an equivalent number of ms, add that number to the 
software timer, store the current value of the hardware timer in last 
time, and subtract any "remainder" ticks from that value.  If the 
elapsed time is less that one ms, do no update of "last hardware time" 
and return the current software counter. If the elapsed time is greater 
than 4.29 seconds, reset the software counter to 0, record the current 
hardware counter time and return the current software counter. In 
between, do the math, which will fit into 32 bits.

If this idea seems like a good one, I can provide more detail on the 
conversions for various hardware capabilities is people want. Comments 
welcome.

Best Regards,

Bill Campbell
>>> If you really closely look at the various implementations of those
>>> timers, you will easyly see the wide variations implemented there.
>> Yes, I am aware of this :-(
> I will start beautifying the AT91 timer - its already quite there,
> except for a possible timer wrap problem in udelay() after a few billion
> years :)
>
> Best Regards,
> Reinhard
>
>

[U-Boot] Timer implementations

[Reinhard Meyer]

Dear J. William Campbell,
> Hi All,
>
> I am pretty sure the migration to 64 bits was caused by 1) people not understanding that the timer operating on time DIFFERENCES would work fine even if the underlying timer wrapped around (most probable problem) and possibly 2) broken timer functions causing bogus timeouts, improperly "fixed" by switching to 64 bits.
>
> I think u-boot could get along just fine with only 2 time related functions, uint_32 get_timer(uint_32 base) and udelay(uint 32 delay). udelay will only work on "small" values of delay, on the order of milliseconds. It is to be used when a "short" but "precise" delay in microsecond resolution is required. Users of get_timer must understand that it is only valid if it is called "often enough", i.e. at least once per period of the underlying timer. This is required because u-boot does not want to rely on interrupts as a timer update method. Therefore, all uses of get_timer must 1) call it once initially to get a start value, and 2) call get_timer at least once per period of the underlying hardware counter. This underlying period is guaranteed to be at least 4.29 seconds (32 bit counter at 4 GHz). Note that this does NOT mean that the total wait must be less than 4.29 seconds, only that the rate at which the elapsed time is TESTED must be adequate.
>
> In order to implement this functionality, at least one hardware timer of some kind is required. An additional software "timer" in 1 ms resolution may be useful in maintaining the software time. If the hardware timer update rate is programmable, u-boot MAY set the update rate on initialization On initialization, u-boot MAY reset the hardware timer and MAY reset any associated software timer. The hardware timer MAY be started on initialization. On each call to get_timer(), u-boot MUST start the hardware timer if it was not started already. On calls to get_timer, u-boot MUST NOT reset the hardware timer if it was already started. The software timer MAY be reset if u-boot can unambiguously determine that more than 4.29 seconds has elapsed since the last call to get_timer.
>
> The simplest case for implementing this scheme is if two programmable timers exist that can be set to 1ms and 1us. The timers are initialized at start-up, get_timer just returns the 32 bit 1 ms timer and udelay just waits for the number of ticks required on the second timer to elapse. The most common harder case is where there is only one timer available, it is running at 1 us per tick or faster, and we cannot control the rate. udelay is still easy, because we can convert the (small) delay in us to a delay in ticks by a 32 bit multiply that will not overflow 32 bits even if we have quite a few fractional bits in the tics per microsecond value. The elapsed ticks required is the (delay in us * us/per tick) >> # fractional bits in us/per tick. If that is not close enough for you, you can do it as (delay in us * (integer part of us/tick)) + ((delay in us * (fractional part)us/tick) >> # fraction bits). For "nice" numbers, like any integral number of MHz, there is no fractional

> part. Only numbers like 66 MHz, or 1.666 GHz require messing with the fractional part.
> For get_timer, it is a bit harder. The program must keep two 32 bit global variables, the timer reading "last time" and the software timer in 1 ms resolution. Whenever get_timer is called, it must increase the software timer by the number of ms that have elapsed since the previous update and record the corresponding timer reading as the new "last time". Note that if the number of ms elapsed is not an integer (a common case), the value recorded as the "last time" must be decreased by the number of ticks not included in the 1 ms resolution software timer. There are many different ways to accomplish update, depending on what hardware math capabilities are available, and whether one thinks efficiency is important here. Conceptually, you convert the elapsed time in ticks into an equivalent number of ms, add that number to the software timer, store the current value of the hardware timer in last time, and subtract any "remainder" ticks from that value. If the elapsed time is les
s
> that one ms, do no update of "last hardware time" and return the current software counter. If the elapsed time is greater than 4.29 seconds, reset the software counter to 0, record the current hardware counter time and return the current software counter. In between, do the math, which will fit into 32 bits.
>
> If this idea seems like a good one, I can provide more detail on the conversions for various hardware capabilities is people want. Comments welcome.

To get the timer mess cleaned up three things have to happen:

1. A consensus and documentation how it MUST be handled

2. Fix all timer implementations to adhere to that

3. Fix all timer uses to adhere to that

To start, RFC:

a) udelay(u32) MUST NOT interfere with other timer functions
b) u32 get_timer(u32 base) MUST return (current_time - base) using
millisecond units
c) get_timer() MUST exhaust the full 32 bit range before wrapping
d) to ensure the function of internal high frequency wrapping
processes, get_timer() MUST be called at least once a second while
a timeout loop is run (this will typically be the case anyway)
e) reset_timer() is not needed, potentially harmful if used and
shall be removed. This will also allow for nested timeouts, e.g.
inner timeout for accessing a hardware and outer timeout for having
the hardware perform a function
f) get_ticks() and get_tbclk() SHALL NOT be used to check for timeouts
except for cases where the better than ms resolution is really needed
g) get_ticks() MUST return true 64 bit time
h) all uses of get_ticks() and get_timer() shall be fixed to use
get_timer() as follows:

u32 start_ms;

start_ms = get_timer(0);

any_type_of_loop {
	...
	/* check for timeout */
	if (get_timer(start_ms) >= TIMEOUT_IN_MS)
		/* handle timeout */
	...
}
Alternative, but in contrast to current get_timer() implementation
is to drop the get_timer parameter and do the subtraction in the if.

--> get_timer(base) is a bit of a misnomer, it should be better named
something like get_ms_since(base).

_Observation:_

It seems possible to move udelay() and get_timer() and static helper
functions into a common, ARCH and SoC independent file, provided that
the ARCH/SoC/board dependant implementations of get_ticks() runs
at >= 1 MHz and is true 64 bits. IF above d) is met get_ticks might be
replaced by a u32 function to be used by the common udelay() and
get_timer() implementation. That would allow all timer code to use 32
bit arithmetic only.

Best Regards,
Reinhard

[U-Boot] Timer implementations

[J. William Campbell]

On 10/27/2010 11:02 PM, Reinhard Meyer wrote:
> Dear J. William Campbell,
>> Hi All,
>>
>> I am pretty sure the migration to 64 bits was caused by 1) people not 
>> understanding that the timer operating on time DIFFERENCES would work 
>> fine even if the underlying timer wrapped around (most probable 
>> problem) and possibly 2) broken timer functions causing bogus 
>> timeouts, improperly "fixed" by switching to 64 bits.
>>
>> I think u-boot could get along just fine with only 2 time related 
>> functions, uint_32 get_timer(uint_32 base) and udelay(uint 32 delay). 
>> udelay will only work on "small" values of delay, on the order of 
>> milliseconds. It is to be used when a "short" but "precise" delay in 
>> microsecond resolution is required. Users of get_timer must 
>> understand that it is only valid if it is called "often enough", i.e. 
>> at least once per period of the underlying timer. This is required 
>> because u-boot does not want to rely on interrupts as a timer update 
>> method. Therefore, all uses of get_timer must 1) call it once 
>> initially to get a start value, and 2) call get_timer at least once 
>> per period of the underlying hardware counter. This underlying period 
>> is guaranteed to be at least 4.29 seconds (32 bit counter at 4 GHz). 
>> Note that this does NOT mean that the total wait must be less than 
>> 4.29 seconds, only that the rate at which the elapsed time is TESTED 
>> must be adequate.
>>
>> In order to implement this functionality, at least one hardware timer 
>> of some kind is required. An additional software "timer" in 1 ms 
>> resolution may be useful in maintaining the software time. If the 
>> hardware timer update rate is programmable, u-boot MAY set the update 
>> rate on initialization On initialization, u-boot MAY reset the 
>> hardware timer and MAY reset any associated software timer. The 
>> hardware timer MAY be started on initialization. On each call to 
>> get_timer(), u-boot MUST start the hardware timer if it was not 
>> started already. On calls to get_timer, u-boot MUST NOT reset the 
>> hardware timer if it was already started. The software timer MAY be 
>> reset if u-boot can unambiguously determine that more than 4.29 
>> seconds has elapsed since the last call to get_timer.
>>
>> The simplest case for implementing this scheme is if two programmable 
>> timers exist that can be set to 1ms and 1us. The timers are 
>> initialized at start-up, get_timer just returns the 32 bit 1 ms timer 
>> and udelay just waits for the number of ticks required on the second 
>> timer to elapse. The most common harder case is where there is only 
>> one timer available, it is running at 1 us per tick or faster, and we 
>> cannot control the rate. udelay is still easy, because we can convert 
>> the (small) delay in us to a delay in ticks by a 32 bit multiply that 
>> will not overflow 32 bits even if we have quite a few fractional bits 
>> in the tics per microsecond value. The elapsed ticks required is the 
>> (delay in us * us/per tick) >> # fractional bits in us/per tick. If 
>> that is not close enough for you, you can do it as (delay in us * 
>> (integer part of us/tick)) + ((delay in us * (fractional 
>> part)us/tick) >> # fraction bits). For "nice" numbers, like any 
>> integral number of MHz, there is no fractional
>
>> part. Only numbers like 66 MHz, or 1.666 GHz require messing with the 
>> fractional part.
>> For get_timer, it is a bit harder. The program must keep two 32 bit 
>> global variables, the timer reading "last time" and the software 
>> timer in 1 ms resolution. Whenever get_timer is called, it must 
>> increase the software timer by the number of ms that have elapsed 
>> since the previous update and record the corresponding timer reading 
>> as the new "last time". Note that if the number of ms elapsed is not 
>> an integer (a common case), the value recorded as the "last time" 
>> must be decreased by the number of ticks not included in the 1 ms 
>> resolution software timer. There are many different ways to 
>> accomplish update, depending on what hardware math capabilities are 
>> available, and whether one thinks efficiency is important here. 
>> Conceptually, you convert the elapsed time in ticks into an 
>> equivalent number of ms, add that number to the software timer, store 
>> the current value of the hardware timer in last time, and subtract 
>> any "remainder" ticks from that value. If the elapsed time is les
> s
>> that one ms, do no update of "last hardware time" and return the 
>> current software counter. If the elapsed time is greater than 4.29 
>> seconds, reset the software counter to 0, record the current hardware 
>> counter time and return the current software counter. In between, do 
>> the math, which will fit into 32 bits.
>>
>> If this idea seems like a good one, I can provide more detail on the 
>> conversions for various hardware capabilities is people want. 
>> Comments welcome.
>
> To get the timer mess cleaned up three things have to happen:
>
Hi All,

       I am glad somebody was still interested. I was afraid I had 
scared everyone off.
> 1. A consensus and documentation how it MUST be handled
>
> 2. Fix all timer implementations to adhere to that
>
> 3. Fix all timer uses to adhere to that
I agree this is required.
>
> To start, RFC:
>
> a) udelay(u32) MUST NOT interfere with other timer functions
> b) u32 get_timer(u32 base) MUST return (current_time - base) using
> millisecond units
> c) get_timer() MUST exhaust the full 32 bit range before wrapping
> d) to ensure the function of internal high frequency wrapping
> processes, get_timer() MUST be called at least once a second while
> a timeout loop is run (this will typically be the case anyway)
I have no problem with this, but it might be stricter than needed. I 
cant imagine anybody has a timer running faster than 4 GHz, and that 
allows 4 seconds of interval.
> e) reset_timer() is not needed, potentially harmful if used and
> shall be removed. This will also allow for nested timeouts, e.g.
> inner timeout for accessing a hardware and outer timeout for having
> the hardware perform a function
> f) get_ticks() and get_tbclk() SHALL NOT be used to check for timeouts
> except for cases where the better than ms resolution is really needed
I have an issue here. I think no such use cases really exist. For things 
like bit-banging, udelay is more what one wants. What are the 
counter-examples where sub-millisecond resolution in a loop waiting for 
some expected event is required?
> g) get_ticks() MUST return true 64 bit time
I hope get_ticks must not exist, at least as a "generic" globally 
visible u-boot function. Having it present spawns confusion. People will 
use it instead of the generic get_timer when they don't have to, 
creating the portability problems we are trying to get rid of.
> h) all uses of get_ticks() and get_timer() shall be fixed to use
> get_timer() as follows:
>
> u32 start_ms;
>
> start_ms = get_timer(0);
>
> any_type_of_loop {
>     ...
>     /* check for timeout */
>     if (get_timer(start_ms) >= TIMEOUT_IN_MS)
>         /* handle timeout */
>     ...
> }
> Alternative, but in contrast to current get_timer() implementation
> is to drop the get_timer parameter and do the subtraction in the if.
No problem here.
>
> --> get_timer(base) is a bit of a misnomer, it should be better named
> something like get_ms_since(base).
>
> _Observation:_
>
> It seems possible to move udelay() and get_timer() and static helper
> functions into a common, ARCH and SoC independent file, provided that
> the ARCH/SoC/board dependant implementations of get_ticks() runs
> at >= 1 MHz and is true 64 bits. IF above d) is met get_ticks might be
> replaced by a u32 function to be used by the common udelay() and
> get_timer() implementation. That would allow all timer code to use 32
> bit arithmetic only.
I see no down-side in requiring d) to be true. It is clearly no problem 
for udelay, as the caller gives up control anyway. I can't imagine a 
wait for event loop that takes a second to execute.
It may be that "common" C code is not really a good idea, in that 
different CPUs may want to convert timer ticks to millisec/microsec in 
different ways, based on clock rates and available CPU capabilities 
(like having or not having a 32 bit divide, needing to deal with clock 
rates with repeating fractional parts etc.). If ticks per microsecond is 
an integer and a 32 bit divide is available, the code is trivial. If we 
must avoid the divide and/or the ticks per microsecond is not an 
integer, things get somewhat messier but not impossible. If we don't 
care about the divide taking a bit of time, generic code is pretty easy 
to write even allowing for fractional ticks per nicrosecond. Usually, 
the high-rate timer can be read without requiring a call to another 
routine. This might be a good idea in that people will not then "grow" 
another timer structure on top of the read timer routine!
It is for sure true that if we are worried about efficiency, there are 
at most three or four versions of the ticks to microseconds or 
milliseconds code required for all the different cases, so it is not so 
bad to write them out and let the users decide which one fits their cpu.

Best Regards,
Bill Campbell
>
> Best Regards,
> Reinhard
>
>

[U-Boot] Timer implementations

[Reinhard Meyer]

On 01.11.2010 14:47, J. William Campbell wrote:
> On 10/27/2010 11:02 PM, Reinhard Meyer wrote:
>> Dear J. William Campbell,
>>> Hi All,
>>>
>>> I am pretty sure the migration to 64 bits was caused by 1) people not understanding that the timer operating on time DIFFERENCES would work fine even if the underlying timer wrapped around (most probable problem) and possibly 2) broken timer functions causing bogus timeouts, improperly "fixed" by switching to 64 bits.
>>>
>>> I think u-boot could get along just fine with only 2 time related functions, uint_32 get_timer(uint_32 base) and udelay(uint 32 delay). udelay will only work on "small" values of delay, on the order of milliseconds. It is to be used when a "short" but "precise" delay in microsecond resolution is required. Users of get_timer must understand that it is only valid if it is called "often enough", i.e. at least once per period of the underlying timer. This is required because u-boot does not want to rely on interrupts as a timer update method. Therefore, all uses of get_timer must 1) call it once initially to get a start value, and 2) call get_timer at least once per period of the underlying hardware counter. This underlying period is guaranteed to be at least 4.29 seconds (32 bit counter at 4 GHz). Note that this does NOT mean that the total wait must be less than 4.29 seconds, only that the rate at which the elapsed time is TESTED must be adequate.
>>>
>>> In order to implement this functionality, at least one hardware timer of some kind is required. An additional software "timer" in 1 ms resolution may be useful in maintaining the software time. If the hardware timer update rate is programmable, u-boot MAY set the update rate on initialization On initialization, u-boot MAY reset the hardware timer and MAY reset any associated software timer. The hardware timer MAY be started on initialization. On each call to get_timer(), u-boot MUST start the hardware timer if it was not started already. On calls to get_timer, u-boot MUST NOT reset the hardware timer if it was already started. The software timer MAY be reset if u-boot can unambiguously determine that more than 4.29 seconds has elapsed since the last call to get_timer.
>>>
>>> The simplest case for implementing this scheme is if two programmable timers exist that can be set to 1ms and 1us. The timers are initialized at start-up, get_timer just returns the 32 bit 1 ms timer and udelay just waits for the number of ticks required on the second timer to elapse. The most common harder case is where there is only one timer available, it is running at 1 us per tick or faster, and we cannot control the rate. udelay is still easy, because we can convert the (small) delay in us to a delay in ticks by a 32 bit multiply that will not overflow 32 bits even if we have quite a few fractional bits in the tics per microsecond value. The elapsed ticks required is the (delay in us * us/per tick) >> # fractional bits in us/per tick. If that is not close enough for you, you can do it as (delay in us * (integer part of us/tick)) + ((delay in us * (fractional part)us/tick) >> # fraction bits). For "nice" numbers, like any integral number of MHz, there is no fraction
al
>>
>>> part. Only numbers like 66 MHz, or 1.666 GHz require messing with the fractional part.
>>> For get_timer, it is a bit harder. The program must keep two 32 bit global variables, the timer reading "last time" and the software timer in 1 ms resolution. Whenever get_timer is called, it must increase the software timer by the number of ms that have elapsed since the previous update and record the corresponding timer reading as the new "last time". Note that if the number of ms elapsed is not an integer (a common case), the value recorded as the "last time" must be decreased by the number of ticks not included in the 1 ms resolution software timer. There are many different ways to accomplish update, depending on what hardware math capabilities are available, and whether one thinks efficiency is important here. Conceptually, you convert the elapsed time in ticks into an equivalent number of ms, add that number to the software timer, store the current value of the hardware timer in last time, and subtract any "remainder" ticks from that value. If the elapsed time is l
es
>> s
>>> that one ms, do no update of "last hardware time" and return the current software counter. If the elapsed time is greater than 4.29 seconds, reset the software counter to 0, record the current hardware counter time and return the current software counter. In between, do the math, which will fit into 32 bits.
>>>
>>> If this idea seems like a good one, I can provide more detail on the conversions for various hardware capabilities is people want. Comments welcome.
>>
>> To get the timer mess cleaned up three things have to happen:
>>
> Hi All,
>
> I am glad somebody was still interested. I was afraid I had scared everyone off.
>> 1. A consensus and documentation how it MUST be handled
>>
>> 2. Fix all timer implementations to adhere to that
>>
>> 3. Fix all timer uses to adhere to that
> I agree this is required.
>>
>> To start, RFC:
>>
>> a) udelay(u32) MUST NOT interfere with other timer functions
>> b) u32 get_timer(u32 base) MUST return (current_time - base) using
>> millisecond units
>> c) get_timer() MUST exhaust the full 32 bit range before wrapping
>> d) to ensure the function of internal high frequency wrapping
>> processes, get_timer() MUST be called at least once a second while
>> a timeout loop is run (this will typically be the case anyway)
> I have no problem with this, but it might be stricter than needed. I cant imagine anybody has a timer running faster than 4 GHz, and that allows 4 seconds of interval.
4 GHz / 2**32 yields 1 second. Assuming no prescaler...
just theoretical anyway. A polling loop with timeout that does poll that seldom
is illogical anyway.
>> e) reset_timer() is not needed, potentially harmful if used and
>> shall be removed. This will also allow for nested timeouts, e.g.
>> inner timeout for accessing a hardware and outer timeout for having
>> the hardware perform a function
>> f) get_ticks() and get_tbclk() SHALL NOT be used to check for timeouts
>> except for cases where the better than ms resolution is really needed
> I have an issue here. I think no such use cases really exist. For things like bit-banging, udelay is more what one wants. What are the counter-examples where sub-millisecond resolution in a loop waiting for some expected event is required?
Well, the "really needed" clause says it. I don't see it either.
>> g) get_ticks() MUST return true 64 bit time
> I hope get_ticks must not exist, at least as a "generic" globally visible u-boot function. Having it present spawns confusion. People will use it instead of the generic get_timer when they don't have to, creating the portability problems we are trying to get rid of.
I also agree it would be best to get rid of externally available get_ticks()
and friends.
>> h) all uses of get_ticks() and get_timer() shall be fixed to use
>> get_timer() as follows:
>>
>> u32 start_ms;
>>
>> start_ms = get_timer(0);
>>
>> any_type_of_loop {
>> ...
>> /* check for timeout */
>> if (get_timer(start_ms) >= TIMEOUT_IN_MS)
>> /* handle timeout */
>> ...
>> }
>> Alternative, but in contrast to current get_timer() implementation
>> is to drop the get_timer parameter and do the subtraction in the if.
> No problem here.
How much code uses get_ticks, or get_timer, or just uses a counted
loop of small udelays? I have seen all variants but never made stats
of that.
We probably should leave get_timer() as it is.
>>
>> --> get_timer(base) is a bit of a misnomer, it should be better named
>> something like get_ms_since(base).
>>
>> _Observation:_
>>
>> It seems possible to move udelay() and get_timer() and static helper
>> functions into a common, ARCH and SoC independent file, provided that
>> the ARCH/SoC/board dependant implementations of get_ticks() runs
>> at >= 1 MHz and is true 64 bits. IF above d) is met get_ticks might be
>> replaced by a u32 function to be used by the common udelay() and
>> get_timer() implementation. That would allow all timer code to use 32
>> bit arithmetic only.
> I see no down-side in requiring d) to be true. It is clearly no problem for udelay, as the caller gives up control anyway. I can't imagine a wait for event loop that takes a second to execute.
> It may be that "common" C code is not really a good idea, in that different CPUs may want to convert timer ticks to millisec/microsec in different ways, based on clock rates and available CPU capabilities (like having or not having a 32 bit divide, needing to deal with clock rates with repeating fractional parts etc.). If ticks per microsecond is an integer and a 32 bit divide is available, the code is trivial. If we must avoid the divide and/or the ticks per microsecond is not an integer, things get somewhat messier but not impossible. If we don't care about the divide taking a bit of time, generic code is pretty easy to write even allowing for fractional ticks per nicrosecond. Usually, the high-rate timer can be read without requiring a call to another routine. This might be a good idea in that people will not then "grow" another timer structure on top of the read timer routine!
> It is for sure true that if we are worried about efficiency, there are at most three or four versions of the ticks to microseconds or milliseconds code required for all the different cases, so it is not so bad to write them out and let the users decide which one fits their cpu.
For efficiency it would help if CONFIG_SYS_HZ would be allowed to be in the range
of 1000 - 2000 Hz (or 700 - 1400), so on any hardware a simple shift would do for
get_timer(). Of course the constant CONFIG_SYS_HZ should be replaced by a function
like get_timer_hz() to allow for runtime calculation of the best fit.

Best Regards,
Reinhard

[U-Boot] Timer implementations

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CCF1CAF.1020107@emk-elektronik.de> you wrote:
>
> For efficiency it would help if CONFIG_SYS_HZ would be allowed to be in the range
> of 1000 - 2000 Hz (or 700 - 1400), so on any hardware a simple shift would do for
> get_timer(). Of course the constant CONFIG_SYS_HZ should be replaced by a function
> like get_timer_hz() to allow for runtime calculation of the best fit.

No. If you need such differing settings, please use a different config
variable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When a man sits with a pretty girl for  an  hour,  it  seems  like  a
minute.  But let him sit on a hot stove for a minute -- and it's lon-
ger than any hour. That's relativity.              -- Albert Einstein