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