xtensa/esp32: When PSRAM is enabled allow drivers and tasks to allocate memory from a separate internal memory #1958

Ouss4 · 2020-10-09T09:13:49Z

Summary

When external RAM and external FLASH are used together we have to make sure that:

For drivers, in case of a DMA transfer, the buffer is DMA capable, i.e. doesn't originate from external RAM. If it does allocate memory from an internal heap then copy it back.
When accessing flash, the cache needs to be disabled, as a consequence any memory access or code execution need to be from internal memory. A "special" critical section around flash operations is created to make sure that:
- Buffers don't originate from external RAM
- Tasks' stack aren't allocated from external RAM
- const variables aren't in .ro.data.
- Code during this sections is in IRAM.

Impact

Impacts ESP32 when both PSRAM and SPIFLASH are enabled.

Testing

esp32-core:spiflash with PSRAM enabled and disabled.

allocated in an internal heap. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

… from DRAM when the given buffer is from PSRAM. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

memory region is available. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

warning that's not relevant anymore. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

donghengqaz · 2020-10-12T11:28:38Z

arch/xtensa/src/common/xtensa_createstack.c

@@ -73,6 +73,18 @@
 #define STACK_ALIGN_DOWN(a) ((a) & ~STACK_ALIGN_MASK)
 #define STACK_ALIGN_UP(a)   (((a) + STACK_ALIGN_MASK) & ~STACK_ALIGN_MASK)

+#ifdef CONFIG_XTENSA_USE_SEPERATE_IMEM


I see these macros are used in some places, and they are defined in multi *.c files, so maybe we can move these into a new file like xtensa_mm.h ,and let these *.c include xtensa_mm.h.

These macros come from config.h (which is generated based on the .config file created by Kconfig tools)
*.c files include the config.h and use the macros.

Ah I see what macros you are talking about, Github confused me.

@donghengqaz done. PTAL.

donghengqaz · 2020-10-12T11:36:37Z

arch/xtensa/src/esp32/esp32_spi.c

+  if (esp32_ptr_extram(txbuffer))
+    {
+      tp = up_imm_malloc(bytes);
+      if (tp == NULL)


Because the esp32_spi_dma_exchange can't return error code tell callers that the function failed, so maybe we add an assert function here ?

something like the following?

if (esp32_ptr_extram(txbuffer)) { tp = up_imm_malloc(bytes); - if (tp == NULL) - { - return; - } + DEBUGASSERT(tp != NULL); }

donghengqaz · 2020-10-12T11:37:15Z

arch/xtensa/src/esp32/esp32_spi.c

+      rp = up_imm_malloc(bytes);
+      if (rp == NULL)
+        {
+          goto error_with_tp;


Same as above.

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

code, assert the return of the imm_malloc function. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

to be 4 bytes. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

yamt · 2020-10-13T07:30:49Z

arch/xtensa/Kconfig

+	range 0x2000 0x28000
+	default 0x28000
+
+config XTENSA_IMEM_PROCFS


i feel it's more convenient for users to have this info in /proc/meminfo than the dedicated file. how do you think?

I was concerned that this change would be too "invasive" in procfsmeminfo. This feature is "too" Xtensa/ESP32 specific.

yamt · 2020-10-13T07:38:55Z

arch/xtensa/Kconfig

@@ -81,6 +81,21 @@ config XTENSA_CP_INITSET
 		is provided by CONFIG_XTENSA_CP_INITSET.  Each bit corresponds to one
 		coprocessor with the same bit layout as for the CPENABLE register.

+config XTENSA_USE_SEPERATE_IMEM
+	bool "Use a seperate heap for internal memory"


i guess the description should at least explain:

what's internal memory

what uses the memory if this config is enabled

I'll add a help section.

yamt · 2020-10-13T07:47:13Z

arch/xtensa/src/esp32/esp32_spi.c

+  if (esp32_ptr_extram(txbuffer))
+    {
+      tp = up_imm_malloc(bytes);
+      DEBUGASSERT(tp != NULL);


don't you need memcpy here?
(i don't understand what this function is doing. just a wild guess.)

yamt · 2020-10-13T07:48:39Z

arch/xtensa/src/esp32/esp32_spi.c

+  if (esp32_ptr_extram(rxbuffer))
+    {
+      memcpy(rxbuffer, rp, bytes);
+      up_imm_free(rp);


tp and rp are modified above. (tp += n)
i guess you should free the original pointer.

Indeed. Thanks, done.

yamt · 2020-10-13T07:48:47Z

arch/xtensa/src/esp32/esp32_spi.c

+#ifdef CONFIG_XTENSA_USE_SEPERATE_IMEM
+  if (esp32_ptr_extram(txbuffer))
+    {
+      up_imm_free(tp);


Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

patacongo · 2020-10-13T13:47:20Z

arch/xtensa/include/arch.h

+bool up_imm_heapmember(FAR void *mem);
+int up_imm_mallinfo(FAR struct mallinfo *info);
+#endif
+


The naming up_imm_* does not follow the NuttX naming conventions. These should all be xtensa_imm_* . See https://cwiki.apache.org/confluence/display/NUTTX/Naming+of+Architecture%2C+MCU%2C+and+Board+Interfaces

The prefix up_ is reserved only for MCU interfaces called directly by the OS and prototyped in included/nuttx/*.h (usually arch.h)

Originally I had in mind that maybe other architectures would need the same functionality and that prototypes would go to one of the locations you mentioned.
But I didn't follow with that and forgot to rename the functions. Thanks for the reminder. Fixed!

acassis · 2020-10-25T19:17:56Z

arch/xtensa/src/esp32/esp32_allocateheap.c

@@ -77,8 +77,13 @@
 void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
 {
  board_autoled_on(LED_HEAPALLOCATE);
+#ifdef CONFIG_XTENSA_USE_SEPERATE_IMEM


A small typo: SEPERATE -> SEPARATE

acassis · 2020-10-25T19:18:53Z

arch/xtensa/src/common/xtensa_createstack.c

@@ -73,6 +73,18 @@
 #define STACK_ALIGN_DOWN(a) ((a) & ~STACK_ALIGN_MASK)
 #define STACK_ALIGN_UP(a)   (((a) + STACK_ALIGN_MASK) & ~STACK_ALIGN_MASK)

+#ifdef CONFIG_XTENSA_USE_SEPERATE_IMEM


SEPERATE -> SEPARATE

acassis · 2020-10-25T19:19:56Z

arch/xtensa/Kconfig

@@ -81,6 +81,16 @@ config XTENSA_CP_INITSET
 		is provided by CONFIG_XTENSA_CP_INITSET.  Each bit corresponds to one
 		coprocessor with the same bit layout as for the CPENABLE register.

+config XTENSA_USE_SEPERATE_IMEM


A small typo: SEPERATE -> SEPARATE

acassis · 2020-10-25T19:21:12Z

arch/xtensa/Kconfig

+config XTENSA_IMEM_PROCFS
+  bool "Internal memory PROCFS support"
+  default n
+	depends on XTENSA_USE_SEPERATE_IMEM && !DISABLE_MOUNTPOINT && FS_PROCFS && FS_PROCFS_REGISTER


please align "depends on" below "default n"

I think this is just a Github glitch. You can see the file here: https://github.com/apache/incubator-nuttx/blob/7b1234bb38379c5ce28c3ffefa82c4f6b4b42e0d/arch/xtensa/Kconfig

be prefixed by xtensa_ instead of up_. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

Ouss4 added 9 commits October 9, 2020 10:22

arch/xtensa/esp32: Allow internal drivers and tasks' stack to be

07d3dae

allocated in an internal heap. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32: Add a PROCFS entry for the internal memory

07df224

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32/esp32_spiflash.c&esp32_spi.c: Allocate a buffer…

1be6e5a

… from DRAM when the given buffer is from PSRAM. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

boards/xtensa/esp32/esp32-core/configs: When there is no PSRAM, only one

a927b46

memory region is available. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32/esp32_allocateheap.c: Delete a preprocessor

d07c0e9

warning that's not relevant anymore. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32/hardware/esp32_soc.h: Lowercase hex value

f61a1b3

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32/hardware: PIN_CTRL was defined twice.

569ff98

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32: Fix PR apache#1958 nxstyle issues.

3c13215

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

baords/xtensa/esp32/esp32-core: Refresh all the defconfigs

3d784de

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

donghengqaz reviewed Oct 12, 2020

View reviewed changes

Ouss4 added 3 commits October 12, 2020 15:39

arch/xtensa/src/common: Refactor the mm_ macros into a separate file.

4a8557a

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/esp32/esp32_spi.c: Instead of returning with no error

7fcfea8

code, assert the return of the imm_malloc function. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/src/commin/xtensa_usestack&createstack.c: Set the alignment

3a6918f

to be 4 bytes. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

yamt reviewed Oct 13, 2020

View reviewed changes

Ouss4 added 2 commits October 13, 2020 10:15

arch/xtensa/src/esp32/esp32_spi&spiflash: Free the correct buffer.

82ad8af

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

arch/xtensa/Kconfig: Add help for the seperate internal heap.

8140eae

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

patacongo reviewed Oct 13, 2020

View reviewed changes

acassis reviewed Oct 25, 2020

View reviewed changes

Ouss4 force-pushed the coex branch 2 times, most recently from 7b1234b to 8eb6b04 Compare October 25, 2020 21:41

arch/xtensa: Fix the naming of the internal heap functions. They should

8eb6b04

be prefixed by xtensa_ instead of up_. Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

acassis merged commit 9b98f20 into apache:master Oct 25, 2020

acassis pushed a commit that referenced this pull request Oct 25, 2020

arch/xtensa/src/esp32: Fix PR #1958 nxstyle issues.

f0ae1dd

Signed-off-by: Abdelatif Guettouche <abdelatif.guettouche@espressif.com>

Ouss4 deleted the coex branch October 26, 2020 09:47

btashton added this to To-Add in Release Notes - 10.1.0 Apr 12, 2021

jerpelea moved this from To-Add to arch in Release Notes - 10.1.0 Apr 13, 2021

jerpelea moved this from arch to Added in Release Notes - 10.1.0 Apr 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xtensa/esp32: When PSRAM is enabled allow drivers and tasks to allocate memory from a separate internal memory #1958

xtensa/esp32: When PSRAM is enabled allow drivers and tasks to allocate memory from a separate internal memory #1958

Ouss4 commented Oct 9, 2020

donghengqaz Oct 12, 2020

Ouss4 Oct 12, 2020

Ouss4 Oct 12, 2020

Ouss4 Oct 12, 2020

donghengqaz Oct 12, 2020

Ouss4 Oct 12, 2020

Ouss4 Oct 12, 2020

donghengqaz Oct 12, 2020

Ouss4 Oct 12, 2020

yamt Oct 13, 2020

Ouss4 Oct 13, 2020

yamt Oct 13, 2020

Ouss4 Oct 13, 2020

yamt Oct 13, 2020

yamt Oct 13, 2020

Ouss4 Oct 13, 2020

yamt Oct 13, 2020

Ouss4 Oct 13, 2020

patacongo Oct 13, 2020

patacongo Oct 13, 2020

Ouss4 Oct 13, 2020

acassis Oct 25, 2020

Ouss4 Oct 25, 2020

acassis Oct 25, 2020

Ouss4 Oct 25, 2020

acassis Oct 25, 2020

Ouss4 Oct 25, 2020

acassis Oct 25, 2020

Ouss4 Oct 25, 2020

Navigation Menu

xtensa/esp32: When PSRAM is enabled allow drivers and tasks to allocate memory from a separate internal memory #1958

xtensa/esp32: When PSRAM is enabled allow drivers and tasks to allocate memory from a separate internal memory #1958

Conversation

Ouss4 commented Oct 9, 2020

Summary

Impact

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment