Re: [PATCH v2 2/3] soc: sifive: Add SiFive private L2 cache driver

[Conor Dooley]

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.
> 
> Signed-off-by: Eric Lin <eric.lin@xxxxxxxxxx>
> Co-developed-by: Nick Hu <nick.hu@xxxxxxxxxx>
> Signed-off-by: Nick Hu <nick.hu@xxxxxxxxxx>
> Reviewed-by: Zong Li <zong.li@xxxxxxxxxx>

> +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 = readl(pl2_base + SIFIVE_PL2CACHE_CONFIG);
> +	banks = FIELD_GET(SIFIVE_PL2CACHE_CONFIG_BANK_MASK, cfg);
> +	ways = FIELD_GET(SIFIVE_PL2CACHE_CONFIG_WAYS_MASK, cfg);
> +	cacheline = FIELD_GET(SIFIVE_PL2CACHE_CONFIG_BLKS_MASK, cfg);
> +	sets = FIELD_GET(SIFIVE_PL2CACHE_CONFIG_SETS_MASK, cfg);
> +	pr_info("%u banks, ways/bank=%u, bytes/block=%llu, sets:%llu, size:%d for CPU:%d\n",
> +		banks, ways, BIT_ULL(cacheline), BIT_ULL(sets), ways << (sets + cacheline), cpu);
> +}

My OCD quite dislikes how the formatting here is not consistent.
"%u banks"
"ways/bank=%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 = 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 = of_cpu_device_node_get(cpu);
> +		pl2_node = of_parse_phandle(cpu_node, "next-level-cache", 0);
> +
> +		/* Found it! */
> +		if (dev_of_node(&pdev->dev) == pl2_node) {
> +			/* Use cpu to get its percpu data sifive_pl2_state. */
> +			pl2_state = 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. */

...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 = 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 = pl2_base;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver sifive_pl2_cache_driver = {
> +	.driver = {
> +		   .name = "SiFive-pL2-cache",
> +		   .of_match_table = sifive_pl2_cache_of_ids,
> +		   },
> +	.probe = 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 = 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 = 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.

Attachment: signature.asc
Description: PGP signature