Re: [PATCH 1/3] soc: sifive: Add SiFive private L2 cache support

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4243 bytes --]

Hey Eric,

On Fri, Jun 16, 2023 at 02:32:08PM +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@sifive.com>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>

Missing a Co-developed-by for Nick?


> +static void pl2_config_read(void __iomem *pl2_base, int cpu)
> +{
> +	u32 regval, bank, way, set, cacheline;
> +
> +	regval = readl(pl2_base);
> +	bank = regval & 0xff;
> +	pr_info("in the CPU: %d\n", cpu);
> +	pr_info("No. of Banks in the cache: %d\n", bank);
> +	way = (regval & 0xff00) >> 8;
> +	pr_info("No. of ways per bank: %d\n", way);
> +	set = (regval & 0xff0000) >> 16;
> +	pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> +	cacheline = (regval & 0xff000000) >> 24;
> +	pr_info("Bytes per cache block: %llu\n", (uint64_t)1 << cacheline);
> +	pr_info("Size: %d\n", way << (set + cacheline));
> +}

Isn't this basically all information that we get anyway in sysfs based
on what gets put into the DT, except printed out once per CPU at
boottime?
If there's reason to keep it, please do as suggested by Ben and cut down
the number of lines emitted. Look at the ccache one for comparison:
	static void ccache_config_read(void)
	{
		u32 cfg;
	
		cfg = readl(ccache_base + SIFIVE_CCACHE_CONFIG);
		pr_info("%llu banks, %llu ways, sets/bank=%llu, bytes/block=%llu\n",
			FIELD_GET(SIFIVE_CCACHE_CONFIG_BANK_MASK, cfg),
			FIELD_GET(SIFIVE_CCACHE_CONFIG_WAYS_MASK, cfg),
			BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_SETS_MASK, cfg)),
			BIT_ULL(FIELD_GET(SIFIVE_CCACHE_CONFIG_BLKS_MASK, cfg)));
	
		cfg = readl(ccache_base + SIFIVE_CCACHE_WAYENABLE);
		pr_info("Index of the largest way enabled: %u\n", cfg);
	}
It'd also be good to print the same things as the ccache, no?

> +static int sifive_pl2_cache_dev_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	int cpu, ret = -EINVAL;
> +	struct device_node *cpu_node, *pl2_node;
> +	struct sifive_pl2_state *pl2_state = NULL;
> +	void __iomem *pl2_base;

Please pick a sensible ordering for variables. IDC if it is reverse xmas
tree, or sorting by types, but this just seems quite random..

> +	/* Traverse all cpu nodes to find the one mapping to its pl2 node. */
> +	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("Not found the corresponding cpu_node in dts.\n");

I don't think this error message is going to be helpful in figuring out
where the problem is on a machine with many of the caches. More
information about *which* cache caused it would be good.
Also it is not grammatically correct, it should read something like
"Failed to find CPU node for cache@abc" or something along those lines.

> +		goto early_err;

early_err just returns ret. Why not just return the error directly?

> +	}
> +
> +	/* Set base address of select and counter registers. */
> +	pl2_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(pl2_base)) {
> +		ret = PTR_ERR(pl2_base);
> +		goto early_err;
> +	}
> +
> +	/* Print pL2 configs. */
> +	pl2_config_read(pl2_base, cpu);
> +	pl2_state->pl2_base = pl2_base;
> +
> +	return 0;
> +
> +early_err:
> +	return ret;
> +}

> +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)
> +{
> +	int ret;
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_RISCV_SIFIVE_PL2_ONLINE,
> +				"soc/sifive/pl2:online",
> +				      sifive_pl2_online_cpu,
> +				      sifive_pl2_offline_cpu);

Got some weird use of whitespace here & above, please remove the spaces.

Cheers,
Conor.

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