[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3003: ESP32: Re-organise the different heap regions

[GitBox <gi...@apache.org>]

yamt commented on a change in pull request #3003:
URL: https://github.com/apache/incubator-nuttx/pull/3003#discussion_r589853261



##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -80,18 +83,16 @@
 #endif
 
 #ifdef CONFIG_XTENSA_IMEM_USE_SEPARATE_HEAP
-#define	XTENSA_IMEM_REGION_SIZE	CONFIG_XTENSA_IMEM_REGION_SIZE
+#  define	XTENSA_IMEM_REGION_SIZE	CONFIG_XTENSA_IMEM_REGION_SIZE
 #else
-#define	XTENSA_IMEM_REGION_SIZE	0
+#  define	XTENSA_IMEM_REGION_SIZE	0
 #endif
 
-/* If CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION is defined, it means
- * using maximum separate heap for internal memory, but part of
- * the available memory is reserved for the Region 1 heap.
- */
+/* Internal heap starts at the end of the ROM data. */
 
-#ifdef CONFIG_XTENSA_IMEM_MAXIMIZE_HEAP_REGION
-#ifndef HEAP_REGION_OFFSET
-#define HEAP_REGION_OFFSET      0x2000
-#endif
+#ifndef CONFIG_SMP
+#  define ESP32_IMEM_START  0x3ffe1330

Review comment:
       how about:
   ```
   #define ESP32_IMEM_START HEAP_REGION2_START
   ```

##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -96,3 +96,8 @@
 #  define ESP32_IMEM_START  0x3ffe7e40
 #endif
 
+/* Region of unused ROM App data */
+
+#define HEAP_REGION_ROMAPP_START  0x3ffe4360
+#define HEAP_REGION_ROMAPP_END    0x3ffe5230

Review comment:
       are there public doc or code from which one can infer these values?

##########
File path: arch/xtensa/src/esp32/esp32_allocateheap.c
##########
@@ -96,8 +96,15 @@ void xtensa_add_region(void)
 {
   size_t region2_start = HEAP_REGION2_START + XTENSA_IMEM_REGION_SIZE;
 
+#ifndef CONFIG_SMP
   umm_addregion((FAR void *)region2_start,
                 (size_t)(uintptr_t)&_eheap - region2_start);
+#else
+  umm_addregion((FAR void *)region2_start,
+                (size_t)HEAP_REGION2_END - region2_start);
+  umm_addregion((FAR void *)HEAP_REGION3_START,
+                (size_t)&_eheap - HEAP_REGION3_START);

Review comment:
       this region3 stuff seems to belong to the other commit ("ESP32: Re-organise the different heap regions"), not this one ("arch/xtensa/esp32: Part of the ROM regions in middle of DRAM are not")

##########
File path: boards/xtensa/esp32/esp32-devkitc/scripts/esp32.template.ld
##########
@@ -50,7 +50,7 @@ MEMORY
    * be used.
    */
 
-  dram0_0_seg (RW) :     org = 0x3ffb0000 + CONFIG_ESP32_BT_RESERVE_DRAM,
+  dram0_0_seg (RW) :     org = 0x3ffae6f0 + CONFIG_ESP32_BT_RESERVE_DRAM,

Review comment:
       is there public doc or code from which one can infer these values?

##########
File path: arch/xtensa/include/esp32/memory_layout.h
##########
@@ -72,9 +72,23 @@
  *
  * When an internal heap is enabled this region starts at an offset equal to
  * the size of the internal heap.
+ *
+ * The QEMU bootloader image is slightly different than the chip's one.
+ * The ROM on PRO and APP uses different regions for static data. In QEMU,
+ * however, we load only one ROM binary, taken from the PRO CPU, and it is
+ * used by both CPUs.  So, in QEMU, if we allocate this part early, it will
+ * be clobbered once the APP CPU starts.
+ * We can delay the allocation to when everything has started through the
+ * board_late_initiliaze hook, as is done for the APP data, however this
+ * should be fixed from QEMU side.  The following macros, then, just skip
+ * PRO CPU's data when a QEMU image generation is enabled with SMP.
  */
 
-#define HEAP_REGION2_START  0x3ffe0450
+#if defined(CONFIG_ESP32_QEMU_IMAGE) && defined(CONFIG_SMP)

Review comment:
       can you consider to introduce a separate kconfig for this?
   
   maybe we can follow this precedent.
   ```
   config TIVA_WITH_QEMU
           bool "Workaround for tiva with qemu"
           default n
   ```
   how do you think?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org