Hi Conor,
On Sat, Jun 17, 2023 at 5:05 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > Signed-off-by: Nick Hu <nick.hu@xxxxxxxxxx>
>
> Missing a Co-developed-by for Nick?
Yes, I'll add Co-developed-by for Nick in v2. Thanks.
>
>
> > +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?
>
Yes, I'll cut down the number of lines as the ccache in v2. Thanks for
the suggestion.
> > +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..
>
Yes, I'll sort by type in v2.
> > + /* 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.
>
OK, I'll rewrite the error message to make it more helpful for the user.
I'll fix it in v2. Thanks for the suggestion.
> > + goto early_err;
>
> early_err just returns ret. Why not just return the error directly?
>
Yeah, it can just return ret. I'll fix it in v2.
> > + }
> > +
> > + /* 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.
>
Yes, I'll remove the whitespace in v2.
Thanks for the review.
Best Regards,
Eric Lin.
> Cheers,
> Conor.
