fix(PeriphDrivers): Ensure single execution of DMA-based SPI transaction callback for all parts

[JeonChangmin]

Description

This pull request contains three commits to ensure that the req->completeCB callback of MXC_SPI_MasterTransactionDMA is only executed once. Previously, the MXC_SPI_MasterTransactionDMA function could trigger the req->completeCB callback multiple times, which is different from users' expectations (see Background & Problem).

Background

The MXC_SPI_MasterTransactionDMA function initiates DMA requests up to two times. To execute req->completeCB only for the last DMA callback, there is an internal DMA callback counting mechanism.

Callbacks

1. req->completeCB: A user-provided callback that should be executed once after MXC_SPI_MasterTransactionDMA completes.

2. MXC_SPI_RevA1_DMACallback: A DMA request callback used internally. The MXC_SPI_MasterTransactionDMA function initiates up to two DMA requests, and each call triggers the MXC_SPI_RevA1_DMACallback function.

How MXC_SPI_MasterTransactionDMA executes the completeCB callback once?

1. Save a flag states[i].txrx_req which indicates whether both of Tx/Rx DMA requests are needed. (ref)
2. Save two flags tx_is_complete and rx_is_complete which indicate whether Tx/Rx DMA requests are needed. (ref)
3. Initiate Tx/Rx DMA when tx/rxData != NULL && !tx/rx_is_complete. (tx, rx)
4. If tx/rx_is_complete == true, run MXC_SPI_RevA1_DMACallback manually. (ref)
5. Inside MXC_SPI_RevA1_DMACallback, when states[i].txrx_req == true, execute req->completeCB for second MXC_SPI_RevA1_DMACallback call. If states[i].txrx_req == false, always call req->completeCB. (ref)

Problem

1. The condition of the states[i].txrx_req flag and the actual DMA request condition differ.

• Example
  • Input req: txData == NULL && txLen == 0 && txCnt == 0 && rxData != NULL && rxLen > 0 && rxCnt = 0
  • Internal variables: txrx_req = false, tx_is_complete = true, rx_is_complete = false
  • Rx DMA will be initiated
  • Due to tx_is_complete == true, this code will execute MXC_SPI_RevA1_DMACallback.
  • Rx DMA also triggers MXC_SPI_RevA1_DMACallback after the DMA request finishes.

2. Inside MXC_SPI_RevA1_DMACallback, the shared variablestates[i].req_done is used multiple times (usage 1, usage 2). This can trigger req->completeCB multiple times when execution of two MXC_SPI_RevA1_DMACallback calls overlaps.

Commit Details

• Commit 1: Solving Problem 1
  • Always check req->rx/txData != NULL && req->rx/txLen for both setting txrx_req and initiating DMA.

* Commit 2: Partially solving Problem 2 * Use the shared variable states[i].req_done once in MXC_SPI_RevA1_DMACallback by utilizing a non-shared local variable req_done. * Potential issue: When an inturrept occurs during req_done = states[i].req_done++ execution, req->completeCB may not be executed.
* Commit 3: Solving potential issue of commit 2 * Use an atomic operation to fetch and increment the shared variable states[i].req_done, ensuring that the operation is safe from interrupt and that the req->completeCB callback is executed correctly.

Tests

• Tested the changes on a MAX78000 device.

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. For API changes or significant features, this is not optional.

[sihyung-maxim]

Works well.

Thank you for your contribution - very nice PR description!

[Jake-Carter]

Thanks @JeonChangmin, looks good. One minor change request below.

I wasn't sure whether stdatomic was well supported on the Arm core out of the box, but the disassembly shows it uses ldrex and strex to implement the atomic compare and swap loop. Thought you might find it interesting.

                req_done = atomic_fetch_add(&states[i].req_done, 1);
  34:	f3bf 8f5b 	dmb	ish
  38:	e854 af00 	ldrex	sl, [r4]
  3c:	f10a 0301 	add.w	r3, sl, #1
  40:	e844 3200 	strex	r2, r3, [r4]
  44:	2a00      	cmp	r2, #0
  46:	d1f7      	bne.n	38 <MXC_SPI_RevA1_DMACallback+0x38>
  48:	f3bf 8f5b 	dmb	ish

Theoretically it's a rare condition anyways. The DMA IRQs are assigned the same priority by default in the NVIC, so instead of nesting into each other they are queued sequentially. So I think the condition would only occur if the DMA RX channel IRQ is manually assigned a lower priority value, or vice versa.

[JeonChangmin]

@Jake-Carter @sihyung-maxim Thank you for the feedback!

If the sequential execution of the DMA handler is guaranteed, then Problem 2 might not be an issue.
I have reverted the two commits that addressed Problem 2.

I thought Problem 2 was the cause because the async DMA call and wait were still unstable even after handling Problem 1.
There are instances where the wait execution stops depending on the presence of printf or other related functions.
I will look for other solutions.

I apologize for the confusion.

Thank you for your understanding.

[Jake-Carter]

There are instances where the wait execution stops depending on the presence of printf or other related functions.
I will look for other solutions.

Do you have an example that can consistently reproduce it?

[JeonChangmin]

@Jake-Carter
I fixed that error a few days ago.
The problem was that the memory of req was freed before the DMA's callback.
After applying the last 3 PRs (#1059, #1070, #1072), DMA-based SPI communications work well in all cases.
Thank you for the reviews.

[Jake-Carter]

@JeonChangmin ah yep, that is a somewhat common issue if the request struct goes out of scope. Thanks for your contributions!