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

mpfs_opensbi_setup: Provide hartid and next_addr as parameters #15590

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

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented Jan 17, 2025

Summary

No reason to keep asking for these over and over. Also fix bumping the CPU booted counter when SBI is enabled.

Impact

MPFS bootloader only, performance optimization and fix issue with g_cpus_booted global variable.

Testing

MPFS

@pussuw pussuw marked this pull request as draft January 17, 2025 10:09
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Jan 17, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 17, 2025

[Experimental Bot, please feedback here]

No, this PR summary does not meet the NuttX requirements. It lacks critical details. Here's why and how to fix it:

Missing Information in Summary:

  • Insufficient Explanation of the "Why": "No reason to keep asking for these over and over" isn't an explanation of the underlying problem. What is being asked repeatedly? Why is it unnecessary? What benefit does removing it bring? (e.g., reduces code size, improves performance, simplifies logic, etc.)
  • Unclear Functional Change: "MPFS bootloader only" is too vague. Which part of the MPFS bootloader is changed? Be specific (e.g., "The network initialization sequence of the MPFS bootloader...", "The CPU boot counter logic in the MPFS bootloader...", etc.)
  • Missing "How": How does the change fix the g_cpus_booted issue? What was the bug, and what's the corrected behavior? How does the change optimize performance? Explain the mechanism (e.g., "Removed redundant checks in the boot sequence," "Optimized a loop in the network initialization," etc.).
  • Missing Issue References: If this relates to any reported issues, link them.

Missing Information in Impact:

  • Specifics on Impact: "performance optimization" and "fix issue with g_cpus_booted global variable" are too general. Quantify the performance improvement if possible (e.g., "Reduces boot time by X milliseconds," "Reduces code size by Y bytes," etc.). Explain the consequences of the g_cpus_booted bug and how the fix resolves them.
  • YES/NO Answers: Use the provided "NO/YES" format for each impact category and elaborate where needed. For example, since this affects performance, the impact on the user is potentially YES. Explain if this is a noticeable improvement or only measurable in specific scenarios.

Missing Information in Testing:

  • Insufficient Detail: "MPFS" is not enough. Specify the build host (OS, CPU architecture, compiler), the target platform (architecture, board, configuration), and the exact steps to reproduce the tests.
  • Missing Logs: Include actual log output before and after the change to demonstrate the fix and performance improvement. Even if the logs are short, they provide valuable evidence.

Example of an Improved Summary:

This PR optimizes the MPFS bootloader's network initialization sequence by removing redundant queries for the MAC address, which were previously performed on each CPU boot. This reduces boot time, especially in multi-core systems.  It also fixes a bug where the `g_cpus_booted` global variable was not incremented correctly when SBI is enabled, leading to inaccurate CPU count reporting. This fixes [NuttX Issue #XXX] (if applicable).

By providing more context and specific details, you make it easier for reviewers to understand and evaluate your changes, leading to a smoother and faster review process.

@pussuw pussuw marked this pull request as ready for review January 17, 2025 10:42
@pussuw pussuw marked this pull request as draft January 17, 2025 11:40
No reason to keep asking for these over and over. Also fix bumping
the CPU booted counter when SBI is enabled.

Must also read g_hart_use_sbi before bumping the CPU booted counter.
@pussuw pussuw force-pushed the mpfs_cpuboot_cntr_fix branch from ef1c63c to 2ef5aa9 Compare January 17, 2025 11:44
@pussuw pussuw marked this pull request as ready for review January 17, 2025 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-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.

3 participants