On Wed, 08 Feb 2012 01:17:04 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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 ;)