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;
MIME-Version: 1.0
References: <20230616063210.19063-1-eric.lin@sifive.com> <20230616063210.19063-2-eric.lin@sifive.com>
 <20230616-errand-glutton-f64783da058c@spud>
In-Reply-To: <20230616-errand-glutton-f64783da058c@spud>
From:   Eric Lin <eric.lin@sifive.com>
Date:   Fri, 23 Jun 2023 17:49:22 +0800
Message-ID: <CAPqJEFr1FfuHYX=BP3BXAsZ0V4JFDqzu7_jjPVgcijOFQzddmQ@mail.gmail.com>
Subject: Re: [PATCH 1/3] soc: sifive: Add SiFive private L2 cache support
To:     Conor Dooley <conor@kernel.org>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        palmer@dabbelt.com, paul.walmsley@sifive.com,
        aou@eecs.berkeley.edu, maz@kernel.org, chenhuacai@kernel.org,
        baolu.lu@linux.intel.com, will@kernel.org,
        kan.liang@linux.intel.com, nnac123@linux.ibm.com,
        pierre.gondois@arm.com, huangguangbin2@huawei.com, jgross@suse.com,
        chao.gao@intel.com, maobibo@loongson.cn,
        linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, dslin1010@gmail.com,
        Nick Hu <nick.hu@sifive.com>, Zong Li <zong.li@sifive.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Conor,

On Sat, Jun 17, 2023 at 5:05=E2=80=AFAM Conor Dooley <conor@kernel.org> wro=
te:
>
> 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?

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 =3D readl(pl2_base);
> > +     bank =3D regval & 0xff;
> > +     pr_info("in the CPU: %d\n", cpu);
> > +     pr_info("No. of Banks in the cache: %d\n", bank);
> > +     way =3D (regval & 0xff00) >> 8;
> > +     pr_info("No. of ways per bank: %d\n", way);
> > +     set =3D (regval & 0xff0000) >> 16;
> > +     pr_info("Total sets: %llu\n", (uint64_t)1 << set);
> > +     cacheline =3D (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 =3D readl(ccache_base + SIFIVE_CCACHE_CONFIG);
>                 pr_info("%llu banks, %llu ways, sets/bank=3D%llu, bytes/b=
lock=3D%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 =3D 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 =3D -EINVAL;
> > +     struct device_node *cpu_node, *pl2_node;
> > +     struct sifive_pl2_state *pl2_state =3D 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 =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_stat=
e. */
> > +                     pl2_state =3D 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 =3D devm_platform_get_and_ioremap_resource(pdev, 0, &res=
);
> > +     if (IS_ERR(pl2_base)) {
> > +             ret =3D PTR_ERR(pl2_base);
> > +             goto early_err;
> > +     }
> > +
> > +     /* Print pL2 configs. */
> > +     pl2_config_read(pl2_base, cpu);
> > +     pl2_state->pl2_base =3D pl2_base;
> > +
> > +     return 0;
> > +
> > +early_err:
> > +     return ret;
> > +}
>
> > +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)
> > +{
> > +     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);
>
> 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.