From eea6cd85b37351ceda1b4c80a804912605977997 Mon Sep 17 00:00:00 2001 From: iabdalkader Date: Thu, 30 Sep 2021 09:13:15 +1000 Subject: [PATCH] stm32/sdram: Enforce gcc opt, and use volatile and DSB in sdram_test. Ensures consistent behaviour and resolves the D-Cache bug (the "exhaustive" argument being lost due to cache being turned off) when O0 is used. The changes in this commit are: - Change -O0 to -Os because "gcc is considered broken at -O0" according to https://github.com/ARM-software/CMSIS_5/issues/620#issuecomment-550235656 - Use volatile for mem_base so the compiler doesn't optimise away reads or writes to the SDRAM, which is being tested. - Use DSB to prevent any other compiler optimisations that would change the testing logic. - Use alternating pattern/antipattern in exhaustive test to catch more hardware/configuration errors. Implementation adapted by @andrewleech, taken directly from investigation by @iabdalkader and @dpgeorge. See #7841 and #7869 for further discussion. --- ports/stm32/sdram.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/ports/stm32/sdram.c b/ports/stm32/sdram.c index e0e3500836..4615c4fe1e 100644 --- a/ports/stm32/sdram.c +++ b/ports/stm32/sdram.c @@ -283,10 +283,10 @@ void sdram_leave_low_power(void) { #pragma GCC diagnostic ignored "-Wstringop-overflow" #endif -bool __attribute__((optimize("O0"))) sdram_test(bool exhaustive) { +bool __attribute__((optimize("Os"))) sdram_test(bool exhaustive) { uint8_t const pattern = 0xaa; uint8_t const antipattern = 0x55; - uint8_t *const mem_base = (uint8_t *)sdram_start(); + volatile uint8_t *const mem_base = (uint8_t *)sdram_start(); #if MICROPY_HW_SDRAM_TEST_FAIL_ON_ERROR char error_buffer[1024]; @@ -310,12 +310,13 @@ bool __attribute__((optimize("O0"))) sdram_test(bool exhaustive) { // Test data bus for (uint32_t i = 0; i < MICROPY_HW_SDRAM_MEM_BUS_WIDTH; i++) { - *((uint32_t *)mem_base) = (1 << i); - if (*((uint32_t *)mem_base) != (1 << i)) { + *((volatile uint32_t *)mem_base) = (1 << i); + __DSB(); + if (*((volatile uint32_t *)mem_base) != (1 << i)) { #if MICROPY_HW_SDRAM_TEST_FAIL_ON_ERROR snprintf(error_buffer, sizeof(error_buffer), "Data bus test failed at 0x%p expected 0x%x found 0x%lx", - &mem_base[0], (1 << i), ((uint32_t *)mem_base)[0]); + &mem_base[0], (1 << i), ((volatile uint32_t *)mem_base)[0]); __fatal_error(error_buffer); #endif return false; @@ -325,6 +326,7 @@ bool __attribute__((optimize("O0"))) sdram_test(bool exhaustive) { // Test address bus for (uint32_t i = 1; i < MICROPY_HW_SDRAM_SIZE; i <<= 1) { mem_base[i] = pattern; + __DSB(); if (mem_base[i] != pattern) { #if MICROPY_HW_SDRAM_TEST_FAIL_ON_ERROR snprintf(error_buffer, sizeof(error_buffer), @@ -338,6 +340,7 @@ bool __attribute__((optimize("O0"))) sdram_test(bool exhaustive) { // Check for aliasing (overlaping addresses) mem_base[0] = antipattern; + __DSB(); for (uint32_t i = 1; i < MICROPY_HW_SDRAM_SIZE; i <<= 1) { if (mem_base[i] != pattern) { #if MICROPY_HW_SDRAM_TEST_FAIL_ON_ERROR @@ -356,15 +359,15 @@ bool __attribute__((optimize("O0"))) sdram_test(bool exhaustive) { // is enabled, it's not just writing and reading from cache. // Note: This test should also detect refresh rate issues. for (uint32_t i = 0; i < MICROPY_HW_SDRAM_SIZE; i++) { - mem_base[i] = pattern; + mem_base[i] = ((i % 2) ? pattern : antipattern); } for (uint32_t i = 0; i < MICROPY_HW_SDRAM_SIZE; i++) { - if (mem_base[i] != pattern) { + if (mem_base[i] != ((i % 2) ? pattern : antipattern)) { #if MICROPY_HW_SDRAM_TEST_FAIL_ON_ERROR snprintf(error_buffer, sizeof(error_buffer), "Address bus slow test failed at 0x%p expected 0x%x found 0x%x", - &mem_base[i], pattern, mem_base[i]); + &mem_base[i], ((i % 2) ? pattern : antipattern), mem_base[i]); __fatal_error(error_buffer); #endif return false;