[RFC] openrisc: Add cacheinfo support

[RFC] openrisc: Add cacheinfo support

[Sahil Siddiq]

Add cacheinfo support for OpenRISC.

Currently, a few CPU cache attributes pertaining to OpenRISC processors
are exposed along with other unrelated CPU attributes in the procfs file
system (/proc/cpuinfo). However, a few cache attributes remain unexposed.
An attempt is also made to pull these CPU cache attributes without
detecting if the relevant cache exists.

This patch provides a mechanism that the generic cacheinfo infrastructure
can employ to expose these attributes via the sysfs file system. These
attributes are exposed in /sys/devices/system/cpu/cpuX/cache/indexN.
Cache attributes are pulled only when the cache component is detected.

The implementation to pull cache attributes from the processor's
registers has been moved from arch/openrisc/kernel/setup.c with a few
modifications.

The patch also moves cache-related fields out of struct cpuinfo_or1k and
into its own struct to keep the implementation straightforward. This
reduces duplication of cache-related fields while keeping cpuinfo_or1k
extensible in case more cache descriptors are added in the future.

This implementation is based on similar work done for MIPS and LoongArch.

Signed-off-by: Sahil Siddiq <sahilcdq@proton.me>
---
Before applying these changes, there is no "cache" directory in
/sys/devices/system/cpu/cpu0. After applying these changes, the
cache directory is present and has the following hierarchy.

~ # tree /sys/devices/system/cpu/cpu0/cache/
/sys/devices/system/cpu/cpu0/cache/
├── index0
│   ├── coherency_line_size
│   ├── level
│   ├── number_of_sets
│   ├── shared_cpu_list
│   ├── shared_cpu_map
│   ├── size
│   ├── type
│   ├── uevent
│   ├── ways_of_associativity
│   └── write_policy
├── index1
│   ├── coherency_line_size
│   ├── level
│   ├── number_of_sets
│   ├── shared_cpu_list
│   ├── shared_cpu_map
│   ├── size
│   ├── type
│   ├── uevent
│   └── ways_of_associativity
└── uevent

The newly implemented "init_cache_level()" pulls the cache details by
reading the processor's special registers as it was being done in
setup.c.

None of the functions in drivers/base/cacheinfo.c that are capable of
pulling these details (e.g.: cache_size) have been used. This is because
they pull these details by reading properties present in the device tree
file. In setup.c, for example, the value of "clock-frequency" is pulled
from the device tree file.

Cache related properties are currently not present in OpenRISC's device
tree files.

Regarding the "shared_cpu_map" cache attribute, I wasn't able to find
anything in the OpenRISC architecture manual to indicate that processors
in a multi-processor system may share the same cache component. MIPS uses
"globalnumber" to detect siblings. LoongArch uses a "CACHE_PRIVATE" flag
to detect siblings sharing the same cache.

I am running with the assumption that every OpenRISC core has its own
icache and dcache. Given that OpenRISC does not support a multi-level
cache architecture and that icache and dcache are like L1 caches, I
think this assumption is reasonable. What are your thoughts on this?

Another issue I noticed is that the unit used in ...cache/indexN/size
is KB. The actual value of the size is right-shifted by 10 before being
reported. When testing these changes using QEMU (and without making any
modifications to the values stored in DCCFGR and ICCFGR), the cache size
is far smaller than 1KB. Consequently, this is reported as 0K. For cache
sizes smaller than 1KB, should something be done to report it in another
unit? Reporting 0K seems a little misleading.

Thanks,
Sahil

 arch/openrisc/include/asm/cpuinfo.h |  16 +++--
 arch/openrisc/kernel/Makefile       |   2 +-
 arch/openrisc/kernel/cacheinfo.c    | 104 ++++++++++++++++++++++++++++
 arch/openrisc/kernel/dma.c          |   6 +-
 arch/openrisc/kernel/setup.c        |  45 ------------
 5 files changed, 117 insertions(+), 56 deletions(-)
 create mode 100644 arch/openrisc/kernel/cacheinfo.c

diff --git a/arch/openrisc/include/asm/cpuinfo.h b/arch/openrisc/include/asm/cpuinfo.h
index 5e4744153d0e..82f5d4c06314 100644
--- a/arch/openrisc/include/asm/cpuinfo.h
+++ b/arch/openrisc/include/asm/cpuinfo.h
@@ -15,16 +15,18 @@
 #ifndef __ASM_OPENRISC_CPUINFO_H
 #define __ASM_OPENRISC_CPUINFO_H
 
+struct cache_desc {
+	u32 size;
+	u32 sets;
+	u32 block_size;
+	u32 ways;
+};
+
 struct cpuinfo_or1k {
 	u32 clock_frequency;
 
-	u32 icache_size;
-	u32 icache_block_size;
-	u32 icache_ways;
-
-	u32 dcache_size;
-	u32 dcache_block_size;
-	u32 dcache_ways;
+	struct cache_desc icache;
+	struct cache_desc dcache;
 
 	u16 coreid;
 };
diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
index 79129161f3e0..e4c7d9bdd598 100644
--- a/arch/openrisc/kernel/Makefile
+++ b/arch/openrisc/kernel/Makefile
@@ -7,7 +7,7 @@ extra-y	:= vmlinux.lds
 
 obj-y	:= head.o setup.o or32_ksyms.o process.o dma.o \
 	   traps.o time.o irq.o entry.o ptrace.o signal.o \
-	   sys_call_table.o unwinder.o
+	   sys_call_table.o unwinder.o cacheinfo.o
 
 obj-$(CONFIG_SMP)		+= smp.o sync-timer.o
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
diff --git a/arch/openrisc/kernel/cacheinfo.c b/arch/openrisc/kernel/cacheinfo.c
new file mode 100644
index 000000000000..515969896c20
--- /dev/null
+++ b/arch/openrisc/kernel/cacheinfo.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * OpenRISC cacheinfo support
+ *
+ * Based on work done for MIPS and LoongArch. All original copyrights
+ * apply as per the original source declaration.
+ *
+ * OpenRISC implementation:
+ * Copyright (C) 2025 Sahil Siddiq <sahilcdq@proton.me>
+ */
+
+#include <linux/cacheinfo.h>
+#include <asm/cpuinfo.h>
+#include <asm/spr.h>
+#include <asm/spr_defs.h>
+
+static inline void ci_leaf_init(struct cacheinfo *this_leaf, enum cache_type type,
+				unsigned int level, struct cache_desc *cache, int cpu)
+{
+	this_leaf->type = type;
+	this_leaf->level = level;
+	this_leaf->coherency_line_size = cache->block_size;
+	this_leaf->number_of_sets = cache->sets;
+	this_leaf->ways_of_associativity = cache->ways;
+	this_leaf->size = cache->size;
+	cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
+}
+
+int init_cache_level(unsigned int cpu)
+{
+	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
+	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+	int leaves = 0, levels = 0;
+	unsigned long upr = mfspr(SPR_UPR);
+	unsigned long iccfgr, dccfgr;
+
+	if (!(upr & SPR_UPR_UP)) {
+		printk(KERN_INFO
+		       "-- no UPR register... unable to detect configuration\n");
+		return -ENOENT;
+	}
+
+	if (upr & SPR_UPR_DCP) {
+		dccfgr = mfspr(SPR_DCCFGR);
+		cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
+		cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
+		cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
+		cpuinfo->dcache.size =
+		    cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
+		leaves += 1;
+		printk(KERN_INFO
+		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
+		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
+		       cpuinfo->dcache.ways);
+	} else
+		printk(KERN_INFO "-- dcache disabled\n");
+
+	if (upr & SPR_UPR_ICP) {
+		iccfgr = mfspr(SPR_ICCFGR);
+		cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
+		cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
+		cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
+		cpuinfo->icache.size =
+		    cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
+		leaves += 1;
+		printk(KERN_INFO
+		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
+		       cpuinfo->icache.size, cpuinfo->icache.block_size,
+		       cpuinfo->icache.ways);
+	} else
+		printk(KERN_INFO "-- icache disabled\n");
+
+	if (!leaves)
+		return -ENOENT;
+
+	levels = 1;
+
+	this_cpu_ci->num_leaves = leaves;
+	this_cpu_ci->num_levels = levels;
+
+	return 0;
+}
+
+int populate_cache_leaves(unsigned int cpu)
+{
+	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
+	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+	struct cacheinfo *this_leaf = this_cpu_ci->info_list;
+	int level = 1;
+
+	if (cpuinfo->dcache.ways) {
+		ci_leaf_init(this_leaf, CACHE_TYPE_DATA, level, &cpuinfo->dcache, cpu);
+		this_leaf->attributes = ((mfspr(SPR_DCCFGR) & SPR_DCCFGR_CWS) >> 8) ?
+					CACHE_WRITE_BACK : CACHE_WRITE_THROUGH;
+		this_leaf++;
+	}
+
+	if (cpuinfo->icache.ways)
+		ci_leaf_init(this_leaf, CACHE_TYPE_INST, level, &cpuinfo->icache, cpu);
+
+	this_cpu_ci->cpu_map_populated = true;
+
+	return 0;
+}
diff --git a/arch/openrisc/kernel/dma.c b/arch/openrisc/kernel/dma.c
index b3edbb33b621..5864c4c7a8b8 100644
--- a/arch/openrisc/kernel/dma.c
+++ b/arch/openrisc/kernel/dma.c
@@ -36,7 +36,7 @@ page_set_nocache(pte_t *pte, unsigned long addr,
 	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
 
 	/* Flush page out of dcache */
-	for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache_block_size)
+	for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache.block_size)
 		mtspr(SPR_DCBFR, cl);
 
 	return 0;
@@ -105,13 +105,13 @@ void arch_sync_dma_for_device(phys_addr_t addr, size_t size,
 	case DMA_TO_DEVICE:
 		/* Flush the dcache for the requested range */
 		for (cl = addr; cl < addr + size;
-		     cl += cpuinfo->dcache_block_size)
+		     cl += cpuinfo->dcache.block_size)
 			mtspr(SPR_DCBFR, cl);
 		break;
 	case DMA_FROM_DEVICE:
 		/* Invalidate the dcache for the requested range */
 		for (cl = addr; cl < addr + size;
-		     cl += cpuinfo->dcache_block_size)
+		     cl += cpuinfo->dcache.block_size)
 			mtspr(SPR_DCBIR, cl);
 		break;
 	default:
diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
index be56eaafc8b9..38172c0989cf 100644
--- a/arch/openrisc/kernel/setup.c
+++ b/arch/openrisc/kernel/setup.c
@@ -107,27 +107,6 @@ static void print_cpuinfo(void)
 	printk(KERN_INFO "CPU: OpenRISC-%x (revision %d) @%d MHz\n",
 	       version, revision, cpuinfo->clock_frequency / 1000000);
 
-	if (!(upr & SPR_UPR_UP)) {
-		printk(KERN_INFO
-		       "-- no UPR register... unable to detect configuration\n");
-		return;
-	}
-
-	if (upr & SPR_UPR_DCP)
-		printk(KERN_INFO
-		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
-		       cpuinfo->dcache_size, cpuinfo->dcache_block_size,
-		       cpuinfo->dcache_ways);
-	else
-		printk(KERN_INFO "-- dcache disabled\n");
-	if (upr & SPR_UPR_ICP)
-		printk(KERN_INFO
-		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
-		       cpuinfo->icache_size, cpuinfo->icache_block_size,
-		       cpuinfo->icache_ways);
-	else
-		printk(KERN_INFO "-- icache disabled\n");
-
 	if (upr & SPR_UPR_DMP)
 		printk(KERN_INFO "-- dmmu: %4d entries, %lu way(s)\n",
 		       1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
@@ -155,8 +134,6 @@ static void print_cpuinfo(void)
 void __init setup_cpuinfo(void)
 {
 	struct device_node *cpu;
-	unsigned long iccfgr, dccfgr;
-	unsigned long cache_set_size;
 	int cpu_id = smp_processor_id();
 	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[cpu_id];
 
@@ -164,20 +141,6 @@ void __init setup_cpuinfo(void)
 	if (!cpu)
 		panic("Couldn't find CPU%d in device tree...\n", cpu_id);
 
-	iccfgr = mfspr(SPR_ICCFGR);
-	cpuinfo->icache_ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
-	cache_set_size = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
-	cpuinfo->icache_block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
-	cpuinfo->icache_size =
-	    cache_set_size * cpuinfo->icache_ways * cpuinfo->icache_block_size;
-
-	dccfgr = mfspr(SPR_DCCFGR);
-	cpuinfo->dcache_ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
-	cache_set_size = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
-	cpuinfo->dcache_block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
-	cpuinfo->dcache_size =
-	    cache_set_size * cpuinfo->dcache_ways * cpuinfo->dcache_block_size;
-
 	if (of_property_read_u32(cpu, "clock-frequency",
 				 &cpuinfo->clock_frequency)) {
 		printk(KERN_WARNING
@@ -320,14 +283,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
 		seq_printf(m, "revision\t\t: %d\n", vr & SPR_VR_REV);
 	}
 	seq_printf(m, "frequency\t\t: %ld\n", loops_per_jiffy * HZ);
-	seq_printf(m, "dcache size\t\t: %d bytes\n", cpuinfo->dcache_size);
-	seq_printf(m, "dcache block size\t: %d bytes\n",
-		   cpuinfo->dcache_block_size);
-	seq_printf(m, "dcache ways\t\t: %d\n", cpuinfo->dcache_ways);
-	seq_printf(m, "icache size\t\t: %d bytes\n", cpuinfo->icache_size);
-	seq_printf(m, "icache block size\t: %d bytes\n",
-		   cpuinfo->icache_block_size);
-	seq_printf(m, "icache ways\t\t: %d\n", cpuinfo->icache_ways);
 	seq_printf(m, "immu\t\t\t: %d entries, %lu ways\n",
 		   1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
 		   1 + (mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTW));
-- 
2.48.1

Re: [RFC] openrisc: Add cacheinfo support

[Stafford Horne]

On Tue, Mar 11, 2025 at 12:43:57AM +0530, Sahil Siddiq wrote:
> Add cacheinfo support for OpenRISC.
> 
> Currently, a few CPU cache attributes pertaining to OpenRISC processors
> are exposed along with other unrelated CPU attributes in the procfs file
> system (/proc/cpuinfo). However, a few cache attributes remain unexposed.
> An attempt is also made to pull these CPU cache attributes without
> detecting if the relevant cache exists.
> 
> This patch provides a mechanism that the generic cacheinfo infrastructure
> can employ to expose these attributes via the sysfs file system. These
> attributes are exposed in /sys/devices/system/cpu/cpuX/cache/indexN.
> Cache attributes are pulled only when the cache component is detected.
> 
> The implementation to pull cache attributes from the processor's
> registers has been moved from arch/openrisc/kernel/setup.c with a few
> modifications.
> 
> The patch also moves cache-related fields out of struct cpuinfo_or1k and
> into its own struct to keep the implementation straightforward. This
> reduces duplication of cache-related fields while keeping cpuinfo_or1k
> extensible in case more cache descriptors are added in the future.
> 
> This implementation is based on similar work done for MIPS and LoongArch.
> 
> Signed-off-by: Sahil Siddiq <sahilcdq@proton.me>
> ---
> Before applying these changes, there is no "cache" directory in
> /sys/devices/system/cpu/cpu0. After applying these changes, the
> cache directory is present and has the following hierarchy.
> 
> ~ # tree /sys/devices/system/cpu/cpu0/cache/
> /sys/devices/system/cpu/cpu0/cache/
> ├── index0
> │   ├── coherency_line_size
> │   ├── level
> │   ├── number_of_sets
> │   ├── shared_cpu_list
> │   ├── shared_cpu_map
> │   ├── size
> │   ├── type
> │   ├── uevent
> │   ├── ways_of_associativity
> │   └── write_policy
> ├── index1
> │   ├── coherency_line_size
> │   ├── level
> │   ├── number_of_sets
> │   ├── shared_cpu_list
> │   ├── shared_cpu_map
> │   ├── size
> │   ├── type
> │   ├── uevent
> │   └── ways_of_associativity
> └── uevent
> 
> The newly implemented "init_cache_level()" pulls the cache details by
> reading the processor's special registers as it was being done in
> setup.c.
> 
> None of the functions in drivers/base/cacheinfo.c that are capable of
> pulling these details (e.g.: cache_size) have been used. This is because
> they pull these details by reading properties present in the device tree
> file. In setup.c, for example, the value of "clock-frequency" is pulled
> from the device tree file.
> 
> Cache related properties are currently not present in OpenRISC's device
> tree files.

If we want to add L2 caches and define them in the device tree would
it "just work" or is more work needed?

> Regarding the "shared_cpu_map" cache attribute, I wasn't able to find
> anything in the OpenRISC architecture manual to indicate that processors
> in a multi-processor system may share the same cache component. MIPS uses
> "globalnumber" to detect siblings. LoongArch uses a "CACHE_PRIVATE" flag
> to detect siblings sharing the same cache.

In SMP environment the L1 caches are not shared they are specific to each CPU.

Also, we do not have hyperthreading in OpenRISC so shared_cpu_map should be a
1-to-1 mapping with the cpu.  Do you need to do extra work to setup that
mapping?

> I am running with the assumption that every OpenRISC core has its own
> icache and dcache. Given that OpenRISC does not support a multi-level
> cache architecture and that icache and dcache are like L1 caches, I
> think this assumption is reasonable. What are your thoughts on this?

Currently this is the case, but it could be possible to create an SoC with L2
caches.  I could imagine these would be outside of the CPU and we could define
them with the device tree.

> Another issue I noticed is that the unit used in ...cache/indexN/size
> is KB. The actual value of the size is right-shifted by 10 before being
> reported. When testing these changes using QEMU (and without making any
> modifications to the values stored in DCCFGR and ICCFGR), the cache size
> is far smaller than 1KB. Consequently, this is reported as 0K. For cache
> sizes smaller than 1KB, should something be done to report it in another
> unit? Reporting 0K seems a little misleading.

I think this is fine, as long as we pass in the correct size in bytes.

> Thanks,
> Sahil
> 
>  arch/openrisc/include/asm/cpuinfo.h |  16 +++--
>  arch/openrisc/kernel/Makefile       |   2 +-
>  arch/openrisc/kernel/cacheinfo.c    | 104 ++++++++++++++++++++++++++++
>  arch/openrisc/kernel/dma.c          |   6 +-
>  arch/openrisc/kernel/setup.c        |  45 ------------
>  5 files changed, 117 insertions(+), 56 deletions(-)
>  create mode 100644 arch/openrisc/kernel/cacheinfo.c
> 
> diff --git a/arch/openrisc/include/asm/cpuinfo.h b/arch/openrisc/include/asm/cpuinfo.h
> index 5e4744153d0e..82f5d4c06314 100644
> --- a/arch/openrisc/include/asm/cpuinfo.h
> +++ b/arch/openrisc/include/asm/cpuinfo.h
> @@ -15,16 +15,18 @@
>  #ifndef __ASM_OPENRISC_CPUINFO_H
>  #define __ASM_OPENRISC_CPUINFO_H
>  
> +struct cache_desc {
> +	u32 size;
> +	u32 sets;
> +	u32 block_size;
> +	u32 ways;
> +};
> +
>  struct cpuinfo_or1k {
>  	u32 clock_frequency;
>  
> -	u32 icache_size;
> -	u32 icache_block_size;
> -	u32 icache_ways;
> -
> -	u32 dcache_size;
> -	u32 dcache_block_size;
> -	u32 dcache_ways;
> +	struct cache_desc icache;
> +	struct cache_desc dcache;
>  
>  	u16 coreid;
>  };
> diff --git a/arch/openrisc/kernel/Makefile b/arch/openrisc/kernel/Makefile
> index 79129161f3e0..e4c7d9bdd598 100644
> --- a/arch/openrisc/kernel/Makefile
> +++ b/arch/openrisc/kernel/Makefile
> @@ -7,7 +7,7 @@ extra-y	:= vmlinux.lds
>  
>  obj-y	:= head.o setup.o or32_ksyms.o process.o dma.o \
>  	   traps.o time.o irq.o entry.o ptrace.o signal.o \
> -	   sys_call_table.o unwinder.o
> +	   sys_call_table.o unwinder.o cacheinfo.o
>  
>  obj-$(CONFIG_SMP)		+= smp.o sync-timer.o
>  obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
> diff --git a/arch/openrisc/kernel/cacheinfo.c b/arch/openrisc/kernel/cacheinfo.c
> new file mode 100644
> index 000000000000..515969896c20
> --- /dev/null
> +++ b/arch/openrisc/kernel/cacheinfo.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * OpenRISC cacheinfo support
> + *
> + * Based on work done for MIPS and LoongArch. All original copyrights
> + * apply as per the original source declaration.
> + *
> + * OpenRISC implementation:
> + * Copyright (C) 2025 Sahil Siddiq <sahilcdq@proton.me>
> + */
> +
> +#include <linux/cacheinfo.h>
> +#include <asm/cpuinfo.h>
> +#include <asm/spr.h>
> +#include <asm/spr_defs.h>
> +
> +static inline void ci_leaf_init(struct cacheinfo *this_leaf, enum cache_type type,
> +				unsigned int level, struct cache_desc *cache, int cpu)
> +{
> +	this_leaf->type = type;
> +	this_leaf->level = level;
> +	this_leaf->coherency_line_size = cache->block_size;
> +	this_leaf->number_of_sets = cache->sets;
> +	this_leaf->ways_of_associativity = cache->ways;
> +	this_leaf->size = cache->size;
> +	cpumask_set_cpu(cpu, &this_leaf->shared_cpu_map);
> +}
> +
> +int init_cache_level(unsigned int cpu)
> +{
> +	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
> +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> +	int leaves = 0, levels = 0;
> +	unsigned long upr = mfspr(SPR_UPR);
> +	unsigned long iccfgr, dccfgr;
> +
> +	if (!(upr & SPR_UPR_UP)) {
> +		printk(KERN_INFO
> +		       "-- no UPR register... unable to detect configuration\n");
> +		return -ENOENT;
> +	}
> +
> +	if (upr & SPR_UPR_DCP) {
> +		dccfgr = mfspr(SPR_DCCFGR);
> +		cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
> +		cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
> +		cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
> +		cpuinfo->dcache.size =
> +		    cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
> +		leaves += 1;
> +		printk(KERN_INFO
> +		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> +		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
> +		       cpuinfo->dcache.ways);

Can we print the number of sets here too?  Also is there a reason to pad these
int's with 4 and 2 spaces? I am not sure the padding is needed.

> +	} else
> +		printk(KERN_INFO "-- dcache disabled\n");
> +
> +	if (upr & SPR_UPR_ICP) {
> +		iccfgr = mfspr(SPR_ICCFGR);
> +		cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
> +		cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
> +		cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
> +		cpuinfo->icache.size =
> +		    cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
> +		leaves += 1;
> +		printk(KERN_INFO
> +		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> +		       cpuinfo->icache.size, cpuinfo->icache.block_size,
> +		       cpuinfo->icache.ways);

Same here.

> +	} else
> +		printk(KERN_INFO "-- icache disabled\n");
> +
> +	if (!leaves)
> +		return -ENOENT;
> +
> +	levels = 1;
> +
> +	this_cpu_ci->num_leaves = leaves;
> +	this_cpu_ci->num_levels = levels;
> +
> +	return 0;
> +}
> +
> +int populate_cache_leaves(unsigned int cpu)
> +{
> +	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
> +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> +	struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> +	int level = 1;
> +
> +	if (cpuinfo->dcache.ways) {
> +		ci_leaf_init(this_leaf, CACHE_TYPE_DATA, level, &cpuinfo->dcache, cpu);
> +		this_leaf->attributes = ((mfspr(SPR_DCCFGR) & SPR_DCCFGR_CWS) >> 8) ?
> +					CACHE_WRITE_BACK : CACHE_WRITE_THROUGH;
> +		this_leaf++;
> +	}
> +
> +	if (cpuinfo->icache.ways)
> +		ci_leaf_init(this_leaf, CACHE_TYPE_INST, level, &cpuinfo->icache, cpu);
> +
> +	this_cpu_ci->cpu_map_populated = true;
> +
> +	return 0;
> +}
> diff --git a/arch/openrisc/kernel/dma.c b/arch/openrisc/kernel/dma.c
> index b3edbb33b621..5864c4c7a8b8 100644
> --- a/arch/openrisc/kernel/dma.c
> +++ b/arch/openrisc/kernel/dma.c
> @@ -36,7 +36,7 @@ page_set_nocache(pte_t *pte, unsigned long addr,
>  	flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>  
>  	/* Flush page out of dcache */
> -	for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache_block_size)
> +	for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache.block_size)
>  		mtspr(SPR_DCBFR, cl);
>  
>  	return 0;
> @@ -105,13 +105,13 @@ void arch_sync_dma_for_device(phys_addr_t addr, size_t size,
>  	case DMA_TO_DEVICE:
>  		/* Flush the dcache for the requested range */
>  		for (cl = addr; cl < addr + size;
> -		     cl += cpuinfo->dcache_block_size)
> +		     cl += cpuinfo->dcache.block_size)
>  			mtspr(SPR_DCBFR, cl);
>  		break;
>  	case DMA_FROM_DEVICE:
>  		/* Invalidate the dcache for the requested range */
>  		for (cl = addr; cl < addr + size;
> -		     cl += cpuinfo->dcache_block_size)
> +		     cl += cpuinfo->dcache.block_size)
>  			mtspr(SPR_DCBIR, cl);
>  		break;
>  	default:
> diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c
> index be56eaafc8b9..38172c0989cf 100644
> --- a/arch/openrisc/kernel/setup.c
> +++ b/arch/openrisc/kernel/setup.c
> @@ -107,27 +107,6 @@ static void print_cpuinfo(void)
>  	printk(KERN_INFO "CPU: OpenRISC-%x (revision %d) @%d MHz\n",
>  	       version, revision, cpuinfo->clock_frequency / 1000000);
>  
> -	if (!(upr & SPR_UPR_UP)) {
> -		printk(KERN_INFO
> -		       "-- no UPR register... unable to detect configuration\n");
> -		return;
> -	}
> -
> -	if (upr & SPR_UPR_DCP)
> -		printk(KERN_INFO
> -		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> -		       cpuinfo->dcache_size, cpuinfo->dcache_block_size,
> -		       cpuinfo->dcache_ways);
> -	else
> -		printk(KERN_INFO "-- dcache disabled\n");
> -	if (upr & SPR_UPR_ICP)
> -		printk(KERN_INFO
> -		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> -		       cpuinfo->icache_size, cpuinfo->icache_block_size,
> -		       cpuinfo->icache_ways);
> -	else
> -		printk(KERN_INFO "-- icache disabled\n");
> -
>  	if (upr & SPR_UPR_DMP)
>  		printk(KERN_INFO "-- dmmu: %4d entries, %lu way(s)\n",
>  		       1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
> @@ -155,8 +134,6 @@ static void print_cpuinfo(void)
>  void __init setup_cpuinfo(void)
>  {
>  	struct device_node *cpu;
> -	unsigned long iccfgr, dccfgr;
> -	unsigned long cache_set_size;
>  	int cpu_id = smp_processor_id();
>  	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[cpu_id];
>  
> @@ -164,20 +141,6 @@ void __init setup_cpuinfo(void)
>  	if (!cpu)
>  		panic("Couldn't find CPU%d in device tree...\n", cpu_id);
>  
> -	iccfgr = mfspr(SPR_ICCFGR);
> -	cpuinfo->icache_ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
> -	cache_set_size = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
> -	cpuinfo->icache_block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
> -	cpuinfo->icache_size =
> -	    cache_set_size * cpuinfo->icache_ways * cpuinfo->icache_block_size;
> -
> -	dccfgr = mfspr(SPR_DCCFGR);
> -	cpuinfo->dcache_ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
> -	cache_set_size = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
> -	cpuinfo->dcache_block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
> -	cpuinfo->dcache_size =
> -	    cache_set_size * cpuinfo->dcache_ways * cpuinfo->dcache_block_size;
> -
>  	if (of_property_read_u32(cpu, "clock-frequency",
>  				 &cpuinfo->clock_frequency)) {
>  		printk(KERN_WARNING
> @@ -320,14 +283,6 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  		seq_printf(m, "revision\t\t: %d\n", vr & SPR_VR_REV);
>  	}
>  	seq_printf(m, "frequency\t\t: %ld\n", loops_per_jiffy * HZ);
> -	seq_printf(m, "dcache size\t\t: %d bytes\n", cpuinfo->dcache_size);
> -	seq_printf(m, "dcache block size\t: %d bytes\n",
> -		   cpuinfo->dcache_block_size);
> -	seq_printf(m, "dcache ways\t\t: %d\n", cpuinfo->dcache_ways);
> -	seq_printf(m, "icache size\t\t: %d bytes\n", cpuinfo->icache_size);
> -	seq_printf(m, "icache block size\t: %d bytes\n",
> -		   cpuinfo->icache_block_size);
> -	seq_printf(m, "icache ways\t\t: %d\n", cpuinfo->icache_ways);
>  	seq_printf(m, "immu\t\t\t: %d entries, %lu ways\n",
>  		   1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
>  		   1 + (mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTW));
> -- 
> 2.48.1
>

This pretty much looks ok to me.

Thanks,

-Stafford

Re: [RFC] openrisc: Add cacheinfo support

[Sahil Siddiq]

Hi,

On 3/14/25 2:54 AM, Stafford Horne wrote:
> On Tue, Mar 11, 2025 at 12:43:57AM +0530, Sahil Siddiq wrote:
>> Add cacheinfo support for OpenRISC.
>>
>> [...]
>> None of the functions in drivers/base/cacheinfo.c that are capable of
>> pulling these details (e.g.: cache_size) have been used. This is because
>> they pull these details by reading properties present in the device tree
>> file. In setup.c, for example, the value of "clock-frequency" is pulled
>> from the device tree file.
>>
>> Cache related properties are currently not present in OpenRISC's device
>> tree files.
> 
> If we want to add L2 caches and define them in the device tree would
> it "just work" or is more work needed?
> 

A little more work will have to be done. The implementation of "init_cache_level"
and "populate_cache_leaves" will have to be extended. To pull L2 cache attributes,
they'll need to make calls to the "of_get_property" family of functions similar to
what's being done for RISC-V and PowerPC.

Shall I resubmit this patch with those extensions? I think I'll be able to test
those changes with a modified device tree file that has an L2 cache component.

>> Regarding the "shared_cpu_map" cache attribute, I wasn't able to find
>> anything in the OpenRISC architecture manual to indicate that processors
>> in a multi-processor system may share the same cache component. MIPS uses
>> "globalnumber" to detect siblings. LoongArch uses a "CACHE_PRIVATE" flag
>> to detect siblings sharing the same cache.
> 
> In SMP environment the L1 caches are not shared they are specific to each CPU.
> 
> Also, we do not have hyperthreading in OpenRISC so shared_cpu_map should be a
> 1-to-1 mapping with the cpu.  Do you need to do extra work to setup that
> mapping?
> 

No extra work has to be done to set up the 1-to-1 mapping. This is already being
done in "ci_leaf_init()".

>> I am running with the assumption that every OpenRISC core has its own
>> icache and dcache. Given that OpenRISC does not support a multi-level
>> cache architecture and that icache and dcache are like L1 caches, I
>> think this assumption is reasonable. What are your thoughts on this?
> 
> Currently this is the case, but it could be possible to create an SoC with L2
> caches.  I could imagine these would be outside of the CPU and we could define
> them with the device tree.

In this case, some extra work will have to be done to set the "shared_cpu_map"
appropriately. But I think the modifications will be quite small. If the L2 cache
is external to all CPUs, then all online CPUs will have their corresponding bit
set in the "shared_cpu_map".

>> Another issue I noticed is that the unit used in ...cache/indexN/size
>> is KB. The actual value of the size is right-shifted by 10 before being
>> reported. When testing these changes using QEMU (and without making any
>> modifications to the values stored in DCCFGR and ICCFGR), the cache size
>> is far smaller than 1KB. Consequently, this is reported as 0K. For cache
>> sizes smaller than 1KB, should something be done to report it in another
>> unit? Reporting 0K seems a little misleading.
> 
> I think this is fine, as long as we pass in the correct size in bytes.

Understood.

>> [...]
>> +
>> +int init_cache_level(unsigned int cpu)
>> +{
>> +	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
>> +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> +	int leaves = 0, levels = 0;
>> +	unsigned long upr = mfspr(SPR_UPR);
>> +	unsigned long iccfgr, dccfgr;
>> +
>> +	if (!(upr & SPR_UPR_UP)) {
>> +		printk(KERN_INFO
>> +		       "-- no UPR register... unable to detect configuration\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	if (upr & SPR_UPR_DCP) {
>> +		dccfgr = mfspr(SPR_DCCFGR);
>> +		cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
>> +		cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
>> +		cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
>> +		cpuinfo->dcache.size =
>> +		    cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
>> +		leaves += 1;
>> +		printk(KERN_INFO
>> +		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>> +		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
>> +		       cpuinfo->dcache.ways);
> 
> Can we print the number of sets here too?  Also is there a reason to pad these
> int's with 4 and 2 spaces? I am not sure the padding is needed.
> 
>> +	} else
>> +		printk(KERN_INFO "-- dcache disabled\n");
>> +
>> +	if (upr & SPR_UPR_ICP) {
>> +		iccfgr = mfspr(SPR_ICCFGR);
>> +		cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
>> +		cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
>> +		cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
>> +		cpuinfo->icache.size =
>> +		    cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
>> +		leaves += 1;
>> +		printk(KERN_INFO
>> +		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>> +		       cpuinfo->icache.size, cpuinfo->icache.block_size,
>> +		       cpuinfo->icache.ways);
> 
> Same here.


Sure, I'll print the number of sets as well.

I don't think there's any reason for the padding. It was part of the original
implementation in setup.c. There shouldn't be any issues in removing them.

>> [...]
>>   	seq_printf(m, "frequency\t\t: %ld\n", loops_per_jiffy * HZ);
>> -	seq_printf(m, "dcache size\t\t: %d bytes\n", cpuinfo->dcache_size);
>> -	seq_printf(m, "dcache block size\t: %d bytes\n",
>> -		   cpuinfo->dcache_block_size);
>> -	seq_printf(m, "dcache ways\t\t: %d\n", cpuinfo->dcache_ways);
>> -	seq_printf(m, "icache size\t\t: %d bytes\n", cpuinfo->icache_size);
>> -	seq_printf(m, "icache block size\t: %d bytes\n",
>> -		   cpuinfo->icache_block_size);
>> -	seq_printf(m, "icache ways\t\t: %d\n", cpuinfo->icache_ways);
>>   	seq_printf(m, "immu\t\t\t: %d entries, %lu ways\n",
>>   		   1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
>>   		   1 + (mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTW));
>> -- 
>> 2.48.1
>>
> 
> This pretty much looks ok to me.
>

Thank you for the review.

Thanks,
Sahil

Re: [RFC] openrisc: Add cacheinfo support

[Stafford Horne]

On Sat, Mar 15, 2025 at 01:34:03AM +0530, Sahil Siddiq wrote:
> Hi,
> 
> On 3/14/25 2:54 AM, Stafford Horne wrote:
> > On Tue, Mar 11, 2025 at 12:43:57AM +0530, Sahil Siddiq wrote:
> > > Add cacheinfo support for OpenRISC.
> > > 
> > > [...]
> > > None of the functions in drivers/base/cacheinfo.c that are capable of
> > > pulling these details (e.g.: cache_size) have been used. This is because
> > > they pull these details by reading properties present in the device tree
> > > file. In setup.c, for example, the value of "clock-frequency" is pulled
> > > from the device tree file.
> > > 
> > > Cache related properties are currently not present in OpenRISC's device
> > > tree files.
> > 
> > If we want to add L2 caches and define them in the device tree would
> > it "just work" or is more work needed?
> > 
> 
> A little more work will have to be done. The implementation of "init_cache_level"
> and "populate_cache_leaves" will have to be extended. To pull L2 cache attributes,
> they'll need to make calls to the "of_get_property" family of functions similar to
> what's being done for RISC-V and PowerPC.
> 
> Shall I resubmit this patch with those extensions? I think I'll be able to test
> those changes with a modified device tree file that has an L2 cache component.

Since we don't have any such hardware now I don't think its needed.

> > > Regarding the "shared_cpu_map" cache attribute, I wasn't able to find
> > > anything in the OpenRISC architecture manual to indicate that processors
> > > in a multi-processor system may share the same cache component. MIPS uses
> > > "globalnumber" to detect siblings. LoongArch uses a "CACHE_PRIVATE" flag
> > > to detect siblings sharing the same cache.
> > 
> > In SMP environment the L1 caches are not shared they are specific to each CPU.
> > 
> > Also, we do not have hyperthreading in OpenRISC so shared_cpu_map should be a
> > 1-to-1 mapping with the cpu.  Do you need to do extra work to setup that
> > mapping?
> > 
> 
> No extra work has to be done to set up the 1-to-1 mapping. This is already being
> done in "ci_leaf_init()".

OK.

> > > I am running with the assumption that every OpenRISC core has its own
> > > icache and dcache. Given that OpenRISC does not support a multi-level
> > > cache architecture and that icache and dcache are like L1 caches, I
> > > think this assumption is reasonable. What are your thoughts on this?
> > 
> > Currently this is the case, but it could be possible to create an SoC with L2
> > caches.  I could imagine these would be outside of the CPU and we could define
> > them with the device tree.
> 
> In this case, some extra work will have to be done to set the "shared_cpu_map"
> appropriately. But I think the modifications will be quite small. If the L2 cache
> is external to all CPUs, then all online CPUs will have their corresponding bit
> set in the "shared_cpu_map".

Yes, it could be so.  For now, let's not do this as no such hardware exists.

> > > Another issue I noticed is that the unit used in ...cache/indexN/size
> > > is KB. The actual value of the size is right-shifted by 10 before being
> > > reported. When testing these changes using QEMU (and without making any
> > > modifications to the values stored in DCCFGR and ICCFGR), the cache size
> > > is far smaller than 1KB. Consequently, this is reported as 0K. For cache
> > > sizes smaller than 1KB, should something be done to report it in another
> > > unit? Reporting 0K seems a little misleading.
> > 
> > I think this is fine, as long as we pass in the correct size in bytes.
> 
> Understood.

OK.

> > > [...]
> > > +
> > > +int init_cache_level(unsigned int cpu)
> > > +{
> > > +	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
> > > +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> > > +	int leaves = 0, levels = 0;
> > > +	unsigned long upr = mfspr(SPR_UPR);
> > > +	unsigned long iccfgr, dccfgr;
> > > +
> > > +	if (!(upr & SPR_UPR_UP)) {
> > > +		printk(KERN_INFO
> > > +		       "-- no UPR register... unable to detect configuration\n");
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	if (upr & SPR_UPR_DCP) {
> > > +		dccfgr = mfspr(SPR_DCCFGR);
> > > +		cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
> > > +		cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
> > > +		cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
> > > +		cpuinfo->dcache.size =
> > > +		    cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
> > > +		leaves += 1;
> > > +		printk(KERN_INFO
> > > +		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> > > +		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
> > > +		       cpuinfo->dcache.ways);
> > 
> > Can we print the number of sets here too?  Also is there a reason to pad these
> > int's with 4 and 2 spaces? I am not sure the padding is needed.
> > 
> > > +	} else
> > > +		printk(KERN_INFO "-- dcache disabled\n");
> > > +
> > > +	if (upr & SPR_UPR_ICP) {
> > > +		iccfgr = mfspr(SPR_ICCFGR);
> > > +		cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
> > > +		cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
> > > +		cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
> > > +		cpuinfo->icache.size =
> > > +		    cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
> > > +		leaves += 1;
> > > +		printk(KERN_INFO
> > > +		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
> > > +		       cpuinfo->icache.size, cpuinfo->icache.block_size,
> > > +		       cpuinfo->icache.ways);
> > 
> > Same here.
> 
> 
> Sure, I'll print the number of sets as well.
> 
> I don't think there's any reason for the padding. It was part of the original
> implementation in setup.c. There shouldn't be any issues in removing them.

Right, it would be good to fix.

> > > [...]
> > >   	seq_printf(m, "frequency\t\t: %ld\n", loops_per_jiffy * HZ);
> > > -	seq_printf(m, "dcache size\t\t: %d bytes\n", cpuinfo->dcache_size);
> > > -	seq_printf(m, "dcache block size\t: %d bytes\n",
> > > -		   cpuinfo->dcache_block_size);
> > > -	seq_printf(m, "dcache ways\t\t: %d\n", cpuinfo->dcache_ways);
> > > -	seq_printf(m, "icache size\t\t: %d bytes\n", cpuinfo->icache_size);
> > > -	seq_printf(m, "icache block size\t: %d bytes\n",
> > > -		   cpuinfo->icache_block_size);
> > > -	seq_printf(m, "icache ways\t\t: %d\n", cpuinfo->icache_ways);
> > >   	seq_printf(m, "immu\t\t\t: %d entries, %lu ways\n",
> > >   		   1 << ((mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTS) >> 2),
> > >   		   1 + (mfspr(SPR_DMMUCFGR) & SPR_DMMUCFGR_NTW));
> > > -- 
> > > 2.48.1
> > > 
> > 
> > This pretty much looks ok to me.
> > 
> 
> Thank you for the review.

Thank you.

-Stafford

Re: [RFC] openrisc: Add cacheinfo support

[Sahil Siddiq]

Hi,

On 3/15/25 2:26 AM, Stafford Horne wrote:
> On Sat, Mar 15, 2025 at 01:34:03AM +0530, Sahil Siddiq wrote:
>> On 3/14/25 2:54 AM, Stafford Horne wrote:
>>> On Tue, Mar 11, 2025 at 12:43:57AM +0530, Sahil Siddiq wrote:
>>>> Add cacheinfo support for OpenRISC.
>>>>
>>>> [...]
>>>> None of the functions in drivers/base/cacheinfo.c that are capable of
>>>> pulling these details (e.g.: cache_size) have been used. This is because
>>>> they pull these details by reading properties present in the device tree
>>>> file. In setup.c, for example, the value of "clock-frequency" is pulled
>>>> from the device tree file.
>>>>
>>>> Cache related properties are currently not present in OpenRISC's device
>>>> tree files.
>>>
>>> If we want to add L2 caches and define them in the device tree would
>>> it "just work" or is more work needed?
>>>
>>
>> A little more work will have to be done. The implementation of "init_cache_level"
>> and "populate_cache_leaves" will have to be extended. To pull L2 cache attributes,
>> they'll need to make calls to the "of_get_property" family of functions similar to
>> what's being done for RISC-V and PowerPC.
>>
>> Shall I resubmit this patch with those extensions? I think I'll be able to test
>> those changes with a modified device tree file that has an L2 cache component.
> 
> Since we don't have any such hardware now I don't think its needed.
> > [...]
>>> Currently this is the case, but it could be possible to create an SoC with L2
>>> caches.  I could imagine these would be outside of the CPU and we could define
>>> them with the device tree.
>>
>> In this case, some extra work will have to be done to set the "shared_cpu_map"
>> appropriately. But I think the modifications will be quite small. If the L2 cache
>> is external to all CPUs, then all online CPUs will have their corresponding bit
>> set in the "shared_cpu_map".
> 
> Yes, it could be so.  For now, let's not do this as no such hardware exists.

Understood.

>>>> [...]
>>>> +
>>>> +int init_cache_level(unsigned int cpu)
>>>> +{
>>>> +	struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
>>>> +	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>>> +	int leaves = 0, levels = 0;
>>>> +	unsigned long upr = mfspr(SPR_UPR);
>>>> +	unsigned long iccfgr, dccfgr;
>>>> +
>>>> +	if (!(upr & SPR_UPR_UP)) {
>>>> +		printk(KERN_INFO
>>>> +		       "-- no UPR register... unable to detect configuration\n");
>>>> +		return -ENOENT;
>>>> +	}
>>>> +
>>>> +	if (upr & SPR_UPR_DCP) {
>>>> +		dccfgr = mfspr(SPR_DCCFGR);
>>>> +		cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
>>>> +		cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
>>>> +		cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
>>>> +		cpuinfo->dcache.size =
>>>> +		    cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
>>>> +		leaves += 1;
>>>> +		printk(KERN_INFO
>>>> +		       "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>>>> +		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
>>>> +		       cpuinfo->dcache.ways);
>>>
>>> Can we print the number of sets here too?  Also is there a reason to pad these
>>> int's with 4 and 2 spaces? I am not sure the padding is needed.
>>>
>>>> +	} else
>>>> +		printk(KERN_INFO "-- dcache disabled\n");
>>>> +
>>>> +	if (upr & SPR_UPR_ICP) {
>>>> +		iccfgr = mfspr(SPR_ICCFGR);
>>>> +		cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
>>>> +		cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
>>>> +		cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
>>>> +		cpuinfo->icache.size =
>>>> +		    cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
>>>> +		leaves += 1;
>>>> +		printk(KERN_INFO
>>>> +		       "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>>>> +		       cpuinfo->icache.size, cpuinfo->icache.block_size,
>>>> +		       cpuinfo->icache.ways);
>>>
>>> Same here.
>>
>>
>> Sure, I'll print the number of sets as well.
>>
>> I don't think there's any reason for the padding. It was part of the original
>> implementation in setup.c. There shouldn't be any issues in removing them.
> 
> Right, it would be good to fix.
> 

I have incorporated these changes in v2 of the patch.

I realized that the kernel hangs during booting when using QEMU without the DC
and IC config register changes. I have added a few more changes so the kernel
can be booted with and without these QEMU changes.

Thanks,
Sahil