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

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

[Zong Li]

Add device tree bindings for performance monitor unit. It passes the
dt_binding_check verification.

Signed-off-by: Zong Li <zong.li@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@sifive.com>
+  - Paul Walmsley <paul.walmsley@sifive.com>
+  - Palmer Dabbelt <palmer@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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Anup Patel]

On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> +  - Paul Walmsley <paul.walmsley@sifive.com>
> +  - Palmer Dabbelt <palmer@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.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Zong Li]

On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > +  - Palmer Dabbelt <palmer@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.

> Regards,
> Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Anup Patel]

On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > +  - Palmer Dabbelt <palmer@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.
2. Each HPMCOUNTER can support a different set of events
so this DT bindings won't work for it.
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.

The SBI PMU event_idx definition is not yet finalized. Please
wait for SBI PMU extension to be finalized.

Please avoid adding DT bindings without SBI PMU extension
being finalized.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Zong Li]

On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > > +  - Palmer Dabbelt <palmer@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.

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

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

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

> Please avoid adding DT bindings without SBI PMU extension
> being finalized.
>
> Regards,
> Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Anup Patel]

On Tue, Jul 28, 2020 at 9:38 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
> > >
> > > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> > > >
> > > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > > > +  - Palmer Dabbelt <palmer@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.

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.

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

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

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.

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.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Atish Patra]

On Tue, Jul 28, 2020 at 5:10 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jul 28, 2020 at 9:38 AM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
> > > >
> > > > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> > > > >
> > > > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > > > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > > > > +  - Palmer Dabbelt <palmer@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.
>
> 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.
>
> >
> > > 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.
>
> (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.
>
> 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.
>

I think the bigger question is : Do we even need perf events for NoMMU Linux ?
I am not sure if kendryte supports it. Even if it supports it, it
doesn't have enough memory
to support decent userspace. We are talking about doing a performance
analysis on Kendryte.

IMHO, it is premature to add a driver for something that we may never use.

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

Do you want to discuss the SBI PMU proposal in the next unix platform
working group meeting ?

> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Zong Li]

On Tue, Jul 28, 2020 at 8:10 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jul 28, 2020 at 9:38 AM Zong Li <zong.li@sifive.com> wrote:
> >
> > On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
> > > >
> > > > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> > > > >
> > > > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > > > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > > > > +  - Palmer Dabbelt <palmer@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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Anup Patel]

On Thu, Jul 30, 2020 at 11:23 AM Zong Li <zong.li@sifive.com> wrote:
>
> On Tue, Jul 28, 2020 at 8:10 PM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Tue, Jul 28, 2020 at 9:38 AM Zong Li <zong.li@sifive.com> wrote:
> > >
> > > On Tue, Jul 28, 2020 at 11:27 AM Anup Patel <anup@brainfault.org> wrote:
> > > >
> > > > On Tue, Jul 28, 2020 at 8:35 AM Zong Li <zong.li@sifive.com> wrote:
> > > > >
> > > > > On Mon, Jul 27, 2020 at 9:13 PM Anup Patel <anup@brainfault.org> wrote:
> > > > > >
> > > > > > On Mon, Jul 27, 2020 at 1:57 PM Zong Li <zong.li@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@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@sifive.com>
> > > > > > > +  - Paul Walmsley <paul.walmsley@sifive.com>
> > > > > > > +  - Palmer Dabbelt <palmer@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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv