[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

 From 3723c8437d8c3d2e04bc3bc1de9c21b33072ab08 Mon Sep 17 00:00:00 2001
From: Renato Andreola <renato.andreola@imagos.it>
Date: Thu, 6 Aug 2009 14:49:59 +0200
Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem 
in tout calculation

With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
solved avoiding the preprocessor conditional and introducing a compile 
time branch between a high freq case and a slow freq case.

Signed-off-by: Renato Andreola renato.andreola@imagos.it
---
  drivers/mtd/cfi_flash.c |    7 ++++---
  1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 81ac5d3..b118f71 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -659,9 +659,10 @@ static int flash_status_check (flash_info_t * info, 
flash_sect_t sector,
  {
  	ulong start;

-#if CONFIG_SYS_HZ != 1000
-	tout *= CONFIG_SYS_HZ/1000;
-#endif
+        if (CONFIG_SYS_HZ > 10000)
+                tout *= CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid 
overflow */
+        else
+                tout = tout * CONFIG_SYS_HZ / 1000 + 1;

  	/* Wait for command completion */
  	start = get_timer (0);
-- 
1.5.5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A7AD624.9030902@imagos.it> you wrote:
>  From 3723c8437d8c3d2e04bc3bc1de9c21b33072ab08 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Thu, 6 Aug 2009 14:49:59 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem 
> in tout calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
> solved avoiding the preprocessor conditional and introducing a compile 
> time branch between a high freq case and a slow freq case.
> 
> Signed-off-by: Renato Andreola renato.andreola at imagos.it
> ---
>   drivers/mtd/cfi_flash.c |    7 ++++---
>   1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 81ac5d3..b118f71 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -659,9 +659,10 @@ static int flash_status_check (flash_info_t * info, 
> flash_sect_t sector,
>   {
>   	ulong start;
> 
> -#if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> -#endif
> +        if (CONFIG_SYS_HZ > 10000)
> +                tout *= CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid 
> overflow */
> +        else
> +                tout = tout * CONFIG_SYS_HZ / 1000 + 1;

NAK, for several reasons:

1) The patch is line wrapped and thus unusable.
2) There should be a Signed-off-by: from Alessandro, too.
3) Please use TAB only for indentation.
4) Please use parens to show what you are relying on, i. e.

	tout = (tout * CONFIG_SYS_HZ) / 1000;

5) Omit the "+1". It seems bogus to me.

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
Digital computers are themselves more complex than most things people
build: They have very large numbers of states. This makes conceiving,
describing, and testing them hard. Software systems  have  orders-of-
magnitude more states than computers do.           - Fred Brooks, Jr.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Alessandro Rubini]

>> +                tout = tout * CONFIG_SYS_HZ / 1000 + 1;

> 2) There should be a Signed-off-by: from Alessandro, too.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>

> 5) Omit the "+1". It seems bogus to me.

Since the timeout is an error condition, It's better to have it longer
than shorter. Since integer division truncates towards 0, I'd better
give my hardware more time before signalling an error (an unlikely
situation), rather than timing out on an operation that would complete
in the expected time. This happens with low HZ values, which I know
are discouraged nowadays.

Not a big issue, anyway, I'm fine either way.

/alessandro

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Alessandro Rubini,

In message <20090806133110.GA26906@mail.gnudd.com> you wrote:
> 
> Since the timeout is an error condition, It's better to have it longer
> than shorter. Since integer division truncates towards 0, I'd better
> give my hardware more time before signalling an error (an unlikely

That's why I recommended to use DIV_ROUND_UP().


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
Many companies that have made themselves dependent on [the  equipment
of  a  certain  major  manufacturer] (and in doing so have sold their
soul to the devil) will collapse under the sheer weight  of  the  un-
mastered complexity of their data processing systems.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:09 Thu 06 Aug     , Renato Andreola wrote:
>  From 3723c8437d8c3d2e04bc3bc1de9c21b33072ab08 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Thu, 6 Aug 2009 14:49:59 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem 
> in tout calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
> solved avoiding the preprocessor conditional and introducing a compile 
> time branch between a high freq case and a slow freq case.
> 
> Signed-off-by: Renato Andreola renato.andreola at imagos.it
> ---
>   drivers/mtd/cfi_flash.c |    7 ++++---
>   1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 81ac5d3..b118f71 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -659,9 +659,10 @@ static int flash_status_check (flash_info_t * info, 
> flash_sect_t sector,
>   {
>   	ulong start;
> 
> -#if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> -#endif
> +        if (CONFIG_SYS_HZ > 10000)
> +                tout *= CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid 
> overflow */
> +        else
> +                tout = tout * CONFIG_SYS_HZ / 1000 + 1;
as we are all supposed to have CONFIG_SYS_HZ at 1000 (mandtory)
to have cfi, tftp & co working perfectly I do not thing this is a good idea

as you will need to fix each part of u-boot that use CONFIG_SYS_HZ
which make no sense

the best will be to simply fix your timer

Best Regards,
J.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <20090806202615.GH13346@game.jcrosoft.org> you wrote:
>
> as we are all supposed to have CONFIG_SYS_HZ at 1000 (mandtory)
> to have cfi, tftp & co working perfectly I do not thing this is a good idea

Yes, this is the rule, and we would like to enforce it.

> as you will need to fix each part of u-boot that use CONFIG_SYS_HZ
> which make no sense
> 
> the best will be to simply fix your timer

However, the current situation is this: more than 60 boards (all of
them ARM, it seems) use very different settings:

include/configs/EB+MCF-EV123.h:		10000000
include/configs/EP1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
include/configs/EP1S10.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
include/configs/EP1S40.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
include/configs/KAREF.h:		100
include/configs/M5271EVB.h:		1000000
include/configs/METROBOX.h:		100
include/configs/MVBLUE.h:		10000
include/configs/PCI5441.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
include/configs/PK1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
include/configs/SMN42.h:		2048
include/configs/VCMA9.h:		1562500
include/configs/actux1.h:		3333333
include/configs/actux2.h:		3333333
include/configs/actux3.h:		3333333
include/configs/actux4.h:		3333333
include/configs/apollon.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/armadillo.h:		2000
include/configs/assabet.h:		3686400
include/configs/at91rm9200dk.h:		AT91C_MASTER_CLOCK/2
include/configs/at91rm9200ek.h:		(AT91C_MASTER_CLOCK / 2)
include/configs/cmc_pu2.h:		(AT91C_MASTER_CLOCK/2)
include/configs/csb637.h:		AT91C_MASTER_CLOCK/2
include/configs/davinci_dm355evm.h:	24000000
include/configs/davinci_dvevm.h:	27000000
include/configs/davinci_schmoogie.h:	27000000
include/configs/davinci_sffsdr.h:	27000000
include/configs/davinci_sonata.h:	27000000
include/configs/dnp1110.h:		3686400
include/configs/eNET.h:			1024
include/configs/ep7312.h:		2000
include/configs/gcplus.h:		3686400
include/configs/idmr.h:			(50000000 / 64)
include/configs/impa7.h:		2000
include/configs/integratorap.h:		24000000
include/configs/integratorcp.h:		1000000
include/configs/ixdp425.h:		3333333
include/configs/kb9202.h:		AT91C_MASTER_CLOCK/2
include/configs/lart.h:			3686400
include/configs/lpc2292sodimm.h:	2048
include/configs/lpd7a400-10.h:		(508469)
include/configs/lpd7a404-10.h:		(508469)
include/configs/m501sk.h:		AT91C_MASTER_CLOCK/2
include/configs/modnet50.h:		900
include/configs/mp2usb.h:		(AT91C_MASTER_CLOCK/2)
include/configs/mx1ads.h:		3686400
include/configs/mx1fs2.h:		3686400
include/configs/ns9750dev.h:		(CPU_CLK_FREQ/64)
include/configs/omap1610h2.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/omap1610inn.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/omap2420h4.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/omap3_zoom2.h:		((V_SCLK) / (2 << CONFIG_SYS_PTV))
include/configs/omap5912osk.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/omap730p2.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
include/configs/rsk7203.h:		(CONFIG_SYS_CLK_FREQ / CMT_CLK_DIVIDER)
include/configs/sbc2410x.h:		1562500
include/configs/sc520_cdp.h:		1024
include/configs/sc520_spunk.h:		1024
include/configs/scb9328.h:		3686400
include/configs/shannon.h:		3686400
include/configs/smdk2400.h:		1562500
include/configs/smdk2410.h:		1562500
include/configs/trab.h:			1562500
include/configs/versatile.h:		(1000000 / 256)


Until all these are fixed, it indeed makes sense to work around issues
cause by such incorrect settings.

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
As usual, this being a 1.3.x release, I haven't  even  compiled  this
kernel yet. So if it works, you should be doubly impressed.
      - Linus Torvalds in <199506181536.SAA10638@keos.cs.Helsinki.FI>

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

In message <20090806205329.8A31C832E416@gemini.denx.de> I wrote:
> 
> However, the current situation is this: more than 60 boards (all of
> them ARM, it seems) use very different settings:
> 
> include/configs/EB+MCF-EV123.h:		10000000
> include/configs/EP1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/EP1S10.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/EP1S40.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/KAREF.h:		100
> include/configs/M5271EVB.h:		1000000
> include/configs/METROBOX.h:		100
> include/configs/MVBLUE.h:		10000
> include/configs/PCI5441.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/PK1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
...

I was wrong. From the CONFIG_SYS_NIOS_TMRCNT it's pretty obvious that
at least 5 of these are NIOS boards.

Best regards,
Viele Gr??e,

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
Be wiser than other people if you can, but do not tell them so.
                                       -- Philip Earl of Chesterfield

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:53 Thu 06 Aug     , Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
> 
> In message <20090806202615.GH13346@game.jcrosoft.org> you wrote:
> >
> > as we are all supposed to have CONFIG_SYS_HZ at 1000 (mandtory)
> > to have cfi, tftp & co working perfectly I do not thing this is a good idea
> 
> Yes, this is the rule, and we would like to enforce it.
> 
> > as you will need to fix each part of u-boot that use CONFIG_SYS_HZ
> > which make no sense
> > 
> > the best will be to simply fix your timer
> 
> However, the current situation is this: more than 60 boards (all of
> them ARM, it seems) use very different settings:
the rm9200 next release we will have a new timer implementation when the
rm9200 will use the same API as the other at91

btw there is other arch too SH2, nios, ppc, x86 which is listed here

honestly I'll not ack the change otherwise noone will update the timers

as example when working on SH4 this release I've found that they do not have
CONFIG_SYS_HZ at 1000 because of a network problem and then I fix the timer

Best Regards,
J.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <20090806211548.GI13346@game.jcrosoft.org> you wrote:
>
> honestly I'll not ack the change otherwise noone will update the timers

That's a pretty purist position which is not exactly helpful, as it
results in broken systems.

Consider it as a needed workaround until a real fix is available.

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
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

Dear Wolfgang,
I'd like to clarify what is the problem with the timeout and the Intel 
flash (even if the following comments can be obvious or already well 
known) and to ask you an opinion on a small code change.

The flash has an internal busy flag that is polled in function 
flash_status_check() and that function is the only one in cfi_flash.c 
that uses the get_timer() func. and the CONFIG_SYS_HZ definition.
In many Altera/Nios boards the CONFIG_SYS_HZ constant evaluate to 999 
due to rounding errors.
With the current implementation 999 != 1000 evaluate to 1 so the 
CONFIG_SYS_HZ/1000 division is done and returns 0.
This lead to a (forced) 0 timeout in the flash_status_check() that 
corresponds to erroneous flash clear, program, etc..

I've proposed to change the code

from

#if CONFIG_SYS_HZ != 1000
    tout *= CONFIG_SYS_HZ/1000;
#endif

to

 #if CONFIG_SYS_HZ != 1000
    unsigned long long ull;
    ull = tout*CONFIG_SYS_HZ + CONFIG_SYS_HZ/2;
    tout = ull/1000; /* Compute: tout *= CONFIG_SYS_HZ/1000; */
 #endif

but this, as you told me and I agree, is too much architecture dependent 
(it uses a 64bit unsigned long).

The alternative I've proposed, suggested by A.Rubini, is as follow:

if (CONFIG_SYS_HZ > 10000)
    tout *= CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid overflow */
else
    tout = (tout * CONFIG_SYS_HZ) / 1000 + 1;

that leads to an evaluation of the timeout in excess of 1 timer tick.

I think that an expression like this

#if CONFIG_SYS_HZ != 1000
if ((ulong)CONFIG_SYS_HZ > 10000)
    tout *= ((ulong)CONFIG_SYS_HZ)/1000;  /* for a big HZ, avoid 
overflow */
else
    tout = (tout * (ulong)CONFIG_SYS_HZ + 500) / 1000;
#endif

could be better because
- it forces the data type of the system dependent CONFIG_SYS_HZ value to 
ulong (no float!)
- it rounds tout to 0.5 timer tick and leaves tout unchanged if 
CONFIG_SYS_HZ == 1000

What do you think about?

Best regards,
Renato Andreola

The polling time is I've seen that the

Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
>
> In message <20090806202615.GH13346@game.jcrosoft.org> you wrote:
>   
>> as we are all supposed to have CONFIG_SYS_HZ at 1000 (mandtory)
>> to have cfi, tftp & co working perfectly I do not thing this is a good idea
>>     
>
> Yes, this is the rule, and we would like to enforce it.
>
>   
>> as you will need to fix each part of u-boot that use CONFIG_SYS_HZ
>> which make no sense
>>
>> the best will be to simply fix your timer
>>     
>
> However, the current situation is this: more than 60 boards (all of
> them ARM, it seems) use very different settings:
>
> include/configs/EB+MCF-EV123.h:		10000000
> include/configs/EP1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/EP1S10.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/EP1S40.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/KAREF.h:		100
> include/configs/M5271EVB.h:		1000000
> include/configs/METROBOX.h:		100
> include/configs/MVBLUE.h:		10000
> include/configs/PCI5441.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/PK1C20.h:		(CONFIG_SYS_CLK_FREQ/(CONFIG_SYS_NIOS_TMRCNT + 1))
> include/configs/SMN42.h:		2048
> include/configs/VCMA9.h:		1562500
> include/configs/actux1.h:		3333333
> include/configs/actux2.h:		3333333
> include/configs/actux3.h:		3333333
> include/configs/actux4.h:		3333333
> include/configs/apollon.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/armadillo.h:		2000
> include/configs/assabet.h:		3686400
> include/configs/at91rm9200dk.h:		AT91C_MASTER_CLOCK/2
> include/configs/at91rm9200ek.h:		(AT91C_MASTER_CLOCK / 2)
> include/configs/cmc_pu2.h:		(AT91C_MASTER_CLOCK/2)
> include/configs/csb637.h:		AT91C_MASTER_CLOCK/2
> include/configs/davinci_dm355evm.h:	24000000
> include/configs/davinci_dvevm.h:	27000000
> include/configs/davinci_schmoogie.h:	27000000
> include/configs/davinci_sffsdr.h:	27000000
> include/configs/davinci_sonata.h:	27000000
> include/configs/dnp1110.h:		3686400
> include/configs/eNET.h:			1024
> include/configs/ep7312.h:		2000
> include/configs/gcplus.h:		3686400
> include/configs/idmr.h:			(50000000 / 64)
> include/configs/impa7.h:		2000
> include/configs/integratorap.h:		24000000
> include/configs/integratorcp.h:		1000000
> include/configs/ixdp425.h:		3333333
> include/configs/kb9202.h:		AT91C_MASTER_CLOCK/2
> include/configs/lart.h:			3686400
> include/configs/lpc2292sodimm.h:	2048
> include/configs/lpd7a400-10.h:		(508469)
> include/configs/lpd7a404-10.h:		(508469)
> include/configs/m501sk.h:		AT91C_MASTER_CLOCK/2
> include/configs/modnet50.h:		900
> include/configs/mp2usb.h:		(AT91C_MASTER_CLOCK/2)
> include/configs/mx1ads.h:		3686400
> include/configs/mx1fs2.h:		3686400
> include/configs/ns9750dev.h:		(CPU_CLK_FREQ/64)
> include/configs/omap1610h2.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/omap1610inn.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/omap2420h4.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/omap3_zoom2.h:		((V_SCLK) / (2 << CONFIG_SYS_PTV))
> include/configs/omap5912osk.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/omap730p2.h:		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> include/configs/rsk7203.h:		(CONFIG_SYS_CLK_FREQ / CMT_CLK_DIVIDER)
> include/configs/sbc2410x.h:		1562500
> include/configs/sc520_cdp.h:		1024
> include/configs/sc520_spunk.h:		1024
> include/configs/scb9328.h:		3686400
> include/configs/shannon.h:		3686400
> include/configs/smdk2400.h:		1562500
> include/configs/smdk2410.h:		1562500
> include/configs/trab.h:			1562500
> include/configs/versatile.h:		(1000000 / 256)
>
>
> Until all these are fixed, it indeed makes sense to work around issues
> cause by such incorrect settings.
>
> Best regards,
>
> Wolfgang Denk
>
>   

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A7BE28A.8080701@imagos.it> you wrote:
>
> I'd like to clarify what is the problem with the timeout and the Intel 
> flash (even if the following comments can be obvious or already well 
> known) and to ask you an opinion on a small code change.

Thanks.

...
> I think that an expression like this
> 
> #if CONFIG_SYS_HZ != 1000
> if ((ulong)CONFIG_SYS_HZ > 10000)
>     tout *= ((ulong)CONFIG_SYS_HZ)/1000;  /* for a big HZ, avoid 
> overflow */
> else
>     tout = (tout * (ulong)CONFIG_SYS_HZ + 500) / 1000;
> #endif
> 
> could be better because
> - it forces the data type of the system dependent CONFIG_SYS_HZ value to 
> ulong (no float!)
> - it rounds tout to 0.5 timer tick and leaves tout unchanged if 
> CONFIG_SYS_HZ == 1000
> 
> What do you think about?

I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"


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
I came home the other night and tried to open the door  with  my  car
keys...and  the  building started up. So I took it out for a drive. A
cop pulled me over for speeding. He asked me where I  live...  "Right
here".                                                - Steven Wright

[U-Boot] Kirkwood gpio (was: timeout calculation underflow if imprecise 1kHz timer: fix)

[Alessandro Rubini]

> I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"

Ok for me.

BTW: Looking for DIV_ROUND_UP, I found that it's defined in
./include/asm/arch-kirkwood/gpio.h as well. I was checking whether it can
be removed, but it looks like nobody is using the driver.

   tornado$ grep -r CONFIG_KIRKWOOD_GPIO .
   ./drivers/gpio/Makefile:COBJS-$(CONFIG_KIRKWOOD_GPIO)   += kw_gpio.o

The header and C file were added on Jun 29th, but there's not user yet.
It has been tested by Heiko on the Suen3, but it's not mainline.
And even that patch doesn't define CONFIG_KIRKWOOD_GPIO .

/alessandro

[U-Boot] Kirkwood gpio (was: timeout calculation underflow if imprecise 1kHz timer: fix)

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Alessandro Rubini
> Sent: Friday, August 07, 2009 4:00 PM
> To: wd at denx.de
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] Kirkwood gpio (was: timeout calculation 
> underflow if imprecise 1kHz timer: fix)
> 
> > I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"
> 
> Ok for me.
> 
> BTW: Looking for DIV_ROUND_UP, I found that it's defined in 
> ./include/asm/arch-kirkwood/gpio.h as well. I was checking 
> whether it can be removed, but it looks like nobody is using 
> the driver.
I think at least heiko is the customer for this stuff.
Copying Heiko,
Unfortunately I do not have use case for this :-(
But in near future I will have it.
Dear Heiko,
can you pls check mainlined stuff?

Regards..
Prafulla . .

> 
>    tornado$ grep -r CONFIG_KIRKWOOD_GPIO .
>    ./drivers/gpio/Makefile:COBJS-$(CONFIG_KIRKWOOD_GPIO)   += 
> kw_gpio.o
> 
> The header and C file were added on Jun 29th, but there's not 
> user yet.
> It has been tested by Heiko on the Suen3, but it's not mainline.
> And even that patch doesn't define CONFIG_KIRKWOOD_GPIO .
> 
> /alessandro
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] Kirkwood gpio

[Heiko Schocher]

Hello Prafulla,

Prafulla Wadaskar wrote:
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de 
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Alessandro Rubini
>> Sent: Friday, August 07, 2009 4:00 PM
>> To: wd at denx.de
>> Cc: u-boot at lists.denx.de
>> Subject: Re: [U-Boot] Kirkwood gpio (was: timeout calculation 
>> underflow if imprecise 1kHz timer: fix)
>>
>>> I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"
>> Ok for me.
>>
>> BTW: Looking for DIV_ROUND_UP, I found that it's defined in 
>> ./include/asm/arch-kirkwood/gpio.h as well. I was checking 
>> whether it can be removed, but it looks like nobody is using 
>> the driver.
> I think at least heiko is the customer for this stuff.
> Copying Heiko,
> Unfortunately I do not have use case for this :-(
> But in near future I will have it.
> Dear Heiko,
> can you pls check mainlined stuff?

I removed locally DIV_ROUND_UP from ./include/asm/arch-kirkwood/gpio.h
and my suen3 port with I2C bitbang compiles and works fine,
so I think we can remove this.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] Kirkwood gpio

[Heiko Schocher]

Hello Alessandro,

Alessandro Rubini wrote:
>> I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"
> 
> Ok for me.
> 
> BTW: Looking for DIV_ROUND_UP, I found that it's defined in
> ./include/asm/arch-kirkwood/gpio.h as well. I was checking whether it can
> be removed, but it looks like nobody is using the driver.
> 
>    tornado$ grep -r CONFIG_KIRKWOOD_GPIO .
>    ./drivers/gpio/Makefile:COBJS-$(CONFIG_KIRKWOOD_GPIO)   += kw_gpio.o
> 
> The header and C file were added on Jun 29th, but there's not user yet.
> It has been tested by Heiko on the Suen3, but it's not mainline.
> And even that patch doesn't define CONFIG_KIRKWOOD_GPIO .

This board will come soon with I2C bitbang support, which uses this
driver. But this driver comes from Dieter Kiermaier, and I thought
it is used from others too ...

bye
Heiko

[U-Boot] Kirkwood gpio (was: timeout calculation underflow if imprecise 1kHz timer: fix)

[Dieter Kiermaier]

Hi Alessandro,


> > I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"
> 
> Ok for me.
> 
> BTW: Looking for DIV_ROUND_UP, I found that it's defined in
> ./include/asm/arch-kirkwood/gpio.h as well. I was checking whether it can
> be removed, but it looks like nobody is using the driver.
> 
>    tornado$ grep -r CONFIG_KIRKWOOD_GPIO .
>    ./drivers/gpio/Makefile:COBJS-$(CONFIG_KIRKWOOD_GPIO)   += kw_gpio.o
> 
> The header and C file were added on Jun 29th, but there's not user yet.
> It has been tested by Heiko on the Suen3, but it's not mainline.
> And even that patch doesn't define CONFIG_KIRKWOOD_GPIO .

I'm working on a bitbanging FPGA load driver based on kirkwood gpio driver at the moment.
But I'm on holiday for the next 3 weeks. After it planning to submit new Lattice fpga load driver
which uses kirkwood gpio driver.

Dieter


> 
> /alessandro
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

Ok, for the change.
What is the preferred way to proceed? have I got to resubmit a patch for 
the change with the DIV_ROUND_UP macro?

Best regards,
Renato Andreola

Wolfgang Denk wrote:
> Dear Renato Andreola,
>
> In message <4A7BE28A.8080701@imagos.it> you wrote:
>   
>> I'd like to clarify what is the problem with the timeout and the Intel 
>> flash (even if the following comments can be obvious or already well 
>> known) and to ask you an opinion on a small code change.
>>     
>
> Thanks.
>
> ...
>   
>> I think that an expression like this
>>
>> #if CONFIG_SYS_HZ != 1000
>> if ((ulong)CONFIG_SYS_HZ > 10000)
>>     tout *= ((ulong)CONFIG_SYS_HZ)/1000;  /* for a big HZ, avoid 
>> overflow */
>> else
>>     tout = (tout * (ulong)CONFIG_SYS_HZ + 500) / 1000;
>> #endif
>>
>> could be better because
>> - it forces the data type of the system dependent CONFIG_SYS_HZ value to 
>> ulong (no float!)
>> - it rounds tout to 0.5 timer tick and leaves tout unchanged if 
>> CONFIG_SYS_HZ == 1000
>>
>> What do you think about?
>>     
>
> I'd prefer "tout = DIV_ROUND_UP(tout*CONFIG_SYS_HZ, 1000);"
>
>
> Best regards,
>
> Wolfgang Denk
>
>   

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A7C1A8E.2080208@imagos.it> you wrote:
> Ok, for the change.
> What is the preferred way to proceed? have I got to resubmit a patch for 
> the change with the DIV_ROUND_UP macro?

Yes, please.

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
Making files is easy under  the  UNIX  operating  system.  Therefore,
users  tend  to  create  numerous  files  using large amounts of file
space. It has been said that the only standard thing about  all  UNIX
systems  is  the  message-of-the-day  telling users to clean up their
files.                             - System V.2 administrator's guide

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

 From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
From: Renato Andreola <renato.andreola@imagos.it>
Date: Fri, 21 Aug 2009 18:05:51 +0200
Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation

With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>
---
  drivers/mtd/cfi_flash.c |    8 +++++---
  1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 81ac5d3..0d8fc54 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
  	ulong start;

  #if CONFIG_SYS_HZ != 1000
-	tout *= CONFIG_SYS_HZ/1000;
-#endif
-
+	if ((ulong)CONFIG_SYS_HZ > 100000)
+		tout *= (ulong)CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid overflow */
+	else
+		tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);
+#endif	
  	/* Wait for command completion */
  	start = get_timer (0);
  	while (flash_is_busy (info, sector)) {
-- 
1.5.5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Shinya Kuribayashi]

Renato Andreola wrote:
>  From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Fri, 21 Aug 2009 18:05:51 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
> 
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>
> ---
>   drivers/mtd/cfi_flash.c |    8 +++++---
>   1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 81ac5d3..0d8fc54 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
>   	ulong start;
> 
>   #if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> -#endif
> -
> +	if ((ulong)CONFIG_SYS_HZ > 100000)
> +		tout *= (ulong)CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid overflow */
> +	else
> +		tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);
> +#endif	
>   	/* Wait for command completion */
>   	start = get_timer (0);
>   	while (flash_is_busy (info, sector)) {

What should to be fixed first in this case, would be your CONFIG_SYS_HZ
setting, that is NIOS2? timer implementation, yeah really.  But I would
also point out that there is another case flash_status_check() doensn't
work as expected.

One of my colleagues found that with some flash device(s) (I don't
recall precisely, sorry), 'tout' would be probed to be zero.  In that
case, a workaround something like above still doesn't work.

We have not sorted out where the problem is; it might be in cfi_flash.c,
or in the flash device itself.  This is observed with v2009.03 release,
and we've been having a workaround for it for months.  I'd like to have
a look someday.

Anyway, checking to see if 'tout' is zero or not would be sometimes
worth a try, when you think cfi_flash.c doesn't work as expected.

Just for your information,

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

This patch is necessary to fix the common case in which CONFIG_SYS_HZ is 
just below 1000 (e.g. 999 that results from an integer division between 
a system clock frequency like 83333333Hz and a divisor like 83334).
In that case the CONFIG_SYS_HZ/1000 expression returns 0 and the tout *= 
CONFIG_SYS_HZ/1000 multiplication is always 0.
As you said, an incorrect Common Flash Interface interrogation of the 
flash registers can lead to a tout of 0 but this misbehavior should have 
been caught by the first stages of the flash interrogation code.
Note that the AMD/Spansion chips does not work correctly in a Nios2 
environment due to "spurious" read cycles generated when flash bus width 
is lower than 32.
If the flash chip you used is not an Intel/Numoyx than you should have 
to do some work to make them behave correctly (e.g. add extra wait state 
into the procedures or some other tricks).
 
Renato

Shinya Kuribayashi wrote:
> Renato Andreola wrote:
>   
>>  From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
>> From: Renato Andreola <renato.andreola@imagos.it>
>> Date: Fri, 21 Aug 2009 18:05:51 +0200
>> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
>>
>> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
>>
>> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>
>> ---
>>   drivers/mtd/cfi_flash.c |    8 +++++---
>>   1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
>> index 81ac5d3..0d8fc54 100644
>> --- a/drivers/mtd/cfi_flash.c
>> +++ b/drivers/mtd/cfi_flash.c
>> @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
>>   	ulong start;
>>
>>   #if CONFIG_SYS_HZ != 1000
>> -	tout *= CONFIG_SYS_HZ/1000;
>> -#endif
>> -
>> +	if ((ulong)CONFIG_SYS_HZ > 100000)
>> +		tout *= (ulong)CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid overflow */
>> +	else
>> +		tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);
>> +#endif	
>>   	/* Wait for command completion */
>>   	start = get_timer (0);
>>   	while (flash_is_busy (info, sector)) {
>>     
>
> What should to be fixed first in this case, would be your CONFIG_SYS_HZ
> setting, that is NIOS2? timer implementation, yeah really.  But I would
> also point out that there is another case flash_status_check() doensn't
> work as expected.
>
> One of my colleagues found that with some flash device(s) (I don't
> recall precisely, sorry), 'tout' would be probed to be zero.  In that
> case, a workaround something like above still doesn't work.
>
> We have not sorted out where the problem is; it might be in cfi_flash.c,
> or in the flash device itself.  This is observed with v2009.03 release,
> and we've been having a workaround for it for months.  I'd like to have
> a look someday.
>
> Anyway, checking to see if 'tout' is zero or not would be sometimes
> worth a try, when you think cfi_flash.c doesn't work as expected.
>
> Just for your information,
>
>   

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Stefan Roese]

On Monday 24 August 2009 10:32:17 Renato Andreola wrote:
>  From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Fri, 21 Aug 2009 18:05:51 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in
> tout calculation
>
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
>
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola
> <renato.andreola@imagos.it>

A few mostly formal comments about this patch:

- Patch doesn't apply:

[stefan at stefan-desktop u-boot-cfi-flash (master)]$ git am -s patches_misc/\[U-
Boot\]\ PATCH\ mtd\ CFI\ flash\:\ timeout\ calculation\ underflow\ if\ 
imprecise\ 1kHz\ timer\:\ fix.mbox                                                                                   
Applying: PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz 
timer: fix
/home/stefan/git/u-boot/u-boot-cfi-flash/.git/rebase-apply/patch:20: trailing 
whitespace.
#endif
error: patch failed: drivers/mtd/cfi_flash.c:660
error: drivers/mtd/cfi_flash.c: patch does not apply

Please fix by using "git format-patch" and "git send-email".

- Patch subject is mangled:

Applying your patch (if possible) would lead to this commit text/subject:
"PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: 
fix". This is because your email subject is incorrect (e.g. PATCH should be 
[PATCH]). And the subject it too long as well. Please use a shorter line here.

- Minor coding style problem (see below)

> ---
>   drivers/mtd/cfi_flash.c |    8 +++++---
>   1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 81ac5d3..0d8fc54 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -660,9 +660,11 @@ static int flash_status_check (flash_info_t * info,
> flash_sect_t sector, ulong start;
>
>   #if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> -#endif
> -
> +	if ((ulong)CONFIG_SYS_HZ > 100000)
> +		tout *= (ulong)CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid 
overflow */

Please insert spaces before and after the "/" above.

> +	else
> +		tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);
> +#endif
>   	/* Wait for command completion */
>   	start = get_timer (0);
>   	while (flash_is_busy (info, sector)) {


Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Stefan Roese]

On Tuesday 25 August 2009 17:39:56 Stefan Roese wrote:
> > +	if ((ulong)CONFIG_SYS_HZ > 100000)
> > +		tout *= (ulong)CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid
>
> overflow */
>
> Please insert spaces before and after the "/" above.
>
> > +	else
> > +		tout = DIV_ROUND_UP(tout*(ulong)CONFIG_SYS_HZ, 1000);

And spaces before and after the "*" above as well.

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A925011.9080906@imagos.it> you wrote:
>  From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Fri, 21 Aug 2009 18:05:51 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
> 
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>

I think we are still waiting for your resubmit? Is this correct?

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
Q: Why do PCs have a reset button on the front?
A: Because they are expected to run Microsoft operating systems.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

Dear Wolfgang Denk,
I've sent the last patch regarding the CFI timeout underflow on 24th
August 2009 and then I Have done no more activity on U-Boot for Nios2.
I've done no more activity regarding the cache flush in
lib_nios2/bootm.c too.
As I remember, the problems with the 2 patches were related to
code white space formatting and a copyright note that can be deleted.

I hope I'll have time to go on with the activity on nios2
starting from the beginning of 2010.

Best regards,
Renato Andreola

Wolfgang Denk wrote:
> Dear Renato Andreola,
>
> In message <4A925011.9080906@imagos.it> you wrote:
>   
>>  From 21d84ab72266f118794233176bd356d8b1cfdf35 Mon Sep 17 00:00:00 2001
>> From: Renato Andreola <renato.andreola@imagos.it>
>> Date: Fri, 21 Aug 2009 18:05:51 +0200
>> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation
>>
>> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000.
>>
>> Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>
>>     
>
> I think we are still waiting for your resubmit? Is this correct?
>
> Best regards,
>
> Wolfgang Denk
>
>   

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4B0A4D04.4030901@imagos.it> you wrote:
>
> I've sent the last patch regarding the CFI timeout underflow on 24th
> August 2009 and then I Have done no more activity on U-Boot for Nios2.
> I've done no more activity regarding the cache flush in
> lib_nios2/bootm.c too.
> As I remember, the problems with the 2 patches were related to
> code white space formatting and a copyright note that can be deleted.

Yes, and we are waiting for you to resubmit a cleaned up patch. Maybe
you can squeeze this in somehow?

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] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

 From 341dbd88695d3514699aae612d640ed93feb8821 Mon Sep 17 00:00:00 2001
From: Renato Andreola <renato.andreola@imagos.it>
Date: Thu, 6 Aug 2009 18:05:00 +0200
Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem in tout calculation

With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
solved avoiding the preprocessor conditional and introducing a compile time branch between a high freq case and a slow freq case.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com> Renato Andreola <renato.andreola@imagos.it>
---
  drivers/mtd/cfi_flash.c |    7 ++++---
  1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 81ac5d3..499044f 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -659,9 +659,10 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector,
  {
  	ulong start;

-#if CONFIG_SYS_HZ != 1000
-	tout *= CONFIG_SYS_HZ/1000;
-#endif
+        if (CONFIG_SYS_HZ > 10000)
+		tout *= CONFIG_SYS_HZ/1000;  /* for a big HZ, avoid overflow */
+        else
+		tout = (tout * CONFIG_SYS_HZ) / 1000 + 1;

  	/* Wait for command completion */
  	start = get_timer (0);
-- 
1.5.5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

 From be54cb97ca26bcbbc1a908d1f2a5447b6639dc59 Mon Sep 17 00:00:00 2001
From: Renato Andreola <renato.andreola@imagos.it>
Date: Thu, 6 Aug 2009 11:40:52 +0200
Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem 
in tout calculation

With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
solved using an unsigned long long

Signed-off-by: Renato Andreola renato.andreola@imagos.it
---
  drivers/mtd/cfi_flash.c |    6 ++++--
  1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 81ac5d3..6bcd102 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -659,8 +659,10 @@ static int flash_status_check (flash_info_t * info, 
flash_sect_t sector,
  {
  	ulong start;

-#if CONFIG_SYS_HZ != 1000
-	tout *= CONFIG_SYS_HZ/1000;
+#if CONFIG_SYS_HZ != 1000	
+	unsigned long long ull;
+	ull = tout*CONFIG_SYS_HZ + CONFIG_SYS_HZ/2;
+	tout = ull/1000; /* Compute: tout *= CONFIG_SYS_HZ/1000; */
  #endif

  	/* Wait for command completion */
-- 
1.5.5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A7AA72C.8010704@imagos.it> you wrote:
>  From be54cb97ca26bcbbc1a908d1f2a5447b6639dc59 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Thu, 6 Aug 2009 11:40:52 +0200
> Subject: [PATCH] drivers/mtd/cfi_flash: precision and underflow problem 
> in tout calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
> solved using an unsigned long long

I don't like this implementation - using LL divisions just pulls in
more dependencies on libgcc code and the like, and blows up the code
size.

> -#if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> +#if CONFIG_SYS_HZ != 1000	
> +	unsigned long long ull;
> +	ull = tout*CONFIG_SYS_HZ + CONFIG_SYS_HZ/2;
> +	tout = ull/1000; /* Compute: tout *= CONFIG_SYS_HZ/1000; */

I think rounding up should be sufficient here, i. e. something like:

	tout *= DIV_ROUND_UP(CONFIG_SYS_HZ,1000);

?

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
Ninety-Ninety Rule of Project Schedules:
        The first ninety percent of the task takes ninety percent of
the time, and the last ten percent takes the other ninety percent.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Alessandro Rubini]

> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
> solved using an unsigned long long

I had the same problem with an ancient version of a vendor-ported u-boot. There
my CFG_HZ was 100, so the timeout was 0.

I used this, which avoids the preprocessor conditional. Since the HZ value
is a compile-time constant, the compiler chooses the if or else branch and doesn't
spit a run-time conditional.2

        if (CFG_HZ > 100000)
                tout *= CFG_HZ/1000;  /* for a big HZ, avoid overflow */
        else
                tout = tout * CFG_HZ / 1000 + 1;


/alessandro

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Wolfgang Denk]

Dear Renato Andreola,

In message <4A7AB614.2020509@imagos.it> you wrote:
> <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
> <html>
> <head>

Please NEVER post HTML here!!

> I agree with Wolfgang Denk in not using the LL type (too
> compiler/library/machine dependent).<br>
> I think that the code of Alessandro Rubini that makes the runtime
> decision using no extra macro definition is more system independent and

This is no extra macro definition. This macro is available by default
(defined in include/common.h).

> keeps a small 1% residual error when CFG_HZ is just above 100000.<br>

Keep in mind that systems with CONFIG_SYS_HZ != 1000 are considered
broken anyway.

As little effort as possible should be spent to support these -
efforts should rather be focussed on fixing these systems.


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
A meeting is an event at which the minutes are kept and the hours are
lost.

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Renato Andreola]

 From 6cc63851e5e4c16012d9afad70cedb41b7340de7 Mon Sep 17 00:00:00 2001
From: Renato Andreola <renato.andreola@imagos.it>
Date: Fri, 10 Apr 2009 12:52:55 +0200
Subject: drivers/mtd/cfi_flash: precision and underflow problem in tout 
calculation

With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
solved using an unsigned long long
---
  drivers/mtd/cfi_flash.c |    4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 175d82a..0aa42a2 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -660,7 +660,9 @@ static int flash_status_check (flash_info_t * info, 
flash_sect_t sector,
  	ulong start;

  #if CONFIG_SYS_HZ != 1000
-	tout *= CONFIG_SYS_HZ/1000;
+	unsigned long long ull;
+	ull = tout*CONFIG_SYS_HZ + CONFIG_SYS_HZ/2;
+	tout = ull/1000; /* Compute: tout *= CONFIG_SYS_HZ/1000; */
  #endif

  	/* Wait for command completion */
-- 
1.5.5

[U-Boot] PATCH mtd CFI flash: timeout calculation underflow if imprecise 1kHz timer: fix

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:28 Fri 10 Apr     , Renato Andreola wrote:
>  From 6cc63851e5e4c16012d9afad70cedb41b7340de7 Mon Sep 17 00:00:00 2001
> From: Renato Andreola <renato.andreola@imagos.it>
> Date: Fri, 10 Apr 2009 12:52:55 +0200
> Subject: drivers/mtd/cfi_flash: precision and underflow problem in tout 
> calculation
> 
> With old configuration it could happen tout=0 if CONFIG_SYS_HZ<1000
> solved using an unsigned long long
> ---
>   drivers/mtd/cfi_flash.c |    4 +++-
>   1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 175d82a..0aa42a2 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -660,7 +660,9 @@ static int flash_status_check (flash_info_t * info, 
> flash_sect_t sector,
>   	ulong start;
> 
>   #if CONFIG_SYS_HZ != 1000
> -	tout *= CONFIG_SYS_HZ/1000;
> +	unsigned long long ull;
> +	ull = tout*CONFIG_SYS_HZ + CONFIG_SYS_HZ/2;
> +	tout = ull/1000; /* Compute: tout *= CONFIG_SYS_HZ/1000; */
>   #endif
drop all of this and fix your CONFIG_SYS_HZ to 1000 (non configurable value)
will be better

Best Regards,
J.