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

extend cache interface to support xtensa coherency architecture #68140

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

nashif
Copy link
Member

@nashif nashif commented Jan 25, 2024

This move custom cache calls implemented via soc APIs to a generic interface that can be used to abstract the cache calls.
Eliminate more use of z_soc_* call

  • cache: introduce coherent cache interface
  • xtensa: move to use system cache API support for coherency

Copy link
Collaborator

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

nits

arch/Kconfig Outdated
depends on CPU_HAS_CACHE_COHERENCY
default y
help
Cache coherency
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a more verbose help?

Comment on lines 339 to 343
bool arch_cache_is_ptr_cached(void *ptr);

#define cache_is_ptr_cached(ptr) arch_cache_is_ptr_cached(ptr)


bool arch_cache_is_ptr_uncached(void *ptr);

#define cache_is_ptr_uncached(ptr) arch_cache_is_ptr_uncached(ptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the space between bool and define (like it is done in the following lines).

@nashif nashif force-pushed the topic/cache/coherency1 branch 2 times, most recently from fd6269e to 354ccdd Compare January 26, 2024 13:21
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 26, 2024

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

Name Old Revision New Revision Diff
sof zephyrproject-rtos/sof@7a0ff76 (zephyr_wip) zephyrproject-rtos/sof@0606152 (zephyr) zephyrproject-rtos/[email protected]

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

lgirdwood
lgirdwood previously approved these changes Jan 26, 2024
Copy link
Collaborator

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

LGTM.

dcpleung
dcpleung previously approved these changes Jan 31, 2024
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

HW test results don't look good in thesofproject/sof#8824 but as stated before, they don't look good already in the first place so it's practically impossible to tell whether this PR makes them better or worse or no difference.

cfriedt
cfriedt previously approved these changes Feb 1, 2024
carlocaione
carlocaione previously approved these changes Feb 1, 2024
@nashif
Copy link
Member Author

nashif commented Feb 1, 2024

so it's practically impossible to tell whether this PR makes them better or worse or no difference.

This PR is not supposed to fix or deal with existing issues in SOF. This just adds a level of abstraction and removes z_soc_ and arch_xtensa_ usages in SOF which has the goal of running on architectures other than xtensa.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

so it's practically impossible to tell whether this PR makes them [the tests] better or worse or no difference.

This PR is not supposed to fix or deal with existing issues in SOF.

I meant "worse or no difference", sorry.

This just adds a level of abstraction and removes z_soc_ and arch_xtensa_ usages in SOF which ...

I understand. Testing the SOF main branch is unfortunately not in a position to prove that and the lack of any regression yet. SOF validation status: unknown.

On the other hand, I triple checked the adress warnings from sparse in cache.h and they do come from this PR.

@nashif
Copy link
Member Author

nashif commented Feb 2, 2024

On the other hand, I triple checked the adress warnings from sparse in cache.h and they do come from this PR.

yes, fedora seems to have an old version of sparse, installed from git now and I am getting the same thing.

@nashif nashif dismissed stale reviews from carlocaione, cfriedt, and dcpleung via 06b14ab February 2, 2024 01:19
@nashif nashif force-pushed the topic/cache/coherency1 branch from 3284d86 to 06b14ab Compare February 2, 2024 01:19
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 2, 2024

yes, fedora seems to have an old version of sparse, installed from git now and I am getting the same thing.

Mystery solved!

We use https://github.com/thesofproject/sparse which I updated recently and is not far from https://git.kernel.org/pub/scm/devel/sparse/sparse.git/log/. There's an extremely recent fix for an infinite loop.

I can update https://github.com/thesofproject/sparse if needed.

@nashif nashif force-pushed the topic/cache/coherency1 branch 2 times, most recently from 597e6d3 to 9e1fbcd Compare February 2, 2024 02:12
@nashif nashif requested review from marc-hb and dcpleung February 2, 2024 02:53
@marc-hb marc-hb dismissed their stale review February 2, 2024 03:58

sparse regressions are fixed in 9e1fbcdee941

{
#if defined(CONFIG_CACHE_MANAGEMENT) && defined(CONFIG_CACHE_DOUBLEMAP)
return cache_is_ptr_cached(ptr);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be an #else? With the #if above being true we're getting unreachable code. Same everywhere below. One could also use if (IS_ENABLED(CONFIG_...)) instead

+ CONFIG_IPM_CAVS_HOST_OUTBOX_OFFSET));
uint32_t *buf = (uint32_t *)sys_cache_uncached_ptr_get(
(void *)((uint32_t)mw0_config->mem_base
+ CONFIG_IPM_CAVS_HOST_OUTBOX_OFFSET));
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't sparse warn about this? Is this file compiled? The argument of sys_cache_uncached_ptr_get() should be a cached pointer, so the type-case should be (void __sparse_cache *) there (in line 54 in this case instead of (void *)

@@ -108,7 +110,7 @@ static bool ipc_handler(const struct device *dev, void *arg,
return -ENODEV;
}
const struct mem_win_config *mw1_config = mw1->config;
uint32_t *msg = arch_xtensa_uncached_ptr((void *)mw1_config->mem_base);
uint32_t *msg = sys_cache_uncached_ptr_get((void *)mw1_config->mem_base);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

"stack memory not cached");
zassert_not_equal(&tag, arch_xtensa_uncached_ptr((void *)&tag),
zassert_not_equal(&tag, sys_cache_uncached_ptr_get((void *)&tag),
Copy link
Collaborator

Choose a reason for hiding this comment

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

argument should be __sparse_cache

"stack memory not cached");
zassert_equal(&static_tag, arch_xtensa_uncached_ptr((void *)&static_tag),
zassert_equal(&static_tag, sys_cache_uncached_ptr_get((void *)&static_tag),
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

volatile int *ctag = (volatile int *)arch_xtensa_cached_ptr((void *)&tag);
volatile int *utag = (volatile int *)arch_xtensa_uncached_ptr((void *)&tag);
volatile int *ctag = (volatile int *)sys_cache_cached_ptr_get((void *)&tag);
volatile int *utag = (volatile int *)sys_cache_uncached_ptr_get((void *)&tag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

all "uncached get" have a cached pointer as their argument, so the latter one should have (void __sparce_cache *) in the argument conversion

Introduce a set of cache APIs used on architectures with cache incoherency.

Signed-off-by: Anas Nashif <[email protected]>
Remove custom implementation and use system cache interface instead.

Signed-off-by: Anas Nashif <[email protected]>
point to d40682678b7378fb930ca5535ccd2ce667ed5c27.

Signed-off-by: Anas Nashif <[email protected]>
@nashif nashif force-pushed the topic/cache/coherency1 branch from 9e1fbcd to 759ff94 Compare February 2, 2024 11:27
@nashif nashif added this to the v3.6.0 milestone Feb 2, 2024
@nashif nashif merged commit f6adaf5 into zephyrproject-rtos:main Feb 3, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants