[flashrom] [PATCH/RFC] finish jedec converstion

[Carl-Daniel Hailfinger]

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 at 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

-- 
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."