Re: [PATCH v2] openrisc: Add cacheinfo support

[Stafford Horne]

Hi Sahil,

On Tue, Mar 18, 2025 at 12:06:30AM +0530, Sahil Siddiq wrote:
> Hello Stafford and Geert,
> 
> Thank you for the reviews.
> 
> On 3/17/25 1:55 PM, Geert Uytterhoeven wrote:
> > On Sun, 16 Mar 2025 at 07:59, Stafford Horne <shorne@xxxxxxxxx> wrote:
> > > [...]
> > > > +struct cache_desc {
> > > > +     u32 size;
> > > > +     u32 sets;
> > > > +     u32 block_size;
> > > > +     u32 ways;
> > > 
> > > Considering the changes below to add cache available checks, maybe we
> > > want to add a field here, such as `bool present`.  Or a flags field like
> > > is used in loongarch?
> > 
> > I assume cache_desc.size is zero when the cache is not present?
> 
> Yes, cache_desc.size will be zero when there's no cache component since
> cpuinfo_or1k[NR_CPUS] is declared as a global variable in setup.c. The
> cache attributes are stored in this struct.
> 
> On 3/17/25 5:30 PM, Stafford Horne wrote:
> > [...]
> > Yes, good point, would be clean too work too.  I was not too happy with using
> > cache_desc.ways as is done below.  Also there ended up bieng 2 different ways
> > that were used.
> > 
> > I am happy to use size too, but I think checking the SPR would be faster or just
> > as fast as using the struct.  I am not too fussed either way.
> 
> There are a few places (e.g. cache_loop() in cache.c) where
> cpuinfo_or1k[smp_processor_id()] is not being referenced. This will have to be
> referenced to access cache_desc. In these cases, I think it would be better to
> read from the SPR instead. For consistency, the SPR can also be read where
> cpuinfo_or1k[smp_processor_id()] is already being referenced.
> 
> On 3/16/25 12:28 PM, Stafford Horne wrote:
> > On Sun, Mar 16, 2025 at 02:09:37AM +0530, Sahil Siddiq wrote:
> > > Add cacheinfo support for OpenRISC.
> > > [...]
> > > diff --git a/arch/openrisc/kernel/cacheinfo.c b/arch/openrisc/kernel/cacheinfo.c
> > > new file mode 100644
> > > index 000000000000..6bb81e246f7e
> > > --- /dev/null
> > > +++ b/arch/openrisc/kernel/cacheinfo.c
> > > @@ -0,0 +1,106 @@
> > > +// 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@xxxxxxxxx>
> > > + */
> > > +
> > > +#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: %d bytes total, %d bytes/line, %d set(s), %d way(s)\n",
> > > +		       cpuinfo->dcache.size, cpuinfo->dcache.block_size,
> > > +			   cpuinfo->dcache.sets,
> > > +		       cpuinfo->dcache.ways);
> > 
> > The indentation of sets looks a bit off here.
> > 
> > > +	} 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: %d bytes total, %d bytes/line, %d set(s), %d way(s)\n",
> > > +		       cpuinfo->icache.size, cpuinfo->icache.block_size,
> > > +			   cpuinfo->icache.sets,
> > > +		       cpuinfo->icache.ways);
> > 
> > The indentation of sets looks a bit off here. Maybe its the others that are out
> > of line, but can you fix?  Also I think sets and ways could be on the same line.
> 
> Sorry, I must have missed this. I'll fix it.
> 
> > > [...]
> > > diff --git a/arch/openrisc/kernel/dma.c b/arch/openrisc/kernel/dma.c
> > > index b3edbb33b621..ffb161e41e9d 100644
> > > --- a/arch/openrisc/kernel/dma.c
> > > +++ b/arch/openrisc/kernel/dma.c
> > > @@ -36,8 +36,10 @@ 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)
> > > -		mtspr(SPR_DCBFR, cl);
> > > +	if (cpuinfo->dcache.ways) {
> > > +		for (cl = __pa(addr); cl < __pa(next); cl += cpuinfo->dcache.block_size)
> > > +			mtspr(SPR_DCBFR, cl);
> > > +	}
> > 
> > I think it would be better to move this to cacheflush.h as a function like
> > flush_dcache_range() or local_dcache_range_flush().
> > 
> > >   	return 0;
> > >   }
> > > @@ -104,15 +106,19 @@ void arch_sync_dma_for_device(phys_addr_t addr, size_t size,
> > >   	switch (dir) {
> > >   	case DMA_TO_DEVICE:
> > >   		/* Flush the dcache for the requested range */
> > > -		for (cl = addr; cl < addr + size;
> > > -		     cl += cpuinfo->dcache_block_size)
> > > -			mtspr(SPR_DCBFR, cl);
> > > +		if (cpuinfo->dcache.ways) {
> > > +			for (cl = addr; cl < addr + size;
> > > +			     cl += cpuinfo->dcache.block_size)
> > > +				mtspr(SPR_DCBFR, cl);
> > > +		}
> > 
> > Also here,I think it would be better to move this to cacheflush.h as a function like
> > flush_dcache_range().
> > 
> > Or, local_dcache_range_flush(), which seems to be the convention we use in
> > cacheflush.h/cache.c.
> > 
> > 
> > >   		break;
> > >   	case DMA_FROM_DEVICE:
> > >   		/* Invalidate the dcache for the requested range */
> > > -		for (cl = addr; cl < addr + size;
> > > -		     cl += cpuinfo->dcache_block_size)
> > > -			mtspr(SPR_DCBIR, cl);
> > > +		if (cpuinfo->dcache.ways) {
> > > +			for (cl = addr; cl < addr + size;
> > > +			     cl += cpuinfo->dcache.block_size)
> > > +				mtspr(SPR_DCBIR, cl);
> > > +		}
> > 
> > This one could be invalidate_dcache_range().   Note, this will also be useful
> > for the kexec patches that I am working on.
> > 
> > Or, local_dcache_range_inv(), which seems to be the convention we use in
> > cacheflush.h/cache.c.
> 
> Understood.
> 
> > > [...]
> > > @@ -29,13 +34,13 @@ static __always_inline void cache_loop(struct page *page, const unsigned int reg
> > >   void local_dcache_page_flush(struct page *page)
> > >   {
> > > -	cache_loop(page, SPR_DCBFR);
> > > +	cache_loop(page, SPR_DCBFR, SPR_UPR_DCP);
> > >   }
> > >   EXPORT_SYMBOL(local_dcache_page_flush);
> > >   void local_icache_page_inv(struct page *page)
> > >   {
> > > -	cache_loop(page, SPR_ICBIR);
> > > +	cache_loop(page, SPR_ICBIR, SPR_UPR_ICP);
> > >   }
> > >   EXPORT_SYMBOL(local_icache_page_inv);
> > 
> > OK, if we move the range flush and invalidate here we will need to add to this
> > cache_loop a bit more.
> 
> Right. I'll see what can be done to keep it concise.
> 
> > > diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
> > > index d0cb1a0126f9..bbe16546c5b9 100644
> > > --- a/arch/openrisc/mm/init.c
> > > +++ b/arch/openrisc/mm/init.c
> > > @@ -124,6 +124,7 @@ static void __init map_ram(void)
> > >   void __init paging_init(void)
> > >   {
> > >   	int i;
> > > +	unsigned long upr;
> > >   	printk(KERN_INFO "Setting up paging and PTEs.\n");
> > > @@ -176,8 +177,11 @@ void __init paging_init(void)
> > >   	barrier();
> > >   	/* Invalidate instruction caches after code modification */
> > > -	mtspr(SPR_ICBIR, 0x900);
> > > -	mtspr(SPR_ICBIR, 0xa00);
> > > +	upr = mfspr(SPR_UPR);
> > > +	if (upr & SPR_UPR_UP & SPR_UPR_ICP) {
> > > +		mtspr(SPR_ICBIR, 0x900);
> > > +		mtspr(SPR_ICBIR, 0xa00);
> > > +	}
> > 
> > Here we could use new utilities such as local_icache_range_inv(0x900,
> > L1_CACHE_BYTES);
> > 
> > Or something like local_icache_block_inv(0x900).  This only needs to flush a
> > single block as the code it is invalidating is just 2 instructions 8 bytes:
> > 
> >      .org 0x900
> > 	l.j     boot_dtlb_miss_handler
> > 	 l.nop
> > 
> >      .org 0xa00
> > 	l.j     boot_itlb_miss_handler
> > 	 l.nop
> 
> Given that there'll be generic local_(i|d)cache_range_inv(start, stop) utility
> functions, would it make sense to simply have a macro defined as:
> 
> #define local_icache_block_inv(addr) local_icache_range_inv(start, L1_CACHE_BYTES)
> 
> instead of having a separate function for invalidating a single cache line? This would
> still use cache_loop() under the hood. The alternative would be to use
> local_icache_range_inv(start, L1_CACHE_BYTES) directly but using the macro might be
> more readable.

Yes, I think a macro would be fine.  Should we use cache_desc.block_size or
L1_CACHE_BYTES?  It doesn't make much difference as L1_CACHE_BYTES is defined as
16 bytes which is the minimum block size and using that will always invalidate a
whole block.  It would be good to have a comment explaining why using
L1_CACHE_BYTES is enough.

-Stafford