Skip to content
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

arch/armv7-a/r: unifying boot flow #15624

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yf13
Copy link
Contributor

@yf13 yf13 commented Jan 21, 2025

Summary

This tries to unify the boot flow of armv7-a and armv7-r so that nx_start() is directly kicked off from C-level boot function.

This is a breaking change as there maybe downstream armv7-a devices where arm_boot() returns to arm_head.S. They can be updated similar to qemu_boot.c and imx6_boot.c.

Impacts

armv7-a devices.

Testing

  • local checks with qemu-armv7a:nsh, qemu-armv7a:smp, qemu-armv7a:knsh and qemu-armv7r:nsh.
  • CI checks.

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: S The size of the change in this PR is small labels Jan 21, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 21, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is this change necessary? What problem does unifying the boot flow solve? Is it for code clarity, performance improvement, bug fix, or something else? This is the most crucial piece of missing information.
  • What functional part of the code is being changed? Be specific. Mention the files/modules affected (e.g., arch/arm/src/armv7-a/boot.c, arch/arm/src/armv7-r/boot.S).
  • How does the change exactly work? Provide a more detailed explanation of the mechanism used to unify the boot flow. What was the old flow, and what is the new flow?

Missing Information in Impact:

  • Is new feature added? Is existing feature changed? This needs a YES/NO answer. It sounds like an existing feature is being changed.
  • Impact on user: This should be NO/YES, followed by a description if YES. Even if the answer is NO, briefly explain why the user won't be affected.
  • Impact on build, hardware, documentation, security, compatibility: All of these require a NO/YES answer and a description if YES. Even if the impact is NO, provide a brief justification (e.g., "Impact on build: NO, no build system changes are required.").

Missing Information in Testing:

  • Build Host(s): Be specific about your development environment. For example: "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0".
  • Target(s): Provide more detail. Instead of "qemu-armv7a:nsh," specify things like: "QEMU ARMv7-A (Cortex-A15), nsh configuration" or similar.
  • Testing logs: The placeholder logs need to be replaced with actual logs demonstrating the functionality before and after the change. Show that the change works and doesn't introduce regressions. Even a simple "nsh>" prompt before and after can be helpful. If the logs are too extensive, consider providing a link to a CI run or a separate log file.

Example of an improved description:

## Summary

This change unifies the boot flow of ARMv7-A and ARMv7-R architectures to improve code maintainability and reduce redundancy. Previously, the ARMv7-A and ARMv7-R had different low-level boot sequences before reaching `nx_start()`. This PR refactors the `arch/arm/src/armv7-a/boot.c` and `arch/arm/src/armv7-r/boot.S` files to use a common C-level boot function that directly calls `nx_start()`, eliminating assembly language differences in the early boot stage.  This simplifies the codebase and makes it easier to add support for new ARMv7 variants in the future.  Relates to [NuttX Issue #XXX](if applicable).

## Impact

* Is new feature added? NO
* Is existing feature changed? YES, the low-level boot process for ARMv7-A is changed to match the ARMv7-R approach.
* Impact on user: NO, the user experience should be identical. The change is internal to the boot process.
* Impact on build: NO, no build system modifications are necessary.
* Impact on hardware: YES, this change affects ARMv7-A architectures.  ARMv7-R architectures are also affected, but the change aligns them with the new unified approach.  No changes are expected for other architectures.
* Impact on documentation: NO, no documentation updates are required.
* Impact on security: NO, this change does not have any security implications.
* Impact on compatibility: NO, this change maintains backward compatibility.  No changes to user applications or drivers are expected.
* Anything else to consider: None.

## Testing

I confirm that changes are verified on local setup and work as intended:
* Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
* Target(s): QEMU ARMv7-A (Cortex-A15), nsh configuration; QEMU ARMv7-A (Cortex-A9) SMP configuration; QEMU ARMv7-A (Cortex-A15) knsh configuration; QEMU ARMv7-R, nsh configuration

Testing logs before change (qemu-armv7a:nsh):

NuttShell (NSH)
nsh>


Testing logs after change (qemu-armv7a:nsh):

NuttShell (NSH)
nsh>


(Repeat logs for other tested targets)

This improved example provides more context, explains the "why," and includes more specific details about the testing environment and results, making it much more likely to be accepted. Remember to replace the placeholders with your actual information.

@yf13 yf13 force-pushed the unified-armv7-boot-flow branch from c96c087 to e5e3af5 Compare January 21, 2025 07:20
@yf13 yf13 force-pushed the unified-armv7-boot-flow branch from e5e3af5 to 6b6fd23 Compare January 21, 2025 09:28
This attempts to unify the boot flow of armv7-a and armv7-r so that
nx_start() is kicked by C-level arm_boot() directly.

Signed-off-by: Yanfeng Liu <[email protected]>
@yf13 yf13 force-pushed the unified-armv7-boot-flow branch from 6b6fd23 to 12af53a Compare January 21, 2025 09:34

mov lr, #0 /* LR = return address (none) */
Copy link
Contributor

Choose a reason for hiding this comment

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

is it better that we modify armv7-r and armv8-r instead?

@yf13 yf13 marked this pull request as draft January 23, 2025 10:07
@acassis
Copy link
Contributor

acassis commented Jan 24, 2025

@yf13 I think it will impact at least these boards:

pcDuino
Beagle Bone Black
giant-board
jupiter-nano
sama5d3-xplained

Did you try it on these boards?

I have a pcDuino here, I will try it on that board.
@pkarashchenko could you please try it on BBB ?
@adamfeuer could you please test it on jupiter-nano?

@yf13
Copy link
Contributor Author

yf13 commented Jan 25, 2025

Did you try it on these boards?

@acassis, no I didn't because I couldn't. Seeing that nx_start() has about 73+ uses in C and 10 in ASM, plus the comment in armv7-r/arm_head.S and tms570 , I felt that jumping to C only once might be the direction if we want unify them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants