drivers: cache: add Andes cache driver

[wtlee1995]

Added cache driver for Andes V5 series SOC and tested on adp_xc7k_ae350 board.

Signed-off-by: Wei-Tai Lee wtlee@andestech.com

[cfriedt]

It might make more sense to mark this as deprecated instead of immediately removing it, even if it does nothing.

So remove the default line, add select DEPRECATED.

That should allow for downstream users to use the same Kconfig between adjacent Zephyr releases, and they should get a warning in CI about using deprecated options.

[fkokosinski]

+1 on marking this as deprecated

[wtlee1995]

Thanks! I'll add select DEPRECATED.

[ycsin]

This doesn't look very 'driver', but should be possible to make it so by adding a binding file for andestech,l2c & use proper devicetree macros for driver and DT_INST_FOREACH_STATUS_OKAY instead of SYS_INIT

[ycsin]

This will resolve to true regardless of the result in L29, should probably be

Suggested change

	#ifdef L2C_EXISTS
	#if IS_ENABLED(L2C_EXISTS)

[wtlee1995]

Thanks for catching it up! DT_NODE_HAS_COMPAT_STATUS would return 0 or 1, so I think #if L2C_EXISTS would be enough?

[ycsin]

#if L2C_EXISTS would be enough?

yeah that will do as well

[ycsin]

this means the node must be named as l2_cache in the devicetree

[wtlee1995]

Thanks for the suggestions!

1. I'll add the binding file, and check L2 cache DTS node by compatible

2. I'm not sure about the issues in DT_INST_FOREACH_STATUS_OKAY and SYS_INIT.
   There are two macros: SYS_INIT and DEVICE_DT_INST_DEFINE for setting drivers up for boot time initialization.
   DEVICE_DT_INST_DEFINE which will create a driver object isn't needed for the cache driver.

   DT_INST_FOREACH_STATUS_OKAY is used in the case that there are multiple nodes. However, in the current
   implementation of the Andes cache driver, the focus is solely on a single L2 cache node.

[ycsin]

2. DT_INST_FOREACH_STATUS_OKAY is used in the case that there are multiple nodes.

Sorry that was a typo - should be DEVICE_DT_INST_DEFINE / DEVICE_DT_DEFINE instead

However, in the current implementation of the Andes cache driver, the focus is solely on a single L2 cache node.

Yeah they both work as fine for a single instance device driver like this without a device API, it's just that the DT_ variant is designed for device drivers and opens to more possibilities in the future. It's nice to refactor this driver a bit at the very beginning to make use of the device driver macros and make these

static struct cache_config cache_cfg; /* this can go to the 'config' of the device */
static struct k_spinlock lock; /* this can go to the 'data' of the device */

instance based so that the driver can be made multi-instance very easily in the future.

But these are all just suggestions.

[wtlee1995]

We have explored the concept of a multi-instance-based driver, but its viability is challenged when confronted with the introduction of a multi-level cache system. Simply parsing the L2 cache node is insufficient to address this complexity.

In a multi-level cache setup, there could be a scenario where each core has its private L2 cache, or two cores share an L2 cache, or with an additional L3 cache. This introduces a set of challenges: determining which level to treat as instances and, when performing cache operations, identifying the appropriate next-level cache. If we limit our focus to parsing the L2 cache exclusively, we not only overlook scenarios involving multiple L2 cache nodes but also make the code less adaptable for future refactoring.

[ycsin]

it would probably be much easier to read if the l2 cache related logics are factorized into functions and moved into a separate file of its own, then make use of CMakeLists.txt to compile it if enabled in the devicetree, rather than having 9 chunks of L2C_EXISTS macro guards scattered throughout this cache driver which involved syscon(+ andestech,atcsmu100), l1_cache & l2_cache

[ycsin]

I'm not experienced in the workings of cache, so take these comments with a pinch of salt:

In a multi-level cache setup, there could be a scenario where each core has its private L2 cache, or two cores share an L2 cache, or with an additional L3 cache. This introduces a set of challenges: determining which level to treat as instances and, when performing cache operations, identifying the appropriate next-level cache.

Some of the concerns above sound like a devicetree problem to me, is it possible to allocate these little pools of memory in the devicetree, perhaps with another device binding if necessary?

If we limit our focus to parsing the L2 cache exclusively, we not only overlook scenarios involving multiple L2 cache nodes but also make the code less adaptable for future refactoring.

I'm actually suggesting that things can be split-off from the cache_andes.c like little LEGOs (l1, l2, ..., ln) so that they can have different combinations, which hopefully makes the code more adaptable.

not sure whether it is acceptable regarding having multiple C files for a single driver.

The current cache_andes.c works like a cache manager that refers to multiple nodes in the devicetree that collectively implements the cache functions in include/zephyr/drivers/cache.h.

With that I think perhaps it makes sense to have multiple files (drivers?) that serve as backends and register itself with the main cache_andes.c driver (i.e. in an address lookup table), so that the main driver knows which cache device it should route the data to/from.

[wtlee1995]

Some of the concerns above sound like a devicetree problem to me, is it possible to allocate these little pools of memory in the devicetree, perhaps with another device binding if necessary?

Yes, these concerns can be solved by adding some device properties, just like 'next-level-cache' mentioned in devicetree-specification-v0.4. But in our plan, we'll add these features when there is a real hardware requirement.

I'm actually suggesting that things can be split-off from the cache_andes.c like little LEGOs (l1, l2, ..., ln) so that they can have different combinations, which hopefully makes the code more adaptable.

Agree with the concept of making the source code of each cache level more adaptable.

The current cache_andes.c works like a cache manager that refers to multiple nodes in the devicetree that collectively implements the cache functions in include/zephyr/drivers/cache.h.

With that I think perhaps it makes sense to have multiple files (drivers?) that serve as backends and register itself with the main cache_andes.c driver (i.e. in an address lookup table), so that the main driver knows which cache device it should route the data to/from.

If it is legal to have multiple files(drivers), we've already had another solution. In this way, we think we can append another cache level more easily in the future.

//in L1_cache.c
int cache_data_invd_all(void) {
  nds_l1d_cache_all(INVD);      
  if (L2 cache exists)
      l2_cache_all(INVD);
}

// in L2_cache.c
int l2_cache_all(INVD) {
   nds_l2_cache_all(INVD)
   if (L3 cache exists)
      nds_l3_cache_all(INVD);
}

[wtlee1995]

The twister-build error is a Kconfig loop dependency. It happened when I added a Kconfig dependency between SYSCON and CACHE_ANDES, but there is no real loop dependency in the Andes platform.

Is there anyone who can address the issue, or should I file an issue to seek help?

[ycsin]

The twister-build error is a Kconfig loop dependency. It happened when I added a Kconfig dependency between SYSCON and CACHE_ANDES, but there is no real loop dependency in the Andes platform.

Is there anyone who can address the issue, or should I file an issue to seek help?

looks like the build system 'saw' the SYSCON in the neorv32 SOC:

zephyr/soc/riscv/neorv32/Kconfig.defconfig

Lines 18 to 20 in 1970004

	config SYSCON
	default y

before the real one in the SYSCON driver:

zephyr/drivers/syscon/Kconfig

Lines 9 to 18 in 1970004

	menuconfig SYSCON
	bool "System Controller (SYSCON) drivers"
	help
	SYSCON (System Controller) drivers. System controller node represents
	a register region containing a set of miscellaneous registers. The
	registers are not cohesive enough to represent as any specific type
	of device. The typical use-case is for some other node's driver, or
	platform-specific code, to acquire a reference to the syscon node and
	extract information from there.

no idea why it does that though, cc @carlocaione

[carlocaione]

Can you move the setting of CONFIG_SYSCON from zephyr/soc/riscv/neorv32/Kconfig.defconfig to the _defconfig file as done for the other boards?

[wtlee1995]

Hi @carlocaione, there are still errors even when I move the settings of CONFIG_SYSCON in all of the soc to the _defconfig files.

[carlocaione]

Hi @carlocaione, there are still errors even when I move the settings of CONFIG_SYSCON in all of the soc to the _defconfig files.

Yeah, no idea. This needs to be debugged.

[wtlee1995]

The Kconfig dependency errors happen when the dependency between SYSCON and CACHE is established. We fix the errors by removing the imply syscon in Kconfig.andes and verify the presence of SYSCON directly within the driver source.

[carlocaione]

Shouldn't we have here also a dependency on DT_HAS_... ?

[wtlee1995]

In the andes cache driver, syscon driver is only needed when the L2 cache exists.

For DCACHE_LINE_SIZE_DETECT, there are already dependencies:
DCACHE_LINE_SIZE_DETECT --> DCACHE --> CPU_HAS_DCACHE (depends on -->)

[cfriedt]

@wtlee1995 - the dependencies are good. Actually both are fine. What Carlo is describing is a relatively new (>3.x.x) facility that reduces the number of "switches" required to enable a driver.

Prior to DT_HAS_FOO (an "automatic" Kconfig), one needed to set status = "okay" and also set CONFIG_FOO=y. Now it's only necessary to enable the node in devictree (approximately). So 1 "switch" instead of 2.

[wtlee1995]

I understand the benefit of using DT_HAS_... which can reduce the number of "switches" to enable the driver.

There is no specified cache DTS node, but rather the Kconfig option CPU_HAS_DCACHE. And we think there is already a hidden dependency between CACHE_ANDES and CPU_HAS_DCACHE, so we don't need to add another dependency in the driver.

As for SYSCON and DCACHE_LINE_SIZE_DETECT, they are not mandatory options to enable the driver; they are merely enhanced features. Additionally, the Andes cache driver targets for the platform with either (a) only L1 cache or (b) both L1 and L2 cache. Regardless of whether it's (a) or (b), we want to enable DCACHE_LINE_SIZE_DETECT by default. And we want to enable SYSON only in (b).

stale

[wtlee1995]

Change SOC_SERIES_ANDES_AE350 to SOC_FAMILY_ANDES_V5 in Kconfig.andes and fix some typos.