LCD/SPI driver does not allow varying clock speed while using chip-select, does not chip-select while re-using CS pin. (IDFGH-12774)

bryghtlabs-richard commented 1 month ago

Answers checklist.

[X] I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
[X] I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
[X] I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v5.0.2 and beyond

Espressif SoC revision.

ESP32-S3

Operating System used.

Windows

How did you build your project?

Command line with idf.py

If you are using Windows, please specify command line type.

CMD

Development Kit.

ESP32-S3-DEVKITC-1-N8R8

Power Supply used.

USB

What is the expected behavior?

I'm using an ST7789 SPI display. It supports writing pixels and registers over SPI at around 60MHz, but reading pixels and registers must be done below about 7MHz. The ESP-IDF spi_master and esp_lcd components each take only a single bus-speed. Sometimes I need to read registers, but I need my pixel transfers to occur at 40MHz to keep our animations smooth.

I tried ESP_Sprite's suggestion here: https://esp32.com/viewtopic.php?t=16704#p63258 , and I was able to create two esp_lcd_panel_io_spi_config_t with the same pins and different speeds and call esp_lcd_new_panel_io_spi() to get two esp_lcd_panel_io_handle_t but only one seems to drive the chip-select line.

Because there is no way to specify two different clock speeds, and because of ESP_Sprite's comment, I was assuming that two SPI devices on the same bus with the same chip-select would both be allowed to use the chip-select.

What is the actual behavior?

When two esp_lcd_panel_io_handle_t are created by esp_lcd_new_panel_io_spi using the same CS-pin, only one triggers the chip select.

Steps to reproduce.

Here is what I'm using for configs, and they both create a new panel-io without errors:

    ESP_LOGI(TAG, "Initialize SPI bus");
    spi_bus_config_t buscfg = {
        .sclk_io_num = BL_GPIO_DISP_SCLK,
        .mosi_io_num = BL_GPIO_DISP_MOSI,
        .miso_io_num = BL_GPIO_DISP_MISO,
        .flags = SPICOMMON_BUSFLAG_IOMUX_PINS, //I have tried with/without this, but no impact: 
        .quadwp_io_num = -1,
        .quadhd_io_num = -1,
        .max_transfer_sz = max_transfer_sz,
    };
    ESP_ERROR_CHECK(spi_bus_initialize(LCD_HOST, &buscfg, SPI_DMA_CH_AUTO));

    esp_lcd_panel_io_handle_t io_read = NULL;
    esp_lcd_panel_io_spi_config_t io_read_config = {
        .dc_gpio_num = BL_GPIO_DISP_DC,
        .cs_gpio_num = BL_GPIO_DISP_CS,
        .pclk_hz = LCD_READ_CLOCK_HZ,
        .lcd_cmd_bits = 8,
        .lcd_param_bits = 8,
        .spi_mode = 0,
        .trans_queue_depth = 2,
        .on_color_trans_done = on_color_trans_done,
        .user_ctx = user_ctx,
    };

    esp_lcd_panel_io_handle_t io_write = NULL;
    esp_lcd_panel_io_spi_config_t io_write_config = {
        .dc_gpio_num = BL_GPIO_DISP_DC,
        .cs_gpio_num = BL_GPIO_DISP_CS,
        .pclk_hz = LCD_PIXEL_CLOCK_HZ,
        .lcd_cmd_bits = 8,
        .lcd_param_bits = 8,
        .spi_mode = 0,
        .trans_queue_depth = 5,
        .on_color_trans_done = on_color_trans_done,
        .user_ctx = user_ctx,
    };
    // Attach the LCD to the SPI bus - TX
    ESP_ERROR_CHECK(esp_lcd_new_panel_io_spi((esp_lcd_spi_bus_handle_t)LCD_HOST, &io_write_config, &io_write));

    // Attach the LCD to the SPI bus - RX
    ESP_ERROR_CHECK(esp_lcd_new_panel_io_spi((esp_lcd_spi_bus_handle_t)LCD_HOST, &io_read_config, &io_read));

Debug Logs.

This issue is only visible on the oscilloscope, but I added some logging to spi_master/spi_common. It seems the second chip-select configuration is different from the first:

I (382) lcd_panel.st7789bl: Initialize SPI bus
I (382) spi_master: bus_attr->flags:0x007a
I (392) spi_master: use_gpio:0
E (392) spi: spicommon_cs_initialize:712 host:1 cs_io_num:10 cs_num:0 force_gpio_matrix:0 spi_periph_signal[host].spics0_iomux_pin:10
E (402) spi: spicommon_cs_initialize:718 spi_periph_signal[host].spics_in:110 spi_periph_signal[host].func:4
I (412) gpio: GPIO[46]| InputEn: 0| OutputEn: 1| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0
I (422) spi_master: bus_attr->flags:0x007a
I (432) spi_master: use_gpio:0
E (432) spi: spicommon_cs_initialize:712 host:1 cs_io_num:10 cs_num:1 force_gpio_matrix:0 spi_periph_signal[host].spics0_iomux_pin:10
E (442) spi: spicommon_cs_initialize:725 spi_periph_signal[host].spics_out[cs_num]:110
I (452) gpio: GPIO[46]| InputEn: 0| OutputEn: 1| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0



### More Information.

I can destroy and reallocate per transfer, but it's too slow.

suda-morris commented 1 month ago

have you called esp_lcd_panel_io_rx_param function with the io_read handle in your apllication code or in your st7789bl driver?

bryghtlabs-richard commented 1 month ago

Yes, I was calling esp_lcd_panel_io_rx_param to read the display registers. If I use only one io handle at 6MHz, I can use esp_lcd_panel_io_tx_param, esp_lcd_panel_io_rx_param, and esp_lcd_panel_io_tx_color. But when I create a second io handle, io_read, and use esp_lcd_panel_io_rx_param the SPI signals seem correct, but the chip-select does not toggle.

suda-morris commented 1 month ago

@bryghtlabs-richard I can reproduce the problem now. Thanks for reproting. The issue lies on, when SPI bus switches devices, it didn't update the connection between the SPI signal and the GPIO.

bryghtlabs-richard commented 1 month ago

I'm glad you see it too @suda-morris.

Instead of having two SPI bus devices sharing one chip-select, would it make sense to add a function to adjust the SPI clock for a specific device? This would also avoid the additional memory usage from the second call to spi_bus_add_device().

wanckl commented 1 month ago

@bryghtlabs-richard Hi ~

Yeah your idea is correct, but I think add API to adjust device's freq have other consideration:

SPI driver can do a sync/async communication, when it work on async, it is hard for you to know freq switching point.
To ensure API works during OTA/Flash operation, it still cost some internal memory
IDF is based on FreertOS, an API is hard to ensure thread safe if you using API in different task, but different device is OK

BTW, device is abstract of a set of communication attribute for SPI bus, change device's attribute against this rule. and change frequency maybe not a common case, at least for now only some LCD and SD card. we can simply resolve issue in another way.

I can try to fix two devices issue and give you feedback, please waiting my good news :handshake:

bryghtlabs-richard commented 3 weeks ago

I'm not sure if this would impact your design, but while working with ILI9481, I noticed it can benefit from adjusting SPI rate during transfer. Maybe ST7789 can too. But I think using spi-device per frequency, it would require starting transfer on one device, then switching device to finish transfer. Would that work? If the freq-switching is fast, it could save up to 40% of read-command bus-time, like this:

Lower chip-select
Send command-byte @ 40MHz
Read data-byte @ 6.66MHz
Raise chip-select

SPI driver can do a sync/async communication, when it work on async, it is hard for you to know freq switching point.

Yes, freq-switching point must be kept in the proper sequence for both sync/async transfers. One option might be to add it to spi_transaction_t, so freq-switching could be sequenced correctly for both.

I can try to fix two devices issue and give you feedback, please waiting my good news 🤝

Thank you for looking into this for us. 🤝

wanckl commented 3 weeks ago

@bryghtlabs-richard

Lower chip-select

Send command-byte @ 40MHz

Read data-byte @ 6.66MHz

Raise chip-select

Yeah, by control CS pin manually, things goes simply, you can freely using different devices on different frequency.

the difficult point is to config different devices on same CS pin, then can control CS by hardware

espressif / esp-idf