On 23.01.2010 23:56, Sean Nelson wrote:
Complete the addition of Feature Bits for all Jedec based chips.
Add Zero entries for FWH, SPI and "Odd Jedec" (FEATURE_ADDR_SHIFTED) chips.
I think that part of the changelog can be killed.
Add FEATURE_SHORT_RESET fix feature_bits to 0 for SST49LF040B
rewrite jedec functions to use getaddrmask
convert write_49f002 to write_jedec_1 convert write_w39v040c to write_jedec_1 convert probe_w39v040c to probe_jedec convert write_49lf040 to write_jedec_1 convert write_pm29f002 to write_jedec convert write_29f040b to write_jedec_1 convert probe_29f040b to probe_jedec convert erase_chip_29f040b to erase_chip_block_jedec convert erase_sector_29f040b to erase_sector_jedec convert write_m29f002b to write_jedec convert write_m29f002t to write_jedec convert *_29f002 to *_jedec
decouple unused files from Makefile: am29f040b.c en29f002a.c m29f002.c mx29f002.c pm29f002.c sst49lf040.c w39v040c.c w49f002u.c
Very nice. I'd like to keep these files around until we have the locking conversion/refactoring done. My patch to do this probably doesn't apply anymore, and it also can't handle partial locking/unlocking. I can repost next week, but I welcome comments about the interface. One of my new ideas is to have the locking function take an struct lockblock {blocksize, lockstatus}array as parameter. To retrieve the locking status, one would pass action=get array=NULL and the function would allocate and return the lockblock array. To set the locking status, one would first retrieve the array (see previous sentence), then walk it and set the desired status of each lockblock, then pass action=set and the modified array to the locking function. Advantages: You can do lock printing in a generic function which just walks the returned array, you can handle enabling and disabling all locks in the same way. Even something like (un)locking only a specific region can be done with a generic wrapper. Unsolved (well, design is not completely ready): Where should we store lock blocks? In struct flashchip (works mostly OK for LPC/FWH chips because locking status is usually stored in register space at the corresponding block address, will be a nightmare for SPI chips because there are usually just 4 bits in the status reg for this, and undecided for Parallel chips because they have the status reg variant and the corresponding block address variant). In the chip drivers (would avoid cluttering flashchips.c, handle the various locking encodings (bitfield, corresponding address in register space)
Updated patch with suggestions and fixes from carldani and uwe.
Signed-off-by: Sean Nelson audiohacked@gmail.com
Close, but one more iteration please. You changed the write function for a lot of chips, but kept .tested=TEST_OK_PRW instead of changing it to TEST_OK_PR. Yes, I do realize that this means our test matrix will look a lot worse. Unfortunately, we can't do better if we want to stay honest.
--- a/jedec.c +++ b/jedec.c @@ -23,14 +23,15 @@ */
#include "flash.h"
#define MAX_REFLASH_TRIES 0x10 #define MASK_FULL 0xffff #define MASK_2AA 0x7ff +#define MASK_AAA 0xfff
Umm. Does this mean we have 555/2AA/555 and 555/AAA/555 chips?
int write_jedec(struct flashchip *flash, uint8_t *buf) {
- int mask;
- mask = getaddrmask(flash); int i, failed = 0; int total_size = flash->total_size * 1024; int page_size = flash->page_size;
I think this will cause compilation warnings/errors for some compilers because variable declarations and assignments are mixed freely. Can you reorder (move mask=getaddrmask after the declaration block)?
And one last comment: FEATURE_EITHER_RESET is not set for any chip, but a few chips now have FEATURE_SHORT_RESET. I think at least one of them should have FEATURE_EITHER_RESET instead. I would also introduce FEATURE_LONG_RESET (as alias to FEATURE_EITHER_RESET) and fill it in for all chips because you're going through the datasheets anyway.
Yes, I should have mentioned that in my first review, but I just had a few seconds to look over the patch and need to continue with finals preparation.
Note: I did _not_ verify the changes in flashchips.c due to time constraints, but the rest of the code and design looks solid and should work fine if the review is addressed.
Regards, Carl-Daniel
P.S. I need to write shorter sentences and especially, considering the amount of reading you, my dear fellow developers, have to keep up with, the nesting of sentences (and of course the inordinate number of parentheses (which sort of serve as subsitute for even more commas)). :-P