[RFC PATCH] dt-bindings: riscv: Add YAML documentation for PMU

[Zong Li]

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.

>
> 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.

> (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

>
> 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.

> Regards,
> Anup