[RFC v1 0/2] riscv: Cache Population Code Refactoring for ACPI and DT Support

[RFC v1 0/2] riscv: Cache Population Code Refactoring for ACPI and DT Support

[Sia Jee Heng]

This series of patches refactors the cache population function to
seamlessly accommodate both DT and ACPI-based platforms. Additionally,
to streamline the code, the unused parameter in the ci_leaf_init()
function has been removed.

Sia Jee Heng (2):
  riscv: cacheinfo: Remove unused parameter
  riscv: cacheinfo: Refactor populate_cache_leaves()

 arch/riscv/kernel/cacheinfo.c | 50 ++++++++++++++---------------------
 1 file changed, 20 insertions(+), 30 deletions(-)


base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC v1 1/2] riscv: cacheinfo: Remove unused parameter

[Sia Jee Heng]

Removing the unused parameter in ci_leaf_init() to simplify the code.

Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
---
 arch/riscv/kernel/cacheinfo.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 09e9b88110d1..30a6878287ad 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -64,7 +64,6 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type)
 }
 
 static void ci_leaf_init(struct cacheinfo *this_leaf,
-			 struct device_node *node,
 			 enum cache_type type, unsigned int level)
 {
 	this_leaf->level = level;
@@ -80,11 +79,11 @@ int populate_cache_leaves(unsigned int cpu)
 	int levels = 1, level = 1;
 
 	if (of_property_read_bool(np, "cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 	if (of_property_read_bool(np, "i-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 	if (of_property_read_bool(np, "d-cache-size"))
-		ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+		ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 
 	prev = np;
 	while ((np = of_find_next_cache_node(np))) {
@@ -97,11 +96,11 @@ int populate_cache_leaves(unsigned int cpu)
 		if (level <= levels)
 			break;
 		if (of_property_read_bool(np, "cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
 		if (of_property_read_bool(np, "i-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
 		if (of_property_read_bool(np, "d-cache-size"))
-			ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
+			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
 		levels = level;
 	}
 	of_node_put(np);
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Sia Jee Heng]

Refactoring the cache population function to support both DT and
ACPI-based platforms.

Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
---
 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.
+		 */
+		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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC v1 0/2] riscv: Cache Population Code Refactoring for ACPI and DT Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 905 bytes --]

On Sun, Jan 28, 2024 at 11:59:55PM -0800, Sia Jee Heng wrote:
> This series of patches refactors the cache population function to
> seamlessly accommodate both DT and ACPI-based platforms. Additionally,
> to streamline the code, the unused parameter in the ci_leaf_init()
> function has been removed.

Why's this an RFC? I don't see any mention of why.

Thanks,
Conor.

> 
> Sia Jee Heng (2):
>   riscv: cacheinfo: Remove unused parameter
>   riscv: cacheinfo: Refactor populate_cache_leaves()
> 
>  arch/riscv/kernel/cacheinfo.c | 50 ++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 
> 
> base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3904 bytes --]

Hey,

Firstly, the $subject should really mention that the motivation for the
refactoring is ACPI support.

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@starfivetech.com>
> ---
>  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.

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
if this has not been populated earlier in boot and leave the DT code
here alone?

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

RE: [RFC v1 0/2] riscv: Cache Population Code Refactoring for ACPI and DT Support

[JeeHeng Sia]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Monday, January 29, 2024 7:50 PM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>
> Cc: linux-kernel@vger.kernel.org; linux-riscv@lists.infradead.org; paul.walmsley@sifive.com; palmer@dabbelt.com;
> aou@eecs.berkeley.edu; sudeep.holla@arm.com; robh@kernel.org; conor.dooley@microchip.com; suagrfillet@gmail.com
> Subject: Re: [RFC v1 0/2] riscv: Cache Population Code Refactoring for ACPI and DT Support
> 
> On Sun, Jan 28, 2024 at 11:59:55PM -0800, Sia Jee Heng wrote:
> > This series of patches refactors the cache population function to
> > seamlessly accommodate both DT and ACPI-based platforms. Additionally,
> > to streamline the code, the unused parameter in the ci_leaf_init()
> > function has been removed.
> 
> Why's this an RFC? I don't see any mention of why.
My bad, I should have mentioned the reason in the cover letter.
Btw, the reason treating this as RFC patch is because I am seeking for more
opinions as it is the first try for riscv acpi based cacheinfo.
> 
> Thanks,
> Conor.
> 
> >
> > Sia Jee Heng (2):
> >   riscv: cacheinfo: Remove unused parameter
> >   riscv: cacheinfo: Refactor populate_cache_leaves()
> >
> >  arch/riscv/kernel/cacheinfo.c | 50 ++++++++++++++---------------------
> >  1 file changed, 20 insertions(+), 30 deletions(-)
> >
> >
> > base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[JeeHeng Sia]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Monday, January 29, 2024 8:31 PM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>
> Cc: linux-kernel@vger.kernel.org; linux-riscv@lists.infradead.org; paul.walmsley@sifive.com; palmer@dabbelt.com;
> aou@eecs.berkeley.edu; sudeep.holla@arm.com; robh@kernel.org; conor.dooley@microchip.com; suagrfillet@gmail.com
> 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@starfivetech.com>
> > ---
> >  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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5236 bytes --]

On Tue, Jan 30, 2024 at 06:24:44AM +0000, JeeHeng Sia wrote:
> > From: Conor Dooley <conor@kernel.org>
> > 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@starfivetech.com>
> > > ---
> > >  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@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv