Re: [coreboot] [PATCH] flashrom: Check if erase worked, check return codes

[Uwe Hermann]

On Mon, Jun 15, 2009 at 12:26:46PM +0200, Carl-Daniel Hailfinger wrote:
> I wasn't really awake while changing that code. Sorry. Same for the
> other bugs you found.
>
> Compared to your version, I changed the following:
> Check inside all erase_*_jedec routines if erase worked, not outside.
> Rename rv to ret. Most functions in flashrom call the return variable ret.
> erase_sector_jedec and erase_block_jedec have changed prototypes to
>
> enable erase checking.
> verify_range uses goto out_free to make sure we don't forget to free().
> Convert all remaining erase functions and actually check return codes
>
> almost everywhere.
>
> Urja, would you remind reviewing the whole patch again? At 1087 lines of
> manual conversions, it is too big for me to be 100% confident that I got
> everything right.
>
> Signed-off-by: Carl-Daniel Hailfinger

With the two bugfixes discussed on IRC this is:

Acked-by: Uwe Hermann

I successfully tested LPC on an CK804 box and SPI on some sb600 box.
> +/* start is an offset to the base address of the flash chip */

Please start sentences with capital letter and end them with full stop
for consistency.
> +/* start is an offset to the base address of the flash chip */

Ditto.
> @@ -146,26 +156,41 @@
>    printf("total_size/page_size = %d\n", total_size / page_size);
>    for (i = 0; i < (total_size / page_size) - 1; i++) {
>    	printf("%04d at address: 0x%08x\n", i, i * page_size);
> block_erase_m29f400bt(bios, bios + i * page_size);
> if (block_erase_m29f400bt(flash, i * page_size, page_size)) {
> 	fprintf(stderr, "ERASE FAILED!\n");
> 	return -1;
> }
>
> write_page_m29f400bt(bios, buf + i * page_size,
> 		     bios + i * page_size, page_size);
> printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
> }
>
> printf("%04d at address: 0x%08x\n", 7, 0x70000);
> block_erase_m29f400bt(bios, bios + 0x70000);
>
> if (block_erase_m29f400bt(flash, 0x70000, 32 * 1024)) {
> fprintf(stderr, "ERASE FAILED!\n");
> return -1;
>
> }
> write_page_m29f400bt(bios, buf + 0x70000, bios + 0x70000, 32 * 1024);
>
> printf("%04d at address: 0x%08x\n", 8, 0x78000);
> block_erase_m29f400bt(bios, bios + 0x78000);
>
> if (block_erase_m29f400bt(flash, 0x78000, 8 * 1024)) {
> fprintf(stderr, "ERASE FAILED!\n");
> return -1;
>
> }
> write_page_m29f400bt(bios, buf + 0x78000, bios + 0x78000, 8 * 1024);
>
> printf("%04d at address: 0x%08x\n", 9, 0x7a000);
> block_erase_m29f400bt(bios, bios + 0x7a000);
>
> if (block_erase_m29f400bt(flash, 0x7a000, 8 * 1024)) {
> fprintf(stderr, "ERASE FAILED!\n");
> return -1;
>
> }
> write_page_m29f400bt(bios, buf + 0x7a000, bios + 0x7a000, 8 * 1024);
>
> printf("%04d at address: 0x%08x\n", 10, 0x7c000);
> block_erase_m29f400bt(bios, bios + 0x7c000);
> if (block_erase_m29f400bt(flash, 0x7c000, 16 * 1024)) {
> fprintf(stderr, "ERASE FAILED!\n");
> return -1;
> }

This code need some refactoring IMHO, we can easily make an array
of block sizes and loop over that (that's for another patch though).

Uwe.