Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [v4 10/11] riscv: dts: fu740: Add pmu node
From:   Jessica Clarke <jrtc27@jrtc27.com>
In-Reply-To: <CAOnJCULqwZvK52nczp2HNinDCBjThnbGbvJpAvdameny1fK4Vw@mail.gmail.com>
Date:   Fri, 29 Oct 2021 01:07:02 +0100
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <EDB030D6-D37A-43D6-9027-222794FDA80D@jrtc27.com>
References: <20211025195350.242914-1-atish.patra@wdc.com>
 <20211025195350.242914-11-atish.patra@wdc.com>
 <YXsMtrmuavGAHk8S@Jessicas-MacBook-Pro.local>
 <CAOnJCULqwZvK52nczp2HNinDCBjThnbGbvJpAvdameny1fK4Vw@mail.gmail.com>
To:     Atish Patra <atishp@atishpatra.org>
Precedence: bulk

On 29 Oct 2021, at 00:37, Atish Patra <atishp@atishpatra.org> wrote:
>=20
> On Thu, Oct 28, 2021 at 1:49 PM Jessica Clarke <jrtc27@jrtc27.com> =
wrote:
>>=20
>> 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.
>>>=20
>>> Add the PMU node with compatible string so that Linux perf driver =
can
>>> utilize this to enable PMU.
>>>=20
>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>> ---
>>> arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>=20
>>> 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 =3D <2>;
>>>              compatible =3D "simple-bus";
>>>              ranges;
>>> +             pmu {
>>> +                     compatible =3D "riscv,pmu";
>>> +             };
>>=20
>> This is a property of the user-replaceable firmware, not a property =
of
>> the hardware,
>=20
> It's a property of hardware that indicates that the hardware supports =
PMU.

All RISC-V hardware provides the CSRs, they=E2=80=99re part of the =
privileged
spec and not marked optional. How many aren=E2=80=99t hard-wired to zero =
is up
to the implementation. But even then you can=E2=80=99t know from the =
hardware
alone what is supported; the firmware has to enable S-mode (and
U-mode)=E2=80=99s ability to read them, so you can=E2=80=99t 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=E2=80=99s=

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=E2=80=99s not relevant here, the FU740 predates and doesn=E2=80=99=
t implement
Sscofmpf, meaning there is no interrupt to even define here. And as I
said on the other patch, don=E2=80=99t conflate =E2=80=9CSBI PMU =
firmware interface is
supported=E2=80=9D and =E2=80=9CSscofmpf is implemented in the =
hardware=E2=80=9D; the former
should be discovered by talking to firmware, and the latter should be
discovered like any other extension (however that ends up happening).

>> 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.
>>=20
> 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=E2=80=99t need a device tree for that; that same code path can =
just be
=E2=80=9Cuse the existing standard firmware interface=E2=80=9D. That =
also has the
benefit that it=E2=80=99s not tied to device tree and so works =
identically for
ACPI, rather than needing an ACPI version of it.

I see nothing here that can=E2=80=99t 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