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

[Conor Dooley]

On Tue, Jan 30, 2024 at 06:24:44AM +0000, JeeHeng Sia wrote:
> > From: Conor Dooley <conor@xxxxxxxxxx>
> > Sent: Monday, January 29, 2024 8:31 PM
> > 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?

That the first level is always split and anything else is always unified?
No, but I think the assumption /in the comment/ is reasonable however.

This is your patch, you need to justify the changes you are making
here, not ask me if it is okay after I noticed that your comments and
code do not match.

> > 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 you don't have a way to determine the cache layout, what makes
-ENOENT worse than making something up?

Why does your system not get information from its ACPI tables?

> > 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.

I don't see how that answers my question.

Why should the DT systems stop trying to parse for the information?

Why must ACPI and DT do the same thing here?

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

Attachment: signature.asc
Description: PGP signature