Re: [RFC] openrisc: Add cacheinfo support

From: Sahil Siddiq
Date: Sat Mar 15 2025 - 16:35:26 EST


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