Change in flashrom[master]: Add support for Atmel/Adesto AT25SF161 and Winbond W25Q80EW.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/25099 )
Change subject: Add support for Atmel/Adesto AT25SF161 and Winbond W25Q80EW. ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/#/c/25099/1/flashchips.c File flashchips.c:
https://review.coreboot.org/#/c/25099/1/flashchips.c@1531 PS1, Line 1531: .feature_bits = FEATURE_WRSR_WREN,
Can't find OTP in datasheet.
The comment is inaccurate, but there is OTP capability via security registers that can be permanently locked.
Is it accurate to say these are OTP registers in the sense that flashrom knows of them? I'm not really an expert on this aspect as I haven't used OTP bits, so perhaps my intuition is incorrect.
What I am asking is: Should the comment change should FEATURE_OTP be removed for this chip?
https://review.coreboot.org/#/c/25099/1/flashchips.c@1532 PS1, Line 1532:
Just noticed the notion of EWSR as "Write Enable for Volatile […]
IIRC Spansion S25FL chips (or maybe FS?) had a similar feature where one could change a volatile register rather than non-volatile.
Whether it suits flashrom better depends on the use case. We should probably expose this on the CLI when we get around to adding the writeprotect stuff to upstream.
I think that for the common case in flashrom we will want the non-volatile registers to be updated. For things like BIOS security it's important that write-protection is activated by hardware once /WP is asserted. Otherwise we need to wait for the a master to issue a command to write volatile status bits, and that may leave some attack surface open.
https://review.coreboot.org/#/c/25099/1/flashchips.c@1555 PS1, Line 1555: .unlock = spi_disable_blockprotect_at2x_global_unprotect, : .write = spi_chip_write_256,
SF family uses very different status register layout
fixed (made generic)
https://review.coreboot.org/#/c/25099/1/flashchips.c@1559 PS1, Line 1559:
can't find 2. […]
fixed (copypasta)
https://review.coreboot.org/#/c/25099/1/flashchips.c@1878 PS1, Line 1878: .tested = TEST_UNTESTED, Not sure if this was intentional, so reverted for now.
https://review.coreboot.org/#/c/25099/1/flashchips.c@15075 PS1, Line 15075: _bit
3*256B ?
Done
https://review.coreboot.org/#/c/25099/1/flashchips.c@15103 PS1, Line 15103:
- […]
True, but other chips in this series have it wrong as well :-/ Not sure if it's better to be correct or consistent.
I went ahead and corrected this one.
-
David Hendricks (Code Review)