From 68791163b27329d26c515967ddb22c6b0fe08cb7 Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Mon, 16 Dec 2019 22:46:37 +0800 Subject: [PATCH 1/2] esp32: Fix esp_dport_access_reg_read --- components/esp32/dport_access.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esp32/dport_access.c b/components/esp32/dport_access.c index cad078386..f7435a164 100644 --- a/components/esp32/dport_access.c +++ b/components/esp32/dport_access.c @@ -255,8 +255,8 @@ uint32_t IRAM_ATTR esp_dport_access_reg_read(uint32_t reg) uint32_t apb; unsigned int intLvl; __asm__ __volatile__ (\ - "movi %[APB], "XTSTR(0x3ff40078)"\n"\ "rsil %[LVL], "XTSTR(CONFIG_ESP32_DPORT_DIS_INTERRUPT_LVL)"\n"\ + "movi %[APB], "XTSTR(0x3ff40078)"\n"\ "l32i %[APB], %[APB], 0\n"\ "l32i %[REG], %[REG], 0\n"\ "wsr %[LVL], "XTSTR(PS)"\n"\ From 106f1658990abf8c23d981fcde2cdfcff1fb41a9 Mon Sep 17 00:00:00 2001 From: KonstantinKondrashov Date: Thu, 19 Dec 2019 21:59:39 +0800 Subject: [PATCH 2/2] esp32: Add UT for DPORT --- components/esp32/test/test_dport.c | 77 +++++++++++++++++++ .../esp32/test/test_dport_xt_highint5.S | 4 +- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/components/esp32/test/test_dport.c b/components/esp32/test/test_dport.c index 61ce26ed8..165e64a0b 100644 --- a/components/esp32/test/test_dport.c +++ b/components/esp32/test/test_dport.c @@ -417,4 +417,81 @@ TEST_CASE("Check pre-read workaround DPORT and Hi-interrupt", "[esp32]") // Hi-interrupt - CPU1 run_tasks("accessAPB", accessAPB, "accessDPORT2", accessDPORT2, 10000); } + +static uint32_t s_shift_counter; + +/* +The test_dport_access_reg_read() is similar DPORT_REG_READ() but has differents: +- generate an interrupt by SET_CCOMPARE +- additional branch command helps get good reproducing an issue with breaking the DPORT pre-read workaround +- uncomment (1) and comment (2) it allows seeing the broken pre-read workaround +For pre-reading the workaround, it is important that the two reading commands APB and DPORT +are executed without interruption. For this reason, it disables interrupts and to do reading inside the safe area. +But despite a disabling interrupt it was still possible that these two readings can be interrupted. +The reason is linked with work parallel execution commands in the pipeline (it is not a bug). +To resolve this issue (1) was moved to (2) position into the disabled interrupt part. +When the read command is interrupted after stage E(execute), the result of its execution will be saved in the internal buffer, +and after returning from the interrupt, this command takes this value from the buffer without repeating the reading, +which is critical for the DPORT pre-read workaround. To fix it we added additional command under safe area ((1)->(2)). +*/ +static uint32_t IRAM_ATTR test_dport_access_reg_read(uint32_t reg) +{ +#if defined(BOOTLOADER_BUILD) || !defined(CONFIG_ESP32_DPORT_WORKAROUND) || !defined(ESP_PLATFORM) + return _DPORT_REG_READ(reg); +#else + uint32_t apb; + unsigned int intLvl; + XTHAL_SET_CCOMPARE(2, XTHAL_GET_CCOUNT() + s_shift_counter); + __asm__ __volatile__ (\ + /* "movi %[APB], "XTSTR(0x3ff40078)"\n" */ /* (1) uncomment for reproduce issue */ \ + "bnez %[APB], kl1\n" /* this branch command helps get good reproducing */ \ + "kl1:\n"\ + "rsil %[LVL], "XTSTR(CONFIG_ESP32_DPORT_DIS_INTERRUPT_LVL)"\n"\ + "movi %[APB], "XTSTR(0x3ff40078)"\n" /* (2) comment for reproduce issue */ \ + "l32i %[APB], %[APB], 0\n"\ + "l32i %[REG], %[REG], 0\n"\ + "wsr %[LVL], "XTSTR(PS)"\n"\ + "rsync\n"\ + : [APB]"=a"(apb), [REG]"+a"(reg), [LVL]"=a"(intLvl)\ + : \ + : "memory" \ + ); + return reg; +#endif +} + +// The accessDPORT3 task is similar accessDPORT2 but uses test_dport_access_reg_read() instead of usual DPORT_REG_READ(). +static void accessDPORT3(void *pvParameters) +{ + xSemaphoreHandle *sema = (xSemaphoreHandle *) pvParameters; + dport_test_result = true; + + TEST_ESP_OK(esp_intr_alloc(ETS_INTERNAL_TIMER2_INTR_SOURCE, ESP_INTR_FLAG_LEVEL5 | ESP_INTR_FLAG_IRAM, NULL, NULL, &inth)); + int i = 0; + while (exit_flag == false) { + if (test_dport_access_reg_read(DPORT_DATE_REG) != test_dport_access_reg_read(DPORT_DATE_REG)) { + dport_test_result = false; + break; + } + if ((++i % 100) == 0) { + s_shift_counter = (s_shift_counter + 1) % 30; + } + } + esp_intr_free(inth); + printf("accessDPORT3 finish\n"); + + xSemaphoreGive(*sema); + vTaskDelete(NULL); +} + +TEST_CASE("Check pre-read workaround DPORT and Hi-interrupt (2)", "[esp32]") +{ + s_shift_counter = 1; + xt_highint5_read_apb = 0; + dport_test_result = false; + apb_test_result = true; + // Access DPORT(pre-read method) - CPU1 + // Hi-interrupt - CPU1 + run_tasks("accessAPB", accessAPB, "accessDPORT3", accessDPORT3, 10000); +} #endif // CONFIG_FREERTOS_UNICORE diff --git a/components/esp32/test/test_dport_xt_highint5.S b/components/esp32/test/test_dport_xt_highint5.S index 965c7385a..19828bf91 100644 --- a/components/esp32/test/test_dport_xt_highint5.S +++ b/components/esp32/test/test_dport_xt_highint5.S @@ -28,9 +28,7 @@ xt_highint5: bnez a0, .read_apb_reg // Short interrupt - esync - rsr a0, CCOUNT - addi a0, a0, 27 + movi a0, 0 wsr a0, CCOMPARE2 esync