Added support for board nucleo u545re q#12
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for the ST Nucleo-U545RE-Q board by introducing new STM32U5 chip support crates (stm32u5xx + stm32u545) and wiring up a board target (nucleo_u545re_q) to boot Tock, provide console I/O over USART1, basic GPIO/EXTI for button/LED, and a TIM2-backed Alarm.
Changes:
- Added new
stm32u5xxchip crate implementing RCC, GPIO/EXTI, TIM2 alarm, USART1, and GPDMA-based TX plumbing. - Added
stm32u545chip wrapper crate and a newnucleo_u545re_qboard target (linker layout, kernel main, flashing helpers). - Registered new workspace members and updated board listings.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| chips/stm32u5xx/src/usart.rs | New USART implementation with DMA TX and IRQ-driven RX FIFO. |
| chips/stm32u5xx/src/tim.rs | New TIM2-based Time/Alarm implementation. |
| chips/stm32u5xx/src/rcc.rs | Minimal RCC clock-enable helpers for STM32U5 peripherals. |
| chips/stm32u5xx/src/lib.rs | New STM32U5 crate root (vectors + init + peripheral bundle). |
| chips/stm32u5xx/src/gpio.rs | New GPIO pin/port implementation with EXTI integration. |
| chips/stm32u5xx/src/exti.rs | New EXTI implementation with line routing/masking and client dispatch. |
| chips/stm32u5xx/src/dma.rs | New GPDMA register mapping and USART1 TX/RX setup routines. |
| chips/stm32u5xx/src/chip.rs | New chip-level interrupt dispatch and Chip trait implementation. |
| chips/stm32u5xx/Cargo.toml | Declares the new stm32u5xx crate and dependencies. |
| chips/stm32u545/src/lib.rs | New STM32U545 wrapper crate re-exporting stm32u5xx. |
| chips/stm32u545/Cargo.toml | Declares the new stm32u545 crate and dependency on stm32u5xx. |
| chips/stm32f4xx/src/gpio.rs | Minor formatting-only change in existing STM32F4 GPIO enum. |
| Cargo.toml | Adds new board and chip crates to the workspace members list. |
| Cargo.lock | Adds lock entries for the new board/chip crates. |
| boards/README.md | Registers the new Nucleo-U545RE-Q (skeleton) board in the boards list. |
| boards/nucleo_u545re_q/src/main.rs | New board main: sets up peripherals, capsules, NVIC routing, and process loading. |
| boards/nucleo_u545re_q/src/io.rs | Board debug/panic writer using a polled USART1 write loop. |
| boards/nucleo_u545re_q/Makefile | Adds probe-rs and J-Link flashing/build helpers (kernel + merged app). |
| boards/nucleo_u545re_q/layout.ld | Board linker layout including appmem symbols and stack override. |
| boards/nucleo_u545re_q/flash.jlink | J-Link script for loading the merged binary at the secure flash alias. |
| boards/nucleo_u545re_q/chip_layout.ld | Defines secure-alias memory map for kernel/apps/ram regions. |
| boards/nucleo_u545re_q/Cargo.toml | Declares the new board crate and capsule/component dependencies. |
| boards/nucleo_u545re_q/.cargo/config.toml | Sets the build target and includes shared Tock cargo flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn handle_deferred_call(&self) { | ||
| // 1. Transmit Completion (DMA) | ||
| if let Some(buf) = self.tx_buffer.take() { | ||
| let len = self.tx_len.get(); | ||
| self.tx_client.map(move |client| { | ||
| client.transmitted_buffer(buf, len, Ok(())); | ||
| }); | ||
| } | ||
|
|
||
| // 2. Receive Completion (FIFO) | ||
| self.try_receive_from_fifo(); | ||
| } |
There was a problem hiding this comment.
handle_deferred_call() unconditionally takes tx_buffer and signals transmitted_buffer(). Because the deferred call is also scheduled from the RX IRQ path, an RX interrupt can cause an in-progress DMA TX to be reported as complete early and return the buffer while DMA is still reading it (potential memory corruption). Track TX completion separately (e.g., a tx_done flag set only in the DMA TX ISR) and only return the TX buffer when the DMA completion interrupt has occurred.
| while regs.isr.is_set(ISR::RXNE) { | ||
| let byte = regs.rdr.get() as u8; | ||
| let mut f = self.fifo.get(); | ||
| let w = self.fifo_write.get(); | ||
| f[w % 32] = byte; | ||
| self.fifo.set(f); | ||
| self.fifo_write.set(w + 1); | ||
| data_received = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The RX FIFO bookkeeping has no overflow handling: fifo_write increments without bound and bytes are always written to f[w % 32], overwriting unread data when more than 32 bytes arrive before the reader drains the FIFO. Consider implementing a true ring buffer with a bounded count (or dropping/flagging overflow) so data loss is deterministic and detectable.
| while regs.isr.is_set(ISR::RXNE) { | |
| let byte = regs.rdr.get() as u8; | |
| let mut f = self.fifo.get(); | |
| let w = self.fifo_write.get(); | |
| f[w % 32] = byte; | |
| self.fifo.set(f); | |
| self.fifo_write.set(w + 1); | |
| data_received = true; | |
| } | |
| let mut f = self.fifo.get(); | |
| let mut r = self.fifo_read.get(); | |
| let mut w = self.fifo_write.get(); | |
| // Keep the counters bounded while preserving the number of unread bytes. | |
| while r >= f.len() && w >= f.len() { | |
| r -= f.len(); | |
| w -= f.len(); | |
| } | |
| self.fifo_read.set(r); | |
| while regs.isr.is_set(ISR::RXNE) { | |
| let byte = regs.rdr.get() as u8; | |
| if (w - r) < f.len() { | |
| f[w % f.len()] = byte; | |
| w += 1; | |
| data_received = true; | |
| } else { | |
| // FIFO full: drop the new byte rather than overwrite unread data. | |
| } | |
| } | |
| self.fifo.set(f); | |
| self.fifo_write.set(w); |
There was a problem hiding this comment.
I changed to dma rx
| dma.clear_interrupt(self.dma_channel_tx.get()); | ||
| }); | ||
| self.registers.cr3.modify(CR3::DMAT::CLEAR); | ||
| self.deferred_call.set(); |
There was a problem hiding this comment.
handle_dma_interrupt(false) is a no-op. Since the chip interrupt table enables a DMA RX IRQ and routes it here, this can leave the DMA RX interrupt pending and retriggering continuously. Either clear the RX channel interrupt flags in the !is_tx branch (and/or disable the RX IRQ if unused).
| self.deferred_call.set(); | |
| self.deferred_call.set(); | |
| } else { | |
| self.dma.map(|dma| { | |
| dma.clear_interrupt(self.dma_channel_rx.get()); | |
| }); |
| // Clear pending flags for this line | ||
| self.registers.rpr1.set(1 << line); | ||
| self.registers.fpr1.set(1 << line); | ||
|
|
||
| // Notify the client directly | ||
| if line < 16 { | ||
| self.clients[line].map(|client| { | ||
| client.fired(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
handle_interrupt() writes 1 << line to clear pending bits before validating line < 16. If this is ever called with an out-of-range value, the shift can panic in debug builds or behave unexpectedly. Perform the bounds check before any shifts/writes.
| // Clear pending flags for this line | |
| self.registers.rpr1.set(1 << line); | |
| self.registers.fpr1.set(1 << line); | |
| // Notify the client directly | |
| if line < 16 { | |
| self.clients[line].map(|client| { | |
| client.fired(); | |
| }); | |
| } | |
| if line >= 16 { | |
| return; | |
| } | |
| // Clear pending flags for this line | |
| self.registers.rpr1.set(1 << line); | |
| self.registers.fpr1.set(1 << line); | |
| // Notify the client directly | |
| self.clients[line].map(|client| { | |
| client.fired(); | |
| }); |
There was a problem hiding this comment.
Agree to this. if you want to avoid an if, define an enum that does not exceed the possible values.
There was a problem hiding this comment.
Changed to pub fn handle_interrupt(&self, line: LineId) and added let line_num = line as usize;. This way, we are sure that line_num < 16.
| client: OptionalCell<&'a dyn time::AlarmClient>, | ||
| } | ||
|
|
||
| impl<'a> Tim2<'a> { | ||
| pub const fn new(base: StaticRef<TimRegisters>) -> Tim2<'a> { | ||
| Tim2 { | ||
| registers: base, | ||
| client: OptionalCell::empty(), | ||
| } | ||
| } | ||
|
|
||
| pub fn enable_clock(&self) { | ||
| // Secure Alias for RCC_APB1ENR1 | ||
| let rcc_apb1enr1 = 0x46020C9C as *mut u32; | ||
| unsafe { | ||
| core::ptr::write_volatile(rcc_apb1enr1, core::ptr::read_volatile(rcc_apb1enr1) | 1); | ||
| } |
There was a problem hiding this comment.
enable_clock() directly reads/writes a hard-coded RCC register address via raw pointers. This duplicates rcc::Rcc::enable_tim2() and makes the clocking path harder to audit/maintain. Prefer using the existing RCC abstraction (or pass an Rcc reference into the timer) instead of embedding literal addresses here.
| client: OptionalCell<&'a dyn time::AlarmClient>, | |
| } | |
| impl<'a> Tim2<'a> { | |
| pub const fn new(base: StaticRef<TimRegisters>) -> Tim2<'a> { | |
| Tim2 { | |
| registers: base, | |
| client: OptionalCell::empty(), | |
| } | |
| } | |
| pub fn enable_clock(&self) { | |
| // Secure Alias for RCC_APB1ENR1 | |
| let rcc_apb1enr1 = 0x46020C9C as *mut u32; | |
| unsafe { | |
| core::ptr::write_volatile(rcc_apb1enr1, core::ptr::read_volatile(rcc_apb1enr1) | 1); | |
| } | |
| enable_clock: fn(), | |
| client: OptionalCell<&'a dyn time::AlarmClient>, | |
| } | |
| impl<'a> Tim2<'a> { | |
| pub const fn new(base: StaticRef<TimRegisters>, enable_clock: fn()) -> Tim2<'a> { | |
| Tim2 { | |
| registers: base, | |
| enable_clock, | |
| client: OptionalCell::empty(), | |
| } | |
| } | |
| pub fn enable_clock(&self) { | |
| (self.enable_clock)(); |
There was a problem hiding this comment.
Added enable_clock to pub struct Tim2<'a>
| impl<'a> uart::Configure for Usart<'a> { | ||
| fn configure(&self, _params: uart::Parameters) -> Result<(), kernel::ErrorCode> { | ||
| let regs = &*self.registers; | ||
| regs.cr1.modify(CR1::UE::CLEAR); | ||
| regs.presc.set(0); | ||
| regs.brr.set(35); | ||
| regs.icr.set(0x3F); | ||
| // Hybrid: RXNEIE is ENABLED for typing interrupts | ||
| regs.cr1 | ||
| .write(CR1::TE::SET + CR1::RE::SET + CR1::UE::SET + CR1::RXNEIE::SET); | ||
| Ok(()) |
There was a problem hiding this comment.
configure() ignores the supplied uart::Parameters (baud rate, stop bits, parity, width, etc.) and instead hardcodes presc=0 and brr=35. This makes the UART configuration API misleading and will break any caller expecting non-115200 settings (or different clocking). Use params to compute BRR/prescaler and configure CR2 STOP/parity/word length as appropriate, or return NOSUPPORT/INVAL for unsupported modes.
|
|
||
| impl IoWrite for Writer { | ||
| fn write(&mut self, buf: &[u8]) -> usize { | ||
| // Literal Secure Alias for USART1 |
There was a problem hiding this comment.
Is there any reason why we do not use the UART implementation from the chip crate? We cannot assume that the UART has already been configured before a panic.
There was a problem hiding this comment.
I centralized the UART base address as a constant in the chip crate and implemented a synchronous transmit_byte method to ensure panic reporting even when the DMA is unavailable.
| stm32u545::tim::Tim2<'static>, | ||
| >, | ||
| >, | ||
| ipc: kernel::ipc::IPC<4>, |
There was a problem hiding this comment.
Do not implement IPC, it is not working and the kernel receives a None IPC.
| let exti = static_init!( | ||
| stm32u545::exti::Exti<'static>, | ||
| stm32u545::exti::Exti::new(StaticRef::new( | ||
| 0x56022000 as *const stm32u545::exti::ExtiRegisters | ||
| )) | ||
| ); | ||
|
|
||
| let dma1 = static_init!( | ||
| stm32u545::dma::Dma, | ||
| stm32u545::dma::Dma::new(StaticRef::new( | ||
| 0x50020000 as *const stm32u545::dma::DmaRegisters | ||
| )) | ||
| ); | ||
|
|
||
| let usart1 = static_init!( | ||
| stm32u545::usart::Usart<'static>, | ||
| stm32u545::usart::Usart::new(StaticRef::new( | ||
| 0x50013800 as *const stm32u545::usart::UsartRegisters | ||
| )) | ||
| ); |
There was a problem hiding this comment.
I would create init functions for these inside the chip crate.
There was a problem hiding this comment.
Created init function for each of those in their file
| let button_pins = static_init!( | ||
| [( | ||
| &'static kernel::hil::gpio::InterruptValueWrapper<'static, stm32u545::gpio::Pin>, | ||
| kernel::hil::gpio::ActivationMode, | ||
| kernel::hil::gpio::FloatingState | ||
| ); 1], | ||
| [( | ||
| button_pin, | ||
| kernel::hil::gpio::ActivationMode::ActiveHigh, | ||
| kernel::hil::gpio::FloatingState::PullDown | ||
| )] | ||
| ); |
There was a problem hiding this comment.
Please use the button_component_helper! helper macro similar to https://github.com/tock/tock/blob/b45c546122a747f7a7575179473cbb7c7fc048a0/boards/microbit_v2/src/main.rs#L329-L346.
There was a problem hiding this comment.
Changed to use button_component_helper!
| // IRQ Numbers | ||
| const EXTI13_IRQ: u32 = 24; | ||
| const GPDMA1_CH0_IRQ: u32 = 29; | ||
| const GPDMA1_CH1_IRQ: u32 = 30; | ||
| const TIM2_IRQ: u32 = 45; | ||
| const USART1_IRQ: u32 = 61; |
There was a problem hiding this comment.
I suggest putting these in a separate file that only lists the interrupts.
| regs.icr.set(0x0F); | ||
| } | ||
|
|
||
| // 2. Receive Logic: Drain the Hardware FIFO. |
There was a problem hiding this comment.
| // 2. Receive Logic: Drain the Hardware FIFO. | |
| // Receive Logic: Drain the Hardware FIFO. |
What the 2. required?
| // 2. Receive Logic: Drain the Hardware FIFO. | ||
| // Used a 'while' loop here to ensure that every byte currently | ||
| // waiting in the hardware Receive Data Register (RDR) is moved into | ||
| // the software FIFO. In high-speed scenarios, multiple bytes may |
There was a problem hiding this comment.
What do we need the software FIFO? Can't we use the received buffer?
There was a problem hiding this comment.
The reason I implemented a software FIFO was related to the up and down arrows. If I don't use it, I can't get the arrows to work. I was only able to get them to work by using a software FIFO. The arrows are sequences of 3 characters
| /// | ||
| /// This ensures that long-running application callbacks are executed in | ||
| /// the kernel's main loop context rather than within the high-priority | ||
| /// hardware interrupt handler. This prevents the console from blocking |
There was a problem hiding this comment.
Tock uses only bottom halves interrupt handling, everything is a deferred call.
There was a problem hiding this comment.
I used deffered + software fifo to get arrow up and down to work. I switched to dma even for rx and it seems to work fine
| while regs.isr.is_set(ISR::RXNE) { | ||
| let byte = regs.rdr.get() as u8; | ||
| let mut f = self.fifo.get(); | ||
| let w = self.fifo_write.get(); | ||
| f[w % 32] = byte; | ||
| self.fifo.set(f); | ||
| self.fifo_write.set(w + 1); | ||
| data_received = true; | ||
| } | ||
|
|
| base: StaticRef<GpioRegisters>, | ||
| pin: usize, | ||
| exti: &'a Exti<'a>, | ||
| port_id: GpioPortNumber, |
There was a problem hiding this comment.
I suggest using the port
| port_id: GpioPortNumber, | |
| port: Port, |
|
|
||
| //! Named constants for NVIC ids shared across the stm32u5xx family of chips | ||
|
|
||
| #![allow(non_upper_case_globals)] |
There was a problem hiding this comment.
| #![allow(non_upper_case_globals)] |
| return Err((kernel::ErrorCode::BUSY, tx_buffer)); | ||
| } | ||
|
|
||
| if self.dma.is_none() { |
There was a problem hiding this comment.
Please use let else to avoid aouble checking (here and when using map.
| return Err((kernel::ErrorCode::BUSY, rx_buffer)); | ||
| } | ||
|
|
||
| if self.dma.is_none() { |
| /* | ||
| * Define the application memory symbols so the Tock kernel | ||
| * knows where to look for processes at boot. | ||
| */ | ||
| _sappmem = ORIGIN(prog); | ||
| _eappmem = ORIGIN(prog) + LENGTH(prog); | ||
|
|
||
| /* | ||
| * Override _estack after including the generic layout. | ||
| */ | ||
| _estack = ORIGIN(ram) + LENGTH(ram); |
There was a problem hiding this comment.
I don't see this for other boards, do we need it?
There was a problem hiding this comment.
Other boards (like nucleo_f429zi) include this in main.rs as extern "C". I moved it from layout.ld to main.rs
| exti, | ||
| dma1, | ||
| gpio_a: gpio::Port::new( | ||
| StaticRef::new(0x52020000 as *const gpio::GpioRegisters), |
There was a problem hiding this comment.
Please define the StaticRef sa a constant named PORT_A_ADDRESS or similar.
| usart1: &'a usart::Usart<'a>, | ||
| ) -> Self { | ||
| Self { | ||
| rcc: rcc::Rcc::new(StaticRef::new(0x46020C00 as *const rcc::RccRegisters)), |
There was a problem hiding this comment.
| rcc: rcc::Rcc::new(StaticRef::new(0x46020C00 as *const rcc::RccRegisters)), | |
| rcc: rcc::Rcc::new(RCC_BASE), |
| ); | ||
| }); | ||
| } | ||
| Ok(()) |
34b8590 to
07eb27c
Compare
3042c2c to
f64e7b7
Compare
This allows a ring buffer to be safely created with a `ring` of uninitialized memory.
Fix in process standard and in debug writer component.
extract rather than copy?
d41d50e to
42328cd
Compare
Pull Request Overview
This pull request adds support for the Nucleo-U545RE-Q board (STM32U545) to Tock OS. It introduces a new stm32u5xx chip crate designed for the ultra-low-power STM32U5 series, with a specific focus on Secure Mode operation and TrustZone compatibility.
Features implemented:
Testing Strategy
This pull request was tested on a physical Nucleo-U545RE-Q hardware target.
-- Verified kernel boot and console output at 115200 baud via USART1.
-- Verified that the User Button (PC13) triggers asynchronous interrupts to toggle the User LED (PA5).
-- Verified userspace delay_ms functionality using the TIM2-backed Alarm HIL.
-- Validated the GPDMA1 configuration (Request ID 25) for stable background printing.
TODO or Help Wanted
This pull request still needs:
Documentation Updated
Formatting
make prepush.cargo fmt.AI Use