Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Sergii Dmytruk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59072 )
Change subject: dummyflasher: add SR2 emulation harness ......................................................................
Patch Set 17:
(11 comments)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/59072/comment/2456ce6a_ffa6b39e PS16, Line 42: int emu_features; /* combination of FEATURE_* constants */
It seems a little odd to have this while most of the code doesn't make […]
I thought it might be extended in the future to handle more `FEATURE_*` flags so emulation would match chip definitions more closely, but I guess that can be done when there will be multiple flags like that.
https://review.coreboot.org/c/flashrom/+/59072/comment/e4f201ca_c55852cd PS16, Line 167: return (status_reg == STATUS1 ? SPI_SR_WEL | SPI_SR_WIP : 0);
Nit, no parentheses needed.
Done.
https://review.coreboot.org/c/flashrom/+/59072/comment/72b8c300_725311c9 PS16, Line 168: }
This seems very chip specific.
This applies to all of the emulated chips, later commits add chip-specific things.
https://review.coreboot.org/c/flashrom/+/59072/comment/60b08b94_9d437469 PS16, Line 335: wrsr_ext = (writecnt == 3 && (data->emu_features & FEATURE_WRSR_EXT));
In this case, I think the additional parentheses make it actually harder […]
Done
https://review.coreboot.org/c/flashrom/+/59072/comment/9c02079c_cef805b6 PS16, Line 338: ro_bits = get_status_ro_bit_mask(STATUS1);
Looks like this changes behavior, i.e. fixes that the WEL could be […]
It doesn't, WEL bit is reset at the bottom of the function. Marking it here as RO just makes its read-only status more explicit.
https://review.coreboot.org/c/flashrom/+/59072/comment/c86f75c9_bbc38b3c PS16, Line 339: data->emu_status[0] &= ro_bits;
This also changes behavior, I guess WIP is not accidentally cleared anymore?.
WIP was RO before, old code is:
data->emu_status = writearr[1] & ~SPI_SR_WIP;
https://review.coreboot.org/c/flashrom/+/59072/comment/50def676_6c486ad2 PS16, Line 340: data->emu_status[0] |= (writearr[1] & ~ro_bits);
Nit, no parentheses needed.
Done
https://review.coreboot.org/c/flashrom/+/59072/comment/670272cf_a45477d4 PS16, Line 347: *
No dangling asterisk, please.
Done
https://review.coreboot.org/c/flashrom/+/59072/comment/5f725598_b9fb61be PS16, Line 347: other chips might differ At least datasheet for W25Q128FV claims otherwise in its note on previous generations:
If /CS is driven high after the eighth clock, the Write Status Register-1 (01h) instruction will only program the Status Register-1, the Status Register-2 will not be affected (Previous generations will clear CMP and QE bits).
https://review.coreboot.org/c/flashrom/+/59072/comment/35c1c69a_bfb4daf0 PS16, Line 364: data->emu_status[1] |= (writearr[1] & ~ro_bits);
Should we add a debug message like above?
Done
https://review.coreboot.org/c/flashrom/+/59072/comment/0a7263bc_5a4e031e PS16, Line 970: 32
Why 32? generally, it seems odd to give an exact number when no exact […]
Done