From c3989e398f809dcde2b0ed1b956498a4d9738619 Mon Sep 17 00:00:00 2001 From: Damien George Date: Tue, 2 Jan 2024 01:03:13 +1100 Subject: [PATCH] rp2/rp2_flash: Lockout second core only when doing flash erase/write. Using the multicore lockout feature in the general atomic section makes it much more difficult to get correct. Signed-off-by: Damien George --- ports/rp2/mpthreadport.c | 4 ---- ports/rp2/rp2_flash.c | 31 ++++++++++++++++++++++--------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/ports/rp2/mpthreadport.c b/ports/rp2/mpthreadport.c index 968de1f78..7bffabb33 100644 --- a/ports/rp2/mpthreadport.c +++ b/ports/rp2/mpthreadport.c @@ -52,9 +52,6 @@ uint32_t mp_thread_begin_atomic_section(void) { // When both cores are executing, we also need to provide // full mutual exclusion. mp_thread_mutex_lock(&atomic_mutex, 1); - // In case this atomic section is for flash access, then - // suspend the other core. - multicore_lockout_start_blocking(); } return save_and_disable_interrupts(); @@ -64,7 +61,6 @@ void mp_thread_end_atomic_section(uint32_t state) { restore_interrupts(state); if (core1_entry) { - multicore_lockout_end_blocking(); mp_thread_mutex_unlock(&atomic_mutex); } } diff --git a/ports/rp2/rp2_flash.c b/ports/rp2/rp2_flash.c index b1afe1cbd..35c516416 100644 --- a/ports/rp2/rp2_flash.c +++ b/ports/rp2/rp2_flash.c @@ -70,6 +70,22 @@ bi_decl(bi_block_device( BINARY_INFO_BLOCK_DEV_FLAG_WRITE | BINARY_INFO_BLOCK_DEV_FLAG_PT_UNKNOWN)); +// Flash erase and write must run with interrupts disabled and the other core suspended, +// because the XIP bit gets disabled. +static uint32_t begin_critical_flash_section(void) { + if (multicore_lockout_victim_is_initialized(1 - get_core_num())) { + multicore_lockout_start_blocking(); + } + return save_and_disable_interrupts(); +} + +static void end_critical_flash_section(uint32_t state) { + restore_interrupts(state); + if (multicore_lockout_victim_is_initialized(1 - get_core_num())) { + multicore_lockout_end_blocking(); + } +} + STATIC mp_obj_t rp2_flash_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *all_args) { // Parse arguments enum { ARG_start, ARG_len }; @@ -135,19 +151,17 @@ STATIC mp_obj_t rp2_flash_writeblocks(size_t n_args, const mp_obj_t *args) { mp_buffer_info_t bufinfo; mp_get_buffer_raise(args[2], &bufinfo, MP_BUFFER_READ); if (n_args == 3) { - // Flash erase/program must run in an atomic section because the XIP bit gets disabled. - mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION(); + mp_uint_t atomic_state = begin_critical_flash_section(); flash_range_erase(self->flash_base + offset, bufinfo.len); - MICROPY_END_ATOMIC_SECTION(atomic_state); + end_critical_flash_section(atomic_state); mp_event_handle_nowait(); // TODO check return value } else { offset += mp_obj_get_int(args[3]); } - // Flash erase/program must run in an atomic section because the XIP bit gets disabled. - mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION(); + mp_uint_t atomic_state = begin_critical_flash_section(); flash_range_program(self->flash_base + offset, bufinfo.buf, bufinfo.len); - MICROPY_END_ATOMIC_SECTION(atomic_state); + end_critical_flash_section(atomic_state); mp_event_handle_nowait(); // TODO check return value return mp_const_none; @@ -170,10 +184,9 @@ STATIC mp_obj_t rp2_flash_ioctl(mp_obj_t self_in, mp_obj_t cmd_in, mp_obj_t arg_ return MP_OBJ_NEW_SMALL_INT(BLOCK_SIZE_BYTES); case MP_BLOCKDEV_IOCTL_BLOCK_ERASE: { uint32_t offset = mp_obj_get_int(arg_in) * BLOCK_SIZE_BYTES; - // Flash erase/program must run in an atomic section because the XIP bit gets disabled. - mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION(); + mp_uint_t atomic_state = begin_critical_flash_section(); flash_range_erase(self->flash_base + offset, BLOCK_SIZE_BYTES); - MICROPY_END_ATOMIC_SECTION(atomic_state); + end_critical_flash_section(atomic_state); // TODO check return value return MP_OBJ_NEW_SMALL_INT(0); }