Re: [PATCH v2] openrisc: Add cacheinfo support

[Sahil Siddiq]

Hi Stafford,

On 3/18/25 1:13 PM, Stafford Horne wrote:
> On Tue, Mar 18, 2025 at 12:06:30AM +0530, Sahil Siddiq wrote:
> > On 3/17/25 1:55 PM, Geert Uytterhoeven wrote:
> > > On Sun, 16 Mar 2025 at 07:59, Stafford Horne <shorne@xxxxxxxxx> wrote:
> > > [...]
> > > > @@ -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.

While working on the patch's v3, I realized I am a bit unclear here. Is the ".org"
macro used to set the address at which the instructions are stored in memory? If so,
the first two instructions should occupy the memory area 0x900 through 0x907, right?
Similarly, the next two instructions will occupy 0xa00-0xa07.

Since the two instructions are 256 bytes apart, they shouldn't be cached in the same
cache line, right? Maybe one cache line will have 16 bytes starting from 0x900 while
another cache line will have 16 bytes starting from 0xa00.

If the above is true, I think it'll be better to simply call mtspr() for each address
individually.

Thanks,
Sahil