[responding to earlier private mail from carl-daniel, when i initially sent the patch directly to him.]
carl-daniel wrote:
Hi Paul,
could you include coreboot@coreboot.org in your reply? That will allow the other developers to chime in. Thanks.
On 11.06.2009 15:46, Paul Fox wrote:
given the way the options are parsed, i expected to be able to run "flashrom -Ewv myfile.img" and have the chip erased, written, and verified in one pass.
Heh. I heard from people who expected first write, then erase. But I admit your interpretation follows the principle of least surprise.
it also matches the behavior of every chip-level programmer i've ever used.
this doesn't currently work, and in fact, will zero the chip without telling you, and tell you that the operation succeeded. much hilarity ensues. :-)
Ouch. This is a long-standing user interface bug. I'm arguing that
- flashrom should auto-verify all operations by default.
- read/erase and read/write and probably even erase/write should be
rejected by flashrom because it is not clear in which order the user wants to run them.
well, i'll have to allow as how given the primary usage of flashrom, i.e., re-flashing one's expensive new motherboard, that one might want the option parsing to be as bullet- and idiot-proof as possible. but speaking as someone who programs flash chips for a living (at least, it seems like that sometimes), i'd like to reduce the length of my edit/compile/erase/write/verify/debug/edit cycle as much as possible. i can barely imagine using a programmer in anything but erase/write/verify mode, and i certainly want to invoke them all at once.
because of the else-if at line 767, specifying -E along -w used to bypass the file open and read, leaving buf[] empty. the chip would then be programmed with nulls. worse, if -v was also used, it would verify correctly.
That needs to be fixed.
with this patch (which makes the usage message match the actual arg parsing) the above command invocation now works correctly.
for real robustness, the verify step should probably re-read the file, in case one of the many worker routines accidentally writes to the buffer. but that's a different patch..
Hm true. Then again, marking the buffer const except in the read-file routine should be a pretty good protection against buffer writing, considering that we tell gcc to error out on any warning.
not really good enough. the code in the ft2232 programmer currently memcpys the user's write data at least once, maybe twice, before it reaches the programmer. those buffers aren't const, of course. (it might be possible to rewrite so the original data never moves, but i think my point remains.) re-reading the file (or, re-copying from an initial "read-only" buffer) should be pretty cheap.
paul =--------------------- paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 55.8 degrees)
but speaking as someone who programs flash chips for a living (at least, it seems like that sometimes), i'd like to reduce the length of my edit/compile/erase/write/verify/debug/edit cycle as much as possible. i can barely imagine using a programmer in anything but erase/write/verify mode, and i certainly want to invoke them all at once.
Right now -wv does that. I've actually never used -E.
Thanks, Myles
myles wrote:
but speaking as someone who programs flash chips for a living (at least, it seems like that sometimes), i'd like to reduce the length of my edit/compile/erase/write/verify/debug/edit cycle as much as possible. i can barely imagine using a programmer in anything but erase/write/verify mode, and i certainly want to invoke them all at once.
Right now -wv does that. I've actually never used -E.
does flashrom's write do an erase beforehand?
paul =--------------------- paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 55.8 degrees)
Boy, this discussion is getting really complicated. That's a sign that it is time to STOP! and think it all over :-)
We're going to guess the order of operations based on arguments, except when we ignore the argument order of operations, because we think we're smarter than the user? And then we're going to reread the file and ....
I'm an old unix buzzard. Our philosophy was, processes are cheap. That's why things like ls | grep are deemed acceptable.
Tools do one thing (or one thing at a time) and do it well. flashrom is getting radically over-engineered.
Please rethink this.
flashrom -e -- erase flashrom -r -- read flashrom -w -- write flashrom -v -- verify
flashrom -any combination -- mistake
Otherwise, no matter how you set this up, you're going to violate somebody's principle of least surprise. This thread already shows how.
If you want more than one thing done, that's what scripts are for :-)
thanks
ron
ron wrote:
Boy, this discussion is getting really complicated. That's a sign that it is time to STOP! and think it all over :-)
in my defense, i thought i had. :-)
We're going to guess the order of operations based on arguments, except when we ignore the argument order of operations, because we think we're smarter than the user? And then we're going to reread the file and ....
I'm an old unix buzzard. Our philosophy was, processes are cheap. That's why things like ls | grep are deemed acceptable.
we also often create convenience options for ourselves, like the 'z' option to tar. but i take your point.
Tools do one thing (or one thing at a time) and do it well. flashrom is getting radically over-engineered.
Please rethink this.
flashrom -e -- erase flashrom -r -- read flashrom -w -- write flashrom -v -- verify
flashrom -any combination -- mistake
Otherwise, no matter how you set this up, you're going to violate somebody's principle of least surprise. This thread already shows how.
i gues i didn't see that much controversy here. the only real surprise i've seen was mine, first at having my chip cleared instead of programmed :-), and then when folks pointed out flashrom's erase-by-default behavior (i.e., erase even without -E). but try as i might, i can't imagine anyone wanting to _regularly_ perform the steps of erase, write, and verify in any other order than that one.
if the consensus is that flashrom do only one thing at a time, that's fine by me. the current behavior is wrong, and needs to be fixed, somehow.
paul =--------------------- paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 55.4 degrees)
On 11.06.2009 22:04, ron minnich wrote:
Boy, this discussion is getting really complicated. That's a sign that it is time to STOP! and think it all over :-)
Indeed. I think we even have a very old trac issue for this.
Tools do one thing (or one thing at a time) and do it well. flashrom is getting radically over-engineered.
Please rethink this.
flashrom -e -- erase flashrom -r -- read flashrom -w -- write flashrom -v -- verify
flashrom -any combination -- mistake
I really like this. May I humbly request we auto-verify both erase and write operations to give the user feedback if an operation failed. Right now it can happen that erase or write fails and the user it not told about it.
Otherwise, no matter how you set this up, you're going to violate somebody's principle of least surprise. This thread already shows how.
True. Except for speed reasons, there shouldn't be any opposition to auto-verify. (For disabling auto-verify, a separate option could be added.)
Basically, we'd keep --verify as standalone operation, but it would be invoked automatically for erase and write.
Thoughts? I have a patch pending for this. I'll clean it up and send.
Regards, Carl-Daniel
I'm fine with auto-verify, it makes sense and it's cheap.
If there is still concern about corrupting the data somehow as part of the programming process then mmap the data read-only and the concern goes away.
ron
I really like this. May I humbly request we auto-verify both erase and write operations to give the user feedback if an operation failed. Right now it can happen that erase or write fails and the user it not told about it.
Currently - jedec.c does this atleast - if different write functions do different things, that should be fixed (make them all do the same) - again @work and dont have the time to check, but: write_jedec: * Erases * Verifies erase * Writes each page, verifying each page after writing ... then you do a -v after that and youll end up near-triple-verifying stuff -- ok it's nice to be sure, but that's not very speedy, but ok for on-motherboard flashing.
I suggest that we have it like this: -e erases and verifies erase -w erases, writes (verifying each page...) and verifies write (as in with -wv now) -r reads -v verifies
Then i suggest we have some flag to disable all verifies (including the one in jedec.c checking each page write) in case one is using a slow external programmer. -n maybe (as in --no-verify)?
Urja Rannikko wrote:
- Verifies erase
- Writes each page, verifying each page after writing
When using an external programmer verifying after each page will increase your programming time considerably due to the turn around time of the read. On our USB 4232H based programmer each extra separate USB operation costs you about 1ms due to various bits of setup overhead.
On the 8Mbit OLPC part with 256 byte pages that 4096 pages. So 4 about seconds for each additional USB transaction.
On 12.06.2009 17:57, Richard Smith wrote:
Urja Rannikko wrote:
- Verifies erase
- Writes each page, verifying each page after writing
When using an external programmer verifying after each page will increase your programming time considerably due to the turn around time of the read. On our USB 4232H based programmer each extra separate USB operation costs you about 1ms due to various bits of setup overhead.
On the 8Mbit OLPC part with 256 byte pages that 4096 pages. So 4 about seconds for each additional USB transaction.
The idea is to check if the erase worked after each erase operation. Basically, if you have an eraseblock size of 64 kByte, there should be exactly one 64 kByte read after erasing. I have not yet seen SPI chips with 256 Byte eraseblock size, but if you did, please tell me the part number so I can take a look at the data sheet.
For writes, this is a bit different since writing is more complicated than erasing. There are SPI chips which only write one byte at a time... that also means we have 1 M write transactions alone for an 8 Mbit chip. Since recovering from a failed write is not as simple as writing again (you may have to erase the chip), performing a full-chip verify after a full-chip write would address your latency concerns and still be a sensible solution AFAICS.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On the 8Mbit OLPC part with 256 byte pages that 4096 pages. So 4 about seconds for each additional USB transaction.
The idea is to check if the erase worked after each erase operation. Basically, if you have an eraseblock size of 64 kByte, there should be exactly one 64 kByte read after erasing. I have not yet seen SPI chips with 256 Byte eraseblock size, but if you did, please tell me the part number so I can take a look at the data sheet.
Right. I wasn't talking about erase. Just verifying after each page write vs writing the entire image and then re-reading the part in one stream read.
While on the subject of erase though. I find thats its much nicer to use block erase rather than full chip erase although perhaps a bit slower.
The (very OLPC specific) advantage is that if I just update the EC code then only the 1st 64k block is erased/written.
The OLPC firmware reflash also does things in a block erase fashion so that if things go wrong (power loss) there's the highest chance of a system that might still boot.
If you chip erase and you have some sort of bad power event half-way you are dead since your reset vector is all 0xff but the system that does it a sector at a time would probably still boot enough to allow you to reflash.
On 12.06.2009 18:42, Richard Smith wrote:
Carl-Daniel Hailfinger wrote:
The idea is to check if the erase worked after each erase operation.
Right. I wasn't talking about erase. Just verifying after each page write vs writing the entire image and then re-reading the part in one stream read.
Fully agreed. Full write, then full read is what the current verify code does.
While on the subject of erase though. I find thats its much nicer to use block erase rather than full chip erase although perhaps a bit slower.
Indeed. I have patches pending which switch to block erase. I hope to update and repost them soon.
Block erase (and block write) is on the todo list. Partial read support has already been posted and is waiting for an ack.
Regards, Carl-Daniel
On Fri, Jun 12, 2009 at 12:52:41PM +0200, Carl-Daniel Hailfinger wrote:
flashrom -e -- erase flashrom -r -- read flashrom -w -- write flashrom -v -- verify
flashrom -any combination -- mistake
I really like this. May I humbly request we auto-verify both erase and write operations to give the user feedback if an operation failed. Right now it can happen that erase or write fails and the user it not told about it.
Yes, please, that sounds sensible to me. Auto-verify is great. Also for read, or does that not make much sense?
Otherwise, no matter how you set this up, you're going to violate somebody's principle of least surprise. This thread already shows how.
True. Except for speed reasons, there shouldn't be any opposition to auto-verify. (For disabling auto-verify, a separate option could be added.)
Sounds perfect with that extra option to disable auto-verify.
Basically, we'd keep --verify as standalone operation, but it would be invoked automatically for erase and write.
Yes.
Thoughts? I have a patch pending for this. I'll clean it up and send.
Please!
Thanks, Ward.
On 12.06.2009 12:52, Carl-Daniel Hailfinger wrote:
On 11.06.2009 22:04, ron minnich wrote:
Tools do one thing (or one thing at a time) and do it well. flashrom is getting radically over-engineered.
Please rethink this.
flashrom -e -- erase flashrom -r -- read flashrom -w -- write flashrom -v -- verify
flashrom -any combination -- mistake
I have a patch pending for this.
Sent, subject is [coreboot] [PATCH] flashrom: Support only one operation at a time
Regards, Carl-Daniel
> > Right now -wv does that. I've actually never used -E.
does flashrom's write do an erase beforehand?
Atleast in jedec.c write_jedec's first code line (after variables) is erase_chip_jedec, so yes for that - i dont care to check them all but i assume all write functions erase before writing.
-
Carl-Daniel Hailfinger -
Myles Watson -
Paul Fox -
Richard Smith -
ron minnich -
Urja Rannikko -
Ward Vandewege