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