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

esp32_spiflash.c: Correctly disable APP's CPU cache. #4570

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

Ouss4
Copy link
Member

@Ouss4 Ouss4 commented Sep 17, 2021

Summary

Correctly disable APP's CPU cache during SPI flash operation.

Impact

ESP32 SPI Flash driver.

Testing

esp32-devkitc:spiflash with SMP.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>
@acassis acassis merged commit 15b68b9 into apache:master Sep 17, 2021
@Ouss4 Ouss4 deleted the spiflash_cache branch September 17, 2021 20:50
@masayuki2009
Copy link
Contributor

@Ouss4

I noticed that esp32-devkitc:wapi_smp does not work with this PR.

@Ouss4
Copy link
Member Author

Ouss4 commented Sep 23, 2021

@masayuki2009 sorry I didn't see your comment earlier. Yes, I'm working on this. The APP CPU needs to be properly paused when the cache is disabled.

@masayuki2009
Copy link
Contributor

@Ouss4

I've just looked into the code and noticed that it manipulates the both of CPU caches.
I think this is dangerous because the other CPU might be still running.
So that's why you mentioned The APP CPU needs to be properly paused when the cache is disabled.

As long as I see the code, for example, esp32_erase() takes g_exclsem and calls esp32_erasecector() which then calls esp32_spiflash_opstart() to take a critical section and disable CPU cache then issues SPI command for erase and finally calls esp32_spiflash_opdone() to enable CPU cache and leave the critical section. In this case, another CPU can not execute esp32_erase() in parallel. Also, while taking the critical section, the currently running task on the CPU would not be switched to another CPU because it took a critical section (i.e. local interrupts are also disabled) and it does not call blocking APIs. So I think cache operation for the currently running CPU is enough.

By the way, I'm still not sure why we need such cache operations before and after sending SPI commands.
In my understanding, it does not use DMA for the SPI flash. So I thought we don't need cache operations.

What do you think?

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

Successfully merging this pull request may close these issues.

3 participants