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

samv7: support RX/TX DMA for UART peripherals #15716

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

michallenc
Copy link
Contributor

@michallenc michallenc commented Jan 29, 2025

Summary

So far only USART drivers could use TX/RX DMA. This commit adds the support for UART drivers as well (with the exception of serial console).

UART peripheral does not have timeout interrupt, so external polling is required to flush the RX data from DMA buffers even if the buffer is not yet full. The board layer can do this by calling sam_serial_dma_poll function.

This adds the support for RX DMA polling from bord layer. The board initializes timer channel SAMV7_UART_RXDMA_POLL_TIMER with frequency SAMV7_UART_RXDMA_POLL_FREQUENCY that triggers DMA polling function.

Impact

UART peripherals on SAMv7 now support RX DMA, with the exception of the peripheral used as a console.

Testing

Tested on samv71-xult evaluation kit and custom samv7 board.

So far only USART drivers could use TX/RX DMA. This commit adds the
support for UART drivers as well (with the exception of serial console).

UART peripheral does not have timeout interrupt, so external polling
is required to flush the RX data from DMA buffers even if the buffer
is not yet full. The board layer can do this by calling
sam_serial_dma_poll function.

Signed-off-by: Michal Lenc <[email protected]>
This adds the support for RX DMA polling. The board initializes
timer channel SAMV7_UART_RXDMA_POLL_TIMER with frequency
SAMV7_UART_RXDMA_POLL_FREQUENCY that triggers DMA polling function.

Signed-off-by: Michal Lenc <[email protected]>
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: L The size of the change in this PR is large labels Jan 29, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 29, 2025

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements but is missing some crucial information. Here's a breakdown:

Strengths:

  • Clear Summary: Explains the "why," "what," and "how" of the change reasonably well. Adding a related NuttX issue number would make it even better.
  • Impact Section Mostly Addressed: The impact on the user, build, hardware, and compatibility is touched upon. However, more details are needed.
  • Testing Mentions Platforms: Identifies the target platforms used for testing.

Weaknesses:

  • Missing Issue References: No NuttX or NuttX Apps issue/PR references are provided.
  • Impact Lacks Detail: While the impact areas are mentioned, the descriptions are very brief. For example, "Impact on user: UART peripherals on SAMv7 now support RX DMA" doesn't explain how a user might need to adapt (e.g., configuration changes, API differences). Similarly, "Impact on build" is missing. Even if the answer is "NO," it should explicitly say so.
  • Documentation Impact Unclear: No mention of whether documentation needs updating or if updates are included.
  • Security and Compatibility Impacts Not Addressed: These are critical, even if the answer is "NO." Explicitly state the impact (or lack thereof). Consider potential security implications of DMA access.
  • "Anything else to consider?" Unanswered: This is an important catch-all for unforeseen issues.
  • Testing Logs Missing: The provided code blocks for "Testing logs before change" and "Testing logs after change" are empty. Include actual log output demonstrating the functionality before and after the change. Show how the DMA improves performance or solves a problem. If logs are too long, consider providing a link to an external file or a concise summary of the key differences observed.
  • Build Host Information Missing: Testing section needs to describe the Build Host environment, including OS, CPU architecture, and compiler version used during development and testing.

Recommendations to improve the PR:

  • Add links to relevant NuttX issues.
  • Expand on the impact descriptions. Be specific. Even if there's no impact, explicitly state "NO" and briefly justify why. For example: "Impact on build: NO. No changes to the build system are required."
  • Address documentation, security, and compatibility impacts explicitly.
  • Provide actual testing logs or a summary of the key observations.
  • Include details about the build host environment.
  • Consider adding benchmarks or performance measurements if applicable to showcase the benefit of using DMA. Quantifiable results greatly strengthen a PR.
  • Use the "Anything else to consider?" section to highlight any potential caveats or limitations of the implemented feature.

By addressing these points, the PR will become significantly stronger, easier to review, and more likely to be merged.

@xiaoxiang781216 xiaoxiang781216 merged commit 4fa5e29 into apache:master Jan 30, 2025
26 checks passed
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 Area: Documentation Improvements or additions to documentation Board: arm 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