[coreboot] Flashrom jedec probe patch + AT29C010A logs

[Carl-Daniel Hailfinger]

On 03.06.2009 00:26, Maciej Pijanka wrote:
> On 02/06/2009, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>   
>> On 02.06.2009 21:57, Maciej Pijanka wrote:
>>     
>>> Hello
>>>
>>> Carl-Daniel pointed me to easy tasks list in wiki, i tried to prepare some
>>> patch that allow to add probe_timing information (int uS value) used
>>> by jedec_probe (patch attached)
>>>
>>>       
>> Thanks for the patch.
>> Review follows:
>> - probe_49fl00x is a wrapper for probe_jedec. Those chips need the
>> probe_timing parameter as well.
>> - probe_sst_fwhub has the same problem.
>> - If someone forgets the probe_timing parameter, it will be zero. That's
>> bad for almost all chips. Maybe add an explicit check for nonzero delays?
>> - It might be best if every chip definition got that probe_timing
>> parameter. Other probe functions could use similar delay info.
>> - SPI chips want zero delay.
>>     
>
> fixed patch in attachment
>   

I really like it.
Other than the comment on IRC, there is one minor nitpick:

> Index: jedec.c
> ===================================================================
> --- jedec.c	(revision 567)
> +++ jedec.c	(working copy)
> @@ -91,7 +91,25 @@
>  
> +	if (flash->probe_timing > 0) 
> +		probe_timing_enter = probe_timing_exit = flash->probe_timing;
> +	else if (flash->probe_timing == TIMING_ZERO) { /* INTENTIONALLY NO DELAY */
> +		probe_timing_enter = probe_timing_exit = 0;
> +	} else if (flash->probe_timing == TIMING_FIXME) { /* FIXME */ 
> +		printf_debug("Chip lacks correct probe timing information, using default 10mS/40uS\n");
> +		probe_timing_enter = 10000;
> +		probe_timing_exit = 40;
> +	} else if (flash->probe_timing == 0) { /* NOT SET at all ? */
> +		printf_debug("Chip lacks correct probe timing information, using default 10mS/40uS\n");
> +		probe_timing_enter = 10000;
> +		probe_timing_exit = 40;
>   

If timing is not set (0), maybe fail as well? That would be accomplished
by killing the above if branch and changing the message below to say
"negative/ininitialized value".

> +	} else {
> +		printf_debug("Chip has negative value of probe_timing, failing without chip access\n");
> +		return 0;
> +	}
>   

Other than that, I think the patch can be Acked.

Regards,
Carl-Daniel

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