cpu/rpx0xx: port RIOT to the Raspberry Pi RP2040 MCU #16609
Conversation
|
@benpicco: May I do a first round of squashing? |
|
sure, squash away! |
0c4a2d7 to
cafacf0
Compare
|
This is the diff from before the first squashing attempt and after the second attempt: https://github.com/RIOT-OS/RIOT/compare/86681da759ab8e2f57b3cb7fd2ba890f26148fc8..cafacf07845d3f1d0c6a3665943b75ebd55bbb21 (Sadly I incorrectly manually merged something on the first try.) |
|
The CI is now happy. Time for another round of squashing? |
| @@ -0,0 +1,19 @@ | |||
| PICO_DEBUG ?= | |||
| ROM_LEN ?= 2097152 # = 2 MiB used in the RPi Pico | |||
There was a problem hiding this comment.
since the MCU doesn't come with flash, shouldn't this always be set by the board?
There was a problem hiding this comment.
It is likely that most boards just use the reference flash chip, as the 2nd stage bootloader will then just work. Not every flash ship supports XIP in the same way, requiring other flash chips to use a different 2nd stage bootloader.
The board can still overwrite this value, if it indeed used a different value. IMO having the "reference flash chip" as default is sensible.
:-/ |
you could try #14553 to use SysTick for xtimer |
I took this as an occasion to upload #16627. |
cpu/rpx0xx/periph/uart.c
Outdated
| void uart_poweron(uart_t uart) | ||
| { | ||
| uint32_t reset_bit_mask = (uart) ? RESETS_RESET_uart1_Msk : RESETS_RESET_uart0_Msk; | ||
| periph_reset(reset_bit_mask); | ||
| periph_reset_done(reset_bit_mask); | ||
| } | ||
|
|
||
| void uart_poweroff(uart_t uart) | ||
| { | ||
| uart_deinit_pins(uart); | ||
| periph_reset((uart) ? RESETS_RESET_uart1_Msk : RESETS_RESET_uart0_Msk); | ||
| } |
There was a problem hiding this comment.
Those are again wonderfully underspecified APIs. IMO, we should just drop uart_poweron(). Assume a UART is first initialized, then powered off, then power on again: Should it resume operation in the state before power off? Why should the driver need to sacrifice RAM to back up the configuration, when the user of the UART exactly knows all configuration details?
IMO, we should drop uart_poweron(). uart_init() powers on and initialized the UART, uart_poweroff() powers it off again. There is no use case for uart_poweron().
There was a problem hiding this comment.
Oh, it is actually documented:
After initialization, the UART peripheral should be powered on and active. The UART can later be explicitly put to sleep and woken up by calling the uart_poweron() and uart_poweroff() functions. Once woken up using uart_poweron(), the UART should transparently continue it's previously configured operation.
Just not at the API of the functions. I still wonder why a low level driver is burdened with backing up the device state. Especially since in most case the compiler will not be able to optimize the RAM used for this out, even if it is never used.
|
Please squash! |
|
keep squashing |
cpu/rpx0xx/periph/uart.c
Outdated
| @@ -67,7 +67,7 @@ void _set_symbolrate(uart_t uart, uint32_t baud) | |||
| dev->UARTIBRD.reg = baud_ibrd; | |||
| dev->UARTFBRD.reg = baud_fbrd; | |||
|
|
|||
| io_reg_atomic_set(&(dev->UARTLCR_H.reg), 0); | |||
| dev->UARTLCR_H.reg = 0; | |||
There was a problem hiding this comment.
This shouldn't be needed, actually. (This also explains why this worked with the previous no-op atomic |= 0 operation). Because:
0x0is the reset value of the register. So after the reset which is triggered duringuart_init(), it will have this value anyway- In
uart_mode()the register is overwritten anyway
I'll just drop this.
|
I squashed again and added a fix for I have to check with a logic analyzer to confirm that the Sadly, the UART is sending some garbage upon cold boot. (Warm boot it garbage-free.) And also a gargabe char is received upon boot (no matter if cold or warm boot). It would be nice to get rid of that, if possible. |
|
Tested the UART using diff --git a/tests/periph_uart_mode/Makefile b/tests/periph_uart_mode/Makefile
index 04882515c0..329a1ac637 100644
--- a/tests/periph_uart_mode/Makefile
+++ b/tests/periph_uart_mode/Makefile
@@ -2,8 +2,7 @@ include ../Makefile.tests_common
FEATURES_REQUIRED += periph_uart
FEATURES_REQUIRED += periph_uart_modecfg
-
-USEMODULE += xtimer
+FEATURES_OPTIONAL += periph_timer
# Set this to prevent welcome message from printing and confusing output
CFLAGS+=-DLOG_LEVEL=LOG_NONE
diff --git a/tests/periph_uart_mode/main.c b/tests/periph_uart_mode/main.c
index d15df67ba0..b37d44796b 100644
--- a/tests/periph_uart_mode/main.c
+++ b/tests/periph_uart_mode/main.c
@@ -22,7 +22,9 @@
#include <string.h>
#include <stdlib.h>
+#include "board.h"
#include "periph/uart.h"
+#include "periph_conf.h"
#include "stdio_uart.h"
#include "xtimer.h"
@@ -52,6 +54,25 @@
/* Stores each mode string for printing at the end of the test */
static char mode_strings[TOTAL_OPTIONS][MODE_STR_LEN];
+static void _delay(void)
+{
+ if (IS_USED(MODULE_XTIMER)) {
+ xtimer_usleep(DELAY_US);
+ }
+ else {
+ /*
+ * As fallback for freshly ported boards with no timer drivers written
+ * yet, we just use the CPU to delay execution and assume that roughly
+ * 20 CPU cycles are spend per loop iteration.
+ *
+ * Note that the volatile qualifier disables compiler optimizations for
+ * all accesses to the counter variable. Without volatile, modern
+ * compilers would detect that the loop is only wasting CPU cycles and
+ * optimize it out - but here the wasting of CPU cycles is desired.
+ */
+ for (volatile uint32_t i = 0; i < CLOCK_CORECLOCK / 20; i++) { }
+ }
+}
static void _get_mode(const uart_data_bits_t data_bits,
const uart_parity_t parity, const uart_stop_bits_t stop_bits, char* mode_str) {
@@ -153,7 +174,7 @@ int main(void)
if (status == UART_OK) {
results[ridx] = true;
printf("%s\n", mode_strings[ridx]);
- xtimer_usleep(DELAY_US);
+ _delay();
}
else {
results[ridx] = false;
5 Data Bits, No Parity, 1 Stop Bit5 Data Bits, No Parity, 2 Stop Bit5 Data Bits, Even Parity, 1 Stop Bit5 Data Bits, Even Parity, 2 Stop Bit6 Data Bits, No Parity, 1 Stop Bit7 Data Bits, No Parity, 1 Stop Bit8 Data Bits, No Parity, 1 Stop Bit8 Data Bits, No Parity, 2 Stop Bit8 Data Bits, Even Parity, 1 Stop Bit8 Data Bits, Even Parity, 2 Stop Bit8 Data Bits, Odd Parity, 1 Stop Bit8 Data Bits, Odd Parity, 2 Stop Bit |
|
In addition to all modes now working correctly (without sending one char with the previous config) the single garbage char previously send / transmitted on (cold) boot is now also gone. |
|
Output of the diff --git a/tests/periph_gpio/main.c b/tests/periph_gpio/main.c
index 8011f45dc1..51e8469419 100644
--- a/tests/periph_gpio/main.c
+++ b/tests/periph_gpio/main.c
@@ -167,6 +167,7 @@ static int enable_int(int argc, char **argv)
}
po = atoi(argv[1]);
+ (void)po;
pi = atoi(argv[2]);
status = atoi(argv[3]); |
| /* restore configuration registers */ | ||
| dev->UARTIBRD.reg = uartibrd; | ||
| dev->UARTFBRD.reg = uartfbrd; | ||
| dev->UARTLCR_H.reg = uartlcr_h; | ||
| dev->UARTCR.reg = uartcr; |
There was a problem hiding this comment.
I think this is the only driver that properly implements this API 😄
|
Looks like Murdock tripped over another bug in |
|
Adding the missing |
Co-authored-by: Fabian Hüßler <fabian.huessler@st.ovgu.de>
Co-authored-by: nickw96 <nick.weiler@st.ovgu.de> Co-authored-by: MaestroOnICe <justus.krebs@st.ovgu.de> Co-authored-by: Franz2000 <franz.freitag@st.ovgu.de>
Co-authored-by: Fabian Hüßler <fabian.huessler@st.ovgu.de> Co-authored-by: nickw96 <nick.weiler@st.ovgu.de> Co-authored-by: MaestroOnICe <justus.krebs@st.ovgu.de> Co-authored-by: Franz2000 <franz.freitag@st.ovgu.de>
|
All green :-) |
|
(And I think the failing test previously might just have been the unit tests for |
|
Thanks for the review! And thanks for the automatic GPIO test. It was very helpful in detecting a bug that would likely have gone unnoticed for quite some time! |
Contribution description
This provides:
periph_gpio,periph_gpio_int, andperiph_uartare provided in this PR. Other than that no timers, no SPI, no nothing is provided.rpi-pico), the reference board for the RP2040 MCU, is providedAcknowledgements
This code was developed as part of a software project in a combined effort of students and mentors. Thanks to @nickw96, @MaestroOnICe, and @Franz2000.
Most of the low level code and especially the boot sequence and clock configuration was developed by @fabian18.
Testing procedure
The following test applications will be useful:
examples/defaulttests/periph_gpioIt would be useful to test all of the flashing options:
pico-debug(currently the default option)Issues/PRs references
Nonecloses #15822