initial SPI flash #9

antmak · 2025-04-27T05:22:05Z

(Proposes the design for discussion.)

Adds initial structure for SPI Flash support, no read, no cache, no mmu
Adds getting flash-id and flash-size info
Currently adds chips: ESP32, S2, C3, S3, H2, C6, C2, C5, P4, C61
(Adding the read/write in next PRs)
(Bringing it in line with the esp_flash driver of the new chips' ROM - in the following PRs)

github-actions · 2025-04-27T05:22:39Z

	Messages
📖	You might consider squashing your 3 commits (simplifying branch history).
📖	This PR seems to be quite large (total lines of code: 20674), you might consider splitting it into smaller PRs

👋 Hello antmak, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 96d57d4

igrr · 2025-04-28T19:35:22Z

Just a comment regarding SPI flash functions: at least on newer chips (S3/C3 and later), I would recommend calling the newer esp_flash driver instead of the legacy ROM functions. There are several advantages:

The new driver supports adding extension to more Flash models. This means that we can add support of a new brand of Flash in the stub, while still relying on the ROM driver for most of the code.
The new driver code is mostly the same as in IDF, which makes it easier for developers without access to the ROM code to understand and debug.
The naming conventions are saner, IMO.

Depending on the specific chip, esp_flash ROM driver requires some patches, but it's easy to find what needs to be patched by looking at the ROM patches in esp-idf.

For older chips, perhaps bundling the entire esp_flash driver into the stub is not such a bad option, need to check the code size...

include/private/rom.h

src/flash.c

src/esp32/src/flash.c

erhankur · 2025-05-07T12:26:10Z

src/esp32/src/flash.c

+    uint32_t strapping = REG_READ(GPIO_STRAP_REG);
+    /*  If GPIO1 (U0TXD) is pulled low and flash pin configuration is not set in efuse, assume
+     * HSPI flash mode (same as normal boot) */
+    if (spiconfig == 0 && (strapping & 0x1c) == 0x08) {


I know we are copying this lines somewhere else, but it would be nice to know what is this magic numbers meaning? 0x1c, 0x8.

Yep, strap stuff is something that will still need putting order. It was just moved from the original stub

updated in target_flash.c, please let me know if it is not clear

src/esp32/src/flash.c

src/esp32c3/src/flash.c

gerekon · 2025-05-13T09:25:41Z

Just a comment regarding SPI flash functions: at least on newer chips (S3/C3 and later), I would recommend calling the newer esp_flash driver instead of the legacy ROM functions. There are several advantages:

The new driver supports adding extension to more Flash models. This means that we can add support of a new brand of Flash in the stub, while still relying on the ROM driver for most of the code.

The new driver code is mostly the same as in IDF, which makes it easier for developers without access to the ROM code to understand and debug.

The naming conventions are saner, IMO.

Depending on the specific chip, esp_flash ROM driver requires some patches, but it's easy to find what needs to be patched by looking at the ROM patches in esp-idf.

For older chips, perhaps bundling the entire esp_flash driver into the stub is not such a bad option, need to check the code size...

Looks like a good idea. As far as I see implementation of https://github.com/espressif/openocd-esp32/blob/master/contrib/loaders/flash/espressif/esp32c3/stub_flasher_chip.c#L346 is almost equal to esp_flash_read_encrypted. So

So for chips which have esp_flash driver in ROM we save code/data space and make support for new flash chips easier.
For older chips we will have to implement the same functionality in any case.

To simplify development I'd implement base simple flash functionality (read, write, erase, w/o encryption) with ROM functions at first. After we have it working and all related tests passed we can try to switch to esp_flash driver. Base API for the driver and ROM functions are very similar:

// ROM funcs
esp_rom_spiflash_result_t esp_rom_spiflash_erase_chip(void);
esp_rom_spiflash_result_t esp_rom_spiflash_erase_area(uint32_t start_addr, uint32_t area_len);
esp_rom_spiflash_result_t esp_rom_spiflash_read(uint32_t src_addr, uint32_t *dest, int32_t len);
esp_rom_spiflash_result_t esp_rom_spiflash_write(uint32_t dest_addr, const uint32_t *src, int32_t len);

// flash driver
esp_err_t esp_flash_erase_chip(esp_flash_t *chip);
esp_err_t esp_flash_erase_region(esp_flash_t *chip, uint32_t start, uint32_t len);
esp_err_t esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t address, uint32_t length);
esp_err_t esp_flash_write(esp_flash_t *chip, const void *buffer, uint32_t address, uint32_t length);

So migration at that point seems to be smooth. We can switch to the driver for ESP32 (the oldest chip) and evaluate difference in size. Of course, that will the diff for base API only.
Before that switch we will check esp_flash driver if there can be other potential disadvantages (besides code/data size) for using it in the stub.

@erhankur WDYT?

gerekon

I agree with @erhankur suggestion to implement functionality enough for OpenOCD command flash info (or special test command) for each target and print size, manufacturer id or something useful.

Also do cleanups and re-organize identical code to be reusable.

include/target/flash.h

src/esp32c3/include/soc/README.txt

gerekon · 2025-05-13T10:17:43Z

src/esp32/src/flash.c

+extern uint32_t ets_efuse_get_spiconfig(void);
+extern void esp_rom_spiflash_attach(uint32_t ishspi, bool legacy);
+extern esp_rom_spiflash_chip_t g_rom_flashchip;
+extern uint8_t g_rom_spiflash_dummy_len_plus[];


These definitions are used for all chips. So seems to be reasonable t move them to one header file like it is done in IDF.

https://github.com/espressif/esp-idf/blob/master/components/esp_rom/include/esp_rom_spiflash.h

https://github.com/espressif/esp-idf/blob/master/components/esp_rom/include/esp_rom_spiflash_defs.h

Made the common code a bit different way, ptal

src/esp32c6/include/soc/README.txt

gerekon · 2025-05-13T10:24:45Z

src/esp32c6/src/flash.c

+#define g_rom_flashchip (rom_spiflash_legacy_data->chip)
+#define g_rom_spiflash_dummy_len_plus (rom_spiflash_legacy_data->dummy_len_plus)
+
+__attribute__((unused)) static inline uint32_t device_id_from_spi_rdid()


We have the same implementation for C3. Can be moved to common file

__attribute__((unused)) static inline uint32_t device_id_from_spi_rdid() { WRITE_PERI_REG(SPI_MEM_W0_REG(1), 0); WRITE_PERI_REG(SPI_MEM_CMD_REG(1), SPI_MEM_FLASH_RDID); while (READ_PERI_REG(SPI_MEM_CMD_REG(1)) != 0) ; uint32_t rdid = READ_PERI_REG(SPI_MEM_W0_REG(1)) & 0xffffff; return ((rdid & 0xff) << 16) | (rdid & 0xff00) | ((rdid & 0xff0000) >> 16);; } __attribute__((unused)) static inline uint32_t device_id_from_rom() { // Sets the correct g_rom_flashchip.device_id from SPI_MEM_FLASH_RDID in ROM code esp_rom_spi_flash_update_id(); return g_rom_flashchip.device_id; }

yep, fixed it according to the design

src/esp32c6/src/flash.c

erhankur · 2025-05-13T11:21:50Z

esp_flash_read_encrypted

@gerekon As we discussed internally, it may be best to write esp_flash_xxx wrappers that call the legacy ROM functions first. Later, we can add support for new chips smoothly, without a large refactoring, when we switch from the legacy ROM calls to the esp_flash versions.

… stages code

antmak · 2025-05-22T12:20:19Z

src/esp32c2/include/target/soc_impl.h

+#define PERIPHS_SPI_FLASH_BITS_RDID         SPI_MEM_FLASH_RDID
+
+// If we have many defines here, we'll move the common code to common/soc_impl_xxx.h,
+// then include it here


TODO: comply the design, make a couple of common headers

erhankur · 2025-05-23T11:25:09Z

@antmak now this PR looks big enough to review carefully. How can we split it to smaller parts? For example;
1 - Add all necessary linker files in the first PR
2 - Add flash init for the legacy targets
3 - Add flash init for other targets

And I would like to discuss structure change reason in one PR. (Maybe in seperate one or in the PR you really need that changes)

antmak force-pushed the feat/spiflash branch 2 times, most recently from f2b9b85 to 09816f4 Compare April 30, 2025 09:48

erhankur mentioned this pull request May 5, 2025

Add traxmem helpers #12

Merged

antmak force-pushed the feat/spiflash branch 5 times, most recently from ac58159 to 887f7f0 Compare May 7, 2025 09:31

antmak mentioned this pull request May 7, 2025

refactor: move target interface to /include/target/ #14

Merged

antmak force-pushed the feat/spiflash branch from 887f7f0 to ea9c576 Compare May 7, 2025 09:45

antmak marked this pull request as ready for review May 7, 2025 09:53

antmak requested review from gerekon and erhankur May 7, 2025 09:54

erhankur reviewed May 7, 2025

View reviewed changes

antmak force-pushed the feat/spiflash branch 3 times, most recently from c67a222 to 6327072 Compare May 12, 2025 05:34

gerekon reviewed May 13, 2025

View reviewed changes

antmak marked this pull request as draft May 14, 2025 10:22

antmak force-pushed the feat/spiflash branch 4 times, most recently from 8efb577 to 6b45958 Compare May 22, 2025 10:01

antmak added 3 commits May 22, 2025 18:23

feat(spiflash): add initial structure for SPI Flash support and early…

4b2a49d

… stages code

change: reordering target uart code

6e50000

change: rename Device ID to Flash ID for better semantic

96d57d4

antmak force-pushed the feat/spiflash branch from 3ed8878 to 96d57d4 Compare May 22, 2025 11:24

antmak commented May 22, 2025

View reviewed changes

dobairoland requested a review from radimkarnis May 27, 2025 11:05

initial SPI flash #9

Are you sure you want to change the base?

initial SPI flash #9

Uh oh!

Conversation

antmak commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igrr commented Apr 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erhankur May 7, 2025

Choose a reason for hiding this comment

Uh oh!

antmak May 8, 2025

Choose a reason for hiding this comment

Uh oh!

antmak May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerekon commented May 13, 2025

Uh oh!

gerekon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gerekon May 13, 2025

Choose a reason for hiding this comment

Uh oh!

antmak May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gerekon May 13, 2025

Choose a reason for hiding this comment

Uh oh!

antmak May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erhankur commented May 13, 2025

Uh oh!

antmak May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erhankur commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

antmak commented Apr 27, 2025 •

edited

Loading

github-actions bot commented Apr 27, 2025 •

edited

Loading

antmak May 22, 2025 •

edited

Loading

antmak May 22, 2025 •

edited

Loading

erhankur commented May 23, 2025 •

edited

Loading