Re: [flashrom] [PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Wed, 01 Feb 2012 00:03:34 +0100
Carl-Daniel Hailfinger  wrote:
> Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > […]
> > todo:
> > handle programmers which have a problem with the dummy bytes needed
>
> AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
> write and read, IIRC it is called DropOneClkOnRead or somthing like
> that. Quite a few other SPI masters have the one-dummy-byte
> functionality as well. This needs to be implemented in a generic way (I
> have a totally bitrotted patch for it), but it should not hold back this
> patch.

what about simulating the dummy byte by reading one additional byte in
the beginning instead of writing one? due to SPI's underlying principle
of shifting bits in and out of master and slave simultaneously this
should give us the same effect, but eases working around programmer
limitations.
> > […]
> > +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>
> I believe that typedef to be pretty unreadable, but I see that avoiding
> the typedef would probably be even worse.

yes :) see http://patchwork.coreboot.org/patch/3492/ ... *shiver*
> > […]
> > .read		= spi_chip_read,
> > .page_size	= 256, /* ? */
>
> Argh, page_size comes to bite us again. Did I already send my "kill most
> uses of page_size" patch?

afaics no
> > /* FIXME: some vendor extensions define this */
> > .voltage	= {},
> >  /* Everything below will be set by the probing function. */
> > .write		= NULL,
> > .total_size	= 0,
> > .feature_bits	= 0,
> > .block_erasers	= {},
> >
> > },
> >
> > {
> > .vendor		= "Unknown",
> >
> > diff --git a/flashchips.h b/flashchips.h
> > index 03efb86..1f2a8ca 100644
> > --- a/flashchips.h
> > +++ b/flashchips.h
> > @@ -36,6 +36,7 @@
> >
> > #define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
> >  #define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> > +#define SFDP_DEVICE_ID		0xfffe
>
> Side note: Should we move PROGMANUF_ID and its companion from the bottom
> of the file to this location to have generic match IDs in one place?

yes and i took the opportunity to do that right away, chunks now looks
like this (i also changed the hex characters to upper case like in the
rest of the file):

-#define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
-#define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
-#define SFDP_DEVICE_ID		0xfffe
+#define GENERIC_MANUF_ID	0xFFFF	/* Check if there is a vendor ID */
+#define GENERIC_DEVICE_ID	0xFFFF	/* Only match the vendor ID */
+#define SFDP_DEVICE_ID		0xFFFE
+#define PROGMANUF_ID		0xFFFE	/* dummy ID for opaque chips behind a programmer */
+#define PROGDEV_ID		0x01	/* dummy ID for opaque chips behind a programmer */
> > #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
> >  #define ALLIANCE_AS29F002B	0x34
> > diff --git a/flashrom.c b/flashrom.c
> > index ee68344..84fb3fc 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -986,7 +986,33 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> >    	 * probe_flash() is the first one and thus no chip has been
> >    	 * found before.
>
> The comment above is not really valid anymore. Can you include the
> following snippet in your patch?
>
> @@ -980,11 +980,10 @@
> /* If this is the first chip found, accept it.
>  * If this is not the first chip found, accept it only if it is
>  * a non-generic match.
>  * We could either make chipcount global or provide it as
>  * parameter, or we assume that startchip==0 means this call to
>  * probe_flash() is the first one and thus no chip has been
>  * found before.
>  * a non-generic match. SFDP and CFI are generic matches.
>  * startchip==0 means this call to probe_flash() is the first
>  * one for this programmer interface and thus no other chip has
>  * been found on this interface.
> */
> if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
> 	break;

done
> > […]
> > +/* FIXME: eventually something similar like this but more generic should be
> > available to split up spi commands. use that then instead */
> >
> > +static int spi_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
> > +{
> > /* FIXME: this is wrong. */
> > int maxstep = 8;
>
> Yes, maxstep should be a programmer-specific setting. However, with our
> current infrastructure there is no way to fix this easily.
> > int ret = 0;
> > while (len > 0) {
> > int step = min(len, maxstep);
> > ret = spi_sfdp_wrapper(flash, address, buf, step);
> > if (ret)
>
> Actually, there is probably a way to determine optimal size for those
> SFDP requests: Check if ret indicates a "command too long" error and
> reduce maxstep by one in that case, then retry. Such code is not exactly
> pretty and I'm not sure if all SPI masters have such a differentiated
> error code reporting.

i have made this table from a code review:
https://docs.google.com/spreadsheet/ccc?key=0Ag1Kfbw63vWfdGFYWk5qSG4xYXhwQkt...

the only SPI programmer that has a small upper bound on the number of
bytes to send/receive and that does not report SPI_INVALID_LENGTH is
dediprog. it has the checks in place but just returns 1, so this is
easily fixable.

The code would probably be quite complex for little gain in speed
(if at all), so i would say that just using a small packet size
(readcnt = 8) is the best solution until we support the more
restrictive programmers/for now.
> > […]
> >
> > +static int sfdp_add_uniform_eraser(struct flashctx *f, uint8_t opcode, uint32_t bsize)
>
> struct flashctx *flash
> Just "f" is too short, and if you try to search for it, you'll get
> thousands of matches you don't want.
>
> bsize or blocksize? I prefer the latter for consistency and readability
> reasons.

both done (here in sfdp_add_uniform_eraser and sfdp_fill_flash too).
> > +{
> > uint32_t total_size = f->total_size * 1024;
> > int i;
> > erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
> > if (erasefn == NULL)
> > return 1;
> > for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
> > struct block_eraser *eraser = &f->block_erasers[i];
> > if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase)
> > 	continue;
> > eraser->block_erase = erasefn;
> > eraser->eraseblocks[0].size = bsize;
> > eraser->eraseblocks[0].count = total_size/bsize;
> > msg_cdbg2("  Block eraser %d: %d x %d B with opcode "
> > 	  "0x%02x\n", i, total_size/bsize, bsize,
> > 	  opcode);
> > return 0;
> > }
> > msg_cinfo("%s: Not enough space to store another eraser (i=%d)."
>
> msg_cerr?

that's the question :)
my rationale is: if there are already MAX erasers, we can continue
without a problem but it would be nice to increase the limit if someone
reports it. msg_cinfo will guarantee that normal users will see this,
so no need for msg_cerr.
there are good arguments for msg_cerr too... if we move this function
to spi25.c i'd vote for err, if it stays in sfdp.c i dont really care.