[RFC PATCH] dt-bindings: riscv: Add YAML documentation for PMU
Anup Patel
anup at brainfault.org
Fri Jul 31 05:44:12 EDT 2020
On Thu, Jul 30, 2020 at 11:23 AM Zong Li <zong.li at sifive.com> wrote:
>
> On Tue, Jul 28, 2020 at 8:10 PM Anup Patel <anup at brainfault.org> wrote:
> >
> > On Tue, Jul 28, 2020 at 9:38 AM Zong Li <zong.li at sifive.com> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup at brainfault.org> wrote:
> > > >
> > > > On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li at sifive.com> wrote:
> > > > >
> > > > > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup at brainfault.org> wrote:
> > > > > >
> > > > > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li at sifive.com> wrote:
> > > > > > >
> > > > > > > Add device tree bindings for performance monitor unit. It passes the
> > > > > > > dt_binding_check verification.
> > > > > > >
> > > > > > > Signed-off-by: Zong Li <zong.li at sifive.com>
> > > > > > > ---
> > > > > > > .../devicetree/bindings/riscv/pmu.yaml | 71 +++++++++++++++++++
> > > > > > > 1 file changed, 71 insertions(+)
> > > > > > > create mode 100644 Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/pmu.yaml b/Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..0c49039a5d3b
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > > @@ -0,0 +1,71 @@
> > > > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/riscv/pmu.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: RISC-V Performance Monitor Units
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > + - Zong Li <zong.li at sifive.com>
> > > > > > > + - Paul Walmsley <paul.walmsley at sifive.com>
> > > > > > > + - Palmer Dabbelt <palmer at dabbelt.com>
> > > > > > > +
> > > > > > > +properties:
> > > > > > > + compatible:
> > > > > > > + items:
> > > > > > > + - const: riscv,pmu
> > > > > > > +
> > > > > > > + riscv,width-hpmcntr:
> > > > > > > + description: The width of hpmcounter CSRs. Default is 64.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > +
> > > > > > > + riscv,n-hpmcntr:
> > > > > > > + description: The number of hpmcounter CSRs. Default is zero.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > +
> > > > > > > + riscv,hw-event-map:
> > > > > > > + description: The mapping of generic hardware events to values of hpmcounter.
> > > > > > > + The key is the encoding of generic hardware events, and the value is the
> > > > > > > + actual value which is implemented by platform. If there is no a key-value
> > > > > > > + pair for specific generic hardware event, view the generic hardware event
> > > > > > > + as not supported. CYCLE and INSTRET be mapped by default, so we shouldn't
> > > > > > > + list PERF_COUNT_HW_CPU_CYCLES and PERF_COUNT_HW_INSTRUCTIONS here.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > > +
> > > > > > > + riscv,hw-cache-event-map:
> > > > > > > + description: The mapping of generic hardware cache events to values of
> > > > > > > + hpmcounter. The key is encoding of generic hardware cache events, and the
> > > > > > > + value is the actual value which is implemented by platform. If there is no
> > > > > > > + a key-value pair for specific generic hardware cache event, view the
> > > > > > > + generic hardware cache event as not supported.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > > +
> > > > > > > + riscv,hpmcntr-of-event:
> > > > > > > + description: The mapping of platform hardware events to allowed hmpcounters.
> > > > > > > + The key is the platform hardware event, and the value is the bitmap for
> > > > > > > + hmpcounters which support this event. If there is no a key-value pair for
> > > > > > > + specific platform hardware events, view the platform hardware events as
> > > > > > > + supported by all hpmcounters.
> > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > > +
> > > > > > > +required:
> > > > > > > + - compatible
> > > > > > > +
> > > > > > > +additionalProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > + - |
> > > > > > > + pmu {
> > > > > > > + compatible = "riscv,pmu";
> > > > > > > + riscv,width-hpmcntr = <40>;
> > > > > > > + riscv,n-hpmcntr = <2>;
> > > > > > > + riscv,hw-event-map = <0x3 0x0202
> > > > > > > + 0x4 0x4000>;
> > > > > > > + riscv,hw-cache-event-map = <0x010201 0x0102
> > > > > > > + 0x010204 0x0802>;
> > > > > > > + riscv,hpmcntr-of-event = <0x100 0x18
> > > > > > > + 0x400 0x10>;
> > > > > > > + };
> > > > > > > +
> > > > > > > +...
> > > > > > > --
> > > > > > > 2.27.0
> > > > > > >
> > > > > >
> > > > > > I don't see the point of sending DT bindings documents
> > > > > > until the SBI PMU extension is defined and accepted
> > > > > > by everyone.
> > > > > >
> > > > >
> > > > > It seems to me that it doesn't conflict with the SBI PMU extension.
> > > > > SBI PMU extension is the interface for communication with the lower
> > > > > privilege levels, we still can come out the DT in parallel, and make
> > > > > sure what information is needed by m-mode firmware. OTOH, we also need
> > > > > a Linux driver which has to parse the dtb when we build Linux as nommu
> > > > > and run it on m-mode privilege, so if we could define the DT for PMU
> > > > > node, we can go the driver for nommu first (the implementation would
> > > > > also consider HW counter, SW counter together). I think you have got
> > > > > the picture about m-mode firmware, it would be great to start to
> > > > > collect the idea of DT.
> > > >
> > > > The RISC-V PMU DT binding is not just for Linux. Even M-mode
> > > > runtime firmware (OpenSBI) will be using this binding.
> > > >
> > > > The DT bindings you are proposing is conflicting with the
> > > > expected RISC-V PMU DT bindings:
> > > > 1. The event-map should be mapping SBI PMU event_idx
> > > > to HARDWARE HPMEVENT value whereas this binding
> > > > defines event-map as mapping Linux perf event types to
> > > > HPMEVENT value.
> > >
> > > It is more suitable by using Linux perf event types directly here. On
> > > nommu Linux, it doesn't have additional m-mode firmware, so it doesn't
> > > have the rule like SMI PMU event_idx. Unless we apply the same rule in
> > > nommu pmu drivers as well, it doesn't need to translate the event_idx
> > > to Linux perf event types in Linux nommu driver.
> >
> > This means you will end-up with a separate driver and binding
> > for NoMMU Linux as compared to S-mode Linux. The PMU DT
> > node for M-mode and S-mode will also look totally different.
>
> No, we would have only one driver. The initial idea is that we
> separate the implementation for HW counter, SW counter and nommu
> respectively, they could register callback functions to the main perf
> driver. Ideally, I hope we can have only one binding for m-mode and
> s-mode, these properties are optional, so in the s-mode case, we don't
> need to give the properties which are needed by m-mode.
Let's focus on only one DT binding and driver for RISC-V PMU which
will work for both M-mode and S-mode.
In future, DT bindings document can clearly state that for event-map
properties are only for NoMMU (i.e. M-mode) Linux.
>
> >
> > For S-mode case, we only need compatible string and optional
> > interrupts DT property.
> >
> > >
> > > > 2. Each HPMCOUNTER can support a different set of events
> > > > so this DT bindings won't work for it.
> > >
> > > There is a property "riscv,hpmcntr-of-event" to indicate which
> > > counters can serve the specific event, if we don't list the particular
> > > event in this property, we view this event could be served by all
> > > counters.
> >
> > The name of this property is totally misleading.
>
> Yes, I also struggled with the name for it, welcome to improve it if
> you have a good idea.
>
> >
> > >
> > > > 3. The SBI PMU driver can have optional edge-triggered
> > > > per-HART interrupts for overflow detection. This DT bindings
> > > > does not consider this as well.
> > > >
> > >
> > > We don't have an overflow interrupt mechanism in RISC-V spec now, we
> > > should postpone and add the properties of interrupt stuff after the
> > > mechanism is standardized.
> >
> > I totally disagree. The RISC-V spec does not prevent any RISC-V
> > implementation from having per-HART edge triggered interrupts
> > which are routed through PLIC.
> >
>
> Yes, I know, my point is that we don't have a standard way to
> implement overflow interrupt mechanism for PMU yet, it is being
> discussed, so even through we give the interrupt properties, it
> doesn't seem to be used for current driver.
Based on discussions on RISC-V privilege mailing list, it seems
we won't have major changes in current PMU CSRs. Mostly
one optional hpmoverflow CSR would be added to handle
overflow interrupts.
This means the RISC-V PMU driver will certainly need an optional
interrupts property.
The RISC-V PMU driver is now gated by SBI PMU extension
and improved PMU support in RISC-V privilege spec.
>
> > (Please see the discussion on RISC-V privilege mailing list).
> >
> > That's why RISC-V PMU DT bindings should have an optional
> > interrupts DT property.
> >
> > >
> > > > The SBI PMU event_idx definition is not yet finalized. Please
> > > > wait for SBI PMU extension to be finalized.
> > > >
> > >
> > > As I mentioned above, it need to be discussed whether we should do id
> > > translation in Linux nommu driver.
> >
> > All this implies that you want two separate drivers, bindings and DT
> > nodes for M-mode and S-mode cases.
>
> No, I hope we only have one driver and one binding, so we should come
> out a common way for them.
>
> >
> > I disagree with this approach. This will lead to a lot of duplication
> > and confusion.
> >
> > Better approach would be to have SBI PMU event_idx to HPMEVENT
> > CSR value mappings in the DT properties. For M-mode (NoMMU) linux,
> > we convert the Linux perf event to SBI PMU event_idx and then to the
> > HPMEVENT value using event-map. For S-mode (MMU) linux, we can
> > convert the Linux perf event to the SBI PMU event_idx and then use
> > this SBI PMU event_idx with SBI_PMU_SET_EVENT call.
>
> It is trade off, we can let m-mode driver convert the event between
> SBI event_idx and Linux perf event, we also can let opensbi convert
> the event between Linux perf event and SBI event_idx, But I think your
> approach is also good to me, because we can make s-mode and m-mode
> driver use identical way to get the actual value of hpmcounter, even
> m-mode driver doesn't need to give an abstract level for SBI PMU
> event_idx
I think having HW event to SBI event_idx mapping in DT bindings is
the way to go because this will avoid making DT bindings Linux specific
and other OSes (e.g. FreeBSD) can also use the same DT bindings.
>
> >
> > The optional interrupts DT property should be available in both M-mode
> > (NoMMU) and S-mode cases so that RISC-V implementation can at
> > least provide edge-triggered per-HART interrupts which are routed
> > through PLIC.
> >
> > Please wait for the SBI PMU extension to be finalized. Once it is
> > finalized, we can come-up with a common RISC-V PMU driver which
> > works for both M-mode (NoMMU) Linux and S-mode Linux.
> >
>
> Ok, fine, though I think DT and SBI PMU extension can be in parallel,
> let's postpone the DT definition.
We will try to close the SBI PMU extension sooner. Even if SBI v0.3 spe
release with SBI PMU extension happens much later. The OpenSBI PMU
extension efforts are also gated on SBI PMU extension.
Regards,
Anup
More information about the linux-riscv
mailing list