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.

Sign up for GitHub

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

Jump to bottom

zephyr: include: rtos: Use native Zephyr cache management functions o… #7458

Merged
merged 2 commits into from
May 25, 2023
Merged

zephyr: include: rtos: Use native Zephyr cache management functions o… #7458

merged 2 commits into from
May 25, 2023

Conversation

LaurentiuM1234
Copy link
Contributor

@LaurentiuM1234 LaurentiuM1234 commented Apr 19, 2023 •
edited
Loading

Since Zephyr offers cache-management operations for the arm64 architecture we can make use of them on all ARM64-based platforms. In time, if the architecture supports it, more platforms could end up using them instead of using the ones in arch/lib/cache.h.

Please see #7192 for PR dependency graph.

@LaurentiuM1234 LaurentiuM1234 mentioned this pull request Apr 19, 2023
Closed
@LaurentiuM1234 LaurentiuM1234 marked this pull request as ready for review April 19, 2023 09:57
kv2019i
kv2019i reviewed Apr 19, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

In principle ok, but some questions, please see inline

zephyr/include/rtos/cache.h Outdated Show resolved Hide resolved
zephyr/include/rtos/cache.h Show resolved Hide resolved
@LaurentiuM1234 LaurentiuM1234 mentioned this pull request Apr 20, 2023
Merged
@lgirdwood
Copy link
Member

@LaurentiuM1234 any update, I think the Zephyr cache changes are now merged ?

@LaurentiuM1234
Copy link
Contributor Author

@LaurentiuM1234 any update, I think the Zephyr cache changes are now merged ?

Checked zephyrproject-rtos/zephyr#50136 and it has been tagged with DNM for now. Seems like it will be merged later on.

Might be worth keeping the wrappers over the Zephyr cache-management API at least on ARM64 where it seems like there's an option to device_map (K_MEM_CACHE_NONE) which marks the region as non-cacheable. In this case there would probably be no need to invalidate the data cache (at least for the window regions: stream, hostbox, outbox etc...). We'd have to pass something like region_is_cached to the wrappers and based on its value decide to call the Zephyr cache-management API. But at this point this is just an idea at the top of my head. Removing the wrappers for now and re-adding them later on could also be a solution I guess since I'm not 100% confident that my idea will work as expected. Let me know what you think @lgirdwood @kv2019i @nashif.

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 28, 2023

@LaurentiuM1234 wrote:

Checked zephyrproject-rtos/zephyr#50136 and it has been tagged with DNM for now. Seems like it will be merged later on.

This was now merged and pulled into SOF main.

Might be worth keeping the wrappers over the Zephyr cache-management API
at least on ARM64 where it seems like there's an option to device_map (K_MEM_CACHE_NONE)

I think we must keep the wrappers as long as we have any non-Zephyr targets in SOF main (although at some point we can turn these into reverse wrappers, i.e. change app code to call Zephyr interface, and provide wrappers for XTOS builds).

But for Zephyr builds, we should map to native Zephyr calls for all targets and not include arch/lib/cache.h (this should be reserved for XTOS).

Can you update the PR so that the mapping to native Zephyr calls is done for all targets and then you can have some additional defines for ARM64 on top?

@LaurentiuM1234 LaurentiuM1234 marked this pull request as draft May 2, 2023 10:59
@LaurentiuM1234 LaurentiuM1234 marked this pull request as ready for review May 8, 2023 07:53
@LaurentiuM1234
Copy link
Contributor Author

@kv2019i Sorry for taking such a long time to update this. It should be good to go RN. Tested on NXP's side and seems to work fine.

@LaurentiuM1234 LaurentiuM1234 marked this pull request as draft May 8, 2023 08:42
@LaurentiuM1234 LaurentiuM1234 force-pushed the native_zephyr_cache_api branch 2 times, most recently from 95d78f8 to c80343d Compare May 12, 2023 12:17
@LaurentiuM1234
Copy link
Contributor Author

Fixed CI errors for sparse Zephyr and included a little macro cleanup.

@LaurentiuM1234 LaurentiuM1234 marked this pull request as ready for review May 12, 2023 12:27
lyakh
lyakh requested changes May 15, 2023
View reviewed changes
zephyr/include/rtos/cache.h Outdated Show resolved Hide resolved
@LaurentiuM1234
Copy link
Contributor Author

Updates:

  1. Introduced CONFIG_SOF_DCACHE_LINE_SIZE which replaces all the messy ifdefs from zephyr/include/rtos/cache.h. This config can be removed when all vendors define CONFIG_DCACHE_LINE_SIZE in their Zephyr board configurations. For now, this configuration will try to use the value of CONFIG_DCACHE_LINE_SIZE if it's set properly, otherwise it will fallback to some default value based on platform.

kv2019i
kv2019i approved these changes May 15, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @LaurentiuM1234 , looks great now!

@LaurentiuM1234
Copy link
Contributor Author

Fixed typo in commit message leading to checkpatch error.

lyakh
lyakh approved these changes May 16, 2023
View reviewed changes
zephyr/include/rtos/cache.h Outdated
*
* TODO: in time, switch to simply using CONFIG_DCACHE_LINE_SIZE
*/
#define DCACHE_LINE_SIZE CONFIG_SOF_DCACHE_LINE_SIZE
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering how much more work would it be to go with CONFIG_DCACHE_LINE_SIZE directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, shouldn't be much trouble. Will give it a shot and submit a Zephyr PR if I can get something working.

Copy link
Collaborator

@dbaluta dbaluta May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh so should we merge this? and make the change in a separate PR?

@LaurentiuM1234 LaurentiuM1234 marked this pull request as draft May 16, 2023 09:14
@LaurentiuM1234
Copy link
Contributor Author

LaurentiuM1234 commented May 16, 2023 •
edited
Loading

Marking this as a draft until I can figure out if we can easily get rid of CONFIG_SOF_DCACHE_LINE_SIZE by using CONFIG_DCACHE_LINE_SIZE from Zephyr. Might as well do this now so we won't have to change it later.

@LaurentiuM1234
Copy link
Contributor Author

Zephyr PR in which CONFIG_DCACHE_LINE_SIZE is set for all SOF-supported boards is ready: zephyrproject-rtos/zephyr#57926. Will update this PR once the Zephyr one is merged.

@LaurentiuM1234
Copy link
Contributor Author

LaurentiuM1234 commented May 22, 2023 •
edited
Loading

zephyrproject-rtos/zephyr#57926 has been merged. Now waiting on #7642 and this can be merged IMO.

EDIT: CI fails are normal for now because CONFIG_DCACHE_LINE_SIZE is not currently set without #7642.

@LaurentiuM1234 LaurentiuM1234 marked this pull request as ready for review May 25, 2023 07:44
@LaurentiuM1234
zephyr: include: rtos: Switch to using Zephyr cache management API
809b414 
Thanks to PR [1], Zephyr cache management API can now be
used on xtensa-based SoCs. As a consequence to this, there's
no longer a need to use SOF's arch/ layer for cache management.
This commit forces all SoCs which support Zephyr to use
its native cache management API.

[1]: zephyrproject-rtos/zephyr#50136

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
@LaurentiuM1234
zephyr: include: rtos: cache.h: Remove unused macros
0beb03f 
This commit removes CACHE_INVALIDATE and CACHE_WRITEBACK_INV
macros which are not used.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
@LaurentiuM1234
Copy link
Contributor Author

Updates:

  1. Dependency west.yml: Update Zephyr revision #7642 has been merged.
  2. Added support for the host architecture for which CONFIG_DCACHE_LINE_SIZE was not set, thus leading to a build error in the IPC fuzzer CI checks. Please let me know if anyone has a better alternative for this architecture, otherwise, once the CI is done this is mergeable IMO.

@kv2019i
Copy link
Collaborator

kv2019i commented May 25, 2023

Known error https://sof-ci.01.org/sofpr/PR7458/build8506/devicetest/index.html , fuzz check failure unrelated to this PR. Proceeding with merge.

@kv2019i kv2019i merged commit ff0fc14 into thesofproject:main May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif left review comments

@lyakh lyakh lyakh approved these changes

@paulstelian97 paulstelian97 paulstelian97 approved these changes

@btian1 btian1 btian1 approved these changes

@kv2019i kv2019i kv2019i approved these changes

@iuliana-prodan iuliana-prodan Awaiting requested review from iuliana-prodan iuliana-prodan is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@abonislawski abonislawski Awaiting requested review from abonislawski abonislawski is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LaurentiuM1234 @lgirdwood @kv2019i @dbaluta @nashif @lyakh @paulstelian97 @btian1