Re: [v4 06/11] dt-binding: pmu: Add RISC-V PMU DT bindings

[Jessica Clarke <jrtc27@jrtc27.com>]

On Mon, Oct 25, 2021 at 12:53:45PM -0700, Atish Patra wrote:
> This patch adds the DT bindings for RISC-V PMU driver. It also defines
> the interrupt related properties to allow counter overflow interrupt.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  .../devicetree/bindings/perf/riscv,pmu.yaml   | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/riscv,pmu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/riscv,pmu.yaml b/Documentation/devicetree/bindings/perf/riscv,pmu.yaml
> new file mode 100644
> index 000000000000..497caad63f16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/riscv,pmu.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pmu/riscv,pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V PMU
> +
> +maintainers:
> +  - Atish Patra <atish.patra@wdc.com>
> +
> +description:
> +  The "Sscofpmf" extension allows the RISC-V PMU counters to overflow and
> +  generate a local interrupt so that event sampling can be done from user-space.
> +  The above said ISA extension is an optional extension to maintain backward
> +  compatibility and will be included in privilege specification v1.12 . That's
> +  why the interrupt property is marked as optional. The platforms with sscofpmf
> +  extension should add this property to enable event sampling.
> +  The device tree node with the compatible string is mandatory for any platform
> +  that wants to use pmu counter start/stop methods using SBI PMU extension.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - riscv,pmu

This is conflating the Sscofpmf extension with the SBI PMU interface;
the former is what the hardware supports, the latter is what the
firmware exposes. The SBI interface exists today and does not require
overflow interrupts to be supported, so there needs to be a distinction
between that case and the case where Sscofpmf is supported in both
hardware and the SBI implementation, which probably means having a
second compatible string for that case that also includes the generic
SBI PMU interface as a fallback compatible string.

Secondly, I do not think this is the right name for this. The riscv,pmu
compatible string (or anything of that nature) should be reserved for
*hardware* that provides usable performance monitoring features to an
OS. This is not that, this is the SBI interface that requires an OS to
make firmware calls for any starting, stopping or configuring of a
counter, which results in an even greater probe effect than is already
present with frameworks like FreeBSD's HWPMC or Linux's perf (I don't
know how the two compare on that front, but I imagine Linux is similar
to FreeBSD). This should have SBI in the name so that it doesn't get in
the way of real performance monitoring support once the architecture is
finally mature enough to have S-mode-configurable counters and a
standardised set of common events like pretty much every other
architecture.

Also I do not like the use of PMU, since that is Arm's terminology,
whereas RISC-V uses HPM, but you've already defined the SBI interface as
being PMU so I guess that ship has sailed.

Jess

> +
> +    description:
> +      Should be "riscv,pmu".
> +
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 4095
> +
> +additionalProperties: false
> +
> +required:
> +  - None
> +optional:
> +  - compatible
> +  - interrupts-extended
> +
> +examples:
> +  - |
> +    pmu {
> +      compatible = "riscv,pmu";
> +      interrupts-extended = <&cpu0intc 13>,
> +                            <&cpu1intc 13>,
> +                            <&cpu2intc 13>,
> +                            <&cpu3intc 13>;
> +    };
> +...
> -- 
> 2.31.1