Skip to content

IPM fix for esp32s3 targets #80887

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.

Sign up for GitHub

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

Jump to bottom
Merged
mmahadevan108 merged 6 commits into from
Nov 8, 2024
Merged

IPM fix for esp32s3 targets #80887

mmahadevan108 merged 6 commits into from
Nov 8, 2024

Conversation

@marekmatej
Copy link
Contributor

@marekmatej marekmatej commented Nov 5, 2024 •
edited
Loading

This PR fixes how the APPCPU is being built in IPM samples. C array encapsulation is removed and instead normal build procedure is used. Fixing the sysbuild builds as well.

Fixes #80890

@zephyrbot zephyrbot added area: Samples Samples platform: ESP32 Espressif ESP32 area: Clock Control area: IPM Inter-Processor Mailbox area: Xtensa Xtensa Architecture labels Nov 5, 2024
@zephyrbot
Copy link

zephyrbot commented Nov 5, 2024 •
edited
Loading

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_espressif zephyrproject-rtos/hal_espressif@23c17a8 zephyrproject-rtos/hal_espressif@174547e (zephyr) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_espressif DNM This PR should not be merged (Do Not Merge) labels Nov 5, 2024
@marekmatej marekmatej force-pushed the bugfix/amp_s3_sample_basic branch from ac2bc4c to b355dfa Compare November 5, 2024 01:07
uLipe
uLipe reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@uLipe uLipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this being sent.

I just left some initial commits.

@marekmatej marekmatej force-pushed the bugfix/amp_s3_sample_basic branch 5 times, most recently from 9771fe2 to 6433c35 Compare November 5, 2024 12:27
@marekmatej marekmatej requested a review from uLipe November 5, 2024 12:27
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Nov 5, 2024
@marekmatej marekmatej force-pushed the bugfix/amp_s3_sample_basic branch from b50d570 to c17cb8f Compare November 6, 2024 13:31
@marekmatej marekmatej changed the title IPM fix for esp32s3_devkitm target IPM fix for esp32s3 targets Nov 6, 2024
uLipe
uLipe previously approved these changes Nov 6, 2024
View reviewed changes
@marekmatej marekmatej added this to the v4.0.0 milestone Nov 6, 2024
@marekmatej
manifest: update hal_espressif
63564ce 
Fetch latest features to support fixes.

Signed-off-by: Marek Matej <[email protected]>
@marekmatej
dts: esp32s3: shm nodes update
c943fbf 
Align the shared memories with the memory.h layout.
Reorder nodes to show memory related nodes together.

Signed-off-by: Marek Matej <[email protected]>
@marekmatej
boards: esp32s3: AMP support and fixes
166f911 
Fix missing flash and code partition.
Add missing dts entries and use common partition tables to all related
non-Espressif boards, previously ommited.
Add uart1 node in pinctrl for APPCPU.

Signed-off-by: Marek Matej <[email protected]>
@marekmatej
drivers: clock_control: amp clock fix
a4bce3c 
Avoid APPCPU to interact with a clock settings.
Fix warning when LOG_LEVEL_DBG.

Signed-off-by: Marek Matej <[email protected]>
@marekmatej
soc: esp32s3: AMP support
776f0be 
Updates and fixes to support APPCPU.
- fix ld scripts
- fix and update memory layout
- fix build issues
- fix sysbuild

Signed-off-by: Marek Matej <[email protected]>
@marekmatej
samples: drivers: IPM fixes
4f16991 
Updates and fixes to support IPM sample on ESP32:

- fix IPM sample code for APPCPU and PROCPU
- align with memory layout, add flash awarenes
- shell commands to stop/start APPCPU
- reorganize overlays

Signed-off-by: Marek Matej <[email protected]>
@marekmatej marekmatej force-pushed the bugfix/amp_s3_sample_basic branch from c17cb8f to 4f16991 Compare November 6, 2024 21:35
@marekmatej marekmatej requested a review from uLipe November 6, 2024 21:49
@uLipe
Copy link
Member

uLipe commented Nov 6, 2024 •
edited
Loading

@mmahadevan108 any chance to put this into the 4.0?

Thank you :)

@mmahadevan108
Copy link
Contributor

mmahadevan108 commented Nov 6, 2024

pinging @nordicjm for a review from HWV2 side as these samples were broken since the switch to HWV2.

@mmahadevan108
Copy link
Contributor

mmahadevan108 commented Nov 7, 2024

@uLipe . This is a fairly big change. Is everything required for fixing the bug?
It might be easier to pull this into 4.0 if we can focus this PR to only include the changes needed to fix the bug and move the remaining to a separate PR that can be merged post 4.0 release.

@marekmatej
Copy link
Contributor Author

marekmatej commented Nov 8, 2024

This is a fairly big change.

@mmahadevan108 you are right, and the reason it is so hefty is it not only fixes the IPM build itself but also stuff related to HWMv2, eg using sysbuild for multi-image builds.

@uLipe
Copy link
Member

uLipe commented Nov 8, 2024 •
edited
Loading

@mmahadevan108 as @marekmatej explained this change fixes the IPM but unfortunately it was necessary to address the sysbuild and hmv2 stuff to get this working propey, other direction from that would lead to hacky fixes.

Although this change carries lots of change, most of them are espressif localized changes that impact only the esp32 IPM part, the only more generic change is the IPM sample which got simplified which we see low impact on the overall zephyr release, but a significant change for people interested that are waiting the esp32 side IPM to get fixed.

Although we feel this is a specific change that only affects espressif platform, I will understand if you prefer to postpone this change due its size :)

@mmahadevan108
Copy link
Contributor

mmahadevan108 commented Nov 8, 2024

@uLipe @marekmatej thank you for clarifying. This PR needs approval from @kartben as he is the assignee before we can merge

@kartben kartben assigned sylvioalves and unassigned kartben Nov 8, 2024
@marekmatej
Copy link
Contributor Author

marekmatej commented Nov 8, 2024

Now that assignee has been updated, I guess we can proceed as is? @mmahadevan108

@uLipe
Copy link
Member

uLipe commented Nov 8, 2024 •
edited
Loading

I'd say the same thing :)

@mmahadevan108 mmahadevan108 merged commit a8ab8b4 into zephyrproject-rtos:main Nov 8, 2024
25 checks passed
@rftafas rftafas mentioned this pull request Nov 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uLipe uLipe uLipe approved these changes

@sylvioalves sylvioalves sylvioalves approved these changes

@wmrsouza wmrsouza wmrsouza approved these changes

@nordicjm nordicjm nordicjm approved these changes

@andyross andyross Awaiting requested review from andyross

@ceolin ceolin Awaiting requested review from ceolin

@dcpleung dcpleung Awaiting requested review from dcpleung

@kartben kartben Awaiting requested review from kartben

@LucasTambor LucasTambor Awaiting requested review from LucasTambor

@nashif nashif Awaiting requested review from nashif

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@raffarost raffarost Awaiting requested review from raffarost

Assignees

@sylvioalves sylvioalves

Labels

area: Clock Control area: IPM Inter-Processor Mailbox area: Samples Samples area: Xtensa Xtensa Architecture manifest manifest-hal_espressif platform: ESP32 Espressif ESP32

Projects

None yet

Milestone

v4.0.0

Development

Successfully merging this pull request may close these issues.

IPM sample for esp32s3_devkitm remote build error

8 participants

@marekmatej @zephyrbot @uLipe @mmahadevan108 @sylvioalves @wmrsouza @nordicjm @kartben