From 2c1845995bbcdd2d908ae0e0e853907e9736a12d Mon Sep 17 00:00:00 2001 From: Armando Date: Thu, 4 Feb 2021 19:09:28 +0800 Subject: [PATCH 1/2] spi_slave_hd: refactor the hal append api to remove the spinlock --- components/driver/spi_slave_hd.c | 11 +--- components/hal/esp32s2/include/hal/spi_ll.h | 28 +++++++- components/hal/include/hal/spi_slave_hd_hal.h | 4 ++ components/hal/spi_slave_hd_hal.c | 65 ++++++++++--------- components/soc/esp32c3/include/soc/soc_caps.h | 3 - 5 files changed, 66 insertions(+), 45 deletions(-) diff --git a/components/driver/spi_slave_hd.c b/components/driver/spi_slave_hd.c index 90dd626e41..7e21b4488f 100644 --- a/components/driver/spi_slave_hd.c +++ b/components/driver/spi_slave_hd.c @@ -87,13 +87,12 @@ esp_err_t spi_slave_hd_init(spi_host_device_t host_id, const spi_bus_config_t *b spi_chan_claimed = spicommon_periph_claim(host_id, "slave_hd"); SPIHD_CHECK(spi_chan_claimed, "host already in use", ESP_ERR_INVALID_STATE); - spi_slave_hd_slot_t* host = malloc(sizeof(spi_slave_hd_slot_t)); + spi_slave_hd_slot_t* host = calloc(1, sizeof(spi_slave_hd_slot_t)); if (host == NULL) { ret = ESP_ERR_NO_MEM; goto cleanup; } spihost[host_id] = host; - memset(host, 0, sizeof(spi_slave_hd_slot_t)); host->int_spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; host->dma_enabled = (config->dma_chan != SPI_DMA_DISABLED); @@ -398,9 +397,7 @@ static IRAM_ATTR void spi_slave_hd_intr_append(void *arg) spi_slave_hd_data_t *trans_desc; while (1) { bool trans_finish = false; - portENTER_CRITICAL_ISR(&host->int_spinlock); trans_finish = spi_slave_hd_hal_get_tx_finished_trans(hal, (void **)&trans_desc); - portEXIT_CRITICAL_ISR(&host->int_spinlock); if (!trans_finish) { break; } @@ -431,9 +428,7 @@ static IRAM_ATTR void spi_slave_hd_intr_append(void *arg) size_t trans_len; while (1) { bool trans_finish = false; - portENTER_CRITICAL_ISR(&host->int_spinlock); trans_finish = spi_slave_hd_hal_get_rx_finished_trans(hal, (void **)&trans_desc, &trans_len); - portEXIT_CRITICAL_ISR(&host->int_spinlock); if (!trans_finish) { break; } @@ -552,17 +547,13 @@ esp_err_t spi_slave_hd_append_trans(spi_host_device_t host_id, spi_slave_chan_t if (ret == pdFALSE) { return ESP_ERR_TIMEOUT; } - portENTER_CRITICAL(&host->int_spinlock); err = spi_slave_hd_hal_txdma_append(hal, trans->data, trans->len, trans); - portEXIT_CRITICAL(&host->int_spinlock); } else { BaseType_t ret = xSemaphoreTake(host->rx_cnting_sem, timeout); if (ret == pdFALSE) { return ESP_ERR_TIMEOUT; } - portENTER_CRITICAL(&host->int_spinlock); err = spi_slave_hd_hal_rxdma_append(hal, trans->data, trans->len, trans); - portEXIT_CRITICAL(&host->int_spinlock); } if (err != ESP_OK) { ESP_LOGE(TAG, "Wait until the DMA finishes its transaction"); diff --git a/components/hal/esp32s2/include/hal/spi_ll.h b/components/hal/esp32s2/include/hal/spi_ll.h index 05f3689a15..d11fbcd98a 100644 --- a/components/hal/esp32s2/include/hal/spi_ll.h +++ b/components/hal/esp32s2/include/hal/spi_ll.h @@ -1077,6 +1077,7 @@ static inline uint32_t spi_ll_slave_hd_get_last_addr(spi_dev_t* hw) * RX DMA (Peripherals->DMA->RAM) * TX DMA (RAM->DMA->Peripherals) *----------------------------------------------------------------------------*/ +//---------------------------------------------------RX-------------------------------------------------// /** * Reset RX DMA which stores the data received from a peripheral into RAM. * @@ -1119,7 +1120,7 @@ static inline void spi_dma_ll_rx_enable_burst_data(spi_dma_dev_t *dma_in, uint32 } /** - * Enable DMA TX channel burst for descriptor + * Enable DMA RX channel burst for descriptor * * @param dma_in Beginning address of the DMA peripheral registers which stores the data received from a peripheral into RAM. * @param channel DMA channel, for chip version compatibility, not used. @@ -1130,6 +1131,19 @@ static inline void spi_dma_ll_rx_enable_burst_desc(spi_dma_dev_t *dma_in, uint32 dma_in->dma_conf.indscr_burst_en = enable; } +/** + * Get the last inlink descriptor address when DMA produces in_suc_eof interrupt + * + * @param dma_in Beginning address of the DMA peripheral registers which stores the data received from a peripheral into RAM. + * @param channel DMA channel, for chip version compatibility, not used. + * @return The address + */ +static inline uint32_t spi_dma_ll_get_in_suc_eof_desc_addr(spi_dma_dev_t *dma_in, uint32_t channel) +{ + return dma_in->dma_in_suc_eof_des_addr; +} + +//---------------------------------------------------TX-------------------------------------------------// /** * Reset TX DMA which transmits the data from RAM to a peripheral. * @@ -1204,6 +1218,18 @@ static inline void spi_dma_ll_enable_out_auto_wrback(spi_dma_dev_t *dma_out, uin dma_out->dma_conf.out_auto_wrback = enable; } +/** + * Get the last outlink descriptor address when DMA produces out_eof intrrupt + * + * @param dma_out Beginning address of the DMA peripheral registers which transmits the data from RAM to a peripheral. + * @param channel DMA channel, for chip version compatibility, not used. + * @return The address + */ +static inline uint32_t spi_dma_ll_get_out_eof_desc_addr(spi_dma_dev_t *dma_out, uint32_t channel) +{ + return dma_out->dma_out_eof_des_addr; +} + static inline void spi_dma_ll_rx_restart(spi_dma_dev_t *dma_in, uint32_t channel) { dma_in->dma_in_link.restart = 1; diff --git a/components/hal/include/hal/spi_slave_hd_hal.h b/components/hal/include/hal/spi_slave_hd_hal.h index de38ee42fa..2a2f3bc357 100644 --- a/components/hal/include/hal/spi_slave_hd_hal.h +++ b/components/hal/include/hal/spi_slave_hd_hal.h @@ -104,16 +104,20 @@ typedef struct { spi_slave_hd_hal_desc_append_t *tx_cur_desc; ///< Current TX DMA descriptor that could be linked (set up). spi_slave_hd_hal_desc_append_t *tx_dma_head; ///< Head of the linked TX DMA descriptors which are not used by hardware spi_slave_hd_hal_desc_append_t *tx_dma_tail; ///< Tail of the linked TX DMA descriptors which are not used by hardware + spi_slave_hd_hal_desc_append_t tx_dummy_head; ///< Dummy descriptor for ``tx_dma_head`` to start uint32_t tx_used_desc_cnt; ///< Number of the TX descriptors that have been setup uint32_t tx_recycled_desc_cnt; ///< Number of the TX descriptors that could be recycled spi_slave_hd_hal_desc_append_t *rx_cur_desc; ///< Current RX DMA descriptor that could be linked (set up). spi_slave_hd_hal_desc_append_t *rx_dma_head; ///< Head of the linked RX DMA descriptors which are not used by hardware spi_slave_hd_hal_desc_append_t *rx_dma_tail; ///< Tail of the linked RX DMA descriptors which are not used by hardware + spi_slave_hd_hal_desc_append_t rx_dummy_head; ///< Dummy descriptor for ``rx_dma_head`` to start uint32_t rx_used_desc_cnt; ///< Number of the RX descriptors that have been setup uint32_t rx_recycled_desc_cnt; ///< Number of the RX descriptors that could be recycled /* Internal status used by the HAL implementation, initialized as 0. */ uint32_t intr_not_triggered; + bool tx_dma_started; + bool rx_dma_started; } spi_slave_hd_hal_context_t; /** diff --git a/components/hal/spi_slave_hd_hal.c b/components/hal/spi_slave_hd_hal.c index fe1054441c..e86e0ebd32 100644 --- a/components/hal/spi_slave_hd_hal.c +++ b/components/hal/spi_slave_hd_hal.c @@ -30,13 +30,15 @@ #include "hal/gdma_ll.h" #define spi_dma_ll_rx_reset(dev, chan) gdma_ll_rx_reset_channel(&GDMA, chan) -#define spi_dma_ll_tx_reset(dev, chan) gdma_ll_tx_reset_channel(&GDMA, chan); -#define spi_dma_ll_rx_enable_burst_data(dev, chan, enable) gdma_ll_rx_enable_data_burst(&GDMA, chan, enable); -#define spi_dma_ll_tx_enable_burst_data(dev, chan, enable) gdma_ll_tx_enable_data_burst(&GDMA, chan, enable); -#define spi_dma_ll_rx_enable_burst_desc(dev, chan, enable) gdma_ll_rx_enable_descriptor_burst(&GDMA, chan, enable); -#define spi_dma_ll_tx_enable_burst_desc(dev, chan, enable) gdma_ll_tx_enable_descriptor_burst(&GDMA, chan, enable); -#define spi_dma_ll_enable_out_auto_wrback(dev, chan, enable) gdma_ll_tx_enable_auto_write_back(&GDMA, chan, enable); -#define spi_dma_ll_set_out_eof_generation(dev, chan, enable) gdma_ll_tx_set_eof_mode(&GDMA, chan, enable); +#define spi_dma_ll_tx_reset(dev, chan) gdma_ll_tx_reset_channel(&GDMA, chan) +#define spi_dma_ll_rx_enable_burst_data(dev, chan, enable) gdma_ll_rx_enable_data_burst(&GDMA, chan, enable) +#define spi_dma_ll_tx_enable_burst_data(dev, chan, enable) gdma_ll_tx_enable_data_burst(&GDMA, chan, enable) +#define spi_dma_ll_rx_enable_burst_desc(dev, chan, enable) gdma_ll_rx_enable_descriptor_burst(&GDMA, chan, enable) +#define spi_dma_ll_tx_enable_burst_desc(dev, chan, enable) gdma_ll_tx_enable_descriptor_burst(&GDMA, chan, enable) +#define spi_dma_ll_enable_out_auto_wrback(dev, chan, enable) gdma_ll_tx_enable_auto_write_back(&GDMA, chan, enable) +#define spi_dma_ll_set_out_eof_generation(dev, chan, enable) gdma_ll_tx_set_eof_mode(&GDMA, chan, enable) +#define spi_dma_ll_get_out_eof_desc_addr(dev, chan) gdma_ll_tx_get_eof_desc_addr(&GDMA, chan) +#define spi_dma_ll_get_in_suc_eof_desc_addr(dev, chan) gdma_ll_rx_get_success_eof_desc_addr(&GDMA, chan) #define spi_dma_ll_rx_start(dev, chan, addr) do {\ gdma_ll_rx_set_desc_addr(&GDMA, chan, (uint32_t)addr);\ gdma_ll_rx_start(&GDMA, chan);\ @@ -54,6 +56,7 @@ static void s_spi_slave_hd_hal_dma_init_config(const spi_slave_hd_hal_context_t spi_dma_ll_rx_enable_burst_desc(hal->dma_in, hal->rx_dma_chan, 1); spi_dma_ll_tx_enable_burst_desc(hal->dma_out, hal->tx_dma_chan, 1); spi_dma_ll_enable_out_auto_wrback(hal->dma_out, hal->tx_dma_chan, 1); + spi_dma_ll_set_out_eof_generation(hal->dma_out, hal->tx_dma_chan, 1); } void spi_slave_hd_hal_init(spi_slave_hd_hal_context_t *hal, const spi_slave_hd_hal_config_t *hal_config) @@ -68,6 +71,10 @@ void spi_slave_hd_hal_init(spi_slave_hd_hal_context_t *hal, const spi_slave_hd_h hal->append_mode = hal_config->append_mode; hal->rx_cur_desc = hal->dmadesc_rx; hal->tx_cur_desc = hal->dmadesc_tx; + STAILQ_NEXT(&hal->tx_dummy_head.desc, qe) = &hal->dmadesc_tx->desc; + hal->tx_dma_head = &hal->tx_dummy_head; + STAILQ_NEXT(&hal->rx_dummy_head.desc, qe) = &hal->dmadesc_rx->desc; + hal->rx_dma_head = &hal->rx_dummy_head; //Configure slave s_spi_slave_hd_hal_dma_init_config(hal); @@ -261,27 +268,27 @@ int spi_slave_hd_hal_rxdma_seg_get_len(spi_slave_hd_hal_context_t *hal) bool spi_slave_hd_hal_get_tx_finished_trans(spi_slave_hd_hal_context_t *hal, void **out_trans) { - if (!hal->tx_dma_head || hal->tx_dma_head->desc.owner) { + if ((uint32_t)&hal->tx_dma_head->desc == spi_dma_ll_get_out_eof_desc_addr(hal->dma_out, hal->tx_dma_chan)) { return false; } + hal->tx_dma_head = (spi_slave_hd_hal_desc_append_t *)STAILQ_NEXT(&hal->tx_dma_head->desc, qe); *out_trans = hal->tx_dma_head->arg; hal->tx_recycled_desc_cnt++; - hal->tx_dma_head = (spi_slave_hd_hal_desc_append_t *)STAILQ_NEXT(&hal->tx_dma_head->desc, qe); return true; } bool spi_slave_hd_hal_get_rx_finished_trans(spi_slave_hd_hal_context_t *hal, void **out_trans, size_t *out_len) { - if (!hal->rx_dma_head || hal->rx_dma_head->desc.owner) { + if ((uint32_t)&hal->rx_dma_head->desc == spi_dma_ll_get_in_suc_eof_desc_addr(hal->dma_in, hal->rx_dma_chan)) { return false; } + hal->rx_dma_head = (spi_slave_hd_hal_desc_append_t *)STAILQ_NEXT(&hal->rx_dma_head->desc, qe); *out_trans = hal->rx_dma_head->arg; *out_len = hal->rx_dma_head->desc.length; hal->rx_recycled_desc_cnt++; - hal->rx_dma_head = (spi_slave_hd_hal_desc_append_t *)STAILQ_NEXT(&hal->rx_dma_head->desc, qe); return true; } @@ -331,22 +338,20 @@ esp_err_t spi_slave_hd_hal_txdma_append(spi_slave_hd_hal_context_t *hal, uint8_t spi_slave_hd_hal_link_append_desc(hal->tx_cur_desc, data, len, false, arg); - if (!hal->tx_dma_head) { - //start a new link - hal->tx_dma_head = hal->tx_cur_desc; + if (!hal->tx_dma_started) { + hal->tx_dma_started = true; + //start a link hal->tx_dma_tail = hal->tx_cur_desc; - - spi_dma_ll_tx_reset(hal->dma_out, hal->tx_dma_chan); - spi_ll_outfifo_empty_clr(hal->dev); spi_ll_clear_intr(hal->dev, SPI_LL_INTR_OUT_EOF); - + spi_ll_dma_tx_fifo_reset(hal->dma_out); + spi_ll_outfifo_empty_clr(hal->dev); + spi_dma_ll_tx_reset(hal->dma_out, hal->tx_dma_chan); spi_ll_dma_tx_enable(hal->dev, 1); - spi_dma_ll_tx_start(hal->dma_out, hal->tx_dma_chan, &hal->tx_dma_head->desc); + spi_dma_ll_tx_start(hal->dma_out, hal->tx_dma_chan, &hal->tx_cur_desc->desc); } else { - //there is already a link + //there is already a consecutive link STAILQ_NEXT(&hal->tx_dma_tail->desc, qe) = &hal->tx_cur_desc->desc; hal->tx_dma_tail = hal->tx_cur_desc; - spi_dma_ll_tx_restart(hal->dma_out, hal->tx_dma_chan); } @@ -374,22 +379,20 @@ esp_err_t spi_slave_hd_hal_rxdma_append(spi_slave_hd_hal_context_t *hal, uint8_t spi_slave_hd_hal_link_append_desc(hal->rx_cur_desc, data, len, false, arg); - if (!hal->rx_dma_head) { - //start a new link - hal->rx_dma_head = hal->rx_cur_desc; + if (!hal->rx_dma_started) { + hal->rx_dma_started = true; + //start a link hal->rx_dma_tail = hal->rx_cur_desc; - - spi_dma_ll_rx_reset(hal->dma_in, hal->rx_dma_chan); - spi_ll_infifo_full_clr(hal->dev); spi_ll_clear_intr(hal->dev, SPI_LL_INTR_CMD7); - + spi_dma_ll_rx_reset(hal->dma_in, hal->rx_dma_chan); + spi_ll_dma_rx_fifo_reset(hal->dma_in); + spi_ll_infifo_full_clr(hal->dev); spi_ll_dma_rx_enable(hal->dev, 1); - spi_dma_ll_rx_start(hal->dma_in, hal->rx_dma_chan, &hal->rx_dma_head->desc); + spi_dma_ll_rx_start(hal->dma_in, hal->rx_dma_chan, &hal->rx_cur_desc->desc); } else { - //there is already a link + //there is already a consecutive link STAILQ_NEXT(&hal->rx_dma_tail->desc, qe) = &hal->rx_cur_desc->desc; hal->rx_dma_tail = hal->rx_cur_desc; - spi_dma_ll_rx_restart(hal->dma_in, hal->rx_dma_chan); } diff --git a/components/soc/esp32c3/include/soc/soc_caps.h b/components/soc/esp32c3/include/soc/soc_caps.h index 74c43daca0..c5b6503634 100644 --- a/components/soc/esp32c3/include/soc/soc_caps.h +++ b/components/soc/esp32c3/include/soc/soc_caps.h @@ -10,9 +10,6 @@ #define SOC_TWAI_SUPPORTED 1 #define SOC_BT_SUPPORTED 1 -// There are 3 DMA channels on ESP32-C3 -// Attention: These fixed DMA channels are temporarily workaround before we have a centralized DMA controller API to help alloc the channel dynamically -// Remove them when GDMA driver API is ready #include "rmt_caps.h" /*-------------------------- DAC CAPS ----------------------------------------*/ From 3e9cd49d32fc2c21b455398755aa95a6974adcf6 Mon Sep 17 00:00:00 2001 From: Armando Date: Mon, 8 Feb 2021 11:53:35 +0800 Subject: [PATCH 2/2] spi_slv_hd: add hal_trans_finish comments for clarifying risk --- components/hal/include/hal/spi_slave_hd_hal.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/hal/include/hal/spi_slave_hd_hal.h b/components/hal/include/hal/spi_slave_hd_hal.h index 2a2f3bc357..099139cc01 100644 --- a/components/hal/include/hal/spi_slave_hd_hal.h +++ b/components/hal/include/hal/spi_slave_hd_hal.h @@ -266,6 +266,10 @@ int spi_slave_hd_hal_get_last_addr(spi_slave_hd_hal_context_t *hal); /** * @brief Return the finished TX transaction * + * @note This API is based on this assumption: the hardware behaviour of current transaction completion is only modified by the its own caller layer. + * This means if some other code changed the hardware behaviour (e.g. clear intr raw bit), or the caller call this API without noticing the HW behaviour, + * this API will go wrong. + * * @param hal Context of the HAL layer * @param out_trans Pointer to the caller-defined transaction * @return 1: Transaction is finished; 0: Transaction is not finished @@ -275,6 +279,10 @@ bool spi_slave_hd_hal_get_tx_finished_trans(spi_slave_hd_hal_context_t *hal, voi /** * @brief Return the finished RX transaction * + * @note This API is based on this assumption: the hardware behaviour of current transaction completion is only modified by the its own caller layer. + * This means if some other code changed the hardware behaviour (e.g. clear intr raw bit), or the caller call this API without noticing the HW behaviour, + * this API will go wrong. + * * @param hal Context of the HAL layer * @param out_trans Pointer to the caller-defined transaction * @param out_len Actual number of bytes of received data