From 4943844764da942afcf73ee75211255e415c13c3 Mon Sep 17 00:00:00 2001 From: Armando Date: Fri, 14 Apr 2023 11:43:38 +0800 Subject: [PATCH 1/2] spi: added transaction length check to refuse longer than hardware supported length --- components/driver/spi/gpspi/spi_master.c | 9 +++++++++ components/hal/esp32/include/hal/spi_ll.h | 3 ++- components/hal/esp32c2/include/hal/spi_ll.h | 3 ++- components/hal/esp32c3/include/hal/spi_ll.h | 3 ++- components/hal/esp32c6/include/hal/spi_ll.h | 3 ++- components/hal/esp32h2/include/hal/spi_ll.h | 3 ++- components/hal/esp32s2/include/hal/spi_ll.h | 3 ++- components/hal/esp32s3/include/hal/spi_ll.h | 3 ++- 8 files changed, 23 insertions(+), 7 deletions(-) diff --git a/components/driver/spi/gpspi/spi_master.c b/components/driver/spi/gpspi/spi_master.c index f2129c29ae..ff9b71ebf2 100644 --- a/components/driver/spi/gpspi/spi_master.c +++ b/components/driver/spi/gpspi/spi_master.c @@ -123,6 +123,7 @@ We have two bits to control the interrupt: #include "soc/soc_memory_layout.h" #include "driver/gpio.h" #include "hal/spi_hal.h" +#include "hal/spi_ll.h" #include "esp_heap_caps.h" @@ -803,6 +804,14 @@ static SPI_MASTER_ISR_ATTR esp_err_t check_trans_valid(spi_device_handle_t handl //Dummy phase is not available when both data out and in are enabled, regardless of FD or HD mode. SPI_CHECK(!tx_enabled || !rx_enabled || !dummy_enabled || !extra_dummy_enabled, "Dummy phase is not available when both data out and in are enabled", ESP_ERR_INVALID_ARG); + if (bus_attr->dma_enabled) { + SPI_CHECK(trans_desc->length <= SPI_LL_DMA_MAX_BIT_LEN, "txdata transfer > hardware max supported len", ESP_ERR_INVALID_ARG); + SPI_CHECK(trans_desc->rxlength <= SPI_LL_DMA_MAX_BIT_LEN, "rxdata transfer > hardware max supported len", ESP_ERR_INVALID_ARG); + } else { + SPI_CHECK(trans_desc->length <= SPI_LL_CPU_MAX_BIT_LEN, "txdata transfer > hardware max supported len", ESP_ERR_INVALID_ARG); + SPI_CHECK(trans_desc->rxlength <= SPI_LL_CPU_MAX_BIT_LEN, "rxdata transfer > hardware max supported len", ESP_ERR_INVALID_ARG); + } + return ESP_OK; } diff --git a/components/hal/esp32/include/hal/spi_ll.h b/components/hal/esp32/include/hal/spi_ll.h index 8b65624bf0..c1fd641379 100644 --- a/components/hal/esp32/include/hal/spi_ll.h +++ b/components/hal/esp32/include/hal/spi_ll.h @@ -39,7 +39,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? &SPI1:((ID)==1? &SPI2 : &SPI3)) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 24) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 24) //reg len: 24 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32c2/include/hal/spi_ll.h b/components/hal/esp32c2/include/hal/spi_ll.h index 377da88758..f1da30f627 100644 --- a/components/hal/esp32c2/include/hal/spi_ll.h +++ b/components/hal/esp32c2/include/hal/spi_ll.h @@ -38,7 +38,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):&GPSPI2) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 18) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 18) //reg len: 18 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32c3/include/hal/spi_ll.h b/components/hal/esp32c3/include/hal/spi_ll.h index 9cdc2c344a..140795c5f0 100644 --- a/components/hal/esp32c3/include/hal/spi_ll.h +++ b/components/hal/esp32c3/include/hal/spi_ll.h @@ -38,7 +38,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):&GPSPI2) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 18) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 18) //reg len: 18 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32c6/include/hal/spi_ll.h b/components/hal/esp32c6/include/hal/spi_ll.h index 3a97b192ca..b43d43859d 100644 --- a/components/hal/esp32c6/include/hal/spi_ll.h +++ b/components/hal/esp32c6/include/hal/spi_ll.h @@ -39,7 +39,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):&GPSPI2) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 18) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 18) //reg len: 18 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32h2/include/hal/spi_ll.h b/components/hal/esp32h2/include/hal/spi_ll.h index aa4d1964ed..64de7c12fe 100644 --- a/components/hal/esp32h2/include/hal/spi_ll.h +++ b/components/hal/esp32h2/include/hal/spi_ll.h @@ -41,7 +41,8 @@ extern "C" { #define SPI_LL_PERIPH_CLK_FREQ (80 * 1000000) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):&GPSPI2) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 18) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 18) //reg len: 18 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32s2/include/hal/spi_ll.h b/components/hal/esp32s2/include/hal/spi_ll.h index ab83769b00..7b09ae2a2b 100644 --- a/components/hal/esp32s2/include/hal/spi_ll.h +++ b/components/hal/esp32s2/include/hal/spi_ll.h @@ -41,7 +41,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):((ID)==1? &GPSPI2 : &GPSPI3)) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 23) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 23) //reg len: 23 bits +#define SPI_LL_CPU_MAX_BIT_LEN (18 * 32) //Fifo len: 18 words /** * The data structure holding calculated clock configuration. Since the diff --git a/components/hal/esp32s3/include/hal/spi_ll.h b/components/hal/esp32s3/include/hal/spi_ll.h index 2477847af2..a5e1fdab29 100644 --- a/components/hal/esp32s3/include/hal/spi_ll.h +++ b/components/hal/esp32s3/include/hal/spi_ll.h @@ -40,7 +40,8 @@ extern "C" { #define HAL_SPI_SWAP_DATA_TX(data, len) HAL_SWAP32((uint32_t)(data) << (32 - len)) #define SPI_LL_GET_HW(ID) ((ID)==0? ({abort();NULL;}):((ID)==1? &GPSPI2 : &GPSPI3)) -#define SPI_LL_DATA_MAX_BIT_LEN (1 << 18) +#define SPI_LL_DMA_MAX_BIT_LEN (1 << 18) //reg len: 18 bits +#define SPI_LL_CPU_MAX_BIT_LEN (16 * 32) //Fifo len: 16 words /** * The data structure holding calculated clock configuration. Since the From 8702e490573bf2d7d44bd57b2f68cfbd8beca06e Mon Sep 17 00:00:00 2001 From: Armando Date: Mon, 24 Apr 2023 14:21:13 +0800 Subject: [PATCH 2/2] spi: added an API to get max transaction length and use in spi lcd driver --- components/driver/spi/gpspi/spi_master.c | 19 ++++++++++++++++++- .../driver/spi/include/driver/spi_master.h | 12 ++++++++++++ components/esp_lcd/src/esp_lcd_panel_io_spi.c | 9 ++++++--- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/components/driver/spi/gpspi/spi_master.c b/components/driver/spi/gpspi/spi_master.c index ff9b71ebf2..8aa328b1ac 100644 --- a/components/driver/spi/gpspi/spi_master.c +++ b/components/driver/spi/gpspi/spi_master.c @@ -111,6 +111,7 @@ We have two bits to control the interrupt: */ #include +#include #include "esp_private/spi_common_internal.h" #include "driver/spi_master.h" #include "esp_clk_tree.h" @@ -126,7 +127,6 @@ We have two bits to control the interrupt: #include "hal/spi_ll.h" #include "esp_heap_caps.h" - typedef struct spi_device_t spi_device_t; /// struct to hold private transaction data (like tx and rx buffer for DMA). @@ -1111,3 +1111,20 @@ esp_err_t SPI_MASTER_ISR_ATTR spi_device_polling_transmit(spi_device_handle_t ha return spi_device_polling_end(handle, portMAX_DELAY); } + +esp_err_t spi_bus_get_max_transaction_len(spi_host_device_t host_id, size_t *max_bytes) +{ + SPI_CHECK(is_valid_host(host_id), "invalid host", ESP_ERR_INVALID_ARG); + if (bus_driver_ctx[host_id] == NULL || max_bytes == NULL) { + return ESP_ERR_INVALID_ARG; + } + + spi_host_t *host = bus_driver_ctx[host_id]; + if (host->bus_attr->dma_enabled) { + *max_bytes = MIN(host->bus_attr->max_transfer_sz, (SPI_LL_DMA_MAX_BIT_LEN / 8)); + } else { + *max_bytes = MIN(host->bus_attr->max_transfer_sz, (SPI_LL_CPU_MAX_BIT_LEN / 8)); + } + + return ESP_OK; +} diff --git a/components/driver/spi/include/driver/spi_master.h b/components/driver/spi/include/driver/spi_master.h index 8c1d140d2d..5cc1f469c4 100644 --- a/components/driver/spi/include/driver/spi_master.h +++ b/components/driver/spi/include/driver/spi_master.h @@ -391,6 +391,18 @@ void spi_get_timing(bool gpio_is_used, int input_delay_ns, int eff_clk, int *dum */ int spi_get_freq_limit(bool gpio_is_used, int input_delay_ns); +/** + * @brief Get max length (in bytes) of one transaction + * + * @param host_id SPI peripheral + * @param[out] max_bytes Max length of one transaction, in bytes + * + * @return + * - ESP_OK: On success + * - ESP_ERR_INVALID_ARG: Invalid argument + */ +esp_err_t spi_bus_get_max_transaction_len(spi_host_device_t host_id, size_t *max_bytes); + #ifdef __cplusplus } #endif diff --git a/components/esp_lcd/src/esp_lcd_panel_io_spi.c b/components/esp_lcd/src/esp_lcd_panel_io_spi.c index 945bd2326b..e4a7ca730e 100644 --- a/components/esp_lcd/src/esp_lcd_panel_io_spi.c +++ b/components/esp_lcd/src/esp_lcd_panel_io_spi.c @@ -21,7 +21,6 @@ #include "esp_log.h" #include "esp_check.h" #include "esp_lcd_common.h" -#include "esp_private/spi_common_internal.h" static const char *TAG = "lcd_panel.io.spi"; @@ -107,9 +106,13 @@ esp_err_t esp_lcd_new_panel_io_spi(esp_lcd_spi_bus_handle_t bus, const esp_lcd_p spi_panel_io->base.tx_color = panel_io_spi_tx_color; spi_panel_io->base.del = panel_io_spi_del; spi_panel_io->base.register_event_callbacks = panel_io_spi_register_event_callbacks; - spi_panel_io->spi_trans_max_bytes = spi_bus_get_attr((spi_host_device_t)bus)->max_transfer_sz; + + size_t max_trans_bytes = 0; + ESP_GOTO_ON_ERROR(spi_bus_get_max_transaction_len((spi_host_device_t)bus, &max_trans_bytes), err, TAG, "get spi max transaction len failed"); + spi_panel_io->spi_trans_max_bytes = max_trans_bytes; + *ret_io = &(spi_panel_io->base); - ESP_LOGD(TAG, "new spi lcd panel io @%p", spi_panel_io); + ESP_LOGD(TAG, "new spi lcd panel io @%p, max_trans_bytes: %d", spi_panel_io, (int)max_trans_bytes); return ESP_OK;