Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TW#24054] Allow selection of VSPI or HSPI for PSRAM 80Mhz mode #2128

Closed
3 tasks
OtherCrashOverride opened this issue Jun 29, 2018 · 9 comments
Closed
3 tasks

Comments

@OtherCrashOverride
Copy link

OtherCrashOverride commented Jun 29, 2018

Environment

  • Development Kit: [none]
  • Kit version (for WroverKit/PicoKit/DevKitC): []
  • Core (if using chip or module): [ESP32-Wrover]
  • IDF version (git rev-parse --short HEAD to get the commit id.):
    568da37
  • Development Env: [Make]
  • Operating System: [Ubuntu]
  • Power Supply: [USB|Battery]

Problem Description

PSRAM 80Mhz mode is hard coded to VSPI. This prevents the use it.

Expected Behavior

PSRAM 80Mhz mode should allow selection of VSPI or HSPI.

Actual Behavior

PSRAM 80Mhz mode always uses VSPI

Steps to repropduce

  1. Enable PSRAM 80Mhz mode
  2. Attempt to use VSPI.

Code to reproduce this issue

N/A

Debug Logs

E (78) spi_master: spi_bus_initialize(146): host already in use

Other items if possible

  • coredump (This provides stacks of tasks.)
  • sdkconfig file (attach the sdkconfig file from your project folder)
  • elf file in the build folder (note this may contain all the code details and symbols of your project.)
@FayeY FayeY changed the title Allow selection of VSPI or HSPI for PSRAM 80Mhz mode [TW#24054] Allow selection of VSPI or HSPI for PSRAM 80Mhz mode Jul 6, 2018
@jaygarcia
Copy link

Ran into this limitation. Thank you @OtherCrashOverride for documenting it.

Is this a hardware limitation or a software one?

@ginkgm
Copy link
Collaborator

ginkgm commented Sep 26, 2018

it seems to be a SW limit, we will support to use HSPI in the future.

@mschwartz
Copy link

Can we get this prioritized to get done ASAP please?

@jaygarcia
Copy link

jaygarcia commented Oct 24, 2018

@ginkgm I've modified my local instance of spi_master.c per the below diff. 80Mhz SPI RAM seems to work perfectly on the ESP32-WROVER. Should we be concerned if we're not using anything other than SPI For video (ILI9341), the DAC and input pins for controls?

/cc @OtherCrashOverride -- have you tried similar?

diff --git a/components/driver/spi_master.c b/components/driver/spi_master.c
index ffbedd2..65a5fe1 100644
--- a/components/driver/spi_master.c
+++ b/components/driver/spi_master.c
@@ -444,7 +444,7 @@ esp_err_t spi_bus_add_device(spi_host_device_t host, const spi_device_interface_
     eff_clk = spi_cal_clock(apbclk, dev_config->clock_speed_hz, duty_cycle, (uint32_t*)&clk_reg);
     int freq_limit = spi_get_freq_limit(!(spihost[host]->flags&SPICOMMON_BUSFLAG_NATIVE_PINS), dev_config->input_delay_ns);
     //GPIO matrix can only change data at 80Mhz rate, which only allows 40MHz SPI clock.
-    SPI_CHECK(eff_clk <= 40*1000*1000 || spihost[host]->flags&SPICOMMON_BUSFLAG_NATIVE_PINS, "80MHz only supported on iomux pins", ESP_ERR_INVALID_ARG);
+    // SPI_CHECK(eff_clk <= 40*1000*1000 || spihost[host]->flags&SPICOMMON_BUSFLAG_NATIVE_PINS, "80MHz only supported on iomux pins", ESP_ERR_INVALID_ARG);
     //Speed >=40MHz over GPIO matrix needs a dummy cycle, but these don't work for full-duplex connections.
     spi_get_timing(!(spihost[host]->flags&SPICOMMON_BUSFLAG_NATIVE_PINS), dev_config->input_delay_ns, eff_clk, &dummy_required, &miso_delay);
     SPI_CHECK( dev_config->flags & SPI_DEVICE_HALFDUPLEX || dummy_required == 0 ||

@negativekelvin
Copy link
Contributor

negativekelvin commented Oct 25, 2018

Note that the now default 64mbit psram does not use an extra spi peripheral so both hspi and vspi are free

/* note: If the third mode(80Mhz+80Mhz) is enabled for 32MBit 1V8 psram, VSPI port will be
occupied by the system.

@mschwartz
Copy link

mschwartz commented Oct 25, 2018

Thanks @ginkgm

What is the downside of using 80MHz for SPI RAM? What specifically do we lose?

I assume you have that SPI_CHECK() there for good reason? I mean, why is the SPI_CHECK() in the code?

@ginkgm
Copy link
Collaborator

ginkgm commented Oct 25, 2018

They are different things.

  1. About the restriction of VSPI

    According to what i know, the restriction is: The SPI RAM of a type (all of them are 4MB) requires continuous clock when work under 80MHz. And the SPI peripheral happen to be able to provide that.
    The feature supporting to use HSPI to provide the clock is on the way, waiting for internal review.

    Other case (use SPIRAM under 40MHz or lower, or the SPIRAM chip doesn't need the clock) the SPI are free.

  2. About the SPI_CHECK about 80MHz with GPIO matrix

    I think this check is indeed wrong there. The check was firstly designed to avoid using SPI to read data with too large delay on the MISO line. But it doesn't take into account that the dummy bits can be used to compensate this delay in the half duplex mode. I'll remove this check later.

    Detail about this:

    If you are using SPI peripheral through GPIO matrix, which allows you to connect the SPI pins to anywhere rather than IOMUX pins, a delay from the matrix is appended.

    (Assume you are using the master) For output signals(CS, CLK, MOSI), there's just gate delay; but for input signal (MISO), the peripheral will get the data after 2 apb clocks of delay (25ns). The spi master fetch the data one clock after the launch edge (rather than 0.5 clock as defined in the spec) to allow the slave output data slower. However 25ns of delay violates the timing constraints if the clock speed is higher or equal to 40MHz.

    To conclude, the spi master can output data at any speed (see NO_DUMMY flag). But it can only read data when the total delay (gpio matrix delay + slave delay) is not too large, or dummy bits should be used.

@OtherCrashOverride
Copy link
Author

@jaygarcia,
When an official patch for this is available, I will investigate integrating into the builds since both WROVER and WROVER-B need to be supported.

@jaygarcia
Copy link

Thanks @OtherCrashOverride =)

@igrr igrr closed this as completed in 20a666f Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants