From 3525a96a57493b0e7725533e0a2b2c0e66e00167 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sun, 4 Jun 2023 17:44:31 -0500 Subject: [PATCH 1/3] Add new pico_flash library, with flash_safe_execute(func) method to help with preventing IRQs and other core accessing flash with pico_multicore or FreeRTOS SMP --- docs/index.h | 3 +- src/common/pico_base/include/pico/error.h | 1 + src/rp2_common/CMakeLists.txt | 1 + .../include/pico/async_context_freertos.h | 2 +- src/rp2_common/pico_btstack/CMakeLists.txt | 2 +- .../pico_btstack/btstack_flash_bank.c | 36 ++- src/rp2_common/pico_flash/CMakeLists.txt | 12 + src/rp2_common/pico_flash/flash.c | 229 ++++++++++++++++++ .../pico_flash/include/pico/flash.h | 130 ++++++++++ src/rp2_common/pico_multicore/multicore.c | 4 +- test/kitchen_sink/CMakeLists.txt | 1 + test/kitchen_sink/kitchen_sink.c | 1 + 12 files changed, 410 insertions(+), 12 deletions(-) create mode 100644 src/rp2_common/pico_flash/CMakeLists.txt create mode 100644 src/rp2_common/pico_flash/flash.c create mode 100644 src/rp2_common/pico_flash/include/pico/flash.h diff --git a/docs/index.h b/docs/index.h index 3deb031d7..d6edc0040 100644 --- a/docs/index.h +++ b/docs/index.h @@ -42,8 +42,9 @@ * set of functionality above the basic hardware interfaces * @{ * \defgroup pico_async_context pico_async_context - * \defgroup pico_multicore pico_multicore + * \defgroup pico_flash pico_flash * \defgroup pico_i2c_slave pico_i2c_slave + * \defgroup pico_multicore pico_multicore * \defgroup pico_rand pico_rand * \defgroup pico_stdlib pico_stdlib * \defgroup pico_sync pico_sync diff --git a/src/common/pico_base/include/pico/error.h b/src/common/pico_base/include/pico/error.h index 2f36e4c9b..7508f1644 100644 --- a/src/common/pico_base/include/pico/error.h +++ b/src/common/pico_base/include/pico/error.h @@ -24,6 +24,7 @@ enum pico_error_codes { PICO_ERROR_IO = -6, PICO_ERROR_BADAUTH = -7, PICO_ERROR_CONNECT_FAILED = -8, + PICO_ERROR_INSUFFICIENT_RESOURCES = -9, }; #endif // !__ASSEMBLER__ diff --git a/src/rp2_common/CMakeLists.txt b/src/rp2_common/CMakeLists.txt index eff827b6a..82f56f8c5 100644 --- a/src/rp2_common/CMakeLists.txt +++ b/src/rp2_common/CMakeLists.txt @@ -45,6 +45,7 @@ if (NOT PICO_BARE_METAL) pico_add_subdirectory(pico_divider) pico_add_subdirectory(pico_double) pico_add_subdirectory(pico_int64_ops) + pico_add_subdirectory(pico_flash) pico_add_subdirectory(pico_float) pico_add_subdirectory(pico_mem_ops) pico_add_subdirectory(pico_malloc) diff --git a/src/rp2_common/pico_async_context/include/pico/async_context_freertos.h b/src/rp2_common/pico_async_context/include/pico/async_context_freertos.h index 4ef8e0638..670a05cd5 100644 --- a/src/rp2_common/pico_async_context/include/pico/async_context_freertos.h +++ b/src/rp2_common/pico_async_context/include/pico/async_context_freertos.h @@ -90,7 +90,7 @@ bool async_context_freertos_init(async_context_freertos_t *self, async_context_f async_context_freertos_config_t config = { .task_priority = ASYNC_CONTEXT_DEFAULT_FREERTOS_TASK_PRIORITY, .task_stack_size = ASYNC_CONTEXT_DEFAULT_FREERTOS_TASK_STACK_SIZE, -#if configUSE_CORE_AFFINITY +#if configUSE_CORE_AFFINITY && configNUM_CORES > 1 .task_core_id = (UBaseType_t)-1, // none #endif }; diff --git a/src/rp2_common/pico_btstack/CMakeLists.txt b/src/rp2_common/pico_btstack/CMakeLists.txt index 9f38bf296..182156be9 100644 --- a/src/rp2_common/pico_btstack/CMakeLists.txt +++ b/src/rp2_common/pico_btstack/CMakeLists.txt @@ -156,7 +156,7 @@ if (EXISTS ${PICO_BTSTACK_PATH}/${BTSTACK_TEST_PATH}) ${CMAKE_CURRENT_LIST_DIR}/btstack_flash_bank.c ) target_include_directories(pico_btstack_flash_bank_headers INTERFACE ${CMAKE_CURRENT_LIST_DIR}/include) - pico_mirrored_target_link_libraries(pico_btstack_flash_bank INTERFACE pico_btstack_base) + pico_mirrored_target_link_libraries(pico_btstack_flash_bank INTERFACE pico_btstack_base pico_flash) pico_add_library(pico_btstack_run_loop_async_context NOFLAG) target_sources(pico_btstack_run_loop_async_context INTERFACE diff --git a/src/rp2_common/pico_btstack/btstack_flash_bank.c b/src/rp2_common/pico_btstack/btstack_flash_bank.c index 670b5c02f..6e1604632 100644 --- a/src/rp2_common/pico_btstack/btstack_flash_bank.c +++ b/src/rp2_common/pico_btstack/btstack_flash_bank.c @@ -5,7 +5,7 @@ */ #include "pico/btstack_flash_bank.h" -#include "hardware/flash.h" +#include "pico/flash.h" #include "hardware/sync.h" #include @@ -33,12 +33,30 @@ static uint32_t pico_flash_bank_get_alignment(void * context) { return 1; } +typedef struct { + bool op_is_erase; + uintptr_t p0; + uintptr_t p1; +} mutation_operation_t; + +static void pico_flash_bank_perform_flash_mutation_operation(void *param) { + const mutation_operation_t *mop = (const mutation_operation_t *)param; + if (mop->op_is_erase) { + flash_range_erase(mop->p0, PICO_FLASH_BANK_SIZE); + } else { + flash_range_program(mop->p0, (const uint8_t *)mop->p1, FLASH_PAGE_SIZE); + } +} + static void pico_flash_bank_erase(void * context, int bank) { (void)(context); DEBUG_PRINT("erase: bank %d\n", bank); - uint32_t status = save_and_disable_interrupts(); - flash_range_erase(PICO_FLASH_BANK_STORAGE_OFFSET + (PICO_FLASH_BANK_SIZE * bank), PICO_FLASH_BANK_SIZE); - restore_interrupts(status); + mutation_operation_t mop = { + .op_is_erase = true, + .p0 = PICO_FLASH_BANK_STORAGE_OFFSET + (PICO_FLASH_BANK_SIZE * bank), + }; + // todo better timeout + flash_safe_execute(pico_flash_bank_perform_flash_mutation_operation, &mop, UINT32_MAX); } static void pico_flash_bank_read(void *context, int bank, uint32_t offset, uint8_t *buffer, uint32_t size) { @@ -118,9 +136,13 @@ static void pico_flash_bank_write(void * context, int bank, uint32_t offset, con offset = 0; // Now program the entire page - uint32_t status = save_and_disable_interrupts(); - flash_range_program(bank_start_pos + (page * FLASH_PAGE_SIZE), page_data, FLASH_PAGE_SIZE); - restore_interrupts(status); + mutation_operation_t mop = { + .op_is_erase = false, + .p0 = bank_start_pos + (page * FLASH_PAGE_SIZE), + .p1 = (uintptr_t)page_data + }; + // todo better timeout + flash_safe_execute(pico_flash_bank_perform_flash_mutation_operation, &mop, UINT32_MAX); } } diff --git a/src/rp2_common/pico_flash/CMakeLists.txt b/src/rp2_common/pico_flash/CMakeLists.txt new file mode 100644 index 000000000..e06b74fa9 --- /dev/null +++ b/src/rp2_common/pico_flash/CMakeLists.txt @@ -0,0 +1,12 @@ +pico_add_library(pico_flash) + +target_sources(pico_flash INTERFACE + ${CMAKE_CURRENT_LIST_DIR}/flash.c +) + +target_include_directories(pico_flash_headers INTERFACE ${CMAKE_CURRENT_LIST_DIR}/include) + +# just include multicore headers, as we don't want to pull in the lib if it isn't pulled in already +target_link_libraries(pico_flash INTERFACE pico_multicore_headers) + +pico_mirrored_target_link_libraries(pico_flash INTERFACE pico_time hardware_sync) diff --git a/src/rp2_common/pico_flash/flash.c b/src/rp2_common/pico_flash/flash.c new file mode 100644 index 000000000..b05b12e5c --- /dev/null +++ b/src/rp2_common/pico_flash/flash.c @@ -0,0 +1,229 @@ +/* + * Copyright (c) 2022 Raspberry Pi (Trading) Ltd. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "pico/flash.h" +#include "hardware/exception.h" +#include "hardware/sync.h" +#if PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT +#include "pico/multicore.h" +#endif +#if PICO_FLASH_SAFE_EXECUTE_SUPPORT_FREERTOS_SMP +#include "FreeRTOS.h" +#include "task.h" +// now we have FreeRTOS header we can check core count... we can only use FreeRTOS SMP mechanism +// with two cores +#if configNUM_CORES == 2 +#if configUSE_CORE_AFFINITY +#define PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP 1 +#else +#error configUSE_CORE_AFFINITY is required for PICO_FLASH_SAFE_EXECUTE_SUPPORT_FREERTOS_SMP +#endif +#endif +#endif + +// There are multiple scenarios: +// +// 1. No use of core 1 - we just want to disable IRQs and not wait on core 1 to acquiesce +// 2. Regular pico_multicore - we need to use multicore lockout. +// 3. FreeRTOS on core 0, no use of core 1 - we just want to disable IRQs +// 4. FreeRTOS SMP on both cores - we need to schedule a high priority task on the other core to disable IRQs. +// 5. FreeRTOS on one core, but application is using the other core. ** WE CANNOT SUPPORT THIS TODAY ** without +// the equivalent PICO_FLASH_ASSUME_COREx_SAFE (i.e. the user mkaing sure the other core is fine) + +static bool default_core_init_deinit(bool init); +static int default_enter_safe_zone_timeout_ms(uint32_t timeout_ms); +static int default_exit_safe_zone_timeout_ms(uint32_t timeout_ms); + +// note the default methods are combined, rather than having a separate helper for +// FreeRTOS, as we may support mixed multicore and non SMP FreeRTOS in the future + +static flash_safety_helper_t default_flash_safety_helper = { + .core_init_deinit = default_core_init_deinit, + .enter_safe_zone_timeout_ms = default_enter_safe_zone_timeout_ms, + .exit_safe_zone_timeout_ms = default_exit_safe_zone_timeout_ms +}; + +#if PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT +// note that these are not reset by core reset, however for now, I think people resetting cores +// and then doing this again without re-initializing pico_flash for that core, is probably +// something we can live with breaking. +static bool core_initialized[NUM_CORES]; +#endif + +#if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP +enum { + FREERTOS_LOCKOUT_NONE = 0, + FREERTOS_LOCKOUT_LOCKER_WAITING, + FREERTOS_LOCKOUT_LOCKEE_READY, + FREERTOS_LOCKOUT_LOCKER_DONE, + FREERTOS_LOCKOUT_LOCKEE_DONE, +}; +// state for the lockout operation launched from the corresponding core +static volatile uint8_t lockout_state[NUM_CORES]; +#endif + +__attribute__((weak)) flash_safety_helper_t *get_flash_safety_helper(void) { + return &default_flash_safety_helper; +} + +bool flash_safe_execute_core_init(void) { + flash_safety_helper_t *helper = get_flash_safety_helper(); + return helper ? helper->core_init_deinit(true) : false; +} + +bool flash_safe_execute_core_deinit(void) { + flash_safety_helper_t *helper = get_flash_safety_helper(); + return helper ? helper->core_init_deinit(false) : false; +} + +int flash_safe_execute(void (*func)(void *), void *param, uint32_t enter_exit_timeout_ms) { + flash_safety_helper_t *helper = get_flash_safety_helper(); + if (!helper) return PICO_ERROR_NOT_PERMITTED; + int rc = helper->enter_safe_zone_timeout_ms(enter_exit_timeout_ms); + if (!rc) { + func(param); + rc = helper->exit_safe_zone_timeout_ms(enter_exit_timeout_ms); + } + return rc; +} + +static bool default_core_init_deinit(__unused bool init) { +#if PICO_FLASH_ASSUME_CORE0_SAFE + if (!get_core_num()) return true; +#endif +#if PICO_FLASH_ASSUME_CORE1_SAFE + if (get_core_num()) return true; +#endif +#if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP + return true; +#endif +#if PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT + if (!init) { + return false; + } + multicore_lockout_victim_init(); + core_initialized[get_core_num()] = init; +#endif + return true; +} + +// irq_state for the lockout operation launched from the corresponding core +static uint32_t irq_state[NUM_CORES]; + +static bool use_irq_only(void) { +#if PICO_FLASH_ASSUME_CORE0_SAFE + if (get_core_num()) return true; +#endif +#if PICO_FLASH_ASSUME_CORE1_SAFE + if (!get_core_num()) return true; +#endif + return false; +} + +#if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP +static void __not_in_flash_func(flash_lockout_task)(__unused void *vother_core_num) { + uint other_core_num = (uintptr_t)vother_core_num; + while (lockout_state[other_core_num] != FREERTOS_LOCKOUT_LOCKER_WAITING) { + __wfe(); // we don't bother to try to let lower priority tasks run + } + uint32_t save = save_and_disable_interrupts(); + lockout_state[other_core_num] = FREERTOS_LOCKOUT_LOCKEE_READY; + __sev(); + while (lockout_state[other_core_num] == FREERTOS_LOCKOUT_LOCKEE_READY) { + __wfe(); // we don't bother to try to let lower priority tasks run + } + restore_interrupts(save); + lockout_state[other_core_num] = FREERTOS_LOCKOUT_LOCKEE_DONE; + __sev(); + // bye bye + vTaskDelete(NULL); +} +#endif + +static int default_enter_safe_zone_timeout_ms(__unused uint32_t timeout_ms) { + int rc = PICO_OK; + if (!use_irq_only()) { +#if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP + // Note that whilst taskENTER_CRITICAL sounds promising (and on non SMP it disabled IRQs), on SMP + // it only prevents the other core from also entering a critical section. + // Therefore, we must do our own handshake which starts a task on the other core and has it disable interrupts + uint core_num = get_core_num(); + // create at low priority + TaskHandle_t task_handle; + if (pdPASS != xTaskCreate(flash_lockout_task, "flash lockout", configMINIMAL_STACK_SIZE, (void *)core_num, 0, &task_handle)) { + return PICO_ERROR_INSUFFICIENT_RESOURCES; + } + lockout_state[core_num] = FREERTOS_LOCKOUT_LOCKER_WAITING; + __sev(); + // bind to other core + vTaskCoreAffinitySet(task_handle, 1u << (core_num ^ 1)); + // and make it super high priority + vTaskPrioritySet(task_handle, configMAX_PRIORITIES -1); + absolute_time_t until = make_timeout_time_ms(timeout_ms); + while (lockout_state[core_num] != FREERTOS_LOCKOUT_LOCKEE_READY && !time_reached(until)) { + __wfe(); // we don't bother to try to let lower priority tasks run + } + if (lockout_state[core_num] != FREERTOS_LOCKOUT_LOCKEE_READY) { + lockout_state[core_num] = FREERTOS_LOCKOUT_LOCKER_DONE; + rc = PICO_ERROR_TIMEOUT; + } + // todo we may get preempted here, but I think that is OK unless what is pre-empts requires + // the other core to be running. +#elif PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT + // we cannot mix multicore_lockout and FreeRTOS as they both use the multicore FIFO... + // the user, will have to roll their own mechanism in this case. +#if LIB_FREERTOS_KERNEL +#if PICO_FLASH_ASSERT_ON_UNSAFE + assert(false); // we expect the other core to have been initialized via flash_safe_execute_core_init() + // unless PICO_FLASH_ASSUME_COREX_SAFE is set +#endif + rc = PICO_ERROR_NOT_PERMITTED; +#else // !LIB_FREERTOS_KERNEL + if (core_initialized[get_core_num()^1]) { + if (!multicore_lockout_start_timeout_us(timeout_ms * 1000ull)) { + rc = PICO_ERROR_TIMEOUT; + } + } else { +#if PICO_FLASH_ASSERT_ON_UNSAFE + assert(false); // we expect the other core to have been initialized via flash_safe_execute_core_init() + // unless PICO_FLASH_ASSUME_COREX_SAFE is set +#endif + rc = PICO_ERROR_NOT_PERMITTED; + } +#endif // !LIB_FREERTOS_KERNEL +#else + // no support for making other core safe provided, so fall through to irq + // note this is the case for a regular single core program +#endif + } + if (rc == PICO_OK) { + // we always want to disable IRQs on our core + irq_state[get_core_num()] = save_and_disable_interrupts(); + } + return rc; +} + +static int default_exit_safe_zone_timeout_ms(__unused uint32_t timeout_ms) { + // assume if we're exiting we're called then entry happened successfully + restore_interrupts(irq_state[get_core_num()]); + if (!use_irq_only()) { +#if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP + uint core_num = get_core_num(); + lockout_state[core_num] = FREERTOS_LOCKOUT_LOCKER_DONE; + __sev(); + absolute_time_t until = make_timeout_time_ms(timeout_ms); + while (lockout_state[core_num] != FREERTOS_LOCKOUT_LOCKEE_DONE && !time_reached(until)) { + __wfe(); // we don't bother to try to let lower priority tasks run + } + if (lockout_state[core_num] != FREERTOS_LOCKOUT_LOCKEE_DONE) { + return PICO_ERROR_TIMEOUT; + } +#elif PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT + return multicore_lockout_end_timeout_us(timeout_ms * 1000ull) ? PICO_OK : PICO_ERROR_TIMEOUT; +#endif + } + return PICO_OK; +} \ No newline at end of file diff --git a/src/rp2_common/pico_flash/include/pico/flash.h b/src/rp2_common/pico_flash/include/pico/flash.h new file mode 100644 index 000000000..aec4d9dbe --- /dev/null +++ b/src/rp2_common/pico_flash/include/pico/flash.h @@ -0,0 +1,130 @@ +/* + * Copyright (c) 2023 Raspberry Pi (Trading) Ltd. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#ifndef _PICO_FLASH_H +#define _PICO_FLASH_H + +#include "pico.h" + +#include "hardware/flash.h" +#include "pico/time.h" + +/** \file pico/flash.h + * \defgroup pico_flash pico_flash + * + * High level flash API + * + * Flash cannot be erased or written to when in XIP mode. However the system cannot directly access memory in the flash + * address space when not in XIP mode. + * + * It is therefore critical that no code or data is being read from flash while flash is been written or erased. + * + * If only one core is being used, then the problem is simple - just disable interrupts; however if code is running on + * the other core, then it has to be asked, nicely, to avoid flash for a bit. This is hard to do if you don't have + * complete control of the code running on that core at all times. + * + * This library provides a \ref flash_safe_execute method which calls a function back having sucessfully gotten + * into a state where interrupts are disabled, and the other core is not executing or reading from flash. + * + * How it does this is dependent on the supported environment (Free RTOS SMP or pico_multicore). Additionally + * the user can provide their own mechanism by providing a strong definition of \ref get_flash_safety_helper(). + * + * Using the default settings, flash_safe_execute will only call the callback function if the state is safe + * otherwise returning an error (or an assert depending on \ref PICO_FLASH_ASSERT_ON_UNSAFE). + * + * There are conditions where safety would not be guaranteed + * + * 1. FreeRTOS smp with `configNUM_CORES=1` - FreeRTOS still uses pico_multicore in this case, so \ref flash_safe_execute + * cannot know what the other core is doing, and there is no way to force code execution between a FreeRTOS core + * and a non FreeRTOS core. + * 2. FreeRTOS non SMP with pico_multicore - Again, there is no way to force code execution between a FreeRTOS core and + * a non FreeRTOS core. + * 3. pico_multicore without \ref flash_safe_execute_core_init() having been called on the other core - The + * \ref flash_safe_execute method does not know if code is executing on the other core, so it has to assume it is. Either + * way, it is not able to intervene if \ref flash_safe_execute_core_init() has not been called on the other core. + * + * Fortunately, all is not lost in this situation, you may: + * + * * Set \ref PICO_FLASH_ASSUME_CORE0_SAFE=1 to explicitly say that core 0 is never using flash. + * * Set \ref PICO_FLASH_ASSUME_CORE1_SAFE=1 to explicitly say that core 1 is never using flash. + */ + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Initialize a core such that the other core can lock it out during \ref flash_safe_execute. + * + * \note This is not necessary for FreeRTOS SMP, but should be used when launching via \ref multicore_launch_core1 + * \return true on success; there is no need to call \ref flash_safe_execute_core_deinit() on failure. + */ +bool flash_safe_execute_core_init(void); + +/** + * De-initialize work done by \ref flash_safe_execute_core_init + * \return true on success + */ +bool flash_safe_execute_core_deinit(void); + +/** + * + * \param func + * \param param + * \param enter_exit_timeout_ms + * + * \return PICO_OK on success (the function will have been called). + * PICO_TIMEOUT on timeout (the function may have been called). + * PICO_ERROR_NOT_PERMITTED if safe execution is not possible (the function will not have been called). + * PICO_ERROR_INSUFFICIENT_RESOURCES if the method fails due to dynamic resource exhaustion (the function will not have been called) + * \note if \ref PICO_FLASH_ASSERT_ON_UNSAFE is 1, this function will assert in debug mode vs returning + * PICO_ERROR_NOT_PERMITTED + */ +int flash_safe_execute(void (*func)(void *), void *param, uint32_t enter_exit_timeout_ms); + +// PICO_CONFIG: PICO_FLASH_ASSERT_ON_UNSAFE, Assert in debug mode rather than returning an error if flash_safe_execute cannot guarantee safety to catch bugs early, type=bool, default=1, group=pico_flash +#ifndef PICO_FLASH_ASSERT_ON_UNSAFE +#define PICO_FLASH_ASSERT_ON_UNSAFE 1 +#endif + +// PICO_CONFIG: PICO_FLASH_ASSUME_CORE0_SAFE, Assume that core 0 will never be accessing flash and so doesn't need to be considered during flash_safe_execute, type=bool, default=0, group=pico_flash +// PICO_CONFIG: PICO_FLASH_ASSUME_CORE1_SAFE, Assume that core 1 will never be accessing flash and so doesn't need to be considered during flash_safe_execute, type=bool, default=0, group=pico_flash + +// PICO_CONFIG: PICO_FLASH_SAFE_EXECUTE_SUPPORT_FREERTOS_SMP, Support using FreeRTOS SMP to make the other core safe during flash_safe_execute, type=bool, default=1 when using FreeRTOS SMP, group=pico_flash +#ifndef PICO_FLASH_SAFE_EXECUTE_SUPPORT_FREERTOS_SMP +#if LIB_FREERTOS_KERNEL && FREE_RTOS_KERNEL_SMP // set by RP2040 SMP port +#define PICO_FLASH_SAFE_EXECUTE_SUPPORT_FREERTOS_SMP 1 +#endif +#endif + +// PICO_CONFIG: PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT, Support using multicore_lockout functions to make the other core safe during flash_safe_execute, type=bool, default=1 when using pico_multicore, group=pico_flash +#ifndef PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT +#if LIB_PICO_MULTICORE +#define PICO_FLASH_SAFE_EXECUTE_PICO_SUPPORT_MULTICORE_LOCKOUT 1 +#endif +#endif + +typedef struct { + bool (*core_init_deinit)(bool init); + int (*enter_safe_zone_timeout_ms)(uint32_t timeout_ms); + int (*exit_safe_zone_timeout_ms)(uint32_t timeout_ms); +} flash_safety_helper_t; + +/** + * Internal method to return the flash safety helper implementation. + * + * Advanced users can provide their own implementation of this function to perform + * different inter-core coordination before disabling XIP mode. + * + * @return the \ref flash_safety_helper_t + */ +flash_safety_helper_t *get_flash_safety_helper(void); + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/src/rp2_common/pico_multicore/multicore.c b/src/rp2_common/pico_multicore/multicore.c index 04856efc5..1dc1ba303 100644 --- a/src/rp2_common/pico_multicore/multicore.c +++ b/src/rp2_common/pico_multicore/multicore.c @@ -196,7 +196,7 @@ static void check_lockout_mutex_init(void) { hw_claim_unlock(save); } -void multicore_lockout_victim_init() { +void multicore_lockout_victim_init(void) { check_lockout_mutex_init(); uint core_num = get_core_num(); irq_set_exclusive_handler(SIO_IRQ_PROC0 + core_num, multicore_lockout_handler); @@ -246,7 +246,7 @@ bool multicore_lockout_start_timeout_us(uint64_t timeout_us) { return multicore_lockout_start_block_until(make_timeout_time_us(timeout_us)); } -void multicore_lockout_start_blocking() { +void multicore_lockout_start_blocking(void) { multicore_lockout_start_block_until(at_the_end_of_time); } diff --git a/test/kitchen_sink/CMakeLists.txt b/test/kitchen_sink/CMakeLists.txt index 807ec47a7..8d98603ef 100644 --- a/test/kitchen_sink/CMakeLists.txt +++ b/test/kitchen_sink/CMakeLists.txt @@ -29,6 +29,7 @@ target_link_libraries(kitchen_sink_libs INTERFACE pico_divider pico_double pico_fix_rp2040_usb_device_enumeration + pico_flash pico_float pico_i2c_slave pico_int64_ops diff --git a/test/kitchen_sink/kitchen_sink.c b/test/kitchen_sink/kitchen_sink.c index 3297dd058..e1c23c031 100644 --- a/test/kitchen_sink/kitchen_sink.c +++ b/test/kitchen_sink/kitchen_sink.c @@ -39,6 +39,7 @@ #include "pico/divider.h" #include "pico/double.h" #include "pico/fix/rp2040_usb_device_enumeration.h" +#include "pico/flash.h" #include "pico/float.h" #include "pico/int64_ops.h" #include "pico/i2c_slave.h" From d31f9fe15206a035329b2c8ba279a37dd910bf15 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sun, 4 Jun 2023 18:34:42 -0500 Subject: [PATCH 2/3] oops missing doc lines --- src/rp2_common/pico_flash/include/pico/flash.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/rp2_common/pico_flash/include/pico/flash.h b/src/rp2_common/pico_flash/include/pico/flash.h index aec4d9dbe..07c0aa78e 100644 --- a/src/rp2_common/pico_flash/include/pico/flash.h +++ b/src/rp2_common/pico_flash/include/pico/flash.h @@ -35,7 +35,7 @@ * Using the default settings, flash_safe_execute will only call the callback function if the state is safe * otherwise returning an error (or an assert depending on \ref PICO_FLASH_ASSERT_ON_UNSAFE). * - * There are conditions where safety would not be guaranteed + * There are conditions where safety would not be guaranteed: * * 1. FreeRTOS smp with `configNUM_CORES=1` - FreeRTOS still uses pico_multicore in this case, so \ref flash_safe_execute * cannot know what the other core is doing, and there is no way to force code execution between a FreeRTOS core @@ -71,10 +71,11 @@ bool flash_safe_execute_core_init(void); bool flash_safe_execute_core_deinit(void); /** + * Execute a function with IRQs disabled and with the other core also not executing/reading flash * - * \param func - * \param param - * \param enter_exit_timeout_ms + * \param func the function to call + * \param param the parameter to pass to the function + * \param enter_exit_timeout_ms the timeout for each of the enter/exit phases when coordinating with the other core * * \return PICO_OK on success (the function will have been called). * PICO_TIMEOUT on timeout (the function may have been called). From 4f0737e45262c699696258582d8d256d932322bd Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Tue, 6 Jun 2023 10:42:14 -0500 Subject: [PATCH 3/3] comment updates --- src/rp2_common/pico_btstack/btstack_flash_bank.c | 8 ++++++-- src/rp2_common/pico_flash/flash.c | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/rp2_common/pico_btstack/btstack_flash_bank.c b/src/rp2_common/pico_btstack/btstack_flash_bank.c index 6e1604632..c58b5e724 100644 --- a/src/rp2_common/pico_btstack/btstack_flash_bank.c +++ b/src/rp2_common/pico_btstack/btstack_flash_bank.c @@ -55,7 +55,9 @@ static void pico_flash_bank_erase(void * context, int bank) { .op_is_erase = true, .p0 = PICO_FLASH_BANK_STORAGE_OFFSET + (PICO_FLASH_BANK_SIZE * bank), }; - // todo better timeout + // todo choice of timeout and check return code... currently we have no way to return an error + // to the caller anyway. flash_safe_execute asserts by default on problem other than timeout, + // so that's fine for now, and UINT32_MAX is a timeout of 49 days which seems long enough flash_safe_execute(pico_flash_bank_perform_flash_mutation_operation, &mop, UINT32_MAX); } @@ -141,7 +143,9 @@ static void pico_flash_bank_write(void * context, int bank, uint32_t offset, con .p0 = bank_start_pos + (page * FLASH_PAGE_SIZE), .p1 = (uintptr_t)page_data }; - // todo better timeout + // todo choice of timeout and check return code... currently we have no way to return an error + // to the caller anyway. flash_safe_execute asserts by default on problem other than timeout, + // so that's fine for now, and UINT32_MAX is a timeout of 49 days which seems long enough flash_safe_execute(pico_flash_bank_perform_flash_mutation_operation, &mop, UINT32_MAX); } } diff --git a/src/rp2_common/pico_flash/flash.c b/src/rp2_common/pico_flash/flash.c index b05b12e5c..528ef620c 100644 --- a/src/rp2_common/pico_flash/flash.c +++ b/src/rp2_common/pico_flash/flash.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022 Raspberry Pi (Trading) Ltd. + * Copyright (c) 2023 Raspberry Pi (Trading) Ltd. * * SPDX-License-Identifier: BSD-3-Clause */ @@ -149,7 +149,7 @@ static int default_enter_safe_zone_timeout_ms(__unused uint32_t timeout_ms) { #if PICO_FLASH_SAFE_EXECUTE_USE_FREERTOS_SMP // Note that whilst taskENTER_CRITICAL sounds promising (and on non SMP it disabled IRQs), on SMP // it only prevents the other core from also entering a critical section. - // Therefore, we must do our own handshake which starts a task on the other core and has it disable interrupts + // Therefore, we must do our own handshake which starts a task on the other core and have it disable interrupts uint core_num = get_core_num(); // create at low priority TaskHandle_t task_handle;