On Mon, Jan 24, 2022 at 5:16 AM Heiko Stübner <heiko@xxxxxxxxx> wrote:
>
> Am Samstag, 25. Dezember 2021, 06:46:44 CET schrieb Atish Patra:
> > From: Atish Patra <atish.patra@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Atish Patra <atishp@xxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
