From 2600f43822aa81b5401c171cb174bcdc7baf07d0 Mon Sep 17 00:00:00 2001 From: "Michael (XIAO Xufeng)" Date: Fri, 28 Sep 2018 11:36:14 +0800 Subject: [PATCH 1/3] spi_master: fix the command and address field when LSB_FIRST enabled Resolves https://github.com/espressif/esp-idf/issues/2444. --- components/driver/spi_master.c | 39 ++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/components/driver/spi_master.c b/components/driver/spi_master.c index 74e4ee16b1..0abf93c1bb 100644 --- a/components/driver/spi_master.c +++ b/components/driver/spi_master.c @@ -934,19 +934,36 @@ static void SPI_MASTER_ISR_ATTR spi_new_trans(spi_device_t *dev, spi_trans_priv_ host->hw->user.usr_addr=addrlen ? 1 : 0; host->hw->user.usr_command=cmdlen ? 1 : 0; - /* Output command will be sent from bit 7 to 0 of command_value, and - * then bit 15 to 8 of the same register field. Shift and swap to send - * more straightly. - */ - host->hw->user2.usr_command_value = SPI_SWAP_DATA_TX(trans->cmd, cmdlen); + if ((dev->cfg.flags & SPI_DEVICE_TXBIT_LSBFIRST)==0) { + /* Output command will be sent from bit 7 to 0 of command_value, and + * then bit 15 to 8 of the same register field. Shift and swap to send + * more straightly. + */ + host->hw->user2.usr_command_value = SPI_SWAP_DATA_TX(trans->cmd, cmdlen); - // shift the address to MSB of addr (and maybe slv_wr_status) register. - // output address will be sent from MSB to LSB of addr register, then comes the MSB to LSB of slv_wr_status register. - if (addrlen>32) { - host->hw->addr = trans->addr >> (addrlen- 32); - host->hw->slv_wr_status = trans->addr << (64 - addrlen); + // shift the address to MSB of addr (and maybe slv_wr_status) register. + // output address will be sent from MSB to LSB of addr register, then comes the MSB to LSB of slv_wr_status register. + if (addrlen > 32) { + host->hw->addr = trans->addr >> (addrlen - 32); + host->hw->slv_wr_status = trans->addr << (64 - addrlen); + } else { + host->hw->addr = trans->addr << (32 - addrlen); + } } else { - host->hw->addr = trans->addr << (32 - addrlen); + /* The output command start from bit0 to bit 15, kept as is. + * The output address start from the LSB of the highest byte, i.e. + * addr[24] -> addr[31] + * ... + * addr[0] -> addr[7] + * slv_wr_status[24] -> slv_wr_status[31] + * ... + * slv_wr_status[0] -> slv_wr_status[7] + * So swap the byte order to let the LSB sent first. + */ + host->hw->user2.usr_command_value = trans->cmd; + uint64_t addr = __builtin_bswap64(trans->addr); + host->hw->addr = addr>>32; + host->hw->slv_wr_status = addr; } if ((!(dev->cfg.flags & SPI_DEVICE_HALFDUPLEX) && trans_buf->buffer_to_rcv) || From e5ed450d957a9a38b4743738e5566863bbe6e37b Mon Sep 17 00:00:00 2001 From: michael Date: Tue, 2 Oct 2018 17:09:02 +0800 Subject: [PATCH 2/3] spi: move gpio direction config to common func for coinsistence (MINOR CHANGE) --- components/driver/spi_common.c | 1 + components/driver/spi_master.c | 1 - components/driver/spi_slave.c | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/components/driver/spi_common.c b/components/driver/spi_common.c index cce50cb8ff..fbaabaa734 100644 --- a/components/driver/spi_common.c +++ b/components/driver/spi_common.c @@ -312,6 +312,7 @@ void spicommon_cs_initialize(spi_host_device_t host, int cs_io_num, int cs_num, gpio_iomux_out(cs_io_num, FUNC_SPI, false); } else { //Use GPIO matrix + gpio_set_direction(cs_io_num, GPIO_MODE_INPUT_OUTPUT); gpio_matrix_out(cs_io_num, spi_periph_signal[host].spics_out[cs_num], false, false); if (cs_num == 0) gpio_matrix_in(cs_io_num, spi_periph_signal[host].spics_in, false); PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[cs_io_num], FUNC_GPIO); diff --git a/components/driver/spi_master.c b/components/driver/spi_master.c index 0abf93c1bb..ffbedd203d 100644 --- a/components/driver/spi_master.c +++ b/components/driver/spi_master.c @@ -484,7 +484,6 @@ Specify ``SPI_DEVICE_NO_DUMMY`` to ignore this checking. Then you can output dat //Set CS pin, CS options if (dev_config->spics_io_num >= 0) { - gpio_set_direction(dev_config->spics_io_num, GPIO_MODE_OUTPUT); spicommon_cs_initialize(host, dev_config->spics_io_num, freecs, !(spihost[host]->flags&SPICOMMON_BUSFLAG_NATIVE_PINS)); } if (dev_config->flags&SPI_DEVICE_CLK_AS_CS) { diff --git a/components/driver/spi_slave.c b/components/driver/spi_slave.c index ca5db19e90..970bc69108 100644 --- a/components/driver/spi_slave.c +++ b/components/driver/spi_slave.c @@ -136,7 +136,6 @@ esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *b ret = err; goto cleanup; } - gpio_set_direction(slave_config->spics_io_num, GPIO_MODE_INPUT); spicommon_cs_initialize(host, slave_config->spics_io_num, 0, !bus_is_iomux(spihost[host])); // The slave DMA suffers from unexpected transactions. Forbid reading if DMA is enabled by disabling the CS line. if (dma_chan != 0) freeze_cs(spihost[host]); From 6b180f70acf28b928d10320ef994e045037e7647 Mon Sep 17 00:00:00 2001 From: michael Date: Thu, 4 Oct 2018 00:16:21 +0800 Subject: [PATCH 3/3] test: modify the command/address test a bit to test the LSBFIRST feature --- components/driver/test/test_spi_master.c | 234 ++++++++++++----------- 1 file changed, 127 insertions(+), 107 deletions(-) diff --git a/components/driver/test/test_spi_master.c b/components/driver/test/test_spi_master.c index 1ead517c64..b431a22142 100644 --- a/components/driver/test/test_spi_master.c +++ b/components/driver/test/test_spi_master.c @@ -800,6 +800,7 @@ static void task_slave(void* arg) t.tx_buffer = txdata.start; t.rx_buffer = recvbuf+8; //loop until trans_len != 0 to skip glitches + memset(recvbuf, 0x66, sizeof(recvbuf)); do { TEST_ESP_OK( spi_slave_transmit( context->spi, &t, portMAX_DELAY ) ); } while ( t.trans_len == 0 ); @@ -810,26 +811,134 @@ static void task_slave(void* arg) } } -TEST_CASE("SPI master variable cmd & addr test","[spi]") +#define TEST_SPI_HOST HSPI_HOST +#define TEST_SLAVE_HOST VSPI_HOST + +static uint8_t bitswap(uint8_t in) { - uint8_t *tx_buf=master_send; - uint8_t rx_buf[320]; - uint8_t *rx_buf_ptr = rx_buf; - - spi_slave_task_context_t slave_context = {}; - esp_err_t err = init_slave_context( &slave_context ); - TEST_ASSERT( err == ESP_OK ); + uint8_t out = 0; + for (int i = 0; i < 8; i++) { + out = out >> 1; + if (in&0x80) out |= 0x80; + in = in << 1; + } + return out; +} +void test_cmd_addr(spi_slave_task_context_t *slave_context, bool lsb_first) +{ spi_device_handle_t spi; + ESP_LOGI(MASTER_TAG, ">>>>>>>>> TEST %s FIRST <<<<<<<<<<<", lsb_first?"LSB":"MSB"); + //initial master, mode 0, 1MHz spi_bus_config_t buscfg=SPI_BUS_TEST_DEFAULT_CONFIG(); - TEST_ESP_OK(spi_bus_initialize(HSPI_HOST, &buscfg, 1)); + TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, 1)); spi_device_interface_config_t devcfg=SPI_DEVICE_TEST_DEFAULT_CONFIG(); - devcfg.clock_speed_hz = 1*1000*1000; //currently only up to 4MHz for internel connect - devcfg.mode = 0; - devcfg.cs_ena_posttrans = 2; - TEST_ESP_OK(spi_bus_add_device(HSPI_HOST, &devcfg, &spi)); + devcfg.clock_speed_hz = 1*1000*1000; + if (lsb_first) devcfg.flags |= SPI_DEVICE_BIT_LSBFIRST; + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &spi)); + + //connecting pins to two peripherals breaks the output, fix it. + gpio_output_sel(buscfg.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spid_out); + gpio_output_sel(buscfg.miso_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spiq_out); + gpio_output_sel(devcfg.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spics_out[0]); + gpio_output_sel(buscfg.sclk_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spiclk_out); + + for (int i= 0; i < 8; i++) { + //prepare slave tx data + slave_txdata_t slave_txdata = (slave_txdata_t) { + .start = slave_send, + .len = 256, + }; + xQueueSend(slave_context->data_to_send, &slave_txdata, portMAX_DELAY); + + vTaskDelay(50); + //prepare master tx data + int cmd_bits = (i+1)*2; + int addr_bits = 56-8*i; + int round_up = (cmd_bits+addr_bits+7)/8*8; + addr_bits = round_up - cmd_bits; + + spi_transaction_ext_t trans = (spi_transaction_ext_t) { + .base = { + .flags = SPI_TRANS_VARIABLE_CMD | SPI_TRANS_VARIABLE_ADDR, + .addr = 0x456789abcdef0123, + .cmd = 0xcdef, + }, + .command_bits = cmd_bits, + .address_bits = addr_bits, + }; + + ESP_LOGI( MASTER_TAG, "===== test%d =====", i ); + ESP_LOGI(MASTER_TAG, "cmd_bits： %d, addr_bits: %d", cmd_bits, addr_bits); + TEST_ESP_OK(spi_device_transmit(spi, (spi_transaction_t*)&trans)); + //wait for both master and slave end + + size_t rcv_len; + slave_rxdata_t *rcv_data = xRingbufferReceive(slave_context->data_received, &rcv_len, portMAX_DELAY); + rcv_len-=8; + uint8_t *buffer = rcv_data->data; + + ESP_LOGI(SLAVE_TAG, "trans_len: %d", rcv_len); + TEST_ASSERT_EQUAL(rcv_len, (rcv_data->len+7)/8); + TEST_ASSERT_EQUAL(rcv_data->len, cmd_bits+addr_bits); + ESP_LOG_BUFFER_HEX("slave rx", buffer, rcv_len); + + uint16_t cmd_expected = trans.base.cmd & (BIT(cmd_bits) - 1); + uint64_t addr_expected = trans.base.addr & ((1ULL<> (16-cmd_bits); + int remain_bits = cmd_bits % 8; + + uint64_t addr_got = *(uint64_t*)data_ptr; + data_ptr += 8; + addr_got = __builtin_bswap64(addr_got); + addr_got = (addr_got << remain_bits); + addr_got |= (*data_ptr >> (8-remain_bits)); + addr_got = addr_got >> (64-addr_bits); + + if (lsb_first) { + cmd_got = __builtin_bswap16(cmd_got); + addr_got = __builtin_bswap64(addr_got); + + uint8_t *swap_ptr = (uint8_t*)&cmd_got; + swap_ptr[0] = bitswap(swap_ptr[0]); + swap_ptr[1] = bitswap(swap_ptr[1]); + cmd_got = cmd_got >> (16-cmd_bits); + + swap_ptr = (uint8_t*)&addr_got; + for (int j = 0; j < 8; j++) swap_ptr[j] = bitswap(swap_ptr[j]); + addr_got = addr_got >> (64-addr_bits); + } + + ESP_LOGI(SLAVE_TAG, "cmd_got: %04X, addr_got: %08X%08X", cmd_got, (uint32_t)(addr_got>>32), (uint32_t)addr_got); + + TEST_ASSERT_EQUAL_HEX16(cmd_expected, cmd_got); + if (addr_bits > 0) { + TEST_ASSERT_EQUAL_HEX32(addr_expected, addr_got); + TEST_ASSERT_EQUAL_HEX32(addr_expected >> 8, addr_got >> 8); + } + + //clean + vRingbufferReturnItem(slave_context->data_received, buffer); + } + + TEST_ASSERT(spi_bus_remove_device(spi) == ESP_OK); + TEST_ASSERT(spi_bus_free(TEST_SPI_HOST) == ESP_OK); +} + +TEST_CASE("SPI master variable cmd & addr test","[spi]") +{ + spi_slave_task_context_t slave_context = {}; + esp_err_t err = init_slave_context( &slave_context ); + TEST_ASSERT( err == ESP_OK ); + TaskHandle_t handle_slave; + xTaskCreate( task_slave, "spi_slave", 4096, &slave_context, 0, &handle_slave); //initial slave, mode 0, no dma int dma_chan = 0; @@ -837,111 +946,22 @@ TEST_CASE("SPI master variable cmd & addr test","[spi]") spi_bus_config_t slv_buscfg=SPI_BUS_TEST_DEFAULT_CONFIG(); spi_slave_interface_config_t slvcfg=SPI_SLAVE_TEST_DEFAULT_CONFIG(); slvcfg.mode = slave_mode; - //Enable pull-ups on SPI lines so we don't detect rogue pulses when no master is connected. - slave_pull_up(&buscfg, slvcfg.spics_io_num); //Initialize SPI slave interface - TEST_ESP_OK( spi_slave_initialize(VSPI_HOST, &slv_buscfg, &slvcfg, dma_chan) ); + TEST_ESP_OK( spi_slave_initialize(TEST_SLAVE_HOST, &slv_buscfg, &slvcfg, dma_chan) ); - - //connecting pins to two peripherals breaks the output, fix it. - gpio_output_sel(PIN_NUM_MOSI, FUNC_GPIO, HSPID_OUT_IDX); - gpio_output_sel(PIN_NUM_MISO, FUNC_GPIO, VSPIQ_OUT_IDX); - gpio_output_sel(PIN_NUM_CS, FUNC_GPIO, HSPICS0_OUT_IDX); - gpio_output_sel(PIN_NUM_CLK, FUNC_GPIO, HSPICLK_OUT_IDX); - - TaskHandle_t handle_slave; - xTaskCreate( task_slave, "spi_slave", 4096, &slave_context, 0, &handle_slave); - - slave_txdata_t slave_txdata[16]; - spi_transaction_ext_t trans[16]; - for( int i= 0; i < 16; i ++ ) { - //prepare slave tx data - slave_txdata[i] = (slave_txdata_t) { - .start = slave_send + 4*(i%3), - .len = 256, - }; - xQueueSend( slave_context.data_to_send, &slave_txdata[i], portMAX_DELAY ); - //prepare master tx data - trans[i] = (spi_transaction_ext_t) { - .base = { - .flags = SPI_TRANS_VARIABLE_CMD | SPI_TRANS_VARIABLE_ADDR, - .addr = 0x456789ab, - .cmd = 0xcdef, - - .length = 8*i, - .tx_buffer = tx_buf+i, - .rx_buffer = rx_buf_ptr, - }, - .command_bits = ((i+1)%3) * 8, - .address_bits = ((i/3)%5) * 8, - }; - if ( trans[i].base.length == 0 ) { - trans[i].base.tx_buffer = NULL; - trans[i].base.rx_buffer = NULL; - } else { - rx_buf_ptr += (trans[i].base.length + 31)/32*4; - } - } - - vTaskDelay(10); - - for ( int i = 0; i < 16; i ++ ) { - TEST_ESP_OK (spi_device_queue_trans( spi, (spi_transaction_t*)&trans[i], portMAX_DELAY ) ); - vTaskDelay(10); - } - - for( int i= 0; i < 16; i ++ ) { - //wait for both master and slave end - ESP_LOGI( MASTER_TAG, "===== test%d =====", i ); - spi_transaction_ext_t *t; - size_t rcv_len; - spi_device_get_trans_result( spi, (spi_transaction_t**)&t, portMAX_DELAY ); - TEST_ASSERT( t == &trans[i] ); - if ( trans[i].base.length != 0 ) { - ESP_LOG_BUFFER_HEX( "master tx", trans[i].base.tx_buffer, trans[i].base.length/8 ); - ESP_LOG_BUFFER_HEX( "master rx", trans[i].base.rx_buffer, trans[i].base.length/8 ); - } else { - ESP_LOGI( "master tx", "no data" ); - ESP_LOGI( "master rx", "no data" ); - } - - slave_rxdata_t *rcv_data = xRingbufferReceive( slave_context.data_received, &rcv_len, portMAX_DELAY ); - uint8_t *buffer = rcv_data->data; - rcv_len = rcv_data->len; - ESP_LOGI(SLAVE_TAG, "trans_len: %d", rcv_len); - ESP_LOG_BUFFER_HEX( "slave tx", slave_txdata[i].start, (rcv_len+7)/8); - ESP_LOG_BUFFER_HEX( "slave rx", buffer, (rcv_len+7)/8); - //check result - uint8_t *ptr_addr = (uint8_t*)&t->base.addr; - uint8_t *ptr_cmd = (uint8_t*)&t->base.cmd; - for ( int j = 0; j < t->command_bits/8; j ++ ) { - TEST_ASSERT_EQUAL( buffer[j], ptr_cmd[t->command_bits/8-j-1] ); - } - for ( int j = 0; j < t->address_bits/8; j ++ ) { - TEST_ASSERT_EQUAL( buffer[t->command_bits/8+j], ptr_addr[t->address_bits/8-j-1] ); - } - if ( t->base.length != 0) { - TEST_ASSERT_EQUAL_HEX8_ARRAY(t->base.tx_buffer, buffer + (t->command_bits + t->address_bits)/8, t->base.length/8); - TEST_ASSERT_EQUAL_HEX8_ARRAY(slave_txdata[i].start + (t->command_bits + t->address_bits)/8, t->base.rx_buffer, t->base.length/8); - } - TEST_ASSERT_EQUAL( t->base.length + t->command_bits + t->address_bits, rcv_len ); - - //clean - vRingbufferReturnItem( slave_context.data_received, buffer ); - } + test_cmd_addr(&slave_context, false); + test_cmd_addr(&slave_context, true); vTaskDelete( handle_slave ); handle_slave = 0; deinit_slave_context(&slave_context); - TEST_ASSERT(spi_slave_free(VSPI_HOST) == ESP_OK); - - TEST_ASSERT(spi_bus_remove_device(spi) == ESP_OK); - TEST_ASSERT(spi_bus_free(HSPI_HOST) == ESP_OK); + TEST_ASSERT(spi_slave_free(TEST_SLAVE_HOST) == ESP_OK); ESP_LOGI(MASTER_TAG, "test passed."); } + /******************************************************************************** * Test Timing By Internal Connections ********************************************************************************/