espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.67k stars 7.29k forks source link

[TW#19301] SPI MISO on input only pins (GPIO34-35-36-39) not working #1736

Closed gdefoest closed 6 years ago

gdefoest commented 6 years ago

Hi all,

I am working on a design that uses an external SPI device (SX1278) with the MISO pin assigned to GPIO35. This is an input only pin so it means the MISO (Master Input Slave Output) should work if the ESP32 is the SPI master but it is not the case.

I am getting the following error in the boot sequence: E (449) gpio: io_num=35 can only be input.

This is my init sequence:

#define PIN_NUM_MISO 35
#define PIN_NUM_MOSI 32
#define PIN_NUM_CLK  33
#define PIN_NUM_CS   5
esp_err_t ret;
spi_device_handle_t spi;

spi_bus_config_t buscfg={
    .miso_io_num=PIN_NUM_MISO,
    .mosi_io_num=PIN_NUM_MOSI,
    .sclk_io_num=PIN_NUM_CLK,
    .quadwp_io_num=-1,
    .quadhd_io_num=-1
};

spi_device_interface_config_t devcfg={
    .clock_speed_hz=1*1000*1000,               //Clock out at 1 MHz
    .mode=0,                                //SPI mode 0
    .spics_io_num=PIN_NUM_CS,               //CS pin
    .queue_size=7,                          //We want to be able to queue 7 transactions at a time
};

I see this in the driver (spi_common.c, line 271)

if (bus_config->miso_io_num >= 0) {
    PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[bus_config->miso_io_num], PIN_FUNC_GPIO);
    gpio_set_direction(bus_config->miso_io_num, GPIO_MODE_INPUT_OUTPUT);
    gpio_matrix_out(bus_config->miso_io_num, io_signal[host].spiq_out, false, false);
    gpio_matrix_in(bus_config->miso_io_num, io_signal[host].spiq_in, false);
}

The issue comes from the fact we assign an input only pin in an INPUT_OUTPUT. So I think the piece of code above should be this instead:

if (bus_config->miso_io_num >= 0) {
   PIN_FUNC_SELECT(GPIO_PIN_MUX_REG[bus_config->miso_io_num], PIN_FUNC_GPIO);
   if ( GPIO_IS_VALID_OUTPUT_GPIO(bus_config->miso_io_num) ) {
      // All pins except the input only are handled here!
       gpio_set_direction(bus_config->miso_io_num, GPIO_MODE_INPUT_OUTPUT);
       gpio_matrix_out(bus_config->miso_io_num, io_signal[host].spiq_out, false, false);
       gpio_matrix_in(bus_config->miso_io_num, io_signal[host].spiq_in, false);
    } else {
       // The input only Pins are handled here!
       gpio_set_direction(bus_config->miso_io_num, GPIO_MODE_INPUT);
       gpio_matrix_in(bus_config->miso_io_num, io_signal[host].spiq_in, false);
    }
}

What are your thoughts?

negativekelvin commented 6 years ago

Yes it looks correct, but miso needs to be output in dual/quad mode or slave mode so it should return an error in those cases.

mreutman commented 6 years ago

It looks like in issue https://github.com/espressif/esp-idf/issues/598 that half duplex is not even functioning, and isn't likely to be fixed any time soon. Given that having half duplex supported is precondition to being able to do quad/dual IO on SPI, it seems rather unfortunate to further reduce the capabilities of the SPI interface. Would be preferable in my humble opinion to error out on spi_bus_add_device() where the spi_device_interface_config_t is specified (and the configuring for normal, dual, or quad IO is provided) rather than on spi_bus_initialize().

ginkgm commented 6 years ago

@mreutman this idea is good, we'll add this.

ginkgm commented 6 years ago

@gdefoest we fixed this, please check whether it works.

gdefoest commented 6 years ago

I tested it again on be81d2c16d7f4caeea9ceb29fece01510664caf3. No issue anymore. Thanks a lot for the fix!