Message-ID: <CAOnJCUJ_TB9+e=W6aC2gFn-iiQe9YDNAM6RzQx646+9_7MVu6w@mail.gmail.com>
Date:   Thu, 17 Feb 2022 12:29:48 -0800
From:   Atish Patra <atishp@...shpatra.org>
To:     Heiko Stübner <heiko@...ech.de>
Cc:     "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Atish Patra <atishp@...osinc.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Anup Patel <anup@...infault.org>,
        Damien Le Moal <damien.lemoal@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Jisheng Zhang <jszhang@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [v5 6/9] RISC-V: Add perf platform driver based on SBI PMU extension

On Mon, Jan 24, 2022 at 5:16 AM Heiko Stübner <heiko@...ech.de> wrote:
>
> Am Samstag, 25. Dezember 2021, 06:46:44 CET schrieb Atish Patra:
> > From: Atish Patra <atish.patra@....com>
> >
> > RISC-V SBI specification added a PMU extension that allows to configure
> > start/stop any pmu counter. The RISC-V perf can use most of the generic
> > perf features except interrupt overflow and event filtering based on
> > privilege mode which will be added in future.
> >
> > It also allows to monitor a handful of firmware counters that can provide
> > insights into firmware activity during a performance analysis.
> >
> > Signed-off-by: Atish Patra <atish.patra@....com>
> > Signed-off-by: Atish Patra <atishp@...osinc.com>
> > ---
>
> [...]
>
> > +static int pmu_sbi_device_probe(struct platform_device *pdev)
> > +{
> > +     struct riscv_pmu *pmu = NULL;
> > +     int num_counters;
> > +     int ret;
> > +
> > +     pr_info("SBI PMU extension is available\n");
> > +     /* Notify legacy implementation that SBI pmu is available*/
> > +     riscv_pmu_legacy_init(true);
>
> Just wondering, shouldn't the riscv_pmu_legacy_init() call live in
> pmu_sbi_devinit) below?
>

That works too. Fixed it. Thanks.

> I.e. when you detected the presence of the PMU sbi extension you already
> know that you don't want the legacy one and you have less control over
> probe-ordering (when the driver actually probes) than the initcall itself.
>
>
> Also, I think a better naming for the function might be good.
> Right now just reading
>
>         riscv_pmu_legacy_init(true);
>
> suggests that you _want_ the legacy-init to be enabled, while
> in reality the function means the opposite, disabling the legacy init.
>
> So maybe something like
>         riscv_pmu_disable_legacy(true); ?
>

yeah. Anup had a similar suggestion as well. I have changed it to
riscv_pmu_legacy_skip_init
without any argument.

>
> Heiko
>
> > +     pmu = riscv_pmu_alloc();
> > +     if (!pmu)
> > +             return -ENOMEM;
> > +
> > +     num_counters = pmu_sbi_find_num_ctrs();
> > +     if (num_counters < 0) {
> > +             pr_err("SBI PMU extension doesn't provide any counters\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     /* cache all the information about counters now */
> > +     if (pmu_sbi_get_ctrinfo(num_counters))
> > +             return -ENODEV;
> > +
> > +     pmu->num_counters = num_counters;
> > +     pmu->ctr_start = pmu_sbi_ctr_start;
> > +     pmu->ctr_stop = pmu_sbi_ctr_stop;
> > +     pmu->event_map = pmu_sbi_event_map;
> > +     pmu->ctr_get_idx = pmu_sbi_ctr_get_idx;
> > +     pmu->ctr_get_width = pmu_sbi_ctr_get_width;
> > +     pmu->ctr_clear_idx = pmu_sbi_ctr_clear_idx;
> > +     pmu->ctr_read = pmu_sbi_ctr_read;
> > +
> > +     ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = perf_pmu_register(&pmu->pmu, "cpu", PERF_TYPE_RAW);
> > +     if (ret) {
> > +             cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver pmu_sbi_driver = {
> > +     .probe          = pmu_sbi_device_probe,
> > +     .driver         = {
> > +             .name   = RISCV_PMU_PDEV_NAME,
> > +     },
> > +};
> > +
> > +static int __init pmu_sbi_devinit(void)
> > +{
> > +     int ret;
> > +     struct platform_device *pdev;
> > +
> > +     if (((sbi_major_version() == 0) && (sbi_minor_version() < 3)) ||
> > +             sbi_probe_extension(SBI_EXT_PMU) <= 0) {
> > +             return 0;
> > +     }
> > +
> > +     ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING,
> > +                                   "perf/riscv/pmu:starting",
> > +                                   pmu_sbi_starting_cpu, pmu_sbi_dying_cpu);
> > +     if (ret) {
> > +             pr_err("CPU hotplug notifier for RISC-V PMU could not be registered: %d\n",
> > +                    ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = platform_driver_register(&pmu_sbi_driver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     pdev = platform_device_register_simple(RISCV_PMU_PDEV_NAME, -1, NULL, 0);
> > +     if (IS_ERR(pdev)) {
> > +             platform_driver_unregister(&pmu_sbi_driver);
> > +             return PTR_ERR(pdev);
> > +     }
> > +
> > +     return ret;
> > +}
> > +device_initcall(pmu_sbi_devinit)
> >
>
>
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

Powered by blists - more mailing lists