[coreboot] Flashrom jedec probe patch + AT29C010A logs
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Jun 3 00:44:25 CEST 2009
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/
More information about the coreboot
mailing list