WIP DNM esp32: add flash cache support #8783

GautierAtWork · 2018-07-06T11:01:08Z

Needs option CONFIG_BOOTLOADER_ESP_IDF

todo:

Add option in Kconfig
Add #ifdef around changes in pinmux_esp32.c

@lpereira: an option like CONFIG_ESP32_FLASH_CACHE will be introduced. Two linker scripts will exist then: one with data cache, one without data cache. Will it be ok for you?

codecov-io · 2018-07-06T11:24:28Z

Codecov Report

Merging #8783 into master will decrease coverage by 0.42%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8783      +/-   ##
==========================================
- Coverage   52.83%   52.41%   -0.43%     
==========================================
  Files         310      198     -112     
  Lines       45311    24912   -20399     
  Branches    10468     5185    -5283     
==========================================
- Hits        23942    13058   -10884     
+ Misses      16584     9754    -6830     
+ Partials     4785     2100    -2685

Impacted Files	Coverage Δ
include/bluetooth/buf.h	`0% <0%> (-100%)`	⬇️
include/drivers/bluetooth/hci_driver.h	`0% <0%> (-100%)`	⬇️
subsys/random/rand32_timer.c	`0% <0%> (-100%)`	⬇️
include/bluetooth/hci.h	`0% <0%> (-77.78%)`	⬇️
drivers/console/native_posix_console.c	`22.36% <0%> (-50.36%)`	⬇️
boards/posix/native_posix/cmdline.c	`16.21% <0%> (-45.49%)`	⬇️
subsys/logging/log_output.c	`2.72% <0%> (-45.06%)`	⬇️
subsys/usb/usb_descriptor.c	`0% <0%> (-44.59%)`	⬇️
subsys/bluetooth/host/hci_core.c	`2.47% <0%> (-41.43%)`	⬇️
include/misc/byteorder.h	`56.81% <0%> (-40.91%)`	⬇️
... and 343 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f90acbf...fb03d9b. Read the comment docs.

Add flash cache support for esp32. Signed-off-by: Gautier Seidel <[email protected]>

lpereira

Nice catch on the pinmux code!

I think it's fine if we have two linker scripts, at least short term. Long term, it should be better to only support building with flash cache enabled; otherwise, it's kind of hard to utilize the full potential of this SoC.

Patch LGTM, but I have a few questions.

lpereira · 2018-07-06T16:21:38Z

arch/xtensa/soc/esp32/linker.ld

+
+    *(.literal.*_int_handler .text.*_int_handler)
+    *(.literal.*_irq_spurious .text.*_irq_spurious)
+    *(.literal.*i2c_esp32_isr .text.*i2c_esp32_isr)


Might be a better idea to mark ISRs in ESP32 drivers with an __attribute__(__section__("IRAM")) or something of the sort; otherwise, for each driver for this platform, we'll need to add a line here. It would be great if we had a libdrivers.a to make this even easier.

Also, I'm curious here: say that someone writes an application using the GPIO ports, and they register a callback. Those are executed in interrupt context. If they end up calling a function that happens to not be in the flash cache, what is going to happen?

lpereira · 2018-07-06T16:22:16Z

arch/xtensa/soc/esp32/linker.ld

@@ -202,12 +209,23 @@ SECTIONS
    *(.dynamic)
    *(.gnu.version_d)
    . = ALIGN(4);               /* this table MUST be 4-byte aligned */
+
+    __devconfig_start = .;


These are very much Zephyr-specific; don't we have linker macros for this sort of thing?

lpereira · 2018-07-06T16:23:36Z

arch/xtensa/soc/esp32/linker.ld

-  iram0_2_seg(RX): org = 0x400D0018, len = 0x330000
-  dram0_0_seg(RW): org = 0x3FFB0000, len = 0x50000
+  iram0_2_seg(RX): org = 0x400D0018, len = 0x330000-18
+  dram0_0_seg(RW): org = 0x3FFB0000, len = 0x2c200


Might be a good idea to explain what's the meaning for -18 and why the dram0_0_seg went from 0x50000 bytes to 0x2c200. I can imagine there are reasons, but linker scripts are tricky and it's best to be as clear as possible with these values.

andyross · 2018-07-16T19:21:51Z

drivers/pinmux/pinmux_esp32.c

-		pinmux_set(NULL, pin, 0);
+		/* pin 6 to 11 are configured for SPI by the second stage bootloader.
+		 * We should not change them, otherwise we can't access the flash
+		 * anymore. */


If the point here is to disallow software from ever touching those pins through this driver, you probably want to put that rule into pinmux_set(), no?

Honestly, I'm not sure I see much value to this initialization loop anyway. If we have code enabled that cares about particular pin state, it will set it in its own initialization. If not, it's probably better to rely on whatever the bootloader set for us. Forcing "function zero" to be the default for everyone always is just going to cause collisions like this. Maybe just remove the loop?

I updated the comment. The idea is to keep the ability to reconfigure these pins during run-time.

Added in board Kconfig for esp32 Signed-off-by: Gautier Seidel <[email protected]>

esp32 pinmux driver doesn't re-initialize the SPI pins if flash cache is used. Signed-off-by: Gautier Seidel <[email protected]>

Now two linker scripts are available: one with flash cache suport, one without. Signed-off-by: Gautier Seidel <[email protected]>

aescolar · 2019-09-18T15:43:07Z

@GautierAtWork This PR seems to have been getting stale for very long.
It would seem you were waiting for some re-reviews(?) If so could you ping those?
Otherwise if you are not planning to continue working on this would you want to close it?

GautierAtWork requested review from andrewboie, andyross and rgundi as code owners July 6, 2018 11:01

esp32: add flash cache support

25934ee

Add flash cache support for esp32. Signed-off-by: Gautier Seidel <[email protected]>

GautierAtWork force-pushed the esp32_flash_cache branch from 1115001 to 25934ee Compare July 6, 2018 12:02

nashif added the DNM This PR should not be merged (Do Not Merge) label Jul 6, 2018

lpereira suggested changes Jul 6, 2018

View reviewed changes

galak added the platform: ESP32 Espressif ESP32 label Jul 6, 2018

andyross reviewed Jul 16, 2018

View reviewed changes

esp32: add Kconfig flash cache option

2a4a86f

Added in board Kconfig for esp32 Signed-off-by: Gautier Seidel <[email protected]>

GautierAtWork requested a review from ramakrishnapallala as a code owner July 17, 2018 20:50

gautierseidel added 2 commits July 18, 2018 22:57

esp32: pinmux spi pins whitelist

74fad89

esp32 pinmux driver doesn't re-initialize the SPI pins if flash cache is used. Signed-off-by: Gautier Seidel <[email protected]>

esp32: add linker script with flash cache support

fb03d9b

Now two linker scripts are available: one with flash cache suport, one without. Signed-off-by: Gautier Seidel <[email protected]>

GautierAtWork force-pushed the esp32_flash_cache branch from d447598 to fb03d9b Compare July 22, 2018 17:43

aescolar added the Stale PR label Sep 18, 2019

GautierAtWork closed this Nov 4, 2019

Skallwar mentioned this pull request Jun 30, 2020

WiFi support for ESP32 #3723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP DNM esp32: add flash cache support #8783

WIP DNM esp32: add flash cache support #8783

GautierAtWork commented Jul 6, 2018 •

edited

Loading

codecov-io commented Jul 6, 2018 •

edited

Loading

lpereira left a comment

lpereira Jul 6, 2018 •

edited

Loading

lpereira Jul 6, 2018

lpereira Jul 6, 2018 •

edited

Loading

andyross Jul 16, 2018

GautierAtWork Jul 18, 2018

aescolar commented Sep 18, 2019

WIP DNM esp32: add flash cache support #8783

WIP DNM esp32: add flash cache support #8783

Conversation

GautierAtWork commented Jul 6, 2018 • edited Loading

codecov-io commented Jul 6, 2018 • edited Loading

Codecov Report

lpereira left a comment

Choose a reason for hiding this comment

lpereira Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

lpereira Jul 6, 2018

Choose a reason for hiding this comment

lpereira Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

andyross Jul 16, 2018

Choose a reason for hiding this comment

GautierAtWork Jul 18, 2018

Choose a reason for hiding this comment

aescolar commented Sep 18, 2019

GautierAtWork commented Jul 6, 2018 •

edited

Loading

codecov-io commented Jul 6, 2018 •

edited

Loading

lpereira Jul 6, 2018 •

edited

Loading

lpereira Jul 6, 2018 •

edited

Loading