Slow reading at compiler version 5.x

[Meinsda]

We have the problem when development the brutzelboy that the speed of reading files or the startup with the compiler version 5.x is very slow. Very slow means a normal gameboy game needs about 2 minutes to load. With the compiler verson 4.x the speed is ok, but not very fast.

After simple debugging the code i found out that the read function is very slow. it needs about 2-3 secounds to read one block.

Then i search for any problems with reading from sd card at expressif esp32. I found this link. https://github.com/espressif/esp-idf/issues/10493 It describe that the fseek function is very slow. If you read the article expressif say they can't find any lost of speed. If the person change the pin from GPIO4 to GPIO13 it works again.

If this is a problem of the hardware, compiler or documentation i don't know, but there is a simple workaround.

Out hardware is ready and we don't want to change anythink on the hardware. So the CS signal you only need if you have more then one device on the spi bus. Most of the schematics has only one device on each spi bus.

I wrote a wirkaround that the cs line is alwas low and active. After compiling it with a 5.x compiler i got .. more speed then before with a 4.x compiled version. We say about 30 up to 50% faster loading.

My code changes: In retro-go\components\retro-go\rg_storage.c

72,76d71
< #if defined(RG_TARGET_BRUTZELBOY)
<     RG_LOGI("Set CS Signal to LOW patch by meins_da");
<     gpio_set_direction(RG_GPIO_SDSPI_CS_ORIG,  GPIO_MODE_OUTPUT);
<     gpio_set_level(RG_GPIO_SDSPI_CS_ORIG, 0);
< #endif	

and retro-go\components\retro-go\targets\brutzelboy\config.h

91,92c91
< #define RG_GPIO_SDSPI_CS            GPIO_NUM_NC
< #define RG_GPIO_SDSPI_CS_ORIG       GPIO_NUM_10
---
> #define RG_GPIO_SDSPI_CS            GPIO_NUM_10

It's possible that all devices have this problem.

@ducalex why is brutzelboy anymore in the repro?

[ducalex]

I've read the thread you've linked and your patch seems reasonable enough. You're probably right that it affects other devices so I'm thinking of something like that:

diff --git a/components/retro-go/rg_storage.c b/components/retro-go/rg_storage.c
index c36806c9..79436878 100644
--- a/components/retro-go/rg_storage.c
+++ b/components/retro-go/rg_storage.c
@@ -16,6 +16,7 @@
 #endif
 
 #ifdef ESP_PLATFORM
+#include <esp_idf_version.h>
 #include <esp_vfs_fat.h>
 #endif
 
@@ -81,6 +82,17 @@ void rg_storage_init(void)
     slot_config.host_id = RG_STORAGE_SDSPI_HOST;
     slot_config.gpio_cs = RG_GPIO_SDSPI_CS;
 
+    // If we're using esp-idf >= 5.0 and the SPI bus is not shared, we must keep the SD card selected
+    // to work around slow accesses. (https://github.com/espressif/esp-idf/issues/10493)
+    #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0)
+    if (RG_STORAGE_SDSPI_HOST != RG_SCREEN_HOST && RG_GPIO_SDSPI_CS != GPIO_NUM_NC)
+    {
+        gpio_set_direction(RG_GPIO_SDSPI_CS, GPIO_MODE_OUTPUT);
+        gpio_set_level(RG_GPIO_SDSPI_CS, 0);
+        slot_config.gpio_cs = GPIO_NUM_NC;
+    }
+    #endif
+
     esp_vfs_fat_mount_config_t mount_config = {
         .format_if_mount_failed = false,
         .max_files = 4,

No changes needed in config.h files. Does it seem reasonable to you?

@ducalex why is brutzelboy anymore in the repro?

That's a very good question. It wasn't intentionally removed, I must have done something with git. I will have to look into it...

[Meinsda]

You don't have to look which version of the compiler is used. It also works good on the older compiler version. Some person says the loading of the files are also much faster. Your idea is good, but it always disable the cs pin if there is a different bus with the display. You cannot enable it if you want. And it is also a point of confusion. You set a CS pin and it disable them? ;)

I think it would be better like i have done at my source. the code like

#if (RG_GPIO_SDPSI_CS==GPIO_NUM_NC)
#ifdef RG_GPIO_SDSPI_CS_ORIG
<code>
#endif
#endif

If you don't modify the config it runs like it is, but if you say GPIO_NUM_NC it don't use CS. With the optinal parameter CS_ORIG you can say which pin you want to set low to always enable the cs.

Only devs they want to use this function can enable and disable them over the config file.

[ducalex]

You're right the developer should be able to control the behavior, it shouldn't be based on esp-idf version alone. I've pushed a change and now if you #define RG_STORAGE_SDSPI_HOLD_CS in your config.h then CS will now be held low all the time, as desired.

That being said I've been trying to find a way that works for shared bus too, because retro-go has several devices that need it.

I've had some luck by:

1. Leaving RG_GPIO_SDSPI_CS as normal, do not define RG_STORAGE_SDSPI_HOLD_CS
2. Putting gpio_set_level(RG_GPIO_SDSPI_CS, 0); at the top of sdcard_do_transaction instead

It will assert CS before esp-idf does its idle dance prior to a transaction, which seems to fix the issue. esp-idf will deassert CS at the end of the transaction and the bus can then be still shared with the display.

Can you try that to see if it works for you too? Because I think it's a better solution.