RE: [RFC v1 2/2] riscv: cacheinfo: Refactor populate_cache_leaves()

[JeeHeng Sia]

> -----Original Message-----
> From: Conor Dooley <conor@xxxxxxxxxx>
> Sent: Monday, January 29, 2024 8:31 PM
> To: JeeHeng Sia <jeeheng.sia@xxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-riscv@xxxxxxxxxxxxxxxxxxx; paul.walmsley@xxxxxxxxxx; palmer@xxxxxxxxxxx;
> aou@xxxxxxxxxxxxxxxxx; sudeep.holla@xxxxxxx; robh@xxxxxxxxxx; conor.dooley@xxxxxxxxxxxxx; suagrfillet@xxxxxxxxx
> Subject: Re: [RFC v1 2/2] riscv: cacheinfo: Refactor populate_cache_leaves()
> 
> Hey,
> 
> Firstly, the $subject should really mention that the motivation for the
> refactoring is ACPI support.
Noted. In fact, the main motivation is to support both DT and ACPI.
> 
> On Sun, Jan 28, 2024 at 11:59:57PM -0800, Sia Jee Heng wrote:
> > Refactoring the cache population function to support both DT and
> > ACPI-based platforms.
> >
> > Signed-off-by: Sia Jee Heng <jeeheng.sia@xxxxxxxxxxxxxxxx>
> > ---
> >  arch/riscv/kernel/cacheinfo.c | 47 ++++++++++++++---------------------
> >  1 file changed, 19 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> > index 30a6878287ad..f10e26fb75b6 100644
> > --- a/arch/riscv/kernel/cacheinfo.c
> > +++ b/arch/riscv/kernel/cacheinfo.c
> > @@ -74,36 +74,27 @@ int populate_cache_leaves(unsigned int cpu)
> >  {
> >  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> >  	struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> > -	struct device_node *np = of_cpu_device_node_get(cpu);
> > -	struct device_node *prev = NULL;
> > -	int levels = 1, level = 1;
> > -
> > -	if (of_property_read_bool(np, "cache-size"))
> > -		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> > -	if (of_property_read_bool(np, "i-cache-size"))
> > -		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> > -	if (of_property_read_bool(np, "d-cache-size"))
> > -		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> > -
> > -	prev = np;
> > -	while ((np = of_find_next_cache_node(np))) {
> > -		of_node_put(prev);
> > -		prev = np;
> > -		if (!of_device_is_compatible(np, "cache"))
> > -			break;
> > -		if (of_property_read_u32(np, "cache-level", &level))
> > -			break;
> > -		if (level <= levels)
> > -			break;
> > -		if (of_property_read_bool(np, "cache-size"))
> > -			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> > -		if (of_property_read_bool(np, "i-cache-size"))
> > -			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> > -		if (of_property_read_bool(np, "d-cache-size"))
> > +	unsigned int level, idx;
> > +
> > +	for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
> > +	     idx < this_cpu_ci->num_leaves; idx++, level++) {
> > +		/*
> > +		 * Since the RISC-V architecture doesn't provide any register for detecting the
> > +		 * Cache Level and Cache type, this assumes that:
> > +		 * - There cannot be any split caches (data/instruction) above a unified cache.
> > +		 * - Data/instruction caches come in pairs.
> > +		 * - Significant work is required elsewhere to fully support data/instruction-only
> > +		 *   type caches.
> > +		 * - The above assumptions are based on conventional system design and known
> > +		 *   examples.
> 
> I don't think this comment matches what you are doing.
> 
> For example, the comment only requires that split caches cannot be above
> unified ones, but the code will always make a level 1 cache be split and
> higher level caches unified.
> 
> The place you took the comment about the split caches from does not
> enforce the type of cache layout that you do where the 1st level is
> always split and anything else is unified.
Correct, I meant to say 1st level is always split and anything else is unified.
But, do we agree with the statement?
> 
> populate_cache_leaves() only gets called in a fallback path when the
> information has not already been configured by other means (and as you
> probably noticed on things like arm64 it uses some other means to fill
> in the data).
> 
> Is there a reason why we would not just return -ENOENT for ACPI systems
I don't think that we should return -ENOENT otherwise the cacheinfo
framework would failed.
> if this has not been populated earlier in boot and leave the DT code
> here alone?
This function is shared by both ACPI and DT.
> 
> Thanks,
> Conor.
> 
> > +		 */
> > +		if (level == 1) {
> >  			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> > -		levels = level;
> > +			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> > +		} else {
> > +			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> > +		}
> >  	}
> > -	of_node_put(np);
> >
> >  	return 0;
> >  }
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv