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

[Eric Lin]

Hi Christophe,

On Sat, Jun 17, 2023 at 3:05 AM Christophe JAILLET
<christophe.jaillet@xxxxxxxxxx> wrote:
>
> Le 16/06/2023 à 08:32, Eric Lin a écrit :
> > From: Greentime Hu <greentime.hu@xxxxxxxxxx>
> >
> > This adds SiFive private L2 cache PMU driver. User
> > can use perf tool to profile by event name and event id.
> >
> > Example:
> > $ perf stat -C 0 -e /sifive_pl2_pmu/inner_acquire_block_btot/
> >                  -e /sifive_pl2_pmu/inner_acquire_block_ntob/
> >                  -e /sifive_pl2_pmu/inner_acquire_block_ntot/ ls
> >
> >   Performance counter stats for 'CPU(s) 0':
> >
> >                 300      sifive_pl2_pmu/inner_acquire_block_btot/
> >               17801      sifive_pl2_pmu/inner_acquire_block_ntob/
> >                5253      sifive_pl2_pmu/inner_acquire_block_ntot/
> >
> >         0.088917326 seconds time elapsed
> >
> > $ perf stat -C 0 -e /sifive_pl2_pmu/event=0x10001/
> >                  -e /sifive_pl2_pmu/event=0x4001/
> >                  -e /sifive_pl2_pmu/event=0x8001/ ls
> >
> >   Performance counter stats for 'CPU(s) 0':
> >
> >                 251      sifive_pl2_pmu/event=0x10001/
> >                2620      sifive_pl2_pmu/event=0x4001/
> >                 644      sifive_pl2_pmu/event=0x8001/
> >
> >         0.092827110 seconds time elapsed
> >
> > Signed-off-by: Greentime Hu <greentime.hu@xxxxxxxxxx>
> > Signed-off-by: Eric Lin <eric.lin@xxxxxxxxxx>
> > Reviewed-by: Zong Li <zong.li@xxxxxxxxxx>
> > Reviewed-by: Nick Hu <nick.hu@xxxxxxxxxx>
> > ---
>
> [...]
>
> > +int sifive_pl2_pmu_probe(struct device_node  *pl2_node,
> > +                      void __iomem *pl2_base, int cpu)
> > +{
> > +     struct sifive_pl2_pmu_event *ptr = per_cpu_ptr(&sifive_pl2_pmu_event, cpu);
> > +     int ret = -EINVAL;
>
> Nit: no need to init
>

OK, I will fix it in v2.

> > +
> > +     /* Get counter numbers. */
> > +     ret = of_property_read_u32(pl2_node, "sifive,perfmon-counters", &ptr->counters);
> > +     if (ret) {
> > +             pr_err("Not found sifive,perfmon-counters property\n");
> > +             goto early_err;
> > +     }
> > +     pr_info("perfmon-counters: %d for CPU %d\n", ptr->counters, cpu);
> > +
> > +     /* Allocate perf_event. */
> > +     ptr->events = kcalloc(ptr->counters, sizeof(struct perf_event), GFP_KERNEL);
> > +     if (!ptr->events)
> > +             return -ENOMEM;
> > +
> > +     ptr->event_select_base = pl2_base + SIFIVE_PL2_SELECT_BASE_OFFSET;
> > +     ptr->event_counter_base = pl2_base + SIFIVE_PL2_COUNTER_BASE_OFFSET;
> > +
> > +     if (!pl2pmu_init_done) {
> > +             ret = perf_pmu_register(sifive_pl2_pmu.pmu, sifive_pl2_pmu.pmu->name, -1);
> > +             if (ret) {
> > +                     cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_SIFIVE_PL2_PMU_ONLINE,
> > +                                                 &sifive_pl2_pmu.node);
> > +                     pr_err("Failed to register sifive_pl2_pmu.pmu: %d\n", ret);
> > +             }
> > +             sifive_pl2_pmu_pm_init();
> > +             pl2pmu_init_done = true;
> > +     }
> > +
> > +     return 0;
> > +
> > +early_err:
> > +     return ret;
> > +}
> > +
> > +int sifive_pl2_pmu_init(void)
> > +{
> > +     int ret = 0;
>
> Nit: no need to init

OK, I will fix it in v2.

>
> > +
> > +     ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_SIFIVE_PL2_PMU_ONLINE,
> > +                                   "perf/sifive/pl2pmu:online",
> > +                                   sifive_pl2_pmu_online_cpu,
> > +                                   sifive_pl2_pmu_offline_cpu);
> > +     if (ret)
> > +             pr_err("Failed to register CPU hotplug notifier %d\n", ret);
> > +
> > +     ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_SIFIVE_PL2_PMU_ONLINE,
> > +                                    &sifive_pl2_pmu.node);
> > +     if (ret)
> > +             pr_err("Failed to add hotplug instance: %d\n", ret);
> > +
> > +     return ret;
>
> Nit: return 0;

OK, I will fix it in v2.

Thanks for your review.

Best regards,
Eric Lin

>
> > +}
>
> [...]
>