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: 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)

TODO:

ESP32

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

	Messages
📖	You might consider squashing your 4 commits (simplifying branch history).

👋 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 cd2f0b3

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.

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)

…C2, C5, P4, C61

erhankur · 2025-06-17T15:49:30Z

Could we keep each target’s flash_init in its own flash.c instead of adding a new target/impl layer?
The current impl functions just forward calls that the compiler will likely inline anyway, so the extra folder feels like one more place to look. Leaving them in the target files seems clearer. If we really need a shared file, how about putting it in src/common/ or include/private/flash_common.h?
You might not be aware of challenges we have in OpenOCD. It must handle two target states:
1. Immediately after reset halt. We have freedom in this state. We can reset everything (flash, cache, xtal freq etc..) And we don't need to restore the previous settings.
2. Running state of the target: When we halt the running target, flash/cache are already configured, and blindly resetting them (e.g. with esp_rom_spiflash_attach) can break the application when target resumes.
  The current stub detects the state and save/restore settings. This PR doesn’t include that logic which is fine. But we should add it soon as it will be easier to test.
Similar to above, we do some check for octal flash and PSRAM. PSRAM is especially critical: we must be sure its contents are no changed after calling flash/cache init functions, particularly esp_rom_spiflash_attach.
32MB Flash size boards are also important we need to test.
What’s the plan for switching to the new flash APIs Ivan mentioned? Could we avoid double work if we just implement legacy chips for now?

gerekon · 2025-06-18T16:01:02Z

Agree with @erhankur. We agreed to use esp_flash driver API.
For new chips it is mostly implemented in ROM, so for them you do not need to write low level SPI functions. E.g. to get flash size you can just call esp_flash_get_size. It is compeletly implemented in ROM starting from ESP32-S3.

So for those chips you do not need

    uint32_t flash_id = stub_target_flash_get_flash_id();
    uint32_t flash_size = flash_id_to_flash_size(flash_id);

instead you need

uint32_t flash_size = esp_flash_get_size();

For ESP32 and ESP32-S2 that function should be implemented in stub. For newer chips it is implemented in ROM.

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

As far as I understand @igrr suggestion above is to try to pull in the esp_flash driver from IDF to the stub. That driver implements API (e.g. esp_flash_get_size) for the cases when they are not in ROM (ESP32 and ESP32-S2):

As a first step in this PR I would pull that driver to some special sub-dir in the lib (maybe via git subtree or just copying) and call esp_flash_get_size and

stub_lib_err_t stub_lib_flash_init(void **state)
{
    esp_flash_t chip;
    uint32_t flash_size;
    
   STUB_LOG_TRACE();
    
    // WARNING: Calling `esp_flash_init` from IDF esp_flash driver as is 
   // when app is already running can be dangerous, so we may need to 
   // call special version of init function at runtime
    esp_err_t err = esp_flash_init(&chip)
    if (err != ESP_OK) {
        STUB_LOGE("Failed to init flash %d!\n", err);
        return STUB_LIB_FAIL;
    }
    err = esp_flash_get_size(&chip, &flash_size);
    if (err != ESP_OK) {
        STUB_LOGE("Failed to get flash size %d!\n", err);
        return STUB_LIB_FAIL;
    }
    STUB_LOG_TRACEF("Flash size: %d MB\n", flash_size / (1024 * 1024));

    return STUB_LIB_OK;
}

@erhankur WDYT? Especially the call to esp_flash_init at runtime?

And regarding pulling esp_flash driver sources... If we preserve driver's structure it will be easier to sync the sources in future. Why do we need to sync driver sources? - Bugfixes, support for new flash chips/types for old chips.

gerekon · 2025-06-18T17:47:48Z

Looks like esp_flash_init does one thing which can have impact on running app: setting the flash mode to whatever default.
https://github.com/espressif/esp-idf/blob/master/components/spi_flash/esp_flash_api.c#L317
So we need to detect when app is running on target somehow and avoid calling set_io_mode or call our special stub_lib_flash_init_runtime to fill in esp_flash_t struct properly to be used for the next calls to other API functions.

For base flash operations I think we can start with esp_flash_init. When we will start working on breakpoints set/clear functionality we can get back to this runtime issue.
@erhankur WDYT?

gerekon · 2025-06-18T18:03:03Z

My bad. Calling esp_flash_init is not enough for flash intialization. It is just for the driver and bringing the flash to default state. Before that bootloader calls
esp_rom_spiflash_attach. So we have to pick up corresponding code from current OpenOCD stub like

//	ets_efuse_read_op();

//	uint32_t spiconfig = ets_efuse_get_spiconfig();
//	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)
//		spiconfig = 1;	/* HSPI flash mode */

        // check if bootloader already attached flash
//	if ((READ_PERI_REG(SPI_CACHE_FCTRL_REG(0)) & SPI_CACHE_FLASH_USR_CMD) == 0) {
//		STUB_LOGI("Attach spi flash...\n");
//		esp_rom_spiflash_attach(spiconfig, 0);
//	} else {
                // do some runtime magic here
		//WRITE_PERI_REG(SPI_CTRL_REG(1), 0x208000);
		//WRITE_PERI_REG(SPI_CLOCK_REG(1), 0x3043);
		//g_rom_spiflash_dummy_len_plus[1] = 0;
//	}
        // the code above is what stub_target_flash_init() should do (just an example).
       // Currently it is done partially.
        stub_target_flash_init();
        // Init driver
        esp_err_t err = esp_flash_init(&chip)
        if (err != ESP_OK) {
             STUB_LOGE("Failed to init flash %d!\n", err);
              return STUB_LIB_FAIL;
         }
        err = esp_flash_get_size(&chip, &flash_size);
        if (err != ESP_OK) {
           STUB_LOGE("Failed to get flash size %d!\n", err);
           return STUB_LIB_FAIL;
        }
       STUB_LOG_TRACEF("Flash size: %d MB\n", flash_size / (1024 * 1024));

erhankur · 2025-06-18T18:32:30Z

In the current stub code, we attach the flash if it hasn't been attached yet. If the flash is already attached, I would expect not to call any initialization related code and to run with the existing settings. We can try that.

But when it comes to the cache, things get more complex. The stub code needs to know the cache settings (e.g., page size) and work with those settings to use the MMU properly. Before implementing cache support in the stub, we also need to consider how to handle IDF-configurable settings. Anyway, this is a topic for another PR.

I am OK, we can handle runtime support in the next PRs, but not too late. From my experience, the most critical parts; runtime init and PSRAM need to be addressed early on. (I assume big sizes will not be a problem with the new esp_flash APIs). The rest, such as read/write/erase, is relatively straightforward.

gerekon · 2025-06-18T19:03:11Z

Yes we agreed to do with esp_flash_xxx wrappers around ROM functions first.
#9 (comment)
I was confused that flash size function does not follow esp_flash_get_size. So I thought that we lost the way for some reason. So @antmak, please, consider my comments regarding esp_flash driver usage like my thoughts on how it could be at the next steps.

gerekon · 2025-06-18T14:38:17Z

src/esp8266/src/flash.c

 * SPDX-License-Identifier: Apache-2.0 OR MIT
 */

+#include <log.h>


I do not think we have ever supported ESP8266 in OpenOCD.

src/esp8266/src/flash.c

gerekon · 2025-06-18T14:53:20Z

include/target/impl/flash_init_auto_spiconfig.h

+  */
+extern void esp_rom_spiflash_attach(uint32_t spiconfig, bool legacy);
+
+inline static


Contents of this file are the same as for include/target/impl/flash_init_no_spiconfig.h. What is the purpose oh having them?

It is not good style to have function implementations in headers files. Please avoid this. Otherwise every change in such functions will trigger rebuild for all files which include such header.

I do not see a big sense to have this as inline and be in headers.

gerekon · 2025-06-18T15:00:48Z

include/target/impl/flash_init_auto_spiconfig.h

+  * @param legacy: Deprecated compatibility API value, must be false
+  *
+  */
+extern void esp_rom_spiflash_attach(uint32_t spiconfig, bool legacy);


you have this externs in several files? In this case It would be better to put ROM functions declarations to header file and include it.

gerekon · 2025-06-18T15:03:50Z

src/esp32/src/flash.c

+  *   EFUSE_SPICONFIG_RET_SPIQ, EFUSE_SPICONFIG_RET_SPID, EFUSE_SPICONFIG_RET_SPICS0, EFUSE_SPICONFIG_RET_SPIHD macros.
+  *   WP pin (for quad I/O modes) is not saved in efuse and not returned by this function.
+  */
+extern uint32_t ets_efuse_get_spiconfig(void);


this is also candidate to be in ROM functions header file. Please have a look at IDF. This function is declared in header file of esp_rom component.

gerekon · 2025-06-18T19:13:49Z

include/target/impl/flash_get_config_from_rom_old.h

+extern esp_rom_spiflash_chip_t g_rom_flashchip;
+
+inline static
+const esp_rom_spiflash_chip_t *flash_impl_get_config_from_rom_old(void)


Do we really need separate headers for this functions?
How about have it like macro and keep it with flash_impl_get_config_from_rom and other similar ROM related staff in one header?

gerekon · 2025-06-18T19:18:08Z

include/target/impl/flash_get_id_from_rom.h

+  * @brief Sets the correct flash_id in the flash config from SPI_MEM_FLASH_RDID in ROM code
+  *
+  */
+extern void esp_rom_spi_flash_update_id(void);


candidate to be mnoved to ROM header

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 erhankur and gerekon 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 5 times, most recently 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

antmak added 2 commits June 17, 2025 15:01

feat(spiflash): add initial structure for SPI Flash support: S2, C3, S3

73a2d80

feat(spiflash): add initial structure for SPI Flash support: H2, C6, …

4992d16

…C2, C5, P4, C61

antmak force-pushed the feat/spiflash branch from 96d57d4 to 4992d16 Compare June 17, 2025 08:04

antmak added 2 commits June 17, 2025 15:24

feat(spiflash): add initial structure for SPI Flash support: ESP32

6d6a0f9

feat(spiflash): add initial structure for SPI Flash support: ESP8266

cd2f0b3

gerekon reviewed Jun 18, 2025

View reviewed changes

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

erhankur commented Jun 17, 2025

Uh oh!

gerekon commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerekon commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerekon commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erhankur commented Jun 18, 2025

Uh oh!

gerekon commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

gerekon commented Jun 18, 2025 •

edited

Loading

gerekon commented Jun 18, 2025 •

edited

Loading

gerekon commented Jun 18, 2025 •

edited

Loading