From 0c3653b1fd7151001143451d4aa95dbf15ee8506 Mon Sep 17 00:00:00 2001 From: Armando Date: Tue, 24 Nov 2020 15:49:38 +0800 Subject: [PATCH 1/2] spi_master: fix master HD mode cannot correctly receive data issue when using DMA. Issue Description: If master is in HD mode, if it sends data without receiving data, it will still enable the RX DMA because of old version ESP32 silicon issue. And because there is no correctly linked RX DMA descriptor, an ``inlink_dscr_error`` intr will be seen, which will influence the following RX transactions. Solution: Trigge this workaround only in FD mode. TODO: Add a test to check if this workaround related issue does exit. If so, reporting to Digital Team is also needed. --- components/hal/spi_hal_iram.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/components/hal/spi_hal_iram.c b/components/hal/spi_hal_iram.c index 91ae71c36b..821c26e543 100644 --- a/components/hal/spi_hal_iram.c +++ b/components/hal/spi_hal_iram.c @@ -151,13 +151,16 @@ void spi_hal_prepare_data(spi_hal_context_t *hal, const spi_hal_dev_config_t *de spi_dma_ll_rx_start(hal->dma_in, hal->dma_config.dmadesc_rx); } - } else { + } +#if CONFIG_IDF_TARGET_ESP32 + else { //DMA temporary workaround: let RX DMA work somehow to avoid the issue in ESP32 v0/v1 silicon - if (hal->dma_enabled) { + if (hal->dma_enabled && !dev->half_duplex) { spi_ll_dma_rx_enable(hal->hw, 1); spi_dma_ll_rx_start(hal->dma_in, 0); } } +#endif if (trans->send_buffer) { if (!hal->dma_enabled) { From 23d08fbe853c54417b74a0ed13324e998b6ed363 Mon Sep 17 00:00:00 2001 From: Armando Date: Mon, 30 Nov 2020 21:21:43 +0800 Subject: [PATCH 2/2] spi_master: add a test for HD master to receive data correctly via dma Issue Description: If master is in HD mode, if it sends data without receiving data, it will still enable the RX DMA because of old version ESP32 silicon issue. And because there is no correctly linked RX DMA descriptor, an inlink_dscr_error intr will be seen, which will influence the following RX transactions. This issue is only found on ESP32. --- .../test/include/test/test_common_spi.h | 7 ++ components/driver/test/test_common_spi.c | 29 +++++- components/driver/test/test_spi_master.c | 94 ++++++++++++++++++- 3 files changed, 128 insertions(+), 2 deletions(-) diff --git a/components/driver/test/include/test/test_common_spi.h b/components/driver/test/include/test/test_common_spi.h index 6b46d304a2..6d87663c39 100644 --- a/components/driver/test/include/test/test_common_spi.h +++ b/components/driver/test/include/test/test_common_spi.h @@ -298,4 +298,11 @@ void master_free_device_bus(spi_device_handle_t spi); //use this function to fix the output source when assign multiple funcitons to a same pin void spitest_gpio_output_sel(uint32_t gpio_num, int func, uint32_t signal_idx); +//use this function to fix the input source when assign multiple funcitons to a same pin +void spitest_gpio_input_sel(uint32_t gpio_num, int func, uint32_t signal_idx); + +//Note this cs_num is the ID of the connected devices' ID, e.g. if 2 devices are connected to the bus, +//then the cs_num of the 1st and 2nd devices are 0 and 1 respectively. +void same_pin_func_sel(spi_bus_config_t bus, spi_device_interface_config_t dev, uint8_t cs_num); + #endif //_TEST_COMMON_SPI_H_ diff --git a/components/driver/test/test_common_spi.c b/components/driver/test/test_common_spi.c index af6c99d551..e302aae771 100644 --- a/components/driver/test/test_common_spi.c +++ b/components/driver/test/test_common_spi.c @@ -203,5 +203,32 @@ void master_free_device_bus(spi_device_handle_t spi) void spitest_gpio_output_sel(uint32_t gpio_num, int func, uint32_t signal_idx) { PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[gpio_num], func); - GPIO.func_out_sel_cfg[gpio_num].func_sel=signal_idx; + GPIO.func_out_sel_cfg[gpio_num].func_sel = signal_idx; +} + +void spitest_gpio_input_sel(uint32_t gpio_num, int func, uint32_t signal_idx) +{ + PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[gpio_num], func); + GPIO.func_in_sel_cfg[signal_idx].func_sel = gpio_num; +} + +//Note this cs_num is the ID of the connected devices' ID, e.g. if 2 devices are connected to the bus, +//then the cs_num of the 1st and 2nd devices are 0 and 1 respectively. +void same_pin_func_sel(spi_bus_config_t bus, spi_device_interface_config_t dev, uint8_t cs_num) +{ + spitest_gpio_output_sel(bus.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spid_out); + spitest_gpio_input_sel(bus.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spid_in); + + spitest_gpio_output_sel(bus.miso_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spiq_out); + spitest_gpio_input_sel(bus.miso_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spiq_in); + + spitest_gpio_output_sel(dev.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spics_out[cs_num]); + spitest_gpio_input_sel(dev.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spics_in); + + spitest_gpio_output_sel(bus.sclk_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spiclk_out); + spitest_gpio_input_sel(bus.sclk_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spiclk_in); + +#if CONFIG_IDF_TARGET_ESP32S2 || CONFIG_IDF_TARGET_ESP32S3 + GPIO.func_in_sel_cfg[FSPIQ_IN_IDX].sig_in_sel = 1; +#endif } diff --git a/components/driver/test/test_spi_master.c b/components/driver/test/test_spi_master.c index ffca8d2522..ae3974a1c8 100644 --- a/components/driver/test/test_spi_master.c +++ b/components/driver/test/test_spi_master.c @@ -936,7 +936,7 @@ TEST_CASE("SPI master variable dummy test", "[spi]") TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &bus_cfg, 0)); TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &dev_cfg, &spi)); - spi_slave_interface_config_t slave_cfg =SPI_SLAVE_TEST_DEFAULT_CONFIG(); + spi_slave_interface_config_t slave_cfg = SPI_SLAVE_TEST_DEFAULT_CONFIG(); TEST_ESP_OK(spi_slave_initialize(TEST_SLAVE_HOST, &bus_cfg, &slave_cfg, 0)); spitest_gpio_output_sel(bus_cfg.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spid_out); @@ -959,9 +959,101 @@ TEST_CASE("SPI master variable dummy test", "[spi]") master_free_device_bus(spi); } +#if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S3) +/** + * This test is to check when the first transaction of the HD master is to send data without receiving data via DMA, + * then if the master could receive data correctly. + * + * Because an old version ESP32 silicon issue, there is a workaround to enable and start the RX DMA in FD/HD mode in + * this condition (TX without RX). And if RX DMA is enabled and started in HD mode, because there is no correctly + * linked RX DMA descriptor, there will be an inlink_dscr_error interrupt emerging, which will influence the following + * RX transactions. + * + * This bug is fixed by triggering this workaround only in FD mode. + * + */ +TEST_CASE("SPI master hd dma TX without RX test", "[spi]") +{ + spi_bus_config_t bus_cfg = SPI_BUS_TEST_DEFAULT_CONFIG(); + TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &bus_cfg, TEST_SPI_HOST)); + + spi_device_handle_t spi; + spi_device_interface_config_t dev_cfg = SPI_DEVICE_TEST_DEFAULT_CONFIG(); + dev_cfg.flags = SPI_DEVICE_HALFDUPLEX; + dev_cfg.clock_speed_hz = 4*1000*1000; + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &dev_cfg, &spi)); + + spi_slave_interface_config_t slave_cfg = SPI_SLAVE_TEST_DEFAULT_CONFIG(); + TEST_ESP_OK(spi_slave_initialize(TEST_SLAVE_HOST, &bus_cfg, &slave_cfg, TEST_SLAVE_HOST)); + + same_pin_func_sel(bus_cfg, dev_cfg, 0); + + uint32_t buf_size = 32; + uint8_t *mst_send_buf = heap_caps_malloc(buf_size, MALLOC_CAP_DMA); + uint8_t *mst_recv_buf = heap_caps_calloc(buf_size, 1, MALLOC_CAP_DMA); + uint8_t *slv_send_buf = heap_caps_malloc(buf_size, MALLOC_CAP_DMA); + uint8_t *slv_recv_buf = heap_caps_calloc(buf_size, 1, MALLOC_CAP_DMA); + + srand(199); + for (int i = 0; i < buf_size; i++) { + mst_send_buf[i] = rand(); + } + + //1. Master sends without receiving, no rx_buffer is set + spi_slave_transaction_t slave_trans = { + .rx_buffer = slv_recv_buf, + .length = buf_size * 8, + }; + TEST_ESP_OK(spi_slave_queue_trans(TEST_SLAVE_HOST, &slave_trans, portMAX_DELAY)); + + spi_transaction_t master_trans = { + .tx_buffer = mst_send_buf, + .length = buf_size * 8, + }; + TEST_ESP_OK(spi_device_transmit(spi, &master_trans)); + + spi_slave_transaction_t *ret_slave; + TEST_ESP_OK(spi_slave_get_trans_result(TEST_SLAVE_HOST, &ret_slave, portMAX_DELAY)); + + spitest_cmp_or_dump(mst_send_buf, slv_recv_buf, buf_size); + + //2. Master receives data + for (int i = 100; i < 110; i++) { + + srand(i); + for (int j = 0; j < buf_size; j++) { + slv_send_buf[j] = rand(); + } + slave_trans = (spi_slave_transaction_t) {}; + slave_trans.tx_buffer = slv_send_buf; + slave_trans.length = buf_size * 8; + TEST_ESP_OK(spi_slave_queue_trans(TEST_SLAVE_HOST, &slave_trans, portMAX_DELAY)); + + vTaskDelay(50); + + master_trans = (spi_transaction_t) {}; + master_trans.rx_buffer = mst_recv_buf; + master_trans.rxlength = buf_size * 8; + TEST_ESP_OK(spi_device_transmit(spi, &master_trans)); + + TEST_ESP_OK(spi_slave_get_trans_result(TEST_SLAVE_HOST, &ret_slave, portMAX_DELAY)); + + spitest_cmp_or_dump(slv_send_buf, mst_recv_buf, buf_size); + } + + free(mst_send_buf); + free(mst_recv_buf); + free(slv_send_buf); + free(slv_recv_buf); + spi_slave_free(TEST_SLAVE_HOST); + master_free_device_bus(spi); +} +#endif // #if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S3) + //There is only one GPSPI controller, so single-board test is disabled. #endif //#if !DISABLED_FOR_TARGETS(ESP32C3) + #if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32C3) /******************************************************************************** * Test SPI transaction interval