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.

Sign up for GitHub

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

Jump to bottom

cpu/stm32: fix periph_dma #18711

Merged
merged 3 commits into from
Oct 27, 2022
Merged

cpu/stm32: fix periph_dma #18711

merged 3 commits into from
Oct 27, 2022

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Oct 7, 2022 •
edited
Loading

Contribution description

This fixes some bugs I found in the STM32 DMA module. See commit messages for further details.

Perhaps a better fix would have been to add a second parameter to the dma_conf_t struct to separately define the DMA controller and stream/channel. This would be similar to the how the ADC module handles the ADC and channel number for ADC lines. Should I have done this instead of fixing the current way of mapping DMA stream 0-7 to DMA controller 1, and streams 8 and beyond to controller 2?

Testing procedure

On any STM32 based board:

  1. initiate a DMA transfer with width not equal to one byte and auto indexing enabled on the destination buffer
  2. call dma_suspend followed dma_resume after some time
  3. observe that after the DMA transfer is complete, the write will underflow the destination buffer
  4. initiate a DMA transfer with auto indexing disabled on the destination buffer
  5. call dma_suspend followed dma_resume after some time
  6. observe that after the DMA transfer is complete, the write will underflow the destination buffer

On any STM32F303 based board with 2 DMA controllers:

  1. enable periph_dma module
  2. observe compiler error about missing DMA2_Channel6_IRQn definition
  3. observe that even if compiler error is fixed, any transfer using DMA2 will never complete or even crash the processor

Issues/PRs references

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 7, 2022
@Enoch247 Enoch247 force-pushed the cpu-stm32-dma-fixes branch from be0e90b to a42f4be Compare October 7, 2022 21:30
@Enoch247
Copy link
Contributor Author

I had planned to add UART4 to the nucleo-f303ze board port to demonstrate this PR. Upon starting that effort, I can see that the board has remnants of DMA support, but this has been broken at one point, or never worked. I don't really want to open that can of worms right now. I can attest that this PR works on my custom board, but see now easy way to demonstrate with any eval board I have on hand.

@Enoch247 Enoch247 changed the title Draft: STM32 DMA Fixes STM32 DMA Fixes Oct 10, 2022
@Enoch247
Copy link
Contributor Author

I also felt like the lines else if (stream < 3 || (stream >= 8 && stream < 11)) { and else if (stream < 3 || (stream >= 8 && stream < 11)) of static IRQn_Type dma_get_irqn(int stream) in dma.c were wrong, but I did not touch those as I don't have a board to test changes to those lines on.

benpicco
benpicco reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good, just two comments:

cpu/stm32/periph/dma.c Outdated Show resolved Hide resolved
cpu/stm32/periph/dma.c Show resolved Hide resolved
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 12, 2022
@Enoch247 Enoch247 force-pushed the cpu-stm32-dma-fixes branch from b44c7df to cf4669a Compare October 12, 2022 19:21
@Enoch247
Copy link
Contributor Author

I just force pushed to squash my fixup commit. There was no change to the code.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 13, 2022
benpicco
benpicco reviewed Oct 13, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco 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 the hardware to test this, but looks good to me.
The magic numbers need a comment though.

cpu/stm32/periph/dma.c Show resolved Hide resolved
cpu/stm32/periph/dma.c Show resolved Hide resolved
@riot-ci
Copy link

riot-ci commented Oct 13, 2022 •
edited
Loading

Murdock results

✔️ PASSED

4e2c63c cpu/stm32/periph/dma: make dma_prepare() generic

Success Failures Total Runtime
1992 0 1992 06m:19s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 13, 2022
@maribu maribu changed the title STM32 DMA Fixes cpu/stm32: fix periph_dma Oct 13, 2022
@benpicco
Copy link
Contributor

@Enoch247 please add the comment how you achieved those numbers, then we can merge this.

@Enoch247
cpu/stm32/periph/dma: fix DMA2 on STMF3 families
264a7c8 
As it was, the calculation of DMA2's IRQ number was inccrorect for some
STM families. The implmentation alocates streams numbers 0 to 7 for the
first DMA controller and 8 and up for the second DMA controller. This
offset of +8 was not accounted for when IRQ's of the second DMA
controller was calculated. This patch corrects this.
@Enoch247
cpu/stm32/periph/dma: fix dma_resume
c888c10 
As implmented, dma_resume assumed that transfers widths were 1 byte and
that the memory address incrmenting was always on and periphial address
incrementing always off. This resulted in memory corruption anytime
these assumptions were not true and a dma was resumed. The DMA module
allows intitiating transfers that did not meet these assumption.

This patch adds proper handling inside dma_resume to safely resume any
transfer. Clearifications and errors are added/fixed in the module's
header file. Also, a few constants are removed from the gobal namespace.
@Enoch247
cpu/stm32/periph/dma: make dma_prepare() generic
4e2c63c 
This patch makes dma_prepare() handle register names a bit more
generically.
@Enoch247 Enoch247 force-pushed the cpu-stm32-dma-fixes branch from cf4669a to 4e2c63c Compare October 26, 2022 14:09
@benpicco benpicco merged commit 578d328 into RIOT-OS:master Oct 27, 2022
@Enoch247 Enoch247 deleted the cpu-stm32-dma-fixes branch October 27, 2022 15:09
@maribu maribu mentioned this pull request Jan 5, 2023
Closed
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

@bergzand bergzand Awaiting requested review from bergzand

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.01
Development

Successfully merging this pull request may close these issues.
5 participants
@Enoch247 @riot-ci @benpicco @maribu @kaspar030