Re: [RFC PATCH 1/6] dt-bindings: riscv: Add YAML documentation for PMU

[Zong Li]

On Mon, Jun 29, 2020 at 4:31 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
>
> On Mon, Jun 29, 2020 at 12:06 PM Zong Li <zong.li@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 29, 2020 at 12:38 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 9:58 AM Zong Li <zong.li@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 12:09 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 8:49 AM Zong Li <zong.li@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Add device tree bindings for performance monitor unit. And it passes the
> > > > > > dt_binding_check verification.
> > > > > >
> > > > > > Signed-off-by: Zong Li <zong.li@xxxxxxxxxx>
> > > > > > ---
> > > > > >  .../devicetree/bindings/riscv/pmu.yaml        | 59 +++++++++++++++++++
> > > > > >  1 file changed, 59 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..f55ccbc6c685
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/riscv/pmu.yaml
> > > > > > @@ -0,0 +1,59 @@
> > > > > > +# 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@xxxxxxxxxx>
> > > > > > +  - Paul Walmsley <paul.walmsley@xxxxxxxxxx>
> > > > > > +  - Palmer Dabbelt <palmer@xxxxxxxxxxx>
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    items:
> > > > > > +      - const: riscv,pmu
> > > > > > +
> > > > > > +  riscv,width-base-cntr:
> > > > > > +    description: The width of cycle and instret CSRs.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > +
> > > > > > +  riscv,width-event-cntr:
> > > > > > +    description: The width of hpmcounter CSRs.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > >
> > > > > The terms "base" and "event" is confusing because
> > > > > we only have counters with no interrupt associated with it.
> > > > >
> > > > > The RISC-V spec defines 3 counters and rest are all
> > > > > implementation specific counters.
> > > >
> > > > As I know, there are 2 counters of spec definition: cycle and instret.
> > > > What is the 3rd counter you mentioned?
> > >
> > > TIME is a counter CSR.
> > >
> > > >
> > > > >
> > > > > I suggest using the terms "spec counters" and "impl counters"
> > > > > instead of "base counters" and "event counters".
> > > >
> > > > OK, they are good to me. Let me change it.
> > > >
> > > >
> > > > >
> > > > > Further, "riscv,width" properties are redundant because
> > > > > RISC-V spec clearly tells that counters are 64bit for both
> > > > > RV32 and RV64.
> > > > >
> >
> > Sorry for the lost replying. The maximum length of counters is 64, but
> > it doesn't require to implement all bits. A real case is that
> > unleashed board only implements 40 bit for mhpmcounters.
>
> The "3.1.11 Hardware Performance Monitor" clearly states that
> all counters are 64bit
>

In the privileged spec, 3.1.11 section said, "The mhpmcounters are
WARL registers that support up to 64 bits of precision on RV32 and
RV64".

It seems to me that WARL implies the size of registers could be
variable, and support up to 64 bits as maximum size.

> To take care of the unleashed board, the "riscv,width-xyz" DT properties
> should be optional. Whenever these properties are not present, we
> should assume 64bit counter width.
>
> >
> > > > > > +
> > > > > > +  riscv,n-event-cntr:
> > > > > > +    description: The number of hpmcounter CSRs.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > +
> > > > > > +  riscv,hw-event-map:
> > > > > > +    description: The mapping of generic hardware events. Default is no mapping.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > +
> > > > > > +  riscv,hw-cache-event-map:
> > > > > > +    description: The mapping of generic hardware cache events.
> > > > > > +      Default is no mapping.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > > > +
> > > > > > +required:
> > > > > > +  - compatible
> > > > > > +  - riscv,width-base-cntr
> > > > > > +  - riscv,width-event-cntr
> > > > > > +  - riscv,n-event-cntr
> > > > > > +
> > > > > > +additionalProperties: false
> > > > > > +
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    pmu {
> > > > > > +      compatible = "riscv,pmu";
> > > > > > +      riscv,width-base-cntr = <64>;
> > > > > > +      riscv,width-event-cntr = <40>;
> > > > > > +      riscv,n-event-cntr = <2>;
> > > > > > +      riscv,hw-event-map = <0x0 0x0 0x1 0x1 0x3 0x0202 0x4 0x4000>;
> > > > > > +      riscv,hw-cache-event-map = <0x010201 0x0102 0x010204 0x0802>;
> > > > > > +    };
> > > > > > +
> > > > > > +...
> > > > > > --
> > > > > > 2.27.0
> > > > > >
> > > > >
> > > > > Regards,
> > > > > Anup
>
> Regards,
> Anup