Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) #12481

FL0WL0W · 2023-10-30T06:32:35Z

Components of the ESP system that are unrelated to FREERTOS should use the appropriate build definitions. Since these are part of the ESP system they should be using CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE instead of CONFIG_FREERTOS_UNICORE.

Since CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE is directly set by CONFIG_FREERTOS_UNICORE, there will functionally be no difference to how it is now. So there is no risk in making these updates

I am working on making Asymetric Multicore Processing (AMP) work using FREERTOS on one core and bare metal application on the other. This is the first step towards making that possible. This change alone will not make this possible. I will create another issue and PR for making that possible as there are more risky changes the need to be made for that.

See Issue #12480

CLAassistant · 2023-10-30T06:32:40Z

All committers have signed the CLA.

github-actions · 2023-10-30T06:33:16Z

	Messages
📖	Good Job! All checks are passing!

👋 Welcome FL0WL0W, thank you for your first contribution to espressif/esp-idf project!

📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project.

Pull request review and merge process you can expect

Espressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

An internal issue has been created for the PR, we assign it to the relevant engineer
They review the PR and either approve it or ask you for changes or clarifications
Once the Github PR is approved, we synchronize it into our internal git repository
In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
If the change is approved and passes the tests it is merged into the master branch
On next sync from the internal git repository merged change will appear in this public Github repository

Generated by 🚫 dangerJS against c235f7b

igrr · 2023-10-31T10:50:37Z

components/esp_mm/esp_mmu_map.c

@@ -424,7 +424,7 @@ static void IRAM_ATTR NOINLINE_ATTR s_do_mapping(mmu_target_t target, uint32_t v

    cache_bus_mask_t bus_mask = cache_ll_l1_get_bus(0, vaddr_start, size);
    cache_ll_l1_enable_bus(0, bus_mask);
-#if !CONFIG_FREERTOS_UNICORE
+#if !CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE


@FL0WL0W In your AMP concept, what meaning do you associate with CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE?

Does it mean that only one core is used — FreeRTOS running on CPU 0, and nothing at all runs on CPU1

Or does it mean that only one core is used by an IDF app, but there may possibly be a bare-metal app running on CPU1?

Or, putting the same question the other way, what will be the value of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE in an AMP scenario?

Hi @igrr

I will be creating another pull request for AMP. This one is more of a logical reason. These places are not FreeRTOS bits of code and are part of the ESP_System.

To answer your question. In AMP, CONFIG_FREERTOS_UNICORE will be defined and CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE will not be defined. FreeRTOS will be running on one core, while the other core runs a bare metal application.

FREERTOS_UNICORE means FreeRTOS is only running on a single core, but should not necessarily mean nothing is running on the other core
ESP_SYSTEM_SINGLE_CORE_MODE means only 1 core is running code and the other core is disabled and held in reset.

Thank you

In that case, I don't think all the changes in this PR are correct. For example, when we disable the cache during flash operations, we should only synchronize with the CPU1 if there is an ipc1 task running there. However, in the AMP case there is no ipc1 task running on CPU 1, so the task writing to flash on CPU 0 will get blocked forever.

Yes you are correct, I switched that to use ipc_isr instead when in AMP mode. But that is a larger change that will need more effort to review. I want to get this PR finished first since it is effectively not changing anything given currently ESP_SYSTEM_SINGLE_CORE_MODE = FREERTOS_UNICORE

o-marshmallow · 2023-11-01T03:51:34Z

Hello @FL0WL0W ,

Thank you for your contribution! Your MR does correct the assumptions that ESP_SYSTEM_SINGLE_CORE_MODE == FREERTOS_UNICORE while writing the drivers, we would like to see it merged once you consider it ready.

Regarding the future AMP feature, it has been discussed in this Github issue in the past: #10410

While merge this AMP feature as part of IDF experimental features is theoretically feasible, we would rather prefer it to be an external component since we don't plan on supporting or maintaining it.

Of course, AMP feature would still be supported by Espressif Zephyr team as part of the Zephyr Project.

FL0WL0W · 2023-11-01T04:49:11Z

Hi @o-marshmallow

I consider this PR as ready for merging.

I understand that Espressif is not interested in supporting the AMP feature. There are a lot of moving parts that make it difficult to support it. As @igrr mentioned in #10410 there are silicon bug workarounds and memory considerations that make it impossible to make this a separate component from the IDF Kernel.

I would be happy to have AMP merged as an experimental feature, with the expectation that many features outside of basic GPIO and heap memory allocation may not work as expected. We can talk more about this in the upcoming PR.

Thank you

FL0WL0W · 2023-11-21T18:39:25Z

Does "Selected For Development" mean you want me to continue developing the AMP feature? or that you are testing this change in your internal development GITLab?

Looking at the best way to implement the AMP feature, What is the logical difference between portNUM_PROCESSORS and configNUM_CORES? they are always the same value and seem to be used interchangeably. Can i assume that portNUM_PROCESSORS should be ESP_SYSTEM related and configNUM_CORES is FREERTOS related?

o-marshmallow · 2023-11-27T06:56:14Z

Hello @FL0WL0W ,

Sorry for the delay of response, Selected For Development means that your PR is accepted and we are going to create an internal PR to merge it IDF. Once merged and synced to Github, it will be updated here.

o-marshmallow · 2023-11-27T07:08:59Z

@FL0WL0W In order to preserve your as the commit author, would you mind squashing your first two commits, rebase on master and modify the description to something more explicit such as:

Use `CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE` configuration option instead of `CONFIG_FREERTOS_UNICORE` in components not related to FreeRTOS

If you don't mind losing the SHA and the commit author for your changes (due to git rebase), I can perform these modifications locally after creating an internal MR.

Tell me what you prefer

…RTOS

FL0WL0W · 2023-11-28T06:42:06Z

@o-marshmallow First time I have used rebase, but I think I did it correctly. If not, feel free to perform the modifications yourself.

o-marshmallow · 2023-11-28T07:47:21Z

@FL0WL0W Thank you, it's much better!

o-marshmallow · 2023-11-28T07:47:27Z

sha=c235f7b0a166b9633fe0d70398f94943dc6997c8

…RTOS Mergeshttps://github.com//pull/12481

FL0WL0W · 2023-12-08T01:10:21Z

I think this can be closed now that it is pulled in.

espressif-bot added the Status: Opened Issue is new label Oct 30, 2023

github-actions bot changed the title ~~Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE~~ Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) Oct 30, 2023

igrr reviewed Oct 31, 2023

View reviewed changes

espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Nov 1, 2023

FL0WL0W force-pushed the master branch 3 times, most recently from eb35ab3 to a4a01d1 Compare November 28, 2023 06:35

Use configuration option instead of in components not related to Free…

c235f7b

…RTOS

FL0WL0W force-pushed the master branch from a4a01d1 to c235f7b Compare November 28, 2023 06:39

o-marshmallow added the PR-Sync-Rebase Pull request sync as rebase commit label Nov 28, 2023

espressif-bot pushed a commit that referenced this pull request Dec 2, 2023

Use configuration option instead of in components not related to Free…

d149c1b

…RTOS Mergeshttps://github.com//pull/12481

FL0WL0W closed this Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) #12481

Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) #12481

FL0WL0W commented Oct 30, 2023

CLAassistant commented Oct 30, 2023 •

edited

github-actions bot commented Oct 30, 2023 •

edited

igrr Oct 31, 2023

FL0WL0W Oct 31, 2023 •

edited

igrr Oct 31, 2023

FL0WL0W Oct 31, 2023 •

edited

o-marshmallow commented Nov 1, 2023

FL0WL0W commented Nov 1, 2023

FL0WL0W commented Nov 21, 2023

o-marshmallow commented Nov 27, 2023

o-marshmallow commented Nov 27, 2023

FL0WL0W commented Nov 28, 2023

o-marshmallow commented Nov 28, 2023

o-marshmallow commented Nov 28, 2023

FL0WL0W commented Dec 8, 2023

Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) #12481

Many places in the ESP_SYSTEM are using CONFIG_FREERTOS_UNICORE instead of CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE (IDFGH-11333) #12481

Conversation

FL0WL0W commented Oct 30, 2023

CLAassistant commented Oct 30, 2023 • edited

github-actions bot commented Oct 30, 2023 • edited

Pull request review and merge process you can expect

igrr Oct 31, 2023

Choose a reason for hiding this comment

FL0WL0W Oct 31, 2023 • edited

Choose a reason for hiding this comment

igrr Oct 31, 2023

Choose a reason for hiding this comment

FL0WL0W Oct 31, 2023 • edited

Choose a reason for hiding this comment

o-marshmallow commented Nov 1, 2023

FL0WL0W commented Nov 1, 2023

FL0WL0W commented Nov 21, 2023

o-marshmallow commented Nov 27, 2023

o-marshmallow commented Nov 27, 2023

FL0WL0W commented Nov 28, 2023

o-marshmallow commented Nov 28, 2023

o-marshmallow commented Nov 28, 2023

FL0WL0W commented Dec 8, 2023

CLAassistant commented Oct 30, 2023 •

edited

github-actions bot commented Oct 30, 2023 •

edited

FL0WL0W Oct 31, 2023 •

edited

FL0WL0W Oct 31, 2023 •

edited