[M] Change in flashrom[master]: ichspi.c: Read chip ID and use it to populate `flash->chip`
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69195 )
Change subject: ichspi.c: Read chip ID and use it to populate `flash->chip` ......................................................................
Patch Set 3:
(12 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/flashrom/+/69195/comment/6a35c849_e0cabe29 PS2, Line 7: ichspi.c: Read chip ID and use it to identify chip
Knowing the chip shouldn't matter for hwseq, as we're not sending direct commands anyway. […]
Two flash chips is an interesting case, I think it will always read the ID of the first chip but I don't have any hardware to test with.
https://review.coreboot.org/c/flashrom/+/69195/comment/d353879c_5a095212 PS2, Line 9: BUG=b:253715389,b:253713774
Ah, so downstream has some extra functionality, probably to control WP on platforms where only hwseq […]
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/e1f58459_2e107666 PS2, Line 11: TEST=builds
I think you can include this was actually tested here across the DUT pool?
Done
Commit Message:
https://review.coreboot.org/c/flashrom/+/69195/comment/471a1d1c_65f04e34 PS3, Line 21: FIX: w,E Something about this change causes erase to fail:
``` Found Winbond flash chip "W25Q128.V..M" (16384 kB, Programmer-specific) on internal. Erasing and writing flash chip... Transaction error between offset 0x00000000 and 0x00000000 (= 0x00000000 + 0)! Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. Looking for another erase function. No usable erase functions left. FAILED! ```
Looks like it's trying to do SPI erase instead of hwseq now. We don't have this issue in cros flashrom so there's probably some additional logic that makes it work, maybe action_descriptor since it mutates erasers a bit. I'll keep debugging.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/69195/comment/b9ad68a8_da0a28ee PS2, Line 1472: entry == NULL
`!entry`
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/bb65953b_71300045 PS2, Line 1475: return 0;
`return -1;`
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/6efbaad6_c65cce11 PS2, Line 1476: } else {
no need for a `else` as you have a early return in the `if` branch.
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/ed6ebbd5_6161eede PS2, Line 1486: flash->chip->tested = entry->tested;
drop line, it is overwritten in the eulogy of `ich_hwseq_probe()`, the call site of this function.
Wouldn't it be better to keep this and delete the line that overwrites it?
That's what I've done in the latest patchset but let me know if I'm missing something.
https://review.coreboot.org/c/flashrom/+/69195/comment/e36e1217_fa5f9d57 PS2, Line 1492: return 1;
`return 0;`
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/36f44c0c_0c4b8fcc PS2, Line 1502: != 1
`< 0`
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/09d8b060_d1e3cda4 PS2, Line 1503: msg_perr("Unable to read flash chip ID\n");
drop as you already have a `msg_perr()` call in the func.
Done
https://review.coreboot.org/c/flashrom/+/69195/comment/4df269a8_f21d7b0e PS2, Line 1552: TEST_OK_PREW;
Done
-
Nikolai Artemiev (Code Review)