From 0566badcefda3146fbccebade4a9131cde5e6f20 Mon Sep 17 00:00:00 2001 From: etagle Date: Wed, 16 May 2018 16:38:17 -0300 Subject: [PATCH] Add memory barrier, optimal interrupt on-off Disabling an ISR on ARM has 3 instructions of latency. A Memory barrier is REQUIRED to ensure proper and predictable disabling. Memory barriers are expensive, so avoid disabling if already disabled (See https://mcuoneclipse.com/2015/10/16/nvic-disabling-interrupts-on-arm-cortex-m-and-the-need-for-a-memory-barrier-instruction/) --- Marlin/src/HAL/HAL_DUE/DebugMonitor_Due.cpp | 5 +++++ Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp | 10 +++++++++ Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp | 10 +++++++++ Marlin/src/HAL/HAL_DUE/watchdog_Due.cpp | 5 +++++ Marlin/src/HAL/HAL_LPC1768/HAL_timers.h | 5 +++++ Marlin/src/HAL/HAL_LPC1768/LPC1768_PWM.cpp | 21 +++++++++++++++++++ .../HAL/HAL_STM32F4/HAL_timers_STM32F4.cpp | 5 +++++ .../HAL/HAL_STM32F7/HAL_timers_STM32F7.cpp | 5 +++++ .../HAL/HAL_TEENSY35_36/HAL_timers_Teensy.cpp | 21 +++++++++++++++++++ Marlin/src/feature/Max7219_Debug_LEDs.cpp | 4 ---- Marlin/src/module/temperature.cpp | 2 -- 11 files changed, 87 insertions(+), 6 deletions(-) diff --git a/Marlin/src/HAL/HAL_DUE/DebugMonitor_Due.cpp b/Marlin/src/HAL/HAL_DUE/DebugMonitor_Due.cpp index d4c21c9810..51064f9ba2 100644 --- a/Marlin/src/HAL/HAL_DUE/DebugMonitor_Due.cpp +++ b/Marlin/src/HAL/HAL_DUE/DebugMonitor_Due.cpp @@ -46,6 +46,11 @@ static void TXBegin(void) { // Disable UART interrupt in NVIC NVIC_DisableIRQ( UART_IRQn ); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + // Disable clock pmc_disable_periph_clk( ID_UART ); diff --git a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp b/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp index 3a2e8fb3cf..eb23692b44 100644 --- a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp +++ b/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp @@ -99,6 +99,11 @@ void HAL_timer_start(const uint8_t timer_num, const uint32_t frequency) { // Disable interrupt, just in case it was already enabled NVIC_DisableIRQ(irq); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + // Disable timer interrupt tc->TC_CHANNEL[channel].TC_IDR = TC_IDR_CPCS; @@ -133,6 +138,11 @@ void HAL_timer_enable_interrupt(const uint8_t timer_num) { void HAL_timer_disable_interrupt(const uint8_t timer_num) { IRQn_Type irq = TimerConfig[timer_num].IRQ_Id; NVIC_DisableIRQ(irq); + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); } // missing from CMSIS: Check if interrupt is enabled or not diff --git a/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp b/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp index 8413c002ab..ac36606308 100644 --- a/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp +++ b/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp @@ -245,6 +245,11 @@ // Disable UART interrupt in NVIC NVIC_DisableIRQ( HWUART_IRQ ); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + // Disable clock pmc_disable_periph_clk( HWUART_IRQ_ID ); @@ -290,6 +295,11 @@ // Disable UART interrupt in NVIC NVIC_DisableIRQ( HWUART_IRQ ); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + pmc_disable_periph_clk( HWUART_IRQ_ID ); } diff --git a/Marlin/src/HAL/HAL_DUE/watchdog_Due.cpp b/Marlin/src/HAL/HAL_DUE/watchdog_Due.cpp index 79081f43a2..3467fdd942 100644 --- a/Marlin/src/HAL/HAL_DUE/watchdog_Due.cpp +++ b/Marlin/src/HAL/HAL_DUE/watchdog_Due.cpp @@ -68,6 +68,11 @@ void watchdogSetup(void) { // Disable WDT interrupt (just in case, to avoid triggering it!) NVIC_DisableIRQ(WDT_IRQn); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + // Initialize WDT with the given parameters WDT_Enable(WDT, value); diff --git a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h b/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h index a4b5bbee94..72b19b9fa9 100644 --- a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h +++ b/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h @@ -143,6 +143,11 @@ FORCE_INLINE static void HAL_timer_disable_interrupt(const uint8_t timer_num) { case 0: NVIC_DisableIRQ(TIMER0_IRQn); // Disable interrupt handler case 1: NVIC_DisableIRQ(TIMER1_IRQn); // Disable interrupt handler } + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); } // This function is missing from CMSIS diff --git a/Marlin/src/HAL/HAL_LPC1768/LPC1768_PWM.cpp b/Marlin/src/HAL/HAL_LPC1768/LPC1768_PWM.cpp index 23cd5798c6..b27d8e1f31 100644 --- a/Marlin/src/HAL/HAL_LPC1768/LPC1768_PWM.cpp +++ b/Marlin/src/HAL/HAL_LPC1768/LPC1768_PWM.cpp @@ -258,6 +258,11 @@ bool LPC1768_PWM_attach_pin(pin_t pin, uint32_t min /* = 1 */, uint32_t max /* = // OK to update the active table because the // ISR doesn't use any of the changed items + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + if (ISR_table_update) //use work table if that's the newest temp_table = work_table; else @@ -342,6 +347,11 @@ bool LPC1768_PWM_detach_pin(pin_t pin) { //// interrupt controlled PWM code NVIC_DisableIRQ(HAL_PWM_TIMER_IRQn); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + if (ISR_table_update) { ISR_table_update = false; // don't update yet - have another update to do NVIC_EnableIRQ(HAL_PWM_TIMER_IRQn); // re-enable PWM interrupts @@ -428,6 +438,12 @@ bool LPC1768_PWM_write(pin_t pin, uint32_t value) { //// interrupt controlled PWM code NVIC_DisableIRQ(HAL_PWM_TIMER_IRQn); + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + if (!ISR_table_update) // use the most up to date table COPY_ACTIVE_TABLE; // copy active table into work table @@ -456,6 +472,11 @@ bool useable_hardware_PWM(pin_t pin) { NVIC_DisableIRQ(HAL_PWM_TIMER_IRQn); + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); + bool return_flag = false; for (uint8_t i = 0; i < NUM_ISR_PWMS; i++) // see if it's already setup if (active_table[i].pin == pin) return_flag = true; diff --git a/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.cpp b/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.cpp index 02c07b1147..1c12f5f4e9 100644 --- a/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.cpp +++ b/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.cpp @@ -123,6 +123,11 @@ void HAL_timer_enable_interrupt(const uint8_t timer_num) { void HAL_timer_disable_interrupt(const uint8_t timer_num) { HAL_NVIC_DisableIRQ(timerConfig[timer_num].IRQ_Id); + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); } hal_timer_t HAL_timer_get_compare(const uint8_t timer_num) { diff --git a/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.cpp b/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.cpp index d5bde97b13..9454b71c81 100644 --- a/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.cpp +++ b/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.cpp @@ -127,6 +127,11 @@ void HAL_timer_enable_interrupt(const uint8_t timer_num) { void HAL_timer_disable_interrupt(const uint8_t timer_num) { HAL_NVIC_DisableIRQ(timerConfig[timer_num].IRQ_Id); + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); } hal_timer_t HAL_timer_get_compare(const uint8_t timer_num) { diff --git a/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.cpp b/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.cpp index 19a8dfc674..d31d9ddf67 100644 --- a/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.cpp +++ b/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.cpp @@ -29,6 +29,22 @@ #include "HAL.h" #include "HAL_timers_Teensy.h" +/** \brief Instruction Synchronization Barrier + Instruction Synchronization Barrier flushes the pipeline in the processor, + so that all instructions following the ISB are fetched from cache or + memory, after the instruction has been completed. +*/ +FORCE_INLINE static void __ISB(void) { + __asm__ __volatile__("isb 0xF":::"memory"); +} + +/** \brief Data Synchronization Barrier + This function acts as a special kind of Data Memory Barrier. + It completes when all explicit memory accesses before this instruction complete. +*/ +FORCE_INLINE static void __DSB(void) { + __asm__ __volatile__("dsb 0xF":::"memory"); +} void HAL_timer_start(const uint8_t timer_num, const uint32_t frequency) { switch (timer_num) { @@ -65,6 +81,11 @@ void HAL_timer_disable_interrupt(const uint8_t timer_num) { case 0: NVIC_DISABLE_IRQ(IRQ_FTM0); break; case 1: NVIC_DISABLE_IRQ(IRQ_FTM1); break; } + + // We NEED memory barriers to ensure Interrupts are actually disabled! + // ( https://dzone.com/articles/nvic-disabling-interrupts-on-arm-cortex-m-and-the ) + __DSB(); + __ISB(); } bool HAL_timer_interrupt_enabled(const uint8_t timer_num) { diff --git a/Marlin/src/feature/Max7219_Debug_LEDs.cpp b/Marlin/src/feature/Max7219_Debug_LEDs.cpp index 616b61c18c..8403f3c18b 100644 --- a/Marlin/src/feature/Max7219_Debug_LEDs.cpp +++ b/Marlin/src/feature/Max7219_Debug_LEDs.cpp @@ -73,7 +73,6 @@ static uint8_t LEDs[8] = { 0 }; #endif void Max7219_PutByte(uint8_t data) { - CRITICAL_SECTION_START; for (uint8_t i = 8; i--;) { SIG_DELAY(); WRITE(MAX7219_CLK_PIN, LOW); // tick @@ -84,12 +83,10 @@ void Max7219_PutByte(uint8_t data) { SIG_DELAY(); data <<= 1; } - CRITICAL_SECTION_END; } void Max7219(const uint8_t reg, const uint8_t data) { SIG_DELAY(); - CRITICAL_SECTION_START; WRITE(MAX7219_LOAD_PIN, LOW); // begin SIG_DELAY(); Max7219_PutByte(reg); // specify register @@ -99,7 +96,6 @@ void Max7219(const uint8_t reg, const uint8_t data) { WRITE(MAX7219_LOAD_PIN, LOW); // and tell the chip to load the data SIG_DELAY(); WRITE(MAX7219_LOAD_PIN, HIGH); - CRITICAL_SECTION_END; SIG_DELAY(); } diff --git a/Marlin/src/module/temperature.cpp b/Marlin/src/module/temperature.cpp index ad944476e3..8e5460f6bb 100644 --- a/Marlin/src/module/temperature.cpp +++ b/Marlin/src/module/temperature.cpp @@ -1085,9 +1085,7 @@ void Temperature::updateTemperaturesFromRawValues() { watchdog_reset(); #endif - CRITICAL_SECTION_START; temp_meas_ready = false; - CRITICAL_SECTION_END; }