[coreboot] [PATCH] flashrom: Add external programmer delay functions

[Carl-Daniel Hailfinger]

On 05.06.2009 19:23, Uwe Hermann wrote:
> On Fri, Jun 05, 2009 at 06:28:41PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> Add external programmer delay functions so external programmers can
>> handle the delay on their own if needed.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>     
>
> Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
>
> ...but see below.
>   

Thanks for the review!


>> Index: flashrom-programmer_delay/flash.h
>> ===================================================================
>> --- flashrom-programmer_delay/flash.h	(Revision 576)
>> +++ flashrom-programmer_delay/flash.h	(Arbeitskopie)
>> @@ -118,6 +120,7 @@
>>  uint8_t chip_readb(const chipaddr addr);
>>  uint16_t chip_readw(const chipaddr addr);
>>  uint32_t chip_readl(const chipaddr addr);
>> +void programmer_delay(uint32_t usecs);
>>     
>
> Why uint32_t here and int above?
>   

Leftover from an earlier version. Should be int.


>> Index: flashrom-programmer_delay/spi.c
>> ===================================================================
>> --- flashrom-programmer_delay/spi.c	(Revision 576)
>> +++ flashrom-programmer_delay/spi.c	(Arbeitskopie)
>> @@ -425,7 +425,7 @@
>>  	 */
>>  	/* FIXME: We assume spi_read_status_register will never fail. */
>>  	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>> -		sleep(1);
>> +		programmer_delay(1000000);
>>     
>
> I'd also make this 100 * 1000 as below, easier on the eyes.
>   

1000 * 1000, sure.

Changed for all other calls as well.

>> Index: flashrom-programmer_delay/internal.c
>> ===================================================================
>> --- flashrom-programmer_delay/internal.c	(Revision 576)
>> +++ flashrom-programmer_delay/internal.c	(Arbeitskopie)
>> @@ -195,6 +195,18 @@
>>  	return *(volatile uint32_t *) addr;
>>  }
>>  
>> +void internal_delay(int usecs)
>> +{
>> +	/* If the delay is >1 s, use usleep because timing does not need to
>> +	 * be so precise.
>>     
>
> Hm, but why not use myusec_delay() always? Does it have drawbacks?
>   

Yes. myusec_delay is a busy-wait to get some precision. Hogging the CPU
for more than 1 second is bad style.


Committed with your suggestions in r578.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/