Re: [PATCH v2] openrisc: Add cacheinfo support

[Stafford Horne <shorne@gmail.com>]

Hi Geert,

On Mon, Mar 17, 2025 at 09:25:26AM +0100, Geert Uytterhoeven wrote:
> Hi Stafford,
> 
> On Sun, 16 Mar 2025 at 07:59, Stafford Horne <shorne@gmail.com> wrote:
> > On Sun, Mar 16, 2025 at 02:09:37AM +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>
> 
> > > --- 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;
> >
> > 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, 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.

-Stafford