Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Nikolai Artemiev, Stefan Reinauer.
Anton Samsonov has posted comments on this change by Anton Samsonov. ( https://review.coreboot.org/c/flashrom/+/85585?usp=email )
Change subject: flashchips: Add Spansion S25FS512S ......................................................................
Patch Set 2:
(3 comments)
File flashchips/spansion.c:
https://review.coreboot.org/c/flashrom/+/85585/comment/d5421685_7b7138ba?usp... : PS2, Line 1043: /* Note on FEATURE_4BA_ENTER: the command set only defines command 0xB7 : to enter 4BA mode, but there is no counterpart command "Exit 4BA mode", : and the code 0xE9 is assigned to another command ("Password unlock"). */ : /* Note on FEATURE_4BA_ENTER_EAR7 (not set): the "Extended address mode" bit : is stored in configuration register CR2V[7], which can only be read / written : by generic commands ("Read any register" / "Write any register"), that itself : expect a 3- or 4-byte address of register in question, which in turn depends on : the same register, that is initially set from non-volatile register CR2NV[7] : that defaults to 0, but can be programmed to 1 for starting in 32-bit mode. */
Thank you for sharing bits of wisdom in the comment!
That is not a "wisdom", but merely observations of trial and error in finding proper combination. Without `FEATURE_4BA_ENTER`, this chip behaves very strange even when the sector 0 or any other sector <16M is addressed: it reports that the erase operation succeeded, when in fact most bits were left intact and other bits turned volatile — i. e. having random state (0 or 1) on each read.
Such an unreliable operation led me to Evaluate Erase Status command (code D0h, see section 9.6.4 on page 113 of the datasheet), which looked very promising as an almost-instant built-in implementation of `is_sector_blank()` functionality that abolish data transmission from chip to host for erase verification. I implemented that in s25f.c, also generalizing `s25fl_sector_erase()` into `s25f_sector_erase()` with adaptive timeouts and automatic retry of failed erases. However, it turned out that this command always returns "OK" even when no erase was attempted at all.
After finding out that simply adding `FEATURE_4BA_ENTER` "just works" even with generic `spi_block_erase_dc()`, I did not went further the "proprietary" way and did not bothered with adding Blank Check During Erase functionality (CR3V[5], see section 7.5.5.2 on page 62 of the datasheet). If someone is eager to have a look at their description and perhaps give an insight on what am I missing in understanding their intended behavior and/or proper use pattern, I can upload my current implementation as a separate "WiP" patch.
File include/flashchips.h:
https://review.coreboot.org/c/flashrom/+/85585/comment/c6526d23_9d59c80c?usp... : PS2, Line 717: 0x02200081
03h -> ?? where it is?
It is not a part of extended Device ID, but rather a header field indicating the total ID-CFI legacy structure size and the offset where the alternate structure starts (that is, 4Dh in this field means that the legacy part ends at byte 50h, and the alternate part starts at byte 51h). While it is essential for parsing the entire structure, it is obviously not a part of Device ID, thus `probe_spi_big_spansion()` simply concatenates bytes 01–02 and 04–05 to form a 32-bit identifier.
The exact chip name, for example "S25FS512S", is stored in parameter 0 of alternate structure (see Table 62 on page 140) starting at offset 02h there; and even the minor part number designation at offset 10h–11h — same as bytes at offset 06h–07h in the legacy Device ID structure. However, using such a string as identifier would require up to 14 bytes, not counting the minor designation, which only `uint128_t` can accommodate. So a hashing algorithm may be necessary to keep 32-bit compatible identifiers, such as "compute `sha256(string)` and take the first 4 bytes as a big-endian number".
https://review.coreboot.org/c/flashrom/+/85585/comment/9d60c1a6_2abbcd92?usp... : PS2, Line 717: SPANSION_S25FS512S
`SPANSION_S25FS512S_UL`
To tell the truth, I was tearing apart between `S25FS512S` and just `S25FS512` (without the trailing "S"), as `S25FL512` already uses that naming scheme, and because the datasheet defines no major variations for 512-Mbit model.
If you vote for the `_UL` suffix, then should `S25FL512` also be changed to `S25FL512S_UL` simultaneously, since this patch alters that macro anyway?
-
Anton Samsonov (Code Review)