libutils: Add support for stm32mp2 SoC family#205
Conversation
Supports UART and the general purpose 32bit autoreload counters TIM2 and TIM3 to implement timeout and timestamp timers utilities Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
Indanz
left a comment
Indanz
left a comment
There was a problem hiding this comment.
You don't handle 32-bit timer overflows, you probably want to register an IRQ handler for that. Alternatively, you can probably link two timers into one 64-bit timer and ignore overflows.
You also use two hard-coded timers, instead of one. Each timer has four channels which can be used for compare independently. I'd use one for the timeout function and another for overflow detection, if there is no other way. Except if it's one of those stupid timers which can't be reprogrammed on-the-fly without stopping it, in which case you do need to use two timers and document this somewhere. But the datasheet says "The TIMx_CCRx register can be updated at any time by software to control the output waveform, provided that the preload register is not enable", so it seems okay.
But if hard-coding timers like this, it doesn't make much sense to use the DTS walker either, as you know everything already. (Keep it if the code is simpler though.)
| /* official device names */ | ||
| enum chardev_id { | ||
| UART0, | ||
| UART1, |
There was a problem hiding this comment.
Is UART1 actually supported by this driver? If not, leave it out.
The SoC has about 10 UARTs, probably a good idea to follow the datasheet UART names in this enum and elsewhere.
|
|
||
| #pragma once | ||
|
|
||
| #define UART0_PADDR 0x400E0000 |
| #pragma once | ||
|
|
||
| #define UART0_PADDR 0x400E0000 | ||
| #define UART1_PADDR 0x400F0000 |
| * Copyright 2026, STMicroelectronics | ||
| * | ||
| * SPDX-License-Identifier: BSD-2-Clause | ||
| */ |
There was a problem hiding this comment.
It's good to give an overview here of which timers you're using and how and why (if it's more than one).
| #define RCC_APB1DIVR_OFF 0x4B4 | ||
| #define RCC_APB1DIVR_PADDR (RCC_PADDR + RCC_APB1DIVR_OFF) | ||
| #define RCC_APB1DIVR_SIZE 4 |
There was a problem hiding this comment.
Weird indentation. And also unused?
| #include "../../common.h" | ||
| #include <utils/util.h> | ||
|
|
||
| #include "../../chardev.h" |
There was a problem hiding this comment.
Duplicate, and see Julia's comment elsewhere.
| uint32_t tisel; | ||
| uint32_t pad[0xEA]; | ||
| uint32_t ipidr; | ||
| } stm32_regs_t; |
There was a problem hiding this comment.
This structure you can move to timer.c instead, leaving a forward declaration behind. Same for most of the defines. But whatever you prefer.
| static const struct dev_defn dev_defn[] = { | ||
| UART_DEFN(0), | ||
| UART_DEFN(1), | ||
| }; |
There was a problem hiding this comment.
If you use this construction instead of hard-coding a specific UART, any reason not to add all compatible ones?
| #include "../../ltimer.h" | ||
|
|
||
| /* | ||
| * We use two dm timers: one to keep track of an absolute time, the other for timeouts. |
There was a problem hiding this comment.
You're using the TIM2 and TIM3 general purpose timers. What do you mean with "dm"?
Also, each timer seems to have four channels, so why do you need to use two timers?
2 participants
Description
Add support for uart and timers for stm32mp2 soc
Link to kernel PR seL4
Testing
Testing was part of the seL4-test and seL4-bench test suites