From 7ff9538c4803f84c876033da9fc60e8a920788a7 Mon Sep 17 00:00:00 2001 From: Alex Lisitsyn Date: Fri, 6 Sep 2019 15:37:55 +0800 Subject: [PATCH] espcoredump: fix issue with spi_flash access spi_flash has been updated and its functions work from flash by default instead of IRAM that cause issue add Kconfig value into espcoredump to enable spi_flash legacy mode (CONFIG_SPI_FLASH_USE_LEGACY_IMPL) when core dump is selected fix spi_flash issues to work correctly with legacy mode when CONFIG_SPI_FLASH_USE_LEGACY_IMPL is used --- components/esp32/Kconfig | 14 ++++++++++ components/esp32/cpu_start.c | 2 ++ components/esp32/panic.c | 8 ++++++ components/esp_gdbstub/linker.lf | 7 +++-- components/espcoredump/Kconfig | 1 + components/espcoredump/linker.lf | 11 +++++--- components/espcoredump/src/core_dump_flash.c | 8 +++--- components/espcoredump/src/core_dump_port.c | 4 ++- components/spi_flash/cache_utils.c | 28 ++++++++++++++++---- components/spi_flash/include/esp_spi_flash.h | 7 +++++ components/spi_flash/partition.c | 9 ++++++- docs/en/api-guides/fatal-errors.rst | 4 +++ 12 files changed, 87 insertions(+), 16 deletions(-) diff --git a/components/esp32/Kconfig b/components/esp32/Kconfig index d0e9b5da0..63afe63a8 100644 --- a/components/esp32/Kconfig +++ b/components/esp32/Kconfig @@ -776,6 +776,20 @@ menu "ESP32-specific" To prevent interrupting DPORT workarounds, need to disable interrupt with a maximum used level in the system. + config ESP32_PANIC_HANDLER_IRAM + bool "Place panic handler code in IRAM" + default n + help + If this option is disabled (default), the panic handler code is placed in flash not IRAM. + This means that if ESP-IDF crashes while flash cache is disabled, the panic handler will + automatically re-enable flash cache before running GDB Stub or Core Dump. This adds some minor + risk, if the flash cache status is also corrupted during the crash. + + If this option is enabled, the panic handler code is placed in IRAM. This allows the panic + handler to run without needing to re-enable cache first. This may be necessary to debug some + complex issues with crashes while flash cache is disabled (for example, when writing to + SPI flash.) + endmenu # ESP32-Specific menu "Power Management" diff --git a/components/esp32/cpu_start.c b/components/esp32/cpu_start.c index 9ce5df4ae..975f09b30 100644 --- a/components/esp32/cpu_start.c +++ b/components/esp32/cpu_start.c @@ -404,9 +404,11 @@ void start_cpu0_default(void) /* init default OS-aware flash access critical section */ spi_flash_guard_set(&g_flash_guard_default_ops); +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL esp_flash_app_init(); esp_err_t flash_ret = esp_flash_init_default_chip(); assert(flash_ret == ESP_OK); +#endif uint8_t revision = esp_efuse_get_chip_ver(); ESP_LOGI(TAG, "Chip Revision: %d", revision); diff --git a/components/esp32/panic.c b/components/esp32/panic.c index 294c3635d..d84ec1ecb 100644 --- a/components/esp32/panic.c +++ b/components/esp32/panic.c @@ -608,6 +608,14 @@ static __attribute__((noreturn)) void commonErrorHandler(XtExcFrame *frame) reconfigureAllWdts(); #endif +#if !CONFIG_ESP32_PANIC_HANDLER_IRAM + // Re-enable CPU cache for current CPU if it was disabled + if (!spi_flash_cache_enabled()) { + spi_flash_enable_cache(core_id); + panicPutStr("Re-enable cpu cache.\r\n"); + } +#endif + #if CONFIG_ESP32_PANIC_GDBSTUB disableAllWdts(); rtc_wdt_disable(); diff --git a/components/esp_gdbstub/linker.lf b/components/esp_gdbstub/linker.lf index b5d88a267..7093ea8b2 100644 --- a/components/esp_gdbstub/linker.lf +++ b/components/esp_gdbstub/linker.lf @@ -1,4 +1,7 @@ [mapping:esp_gdbstub] archive: libesp_gdbstub.a -entries: - * (noflash) +entries: + if ESP32_PANIC_HANDLER_IRAM = y: + * (noflash_text) + else: + * (default) \ No newline at end of file diff --git a/components/espcoredump/Kconfig b/components/espcoredump/Kconfig index 991bf18f7..fe8b6e455 100644 --- a/components/espcoredump/Kconfig +++ b/components/espcoredump/Kconfig @@ -13,6 +13,7 @@ menu "Core dump" config ESP32_ENABLE_COREDUMP_TO_FLASH bool "Flash" select ESP32_ENABLE_COREDUMP + select SPI_FLASH_USE_LEGACY_IMPL config ESP32_ENABLE_COREDUMP_TO_UART bool "UART" select ESP32_ENABLE_COREDUMP diff --git a/components/espcoredump/linker.lf b/components/espcoredump/linker.lf index 02dd9cfe7..131e10c30 100644 --- a/components/espcoredump/linker.lf +++ b/components/espcoredump/linker.lf @@ -1,7 +1,10 @@ [mapping:espcoredump] archive: libespcoredump.a entries: - core_dump_uart (noflash_text) - core_dump_flash (noflash_text) - core_dump_common (noflash_text) - core_dump_port (noflash_text) \ No newline at end of file + if ESP32_PANIC_HANDLER_IRAM = y: + core_dump_uart (noflash_text) + core_dump_flash (noflash_text) + core_dump_common (noflash_text) + core_dump_port (noflash_text) + else: + * (default) diff --git a/components/espcoredump/src/core_dump_flash.c b/components/espcoredump/src/core_dump_flash.c index 9c2ac7876..21e85731e 100644 --- a/components/espcoredump/src/core_dump_flash.c +++ b/components/espcoredump/src/core_dump_flash.c @@ -200,8 +200,8 @@ static esp_err_t esp_core_dump_flash_write_data(void *priv, void * data, uint32_ void esp_core_dump_to_flash(XtExcFrame *frame) { - core_dump_write_config_t wr_cfg; - core_dump_write_flash_data_t wr_data; + static core_dump_write_config_t wr_cfg; + static core_dump_write_flash_data_t wr_data; core_dump_crc_t crc = esp_core_dump_calc_flash_config_crc(); if (s_core_flash_config.partition_config_crc != crc) { @@ -214,8 +214,10 @@ void esp_core_dump_to_flash(XtExcFrame *frame) return; } - /* init non-OS flash access critical section */ +#if CONFIG_SPI_FLASH_USE_LEGACY_IMPL + // init non-OS flash access critical section spi_flash_guard_set(&g_flash_guard_no_os_ops); +#endif memset(&wr_cfg, 0, sizeof(wr_cfg)); wr_cfg.prepare = esp_core_dump_flash_write_prepare; diff --git a/components/espcoredump/src/core_dump_port.c b/components/espcoredump/src/core_dump_port.c index 2ac1b6482..1e6ab6a90 100644 --- a/components/espcoredump/src/core_dump_port.c +++ b/components/espcoredump/src/core_dump_port.c @@ -94,7 +94,9 @@ bool esp_core_dump_process_stack(core_dump_task_header_t* task_snaphort, uint32_ ESP_COREDUMP_LOG_PROCESS("Stack len = %lu (%x %x)", len, task_snaphort->stack_start, task_snaphort->stack_end); // Take stack padding into account - *length = (len + sizeof(uint32_t) - 1) & ~(sizeof(uint32_t) - 1); + if (length) { + *length = (len + sizeof(uint32_t) - 1) & ~(sizeof(uint32_t) - 1); + } task_is_valid = true; } return task_is_valid; diff --git a/components/spi_flash/cache_utils.c b/components/spi_flash/cache_utils.c index 25f937d74..63603097c 100644 --- a/components/spi_flash/cache_utils.c +++ b/components/spi_flash/cache_utils.c @@ -31,6 +31,18 @@ #include "esp_spi_flash.h" #include "esp_log.h" +#define DPORT_CACHE_BIT(cpuid, regid) DPORT_ ## cpuid ## regid + +#define DPORT_CACHE_MASK(cpuid) (DPORT_CACHE_BIT(cpuid, _CACHE_MASK_OPSDRAM) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DROM0) | \ + DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DRAM1) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IROM0) | \ + DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM1) | DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM0) ) + +#define DPORT_CACHE_VAL(cpuid) (~(DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DROM0) | \ + DPORT_CACHE_BIT(cpuid, _CACHE_MASK_DRAM1) | \ + DPORT_CACHE_BIT(cpuid, _CACHE_MASK_IRAM0))) + +#define DPORT_CACHE_GET_VAL(cpuid) (cpuid == 0) ? DPORT_CACHE_VAL(PRO) : DPORT_CACHE_VAL(APP) +#define DPORT_CACHE_GET_MASK(cpuid) (cpuid == 0) ? DPORT_CACHE_MASK(PRO) : DPORT_CACHE_MASK(APP) static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_state); static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_state); @@ -256,13 +268,10 @@ void IRAM_ATTR spi_flash_enable_interrupts_caches_no_os(void) * Cache_Flush before Cache_Read_Enable, even if cached data was not modified. */ -static const uint32_t cache_mask = DPORT_APP_CACHE_MASK_OPSDRAM | DPORT_APP_CACHE_MASK_DROM0 | - DPORT_APP_CACHE_MASK_DRAM1 | DPORT_APP_CACHE_MASK_IROM0 | - DPORT_APP_CACHE_MASK_IRAM1 | DPORT_APP_CACHE_MASK_IRAM0; - static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_state) { uint32_t ret = 0; + const uint32_t cache_mask = DPORT_CACHE_GET_MASK(cpuid); if (cpuid == 0) { ret |= DPORT_GET_PERI_REG_BITS2(DPORT_PRO_CACHE_CTRL1_REG, cache_mask, 0); while (DPORT_GET_PERI_REG_BITS2(DPORT_PRO_DCACHE_DBUG0_REG, DPORT_PRO_CACHE_STATE, DPORT_PRO_CACHE_STATE_S) != 1) { @@ -281,6 +290,7 @@ static void IRAM_ATTR spi_flash_disable_cache(uint32_t cpuid, uint32_t* saved_st static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_state) { + const uint32_t cache_mask = DPORT_CACHE_GET_MASK(cpuid); if (cpuid == 0) { DPORT_SET_PERI_REG_BITS(DPORT_PRO_CACHE_CTRL_REG, 1, 1, DPORT_PRO_CACHE_ENABLE_S); DPORT_SET_PERI_REG_BITS(DPORT_PRO_CACHE_CTRL1_REG, cache_mask, saved_state, 0); @@ -290,7 +300,6 @@ static void IRAM_ATTR spi_flash_restore_cache(uint32_t cpuid, uint32_t saved_sta } } - IRAM_ATTR bool spi_flash_cache_enabled(void) { bool result = (DPORT_REG_GET_BIT(DPORT_PRO_CACHE_CTRL_REG, DPORT_PRO_CACHE_ENABLE) != 0); @@ -299,3 +308,12 @@ IRAM_ATTR bool spi_flash_cache_enabled(void) #endif return result; } + +void IRAM_ATTR spi_flash_enable_cache(uint32_t cpuid) +{ + uint32_t cache_value = DPORT_CACHE_GET_VAL(cpuid); + cache_value &= DPORT_CACHE_GET_MASK(cpuid); + + // Re-enable cache on this CPU + spi_flash_restore_cache(cpuid, cache_value); +} diff --git a/components/spi_flash/include/esp_spi_flash.h b/components/spi_flash/include/esp_spi_flash.h index 33bdc4d83..5966f0ecc 100644 --- a/components/spi_flash/include/esp_spi_flash.h +++ b/components/spi_flash/include/esp_spi_flash.h @@ -295,6 +295,13 @@ const void *spi_flash_phys2cache(size_t phys_offs, spi_flash_mmap_memory_t memor */ bool spi_flash_cache_enabled(void); +/** + * @brief Re-enable cache for the core defined as cpuid parameter. + * + * @param cpuid the core number to enable instruction cache for + */ +void spi_flash_enable_cache(uint32_t cpuid); + /** * @brief SPI flash critical section enter function. * diff --git a/components/spi_flash/partition.c b/components/spi_flash/partition.c index 14de32c9f..a6b33a138 100644 --- a/components/spi_flash/partition.c +++ b/components/spi_flash/partition.c @@ -172,7 +172,9 @@ static esp_err_t load_partitions(void) err = ESP_ERR_NO_MEM; break; } +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL item->info.flash_chip = esp_flash_default_chip; +#endif item->info.address = it->pos.offset; item->info.size = it->pos.size; item->info.type = it->type; @@ -334,10 +336,11 @@ esp_err_t esp_partition_read(const esp_partition_t* partition, #endif // CONFIG_SPI_FLASH_USE_LEGACY_IMPL } else { #if CONFIG_SECURE_FLASH_ENC_ENABLED +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL if (partition->flash_chip != esp_flash_default_chip) { return ESP_ERR_NOT_SUPPORTED; } - +#endif /* Encrypted partitions need to be read via a cache mapping */ const void *buf; spi_flash_mmap_handle_t handle; @@ -376,9 +379,11 @@ esp_err_t esp_partition_write(const esp_partition_t* partition, #endif // CONFIG_SPI_FLASH_USE_LEGACY_IMPL } else { #if CONFIG_SECURE_FLASH_ENC_ENABLED +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL if (partition->flash_chip != esp_flash_default_chip) { return ESP_ERR_NOT_SUPPORTED; } +#endif return spi_flash_write_encrypted(dst_offset, src, size); #else return ESP_ERR_NOT_SUPPORTED; @@ -428,9 +433,11 @@ esp_err_t esp_partition_mmap(const esp_partition_t* partition, size_t offset, si if (offset + size > partition->size) { return ESP_ERR_INVALID_SIZE; } +#ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL if (partition->flash_chip != esp_flash_default_chip) { return ESP_ERR_NOT_SUPPORTED; } +#endif size_t phys_addr = partition->address + offset; // offset within 64kB block size_t region_offset = phys_addr & 0xffff; diff --git a/docs/en/api-guides/fatal-errors.rst b/docs/en/api-guides/fatal-errors.rst index c64268fb7..f1ca3f184 100644 --- a/docs/en/api-guides/fatal-errors.rst +++ b/docs/en/api-guides/fatal-errors.rst @@ -62,6 +62,10 @@ Behavior of panic handler is affected by two other configuration options. - If :doc:`Core Dump ` feature is enabled (``CONFIG_ESP32_ENABLE_COREDUMP_TO_FLASH`` or ``CONFIG_ESP32_ENABLE_COREDUMP_TO_UART`` options), then system state (task stacks and registers) will be dumped either to Flash or UART, for later analysis. +- If :ref:`CONFIG_ESP32_PANIC_HANDLER_IRAM` is disabled (disabled by default), the panic handler code is placed in flash memory not IRAM. This means that if ESP-IDF crashes while flash cache is disabled, the panic handler will automatically re-enable flash cache before running GDB Stub or Core Dump. This adds some minor risk, if the flash cache status is also corrupted during the crash. + + If this option is enabled, the panic handler code is placed in IRAM. This allows the panic handler to run without needing to re-enable cache first. This may be necessary to debug some complex issues with crashes while flash cache is disabled (for example, when writing to SPI flash). + The following diagram illustrates panic handler behavior: .. blockdiag::