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@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=0Ag1Kfbw63vWfdGFYWk5qSG4xYXhwQkt...
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