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

fix(PeriphDrivers): Resolve incorrect DMA request in DMA-based SPI transactions for all parts #1059

Merged
merged 6 commits into from
Jul 2, 2024
Merged

Conversation

JeonChangmin
Copy link
Contributor

@JeonChangmin JeonChangmin commented Jun 26, 2024

Description

This pull request addresses an issue in the DMA-based SPI transaction implementation where the DMA request would not be executed under specific conditions.

Problem Details

  • Issue: When MXC_SPI_MasterTransactionDMA is called with txData == NULL and rxData != NULL in SPI_WIDTH_STANDARD (full duplex) mode, the function sets reqselTx to -1. However, in MXC_SPI_RevA1_MasterTransactionDMA, MXC_SPI_RevA1_TransSetup may additionally set txData based on the request. This leads to the use of an invalid request selector, causing the DMA to not be executed and resulting in an infinite wait state.

  • Call Stack:

Solution

  • Always pass valid reqselRx and reqselTx values from MXC_SPI_MasterTransactionDMA.
  • Configure DMA in MXC_SPI_RevA1_MasterTransactionDMA after executing MXC_SPI_RevA1_TransSetup.

Solution Rationale: By ensuring valid reqselRx and reqselTx values are always passed from MXC_SPI_MasterTransactionDMA, the MXC_SPI_RevA1_MasterTransactionDMA function can correctly configure the DMA settings based on the request. This prevents the DMA from not being executed due to invalid request selectors. Additionally, the MXC_SPI_RevA1_MasterTransactionDMA function always checks the null status of rxData and txData before using the request selectors, ensuring that no other issues arise from this change.

Additional Fixes

  • Applied the same fix to MXC_SPI_SlaveTransactionDMA.
  • Corrected potentially swapped reqselRx and reqselTx values in spi_me11.c and spi_me18.c files. Please confirm if this change was intended. (fix in this PR)

Tests

  • Tested the changes on a MAX78000 device using spi_me17.c as a basis.

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • Provide info on any relevant functional testing/validation.

@github-actions github-actions bot added MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32665 Related to the MAX32665 (ME14) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78000 Related to the MAX78000 (AI85) MAX78002 Related to the MAX78002 (AI87) MAX32662 Related to the MAX32662 (ME12) MAX32572 Related to the MAX32572 (ME55) labels Jun 26, 2024
@JeonChangmin JeonChangmin changed the title fix(PeriphDrivers): Resolve incorrect DMA request in DMA-based SPI transactions fix(PeriphDrivers): Resolve incorrect DMA request in DMA-based SPI transactions for all parts Jun 26, 2024
Copy link
Contributor

@Jake-Carter Jake-Carter 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 for your very nice PR.

Can confirm you've caught some copy/paste typos on the mismatched reqsel values.

The AI87 values are also swapped if you wouldn't mind fixing that as well.

}
switch (spi_num) {
case 0:
reqselTx = MXC_DMA_REQUEST_SPI1TX;
Copy link
Contributor

Choose a reason for hiding this comment

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

AI87 implementation has SPI0/SPI1 swapped as well. Should be reqselTx = MXC_DMA_REQUEST_SPI0TX; here.

Not your fault, this was there before your fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the original implementation was correct. SPI0 and SPI1 are swapped in several places as an unfortunate result of incorrect documentation, and it's been stuck like that ever since to prevent breaking projects... It's why we have messes like this for the ME17, AI85, and AI87.
image

I say we fix this even if it might break existing projects. Usually, instance 0 is mapped to the lowest peripheral address, and then everything goes in ascending order:

  1. SPI0 (APB) - 0x40046000
  2. SPI1 (APB) - 0x40047000
  3. SPI[n] (APB) - 0x4004m000
  4. SPI[n+1] (AHB) - 0x400BE000
  5. SPI[n+2] (AHB) - 0x400BE400

However, the ME17, AI85, and AI87 doesn't follow this:

  1. SPI0 (AHB) - 0x400BE000
  2. SPI1 (APB) - 0x40046000

This has caused confusion, but it'll be a major breaking change to swap the SPI instance names. Instead, we can fix the MXC_SPI_GET_* definitions in the {device}.h file where SPI0 would be associated with index 0, and SPI1 with index 1. This would still be a breaking change, but it would at least help keep all the indexing aligned with the correct SPI names.

What do you think? @Jake-Carter & @lorne-maxim

@JeonChangmin
Copy link
Contributor Author

Thanks for the review!
I fixed the cases where spi_num and reqselTx/Rx didn't match as you suggested.
There are more than AI87, so I fixed all of them, but please let me know if there is something else you intended.

Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Thanks @JeonChangmin, no this is good. Looks like the same mistake got copy/pasted across the drivers since the ME12.

@Jake-Carter Jake-Carter merged commit 796e710 into analogdevicesinc:main Jul 2, 2024
8 checks passed
@JeonChangmin JeonChangmin deleted the fix/spi-full-duplex-dma branch July 3, 2024 01:48
@JeonChangmin JeonChangmin restored the fix/spi-full-duplex-dma branch July 3, 2024 16:10
@JeonChangmin JeonChangmin deleted the fix/spi-full-duplex-dma branch July 3, 2024 16:26
EricB-ADI pushed a commit that referenced this pull request Aug 21, 2024
…d SPI transactions for all parts (#1059)"

This reverts commit 796e710.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32572 Related to the MAX32572 (ME55) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32662 Related to the MAX32662 (ME12) MAX32665 Related to the MAX32665 (ME14) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78000 Related to the MAX78000 (AI85) MAX78002 Related to the MAX78002 (AI87)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants