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?
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); ?
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)
>
