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

Wed Feb 8 01:17:04 CET 2012

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.

>>> […]
>>> +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).

>>> […]
>>> +		.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.

>>> +		/* 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 */

Thanks!

>>>  
>>>  #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
>>>
>>> […]
>>> +/* 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=0Ag1Kfbw63vWfdGFYWk5qSG4xYXhwQktDUzdmNmc0WVE&hl=en_US#gid=0

Thanks.

> 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.

Hm OK. Not really happy, but we can always do this with a followup
patch. Just add a FIXME comment.

>>> +{
>>> +	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.

msg_cinfo it is, then.

Regards,
Carl-Daniel

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