From a7ad8bc8737e17ec085faaacc8916e7646263f03 Mon Sep 17 00:00:00 2001 From: michael Date: Thu, 6 Sep 2018 14:47:45 +0800 Subject: [PATCH] spi_slave: fix the issue rx dma get broken by master unexpected transaction --- components/driver/include/driver/spi_common.h | 44 ++++++++++++++----- components/driver/spi_common.c | 20 +++++++++ components/driver/spi_slave.c | 20 ++++++--- 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/components/driver/include/driver/spi_common.h b/components/driver/include/driver/spi_common.h index 6adcc06a9..e093b89f0 100644 --- a/components/driver/include/driver/spi_common.h +++ b/components/driver/include/driver/spi_common.h @@ -46,7 +46,7 @@ typedef enum { * @brief This is a configuration structure for a SPI bus. * * You can use this structure to specify the GPIO pins of the bus. Normally, the driver will use the - * GPIO matrix to route the signals. An exception is made when all signals either can be routed through + * GPIO matrix to route the signals. An exception is made when all signals either can be routed through * the IO_MUX or are -1. In that case, the IO_MUX is used, allowing for >40MHz speeds. * * @note Be advised that the slave driver does not use the quadwp/quadhd lines and fields in spi_bus_config_t refering to these lines will be ignored and can thus safely be left uninitialized. @@ -81,20 +81,20 @@ bool spicommon_periph_free(spi_host_device_t host); /** * @brief Try to claim a SPI DMA channel - * + * * Call this if your driver wants to use SPI with a DMA channnel. - * + * * @param dma_chan channel to claim - * + * * @return True if success; false otherwise. */ bool spicommon_dma_chan_claim(int dma_chan); /** * @brief Return the SPI DMA channel so other driver can claim it, or just to power down DMA. - * + * * @param dma_chan channel to return - * + * * @return True if success; false otherwise. */ bool spicommon_dma_chan_free(int dma_chan); @@ -107,7 +107,7 @@ bool spicommon_dma_chan_free(int dma_chan); * @brief Connect a SPI peripheral to GPIO pins * * This routine is used to connect a SPI peripheral to the IO-pads and DMA channel given in - * the arguments. Depending on the IO-pads requested, the routing is done either using the + * the arguments. Depending on the IO-pads requested, the routing is done either using the * IO_mux or using the GPIO matrix. * * @param host SPI peripheral to be routed @@ -116,7 +116,7 @@ bool spicommon_dma_chan_free(int dma_chan); * @param flags Combination of SPICOMMON_BUSFLAG_* flags * @param[out] is_native A value of 'true' will be written to this address if the GPIOs can be * routed using the IO_mux, 'false' if the GPIO matrix is used. - * @return + * @return * - ESP_ERR_INVALID_ARG if parameter is invalid * - ESP_OK on success */ @@ -126,7 +126,7 @@ esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_conf * @brief Free the IO used by a SPI peripheral * * @param host SPI peripheral to be freed - * @return + * @return * - ESP_ERR_INVALID_ARG if parameter is invalid * - ESP_OK on success */ @@ -180,6 +180,26 @@ void spicommon_setup_dma_desc_links(lldesc_t *dmadesc, int len, const uint8_t *d */ spi_dev_t *spicommon_hw_for_host(spi_host_device_t host); + +/** Temporarily connect CS signal input to high to avoid slave detecting unexpected transactions. + * + * @note Don't use this in the application. + * + * @param host The spi host. + */ +void spicommon_freeze_cs(spi_host_device_t host); + +/** Use this function instead of cs_initial to avoid overwrite the output config + * This is used in test by internal gpio matrix connections + * + * @note Don't use this in the application. + * + * @param host The spi host. + * @param cs_io_num GPIO number of the CS pin. + * @param iomux The peripheral is using iomux pins. + */ +void spicommon_restore_cs(spi_host_device_t host, int cs_io_num, bool iomux); + /** * @brief Get the IRQ source for a specific SPI host * @@ -201,10 +221,10 @@ typedef void(*dmaworkaround_cb_t)(void *arg); * @note In some (well-defined) cases in the ESP32 (at least rev v.0 and v.1), a SPI DMA channel will get confused. This can be remedied * by resetting the SPI DMA hardware in case this happens. Unfortunately, the reset knob used for thsi will reset _both_ DMA channels, and * as such can only done safely when both DMA channels are idle. These functions coordinate this. - * + * * Essentially, when a reset is needed, a driver can request this using spicommon_dmaworkaround_req_reset. This is supposed to be called - * with an user-supplied function as an argument. If both DMA channels are idle, this call will reset the DMA subsystem and return true. - * If the other DMA channel is still busy, it will return false; as soon as the other DMA channel is done, however, it will reset the + * with an user-supplied function as an argument. If both DMA channels are idle, this call will reset the DMA subsystem and return true. + * If the other DMA channel is still busy, it will return false; as soon as the other DMA channel is done, however, it will reset the * DMA subsystem and call the callback. The callback is then supposed to be used to continue the SPI drivers activity. * * @param dmachan DMA channel associated with the SPI host that needs a reset diff --git a/components/driver/spi_common.c b/components/driver/spi_common.c index ad51379a8..759be5cd9 100644 --- a/components/driver/spi_common.c +++ b/components/driver/spi_common.c @@ -371,6 +371,26 @@ void IRAM_ATTR spicommon_setup_dma_desc_links(lldesc_t *dmadesc, int len, const dmadesc[n - 1].qe.stqe_next = NULL; } +void spicommon_freeze_cs(spi_host_device_t host) +{ + gpio_matrix_in(0x38, io_signal[host].spics_in, false); +} + +static void IOMUX_IN(uint32_t gpio, uint32_t signal_idx) +{ + GPIO.func_in_sel_cfg[signal_idx].sig_in_sel = 0; + PIN_INPUT_ENABLE(GPIO_PIN_MUX_REG[gpio]); +} + +void spicommon_restore_cs(spi_host_device_t host, int cs_io_num, bool iomux) +{ + if (iomux) { + IOMUX_IN(cs_io_num, io_signal[host].spics_in); + } else { + gpio_matrix_in(cs_io_num, io_signal[host].spics_in, false); + } +} + /* Code for workaround for DMA issue in ESP32 v0/v1 silicon diff --git a/components/driver/spi_slave.c b/components/driver/spi_slave.c index c65cb3b52..8ed509446 100644 --- a/components/driver/spi_slave.c +++ b/components/driver/spi_slave.c @@ -50,6 +50,7 @@ static const char *SPI_TAG = "spi_slave"; #define VALID_HOST(x) (x>SPI_HOST && x<=VSPI_HOST) typedef struct { + int id; spi_slave_interface_config_t cfg; intr_handle_t intr; spi_dev_t *hw; @@ -79,7 +80,7 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b spi_chan_claimed=spicommon_periph_claim(host); SPI_CHECK(spi_chan_claimed, "host already in use", ESP_ERR_INVALID_STATE); - + if ( dma_chan != 0 ) { dma_chan_claimed=spicommon_dma_chan_claim(dma_chan); if ( !dma_chan_claimed ) { @@ -92,10 +93,13 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b if (spihost[host] == NULL) goto nomem; memset(spihost[host], 0, sizeof(spi_slave_t)); memcpy(&spihost[host]->cfg, slave_config, sizeof(spi_slave_interface_config_t)); + spihost[host]->id = host; spicommon_bus_initialize_io(host, bus_config, dma_chan, SPICOMMON_BUSFLAG_SLAVE, &native); gpio_set_direction(slave_config->spics_io_num, GPIO_MODE_INPUT); - spicommon_cs_initialize(host, slave_config->spics_io_num, 0, native == false); + spicommon_cs_initialize(host, slave_config->spics_io_num, 0, native==false); + // The slave DMA suffers from unexpected transactions. Forbid reading if DMA is enabled by disabling the CS line. + if (dma_chan != 0) spicommon_freeze_cs(host); spihost[host]->no_gpio_matrix = native; spihost[host]->dma_chan = dma_chan; if (dma_chan != 0) { @@ -239,9 +243,9 @@ esp_err_t spi_slave_queue_trans(spi_host_device_t host, const spi_slave_transact BaseType_t r; SPI_CHECK(VALID_HOST(host), "invalid host", ESP_ERR_INVALID_ARG); SPI_CHECK(spihost[host], "host not slave", ESP_ERR_INVALID_ARG); - SPI_CHECK(spihost[host]->dma_chan == 0 || trans_desc->tx_buffer==NULL || esp_ptr_dma_capable(trans_desc->tx_buffer), + SPI_CHECK(spihost[host]->dma_chan == 0 || trans_desc->tx_buffer==NULL || esp_ptr_dma_capable(trans_desc->tx_buffer), "txdata not in DMA-capable memory", ESP_ERR_INVALID_ARG); - SPI_CHECK(spihost[host]->dma_chan == 0 || trans_desc->rx_buffer==NULL || esp_ptr_dma_capable(trans_desc->rx_buffer), + SPI_CHECK(spihost[host]->dma_chan == 0 || trans_desc->rx_buffer==NULL || esp_ptr_dma_capable(trans_desc->rx_buffer), "rxdata not in DMA-capable memory", ESP_ERR_INVALID_ARG); SPI_CHECK(trans_desc->length <= spihost[host]->max_transfer_sz * 8, "data transfer > host maximum", ESP_ERR_INVALID_ARG); @@ -325,8 +329,11 @@ static void IRAM_ATTR spi_intr(void *arg) if (!host->hw->slave.trans_done) return; if (host->cur_trans) { + // When DMA is enabled, the slave rx dma suffers from unexpected transactions. Forbid reading until transaction ready. + if (host->dma_chan != 0) spicommon_freeze_cs(host->id); + //when data of cur_trans->length are all sent, the slv_rdata_bit - //will be the length sent-1 (i.e. cur_trans->length-1 ), otherwise + //will be the length sent-1 (i.e. cur_trans->length-1 ), otherwise //the length sent. host->cur_trans->trans_len = host->hw->slv_rd_bit.slv_rdata_bit; if ( host->cur_trans->trans_len == host->cur_trans->length - 1 ) { @@ -433,6 +440,9 @@ static void IRAM_ATTR spi_intr(void *arg) host->hw->user.usr_mosi = (trans->tx_buffer == NULL) ? 0 : 1; host->hw->user.usr_miso = (trans->rx_buffer == NULL) ? 0 : 1; + //The slave rx dma get disturbed by unexpected transaction. Only connect the CS when slave is ready. + if (host->dma_chan != 0) spicommon_restore_cs(host->id, host->cfg.spics_io_num, host->no_gpio_matrix); + //Kick off transfer host->hw->cmd.usr = 1; if (host->cfg.post_setup_cb) host->cfg.post_setup_cb(trans);