From c218f669babeb2cfb7de0d573f6f63e5659d0713 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Mon, 28 Dec 2020 11:39:25 +0800 Subject: [PATCH] panic on RISC-V: Take into account Merge Request comments --- components/esp32c3/cache_err_int.c | 4 ++-- .../esp_system/port/arch/riscv/panic_arch.c | 8 +++++-- .../esp_system/port/arch/xtensa/panic_arch.c | 2 +- components/esp_system/port/panic_handler.c | 8 +++++-- .../include/hal/interrupt_controller_hal.h | 2 +- .../riscv/include/esp_private/panic_reason.h | 14 ++++++++----- components/riscv/vectors.S | 20 +++++++++--------- .../spi_flash/test/test_cache_disabled.c | 21 +++---------------- 8 files changed, 38 insertions(+), 41 deletions(-) diff --git a/components/esp32c3/cache_err_int.c b/components/esp32c3/cache_err_int.c index d4efe24d2c..20cf2be2a2 100644 --- a/components/esp32c3/cache_err_int.c +++ b/components/esp32c3/cache_err_int.c @@ -18,8 +18,8 @@ to panic the CPU, which from a debugging perspective is better than grabbing bad data from the bus. */ - -#include "freertos/FreeRTOS.h" +#include "esp32c3/rom/ets_sys.h" +#include "esp_attr.h" #include "esp_intr_alloc.h" #include "soc/extmem_reg.h" #include "soc/periph_defs.h" diff --git a/components/esp_system/port/arch/riscv/panic_arch.c b/components/esp_system/port/arch/riscv/panic_arch.c index 66b9cfe7ab..21a283ec01 100644 --- a/components/esp_system/port/arch/riscv/panic_arch.c +++ b/components/esp_system/port/arch/riscv/panic_arch.c @@ -184,11 +184,13 @@ void panic_soc_fill_info(void *f, panic_info_t *info) RvExcFrame *frame = (RvExcFrame *) f; /* Please keep in sync with PANIC_RSN_* defines */ - static const char *pseudo_reason[PANIC_RSN_MAX + 1] = { + static const char *pseudo_reason[PANIC_RSN_COUNT] = { "Unknown reason", "Interrupt wdt timeout on CPU0", +#if SOC_CPU_NUM > 1 "Interrupt wdt timeout on CPU1", - "Cache exception", +#endif + "Cache error" }; info->reason = pseudo_reason[0]; @@ -213,8 +215,10 @@ void panic_soc_fill_info(void *f, panic_info_t *info) info->core = core; info->exception = PANIC_EXCEPTION_TWDT; +#if SOC_CPU_NUM > 1 _Static_assert(PANIC_RSN_INTWDT_CPU0 + 1 == PANIC_RSN_INTWDT_CPU1, "PANIC_RSN_INTWDT_CPU1 must be equal to PANIC_RSN_INTWDT_CPU0 + 1"); +#endif info->reason = pseudo_reason[PANIC_RSN_INTWDT_CPU0 + core]; } } diff --git a/components/esp_system/port/arch/xtensa/panic_arch.c b/components/esp_system/port/arch/xtensa/panic_arch.c index 752b46fa9e..e4afe8256d 100644 --- a/components/esp_system/port/arch/xtensa/panic_arch.c +++ b/components/esp_system/port/arch/xtensa/panic_arch.c @@ -411,7 +411,7 @@ void panic_soc_fill_info(void *f, panic_info_t *info) #if CONFIG_IDF_TARGET_ESP32 "Cache disabled but cached memory region accessed", #elif CONFIG_IDF_TARGET_ESP32S2 - "Cache exception", + "Cache error", #endif }; diff --git a/components/esp_system/port/panic_handler.c b/components/esp_system/port/panic_handler.c index 24f34e5ce0..fad73a8390 100644 --- a/components/esp_system/port/panic_handler.c +++ b/components/esp_system/port/panic_handler.c @@ -179,8 +179,12 @@ static void panic_handler(void *frame, bool pseudo_excause) panic_set_address(frame, (uint32_t)&_invalid_pc_placeholder); } #endif - if (panic_get_cause(frame) == PANIC_RSN_INTWDT_CPU0 || - panic_get_cause(frame) == PANIC_RSN_INTWDT_CPU1) { + if (panic_get_cause(frame) == PANIC_RSN_INTWDT_CPU0 +#if !CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE + || panic_get_cause(frame) == PANIC_RSN_INTWDT_CPU1 +#endif + ) + { wdt_hal_write_protect_disable(&wdt0_context); wdt_hal_handle_intr(&wdt0_context); wdt_hal_write_protect_enable(&wdt0_context); diff --git a/components/hal/include/hal/interrupt_controller_hal.h b/components/hal/include/hal/interrupt_controller_hal.h index 9163a46705..c0a6d61822 100644 --- a/components/hal/include/hal/interrupt_controller_hal.h +++ b/components/hal/include/hal/interrupt_controller_hal.h @@ -55,7 +55,7 @@ __attribute__((pure)) int interrupt_controller_hal_desc_level(int interrupt_num */ __attribute__((pure)) int_desc_flag_t interrupt_controller_hal_desc_flags(int interrupt_number, int cpu_number); -#if CONFIG_IDF_TARGET_ARCH_RISCV +#if SOC_INTERRUPT_LEVEL_CAN_SET /** * @brief Set the interrupt level given an interrupt number. * diff --git a/components/riscv/include/esp_private/panic_reason.h b/components/riscv/include/esp_private/panic_reason.h index a91e6b3396..89d061c444 100644 --- a/components/riscv/include/esp_private/panic_reason.h +++ b/components/riscv/include/esp_private/panic_reason.h @@ -13,8 +13,12 @@ // limitations under the License. #pragma once -#define PANIC_RSN_NONE 0 -#define PANIC_RSN_INTWDT_CPU0 1 -#define PANIC_RSN_INTWDT_CPU1 2 -#define PANIC_RSN_CACHEERR 3 -#define PANIC_RSN_MAX 3 +enum _panic_reasons { + PANIC_RSN_NONE = 0, + PANIC_RSN_INTWDT_CPU0, +#if SOC_CPU_NUM > 1 + PANIC_RSN_INTWDT_CPU1, +#endif + PANIC_RSN_CACHEERR, + PANIC_RSN_COUNT +} panic_reasons; diff --git a/components/riscv/vectors.S b/components/riscv/vectors.S index 74e1482ce0..d273f3ab17 100644 --- a/components/riscv/vectors.S +++ b/components/riscv/vectors.S @@ -18,8 +18,8 @@ .equ SAVE_REGS, 32 .equ CONTEXT_SIZE, (SAVE_REGS * 4) - .equ exception_from_panic, xt_unhandled_exception - .equ exception_from_isr, panicHandler + .equ panic_from_exception, xt_unhandled_exception + .equ panic_from_isr, panicHandler .macro save_regs addi sp, sp, -CONTEXT_SIZE @@ -182,20 +182,20 @@ _panic_handler: csrr t0, mhartid sw t0, RV_STK_MHARTID(sp) - /* Call exception_from_panic(sp) or exception_from_isr(sp) + /* Call panic_from_exception(sp) or panic_from_isr(sp) * depending on whether we have a pseudo excause or not. * If mcause's highest bit is 1, then an interrupt called this routine, - * so we have a pseudo excause. Else, it is due to a panic, we don't have - * an excause */ + * so we have a pseudo excause. Else, it is due to a exception, we don't + * have an pseudo excause */ mv a0, sp csrr a1, mcause - /* Branches instructions don't accept immediates values, so use t1 to store - * our comparator */ + /* Branches instructions don't accept immediates values, so use t1 to + * store our comparator */ li t0, 0x80000000 bgeu a1, t0, _call_panic_handler sw a1, RV_STK_MCAUSE(sp) /* exception_from_panic never returns */ - j exception_from_panic + j panic_from_exception _call_panic_handler: /* Remove highest bit from mcause (a1) register and save it in the * structure */ @@ -203,8 +203,8 @@ _call_panic_handler: and a1, a1, t0 sw a1, RV_STK_MCAUSE(sp) /* exception_from_isr never returns */ - j exception_from_isr - .size exception_from_isr, .-exception_from_isr + j panic_from_isr + .size panic_from_isr, .-panic_from_isr /* This is the interrupt handler. * It saves the registers on the stack, diff --git a/components/spi_flash/test/test_cache_disabled.c b/components/spi_flash/test/test_cache_disabled.c index 40f5bb0d4c..aae44bd431 100644 --- a/components/spi_flash/test/test_cache_disabled.c +++ b/components/spi_flash/test/test_cache_disabled.c @@ -51,19 +51,10 @@ TEST_CASE("spi_flash_cache_enabled() works on both CPUs", "[spi_flash][esp_flash vQueueDelete(result_queue); } -/** - * On ESP32-C3 boards, constant data with a size less or equal to 8 bytes - * (64 bits) are placed in the DRAM. - * Let's add a third unused element to this array to force it to the DROM. - */ -static const uint32_t s_in_rodata[] = { 0x12345678, 0xfedcba98, 0x42 }; +static const uint32_t s_in_rodata[] = { 0x12345678, 0xfedcba98 }; static void IRAM_ATTR cache_access_test_func(void* arg) { - /* Assert that the array s_in_rodata is in DROM. If not, this test is - * invalid as disabling the cache wouldn't have any effect. */ - TEST_ASSERT(esp_ptr_in_drom(s_in_rodata)); - spi_flash_disable_interrupts_caches_and_other_cpu(); volatile uint32_t* src = (volatile uint32_t*) s_in_rodata; uint32_t v1 = src[0]; @@ -74,15 +65,9 @@ static void IRAM_ATTR cache_access_test_func(void* arg) vTaskDelete(NULL); } -#ifdef CONFIG_IDF_TARGET_ESP32C3 -#define CACHE_ERROR_REASON "Cache exception,RTC_SW_CPU_RST" -#else -#define CACHE_ERROR_REASON "Cache disabled,SW_RESET" -#endif - // These tests works properly if they resets the chip with the // "Cache disabled but cached memory region accessed" reason and the correct CPU is logged. -TEST_CASE("invalid access to cache raises panic (PRO CPU)", "[spi_flash][reset="CACHE_ERROR_REASON"]") +TEST_CASE("invalid access to cache raises panic (PRO CPU)", "[spi_flash][ignore]") { xTaskCreatePinnedToCore(&cache_access_test_func, "ia", 2048, NULL, 5, NULL, 0); vTaskDelay(1000/portTICK_PERIOD_MS); @@ -90,7 +75,7 @@ TEST_CASE("invalid access to cache raises panic (PRO CPU)", "[spi_flash][reset=" #ifndef CONFIG_FREERTOS_UNICORE -TEST_CASE("invalid access to cache raises panic (APP CPU)", "[spi_flash][reset=TG1WDT_SYS_RESET]") +TEST_CASE("invalid access to cache raises panic (APP CPU)", "[spi_flash][ignore]") { xTaskCreatePinnedToCore(&cache_access_test_func, "ia", 2048, NULL, 5, NULL, 1); vTaskDelay(1000/portTICK_PERIOD_MS);