-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add bare metal support to NXP targets #14359
Conversation
@harmut01, thank you for your changes. |
@@ -121,6 +121,8 @@ LR_IROM1 m_interrupts_start m_text_start+m_text_size-m_interrupts_start { ; load | |||
} | |||
RW_IRAM1 ((ImageLimit(RW_m_data) == m_data_start) ? ImageLimit(RW_m_data) : +0) EMPTY Heap_Size { ; Heap region growing up | |||
} | |||
ARM_LIB_HEAP AlignExpr(+0, 16) EMPTY Heap_Size{ ; Heap region growing up |
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.
Heap_Size
should probably be calculated as done for other platforms (MBED_RAM_SIZE - RAM_FIXED_SIZE - (AlignExpr(ImageLimit(RW_IRAM1), 16) - MBED_RAM1_START)
) instead of setting it to 0x0400
.
@ARMmbed/team-nxp can you confirm?
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.
@hugueskamba I've had to remove the dynamic calculation of the heap size since it was causing CI to fail. The change I made resulted in a circular definition between Heap_Size and RW_IRAM1.
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.
Can you show me what you had that caused the circular definition?
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 was the original change:
+ #define Heap_Size (m_data_size - Stack_Size - (AlignExpr(ImageLimit(RW_IRAM1), 16) - m_data_start))
- #define Heap_Size 0x0400
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.
@ARMmbed/team-nxp any thoughts?
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
@harmut01 looks like there are some errors to investigate... |
CI restarted (internal CI issue yesterday) |
I realized the CI run 2 days ago, I aborted the current job. @harmut01 The failures are related, please review |
@harmut01 is this ready for CI again? |
Yes @adbridge, we're good to go again! |
Ci started |
Jenkins CI Test : ❌ FAILEDBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Ci restarted |
Jenkins CI Test : ❌ FAILEDBuild Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Timeout error, re-running again |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 5 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
"uARM", | ||
"GCC_ARM", | ||
"IAR" | ||
"GCC_ARM" |
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.
Please do the same for all NXP targets (perhaps as a separate commit). There are inconsistencies where IAR is listed as a supported toolchain but no supported c libs are listed for IAR. See LPC541114 for example.
Modify linker scripts to add heap to memory map, add list of supported c libs and "bare-metal" to supported_application_profiles.
Toolchains should provide support for small c lib, remove IAR from the NXP targets that don't currently do so.
Apologies, this force-push was unintended. It adds a commit that removes IAR from supported tool-chains for targets that don't currently support the small c library with IAR. |
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.
We should aim to get this PR tested and merged after the review comment is addressed.
Thanks @LDong-Arm, I've built the baremetal blinky example locally to confirm the changes work. @0xc0170 can we start CI on this? |
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.
LGTM
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 6 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Summary of changes
Fixes ARMmbed/mbed-os-example-blinky-baremetal#55
Modifies
targets.json
to provide bare metal support to the following targets: LPC1768, LPC54114, and LPC546XX. The corresponding scatter files have also been changed to ensure that a heap load region exists. This pull request also removes LPC1114 µARM dependencies as this tool-chain has been deprecated.Issue raised in #13099.
Impact of changes
None
Migration actions required
None
Documentation
None
Pull request type
Test results
Reviewers