Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/esp32_sdk: additional patches required for ESP32-S3 #18408

Merged

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Aug 7, 2022

Contribution description

This PR is a split-off from PR #18185 and provides additional patches of ESP-IDF required for the ESP32-S3 support.

In detail the patches include following changes:

  • define ARRAY_SIZE in component/spi_flash/spi_flash_timing_tuning.c only if it is not yet defined by RIOT macros
  • add an alternative implementation of spi_flash_disable_interrupts_caches_and_other_cpu and spi_flash_enable_interrupts_caches_and_other_cpu if component/spi_flash/cache_utils.c is compiled for RIOT
  • fix the undefined reference to rtc_gpio_force_hold_en_all in components/driver/gpio.c

Testing procedure

Green CI

Issues/PRs references

Split-off from PR #18185

@github-actions github-actions bot added the Area: pkg Area: External package ports label Aug 7, 2022
@gschorcht gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Aug 7, 2022
#define DPORT_CACHE_GET_VAL(cpuid) (cpuid == 0) ? DPORT_CACHE_VAL(PRO) : DPORT_CACHE_VAL(APP)
#define DPORT_CACHE_GET_MASK(cpuid) (cpuid == 0) ? DPORT_CACHE_MASK(PRO) : DPORT_CACHE_MASK(APP)

-#ifndef RIOT_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not fix pkg/esp32_sdk/patches/0010-spi_flash-disable-functions-not-required-or-not-supp.patch instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍 Hm, to be honest, I didn't have this option in mind. Does it matter? I can change it, but that requires at least one additional Murdock compilation, which is a bit of a gamble right now 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, merging this patch with pkg/esp32_sdk/patches/0010-spi_flash-disable-functions-not-required-or-not-supp.patch would affect the serial number of subsequent patches which are either already merged (e.g. #17601) or part of other PRs (e.g. PR #17442).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have changed patch pkg/esp32_sdk/patches/0010-spi_flash-disable-functions-not-required-or-not-supp.patch accordingly. Furthermore, I renamed the bootloader patch pkg/esp32_sdk/patches/0024-bootloader-remove-compile-from-banner.patch to fix the serial number.

I squashed directly to avoid an additional CI compilation.

Copy link
Contributor Author

@gschorcht gschorcht Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the title of 0010-spi_flash-disable-functions-not-required-or-not-supp.patch to something 0010-spi_flash-disable-and-replace-functions.patch didn't work. The result was that the additional functions were lost and the compilation failed. I'm not really happy with it, because the patch now does more than just disable some functions. If it makes more sense to have a separate patch for the additional functions, I can change it back.

Copy link
Contributor Author

@gschorcht gschorcht Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benpicco Are the patches OK now or should I revert the last changes?

The patches include the following changes:
- define ARRAY_SIZE in `component/spi_flash/spi_flash_timing_tuning.c` only if it is not yet defined by RIOT macros
- add alternative implementations for`spi_flash_disable_interrupts_caches_and_other_cpu` and `spi_flash_enable_interrupts_caches_and_other_cpu` if compiled for RIOT
- fix the undefined reference to `rtc_gpio_force_hold_en_all` in `components/driver/gpio.c`
- rename the bootloader patch to fix the serial number
@gschorcht gschorcht force-pushed the pkg/esp32_sdk/patches_for_esp32s3 branch from c472d82 to ca34e97 Compare August 8, 2022 06:18
@github-actions github-actions bot removed Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Aug 8, 2022
@gschorcht gschorcht added the Platform: ESP Platform: This PR/issue effects ESP-based platforms label Aug 8, 2022
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better - always like less patches :)

@benpicco benpicco enabled auto-merge August 9, 2022 19:53
@benpicco benpicco merged commit 1310128 into RIOT-OS:master Aug 9, 2022
@gschorcht gschorcht deleted the pkg/esp32_sdk/patches_for_esp32s3 branch August 10, 2022 04:57
@gschorcht
Copy link
Contributor Author

gschorcht commented Aug 10, 2022

@benpicco Thanks for reviewing and merging.

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants