Re: [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  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