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

[Stefan Tauner]

On Wed, 08 Feb 2012 01:17:04 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 07.02.2012 23:23 schrieb Stefan Tauner:
> > On Wed, 01 Feb 2012 00:03:34 +0100
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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.
> 
> Right. The direction is a don't-care thing for dummy bytes. Maybe just
> add a FIXME comment that we should handle dummy bytes explicitly later.

		/* FIXME: the following dummy byte explodes on some programmers.
		 * One possible workaround would be to read the dummy byte
		 * instead and discard its value.
		 */
will post the whole reworked patch later, so it is probably better if
you would comment to that then.

> >>> […]
> >>> +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*
> 
> Now that you point me to the alternative, I have to say that I think the
> typedef is less readable (at least the typedef definition itself).

a few lines above that line is the definition of struct flash(ctx)
which defines the different function pointers almost identically...
this stuff is just not made to be read i think :) if you compare the
two versions of get_erasefn_from_opcode you will notice the
obvious benefit:

int (*get_erasefn_from_opcode(uint8_t opcode)) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen)
vs.
erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode)

yes, this is really equivalent and the first one is not intentionally
obfuscated. :)

> >>> […]
> >>> +		.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
> 
> Yes, it's still in beta. But I can send it anyway so people can review
> it mercilessly.

cant hurt to post it... we are not that ruthless usually ;)

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner