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

arch/mcx-nxxx: Add support for NXP MCX-N236 CPU #15879

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

pussuw
Copy link
Contributor

@pussuw pussuw commented Feb 20, 2025

Summary

This adds minimal support for NXP MCX-N236 CPU. Peripherals supported are:

  • GPIO
  • PINMUX
  • CLOCK
  • LPUART

An example board, FRDM-MCXN236 is also added, with a basic profile that boots into nsh.

Impact

No impact on existing code, a new architecture entirely is added with an example board implementation.

Testing

Tested by booting nsh on FRDM-MCX236 evaluation kit.
Ran ostest once.

Final memory usage:
VARIABLE  BEFORE   AFTER
======== ======== ========
arena       62928    62928
ordblks         2        5
mxordblk    5b5a0    56c80
uordblks     7380     9ae8
fordblks    5b5a0    58e38
user_main: Exiting
ostest_main: Exiting with status 0
stdio_test: Standard I/O Check: fprintf to stderr
nsh> 

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Feb 20, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 20, 2025

[Experimental Bot, please feedback here]

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

Here's a breakdown of what's missing:

  • Summary: While it describes the what, it lacks the why. Why is MCX-N236 support needed? What use cases does it address? The "how" is also too brief. It lists peripherals but doesn't explain how they are implemented (e.g., new drivers, modifications to existing ones). Issue references are missing.

  • Impact: The impact section is too general. While it states "no impact on existing code," it needs to address all the specific points:

    • New feature? Yes, explicitly state this.
    • User impact? Likely yes, as a new architecture and board are available. Even if minimal, this should be mentioned.
    • Build impact? Yes, the build system needs to be aware of the new architecture and board. This should be described.
    • Hardware impact? Yes, obviously, as this supports new hardware. Be more specific.
    • Documentation? Does this PR include documentation for the new architecture and board? If not, it should, and this needs to be called out.
    • Security? Even if there's no known security impact, it's good practice to explicitly state that security implications were considered.
    • Compatibility? Likely no impact, but state it explicitly.
  • Testing: "No other testing was performed whatsoever" is a red flag. While booting into nsh is a good start, it's insufficient. At a minimum:

    • More Specific Build Host Info: Provide details like the specific Linux distribution, GCC version, etc.
    • More Target Details: Specify the FRDM-MCXN236 configuration used (e.g., clock speed, memory configuration).
    • More Extensive Tests: Test the listed peripherals (GPIO, PINMUX, CLOCK, LPUART) individually. Provide logs for each peripheral test, both before (if applicable) and after the changes. Just booting to nsh doesn't prove these peripherals work correctly.
    • Before Logs: If there were no "before" logs (because the functionality didn't exist), explain that explicitly. Don't just leave the section empty.

In short, the PR needs significantly more detail and more thorough testing before it can be considered complete. It needs to convince reviewers that the changes are well-motivated, correctly implemented, and thoroughly tested.

@pussuw pussuw force-pushed the mcx-n236_port branch 2 times, most recently from 6deaf8f to b4f910c Compare February 20, 2025 12:00
This adds minimal support for NXP MCX-N236 CPU. Peripherals supported are:
- GPIO
- PINMUX
- CLOCK
- LPUART

An example board, FRDM-MCXN236 is also added, with a basic profile that
boots into nsh.

Signed-off-by: Ville Juven <[email protected]>
Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

I don't have this hardware to run actual tests, but the patch merges cleanly, builds successfully, and code LGTM. Only one minor nit (spelling in a comment)... Thanks for adding new architecture and board support!

modifyreg32(priv->uartbase + NXXX_LPUART_BAUD_OFFSET,
0, LPUART_BAUD_RDMAE);

/* Enable interrupt on idle and erros */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* Enable interrupt on idle and erros */
/* Enable interrupt on idle and errors */

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @pussuw amazing work!! :-)

@acassis acassis merged commit 17a80e9 into apache:master Feb 22, 2025
25 checks passed
@pussuw pussuw deleted the mcx-n236_port branch February 23, 2025 12:13
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 Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants