Skip to content

Conversation

@anchao
Copy link
Contributor

@anchao anchao commented Sep 19, 2024

Summary

arm/cortex-a,r: replace cp15 instruct to macros to align operation

This is continue work of #13486
Discussion here:
#13486 (comment)

  1. move cp15.h to arch public
  2. replace cp15 instruct to macros to align operation
  3. add memory barrier to avoid compiler optimization

Signed-off-by: chao an anchao@lixiang.com

Impact

N/A

Testing

ci-check

@anchao anchao force-pushed the 24091901 branch 2 times, most recently from 1c69357 to 4f1d349 Compare September 19, 2024 01:07
@anchao anchao changed the title arm/cortex-a,r: replace cp15 instruct to macros align operation arm/cortex-a,r: replace cp15 instruct to macros to align operation Sep 19, 2024
@lupyuen
Copy link
Member

lupyuen commented Sep 19, 2024

[Experimental Bot, please feedback here]

Let's break down this PR against the NuttX requirements:

Summary:

  • Why is change necessary?
    • This part is a little weak. While it links to a prior PR and discussion, it should summarize the reason for those changes within this PR's description. Is it a bug fix? Performance improvement? Code clarity?
  • What functional part of the code is being changed?
    • Clearly identifies: arm/cortex-a,r architecture code, specifically related to cp15 instructions.
  • How does the change work?
    • Provides a good high-level overview:
      • Moves cp15.h
      • Replaces instructions with macros for alignment.
      • Adds memory barriers.
  • Related Issues/PRs:
    • Links provided, which is excellent.

Impact:

  • Mostly "N/A" – This needs more detail. Even if the impact is minimal, explain why. For example:
    • User Impact: "No change in user-facing behavior".
    • Build Impact: "No changes to the build process itself, though compiled output may differ slightly".
    • Hardware Impact: Specify if this is only relevant to some ARM/Cortex variants or if it's broad.
    • Documentation: Does cp15.h being moved require any documentation updates?
    • Security: Consider if using macros instead of raw instructions has any security implications (even if unlikely).
    • Compatibility: Crucial for architecture-specific changes. Are there any compatibility concerns with older ARM versions?

Testing:

  • Build Hosts: Insufficient. List the specific OS, compiler versions, etc., used for testing.
  • Targets: Be specific about the ARM/Cortex architectures and boards tested on, including simulator usage if any.
  • Testing logs: While "ci-check" implies CI was used, include relevant snippets of logs showing:
    • Before: The issue/behavior the change addresses.
    • After: The successful resolution or improvement.

Overall:

The PR has a good structure and clearly explains the technical what. However, it's weak on the why (justification) and needs more thorough impact assessment and testing details.

Recommendation: Don't just state "N/A" – explain why there's no impact for each category. Provide specific testing information to build confidence in the change.

@anchao anchao force-pushed the 24091901 branch 3 times, most recently from 2b0b5e2 to 03c2bb2 Compare September 19, 2024 09:57
This is continue work of apache#13486

Discussion here:
apache#13486 (comment)

1. move cp15.h to arch public
2. replace cp15 instruct to macros to align operation
3. add memory barrier to avoid compiler optimization

Signed-off-by: chao an <anchao@lixiang.com>
@xiaoxiang781216 xiaoxiang781216 merged commit b82717b into apache:master Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants