Skip to content

arch/arm/stm32h5: GPDMA Hardware files #16218

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Conversation

stbenn
Copy link
Contributor

@stbenn stbenn commented Apr 15, 2025

Add the hardware register definition file for the GPDMA peripheral. This is necessary to support implementing dma driver in the future.

Also adds skeleton arch dma header that includes the hardware file.

Summary

  • Why the change is necessary: To support future STM32H5 DMA driver implementation.
  • How: Adds register definition macros

Impact

  • Build process: NA
  • Hardware: NA
  • Documentation: NA
  • Security: NA
  • Compatibility: NA

Testing

Since this is a hardware definition header file, very little testing necessary:

  • Verified clean output from nxstyle on all files in arch/arm/stm32h5
  • nucleo-h563zi:nsh compiled without issue (as expected, this does not change anything that will be included in a build yet)

Add the hardware register definition file for the GPDMA peripheral.
This is necessary to support implementing dma driver in the future.

Also adds skeleton arch dma header that includes the hardware file.

Signed-off-by: Tyler Bennett <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: L The size of the change in this PR is large labels Apr 15, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 15, 2025

[Experimental Bot, please feedback here]

Yes, this PR mostly meets the NuttX requirements, but could be improved. Here's a breakdown:

Strengths:

  • Clearly explains the why of the change (supporting future DMA driver).
  • Concisely describes the how (adding register definitions).
  • Addresses most of the impact categories.
  • Provides some basic testing verification.

Weaknesses & Suggestions for Improvement:

  • Missing Issue References: Even if there isn't a directly related issue, consider creating one to track the overall DMA driver implementation effort. Referencing this issue in the PR would provide valuable context.
  • Incomplete Summary: The "What functional part of the code is being changed?" is missing. Specify that the change adds files to the arch/arm/src/stm32h5 directory (or wherever the files are located). Also, explicitly mention the creation of the skeleton DMA header file.
  • Impact Overly Simplified: While marking many impacts as "NA" is fine, providing slightly more detail builds confidence. For example, instead of "Build process: NA", consider "Build process: No changes to the build process are expected at this stage, as the new header file is not yet included in any build targets." This clarifies that the impact is understood, even if it's negligible.
  • Testing Insufficient: While nxstyle and basic compilation are good, they don't fully test the functionality of the register definitions. Consider adding a simple test program (even if it doesn't utilize DMA yet) that at least includes the header file and perhaps checks the values of a few defined macros to ensure they are correct. This will prevent regressions if the header file is modified later. Also, specify the build host and target details used for testing (OS, compiler version, etc.). Even "Linux, GCC 12.2, nucleo-h563zi:nsh" is better than nothing. Providing "before" logs is less critical in this specific case since there was no prior functionality, but "after" logs showing the successful compilation output (even just a snippet) would be helpful.

Example of Improved Testing Section:

## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host: Linux, GCC 12.2
* Target: nucleo-h563zi:nsh (simulated on QEMU)

Testing logs after change (snippet of successful compilation):

...
LINK: nuttx
...


A simple test program (stm32h5_dma_test.c) was created to include the new header file and verify a few register definitions.  This program compiled and linked without error.

By addressing these points, the PR will be stronger and provide more confidence in the changes.

@xiaoxiang781216 xiaoxiang781216 merged commit 1f53575 into apache:master Apr 16, 2025
25 checks passed
@stbenn stbenn deleted the h5-dma branch April 16, 2025 13:22
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: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants