spi: fix the bug of connecting SPI peripheral to read-only pins

The requirements of pin capabilites is different for spi master and
slave.  The master needs CS, SCLK, MOSI to be output-able, while slave
needs MISO to be output-able.

Previous code is for master only.

This commit allows to place other 3 pins than MISO on input-only pins
for slaves. Refactoring for spi_common is also included.

Resolves https://github.com/espressif/esp-idf/issues/2455

diff --git a/components/driver/spi_common.c b/components/driver/spi_common.c
index 1b4fe62fd14718bca298b4ed87f9b37d31ee5c1c..8e43f57ea3e07477f8b29e596a57d4bb4c0ac2c5 100644 (file)
--- a/components/driver/spi_common.c
+++ b/components/driver/spi_common.c
@@ -34,11 +34,18 @@
 
 static const char *SPI_TAG = "spi";
 
-#define SPI_CHECK(a, str, ret_val) \
+#define SPI_CHECK(a, str, ret_val) do { \
     if (!(a)) { \
         ESP_LOGE(SPI_TAG,"%s(%d): %s", __FUNCTION__, __LINE__, str); \
         return (ret_val); \
-    }
+    } \
+    } while(0)
+
+#define SPI_CHECK_PIN(pin_num, pin_name, check_output) if (check_output) { \
+            SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(pin_num), pin_name" not valid", ESP_ERR_INVALID_ARG); \
+        } else { \
+            SPI_CHECK(GPIO_IS_VALID_GPIO(pin_num), pin_name" not valid", ESP_ERR_INVALID_ARG); \
+        }
 
 
 typedef struct spi_device_t spi_device_t;
@@ -115,6 +122,22 @@ bool spicommon_dma_chan_free(int dma_chan)
     return true;
 }
 
+static bool bus_uses_iomux_pins(spi_host_device_t host, const spi_bus_config_t* bus_config)
+{
+    if (bus_config->sclk_io_num>=0 &&
+        bus_config->sclk_io_num != spi_periph_signal[host].spiclk_iomux_pin) return false;
+    if (bus_config->quadwp_io_num>=0 &&
+        bus_config->quadwp_io_num != spi_periph_signal[host].spiwp_iomux_pin) return false;
+    if (bus_config->quadhd_io_num>=0 &&
+        bus_config->quadhd_io_num != spi_periph_signal[host].spihd_iomux_pin) return false;
+    if (bus_config->mosi_io_num >= 0 &&
+        bus_config->mosi_io_num != spi_periph_signal[host].spid_iomux_pin) return false;
+    if (bus_config->miso_io_num>=0 &&
+        bus_config->miso_io_num != spi_periph_signal[host].spiq_iomux_pin) return false;
+
+    return true;
+}
+
 /*
 Do the common stuff to hook up a SPI host to a bus defined by a bunch of GPIO pins. Feed it a host number and a
 bus config struct and it'll set up the GPIO matrix and enable the device. If a pin is set to non-negative value,
@@ -122,67 +145,70 @@ it should be able to be initialized.
 */
 esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_config_t *bus_config, int dma_chan, uint32_t flags, uint32_t* flags_o)
 {
-    bool use_iomux = true;
     uint32_t temp_flag=0;
-    bool quad_pins_exist = true;
-    //the MISO should be output capable in slave mode, or in DIO/QIO mode.
-    bool miso_output = !(flags&SPICOMMON_BUSFLAG_MASTER) || flags&SPICOMMON_BUSFLAG_DUAL;
-    //the MOSI should be output capble in master mode, or in DIO/QIO mode.
-    bool mosi_output = (flags&SPICOMMON_BUSFLAG_MASTER)!=0 || flags&SPICOMMON_BUSFLAG_DUAL;
 
-    //check pins existence and if the selected pins correspond to the iomux pins of the peripheral
+    bool miso_need_output;
+    bool mosi_need_output;
+    bool sclk_need_output;
+    if ((flags&SPICOMMON_BUSFLAG_MASTER) != 0) {
+        //initial for master
+        miso_need_output = ((flags&SPICOMMON_BUSFLAG_DUAL) != 0) ? true : false;
+        mosi_need_output = true;
+        sclk_need_output = true;
+    } else {
+        //initial for slave
+        miso_need_output = true;
+        mosi_need_output = ((flags&SPICOMMON_BUSFLAG_DUAL) != 0) ? true : false;
+        sclk_need_output = false;
+    }
+
+    const bool wp_need_output = true;
+    const bool hd_need_output = true;
+
+    //check pin capabilities
     if (bus_config->sclk_io_num>=0) {
         temp_flag |= SPICOMMON_BUSFLAG_SCLK;
-        SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(bus_config->sclk_io_num), "sclk not valid", ESP_ERR_INVALID_ARG);
-        if (bus_config->sclk_io_num != spi_periph_signal[host].spiclk_iomux_pin) use_iomux = false;
-    } else {
-        SPI_CHECK((flags&SPICOMMON_BUSFLAG_SCLK)==0, "sclk pin required.", ESP_ERR_INVALID_ARG);
+        SPI_CHECK_PIN(bus_config->sclk_io_num, "sclk", sclk_need_output);
     }
     if (bus_config->quadwp_io_num>=0) {
-        SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(bus_config->quadwp_io_num), "spiwp not valid", ESP_ERR_INVALID_ARG);
-        if (bus_config->quadwp_io_num != spi_periph_signal[host].spiwp_iomux_pin) use_iomux = false;
-    } else {
-        quad_pins_exist = false;
-        SPI_CHECK((flags&SPICOMMON_BUSFLAG_WPHD)==0, "spiwp pin required.", ESP_ERR_INVALID_ARG);
+        SPI_CHECK_PIN(bus_config->quadwp_io_num, "wp", wp_need_output);
     }
     if (bus_config->quadhd_io_num>=0) {
-        SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(bus_config->quadhd_io_num), "spihd not valid", ESP_ERR_INVALID_ARG);
-        if (bus_config->quadhd_io_num != spi_periph_signal[host].spihd_iomux_pin) use_iomux = false;
-    } else {
-        quad_pins_exist = false;
-        SPI_CHECK((flags&SPICOMMON_BUSFLAG_WPHD)==0, "spihd pin required.", ESP_ERR_INVALID_ARG);
+        SPI_CHECK_PIN(bus_config->quadhd_io_num, "hd", hd_need_output);
     }
+    //set flags for QUAD mode according to the existence of wp and hd
+    if (bus_config->quadhd_io_num >= 0 && bus_config->quadwp_io_num >= 0) temp_flag |= SPICOMMON_BUSFLAG_WPHD;
     if (bus_config->mosi_io_num >= 0) {
         temp_flag |= SPICOMMON_BUSFLAG_MOSI;
-        if (mosi_output) {
-            SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(bus_config->mosi_io_num), "mosi not valid", ESP_ERR_INVALID_ARG);
-        } else {
-            SPI_CHECK(GPIO_IS_VALID_GPIO(bus_config->mosi_io_num), "mosi not valid", ESP_ERR_INVALID_ARG);
-        }
-        if (bus_config->mosi_io_num != spi_periph_signal[host].spid_iomux_pin) use_iomux = false;
-    } else {
-        SPI_CHECK((flags&SPICOMMON_BUSFLAG_MOSI)==0, "mosi pin required.", ESP_ERR_INVALID_ARG);
+        SPI_CHECK_PIN(bus_config->mosi_io_num, "mosi", mosi_need_output);
     }
     if (bus_config->miso_io_num>=0) {
         temp_flag |= SPICOMMON_BUSFLAG_MISO;
-        if (miso_output) {
-            SPI_CHECK(GPIO_IS_VALID_OUTPUT_GPIO(bus_config->miso_io_num), "miso not valid", ESP_ERR_INVALID_ARG);
-        } else {
-            SPI_CHECK(GPIO_IS_VALID_GPIO(bus_config->miso_io_num), "miso not valid", ESP_ERR_INVALID_ARG);
-        }
-        if (bus_config->miso_io_num != spi_periph_signal[host].spiq_iomux_pin) use_iomux = false;
-    } else {
-        SPI_CHECK((flags&SPICOMMON_BUSFLAG_MISO)==0, "miso pin required.", ESP_ERR_INVALID_ARG);
+        SPI_CHECK_PIN(bus_config->miso_io_num, "miso", miso_need_output);
     }
     //set flags for DUAL mode according to output-capability of MOSI and MISO pins.
     if ( (bus_config->mosi_io_num < 0 || GPIO_IS_VALID_OUTPUT_GPIO(bus_config->mosi_io_num)) &&
         (bus_config->miso_io_num < 0 || GPIO_IS_VALID_OUTPUT_GPIO(bus_config->miso_io_num)) ) {
         temp_flag |= SPICOMMON_BUSFLAG_DUAL;
     }
-    //set flags for QUAD mode according to the existence of wp and hd
-    if (quad_pins_exist) temp_flag |= SPICOMMON_BUSFLAG_WPHD;
-    //check iomux pins if required.
-    SPI_CHECK((flags&SPICOMMON_BUSFLAG_NATIVE_PINS)==0 || use_iomux, "not using iomux pins", ESP_ERR_INVALID_ARG);
+
+    //check if the selected pins correspond to the iomux pins of the peripheral
+    bool use_iomux = bus_uses_iomux_pins(host, bus_config);
+    if (use_iomux) temp_flag |= SPICOMMON_BUSFLAG_NATIVE_PINS;
+
+    uint32_t missing_flag = flags & ~temp_flag;
+    missing_flag &= ~SPICOMMON_BUSFLAG_MASTER;//don't check this flag
+
+    if (missing_flag != 0) {
+    //check pins existence
+        if (missing_flag & SPICOMMON_BUSFLAG_SCLK) ESP_LOGE(SPI_TAG, "sclk pin required.");
+        if (missing_flag & SPICOMMON_BUSFLAG_MOSI) ESP_LOGE(SPI_TAG, "mosi pin required.");
+        if (missing_flag & SPICOMMON_BUSFLAG_MISO) ESP_LOGE(SPI_TAG, "miso pin required.");
+        if (missing_flag & SPICOMMON_BUSFLAG_DUAL) ESP_LOGE(SPI_TAG, "not both mosi and miso output capable");
+        if (missing_flag & SPICOMMON_BUSFLAG_WPHD) ESP_LOGE(SPI_TAG, "both wp and hd required.");
+        if (missing_flag & SPICOMMON_BUSFLAG_NATIVE_PINS) ESP_LOGE(SPI_TAG, "not using iomux pins");
+        SPI_CHECK(missing_flag == 0, "not all required capabilities satisfied.", ESP_ERR_INVALID_ARG);
+    }
 
     if (use_iomux) {
         //All SPI iomux pin selections resolve to 1, so we put that here instead of trying to figure
@@ -213,7 +239,7 @@ esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_conf
         //Use GPIO matrix
         ESP_LOGD(SPI_TAG, "SPI%d use gpio matrix.", host );
         if (bus_config->mosi_io_num >= 0) {
-            if (mosi_output || (temp_flag&SPICOMMON_BUSFLAG_DUAL)) {
+            if (mosi_need_output || (temp_flag&SPICOMMON_BUSFLAG_DUAL)) {
                 gpio_set_direction(bus_config->mosi_io_num, GPIO_MODE_INPUT_OUTPUT);
                 gpio_matrix_out(bus_config->mosi_io_num, spi_periph_signal[host].spid_out, false, false);
             } else {
@@ -223,7 +249,7 @@ esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_conf
             PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[bus_config->mosi_io_num], FUNC_GPIO);
         }
         if (bus_config->miso_io_num >= 0) {
-            if (miso_output || (temp_flag&SPICOMMON_BUSFLAG_DUAL)) {
+            if (miso_need_output || (temp_flag&SPICOMMON_BUSFLAG_DUAL)) {
                 gpio_set_direction(bus_config->miso_io_num, GPIO_MODE_INPUT_OUTPUT);
                 gpio_matrix_out(bus_config->miso_io_num, spi_periph_signal[host].spiq_out, false, false);
             } else {
@@ -245,8 +271,12 @@ esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_conf
             PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[bus_config->quadhd_io_num], FUNC_GPIO);
         }
         if (bus_config->sclk_io_num >= 0) {
-            gpio_set_direction(bus_config->sclk_io_num, GPIO_MODE_INPUT_OUTPUT);
-            gpio_matrix_out(bus_config->sclk_io_num, spi_periph_signal[host].spiclk_out, false, false);
+            if (sclk_need_output) {
+                gpio_set_direction(bus_config->sclk_io_num, GPIO_MODE_INPUT_OUTPUT);
+                gpio_matrix_out(bus_config->sclk_io_num, spi_periph_signal[host].spiclk_out, false, false);
+            } else {
+                gpio_set_direction(bus_config->sclk_io_num, GPIO_MODE_INPUT);
+            }
             gpio_matrix_in(bus_config->sclk_io_num, spi_periph_signal[host].spiclk_in, false);
             PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[bus_config->sclk_io_num], FUNC_GPIO);
         }
@@ -309,8 +339,12 @@ 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 (GPIO_IS_VALID_OUTPUT_GPIO(cs_io_num)) {
+            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);
+        } else {
+            gpio_set_direction(cs_io_num, GPIO_MODE_INPUT);
+        }
         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/test/test_spi_master.c b/components/driver/test/test_spi_master.c
index f2231353bbf1ea3a0ffbcc2b4fbfdf62ae119c39..4277be90736da8b78efe8f5915b540098a1473c0 100644 (file)
--- a/components/driver/test/test_spi_master.c
+++ b/components/driver/test/test_spi_master.c
@@ -25,6 +25,14 @@
 
 const static char TAG[] = "test_spi";
 
+#define PIN_NUM_MISO    HSPI_IOMUX_PIN_NUM_MISO
+#define PIN_NUM_MOSI    HSPI_IOMUX_PIN_NUM_MOSI
+#define PIN_NUM_CLK     HSPI_IOMUX_PIN_NUM_CLK
+#define PIN_NUM_CS      HSPI_IOMUX_PIN_NUM_CS
+
+#define TEST_SPI_HOST   HSPI_HOST
+#define TEST_SLAVE_HOST VSPI_HOST
+
 #define SPI_BUS_TEST_DEFAULT_CONFIG() {\
         .miso_io_num=PIN_NUM_MISO, \
         .mosi_io_num=PIN_NUM_MOSI,\
@@ -44,6 +52,14 @@ const static char TAG[] = "test_spi";
     .input_delay_ns = 62.5,\
 }
 
+//default device config for slave devices
+#define SPI_SLAVE_TEST_DEFAULT_CONFIG() {\
+        .mode=0,\
+        .spics_io_num=PIN_NUM_CS,\
+        .queue_size=3,\
+        .flags=0,\
+    }
+
 #define FUNC_SPI    1
 #define FUNC_GPIO   2
 
@@ -291,6 +307,65 @@ TEST_CASE("SPI Master test, interaction of multiple devs", "[spi][ignore]") {
     destroy_spi_bus(handle1);
 }
 
+static esp_err_t test_master_pins(int mosi, int miso, int sclk, int cs)
+{
+    esp_err_t ret;
+    spi_bus_config_t cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    cfg.mosi_io_num = mosi;
+    cfg.miso_io_num = miso;
+    cfg.sclk_io_num = sclk;
+
+    spi_device_interface_config_t master_cfg = SPI_DEVICE_TEST_DEFAULT_CONFIG();
+    master_cfg.spics_io_num = cs;
+
+    ret = spi_bus_initialize(TEST_SPI_HOST, &cfg, 1);
+    if (ret != ESP_OK) return ret;
+
+    spi_device_handle_t spi;
+    ret = spi_bus_add_device(TEST_SPI_HOST, &master_cfg, &spi);
+    if (ret != ESP_OK) {
+        spi_bus_free(TEST_SPI_HOST);
+        return ret;
+    }
+
+    TEST_ASSERT(spi_bus_remove_device(spi) == ESP_OK);
+    TEST_ASSERT(spi_bus_free(TEST_SPI_HOST) == ESP_OK);
+    return ESP_OK;
+}
+
+static esp_err_t test_slave_pins(int mosi, int miso, int sclk, int cs)
+{
+    esp_err_t ret;
+    spi_bus_config_t cfg = SPI_BUS_TEST_DEFAULT_CONFIG();
+    cfg.mosi_io_num = mosi;
+    cfg.miso_io_num = miso;
+    cfg.sclk_io_num = sclk;
+
+    spi_slave_interface_config_t slave_cfg = SPI_SLAVE_TEST_DEFAULT_CONFIG();
+    slave_cfg.spics_io_num = cs;
+
+    ret = spi_slave_initialize(TEST_SLAVE_HOST, &cfg, &slave_cfg, 1);
+    if (ret != ESP_OK) return ret;
+
+    spi_slave_free(TEST_SLAVE_HOST);
+    return ESP_OK;
+}
+
+TEST_CASE("spi placed on input-only pins", "[spi]")
+{
+    TEST_ESP_OK(test_master_pins(PIN_NUM_MOSI, PIN_NUM_MISO, PIN_NUM_CLK, PIN_NUM_CS));
+    TEST_ASSERT(test_master_pins(34, PIN_NUM_MISO, PIN_NUM_CLK, PIN_NUM_CS)!=ESP_OK);
+    TEST_ESP_OK(test_master_pins(PIN_NUM_MOSI, 34, PIN_NUM_CLK, PIN_NUM_CS));
+    TEST_ASSERT(test_master_pins(PIN_NUM_MOSI, PIN_NUM_MISO, 34, PIN_NUM_CS)!=ESP_OK);
+    TEST_ASSERT(test_master_pins(PIN_NUM_MOSI, PIN_NUM_MISO, PIN_NUM_CLK, 34)!=ESP_OK);
+
+    TEST_ESP_OK(test_slave_pins(PIN_NUM_MOSI, PIN_NUM_MISO, PIN_NUM_CLK, PIN_NUM_CS));
+    TEST_ESP_OK(test_slave_pins(34, PIN_NUM_MISO, PIN_NUM_CLK, PIN_NUM_CS));
+    TEST_ASSERT(test_slave_pins(PIN_NUM_MOSI, 34, PIN_NUM_CLK, PIN_NUM_CS)!=ESP_OK);
+    TEST_ESP_OK(test_slave_pins(PIN_NUM_MOSI, PIN_NUM_MISO, 34, PIN_NUM_CS));
+    TEST_ESP_OK(test_slave_pins(PIN_NUM_MOSI, PIN_NUM_MISO, PIN_NUM_CLK, 34));
+}
+
 TEST_CASE("spi bus setting with different pin configs", "[spi]")
 {
     spi_bus_config_t cfg;
@@ -678,13 +753,6 @@ static void master_deinit(spi_device_handle_t spi)
     TEST_ESP_OK( spi_bus_free(HSPI_HOST) );
 }
 
-#define SPI_SLAVE_TEST_DEFAULT_CONFIG() {\
-        .mode=0,\
-        .spics_io_num=PIN_NUM_CS,\
-        .queue_size=3,\
-        .flags=0,\
-}
-
 static void slave_pull_up(const spi_bus_config_t* cfg, int spics_io_num)
 {
     gpio_set_pull_mode(cfg->mosi_io_num, GPIO_PULLUP_ENABLE);