Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Jul 2023 08:15:44 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Eric Lin <eric.lin@sifive.com>
CC:     <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <palmer@dabbelt.com>,
        <paul.walmsley@sifive.com>, <will@kernel.org>,
        <mark.rutland@arm.com>, <tglx@linutronix.de>,
        <peterz@infradead.org>, <devicetree@vger.kernel.org>,
        <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <zong.li@sifive.com>, <greentime.hu@sifive.com>,
        <vincent.chen@sifive.com>, Nick Hu <nick.hu@sifive.com>
Subject: Re: [PATCH v2 2/3] soc: sifive: Add SiFive private L2 cache driver
Message-ID: <20230728-nintendo-umpire-3ca2388fa826@wendy>
References: <20230720135125.21240-1-eric.lin@sifive.com>
 <20230720135125.21240-3-eric.lin@sifive.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="tDKcUoKE7397b15h"
Content-Disposition: inline
In-Reply-To: <20230720135125.21240-3-eric.lin@sifive.com>
Precedence: bulk

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

Hey,

On Thu, Jul 20, 2023 at 09:51:20PM +0800, Eric Lin wrote:
> This adds SiFive private L2 cache driver which will show
> cache config information when booting and add cpu hotplug
> callback functions.
>=20
> Signed-off-by: Eric Lin <eric.lin@sifive.com>
> Co-developed-by: Nick Hu <nick.hu@sifive.com>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> Reviewed-by: Zong Li <zong.li@sifive.com>

> +static void pl2_config_read(void __iomem *pl2_base, int cpu)

This function's prefix does not match the rest of the driver.

> +{
> +	u32 cfg, banks, ways, cacheline, sets;
> +
> +	cfg =3D readl(pl2_base + SIFIVE_PL2CACHE_CONFIG);
> +	banks =3D FIELD_GET(SIFIVE_PL2CACHE_CONFIG_BANK_MASK, cfg);
> +	ways =3D FIELD_GET(SIFIVE_PL2CACHE_CONFIG_WAYS_MASK, cfg);
> +	cacheline =3D FIELD_GET(SIFIVE_PL2CACHE_CONFIG_BLKS_MASK, cfg);
> +	sets =3D FIELD_GET(SIFIVE_PL2CACHE_CONFIG_SETS_MASK, cfg);
> +	pr_info("%u banks, ways/bank=3D%u, bytes/block=3D%llu, sets:%llu, size:=
%d for CPU:%d\n",
> +		banks, ways, BIT_ULL(cacheline), BIT_ULL(sets), ways << (sets + cachel=
ine), cpu);
> +}

My OCD quite dislikes how the formatting here is not consistent.
"%u banks"
"ways/bank=3D%u"
"sets:%llu"

Could you please do me a favour and pick just one style here?

> +
> +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> +{
> +	struct device_node *cpu_node, *pl2_node;
> +	struct sifive_pl2_state *pl2_state =3D NULL;
> +	struct resource *res;
> +	void __iomem *pl2_base;
> +	int cpu;
> +
> +	/* Traverse all cpu nodes to find the one mapping to its pl2 node. */

While comments like this are somewhat useful...

> +	for_each_cpu(cpu, cpu_possible_mask) {
> +		cpu_node =3D of_cpu_device_node_get(cpu);
> +		pl2_node =3D of_parse_phandle(cpu_node, "next-level-cache", 0);
> +
> +		/* Found it! */
> +		if (dev_of_node(&pdev->dev) =3D=3D pl2_node) {
> +			/* Use cpu to get its percpu data sifive_pl2_state. */
> +			pl2_state =3D per_cpu_ptr(&sifive_pl2_state, cpu);
> +			break;
> +		}
> +	}
> +
> +	if (!pl2_state) {
> +		pr_err("Failed to find CPU node for %s.\n", pdev->name);
> +		return -EINVAL;
> +	}
> +
> +	/* Set base address of select and counter registers. */

=2E..something like this just restates what this function always does.
It's the same in some other places, with things like:
	/* save the state */
	save_state();
Could you drop the ones that restate the obvious please?

> +	pl2_base =3D devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(pl2_base))
> +		return PTR_ERR(pl2_base);
> +
> +	/* Print pL2 configs. */
> +	pl2_config_read(pl2_base, cpu);
> +	pl2_state->pl2_base =3D pl2_base;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver sifive_pl2_cache_driver =3D {
> +	.driver =3D {
> +		   .name =3D "SiFive-pL2-cache",
> +		   .of_match_table =3D sifive_pl2_cache_of_ids,
> +		   },
> +	.probe =3D sifive_pl2_cache_dev_probe,
> +};
> +
> +static int __init sifive_pl2_cache_init(void)

Why is the split between initcall and normal probe function required?
Does the hotplug stuff require that?

> +{
> +	int ret;
> +
> +	ret =3D cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> +				"soc/sifive/pl2:online",
> +				sifive_pl2_online_cpu,
> +				sifive_pl2_offline_cpu);
> +	if (ret < 0) {
> +		pr_err("Failed to register CPU hotplug notifier %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret =3D platform_driver_register(&sifive_pl2_cache_driver);
> +	if (ret) {
> +		pr_err("Failed to register sifive_pl2_cache_driver: %d\n", ret);
> +		cpuhp_remove_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE);
> +		return ret;
> +	}
> +
> +	sifive_pl2_pm_init();
> +
> +	return 0;
> +}

Cheers,
Conor.

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

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

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZMNrIAAKCRB4tDGHoIJi
0lUMAQC+YBZKyezcpOdepBSTcDE8d/8KKQhLZWlOf3Dxgmpf3wD9G00D01RwYreP
xr5jQCN1+kjS9dv4O3bCi+fMO5E/qAU=
=/HJJ
-----END PGP SIGNATURE-----

--tDKcUoKE7397b15h--