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 boot enhancements #14994

Merged
merged 2 commits into from
Nov 29, 2024
Merged

MPFS boot enhancements #14994

merged 2 commits into from
Nov 29, 2024

Conversation

eenurkka
Copy link
Contributor

Summary

rmpfs IPI (Inter-processor Interrupts) are not cleared at boot currently. It's possible they're set at boot time, which then breaks the boot logic; harts1-4 should stay at wfi loop until they're released by an IPI.

In addition, clocks don't need to be reset if they're already set. Let the use configure whether the system clocks are already set or not.

Impact

If there's a chain of NuttX OS:es, for example, one NuttX that jumps to the next, the IPI fired at the 1st NuttX will make the follow-up NuttX proceed before it's released. Thus, clear the IPIs in like manner the interrupts are turned off at boot.

Testing

Various mpfs based products with more than one NuttX OSs following each other. The 1st one sets the clokcs, while the 2nd doesn't alter them.

This CONFIG_MPFS_CLKINIT is set with bootloaders by default. However,
this gives an option to have it unset. In some cases, the clocks
may be already set so it becomes unnecessary to re-initialize them.

Signed-off-by: Eero Nurkkala <[email protected]>
Inter-processor interrupts (IPIs) are not cleared via mie/mip registers but
rather, at the MPFS_CLINT_BASE + mhartid * 4 (a word or 4-byte offset for
each hart).

If there's an IPI waiting, the system will continue to boot altough it's
expected to stay at the wfi loop waiting for the IPI.

Signed-off-by: Eero Nurkkala <[email protected]>
@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 Nov 29, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although the Testing section could be improved.

Here's a breakdown:

  • Summary: Clearly explains the problem (uncleared IPIs causing boot issues), the solution (clearing IPIs at boot, optional clock reset), and the rationale. It's concise and informative. A link to the related NuttX issue would strengthen it further.

  • Impact: Adequately addresses the impact on users (potential boot issues in chained NuttX OS scenarios) and provides context. It lacks detail on other impact areas, especially regarding build process, hardware, documentation, security, and compatibility. While these might not be affected, explicitly stating "NO" for each with a brief justification ("No changes to build process," etc.) would enhance clarity and completeness.

  • Testing: While it mentions the testing environment ("various mpfs based products"), it's lacking specific details. The NuttX requirements ask for build host details (OS, CPU, compiler) and target details (arch, board, config). Crucially, the "Testing logs before change" and "Testing logs after change" sections are empty. This is the weakest part of the PR description and needs significant improvement by including actual logs demonstrating the issue and the fix. Simply stating "works as intended" is insufficient.

Recommendation:

To fully meet the NuttX requirements, the PR should be updated to:

  1. Summary: Include a link to the related NuttX issue if one exists.

  2. Impact: Explicitly address all impact areas (build, hardware, documentation, security, compatibility) even if the impact is "NO." Provide brief justifications for each.

  3. Testing: Provide detailed information about the build host and target environments. Include actual testing logs demonstrating the issue before the change and the corrected behavior after the change. These logs are crucial for reviewers to verify the fix.

@acassis
Copy link
Contributor

acassis commented Nov 29, 2024

@eenurkka I think you need to refresh some board configs, not sure, please look about the issue in rv_virt config

@xiaoxiang781216
Copy link
Contributor

@eenurkka I think you need to refresh some board configs, not sure, please look about the issue in rv_virt config

it's a known ltp error on riscv, and not related to this patch.

@xiaoxiang781216 xiaoxiang781216 merged commit 53f4216 into apache:master Nov 29, 2024
16 of 17 checks passed
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.

4 participants