From: Atish Patra <atishp@atishpatra.org> To: Jessica Clarke <jrtc27@jrtc27.com> Cc: Atish Patra <atish.patra@wdc.com>, "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>, Anup Patel <anup.patel@wdc.com>, David Abdurachmanov <david.abdurachmanov@sifive.com>, devicetree <devicetree@vger.kernel.org>, Greentime Hu <greentime.hu@sifive.com>, Guo Ren <guoren@linux.alibaba.com>, Heinrich Schuchardt <xypron.glpk@gmx.de>, Jonathan Corbet <corbet@lwn.net>, Linux Doc Mailing List <linux-doc@vger.kernel.org>, linux-perf-users@vger.kernel.org, linux-riscv <linux-riscv@lists.infradead.org>, Nick Kossifidis <mick@ics.forth.gr>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Rob Herring <robh+dt@kernel.org>, Vincent Chen <vincent.chen@sifive.com> Subject: Re: [v4 10/11] riscv: dts: fu740: Add pmu node Date: Thu, 28 Oct 2021 23:05:42 -0700 [thread overview] Message-ID: <CAOnJCUJjzmW=QobLPKAWYGppFeoJXjT2Ee6eG2-H=s2mnei=RQ@mail.gmail.com> (raw) In-Reply-To: <EDB030D6-D37A-43D6-9027-222794FDA80D@jrtc27.com> On Thu, Oct 28, 2021 at 5:07 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 29 Oct 2021, at 00:37, Atish Patra <atishp@atishpatra.org> wrote: > > > > On Thu, Oct 28, 2021 at 1:49 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > >> > >> On Mon, Oct 25, 2021 at 12:53:49PM -0700, Atish Patra wrote: > >>> HiFive unmatched supports HPMCounters but does not implement mcountinhibit > >>> or sscof extension. Thus, perf monitoring can be used on the unmatched > >>> board without sampling. > >>> > >>> Add the PMU node with compatible string so that Linux perf driver can > >>> utilize this to enable PMU. > >>> > >>> Signed-off-by: Atish Patra <atish.patra@wdc.com> > >>> --- > >>> arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> index abbb960f90a0..b35b96b58820 100644 > >>> --- a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> +++ b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> @@ -140,6 +140,9 @@ soc { > >>> #size-cells = <2>; > >>> compatible = "simple-bus"; > >>> ranges; > >>> + pmu { > >>> + compatible = "riscv,pmu"; > >>> + }; > >> > >> This is a property of the user-replaceable firmware, not a property of > >> the hardware, > > > > It's a property of hardware that indicates that the hardware supports PMU. > > All RISC-V hardware provides the CSRs, they’re part of the privileged > spec and not marked optional. How many aren’t hard-wired to zero is up > to the implementation. But even then you can’t know from the hardware > alone what is supported; the firmware has to enable S-mode (and > U-mode)’s ability to read them, so you can’t assume anything in a > static device tree hard-coded in Linux about what firmware has done. > Since you currently have to query the firmware to determine what’s > available to you anyway, I see no benefit from having a node in the > device tree that tells you your firmware *might* have counters you can > use. > > > Additionally, the counter overflow interrupt number needs to be > > defined through the DT as well > > so that a clean platform driver can be implemented. > > The interrupt number is specified as 13 by the Sscofmpf spec. > But that’s not relevant here, the FU740 predates and doesn’t implement > Sscofmpf, meaning there is no interrupt to even define here. And as I > said on the other patch, don’t conflate “SBI PMU firmware interface is > supported” and “Sscofmpf is implemented in the hardware”; the former > should be discovered by talking to firmware, and the latter should be > discovered like any other extension (however that ends up happening). Presence of sscof extension can be discovered through general extension discovery mechanism (probably a separate DT node..that's a separate discussion). However, the interrupt number discovery has to be through DT so the platform driver can probe the DT to figure out that. > > >> so having this in the device tree under /soc, let alone > >> hard-coded in Linux, is utterly wrong. Why can this not just be probed > >> like any other SBI interface? The "Probe SBI extension" interface is > >> precisely for this kind of thing. > >> > > SBI extension is anyways probed to verify if the firmware has PMU > > extension or not. > > However, adding the DT property allows different platforms (with or > > without sscof extension) > > to use the same code path. > > You don’t need a device tree for that; that same code path can just be > “use the existing standard firmware interface”. That also has the > benefit that it’s not tied to device tree and so works identically for > ACPI, rather than needing an ACPI version of it. > I don't disagree with that argument. However, we need a DT node for interrupt number as explained in the above. A DT based platform driver allows us to provide a unified code path which can handle both kinds of platforms described below. 1. Platforms without sscof extension 2. Platforms with sscof extension that requires a DT node for interrupt number Otherwise, the driver has to do the following things in order. 1. Probe PMU extension 2. first check if sscof extension is present in the special RISC-V ISA extension DT node (which is yet to finalize) 3. If sscof extension is present then register for a DT based platform driver. 4. Otherwise, register a simple platform driver. I am not completely opposed to doing that if there is a strong technical issue with the current approach. > I see nothing here that can’t be discovered through pre-existing means. > If it can be discovered without use of the device tree then it does not > belong in the device tree; the device tree is purely for things that > cannot otherwise be discovered. > > Jess > -- Regards, Atish
WARNING: multiple messages have this Message-ID (diff)
From: Atish Patra <atishp@atishpatra.org> To: Jessica Clarke <jrtc27@jrtc27.com> Cc: Atish Patra <atish.patra@wdc.com>, "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>, Anup Patel <anup.patel@wdc.com>, David Abdurachmanov <david.abdurachmanov@sifive.com>, devicetree <devicetree@vger.kernel.org>, Greentime Hu <greentime.hu@sifive.com>, Guo Ren <guoren@linux.alibaba.com>, Heinrich Schuchardt <xypron.glpk@gmx.de>, Jonathan Corbet <corbet@lwn.net>, Linux Doc Mailing List <linux-doc@vger.kernel.org>, linux-perf-users@vger.kernel.org, linux-riscv <linux-riscv@lists.infradead.org>, Nick Kossifidis <mick@ics.forth.gr>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Rob Herring <robh+dt@kernel.org>, Vincent Chen <vincent.chen@sifive.com> Subject: Re: [v4 10/11] riscv: dts: fu740: Add pmu node Date: Thu, 28 Oct 2021 23:05:42 -0700 [thread overview] Message-ID: <CAOnJCUJjzmW=QobLPKAWYGppFeoJXjT2Ee6eG2-H=s2mnei=RQ@mail.gmail.com> (raw) In-Reply-To: <EDB030D6-D37A-43D6-9027-222794FDA80D@jrtc27.com> On Thu, Oct 28, 2021 at 5:07 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 29 Oct 2021, at 00:37, Atish Patra <atishp@atishpatra.org> wrote: > > > > On Thu, Oct 28, 2021 at 1:49 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > >> > >> On Mon, Oct 25, 2021 at 12:53:49PM -0700, Atish Patra wrote: > >>> HiFive unmatched supports HPMCounters but does not implement mcountinhibit > >>> or sscof extension. Thus, perf monitoring can be used on the unmatched > >>> board without sampling. > >>> > >>> Add the PMU node with compatible string so that Linux perf driver can > >>> utilize this to enable PMU. > >>> > >>> Signed-off-by: Atish Patra <atish.patra@wdc.com> > >>> --- > >>> arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> index abbb960f90a0..b35b96b58820 100644 > >>> --- a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> +++ b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi > >>> @@ -140,6 +140,9 @@ soc { > >>> #size-cells = <2>; > >>> compatible = "simple-bus"; > >>> ranges; > >>> + pmu { > >>> + compatible = "riscv,pmu"; > >>> + }; > >> > >> This is a property of the user-replaceable firmware, not a property of > >> the hardware, > > > > It's a property of hardware that indicates that the hardware supports PMU. > > All RISC-V hardware provides the CSRs, they’re part of the privileged > spec and not marked optional. How many aren’t hard-wired to zero is up > to the implementation. But even then you can’t know from the hardware > alone what is supported; the firmware has to enable S-mode (and > U-mode)’s ability to read them, so you can’t assume anything in a > static device tree hard-coded in Linux about what firmware has done. > Since you currently have to query the firmware to determine what’s > available to you anyway, I see no benefit from having a node in the > device tree that tells you your firmware *might* have counters you can > use. > > > Additionally, the counter overflow interrupt number needs to be > > defined through the DT as well > > so that a clean platform driver can be implemented. > > The interrupt number is specified as 13 by the Sscofmpf spec. > But that’s not relevant here, the FU740 predates and doesn’t implement > Sscofmpf, meaning there is no interrupt to even define here. And as I > said on the other patch, don’t conflate “SBI PMU firmware interface is > supported” and “Sscofmpf is implemented in the hardware”; the former > should be discovered by talking to firmware, and the latter should be > discovered like any other extension (however that ends up happening). Presence of sscof extension can be discovered through general extension discovery mechanism (probably a separate DT node..that's a separate discussion). However, the interrupt number discovery has to be through DT so the platform driver can probe the DT to figure out that. > > >> so having this in the device tree under /soc, let alone > >> hard-coded in Linux, is utterly wrong. Why can this not just be probed > >> like any other SBI interface? The "Probe SBI extension" interface is > >> precisely for this kind of thing. > >> > > SBI extension is anyways probed to verify if the firmware has PMU > > extension or not. > > However, adding the DT property allows different platforms (with or > > without sscof extension) > > to use the same code path. > > You don’t need a device tree for that; that same code path can just be > “use the existing standard firmware interface”. That also has the > benefit that it’s not tied to device tree and so works identically for > ACPI, rather than needing an ACPI version of it. > I don't disagree with that argument. However, we need a DT node for interrupt number as explained in the above. A DT based platform driver allows us to provide a unified code path which can handle both kinds of platforms described below. 1. Platforms without sscof extension 2. Platforms with sscof extension that requires a DT node for interrupt number Otherwise, the driver has to do the following things in order. 1. Probe PMU extension 2. first check if sscof extension is present in the special RISC-V ISA extension DT node (which is yet to finalize) 3. If sscof extension is present then register for a DT based platform driver. 4. Otherwise, register a simple platform driver. I am not completely opposed to doing that if there is a strong technical issue with the current approach. > I see nothing here that can’t be discovered through pre-existing means. > If it can be discovered without use of the device tree then it does not > belong in the device tree; the device tree is purely for things that > cannot otherwise be discovered. > > Jess > -- Regards, Atish _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-10-29 6:05 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-25 19:53 [v4 00/11] Improve RISC-V Perf support using SBI PMU and sscofpmf extension Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 01/11] RISC-V: Remove the current perf implementation Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 02/11] RISC-V: Add CSR encodings for all HPMCOUNTERS Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 03/11] RISC-V: Add a perf core library for pmu drivers Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 04/11] RISC-V: Add a simple platform driver for RISC-V legacy perf Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 05/11] RISC-V: Add RISC-V SBI PMU extension definitions Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-12-15 8:02 ` Nikita Shubin 2021-12-15 8:02 ` Nikita Shubin 2021-12-15 16:03 ` Atish Patra 2021-12-15 16:03 ` Atish Patra 2021-10-25 19:53 ` [v4 06/11] dt-binding: pmu: Add RISC-V PMU DT bindings Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-26 18:03 ` Rob Herring 2021-10-26 18:03 ` Rob Herring 2021-10-26 18:57 ` Rob Herring 2021-10-26 18:57 ` Rob Herring 2021-10-28 20:17 ` Jessica Clarke 2021-10-28 20:17 ` Jessica Clarke 2021-10-25 19:53 ` [v4 07/11] RISC-V: Add perf platform driver based on SBI PMU extension Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 08/11] RISC-V: Add interrupt support for perf Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 09/11] Documentation: riscv: Remove the old documentation Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-25 19:53 ` [v4 10/11] riscv: dts: fu740: Add pmu node Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-10-28 20:48 ` Jessica Clarke 2021-10-28 20:48 ` Jessica Clarke 2021-10-28 23:37 ` Atish Patra 2021-10-28 23:37 ` Atish Patra 2021-10-29 0:07 ` Jessica Clarke 2021-10-29 0:07 ` Jessica Clarke 2021-10-29 6:05 ` Atish Patra [this message] 2021-10-29 6:05 ` Atish Patra 2021-10-29 12:25 ` Jessica Clarke 2021-10-29 12:25 ` Jessica Clarke 2021-10-25 19:53 ` [v4 11/11] MAINTAINERS: Add entry for RISC-V PMU drivers Atish Patra 2021-10-25 19:53 ` Atish Patra 2021-12-14 1:51 ` [v4 00/11] Improve RISC-V Perf support using SBI PMU and sscofpmf extension Palmer Dabbelt 2021-12-14 1:51 ` Palmer Dabbelt 2021-12-14 3:16 ` Atish Patra 2021-12-14 3:16 ` Atish Patra 2021-12-14 18:09 ` Will Deacon 2021-12-14 18:09 ` Will Deacon 2021-12-14 9:14 ` Nikita Shubin 2021-12-14 9:14 ` Nikita Shubin 2021-12-14 18:29 ` Atish Patra 2021-12-14 18:29 ` Atish Patra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAOnJCUJjzmW=QobLPKAWYGppFeoJXjT2Ee6eG2-H=s2mnei=RQ@mail.gmail.com' \ --to=atishp@atishpatra.org \ --cc=anup.patel@wdc.com \ --cc=atish.patra@wdc.com \ --cc=corbet@lwn.net \ --cc=david.abdurachmanov@sifive.com \ --cc=devicetree@vger.kernel.org \ --cc=greentime.hu@sifive.com \ --cc=guoren@linux.alibaba.com \ --cc=jrtc27@jrtc27.com \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=mick@ics.forth.gr \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=robh+dt@kernel.org \ --cc=vincent.chen@sifive.com \ --cc=xypron.glpk@gmx.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.