-
Notifications
You must be signed in to change notification settings - Fork 8.1k
soc: raspberrypi: rp2350: support AMP builds #93379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
9e014ef
to
7fffa17
Compare
Hello, |
Hello Thomas, I'm glad you offered your help to improve this work. You can see the current AMP work assembled for the RP2350 here: https://github.com/dsseng/zephyr/commits/dss-worktree/ (this is just a single patch out of the series to support AMP). You will need some devicetree and Kconfig knowledge to port all of that to RP2040. What board are you using? If possible, reach out to the |
7fffa17
to
44859f1
Compare
44859f1
to
032f4f7
Compare
Will only enable in core-specific dts. Signed-off-by: Dmitrii Sharshakov <[email protected]>
9fc924b
to
11aa328
Compare
11aa328
to
bc2e230
Compare
Given Twister passes and I am certain this will receive more comments I will fix commit description with the next update to avoid extra pings and CI runs for a text-only change |
bc2e230
to
8c7b9dc
Compare
}; | ||
}; | ||
|
||
uart1_default: uart1_default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not enable uart1 here, as there is no explicit reason to make this the default setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not enable anything, but rather create a definition. Still, the pinctrl definition can be moved to the CPU1 side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a request because rpi_pico has a shield definition,
zephyr/boards/raspberrypi/rpi_pico2/rpi_pico2.dtsi
Lines 28 to 59 in 1b97f4b
pico_header: connector { | |
compatible = "raspberrypi,pico-header"; | |
#gpio-cells = <2>; | |
gpio-map-mask = <0xffffffff 0xffffffc0>; | |
gpio-map-pass-thru = <0 0x3f>; | |
gpio-map = <0 0 &gpio0 0 0>, /* GP0 */ | |
<1 0 &gpio0 1 0>, /* GP1 */ | |
<2 0 &gpio0 2 0>, /* GP2 */ | |
<3 0 &gpio0 3 0>, /* GP3 */ | |
<4 0 &gpio0 4 0>, /* GP4 */ | |
<5 0 &gpio0 5 0>, /* GP5 */ | |
<6 0 &gpio0 6 0>, /* GP6 */ | |
<7 0 &gpio0 7 0>, /* GP7 */ | |
<8 0 &gpio0 8 0>, /* GP8 */ | |
<9 0 &gpio0 9 0>, /* GP9 */ | |
<10 0 &gpio0 10 0>, /* GP10 */ | |
<11 0 &gpio0 11 0>, /* GP11 */ | |
<12 0 &gpio0 12 0>, /* GP12 */ | |
<13 0 &gpio0 13 0>, /* GP13 */ | |
<14 0 &gpio0 14 0>, /* GP14 */ | |
<15 0 &gpio0 15 0>, /* GP15 */ | |
<16 0 &gpio0 16 0>, /* GP16 */ | |
<17 0 &gpio0 17 0>, /* GP17 */ | |
<18 0 &gpio0 18 0>, /* GP18 */ | |
<19 0 &gpio0 19 0>, /* GP19 */ | |
<20 0 &gpio0 20 0>, /* GP20 */ | |
<21 0 &gpio0 21 0>, /* GP21 */ | |
<22 0 &gpio0 22 0>, /* GP22 */ | |
<26 0 &gpio0 26 0>, /* GP26 */ | |
<27 0 &gpio0 27 0>, /* GP27 */ | |
<28 0 &gpio0 28 0>; /* GP28 */ | |
}; |
It is a consideration to ensure that only the default settings need to be considered when creating a shield definition.
https://datasheets.raspberrypi.com/pico/Pico-2-Pinout.pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed uart1
uart1_default: uart1_default { | ||
group1 { | ||
pinmux = <UART1_TX_P24>; | ||
}; | ||
group2 { | ||
pinmux = <UART1_RX_P25>; | ||
input-enable; | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to:
Raspberry Pi Pico Datasheet, page 7
Raspberry Pi Pico 2 Datasheet, page 8
GPIO24 - IP VBUS sense
GPIO25 - OP Connected to user LED
It looks like UART1_TX_P8
and UART1_RX_P9
don't interfere with other default pinctrl definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed uart1
To ensure AMP works, only enable peripherals in the final dts Co-authored-by: Dan Collins <[email protected]> Signed-off-by: Dan Collins <[email protected]> Signed-off-by: Dmitrii Sharshakov <[email protected]>
CPU0 gets flash, UART0, 256KB SRAM CPU1 gets USB, 264KB SRAM (RAM boot only for now) This commit introduces a means to build Zephyr for both Cortex M33 cores of the RP2350, and for CPU0 to boot CPU1. It includes Kconfig changes to build for both cores in an AMP configuration. Co-authored-by: Dmitrii Sharshakov <[email protected]> Signed-off-by: Dan Collins <[email protected]> Signed-off-by: Dmitrii Sharshakov <[email protected]>
This change enables building an image to be stored in a flash partition to be loaded using `image-source`. Signed-off-by: Dmitrii Sharshakov <[email protected]>
We don't want to reinitialise the clock control when we boot the second core - so this commit introduces a means to skip initialisation (i.e. assume it is already initialised). Co-authored-by: Dmitrii Sharshakov <[email protected]> Signed-off-by: Dan Collins <[email protected]> Signed-off-by: Dmitrii Sharshakov <[email protected]>
When configured to do so, the SoC init code will load the CPU1 app, reset CPU1 and boot it into the app. There is also an optional validation method to make sure CPU1 VTOR is valid. Modeled similarly to soc/nordic/nrf54h/soc.c Co-authored-by: Dmitrii Sharshakov <[email protected]> Signed-off-by: Dan Collins <[email protected]> Signed-off-by: Dmitrii Sharshakov <[email protected]>
8c7b9dc
to
a0805a6
Compare
|
socs: | ||
- name: rp2350a | ||
cpuclusters: | ||
- name: m33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't rp2350a/m33 be migrated into rp2350a/m33/cpu0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm keeping the "whole board under one CPU" scenario. Later this could use cores in a symmetric multiprocessing configuration.
AMP is not easy nor suits all apps, so the default should keep all peripherals to one core to not confuse anyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I think the board name should follow the hardware configuration.
If you're not using AMP, I think it should be a variant of cpu0, like rp2350a/m33/cpu0/no_amp
(although the name is an extreme example).
@nordicjm Could we hear your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning behind keeping it as is:
- existing apps build for
rpi_pico2/rp2350a/m33
, why should we deprecate this and move? - AMP apps are rare
- I recall there being a plan for Cortex-M SMP, and RISC-V perhaps already can do SMP. So the "whole" board type is the place we can enable SMP in, allowing tasks to be scheduled on both CPUs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally there is no extra part for the default single core configuration but is for others so smp
for SMP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seemingly just noticed there is m33, m33_cpu0 and m33_cpu1, this is not how it should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm thanks for review. Sorry, could you please explain where should this go? Other examples of such SoCs like nRF54H20 have different cores in cpuclusters:
Lines 40 to 47 in d330e86
- name: nrf54h | |
socs: | |
- name: nrf54h20 | |
cpuclusters: | |
- name: cpuapp | |
- name: cpurad | |
- name: cpuppr | |
- name: cpuflpr |
I'm not aware of any other MCU that can be used both in whole and as two distinct ones, so I cannot find an exactly correct example
@soburi @ajf58 @ThreeEights could you please take another look? This PR seems to have no outstanding work for my side except a need to rebase it on top of MCUboot changes which should be a trivial step to do together with other changes |
// Flash is only assigned to CPU0, CPU1 must | ||
// execute from RAM to avoid access conflicts | ||
// You can add a code-partition | ||
// Then add image-source to the &cpu1_launcher on cpu0 | ||
// zephyr,code-partition = &cpu1_slot0_partition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* */
style comments
sram8: memory@20080000 { | ||
reg = <0x20080000 DT_SIZE_K(4)>; | ||
}; | ||
sram9: memory@20081000 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline between nodes, line 29
select CPU_HAS_ARM_SAU | ||
select CPU_HAS_FPU | ||
|
||
config RP2_REQUIRES_IMAGE_DEFINITION_BLOCK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config RP2_REQUIRES_IMAGE_DEFINITION_BLOCK | |
if SOC_SERIES_RP2350 | |
config RP2_REQUIRES_IMAGE_DEFINITION_BLOCK |
default y if !SOC_RP2350A_M33_CPU1 | ||
# Currently the IDF only supports using the Cortex-M33 cores. Enforce | ||
# this at build configuration time. | ||
depends on SOC_SERIES_RP2350 && CPU_CORTEX_M33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends on SOC_SERIES_RP2350 && CPU_CORTEX_M33 | |
depends on CPU_CORTEX_M33 |
$(dt_chosen_enabled,$(DT_CHOSEN_CODE_PARTITION)) && \ | ||
$(dt_chosen_enabled,$(DT_CHOSEN_SRAM)) | ||
|
||
endif # SOC_RP2350A_M33_CPU1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endif # SOC_RP2350A_M33_CPU1 | |
endif # SOC_SERIES_RP2350 |
config BUILD_OUTPUT_ADJUST_LMA | ||
default "$(dt_chosen_partition_addr_hex,$(DT_CHOSEN_CODE_PARTITION)) - \ | ||
$(dt_chosen_reg_addr_hex,$(DT_CHOSEN_SRAM))" if SOC_RP2350A_M33_CPU1 && \ | ||
$(dt_chosen_enabled,$(DT_CHOSEN_CODE_PARTITION)) && \ | ||
$(dt_chosen_enabled,$(DT_CHOSEN_SRAM)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this goes in Kconfig.defconfig
socs: | ||
- name: rp2350a | ||
cpuclusters: | ||
- name: m33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seemingly just noticed there is m33, m33_cpu0 and m33_cpu1, this is not how it should be
Continuation of #90922
To test:
Or see https://github.com/dsseng/zephyr/commits/dss-worktree/ for integration tests with IPC