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.2...@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/ _______________________________________________ flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
