Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20211225054647.1750577-1-atishp@rivosinc.com> <20211225054647.1750577-7-atishp@rivosinc.com>
 <13483045.gklhn8uf4L@diego>
In-Reply-To: <13483045.gklhn8uf4L@diego>
From:   Atish Patra <atishp@atishpatra.org>
Date:   Thu, 17 Feb 2022 12:29:48 -0800
Message-ID: <CAOnJCUJ_TB9+e=W6aC2gFn-iiQe9YDNAM6RzQx646+9_7MVu6w@mail.gmail.com>
Subject: Re: [v5 6/9] RISC-V: Add perf platform driver based on SBI PMU extension
To:     =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>
Cc:     "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Atish Patra <atishp@rivosinc.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Anup Patel <anup@brainfault.org>,
        Damien Le Moal <damien.lemoal@wdc.com>,
        devicetree <devicetree@vger.kernel.org>,
        Jisheng Zhang <jszhang@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Rob Herring <robh+dt@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jan 24, 2022 at 5:16 AM Heiko St=C3=BCbner <heiko@sntech.de> wrote:
>
> Am Samstag, 25. Dezember 2021, 06:46:44 CET schrieb Atish Patra:
> > From: Atish Patra <atish.patra@wdc.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 provi=
de
> > insights into firmware activity during a performance analysis.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
>
> [...]
>
> > +static int pmu_sbi_device_probe(struct platform_device *pdev)
> > +{
> > +     struct riscv_pmu *pmu =3D 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 =3D riscv_pmu_alloc();
> > +     if (!pmu)
> > +             return -ENOMEM;
> > +
> > +     num_counters =3D 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 =3D num_counters;
> > +     pmu->ctr_start =3D pmu_sbi_ctr_start;
> > +     pmu->ctr_stop =3D pmu_sbi_ctr_stop;
> > +     pmu->event_map =3D pmu_sbi_event_map;
> > +     pmu->ctr_get_idx =3D pmu_sbi_ctr_get_idx;
> > +     pmu->ctr_get_width =3D pmu_sbi_ctr_get_width;
> > +     pmu->ctr_clear_idx =3D pmu_sbi_ctr_clear_idx;
> > +     pmu->ctr_read =3D pmu_sbi_ctr_read;
> > +
> > +     ret =3D cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &p=
mu->node);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret =3D 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 =3D {
> > +     .probe          =3D pmu_sbi_device_probe,
> > +     .driver         =3D {
> > +             .name   =3D RISCV_PMU_PDEV_NAME,
> > +     },
> > +};
> > +
> > +static int __init pmu_sbi_devinit(void)
> > +{
> > +     int ret;
> > +     struct platform_device *pdev;
> > +
> > +     if (((sbi_major_version() =3D=3D 0) && (sbi_minor_version() < 3))=
 ||
> > +             sbi_probe_extension(SBI_EXT_PMU) <=3D 0) {
> > +             return 0;
> > +     }
> > +
> > +     ret =3D 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 =3D platform_driver_register(&pmu_sbi_driver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     pdev =3D 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


--=20
Regards,
Atish