Received-SPF: pass (google.com: domain of linux-kernel+bounces-42689-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Jan 2024 12:30:59 +0000
From: Conor Dooley <conor@kernel.org>
To: Sia Jee Heng <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()
Message-ID: <20240129-parrot-dropout-c4ece33a98da@spud>
References: <20240129075957.116033-1-jeeheng.sia@starfivetech.com>
 <20240129075957.116033-3-jeeheng.sia@starfivetech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="vVQN45TubuFKPBU4"
Content-Disposition: inline
In-Reply-To: <20240129075957.116033-3-jeeheng.sia@starfivetech.com>


--vVQN45TubuFKPBU4
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> ---
>  arch/riscv/kernel/cacheinfo.c | 47 ++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
>=20
> 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 =3D get_cpu_cacheinfo(cpu);
>  	struct cacheinfo *this_leaf =3D this_cpu_ci->info_list;
> -	struct device_node *np =3D of_cpu_device_node_get(cpu);
> -	struct device_node *prev =3D NULL;
> -	int levels =3D 1, level =3D 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 =3D np;
> -	while ((np =3D of_find_next_cache_node(np))) {
> -		of_node_put(prev);
> -		prev =3D np;
> -		if (!of_device_is_compatible(np, "cache"))
> -			break;
> -		if (of_property_read_u32(np, "cache-level", &level))
> -			break;
> -		if (level <=3D 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 =3D 0, level =3D 1; level <=3D this_cpu_ci->num_levels &&
> +	     idx < this_cpu_ci->num_leaves; idx++, level++) {
> +		/*
> +		 * Since the RISC-V architecture doesn't provide any register for dete=
cting the
> +		 * Cache Level and Cache type, this assumes that:
> +		 * - There cannot be any split caches (data/instruction) above a unifi=
ed cache.
> +		 * - Data/instruction caches come in pairs.
> +		 * - Significant work is required elsewhere to fully support data/inst=
ruction-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 =3D=3D 1) {
>  			ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> -		levels =3D level;
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> +		} else {
> +			ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
> +		}
>  	}
> -	of_node_put(np);
> =20
>  	return 0;
>  }
> --=20
> 2.34.1
>=20
>=20
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

--vVQN45TubuFKPBU4
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZbeagwAKCRB4tDGHoIJi
0p2vAP91xqgYKdI5R4E8M2TeUKf7W4MillrhtPoe4A++bU5YagD+NYICS59UguEb
kC/D3NRlCAVajtBJRjiLWpUC77PETws=
=E7YN
-----END PGP SIGNATURE-----

--vVQN45TubuFKPBU4--