feedc0de/esp-idf
1
0
Fork 0
forked from espressif/esp-idf
Code Pull Requests Activity

Merge branch 'fix/spi_slave_thread_debug' into 'master'

Browse Source 
fix(spi_slave): correct param check for trans APIs

See merge request espressif/esp-idf!27407
This commit is contained in:
Zhang Wen Xu
2023-12-15 12:08:54 +08:00
parent 34ee34707b 25c17da4bb
commit d5cd130469
3 changed files with 28 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
components/esp_driver_spi/include/driver/spi_slave.h
View File

@@ -122,6 +122,8 @@ esp_err_t spi_slave_free(spi_host_device_t host);
/** /**
* @brief Queue a SPI transaction for execution * @brief Queue a SPI transaction for execution
* *
* @note On esp32, if trans length not WORD aligned, the rx buffer last word memory will still overwritten by DMA HW
*
* Queues a SPI transaction to be executed by this slave device. (The transaction queue size was specified when the slave * Queues a SPI transaction to be executed by this slave device. (The transaction queue size was specified when the slave
* device was initialised via spi_slave_initialize.) This function may block if the queue is full (depending on the * device was initialised via spi_slave_initialize.) This function may block if the queue is full (depending on the
* ticks_to_wait parameter). No SPI operation is directly initiated by this function, the next queued transaction * ticks_to_wait parameter). No SPI operation is directly initiated by this function, the next queued transaction

37
components/esp_driver_spi/src/gpspi/spi_slave.c
View File

@@ -401,10 +401,16 @@ esp_err_t SPI_SLAVE_ATTR spi_slave_queue_trans(spi_host_device_t host, const spi
SPI_CHECK(spihost[host], "host not slave", ESP_ERR_INVALID_ARG); SPI_CHECK(spihost[host], "host not slave", ESP_ERR_INVALID_ARG);
SPI_CHECK(spihost[host]->dma_enabled == 0 || trans_desc->tx_buffer == NULL || esp_ptr_dma_capable(trans_desc->tx_buffer), SPI_CHECK(spihost[host]->dma_enabled == 0 || trans_desc->tx_buffer == NULL || esp_ptr_dma_capable(trans_desc->tx_buffer),
"txdata not in DMA-capable memory", ESP_ERR_INVALID_ARG); "txdata not in DMA-capable memory", ESP_ERR_INVALID_ARG);
// We don't check length WORD alignment for rx when using DMA, seems break DMA requirement,
// however peripheral can also stop DMA from over writing memory even if it not aligned (except esp32).
// ATTENTION!: On esp32, peripheral can NOT stop DMA, if length not WORD aligned,
// remain bytes in last word domain will overwritten by DMA HW, which may cause unexpected issues!
// But driver already used for long time, to avoid breaking changes, we still don't add alignment limit.
SPI_CHECK(spihost[host]->dma_enabled == 0 || trans_desc->rx_buffer == NULL || SPI_CHECK(spihost[host]->dma_enabled == 0 || trans_desc->rx_buffer == NULL ||
(esp_ptr_dma_capable(trans_desc->rx_buffer) && esp_ptr_word_aligned(trans_desc->rx_buffer) && (esp_ptr_dma_capable(trans_desc->rx_buffer) && esp_ptr_word_aligned(trans_desc->rx_buffer) &&
(trans_desc->length % 4 == 0)), (trans_desc->length % 8 == 0)),
"rxdata not in DMA-capable memory or not WORD aligned", ESP_ERR_INVALID_ARG); "rxdata not in DMA-capable memory or not BYTE aligned", ESP_ERR_INVALID_ARG);
SPI_CHECK(trans_desc->length <= spihost[host]->max_transfer_sz * 8, "data transfer > host maximum", ESP_ERR_INVALID_ARG); SPI_CHECK(trans_desc->length <= spihost[host]->max_transfer_sz * 8, "data transfer > host maximum", ESP_ERR_INVALID_ARG);
@@ -423,6 +429,7 @@ esp_err_t SPI_SLAVE_ATTR spi_slave_queue_trans(spi_host_device_t host, const spi
* @note * @note
* This API is used to reset SPI Slave transaction queue. After calling this function: * This API is used to reset SPI Slave transaction queue. After calling this function:
* - The SPI Slave transaction queue will be reset. * - The SPI Slave transaction queue will be reset.
* - The transaction which already mount on hardware will NOT be reset, and can be overwritten by next `trans_queue`
* *
* Therefore, this API shouldn't be called when the corresponding SPI Master is doing an SPI transaction. * Therefore, this API shouldn't be called when the corresponding SPI Master is doing an SPI transaction.
* *
@@ -460,20 +467,20 @@ esp_err_t SPI_SLAVE_ISR_ATTR spi_slave_queue_trans_isr(spi_host_device_t host, c
ESP_RETURN_ON_FALSE_ISR(trans_desc->length <= spihost[host]->max_transfer_sz * 8, ESP_ERR_INVALID_ARG, SPI_TAG, "data transfer > host maximum"); ESP_RETURN_ON_FALSE_ISR(trans_desc->length <= spihost[host]->max_transfer_sz * 8, ESP_ERR_INVALID_ARG, SPI_TAG, "data transfer > host maximum");
if (spihost[host]->dma_enabled) { if (spihost[host]->dma_enabled) {
uint16_t alignment = spihost[host]->internal_mem_align_size; uint16_t alignment = spihost[host]->internal_mem_align_size;
uint32_t buffer_byte_len = (trans_desc->length + 7) / 8; (void) alignment;
ESP_RETURN_ON_FALSE_ISR(\ #if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE
(trans_desc->tx_buffer && \ // For those targets length and addr alignment is still required from Cache side
esp_ptr_dma_capable(trans_desc->tx_buffer) && \ uint32_t buffer_byte_len = (trans_desc->length + 7) / 8;
((((uint32_t)trans_desc->tx_buffer) | buffer_byte_len) & (alignment - 1)) == 0), \ bool tx_aligned = (trans_desc->tx_buffer == NULL) || (esp_ptr_dma_capable(trans_desc->tx_buffer) && ((((uint32_t)trans_desc->tx_buffer | buffer_byte_len) & (alignment - 1)) == 0));
ESP_ERR_INVALID_ARG, SPI_TAG, "txdata addr & len not align to %d bytes or not dma_capable", alignment\ bool rx_aligned = (trans_desc->rx_buffer == NULL) || (esp_ptr_dma_capable(trans_desc->rx_buffer) && ((((uint32_t)trans_desc->rx_buffer | buffer_byte_len) & (alignment - 1)) == 0));
); #else
ESP_RETURN_ON_FALSE_ISR(\ bool tx_aligned = (trans_desc->tx_buffer == NULL) || esp_ptr_dma_capable(trans_desc->tx_buffer);
(trans_desc->rx_buffer && \ bool rx_aligned = (trans_desc->rx_buffer == NULL) || (esp_ptr_dma_capable(trans_desc->rx_buffer) && esp_ptr_word_aligned(trans_desc->rx_buffer) && (trans_desc->length % 8 == 0));
esp_ptr_dma_capable(trans_desc->rx_buffer) && \ #endif
((((uint32_t)trans_desc->rx_buffer) | buffer_byte_len) & (alignment - 1)) == 0), \
ESP_ERR_INVALID_ARG, SPI_TAG, "rxdata addr & len not align to %d bytes or not dma_capable", alignment\ ESP_RETURN_ON_FALSE_ISR(tx_aligned, ESP_ERR_INVALID_ARG, SPI_TAG, "txdata addr & len not align to %d bytes or not dma_capable", alignment);
); ESP_RETURN_ON_FALSE_ISR(rx_aligned, ESP_ERR_INVALID_ARG, SPI_TAG, "rxdata addr & len not align to %d bytes or not dma_capable", alignment);
} }
spi_slave_trans_priv_t priv_trans = { spi_slave_trans_priv_t priv_trans = {

6
components/esp_driver_spi/test_apps/spi/slave/main/test_spi_slave.c
View File

@@ -664,6 +664,8 @@ static IRAM_ATTR void spi_queue_reset_in_isr(void)
}; };
unity_wait_for_signal("Master ready"); unity_wait_for_signal("Master ready");
queue_reset_isr_trans_cnt = 0;
test_queue_reset_isr_fail = 0;
for (uint8_t i = 0; i < 2; i++) { for (uint8_t i = 0; i < 2; i++) {
dummy_trans[i].tx_buffer = dummy_data + i * 64; dummy_trans[i].tx_buffer = dummy_data + i * 64;
dummy_trans[i].rx_buffer = dummy_data + i * 64; dummy_trans[i].rx_buffer = dummy_data + i * 64;
@@ -672,12 +674,12 @@ static IRAM_ATTR void spi_queue_reset_in_isr(void)
} }
// start a trans by normal API first to trigger spi isr // start a trans by normal API first to trigger spi isr
spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg, portMAX_DELAY); spi_slave_queue_trans(TEST_SPI_HOST, &trans_cfg, portMAX_DELAY);
// spi_flash_disable_interrupts_caches_and_other_cpu(); spi_flash_disable_interrupts_caches_and_other_cpu();
esp_rom_printf(DRAM_STR("Send signal: [Slave ready]!\n")); esp_rom_printf(DRAM_STR("Send signal: [Slave ready]!\n"));
while (queue_reset_isr_trans_cnt <= TEST_IRAM_TRANS_NUM) { while (queue_reset_isr_trans_cnt <= TEST_IRAM_TRANS_NUM) {
esp_rom_delay_us(10); esp_rom_delay_us(10);
} }
// spi_flash_enable_interrupts_caches_and_other_cpu(); spi_flash_enable_interrupts_caches_and_other_cpu();
if (test_queue_reset_isr_fail) { if (test_queue_reset_isr_fail) {
TEST_FAIL(); TEST_FAIL();
} }