cpu/esp32: add support for ESP32-S2

[gschorcht]

Contribution description

This PR is a split-off from PR #18235, which provides

• the support for ESP32-S2 SoC
• a common ESP32-S2 board definition
• the board definition for the ESP32S2-DevKit board.

This PR requires PRs #18502, #18503, #18504, #18505 and #18509 to be compilable in CI.

Testing procedure

Beside of Green CI some basic tests should work (Of course, I have already tested all of them and many more):

• tests/shell
• tests/periph_cpuid
• tests/periph_gpio
• tests/periph_hwrng
• tests/periph_i2c
• tests/periph_spi
• tests/periph_rtt
• tests/periph_timer
• examples/gnrc_networking using esp_wifi as optional module.

Issues/PRs references

Depends on #18502
Depends on #18503
Depends on #18504
Depends on #18505
Depends on #18509

[benpicco]

Shouldn't TIMER_NUMOF be 3 then?

[gschorcht]

If the hardware counter implementation for periph/timer is used (MODULE_ESP_HW_COUNTER is defined), there are three timer devices, but the third timer device uses a high-level interrupt that cannot be disabled. Therefore, the number of timer devices is limited to two, so only TIMER_DEV(0) and TIMER_DEV(1) can be used.

In case of the hardware timer implementation for periph/timer (MODULE_ESP_HW_COUNTER is not defined), the number of timers TIMER_NUMOF is derived from SOC_TIMER_GROUP_TOTAL_TIMERS here

RIOT/cpu/esp32/include/periph_cpu.h

Line 659 in c908a60

#define TIMER_NUMOF (SOC_TIMER_GROUP_TOTAL_TIMERS - 1)

.

[benpicco]

Awesome, works like a charm!
I had to patch two lines to get it to work though

Whitespace gets added to the variable

--- a/cpu/esp32/bootloader/Makefile
+++ b/cpu/esp32/bootloader/Makefile
@@ -321,7 +321,8 @@ ESPTOOL ?= $(RIOTTOOLS)/esptools/esptool_v3.2.py
 # require to export these values.
 FLASH_MODE ?= dio   # ESP-IDF uses dio as default flash mode for the bootloader
 FLASH_FREQ ?= 40m   # lowest frequency all ESP32x SoC support
-FLASH_SIZE ?= 2     # smallesrt size all ESP32x SoC support
+# smallesrt size all ESP32x SoC support
+FLASH_SIZE ?= 2

otherwise I get

home/benpicco/dev/RIOT/dist/tools/esptools/esptool_v3.2.py --chip esp32s2 elf2image --flash_mode dio    \
  --flash_size 2     MB --flash_freq 40m    -o /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/esp32s2-devkit/esp_bootloader/bootloader.bin /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/esp32s2-devkit/esp_bootloader/bootloader.elf
usage: esptool elf2image [-h] [--output OUTPUT] [--version {1,2}] [--min-rev {0,1,2,3}] [--secure-pad]
                         [--secure-pad-v2] [--elf-sha256-offset ELF_SHA256_OFFSET] [--use_segments]
                         [--flash_freq {40m,26m,20m,80m}] [--flash_mode {qio,qout,dio,dout}]
                         [--flash_size FLASH_SIZE] [--spi-connection SPI_CONNECTION]
                         input
esptool elf2image: error: argument --flash_size/-fs: 2 is not a known flash size. Known sizes: 512KB, 256KB, 1MB, 2MB, 4MB, 2MB-c1, 4MB-c1, 8MB, 16MB

--- a/cpu/esp_common/esp-xtensa/thread_arch.c
+++ b/cpu/esp_common/esp-xtensa/thread_arch.c
@@ -286,7 +286,7 @@ void IRAM_ATTR thread_yield_higher(void)
         ets_soft_int_type = ETS_SOFT_INT_YIELD;
         WSR(BIT(ETS_SOFT_INUM), interrupt);
         critical_exit();
-#elif defined(CPU_FAM_ESP32)
+#elif defined(DPORT_CPU_INTR_FROM_CPU_0_REG)
         /* generate the software interrupt to switch the context */
         DPORT_WRITE_PERI_REG(DPORT_CPU_INTR_FROM_CPU_0_REG, DPORT_CPU_INTR_FROM_CPU_0);
 #else

[gschorcht]

Awesome, works like a charm! I had to patch two lines to get it to work though

Whitespace gets added to the variable

Strange, this isn't a change made in this PR and I didn't test it after rebasing. It isn't a problem for the other ESP32x SoCs, probably because FLASH_SIZE is already set for them.

Maybe, it would be better to define the FLASH_SIZE together with the unit as we already do for the FLASH_SPEED to be consistent.

[gschorcht]

Strange, this isn't a change made in this PR and I didn't test it after rebasing. It isn't a problem for the other ESP32x SoCs, probably because FLASH_SIZE is already set for them.

Hm, the default size FLASH_SIZE is defined for each ESP32x SoC at the beginning of cpu/esp32/Makefile.include and is known for all ESP32x SoCs in cpu/esp32/bootloader/Makefile when entering cpu/esp32/bootloader except for the ESP32-S2. At the moment I have no idea what the difference could be.

[benpicco]

Yea I don't know who thought it was a great idea that whitespace before a comment should be added to a variable, but we also had this problem with pkts recently: #18494

[gschorcht]

Yea I don't know who thought it was a great idea that whitespace before a comment should be added to a variable, but we also had this problem with pkts recently: #18494

Even if we can fix the problem for this variable in some way, the question remains why the default definition of FLASH_SIZE in cpu/esp32/Makefile.include is only not visible for the ESP32-S2 in cpu/esp32/bootloader/Makefile while it is for all other ESP32x SoCs.

[benpicco]

But isn't the problem that only ESP32-S2 uses the default definition of FLASH_SIZE, so the others don't experience the problem with the trailing whitespace because they overwrite it?

[gschorcht]

But isn't the problem that only ESP32-S2 uses the default definition of FLASH_SIZE, so the others don't experience the problem with the trailing whitespace because they overwrite it?

I don't understand why the default definition in cpu/esp32/bootloader/Makefile is used for ESP32-S2 only. FLASH_SIZE is set for ESP32-S2 as for all other ESP32x SoCs here:

RIOT/cpu/esp32/Makefile.include

Lines 16 to 19 in 5b6dff0

	else ifneq (,$(filter esp32s2,$(CPU_FAM)))
	FLASH_MODE ?= qio
	FLASH_FREQ ?= 80m
	FLASH_SIZE ?= 4

[benpicco]

Ah I found the reason why

--- a/cpu/esp32/Makefile.include
+++ b/cpu/esp32/Makefile.include
@@ -14,9 +14,9 @@ else ifneq (,$(filter esp32c3 esp32s3,$(CPU_FAM)))
   export FLASH_SIZE ?= 2
   BOOTLOADER_POS = 0x0000
 else ifneq (,$(filter esp32s2,$(CPU_FAM)))
-  FLASH_MODE ?= qio
-  FLASH_FREQ ?= 80m
-  FLASH_SIZE ?= 4
+  export FLASH_MODE ?= qio
+  export FLASH_FREQ ?= 80m
+  export FLASH_SIZE ?= 4
   BOOTLOADER_POS = 0x1000
 else
   $(error Unkwnown ESP32x SoC variant (family))

[gschorcht]

Ah I found the reason why

--- a/cpu/esp32/Makefile.include
+++ b/cpu/esp32/Makefile.include
@@ -14,9 +14,9 @@ else ifneq (,$(filter esp32c3 esp32s3,$(CPU_FAM)))
   export FLASH_SIZE ?= 2
   BOOTLOADER_POS = 0x0000
 else ifneq (,$(filter esp32s2,$(CPU_FAM)))
-  FLASH_MODE ?= qio
-  FLASH_FREQ ?= 80m
-  FLASH_SIZE ?= 4
+  export FLASH_MODE ?= qio
+  export FLASH_FREQ ?= 80m
+  export FLASH_SIZE ?= 4
   BOOTLOADER_POS = 0x1000
 else
   $(error Unkwnown ESP32x SoC variant (family))

I looked at it 100 times and still missed this 😟

[gschorcht]

I had to rebase to fix it.

[benpicco]

I still need this patch for it to compile

--- a/cpu/esp_common/esp-xtensa/thread_arch.c
+++ b/cpu/esp_common/esp-xtensa/thread_arch.c
@@ -286,7 +286,7 @@ void IRAM_ATTR thread_yield_higher(void)
         ets_soft_int_type = ETS_SOFT_INT_YIELD;
         WSR(BIT(ETS_SOFT_INUM), interrupt);
         critical_exit();
-#elif defined(CPU_FAM_ESP32)
+#elif defined(DPORT_CPU_INTR_FROM_CPU_0_REG)
         /* generate the software interrupt to switch the context */
         DPORT_WRITE_PERI_REG(DPORT_CPU_INTR_FROM_CPU_0_REG, DPORT_CPU_INTR_FROM_CPU_0);
 #else

[benpicco]

Please squash

[gschorcht]

Somewhere here:

(adding undefine BOARD_VERSION there fixes make info-boards-supported)

Hm, BOARD_VERSION isn't an official variable of RIOT's make system. I introduced it only for ESP32-S2-DevKit and ESP-S3-DevKit boards to avoid separate board definitions for dozens of variants of an identical board, differing only in the size of the integrated flash and the integrated SPI RAM. All these board variants are identical in their peripheral configuration so that separate board definitions would only blow up the number of boards in CI compilation a lot.

[kaspar030]

Sorry, this is one of the many quirks of the make based dependency resolution. It doesn't matter if the variable is "official", if one iteration sets a value that the next doesn't understand, the dep resolution fails.
I think it's a nice solution. But that variable needs to be cleared...

[benpicco]

Now there is only a KConfig mismatch left for this to be merged 😃

[gschorcht]

Now there is only a KConfig mismatch left for this to be merged smiley

Hm, when I test it with

TEST_KCONFIG=1 BOARD=esp32s2-devkit make -j8 -C tests/shell info-features-provided

all provided features are the same as without TEST_KCONFIG=1.

[gschorcht]

@benpicco I squashed the fix suggested by @MrKevinWeiss directly into commit 1862d4e. I added a new commit 18cf35b to fix the default BOARD_VERSION setting.

[gschorcht]

@benpicco I squashed the fix suggested by @MrKevinWeiss directly into commit 1862d4e. I added a new commit 18cf35b to fix the default BOARD_VERSION setting.

I fixed it for ESP32-S2 and ESP32-S3 instead of opening a separate PR for this very small change, even though this PR is not related to ESP32-S3.

[gschorcht]

@benpicco Thanks for reviewing and merging.

[gschorcht]

The next big challenge will be to upgrade ESP-IDF to version 5.0, which includes a lot of changes, also in the APIs. ESP-IDF 5.0 is required to support ESP32-C2 and ESP32-H2.

[gschorcht]

@kaspar030 @MrKevinWeiss thanks for your ideas to solve the last remaining compile problems.