Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34413 )
Change subject: flashchips: Fill out support for N25Q/MT25Q chips ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/flashrom/+/34413/2/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/34413/2/flashchips.c@10608 PS2, Line 10608: }, {
My understanding here is that the newer MT25 series supports the 32K subsector erase, but the older […]
We handle such differences with multiple chip definitions that share the ID. Flashrom supports that and would query the user to decide which chip it is. See for instance Macronix definitions (e.g. MX25L6405), they very often recycle IDs.
So you would have to add two entries, one for "N25Q512..1E" and one for "MT25QU512".
https://review.coreboot.org/c/flashrom/+/34413/2/flashchips.c@10616 PS2, Line 10616: .block_erase = spi_block_erase_c7,
I'm new to this codebase so I don't know the best way to address this issue. Probably the safest thing to do short term is remove the bulk erase command altogether for the 512Mb device.
I think the chip definitions should be as comprehensive as possible (within flashrom's code limits).
In practice, I've found that erasing a sector at a time is only marginally slower than issuing a bulk erase. Thoughts?
Actually, flashrom tries the list of erase functions from top to bottom anyway. It would only choose the bulk erase if the programmer situation doesn't allow any preceding function. The whole logic is not very smart and should change in the future. Hence, the chip definitions should be free from any assumptions.
Also, to be clear, this bug affects the pre-existing N25Q512..3E in addition to the N25Q512..1E that I am adding.
That should be addressed in a separate commit. Let me know if you want to do so, otherwise I'll take it.