Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40325 )
Change subject: Add writeprotect support infrastructure ......................................................................
Patch Set 8:
(5 comments)
Patch Set 7: Code-Review-1
Uhm, looks like something isn't right...
Found Winbond flash chip "W25Q64.V" (8192 kB, SPI) mapped at physical address 0x00000000ff800000. No data in flash context! Error: write protect is not supported on this flash chip.
I have not seen that before however lets deal with it in the winbond support patch as a follow up as there is a fair amount of work here to get this split up and cleaned up. Thanks for the report though I will try to EM100 locally to repro the issue
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.h File writeprotect.h:
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.h@44 PS7, Line 44: wp_wpce775x
Do we have this programmer upstream?
It's coming once I have it cleaned up locally, CrOS code quality sucks.
I removed it out of this commit and made it into a much simpler commit to deal with the writeprotect functionality scope.
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@873 PS7, Line 873: uint8_t mx25l_read_config_register(const struct flashctx *flash);//XXX
Hmm?
poor cros code quality, was required to make it compile upstream. Removed from commit for now.
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@1132 PS7, Line 1132: //case ATMEL_AT25SF128A:
Hm?
Removed for now.
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@1473 PS7, Line 1473: /* FIXME: this is NOT endian-free copy. */
How about providing a union instead of a struct? Here's an example: https://github. […]
I have moved the winbond support into a follow up commit to deal with it separately from the wp infrastructure.
https://review.coreboot.org/c/flashrom/+/40325/7/writeprotect.c@2236 PS7, Line 2236: //case SPANSION_ID: : // switch (flash->chip->model_id) { : // case SPANSION_S25FS128S_L: : // case SPANSION_S25FS128S_S: { : // *wp = &s25fs128s_wp; : // (*wp)->descrs = s25fs128s_ranges; : // *num_entries = ARRAY_SIZE(s25fs128s_ranges); : // break; : // } : // case SPANSION_S25FL256S_UL: : // case SPANSION_S25FL256S_US: { : // *wp = &s25fl256s_wp; : // (*wp)->descrs = s25fl256s_ranges; : // *num_entries = ARRAY_SIZE(s25fl256s_ranges); : // break; : // } : // default: : // msg_cerr("%s():%d Spansion flash chip mismatch (0x%04x)" : // ", aborting\n", __func__, __LINE__, : // flash->chip->model_id); : // return -1; : // } : // break;
Um?
Missing chips upstream, removed for now.