wip

blog/content/post/2023-06-07-twpm-spi-fix.md

@@ -209,11 +209,294 @@ by master, so SPI disabling should be avoided if not needed.
 The problem becomes even more pronounced when we want to implement TPM protocol
 as we don't know size (and direction) of data payload. Each TPM frame starts
 with a 4 byte header which tells us what is the size of transfer and what is the
-direction (read from or write to a register). After we read the header, SPI
-disabled causing a few things:
+direction (read from or write to a register). After we read the header, we
+disable SPI, causing a few things:
 
 - MISO is left floating (we have SPI v1.3 on STM32L4)
-- we introduce additional delay by re-configuring SPI (mind that we are still)
+- we introduce additional delay by re-configuring SPI
+
+## Fixing SPI
+
+I will do initial work using HAL and STM32CubeIDE, at a later stage I will port
+that to Zephyr. I create a new STM32CubeMX project the proceed to setting up
+SPI2 controller through graphical configuration manager. Basic settings involve
+configuring SPI as `Full-Duplex Slave`, configuring NSS (Chip Select) pin as
+input, setting up DMA channels and setting SPI frame length to 8 bits (as
+required by TPM spec):
+
+![](/img/stm32cube_spi2_setup.png)
+
+![](/img/stm32cube_spi_setup_dma.png)
+
+STM32CubeMX generates code that performs hardware initialization, including SPI.
+We are ready to do SPI transactions using `HAL_SPI_TransmitReceive_DMA`
+function. Of course that would give the same result as Zephyr does, instead I'm
+going to roll my own implementation.
+
+But, first, let's look at `HAL_SPI_TransmitReceive_DMA` implementation:
+
+```c
+HAL_StatusTypeDef HAL_SPI_TransmitReceive_DMA(SPI_HandleTypeDef *hspi, uint8_t *pTxData, uint8_t *pRxData, uint16_t Size)
+{
+  ...
+
+  /* Reset the threshold bit */
+  CLEAR_BIT(hspi->Instance->CR2, SPI_CR2_LDMATX | SPI_CR2_LDMARX);
+
+  /* The packing mode management is enabled by the DMA settings according the spi data size */
+  if (hspi->Init.DataSize > SPI_DATASIZE_8BIT)
+  {
+    /* Set fiforxthreshold according the reception data length: 16bit */
+    CLEAR_BIT(hspi->Instance->CR2, SPI_RXFIFO_THRESHOLD);
+  }
+  else
+  {
+    /* Set RX Fifo threshold according the reception data length: 8bit */
+    SET_BIT(hspi->Instance->CR2, SPI_RXFIFO_THRESHOLD);
+
+    if (hspi->hdmatx->Init.MemDataAlignment == DMA_MDATAALIGN_HALFWORD)
+    {
+      if ((hspi->TxXferSize & 0x1U) == 0x0U)
+      {
+        CLEAR_BIT(hspi->Instance->CR2, SPI_CR2_LDMATX);
+        hspi->TxXferCount = hspi->TxXferCount >> 1U;
+      }
+      else
+      {
+        SET_BIT(hspi->Instance->CR2, SPI_CR2_LDMATX);
+        hspi->TxXferCount = (hspi->TxXferCount >> 1U) + 1U;
+      }
+    }
+
+    if (hspi->hdmarx->Init.MemDataAlignment == DMA_MDATAALIGN_HALFWORD)
+    {
+      /* Set RX Fifo threshold according the reception data length: 16bit */
+      CLEAR_BIT(hspi->Instance->CR2, SPI_RXFIFO_THRESHOLD);
+
+      if ((hspi->RxXferCount & 0x1U) == 0x0U)
+      {
+        CLEAR_BIT(hspi->Instance->CR2, SPI_CR2_LDMARX);
+        hspi->RxXferCount = hspi->RxXferCount >> 1U;
+      }
+      else
+      {
+        SET_BIT(hspi->Instance->CR2, SPI_CR2_LDMARX);
+        hspi->RxXferCount = (hspi->RxXferCount >> 1U) + 1U;
+      }
+    }
+  }
+
+  /* Check if we are in Rx only or in Rx/Tx Mode and configure the DMA transfer complete callback */
+  if (hspi->State == HAL_SPI_STATE_BUSY_RX)
+  {
+    /* Set the SPI Rx DMA Half transfer complete callback */
+    hspi->hdmarx->XferHalfCpltCallback = SPI_DMAHalfReceiveCplt;
+    hspi->hdmarx->XferCpltCallback     = SPI_DMAReceiveCplt;
+  }
+  else
+  {
+    /* Set the SPI Tx/Rx DMA Half transfer complete callback */
+    hspi->hdmarx->XferHalfCpltCallback = SPI_DMAHalfTransmitReceiveCplt;
+    hspi->hdmarx->XferCpltCallback     = SPI_DMATransmitReceiveCplt;
+  }
+
+  /* Set the DMA error callback */
+  hspi->hdmarx->XferErrorCallback = SPI_DMAError;
+
+  /* Set the DMA AbortCpltCallback */
+  hspi->hdmarx->XferAbortCallback = NULL;
+
+  /* Enable the Rx DMA Stream/Channel  */
+  if (HAL_OK != HAL_DMA_Start_IT(hspi->hdmarx, (uint32_t)&hspi->Instance->DR, (uint32_t)hspi->pRxBuffPtr,
+                                 hspi->RxXferCount))
+  {
+    /* Update SPI error code */
+    SET_BIT(hspi->ErrorCode, HAL_SPI_ERROR_DMA);
+    errorcode = HAL_ERROR;
+
+    hspi->State = HAL_SPI_STATE_READY;
+    goto error;
+  }
+
+  /* Enable Rx DMA Request */
+  SET_BIT(hspi->Instance->CR2, SPI_CR2_RXDMAEN);
+
+  /* Set the SPI Tx DMA transfer complete callback as NULL because the communication closing
+  is performed in DMA reception complete callback  */
+  hspi->hdmatx->XferHalfCpltCallback = NULL;
+  hspi->hdmatx->XferCpltCallback     = NULL;
+  hspi->hdmatx->XferErrorCallback    = NULL;
+  hspi->hdmatx->XferAbortCallback    = NULL;
+
+  /* Enable the Tx DMA Stream/Channel  */
+  if (HAL_OK != HAL_DMA_Start_IT(hspi->hdmatx, (uint32_t)hspi->pTxBuffPtr, (uint32_t)&hspi->Instance->DR,
+                                 hspi->TxXferCount))
+  {
+    /* Update SPI error code */__HAL_SPI_ENABLE
+    SET_BIT(hspi->ErrorCode, HAL_SPI_ERROR_DMA);
+    errorcode = HAL_ERROR;
+
+    hspi->State = HAL_SPI_STATE_READY;
+    goto error;
+  }
+
+  /* Check if the SPI is already enabled */
+  if ((hspi->Instance->CR1 & SPI_CR1_SPE) != SPI_CR1_SPE)
+  {
+    /* Enable SPI peripheral */
+    __HAL_SPI_ENABLE(hspi);
+  }
+  /* Enable the SPI Error Interrupt Bit */
+  __HAL_SPI_ENABLE_IT(hspi, (SPI_IT_ERR));
+
+  /* Enable Tx DMA Request */
+  SET_BIT(hspi->Instance->CR2, SPI_CR2_TXDMAEN);
+
+  ...
+}
+```
+
+What this code does:
+
+- Initialize some callbacks (like transfer complete callbacks)
+- Configure some SPI registers (note that registers are written each time, but
+  some don't have to be)
+- Initialize DMA channel and enables DMA on SPI controller (RXDMAEN and TXDMAEN
+  bits)
+- Enable SPI interrupts
+- Enable SPI controller
+
+Callbacks handle end of transaction event which involves waiting for SPI to
+become idle, while generally this is a desirable behaviour, it is not in that
+case. As mentioned before, we need to read 4 byte header, which contains
+information such as transfer direction (read or write) and data length. Waiting
+for SPI to become idle introduces additional overhead.
+
+I created a stripped-down version of `HAL_SPI_TransmitReceive_DMA`:
+
+```c
+static uint8_t tx_buf[4] = {0xaa, 0xe0, 0xbb, 0xa5};
+static uint8_t rx_buf[4] = {0};
+
+static void rxdma_complete(DMA_HandleTypeDef *hdma)
+{
+	SPI_HandleTypeDef *hspi = (SPI_HandleTypeDef *)(((DMA_HandleTypeDef *)hdma)->Parent);
+	HAL_DMA_Start_IT(hspi->hdmarx, (uint32_t)&hspi->Instance->DR, (uint32_t)rx_buf, 4);
+}
+
+static void txdma_complete(DMA_HandleTypeDef *hdma)
+{
+	SPI_HandleTypeDef *hspi = (SPI_HandleTypeDef *)(((DMA_HandleTypeDef *)hdma)->Parent);
+	HAL_DMA_Start_IT(hspi->hdmatx, (uint32_t)tx_buf, (uint32_t)&hspi->Instance->DR, 4);
+}
+
+void app_main() {
+	SPI_HandleTypeDef *hspi = &hspi2;
+
+	// Initialize callbacks
+	hspi->hdmatx->XferCpltCallback = txdma_complete;
+	hspi->hdmarx->XferCpltCallback = rxdma_complete;
+
+	// One-time SPI configuration
+	// Clear SPI_RXFIFO_THRESHOLD to trigger DMA on each byte available.
+	CLEAR_BIT(hspi->Instance->CR2, SPI_RXFIFO_THRESHOLD);
+
+	// Start the transfer
+	SET_BIT(hspi->Instance->CR2, SPI_CR2_RXDMAEN);
+	HAL_DMA_Start_IT(hspi->hdmarx, (uint32_t)&hspi->Instance->DR, (uint32_t)rx_buf, 4);
+
+	HAL_DMA_Start_IT(hspi->hdmatx, (uint32_t)tx_buf, (uint32_t)&hspi->Instance->DR, 4);
+	SET_BIT(hspi->Instance->CR2, SPI_CR2_TXDMAEN);
+
+	__HAL_SPI_ENABLE(hspi);
+}
+```
+
+We reduced the code used to almost a minimum (still some optimizations could be
+done in `HAL_DMA_Start_IT`). The code lacks most features, such as SPI error
+detection (those will be added later on), and we transfer only a single, static
+data. DMA is restarted directly from interrupt handler, we only reprogram the
+channel, we don't touch any SPI registers. Please note that I'm using a bit
+different initialization order than originally done by HAL. We first enable
+`RXDMAEN`, then, program DMA channels, enable `TXDMAEN` and finally enable SPI.
+HAL enables `RXDMAEN` after programming RX channel and `TXDMAEN` after enabling
+SPI. Our code follows what has been stated in the STM32 Programming Manual
+(**rm0351**):
+
+![](/img/rm0351_spi_dma_enable_procedure.png)
+
+For testing purposes, I am using Raspberry PI 3B as SPI host. Configuration is
+pretty straightforward, you can enable `spidev` by uncommenting
+`dtoverlay=spi0-1cs` in `/boot/config.txt` and rebooting. For communicating with
+`spidev` I am using a custom Python script:
+
+```python
+from spidev import SpiDev
+
+class Spi:
+    def __init__(self):
+        self.device = SpiDev()
+        self.device.open(0, 0)
+        self.device.bits_per_word = 8
+        self.device.mode = 0b00
+        self.device.max_speed_hz = 24000000
+        self.freq = 24000000
+
+    def get_frequency(self):
+        return self.freq
+
+    def set_frequency(self, freq):
+        self.freq = freq
+
+    def xfer(self, data: bytes) -> bytes:
+        return self.device.xfer(data, self.freq)
+
+
+def test(func):
+    def wrapper():
+        freq = spi.freq
+        iteration = 0
+
+        try:
+            for i in range(10):
+                iteration = i
+                func()
+            print(f'OK: {func.__name__} @ {freq} Hz')
+        except AssertionError:
+            print(f'FAIL: {func.__name__} @ {freq} Hz (iteration {iteration})')
+    return wrapper
+
+
+@test
+def test_read_constant_4byte():
+    expected_data = [0xaa, 0xe0, 0xbb, 0xa5]
+    result = spi.xfer([0xff] * len(expected_data))
+    print('result = {:x} {:x} {:x} {:x}'.format(result[0], result[1], result[2], result[3]))
+    assert result == expected_data
+
+
+def main():
+    global spi
+
+    try:
+        spi = Spi()
+        spi.set_frequency(24000000)
+        test_read_constant_4byte()
+    finally:
+        spi.device.close()
+
+if __name__ == '__main__':
+    main()
+```
+
+When I ran the test code I could see through logic analyzer the transmitted data
+was correct, however Raspberry PI did not receive correct data. This quickly
+turned out to be a problem with connection between RPI and Nucleo. Previously
+I've been using two jumper-wire cables for each pin, those cables were connected
+to breadboard together with logic analyzer. Now I'm using single cable, and I
+can reach stable 22 MHz (contrary to stable 18 MHz on old cables), 24 MHz is
+mostly stable but sometimes problems occur, if I connect logic analyzer 24 Mhz
+becomes broken completely.
 
 ## Summary