cpu/rpx0xx/cmsis: Update vendor header files #19416
Conversation
maribu
added
CI: ready for build
github-actions
bot
added
Area: CI
maribu
added
the
CI: needs squashing
maribu
removed
CI: needs squashing
maribu
force-pushed
the
cpu/rpx0xx/cmsis
branch
from
efedcee
to
9c98f90
Compare
github-actions
bot
added
Area: boards
maribu
force-pushed
the
cpu/rpx0xx/cmsis
branch
2 times, most recently
from
8721c50
to
3239e28
Compare
|
Beware: I dropped the E.g. with /**
* @brief Controls the crystal oscillator (XOSC)
*/
typedef struct { /*!< (@ 0x40024000) XOSC Structure */
union {
__IOM uint32_t reg; /*!< (@ 0x00000000) Crystal Oscillator Control */
struct {
__IOM uint32_t FREQ_RANGE : 12; /*!< [11..0] Frequency range. This resets to 0xAA0 and cannot be
changed. */
__IOM uint32_t ENABLE : 12; /*!< [23..12] On power-up this field is initialised to DISABLE and
the chip runs from the ROSC.
If the chip has subsequently been programmed to run from
the XOSC then setting this field to DISABLE may lock-up
the chip. If this is a concern then run the clk_ref from
the ROSC and enable the clk_sys RESUS feature.
The 12-bit code is intended to give some protection against
accidental writes. An invalid setting will enable the oscillator. */
uint32_t : 8;
} bit;
} CTRL;
union {
__IOM uint32_t reg; /*!< (@ 0x00000004) Crystal Oscillator Status */
struct {
__IM uint32_t FREQ_RANGE : 2; /*!< [1..0] The current frequency range setting, always reads 0 */
uint32_t : 10;
__IM uint32_t ENABLED : 1; /*!< [12..12] Oscillator is enabled but not necessarily running and
stable, resets to 0 */
uint32_t : 11;
__IOM uint32_t BADWRITE : 1; /*!< [24..24] An invalid value has been written to CTRL_ENABLE or
CTRL_FREQ_RANGE or DORMANT */
uint32_t : 6;
__IM uint32_t STABLE : 1; /*!< [31..31] Oscillator is running and stable */
} bit;
} STATUS;
__IOM uint32_t DORMANT; /*!< (@ 0x00000008) Crystal Oscillator pause control
This is used to save power by pausing the
XOSC
On power-up this field is initialised to
WAKE
An invalid write will also select WAKE
WARNING: stop the PLLs before selecting
dormant mode
WARNING: setup the irq before selecting
dormant mode */
union {
__IOM uint32_t reg; /*!< (@ 0x0000000C) Controls the startup delay */
struct {
__IOM uint32_t DELAY : 14; /*!< [13..0] in multiples of 256*xtal_period. The reset value of
0xc4 corresponds to approx 50 000 cycles. */
uint32_t : 6;
__IOM uint32_t X4 : 1; /*!< [20..20] Multiplies the startup_delay by 4. This is of little
value to the user given that the delay can be programmed
directly. */
uint32_t : 11;
} bit;
} STARTUP;
__IM uint32_t RESERVED[3];
union {
__IOM uint32_t reg; /*!< (@ 0x0000001C) A down counter running at the xosc frequency
which counts to zero and stops.
To start the counter write a non-zero value.
Can be used for short software pauses when
setting up time sensitive hardware. */
struct {
__IOM uint32_t COUNT : 8; /*!< [7..0] COUNT */
uint32_t : 24;
} bit;
} COUNT;
} XOSC_Type; /*!< Size = 32 (0x20) */Without it, it looks like this: /**
* @brief Controls the crystal oscillator (XOSC)
*/
typedef struct { /*!< (@ 0x40024000) XOSC Structure */
__IOM uint32_t CTRL; /*!< (@ 0x00000000) Crystal Oscillator Control */
__IOM uint32_t STATUS; /*!< (@ 0x00000004) Crystal Oscillator Status */
__IOM uint32_t DORMANT; /*!< (@ 0x00000008) Crystal Oscillator pause control
This is used to save power by pausing the
XOSC
On power-up this field is initialised to
WAKE
An invalid write will also select WAKE
WARNING: stop the PLLs before selecting
dormant mode
WARNING: setup the irq before selecting
dormant mode */
__IOM uint32_t STARTUP; /*!< (@ 0x0000000C) Controls the startup delay */
__IM uint32_t RESERVED[3];
__IOM uint32_t COUNT; /*!< (@ 0x0000001C) A down counter running at the xosc frequency
which counts to zero and stops.
To start the counter write a non-zero value.
Can be used for short software pauses when
setting up time sensitive hardware. */
} XOSC_Type; /*!< Size = 32 (0x20) */Dropping the But dropping also It makes the diff a bit larger, though. |
Generated new vendor header files from upstream SVD files using:
./SVDConv "$PICO_SDK_DIR"/src/rp2040/hardware_regs/rp2040.svd \
--generate=header --fields=macro --fields=enum
Note: The missing `--fields=struct` flag resulted in the header no
longer containing bit-fields to represent different fields
within registers. While this would generally ease writing code,
the RP2040 has the unpleasant feature of corrupting the
remaining bits of the register when a write access that is not
word-sized occurs in the memory mapped I/O area. This could
happen e.g. when a bit field is byte-sized and byte-aligned.
maribu
force-pushed
the
cpu/rpx0xx/cmsis
branch
from
3239e28
to
bf96c28
Compare
There was a problem hiding this comment.
Looks good.
However the binaries of bf96c28 and 9719bbf are not the same.
tests_periph_timer_periodic_master.elf.zip
tests_periph_timer_periodic.elf.zip
$diff tests_periph_timer_periodic.elf tests_periph_timer_periodic_master.elf
Binärdateien tests_periph_timer_periodic.elf und tests_periph_timer_periodic_master.elf sind verschieden.
Should we really be worried about that? I think if some changes would be missing it would not compile.
There was a problem hiding this comment.
UART and timer seem to work still fine
2023-03-22 21:05:56,678 # START
2023-03-22 21:05:56,685 # main(): This is RIOT! (Version: 2023.04-devel-712-gbf96c-cpu/rpx0xx/cmsis)
2023-03-22 21:05:56,685 #
2023-03-22 21:05:56,688 # Running Timer 0 at 1000000 Hz.
2023-03-22 21:05:56,691 # One counter cycle is 25000 ticks or 25 ms
2023-03-22 21:05:56,694 # Will print 'tick' every cycle.
2023-03-22 21:05:56,694 #
2023-03-22 21:05:56,695 # TEST START
2023-03-22 21:05:56,697 # Running iteration 1 of 3
2023-03-22 21:05:56,723 # [0] tick
2023-03-22 21:05:56,748 # [0] tick
2023-03-22 21:05:56,773 # [0] tick
2023-03-22 21:05:56,798 # [0] tick
2023-03-22 21:05:56,823 # [0] tick
2023-03-22 21:05:56,848 # [0] tick
2023-03-22 21:05:56,874 # [0] tick
2023-03-22 21:05:56,898 # [0] tick
2023-03-22 21:05:56,923 # [0] tick
2023-03-22 21:05:56,948 # [0] tick
2023-03-22 21:05:56,973 # [0] tick
2023-03-22 21:05:56,998 # [0] tick
2023-03-22 21:05:56,999 #
2023-03-22 21:05:56,999 # Cycles:
2023-03-22 21:05:57,000 # channel 0 = 12 [OK]
2023-03-22 21:05:57,003 # Running iteration 2 of 3
2023-03-22 21:05:57,028 # [0] tick
2023-03-22 21:05:57,053 # [0] tick
2023-03-22 21:05:57,078 # [0] tick
2023-03-22 21:05:57,103 # [0] tick
2023-03-22 21:05:57,129 # [0] tick
2023-03-22 21:05:57,153 # [0] tick
2023-03-22 21:05:57,178 # [0] tick
2023-03-22 21:05:57,203 # [0] tick
2023-03-22 21:05:57,229 # [0] tick
2023-03-22 21:05:57,253 # [0] tick
2023-03-22 21:05:57,279 # [0] tick
2023-03-22 21:05:57,303 # [0] tick
2023-03-22 21:05:57,304 #
2023-03-22 21:05:57,304 # Cycles:
2023-03-22 21:05:57,306 # channel 0 = 12 [OK]
2023-03-22 21:05:57,308 # Running iteration 3 of 3
2023-03-22 21:05:57,334 # [0] tick
2023-03-22 21:05:57,359 # [0] tick
2023-03-22 21:05:57,384 # [0] tick
2023-03-22 21:05:57,409 # [0] tick
2023-03-22 21:05:57,434 # [0] tick
2023-03-22 21:05:57,459 # [0] tick
2023-03-22 21:05:57,484 # [0] tick
2023-03-22 21:05:57,509 # [0] tick
2023-03-22 21:05:57,534 # [0] tick
2023-03-22 21:05:57,559 # [0] tick
2023-03-22 21:05:57,584 # [0] tick
2023-03-22 21:05:57,609 # [0] tick
2023-03-22 21:05:57,609 #
2023-03-22 21:05:57,610 # Cycles:
2023-03-22 21:05:57,611 # channel 0 = 12 [OK]
2023-03-22 21:05:58,813 # TEST SUCCEEDED
2023-03-22 21:05:58,819 # { "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 448}]}
gpio also works tests/periph_gpio
2023-03-22 21:13:54,642 # set PA 25
clear s PA 25
2023-03-22 21:14:08,483 # clear PA 25
auto_test PA 14 PA 15
2023-03-22 21:16:29,140 # auto_test PA 14 PA 15
2023-03-22 21:16:29,141 # [START]
2023-03-22 21:16:29,148 # [SUCCESS]
Just remove the zip extension. |
|
I think the diffs in binaries may actually be the code no longer using the bit fields. (It was so for reads, which would be safe even with non-word sized memory accesses.) But maybe we should wait for the soft feature freeze to end before getting this in. If it would change any binaries, I'd say it is as unscary as it gets. @MrKevinWeiss may have an opinion on pushing this through or putting it on a hold for now. |
|
Btw: An alternative cause od action would be to just update the vendor header files but not changing the flags uses to generate them. Maybe this would yield the same binaries and provide the defines needed for USB support. (I still think that at least dropping ansi C compatibility would improve readability, but that is actually a separate concern that doesn't need to be bundled with the update to the new SVD file.) |
|
I haven't done any testing on this cpu yet and if you think it is low risk (ie actually compared a subset of binaries) then I would be ok with it. I always like negative LoC PRs. |
|
bors merge |
|
Build succeeded: |
I suppose that specific to the rp2040? Microchip vendor files also make extensive use of that. |
|
Yes, silently corrupting registers on non-word-sized writes is a special RP2040 "feature". If this would be more widespread, I assume some magic attribute to attach to the structs modeling the register mapping would materialize that allow telling the compiler to only use word-sized writes, even when this means replacing a single byte-wise write by a read-modify-write sequence. |
Contribution description
Generated new vendor header files from upstream SVD files using:
Note: The missing
--fields=structflag resulted in the header no longer containing bit-fields to represent different fields within registers. While this would generally ease writing code, the RP2040 has the unpleasant feature of corrupting the remaining bits of the register when a write access that is not word-sized occurs in the memory mapped I/O area. This could happen e.g. when a bit field is byte-sized and byte-aligned.Testing procedure
No binary changes (hopefully).
Issues/PRs references
This adds a few additional vendor defines, notably for USB. If anyone were to implement USB, this would be a requirement.