feat(PeriphDrivers)!: Add SPI v2 Implementation to MAX78002 #632
Conversation
- Working standard reads/writes - Working QSPI reads/writes
- If the value of the dummy byte was defined locally inside of the spi_transmit function, it would introduce some glitches on the SPI MOSI line. Suspected compiler optimization issue. For some reason, these glitches also corrupted the MISO line. This caused RX data to be corrupted, and ram reads would return incorrect values.
…es are corrupted by misaligned IO1)
…n 86us, VGA read in 65us (!!!)
…rrent work in case of deletion.
Examples/MAX78002/SPI/README.md
Outdated
| - `mxc_spi_clkmode_t clk_mode` //<== Clock Mode (CPOL:CPHA) | ||
| - `mxc_spi_interface_t mode` //<== Select Interface (Standard 4-wire, 3-wire, dual, quad) | ||
| - `mxc_spi_tscontrol_t ts_control` //<== HW Auto, SW Driver, or SW Application Target Control | ||
| - `mxc_spi_target_t target` //<== |
There was a problem hiding this comment.
No description for target
|
|
||
| int MXC_SPI_InitStruct(mxc_spi_init_t *init) | ||
| { | ||
| return MXC_SPI_InitStruct(init); |
There was a problem hiding this comment.
MXC_SPI_RevA2_InitStruct & MXC_SPI_RevA2InitStructDMA?
There was a problem hiding this comment.
This was updated while you were reviewing.
|
|
||
| #ifdef MXC_SPI0 | ||
| } else if (spi == MXC_SPI0) { | ||
| if (index > MXC_SPI1_TS_INSTANCES) { |
There was a problem hiding this comment.
MXC_SPI0_TS_INSTANCES
|
|
||
| if (req->tx_buffer != NULL) { | ||
| switch (spi_num) { | ||
| case 0: |
There was a problem hiding this comment.
Any reason SPI1 goes with index 0 and SPI0 goes with index 1 here? Same question for rx_reqsel?
There was a problem hiding this comment.
This is prevalent in the ME17, ME18, AI85, and AI87. Originally, I believe this numbering flip was made to support the previous design where the AHB SPI had a lower instance number than the APB SPI, which caused problems with the RISCV seeing incorrect SPI instances (RISCV only sees the APB SPI). We usually order our SPI instances with APB SPI instances first (lower instance numbers) and AHB instances second (higher instance numbers).
The SPI instance ordering might have been updated with later designs, but the current instance numbering was kept since it is a breaking change.
|
|
||
| target.index = req->ssIdx; | ||
|
|
||
| error = MXC_SPI_SetRegisterCallback(req->spi, req->callback, req->callback_data); |
There was a problem hiding this comment.
SetRegisterCallback is odd. I'd suggest choosing one verb or the other: SetCallback or RegisterCallback.
There was a problem hiding this comment.
I replaced with SetCallback
| } | ||
| } | ||
|
|
||
| int MXC_SPI_ReadyForSleep(mxc_spi_reva_regs_t *spi) |
There was a problem hiding this comment.
Same here. Any reason these aren't supported going forward?
There was a problem hiding this comment.
Nope, it's implemented now.
| if (STATES[spi_num].tx_buffer && STATES[spi_num].tx_len > 0) { | ||
| // Write to the FIFO for byte size transactions (message sizes for 8 bits or less) | ||
| if (STATES[spi_num].init.frame_size <= 8) { | ||
| while (((spi->dma & MXC_F_SPI_REVA_DMA_TX_LVL) >> MXC_F_SPI_REVA_DMA_TX_LVL_POS) < |
There was a problem hiding this comment.
Shouldn't we exit this loop if tx_len == tx_cnt?
There was a problem hiding this comment.
The loop condition checks whether there is available space in the transmit FIFO before writing to the transmit FIFO.
There was a problem hiding this comment.
Wrote this later, but note, the transaction (mainly for message lengths greater than SPI FIFO depth), doesn't spend its entire time in a single interrupt. Multiple interrupts to write/read the FIFO are triggered, depending on the status of the FIFO. Doing tx_len == tx_cnt for long transactions causes the CPU to spend its entire time in a single interrupt handler even if the FIFO is not ready.
|
|
||
| remain = STATES[spi_num].tx_len - STATES[spi_num].tx_cnt; | ||
|
|
||
| if (remain) { |
There was a problem hiding this comment.
Any reason the threshold is only updated for halfword size transactions? Same for RX threshold below.
There was a problem hiding this comment.
For byte size frames, it's as simple as writing a byte to the TX FIFO. Unfortunately, transmitting halfword size frames is not as straightforward. Halfword-size frames require extra care on how it's packaged during the transaction. The remaining number of frames (if less than FIFO depth) needs to match the threshold value for the hardware to properly send the remaining frames or the transaction might not end.
| if (STATES[spi_num].rx_buffer && STATES[spi_num].rx_len > 0) { | ||
| // Read the FIFO for byte size transactions (message sizes for 8 bits or less) | ||
| if (STATES[spi_num].init.frame_size <= 8) { | ||
| while ((spi->dma & MXC_F_SPI_REVA_DMA_RX_LVL)) { |
There was a problem hiding this comment.
Exit loop if rx_cnt == rx_len?
There was a problem hiding this comment.
Register level is more reliable than system level checks. Note, the transaction (mainly for message lengths greater than SPI FIFO depth), doesn't spend its entire time in a single interrupt. Multiple interrupts to write/read the FIFO are triggered, depending on the status of the FIFO.
| } | ||
| } | ||
|
|
||
| spi->dma = 2151718466; |
There was a problem hiding this comment.
Whoops, need to delete that.
|
/clang-format-run |
| int MXC_SPI_ControllerTransaction(mxc_spi_regs_t *spi, uint8_t *tx_buffer, uint32_t tx_fr_len, | ||
| uint8_t *rx_buffer, uint32_t rx_fr_len, uint8_t deassert, | ||
| mxc_spi_target_t *target); | ||
|
|
There was a problem hiding this comment.
With the new naming convention, is it clear that the non-blocking functions are "async" and the AsyncHandler needs to be called when using these?
There was a problem hiding this comment.
I've added a note in the header file. Do you think the "Async" name should be added to these controller functions?
There was a problem hiding this comment.
My initial gut reaction was "yes". Transaction, TransactionAsync, and TransactionDMA all make sense to me. TransactionDMAAsync, not so much.
| * This function initializes everything necessary to call a SPI transaction function. | ||
| * Some parameters are set to defaults as follows: | ||
| * SPI Mode - 0 | ||
| * SPI Width - SPI_WIDTH_STANDARD (even if quadModeUsed is set) |
There was a problem hiding this comment.
Why are SPI mode and width forced here?
There was a problem hiding this comment.
It's not, just a copy-paste error.
| union { | ||
| int deassert; | ||
| int ssDeassert; // ssDeassert - deprecated name | ||
| }; |
There was a problem hiding this comment.
Doing this with a union is cool. Good approach for the future
| * txData begin to recieve data, padding MOSI with DefaultTXData | ||
| * | ||
| * | ||
| * @deprecated. |
There was a problem hiding this comment.
Why is mxc_spi_width_t deprecated? I don't immediately see the necessity for renaming the struct and functions to mxc_spi_interface_t equivalents. Seems like they do exactly the same thing?
There was a problem hiding this comment.
They do, but it doesn't follow the naming convention in the types (lack of MXC_ prefix). The mxc_spi_width_t is not the best name to use for the interface mode because it can be confused with the message/frame widths. It's also not a common term used to select the interface mode in other peripheral libraries from other vendors (I've only seen ours use it).
This reason is similar to why I added mxc_spi_clkmode_t to replace mxc_spi_mode_t in SPI v2. The term "mode" has been thrown around a lot and, although it refers to the clock mode, it's been used to represent either "master/slave mode", "clock mode", or the interface mode (dual mode, quad mode, 3-wire mode, and 4-wire mode).
I just want to make the terminology clearer.
|
/clang-format-run |
|
/clang-format-run |
The transaction functions look long, but that's because there are register fields explicitly set (even to 0) and there are more checks in place, whereas the previous implementation got away with assuming certain register fields are 0 after a reset and never cleared any fields.
Ready for review!
Average estimate times from start of transaction function call to start of transaction:
On default ISO.
SPI v2
~43us non-DMA
~26us DMA
Previous SPI
~60us non-DMA
~142us DMA