From f3e54414e5771a2463a8a93b4dcbb395584d2fe3 Mon Sep 17 00:00:00 2001 From: Sean Cross Date: Thu, 24 Dec 2020 14:03:10 +0800 Subject: [PATCH 1/2] litex: ensure we don't re-enable interrups during ISR During an interrupt handler, interrupts are implicitly disabled. They will be re-enabled when the interrupt handler returns. Due to some changes that were made, varous calls will re-enable interrupts after they're finished. Examples of this include calling `CALLBACK_CRITICAL_END` and getting the number of ticks with `port_get_raw_ticks()`. This patch prevents this from happening by doing two things: 1. Use standard calls in `port_get_raw_ticks()` to disable and re-enable interrupts, preventing nesting issues, and 2. Increase the nesting count inside `isr()`, reflecting the implicit call that is made by hardware when an interrupt is handled This helps to address #3841. Signed-off-by: Sean Cross --- ports/litex/mphalport.c | 15 +++++++++++++++ ports/litex/supervisor/port.c | 6 ++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/ports/litex/mphalport.c b/ports/litex/mphalport.c index 862f163939..c7369dc050 100644 --- a/ports/litex/mphalport.c +++ b/ports/litex/mphalport.c @@ -44,16 +44,31 @@ void mp_hal_delay_us(mp_uint_t delay) { extern void SysTick_Handler(void); +// This value contains the number of times "common_hal_mcu_disable_interrupts()" +// has been called without calling "common_hal_mcu_enable_interrupts()". Since +// this is the interrupt handler, that means we're handling an interrupt, so +// this value should be `0`. +// +// Interrupts should already be disabled when this handler is running, which means +// this value is logically already `1`. If we didn't do this, then interrupts would +// be prematurely enabled by interrupt handlers that enable and disable interrupts. +extern volatile uint32_t nesting_count; + __attribute__((section(".ramtext"))) void isr(void) { uint8_t irqs = irq_pending() & irq_getmask(); + // Increase the "nesting count". Note: This should be going from 0 -> 1. + nesting_count += 1; #ifdef CFG_TUSB_MCU if (irqs & (1 << USB_INTERRUPT)) usb_irq_handler(); #endif if (irqs & (1 << TIMER0_INTERRUPT)) SysTick_Handler(); + + // Decrease the "nesting count". Note: This should be going from 1 -> 0. + nesting_count -= 1; } mp_uint_t cpu_get_regs_and_sp(mp_uint_t *regs) { diff --git a/ports/litex/supervisor/port.c b/ports/litex/supervisor/port.c index 02617b9af7..9897fa397e 100644 --- a/ports/litex/supervisor/port.c +++ b/ports/litex/supervisor/port.c @@ -32,6 +32,8 @@ #include "irq.h" #include "csr.h" +#include "shared-bindings/microcontroller/__init__.h" + // Global millisecond tick count. 1024 per second because most RTCs are clocked with 32.768khz // crystals. volatile uint64_t raw_ticks = 0; @@ -129,9 +131,9 @@ uint32_t port_get_saved_word(void) { uint64_t port_get_raw_ticks(uint8_t* subticks) { // Reading 64 bits may take two loads, so turn of interrupts while we do it. - irq_setie(false); + common_hal_mcu_disable_interrupts(); uint64_t raw_tick_snapshot = raw_ticks; - irq_setie(true); + common_hal_mcu_enable_interrupts(); return raw_tick_snapshot; } From 2f95cc95a494395658eefed6130b9a8732117482 Mon Sep 17 00:00:00 2001 From: Sean Cross Date: Thu, 24 Dec 2020 14:06:57 +0800 Subject: [PATCH 2/2] litex: move more critical code to RAM The XIP SPI flash on Fomu is slow, which results in certain operations taking a long time. This becomes a problem for time-critical operations such as USB. Move various calls into RAM to improve performance. This includes the call to __modsi3 and __udivsi3 which are used by the supervisor handler to determine if periodic callbacks need to be run. This finishes fixing #3841 Signed-off-by: Sean Cross --- ports/litex/boards/fomu/fomu-spi.ld | 5 +++++ ports/litex/common-hal/microcontroller/__init__.c | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ports/litex/boards/fomu/fomu-spi.ld b/ports/litex/boards/fomu/fomu-spi.ld index 9b6443c673..e7db25b0c5 100644 --- a/ports/litex/boards/fomu/fomu-spi.ld +++ b/ports/litex/boards/fomu/fomu-spi.ld @@ -51,6 +51,11 @@ SECTIONS *(.text.tu_edpt_dir) *(.text.tu_fifo_empty) *(.text.usbd_edpt_busy) + *(.text.usb_irq_handler) + *(.text.supervisor_tick) + *(.text.port_get_raw_ticks) + *(.text.__modsi3) + *(.text.__udivsi3) *(.text.irq_getmask) *(.text.irq_setmask) *(.text.irq_pending) diff --git a/ports/litex/common-hal/microcontroller/__init__.c b/ports/litex/common-hal/microcontroller/__init__.c index 3c91661144..522a41e2e3 100644 --- a/ports/litex/common-hal/microcontroller/__init__.c +++ b/ports/litex/common-hal/microcontroller/__init__.c @@ -59,12 +59,15 @@ void common_hal_mcu_delay_us(uint32_t delay) { volatile uint32_t nesting_count = 0; +__attribute__((section(".ramtext"))) void common_hal_mcu_disable_interrupts(void) { - irq_setie(0); - // __DMB(); + if (nesting_count == 0) { + irq_setie(0); + } nesting_count++; } +__attribute__((section(".ramtext"))) void common_hal_mcu_enable_interrupts(void) { if (nesting_count == 0) { // This is very very bad because it means there was mismatched disable/enables so we