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

[Eric Lin]

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.