Cleanup cache initialization

[benediktibk]

Data and instruction cache initialization is currently a little bit messy. This PR aims at unifying the available config options and providing a more consistent approach on cache activation.

This is a follow up to #66443.

Open is to check where the caches were automatically enabled previously (SystemInit, Hal, ...) and disabling them directly after.

• soc_rt10xx.c
• soc_rt11xx.c
• kv5x/soc.c
• same70/soc.c
• samv71/soc.c

[carlocaione]

This is fixing the wrong way a pre-existing issue.

The issue is that the cache APIs must be used and not rely on architecture-specific operations like SCB_*. The guards are already included in the APIs, see https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/cache.h

[carlocaione]

The problem here is that the architecture must not use arch_dcache_enable directly, but instead they have to rely on sys_cache_data_enable.

sys_cache_data_enable collapse to a NOP when !CONFIG_CACHE_MANAGEMENT or !CONFIG_DCACHE.

[benediktibk]

fine for me, will change it

[pillo79]

Forgive me if I'm wrong, but that would never turn on caches unless both of these settings were enabled, and it makes sense to be able to use the CPU cache but not the Cache API interface.
If using arch_ functions is wrong in this context, perhaps the suggestion should be to use a guarded cache_data_enable() instead?

[carlocaione]

Forgive me if I'm wrong, but that would never turn on caches unless both of these settings were enabled

Correct, and that is explained in https://docs.zephyrproject.org/latest/hardware/cache/index.html indeed.

CONFIG_DCACHE is telling you that a DCACHE is present but whether that is actually used or not it is up to CONFIG_CACHE_MANAGEMENT option.

it makes sense to be able to use the CPU cache but not the Cache API interface.

Does it though? Before the cache API was introduced every single architecture was doing something different and custom, the cache API was introduced exactly to put order in the cache management chaos. So the path forward here is just to move to the new cache API, not adding workarounds.

[benediktibk]

I agree with @pillo79, CONFIG_CACHE_MANAGEMENT might be disabled manually, but still CONFIG_DCACHE enabled. In such a case I would expect not to be able to enable or disable the caches from the application, but still the data cache to be active.

static ALWAYS_INLINE void sys_cache_data_enable(void)
{
#if defined(CONFIG_CACHE_MANAGEMENT) && defined(CONFIG_DCACHE)
    cache_data_enable();
#endif
}

I'm rather thinking towards calling cache_data_enable, with IS_ENABLED(CONFIG_DCACHE) around it.

[carlocaione]

when the cache API was introduced we could not move all the cache implementation over right away, it was just too much work. So where possible we simply used CONFIG_DCACHE around the legacy cache operation code and we added CONFIG_DCACHE to be set at SoC / platform level to say: "the SoC has a functional DCACHE that is being used by some code".

For legacy code we moved from:

  custom_cache_op();

to:

#ifdef CONFIG_DCACHE
  custom_cache_op();
#endif

So, a quick recap is:

• CONFIG_CPU_HAS_DCACHE: the CPU has a DCACHE, it can be used or not
• CONFIG_DCACHE: the CPU has a DCACHE that is being used somehow (this was done to support legacy cache operations)
• CONFIG_CACHE_MANAGEMENT: the CPU has a DCACHE that is being used using the cache API

So, back to this PR. Is this PR correct? It is, but it is simply fixing a legacy usage that it should (and it is) deprecated.

[benediktibk]

Thanks for the clarification, but I think one version in between is missing.

• CONFIG_CPU_HAS_DCACHE: the CPU has a DCACHE, it can be used or not
• CONFIG_DCACHE: the CPU has a DCACHE that is being used somehow (this was done to support legacy cache operations)
• ???: the CPU as DCACHE which is active, but it is not accessible using the cache API
• CONFIG_CACHE_MANAGEMENT: the CPU has a DCACHE that is being used using the cache API

I will in the meantime go ahead with moving forward to sys_cache_data_enable as CONFIG_DCACHE seems to be somewhat deprecated.

[carlocaione]

the CPU as DCACHE which is active, but it is not accessible using the cache API

Well, that would be CONFIG_DCACHE=y and CONFIG_CACHE_MANAGEMENT=n.

That is: you have something managing the DCACHE using some kind of custom API that it is not the zephyr cache API. In this case your code should use the custom / HAL API to manage the cache and now you see why we needed a formal, shared API for that, because we needed to abstract the cache operations away from the actual implementation.

[benediktibk]

But with CONFIG_DCACHE=y and CONFIG_CACHE_MANAGEMENT=n sys_cache_data_enable will turn into NOP. Therefore I won't be able to use the cache management API.
I totally agree with you that having an abstract cache operations API is a good thing to have. I'm just saying that having this API available doesn't necessarily mean that one wants to automatically enable the cache. These are two separate things for me.

[carlocaione]

But with CONFIG_DCACHE=y and CONFIG_CACHE_MANAGEMENT=n sys_cache_data_enable will turn into NOP

Yes, exactly.

You can have CONFIG_DCACHE=y and CONFIG_CACHE_MANAGEMENT=n in two cases:

1. Your SoC has sys_cache_data_enabled implemented but you do not want to use it, this is why you are setting CONFIG_CACHE_MANAGEMENT=n, so sys_cache_data_enable() is returning -ENOTSUP.
2. Your SoC doesn't have sys_cache_data_enabled implemented, but to enable the DCACHE it is relying on some custom API, so sys_cache_data_enable() is returning -ENOTSUP because the API that you want to use is different (custom).

[nandojve]

Hi @benediktibk ,

In the case of SAM only:

The initial discussion around SAM cache started here #60425. Later the #61005 solve the issues and let user setup cache as it pleases. The default value is cache disabled and the below examples are for SAMV71 which have both I/D Cache available. User should add below lines on their own board.

I Cache only:
CONFIG_CACHE_MANAGEMENT=y
CONFIG_DCACHE=n

D Cache only:
CONFIG_CACHE_MANAGEMENT=y
CONFIG_ICACHE=n

I/D Cache disabled:
CONFIG_ICACHE=n
CONFIG_DCACHE=n

I/D Cache Enabled:
CONFIG_CACHE_MANAGEMENT=y

IMHO, cache should be user defined since user may want to use ITCM/DTCM for other purposes. So, I'm interested in know how this PR changes the above behavior?

[benediktibk]

Hi @benediktibk ,

In the case of SAM only:

The initial discussion around SAM cache started here #60425. Later the #61005 solve the issues and let user setup cache as it pleases. The default value is cache disabled and the below examples are for SAMV71 which have both I/D Cache available. User should add below lines on their own board.

I Cache only: CONFIG_CACHE_MANAGEMENT=y CONFIG_DCACHE=n

D Cache only: CONFIG_CACHE_MANAGEMENT=y CONFIG_ICACHE=n

I/D Cache disabled: CONFIG_ICACHE=n CONFIG_DCACHE=n

I/D Cache Enabled: CONFIG_CACHE_MANAGEMENT=y

IMHO, cache should be user defined since user may want to use ITCM/DTCM for other purposes. So, I'm interested in know how this PR changes the above behavior?

This seems to contradict what I got told in this PR earlier on by @carlocaione. If I understood it correctly SoCs should use in their init code sys_cache_data/instr_enable to enable the caches. Therefore, configuring CONFIG_CACHE_MANAGEMENT=n will result in disabled caches in anyway, and CONFIG_DCACHE and CONFIG_ICACHE will only indicate if the SoC has a cache or not.

Btw, I saw that in the of the PRs you explictly added a disable-call for the caches during initialization. Are the caches enabled using something else earlier on for these SoCs?

[carlocaione]

This seems to contradict what I got told in this PR earlier on by @carlocaione. If I understood it correctly SoCs should use in their init code sys_cache_data/instr_enable to enable the caches. Therefore, configuring CONFIG_CACHE_MANAGEMENT=n will result in disabled caches in anyway, and CONFIG_DCACHE and CONFIG_ICACHE will only indicate if the SoC has a cache or not.

yes, this is correct. @nandojve you should never toggle CONFIG_DCACHE yourself if you are using the Zephyr cache APIs.

I'll try to better explain what is the rationale here (when using the proper cache APIs):

1. All the zephyr code should use the sys_cache_* API to do cache operations so that the code is agnostic about the actual hardware it is running on.
2. The only switch that the user could use to disable / enable the cache operations is CONFIG_CACHE_MANAGEMENT and nothing else. If the hardware does not support cache ops that option is doing nothing from the beginning (see following point), otherwise we are simply converting all the cache operations to -ENOTSUP when setting that to =n.
3. At SoC level the SoC is defining CONFIG_{D,I}CACHE whether we support cache operations at SoC level. If the SoC does not support caches, it simply do not define those and the sys_cache_* ops are falling to -ENOTSUP.

[benediktibk]

1. All the zephyr code should use the sys_cache_* API to do cache operations so that the code is agnostic about the actual hardware it is running on.

One small caveat: If external code (for instance HAL) is used for the initalization, which automatically activates caches, the caches have to be deactivated directly after such a call using a low level function call, not sys_cache_. Then during the SoC initialization later on sys_cache_ can be used to activate the caches.

@carlocaione Correct?

[benediktibk]

@carlocaione There are, of now, some spots where I have to disable caches, although CONFIG_CACHE_MANAGEMENT might be disabled. I decided to use arch_dcache_disable for this, as I did not want to spread calls to SCB_DisableDCache all over the place. Unfortunately, these functions are only built and linked if CONFIG_CACHE_MANAGEMENT is enabled. To work around this I introduced CONFIG_ANY_CACHE, which is basically the same as CONFIG_CACHE_MANAGEMENT but without a prompt.

Which version would you prefer, CONFIG_ANY_CACHE or calling SCB_DisableDCache directly? Or something completely different?

The problematic spots are:

• arch/arm/core/cortex_m/scb.c
• soc/arm/atmel_sam/same70/soc.c
• soc/arm/atmel_sam/samv71/soc.c

[nandojve]

atmel_sam

[carlocaione]

Which version would you prefer, CONFIG_ANY_CACHE or calling SCB_DisableDCache directly? Or something completely different?

I'm honestly really lost about what is the problem here and why only ARM is so messed up in this regard while the other architectures are implementing this nicely (@microbuilder). That said I really want to avoid adding one more cache related Kconfig symbol, so if this could not be solved cleanly just use SCB_DisableDCache directly.

[benediktibk]

Removed ANY_CACHE again and used direct calls to SCB_* wherever it was necessary.

[benediktibk]

Removed ANY_CACHE again and used direct calls to SCB_* wherever it was necessary.

[carlocaione]

Way better IMO

[benediktibk]

Rebased to trigger CI as the github incident has been resolved.