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

[RFC] Cache API rework #50988

Merged
merged 9 commits into from
Dec 1, 2022
Merged

Conversation

carlocaione
Copy link
Collaborator

@carlocaione carlocaione commented Oct 5, 2022

This is the outcome of the discussion in #50747 and #50136.

The cache API is now composed by:

  • cache_{data,instr}_enable()
  • cache_{data,instr}_disable()
  • cache_{data,instr}_line_size_get()
  • cache_{data,instr}_flush_all()
  • cache_{data,instr}_flush_range(addr, size)
  • cache_{data,instr}_invd_all()
  • cache_{data,instr}_invd_range(addr, size)
  • cache_{data,instr}_flush_and_invd_all(addr, size)
  • cache_{data,instr}_flush_and_invd_range(addr, size)

And functions can now be inlined (see ARM64 implementation).

This is still an RFC.

IMPORTANT

Before this PR is considered to me merge-able we need to address two issues:

  • bisectability (easily solved by squashing all the commit together)
  • API breakage (but I honestly doubt there is any out-of-tree user of this API)

@carlocaione carlocaione force-pushed the cache_rework branch 3 times, most recently from ff39b71 to 30a9b78 Compare October 5, 2022 17:08
@carlocaione carlocaione closed this Oct 5, 2022
@carlocaione carlocaione reopened this Oct 5, 2022
@zephyrbot zephyrbot added manifest west manifest-libmetal DNM This PR should not be merged (Do Not Merge) labels Oct 5, 2022
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 5, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
libmetal zephyrproject-rtos/libmetal@2f586b4 zephyrproject-rtos/libmetal@efa2ace (master) zephyrproject-rtos/libmetal@2f586b4f..efa2ace6
open-amp zephyrproject-rtos/open-amp@8d53544 zephyrproject-rtos/open-amp@aedcc26 (master) zephyrproject-rtos/open-amp@8d535448..aedcc262

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@carlocaione carlocaione force-pushed the cache_rework branch 5 times, most recently from 749b841 to 9faa76b Compare October 12, 2022 12:35
@carlocaione carlocaione added the RFC Request For Comments: want input from the community label Oct 12, 2022
@carlocaione carlocaione self-assigned this Oct 12, 2022
@carlocaione carlocaione marked this pull request as ready for review October 12, 2022 13:08
@teburd
Copy link
Collaborator

teburd commented Nov 28, 2022

Why DNM?

@carlocaione
Copy link
Collaborator Author

Why DNM?

At least zephyrproject-rtos/libmetal#17 needs to go in first.

The cache operations must be quick, optimized and possibly inlined. The
current API is clunky, functions are not inlined and passing parameters
around that are basically always known at compile time.

In this patch we rework the cache functions to allow us to get rid of
useless parameters and make inlining easier.

In particular this changeset is doing three things:

1. `CONFIG_HAS_ARCH_CACHE` is now `CONFIG_ARCH_CACHE` and
   `CONFIG_HAS_EXTERNAL_CACHE` is now `CONFIG_EXTERNAL_CACHE`

2. The cache API has been reworked.

3. Comments are added.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
This is useful to prove that the implementation of the API is done
correctly.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
And use the new API.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
And use the new API.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
And use the new API.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
To be compliant to the new cache API.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Fix the usage to be compliant to the new cache API.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Make libmetal using the new zephyr cache APIs.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
Bump the openamp version to v2022.10.0

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Nov 28, 2022
@nashif nashif merged commit 1258d35 into zephyrproject-rtos:main Dec 1, 2022
@Thalley
Copy link
Collaborator

Thalley commented Dec 2, 2022

Just curious: Why was the openamp version updated as part of this PR (from 1258d35)?

@carlocaione carlocaione deleted the cache_rework branch December 3, 2022 15:26
@carlocaione
Copy link
Collaborator Author

Just curious: Why was the openamp version updated as part of this PR (from 1258d35)?

It was requested by the OpenAMP maintainer. See #50988 (review)

@@ -804,7 +804,7 @@ static void enable_mmu_el1(struct arm_mmu_ptables *ptables, unsigned int flags)
isb();

/* Invalidate all data caches before enable them */
sys_cache_data_all(K_CACHE_INVD);
Copy link
Member

Choose a reason for hiding this comment

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

Just want to note that we are affected by this change while working on internal firmware upgrade from v2.x, hopefully we are the only out-of-tree user..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARC ARC Architecture area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Base OS Base OS Library (lib/os) area: CAN area: Ethernet area: IPC Inter-Process Communication area: Kernel area: West West utility area: X86 x86 Architecture (32-bit) manifest manifest-libmetal manifest-open-amp RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.