Update to latest generated headers - note SVD peripherals section is now alphabetically sorted #2905
No reviewers
Labels
No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-raspberrypi/pico-sdk!2905
Loading…
Reference in a new issue
No description provided.
Delete branch "hardware-header-updates"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
the ordering in the SVD is not meaningful and was random before - it will at least now be stable
I also included all the hardware_structs into kitchen_sink.c to make sure they are all good (not all are included by other libraries)
@ -170,6 +170,46 @@// 0x3d -> Select Timer 2 as TREQ (Optional)// 0x3e -> Select Timer 3 as TREQ (Optional)// 0x3f -> Permanent request, for unpaced transfers.// 0x00 -> Select PIO0's TX FIFO 0 as TREQThere's a comment on line 167 which says "0x0 to 0x3a -> select DREQ n as TREQ". I guess that comment could be removed, now that the values are explicitly enumerated here?
@matiasilva Just out of curiosity, why was the line-wrap length changed? It means that this PR is showing a lot of changes which aren't "real changes" 🫤
@ -47,3 +47,3 @@#define sysinfo_hw ((sysinfo_hw_t *)SYSINFO_BASE)static_assert(sizeof (sysinfo_hw_t) == 0x0014, "");static_assert(sizeof (sysinfo_hw_t) == 0x0044, "");Why the extra padding in the new version?
@ -2493,2 +2493,2 @@// Field : ACCESSCTRL_ADC0_DBG// Description : If 1, ADC0 can be accessed by the debugger, at// Field : ACCESSCTRL_ADC_DBG// Description : If 1, ADC can be accessed by the debugger, atHmmm, in case any code was using the old
ACCESSCTRL_ADC0_OFFSETdefine, should we provide some backwards-compatibility defines? e.g.etc. ? Same question also for the
ACCESSCTRL_RSM_xxx->ACCESSCTRL_PSM_xxxdefines.(Or if not, this is something that we should probably mention in the SDK 2.2.1 release notes)
See earlier question about whether we want to provide "backwards-compatibility defines" for any existing code that might be using the old names.
@ -32,3 +32,3 @@#define PWM_CH0_CSR_PH_ADV_LSB _u(7)#define PWM_CH0_CSR_PH_ADV_ACCESS "SC"#define PWM_CH0_CSR_PH_ADV_ACCESS "WC"// -----------------------------------------------------------------------------Hmmm, description says "Self-clearing. Write a 1, and poll until low." - does that mean that the
SCwas more appropriate thanWC?@ -38,3 +37,4 @@#define SYSINFO_CHIP_ID_MANUFACTURER_RESET _u(0x493)#define SYSINFO_CHIP_ID_MANUFACTURER_BITS _u(0x00000ffe)#define SYSINFO_CHIP_ID_MANUFACTURER_MSB _u(11)#define SYSINFO_CHIP_ID_MANUFACTURER_LSB _u(1)Nice to see that this field now has a reset value listed 👍 But just out of curiosity, why does the
SYSINFO_CHIP_ID_MANUFACTURERhave a different value between RP2040 and RP2350?@ -402,7 +402,7 @@ typedef struct {// 0x00000002 [1] NSP (0) If 1, XOSC can be accessed from a Non-secure, Privileged contextDoes this change mean that https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/cmsis/stub/CMSIS/Device/RP2350/Include/RP2350.h#L1723 also ought to be updated from
ADC0toADC, for consistency?(Oh, looks like that file is auto-generated from
RP2350.svd(which is itself autogenerated), so maybe this PR ought to be updating the CMSIS headers too? 🤔 )@ -79,3 +79,3 @@io_rw_32 scr;_REG_(M33_CCR_OFFSET) // M33_CCRIs it intentional that this appears in both
src/rp2350/hardware_structs/include/hardware/structs/m33.hand alsosrc/rp2350/hardware_structs/include/hardware/structs/scb.h?Whilst this is true for e.g. the GPIO registers, it isn't actually true for the MMFR registers:
However this is such an edge-case that it's probably not worth fixing?
@ -29,2 +28,3 @@// 0xffffffff [31:0] TIMEHW (0x00000000)// Write to bits 63:32 of time +// 0xffffffff [31:0] TIMEHW (0x00000000)io_wo_32 timehw;Hmmm, the
+on the end of this line (and also on lines 33, 38 and 80) doesn't look right?All the extra whitespace changes made it very hard to review the actual changes here; but apart from the things I've noted above it looks like this includes lots of nice little fix-ups 🙂
Did you forget to push those changes to this PR?
Looks like this fixes #2380 too 👍
Nice 👍
yeah, it is annoying once but i'm cool with it
@ -47,3 +47,3 @@#define sysinfo_hw ((sysinfo_hw_t *)SYSINFO_BASE)static_assert(sizeof (sysinfo_hw_t) == 0x0014, "");static_assert(sizeof (sysinfo_hw_t) == 0x0044, "");because it was wrong before ;-)
@ -2493,2 +2493,2 @@// Field : ACCESSCTRL_ADC0_DBG// Description : If 1, ADC0 can be accessed by the debugger, at// Field : ACCESSCTRL_ADC_DBG// Description : If 1, ADC can be accessed by the debugger, atyeah i probably will add them and note them
yuo
@ -402,7 +402,7 @@ typedef struct {// 0x00000002 [1] NSP (0) If 1, XOSC can be accessed from a Non-secure, Privileged contextgood point - i will update those
apparently so; will fix
note - i recommend using a DIFF tool that supports ignoring whitespace!!
@ -170,6 +170,46 @@// 0x3d -> Select Timer 2 as TREQ (Optional)// 0x3e -> Select Timer 3 as TREQ (Optional)// 0x3f -> Permanent request, for unpaced transfers.// 0x00 -> Select PIO0's TX FIFO 0 as TREQI can remove the explicit enumeration, if that's better?
I wrapped to column length of 80, which is standard I think.
@ -32,3 +32,3 @@#define PWM_CH0_CSR_PH_ADV_LSB _u(7)#define PWM_CH0_CSR_PH_ADV_ACCESS "SC"#define PWM_CH0_CSR_PH_ADV_ACCESS "WC"// -----------------------------------------------------------------------------https://github.com/raspberrypi/pico-sdk/issues/1466
@ -79,3 +79,3 @@io_rw_32 scr;_REG_(M33_CCR_OFFSET) // M33_CCRscb.h is a subset of m33.h
The whitespace changes were fixes for the most part, so it had to be done. Unless there are any that I've missed that shouldn't be there?
@ -170,6 +170,46 @@// 0x3d -> Select Timer 2 as TREQ (Optional)// 0x3e -> Select Timer 3 as TREQ (Optional)// 0x3f -> Permanent request, for unpaced transfers.// 0x00 -> Select PIO0's TX FIFO 0 as TREQPlease don't - I think that it's great that these are now explicitly enumerated; sorry if I didn't make that clear.
@ -32,3 +32,3 @@#define PWM_CH0_CSR_PH_ADV_LSB _u(7)#define PWM_CH0_CSR_PH_ADV_ACCESS "SC"#define PWM_CH0_CSR_PH_ADV_ACCESS "WC"// -----------------------------------------------------------------------------Huh, interesting... The person in that issue is complaining that
SCwasn't appropriate because the bit isn't "cleared on the next clock cycle".But Appendix A in the RP2350 datasheet now describes
SCas "Writing a 1 to a bit in an SC field will trigger an event, once the event is triggered the bit clears automatically" (it looks like this was updated in February 2025), and no longer says anything about "next clock cycle". In contrast,WCis described as "Writing a 1 to a bit in a WC field will write that bit to 0" which implies that the bit gets written to 0 immediately?Sorry for moaning - I realise that it's a necessary evil 😉
@ -2493,2 +2493,2 @@// Field : ACCESSCTRL_ADC0_DBG// Description : If 1, ADC0 can be accessed by the debugger, at// Field : ACCESSCTRL_ADC_DBG// Description : If 1, ADC can be accessed by the debugger, atactually changed my mind - this is a lot of clutter for the probably no one who has used them
@ -38,3 +37,4 @@#define SYSINFO_CHIP_ID_MANUFACTURER_RESET _u(0x493)#define SYSINFO_CHIP_ID_MANUFACTURER_BITS _u(0x00000ffe)#define SYSINFO_CHIP_ID_MANUFACTURER_MSB _u(11)#define SYSINFO_CHIP_ID_MANUFACTURER_LSB _u(1)the bit field on RP2040 is actually wrong - it includes bit 0 - the RP2040 constant is doubly wrong though as bit 0 is actually 1 in practice here. Probably worth fixing the RP2040 version (cc @matiasilva )
@ -38,3 +37,4 @@#define SYSINFO_CHIP_ID_MANUFACTURER_RESET _u(0x493)#define SYSINFO_CHIP_ID_MANUFACTURER_BITS _u(0x00000ffe)#define SYSINFO_CHIP_ID_MANUFACTURER_MSB _u(11)#define SYSINFO_CHIP_ID_MANUFACTURER_LSB _u(1)Ahhh, 0x926 >> 1 == 0x493. Looks like the RP2040 version is missing the
SYSINFO_CHIP_ID_STOP_BIT.@ -32,3 +32,3 @@#define PWM_CH0_CSR_PH_ADV_LSB _u(7)#define PWM_CH0_CSR_PH_ADV_ACCESS "SC"#define PWM_CH0_CSR_PH_ADV_ACCESS "WC"// -----------------------------------------------------------------------------I'm still not convinced that it makes sense to change all these PWM fields from
SCtoWC? RP2350 datasheet says:Done in #2913 👍
@ -32,3 +32,3 @@#define PWM_CH0_CSR_PH_ADV_LSB _u(7)#define PWM_CH0_CSR_PH_ADV_ACCESS "SC"#define PWM_CH0_CSR_PH_ADV_ACCESS "WC"// -----------------------------------------------------------------------------I'm pretty sure Luke's comment on this was that it is self clearing it just takes more than one cycle to clear
I decided to summarise what has changed while doing my review. I did not look at the SVD diff as it's too large
Mostly whitespace changes
PWM: It is SC not WC. It just takes more than 1 cycle to clear. This was Luke's comment and the description says it is SC (i.e. you write 1 and it goes to 0 when it's done)
DMA: Added enumval for each TREQ
SIOB renamed to -> SIO in enumvals etc
USB controller changes to WC make sense
M33 ID registers have gone from RW to "-". Don't think this is a big deal
M33 NMI mask description fixed up
OTP: exec2 deleted, mac address added
RSM renamed to PSM in powman regs. May want backwards compatibility defines for these?
TIMER: Agree with Andrew's comment about "// Write to bits 63:32 of time +" looks wrong to have the plus in there
GITREF RP2040 offset. Did this actually change?
Has this actually changed location?
I suggested that in one of my earlier (now hidden) comments. Graham said "this is a lot of clutter for the probably no one who has used them".
No, but I believe it was incorrectly set before
closed in favor of #3050 but i think its probably worth re-reviewing that with regards to comments here
@ -402,7 +402,7 @@ typedef struct {// 0x00000002 [1] NSP (0) If 1, XOSC can be accessed from a Non-secure, Privileged contextFixed in #3050
@ -29,2 +28,3 @@// 0xffffffff [31:0] TIMEHW (0x00000000)// Write to bits 63:32 of time +// 0xffffffff [31:0] TIMEHW (0x00000000)io_wo_32 timehw;Fixed in #3050
Done 👍
Pull request closed