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

arm/isr: move up_set_interrupt_context() to chip define #14959

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

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Nov 26, 2024

Summary

arm/isr: move up_set_interrupt_context() to chip define

up_set_interrupt_context() is chip specific implement, move this function to correct place

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

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

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change and testing information, it lacks crucial details.

Here's what's missing:

  • Summary: The summary is too brief. It needs to explain why moving up_set_interrupt_context() to a chip-specific definition is necessary. What problem does this solve? What are the benefits? It mentions "correct place," but doesn't explain what makes that location correct.
  • Impact: Simply stating "N/A" is insufficient. The author needs to thoughtfully consider and address each impact point. Even if there's no impact, they should explicitly state "NO" for each item. For example, there might be an impact on hardware (specific architectures) or documentation (if this function was previously documented elsewhere). Dismissing the entire Impact section suggests a lack of thoroughness.
  • Testing: "ci-check" is not sufficient testing information. While CI is important, the PR needs to provide specific details about the local testing performed. This includes:
    • The specific build host operating system, CPU architecture, and compiler version used.
    • The specific target architecture, board, and configuration tested.
    • Actual testing logs (or a link to them if they are extensive) demonstrating the behavior before and after the change. "ci-check" doesn't provide any evidence of the change's effectiveness.

In short, the PR needs to be significantly more detailed and address all the required points in the template to be considered complete.

@@ -244,6 +244,12 @@ static inline_function bool up_interrupt_context(void)
#endif
}

noinstrument_function
static inline_function void up_set_interrupt_context(bool flag)
Copy link
Contributor

Choose a reason for hiding this comment

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

but why move arch internal function to public header file? let's change to arm_set_interrupt_context and keep in arm_internal.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. up_set_interrupt_context()/up_interrupt_context() are pair functions. It is easier to read if they are defined in the same file place.
  2. up_set_interrupt_context() is implemented differently on each micro-architecture
    a. cortex-a/r uses TPIDRPRW
    b. cortex-m uses ipsr, and this architecture does not need to implement up_set_interrupt_context()
    c. The classic arm architecture and variant architecture tlsr use the legacy global variable g_interrupt_context

The implementation on each variant architecture is different, why is it defined in the common file?

up_set_interrupt_context() is chip specific implement, move this function to correct place

Signed-off-by: chao an <[email protected]>
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.

3 participants