On Thu, Dec 22, 2022 at 11:24 AM Conor Dooley <conor@xxxxxxxxxx> wrote: > > Hey Rob, > > On Thu, Dec 22, 2022 at 12:06:27PM -0600, Rob Herring wrote: > > On Wed, Dec 21, 2022 at 02:15:49PM +0000, Conor Dooley wrote: > > > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > > > > > > The SBI PMU extension requires a firmware to be aware of the event to > > > counter/mhpmevent mappings supported by the hardware. OpenSBI may use > > > DeviceTree to describe the PMU mappings. This binding is currently > > > described in markdown in OpenSBI (since v1.0 in Dec 2021) & used by QEMU > > > since v7.2.0. > > > > > > Import the binding for use while validating dtb dumps from QEMU and > > > upcoming hardware (eg JH7110 SoC) that will make use of the event > > > mapping. > > > > > > Link: https://github.com/riscv-software-src/opensbi/blob/master/docs/pmu_support.md > > > Co-developed-by: Atish Patra <atishp@xxxxxxxxxxxx> > > > Signed-off-by: Atish Patra <atishp@xxxxxxxxxxxx> > > > Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > > > --- > > > I asked Rob on IRC about these bindings a few weeks ago & he said he > > > would be willing to take them. I have modified wording slightly in the > > > descriptions, but have mostly left things as close to the OpenSBI > > > documentation as possible. > > > > Please CC the perf maintainers. Might be crickets, but so they at least > > have a chance to see it. > > Yah, I was kinda unsure who to CC. It does list them as being > specifically ARM so I probably made the wrong choice about inclusion. > I've added them now. > > > > I'm not super sure about what I've done with the properties being > > > correct type wise, I went digging in bindings and am sorta using the > > > first thing that "fit". > > > > > > Since you wrote the md doc Atish, I put your co-developed-by. OpenSBI > > > is BSD-2-Clause licensed so I am also unsure as to what license I can > > > use for this binding since that's where I took it from. > > > --- > > > .../devicetree/bindings/perf/riscv,pmu.yaml | 158 ++++++++++++++++++ > > > 1 file changed, 158 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..d65f937680af > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/perf/riscv,pmu.yaml > > > @@ -0,0 +1,158 @@ > > > +# SPDX-License-Identifier: BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/perf/riscv,pmu.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: RISC-V SBI PMU events > > > + > > > +maintainers: > > > + - Atish Patra <atishp@xxxxxxxxxxxx> > > > + > > > +description: | > > > + SBI PMU extension supports allow supervisor software to configure, start & > > > + stop any performance counter at anytime. Thus, a user can leverage full > > > + capability of performance analysis tools such as perf if the SBI PMU > > > + extension is enabled. The OpenSBI implementation makes the following > > > + assumptions about the hardware platform: > > > + MCOUNTINHIBIT CSR must be implemented in the hardware. Otherwise, the SBI > > > + PMU extension will not be enabled. > > > + This is no longer true since OpenSBI commit b28f070. We should remove this from OpenSBI docs as well. > > > + The platform must provide information about PMU event to counter mapping > > > + via device tree or platform specific hooks. Otherwise, the SBI PMU > > > + extension will not be enabled. > > > + > > > + The platforms should provide information about the PMU event selector > > > + values that should be encoded in the expected value of MHPMEVENTx while > > > + configuring MHPMCOUNTERx for that specific event. This can be done via a > > > + device tree or platform specific hooks. The exact value to be written to > > > + the MHPMEVENTx is completely dependent on the platform. The generic > > > + platform writes the zero-extended event_idx as the expected value for > > > + hardware cache/generic events as suggested by the SBI specification. > > > + But the larger question is these are OpenSBI implementation specific assumptions. Should it be included in the DT binding ? > > > +properties: > > > + compatible: > > > + const: riscv,pmu > > > + > > > + riscv,event-to-mhpmevent: > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > + description: > > > + Represents an ONE-to-ONE mapping between a PMU event and the event > > > + selector value that platform expects to be written to the MHPMEVENTx CSR > > > + for that event. > > > + The mapping is encoded in an array format where each row represents an > > > > s/array/matrix/ > > > > > + event. The first element represents the event idx while the second & > > > + third elements represent the event selector value that should be encoded > > > + in the expected value to be written in MHPMEVENTx. > > > + This property shouldn't encode any raw hardware event. > > > + minItems: 1 > > > + maxItems: 255 > > I copied these 255s from dt-schema somewhere as a sane max. > Atish, is there a cap here or should we allow as many as someone likes? > The md binding doesn't mention any limits - is it in the SBI spec? > The strongest wording I saw there was "limited" & event idx is 20 bits > wide as per the spec [0]. Does that make 2^20 the hard limit here? > This is for hardware & cache events. The total number of events defined there are 52 (10 HW + 42 Cache) events. So 255 is a sane max that provides enough room for expansion in future if required. > 0 - https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/riscv-sbi.adoc#11-performance-monitoring-unit-extension-eid-0x504d55-pmu > > > > + items: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > Huh? A property can only have 1 type. I wonder what the tools do with > > this... > > I suppose I left this over (by accident) from when I had it arranged > slightly differently. I guess I never actually noticed as the tools > don't appear to complain. Now dropped :) > > > > + maxItems: 3 > > > > Better to do: > > > > items: > > - description: what's in the 1st word > > - description: what's in the 2nd word > > - description: what's in the 3rd word > > > > And rework the overall description to not say the same thing. > > Yah, good idea. I'll do that for the next version. > > > > + riscv,event-to-mhpmcounters: > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > + description: > > > + Represents a MANY-to-MANY mapping between a range of events and all the > > > + MHPMCOUNTERx in a bitmap format that can be used to monitor these range > > > + of events. The information is encoded in an array format where each > > > + row represents a certain range of events and corresponding counters. > > > + The first element represents starting of the pmu event id and 2nd column > > > + represents the end of the pmu event id. The third element represent a > > > + bitmap of all the MHPMCOUNTERx. > > > + This property is mandatory if event-to-mhpmevent is present. Otherwise, > > > + it can be omitted. > > > > No need to state this in freeform text, use 'dependencies'. > > Oh! I didn't know that that existed. I was going to go and do some sort > of required properties dance but perhaps that's not needed now (at least > to the same extent). > > > > + This property shouldn't encode any raw event. > > > + minItems: 1 > > > + maxItems: 255 > > > + items: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + maxItems: 3 > > > + > > > + riscv,raw-event-to-mhpmcounters: > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > + description: > > > + Represents an ONE-to-MANY or MANY-to-MANY mapping between the rawevent(s) > > > + and all the MHPMCOUNTERx in a bitmap format that can be used to monitor > > > + that raw event. > > > + The encoding of the raw events are platform specific. The information is > > > + encoded in an array format where each row represents the specific raw > > > + event(s). The first element is a 64-bit match value where the invariant > > > + bits of range of events are set. The second element is a 64-bit mask that > > > + will have all the variant bits of the range of events cleared. All other > > > + bits should be set in the mask. The third element is a 32-bit value to > > > + represent bitmap of all MHPMCOUNTERx that can monitor these set of > > > + event(s). If a platform directly encodes each raw PMU event as a unique > > > + ID, the value of select_mask must be 0xffffffff_ffffffff. > > > + minItems: 1 > > > + maxItems: 255 > > > + items: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + maxItems: 5 > > > + > > > +required: > > > + - compatible > > > > I assume at least one of the other properties must be present? > > Atish: (and +CC the OpenSBI list) > Which properties are actually needed here? Or are any? The md binding > in the OpenSBI sources doesn't seem to require anything other than the > compatible? > That's true. Opensbi allows the platform to define functions to provide the mapping. Now that's an OpenSBI implementation thing. Other implementations may choose to use it differently. > Thanks Rob, > Conor. >
