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

arm64: changing the startup of arm64 SMP from serial to parallel #13640

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Sep 26, 2024

Summary

arm64: changing the startup of arm64 SMP from serial to parallel

Impact

arm64

before
text data bss dec hex filename
262624 49985 63638 376247 5bdb7 nuttx

after
text data bss dec hex filename
262176 49921 63630 375727 5bbaf nuttx

size -520

Testing

ostest

…ization

To align with the implementation of ARMv7-A, remove the operation of clearing
interrupts during GIC initialization to avoid losing interrupts during asynchronous startup.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: M The size of the change in this PR is medium labels Sep 26, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 26, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and what needs improvement:

  • Insufficient Summary:

    • Missing "Why": You haven't explained why changing the SMP startup from serial to parallel is necessary. What problem does it solve? What are the benefits (e.g., performance improvement)?
    • Lack of Detail: "changing the startup" is very vague. Be specific about what code is modified and how the parallel startup mechanism works.
    • Missing References: Are there related issues in the NuttX or NuttX Apps repositories?
  • Incomplete Impact Assessment:

    • Limited Scope: Simply stating "arm64" doesn't provide enough information. Specify the impact on:
      • Architectures: Does this only affect arm64?
      • Boards: Are any specific boards affected more than others?
      • Drivers: Are there driver implications?
    • Missing Details: You need to address all other impact points (user impact, build process, documentation, security, compatibility). Even if the answer is "NO," state it explicitly.
  • Inadequate Testing:

    • Vague Host/Target: "ostest" is not descriptive enough. Provide details about:
      • Build Host: Your operating system, CPU architecture, compiler version.
      • Target: The specific ARM64 architecture, board, and configuration you tested on.
    • Missing Logs: You need to include actual testing logs from before and after your changes. This demonstrates how the change works and helps reviewers verify its correctness.

To improve your PR:

  1. Expand your Summary: Provide a clear and concise explanation of the change's purpose, functionality, and any relevant issue references.
  2. Complete the Impact Assessment: Address all impact points with specific details, even if the impact is minimal or non-existent.
  3. Provide Detailed Testing Information: Include your build host and target details along with actual testing logs.

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

Successfully merging this pull request may close these issues.

3 participants