cpu/esp32: add support for ESP32-S3 #18421

gschorcht · 2022-08-09T05:43:17Z

Contribution description

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

the support for ESP32-S3 SoC
a common ESP32-S3 board definition
the board definition for the ESP32S3-DevKit board.

This PR requires PRs ~~#18408, #18409, #18410 and #18411~~ to be compilable in CI. ~~Furthermore the CI workers need an update with riotdocker #200~~

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 #18408
Depends on #18409
Depends on #18410
Depends on #18411

gschorcht · 2022-08-11T06:06:27Z

@kaspar030 This PR requires an update of CI workers. Is there a plan when it is done next time? How is it coordinated?

gschorcht · 2022-08-12T11:26:50Z

@benpicco Do you think the CI workers are updated now?

benpicco · 2022-08-12T11:28:32Z

Well it's worth a try. And with the current CI run times the compilation will happen during the night anyway.

If not we got something to point to showing that the automatic updates apparently do not work.

benpicco · 2022-08-12T13:48:56Z

cpu/esp32/include/sdkconfig_esp32.h

+/**
+ * ESP32 specific sleep configuration (DO NOT CHANGE)
+ */
+#define CONFIG_ESP_SLEEP_RTC_BUS_ISO_WORKAROUND 1


What is this about?

To be honest, I don't knwo. It's just a setting that comes from ESP-IDF's Kconfig without any explaination.

config ESP_SLEEP_RTC_BUS_ISO_WORKAROUND bool default y if IDF_TARGET_ESP32(=n) || IDF_TARGET_ESP32S2(=n) || IDF_TARGET_ESP32S3(=n)

Interesting - better add a note that this is from the SDK and true for all XTensa ESPs

Hm, but this is actually the case for all CONFIG_* definitions in the sdkconfig*.h files. They all define configurations that are only required for compiling ESP-IDF sources and are set to the default values of ESP-IDF's Kconfig. Maybe it is better to move them to a directory cpu/esp32/esp-idf/include/config so that it becomes better clear what these files are for.

benpicco · 2022-08-12T13:50:12Z

cpu/esp32/include/sdkconfig_esp32s3.h

+/**
+ * @brief Mapping configured ESP32 default clock to CLOCK_CORECLOCK define
+ */
+#define CLOCK_CORECLOCK     (1000000UL * CONFIG_ESP32S3_DEFAULT_CPU_FREQ_MHZ)


Suggested change

#define CLOCK_CORECLOCK (1000000UL * CONFIG_ESP32S3_DEFAULT_CPU_FREQ_MHZ)

#define CLOCK_CORECLOCK MHZ(CONFIG_ESP32S3_DEFAULT_CPU_FREQ_MHZ)

This would require to include RIOT's macros.h which leads to a number of naming conflicts when compiling ESP IDF sources, including the MHZ macro. sdkconfig_*.h are mainly used for the configuration of ESP-IDF with this only acception. A solution could be to move CLOCK_CORECLOCK to cpu_conf.h. It's just still there since it was added there with 5ae5c40f26de541c1ac804623520f67442588f83`.

Should I try to move it to cpu_conf.h for all ESP32x SoCs with this PR?

It would make more sense in cpu_conf.h since it's not for configuring the SDK but for RIOT consumption only.

Should it be a separate PR which would require an additional rebase or could it be part of this PR?

I don't really have a strong opinion there, whatever you prefer.

Commit 066239d moves the define to the corresponding cpu_*.h files.

cpu/esp32/include/sdkconfig_esp32s3.h

cpu/esp_common/esp-xtensa/thread_arch.c

gschorcht · 2022-08-13T05:11:33Z

Well it's worth a try. And with the current CI run times the compilation will happen during the night anyway.

If not we got something to point to showing that the automatic updates apparently do not work.

The toolchain is still missing 😟

gschorcht · 2022-08-13T05:47:35Z

Well it's worth a try. And with the current CI run times the compilation will happen during the night anyway.
If not we got something to point to showing that the automatic updates apparently do not work.

The toolchain is still missing worried

The question is how we can get the CI worker up to date.

benpicco · 2022-08-13T10:46:10Z

I'd keep the ready for build label set - makes it easier to complain about it on Matrix 😄

gschorcht · 2022-08-17T13:51:40Z

CI is not happy anymore
undefined reference to `cpu_coreclk'
worried Hm, I compiled some tests before pushing the changes. That is, we need an extra CI compilation cycle, quite annoying.

I have moved CLOCK_CORECLOCK definition to periph_cpu.h, see commit e7bf789. Let's hope that it doesn't cause other compilation problems.

This define does not belong to the defines in `sdkconfig_*.h` that are used for the ESP-IDF SDK. It is therefore moved to the corresponding `periph_cpu_*.h` file.

gschorcht · 2022-08-18T07:40:00Z

@benpicco I am getting desperate as I am getting another unrelated error in the CI compilation. I was hoping to merge the PR today to be able to provide the PRs based on it for the ESP32-S3 before my vacation starting tomorrow 😟

maribu · 2022-08-18T07:40:25Z

The current CI run will fail due to unrelated issues. Failures that look like

-- running on worker breeze thread 3, build number 316686.
make: Entering directory '/tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/tests/driver_sam0_eth'
make: Leaving directory '/tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/tests/driver_sam0_eth'
make: Entering directory '/tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/tests/driver_sam0_eth'
Building application "tests_driver_sam0_eth" for "same54-xpro" with MCU "samd5x".

sha1sum /tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/build/tests_driver_sam0_eth.bin > /tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/build/test-input-hash.sha1
   text	   data	    bss	    dec	    hex	filename
  19468	    180	  16932	  36580	   8ee4	/tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/build/tests_driver_sam0_eth.elf
make: Leaving directory '/tmp/dwq.0.3566048456678582/1bc9f0dc6a1abbb862a0fb9058807d10/tests/driver_sam0_eth'
32d31
< netdev_legacy_api
{"build/": 112}

will be fixed by #18466

MrKevinWeiss · 2022-08-18T07:51:12Z

If it is passing mudock and acked there is no reason it can't be merged today or even after your vacation. Maybe we want to enable automerge?

benpicco · 2022-08-18T21:51:43Z

Sorry this didn’t make it, enjoy your vacation! Let’s hope we’re getting CI fixed by the time you’re back 😄

MrKevinWeiss · 2022-08-22T09:38:13Z

I will build once we fix the CI

gschorcht · 2022-08-22T15:43:26Z

Thanks for reviewing and merging.

gschorcht requested review from leandrolanzieri, jia200x, MrKevinWeiss and kaspar030 as code owners August 9, 2022 05:43

gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Aug 9, 2022

gschorcht mentioned this pull request Aug 9, 2022

tests/external_boards: add esp32s3-ci board for optional modules #18422

Merged

gschorcht removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 10, 2022

gschorcht force-pushed the cpu/esp32/add_esp32s3_cpu_support branch from e389aa0 to 55b1686 Compare August 10, 2022 16:00

gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 10, 2022

gschorcht requested a review from benpicco August 11, 2022 06:51

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 12, 2022

benpicco reviewed Aug 12, 2022

View reviewed changes

gschorcht removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 13, 2022

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 13, 2022

gschorcht force-pushed the cpu/esp32/add_esp32s3_cpu_support branch 2 times, most recently from 211f299 to 07e484c Compare August 17, 2022 13:50

cpu/esp32: move CLOCK_CORECLOCK to periph_cpu_*.h

e7bf789

This define does not belong to the defines in `sdkconfig_*.h` that are used for the ESP-IDF SDK. It is therefore moved to the corresponding `periph_cpu_*.h` file.

gschorcht force-pushed the cpu/esp32/add_esp32s3_cpu_support branch from 07e484c to e7bf789 Compare August 17, 2022 13:58

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 17, 2022

MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 18, 2022

maribu enabled auto-merge August 18, 2022 08:34

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 18, 2022

MrKevinWeiss disabled auto-merge August 22, 2022 08:09

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 22, 2022

MrKevinWeiss removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 22, 2022

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 22, 2022

benpicco enabled auto-merge August 22, 2022 14:42

benpicco merged commit 00ede8f into RIOT-OS:master Aug 22, 2022

gschorcht deleted the cpu/esp32/add_esp32s3_cpu_support branch August 23, 2022 15:22

gschorcht mentioned this pull request Aug 28, 2022

cpu/esp32: add support for ESP32-S2 #18506

Merged

maribu added this to the Release 2022.10 milestone Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/esp32: add support for ESP32-S3 #18421

cpu/esp32: add support for ESP32-S3 #18421

gschorcht commented Aug 9, 2022 •

edited

Loading

gschorcht commented Aug 11, 2022

gschorcht commented Aug 12, 2022

benpicco commented Aug 12, 2022

benpicco Aug 12, 2022

gschorcht Aug 13, 2022

benpicco Aug 16, 2022 •

edited

Loading

gschorcht Aug 16, 2022

benpicco Aug 12, 2022

gschorcht Aug 13, 2022

benpicco Aug 16, 2022

gschorcht Aug 16, 2022

benpicco Aug 16, 2022

gschorcht Aug 16, 2022

gschorcht commented Aug 13, 2022

gschorcht commented Aug 13, 2022

benpicco commented Aug 13, 2022

gschorcht commented Aug 17, 2022 •

edited

Loading

gschorcht commented Aug 18, 2022

maribu commented Aug 18, 2022

MrKevinWeiss commented Aug 18, 2022

benpicco commented Aug 18, 2022

MrKevinWeiss commented Aug 22, 2022

gschorcht commented Aug 22, 2022

	#define CLOCK_CORECLOCK (1000000UL * CONFIG_ESP32S3_DEFAULT_CPU_FREQ_MHZ)
	#define CLOCK_CORECLOCK MHZ(CONFIG_ESP32S3_DEFAULT_CPU_FREQ_MHZ)

cpu/esp32: add support for ESP32-S3 #18421

cpu/esp32: add support for ESP32-S3 #18421

Conversation

gschorcht commented Aug 9, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

gschorcht commented Aug 11, 2022

gschorcht commented Aug 12, 2022

benpicco commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Aug 16, 2022 • edited Loading

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

gschorcht commented Aug 13, 2022

gschorcht commented Aug 13, 2022

benpicco commented Aug 13, 2022

gschorcht commented Aug 17, 2022 • edited Loading

gschorcht commented Aug 18, 2022

maribu commented Aug 18, 2022

MrKevinWeiss commented Aug 18, 2022

benpicco commented Aug 18, 2022

MrKevinWeiss commented Aug 22, 2022

gschorcht commented Aug 22, 2022

gschorcht commented Aug 9, 2022 •

edited

Loading

benpicco Aug 16, 2022 •

edited

Loading

gschorcht commented Aug 17, 2022 •

edited

Loading