Support for Winbond 512 mbit and 1 gbit flash chips (IDFGH-7854)

[johnboiles]

Currently the flash detection logic doesn't work for Winbond chips 512 megabit and above. The bitwise math assumption that uses the last byte of the ID seems to work fine for all smaller chips but for some unknown reason there's not a clean increment of that value from 256 megabit (which has a last byte of 0x19) to 512 megabit (which has a last byte of 0x20). Seems possible this was a design mistake on Winbond's part that they now can't undo.

LittleFS has a handy list of Winbond IDs and sizes: https://github.com/PaulStoffregen/LittleFS/blob/d5de4cf8d1587aa8b2433962ea995d45a991b918/src/LittleFS.cpp#L46

This PR adds a special case for known Winbond 512 megabit and 1 gigabit flash chips mentioned in the above list. I've personally tested this against the W25Q512JVEQ chip on an ESP32-S3 and it resolves a bootloader failure that looks like this:

I (712) spi_flash: detected chip: winbond
I (716) spi_flash: flash io: dio
E (720) spi_flash: Detected size(0k) smaller than the size in the binary image header(16384k). Probe failed.
E (761) esp_core_dump_flash: Core dump flash config is corrupted! CRC=0x7bd5c66f instead of 0x0

This makes sense, because 1 << 0x20 == 4294967296 which, for a uint32_t like size would be the same as 0.

[Alvin1Zhang]

Thanks for your contribution.

[johnboiles]

You're welcome! I'm eager to get support for these chips in IDF!

[igrr]

@johnboiles Do you know if this logic for 512M/1Gbit chips is common for other vendors?

If this is Winbond-specific logic, then it could be a better option to define a Winbond-specific spi_flash_chip_winbond_detect_size function in spi_flash_chip_winbond.c and replace the .detect_size function pointer with the Winbond-specific version:

esp-idf/components/spi_flash/spi_flash_chip_winbond.c

Line 169 in 21b63d0

.detect_size = spi_flash_chip_generic_detect_size,

[johnboiles]

I don't know! But I doubt it's common, seems like a mistake. I'll move this logic to a Winbond specific method. Good idea

[igrr]

Having checked the GD25Q512 datasheet, the ID is 0xc84020. So at least it's common between GD and Winbond.

[johnboiles]

Is that enough reason to keep this logic in the common method you think?

[KonssnoK]

Hello, if i may add a suggestion, i would remove magic numbers in favor of defines :)
Like, what is 0x4020 ?

thanks!

[johnboiles]

Agree that it's worth killing magic numbers in general. What do you think this should be named? COMMON_FLASH_PRODUCT_ID_512MBIT perhaps?

Reading the IDF 4.4.2 release notes I came across #7688 and it's a bit of a mystery to me how they were able to test the 512mbit and 1024mbit Winbond chips without this fix.

[KonssnoK]

I guess they didn't :)
"as soon we can test 256MB, 512MB and the 1024MB i update with a new Pull Request"
(also there seems to be some confusion between bit and bytes, i guess those are bits, since the maximum size offered by Winbond is 4Gbit -> 512MB 🤔 )
W25N04KV

I would take other defines structure.
SPI_FLASH_GENERIC_PAGE_PROGRAM_TIMEOUT_MS
So maybe
SPI_FLASH_GENERIC_ID_512MBIT ? In the end strict naming is not important if it's clear what is it about and where it belongs

also,
@johnboiles could you take a look at
#9503
?
Thanks!

[KonssnoK]

Having checked the GD25Q512 datasheet, the ID is 0xc84020. So at least it's common between GD and Winbond.

Winbond W25N01GV is 1Gbit and has AA21 as identifier, so this change won't work for that.

EDIT:

0x15 = 16Mbit
0x16 = 32Mbit
0x17 = 64Mbit
0x18 = 128Mbit
0x19 = 256Mbit
0x20 = 512Mbit
0x21 = 1Gbit

I guess they didn't count in hex but in decimal...

[KonssnoK]

@igrr
Please check my changes in the above referenced PR
(substitutes this one)

[johnboiles]

Micron also looks like it uses the same weird convention and it extends to 2gbit:

@KonssnoK your alternate PR (#9566) looks good to me. Let's run with that instead.

[lorenzopeluso]

Just for information and confirmation, I tried with Winbond W25Q02JVTBIM with 2Mb of capacity. The proposed patch does work!

Need just a other modification:

if ((id & 0xFFFF) == 0x4020 || (id & 0xFFFF) == 0x7020) {
        *size = 0x4000000;
    } else if ((id & 0xFFFF) == 0x4021) {
        *size = 0x8000000;
    } else if ((id & 0xFFFF) == 0x7022) {
        *size = 0x10000000;
    } else {
        *size = 1 << (id & 0xFF);
    }

in order to add the ID (from datasheet) 0x7022 and to assign the right capacity of 262144 KB.

The result is shown in the attached image.

Be careful.....the format takes about 16 minutes. :).

Thanks a lot.
Cheers,
Lorenzo.

[KonssnoK]

@lorenzopeluso please try #9566
it should work without changes