mbox series

[v2,0/4] Introduce pmu-events support for HiFive Unmatched

Message ID 20211116154812.17008-1-joao.mario@tecnico.ulisboa.pt (mailing list archive)
Headers show
Series Introduce pmu-events support for HiFive Unmatched | expand

Message

João Mário Domingos Nov. 16, 2021, 3:48 p.m. UTC
This series of patches introduces support for the RISC-V PMU identification and raw events matching between perf and the PMU.
The HiFive Unmatched board can now use all the counters with named events.

The work in these patches is completelly and directly dependent on the proposed SBI PMU patch [4], by Atish Patra.

This series has been tested in the HiFive Unmatched board.
OpenSBI[1] and U-Boot[2] patches are required to test it on the board, alongside with the series of patches that introduce RISC-V Perf support with the SBI PMU and sscofpmf extension [3-4].
The U-Boot fu740 dts [5] must be updated, may be fixed in future U-BOOT versions, with the events that will be tested, this comprehends changes to the riscv,raw-event-to-mhpmcounters entry. Also, an OpenSBI patch [6] proposes bitfield awareness for the DT raw-events, this will simplify, and improve, events description in the DT file. 

Here is the output of perf list and perf stat with all the available unmatched events monitoring a stress-ng trignometric stressor.

HiFive Unmatched:
=================
perf list pmu

  instructions:
    atomic_memory_retired
         [Atomic memory operation retired]
    conditional_branch_retired
         [Conditional branch retired]
    exception_taken
         [Exception taken]
    fp_addition_retired
         [Floating-point addition retired]
    fp_div_sqrt_retired
         [Floating-point division or square-root retired]
    fp_fusedmadd_retired
         [Floating-point fused multiply-add retired]
    fp_load_retired
         [Floating-point load instruction retired]
    fp_multiplication_retired
         [Floating-point multiplication retired]
    fp_store_retired
         [Floating-point store instruction retired]
    integer_arithmetic_retired
         [Integer arithmetic instruction retired]
    integer_division_retired
         [Integer division instruction retired]
    integer_load_retired
         [Integer load instruction retired]
    integer_multiplication_retired
         [Integer multiplication instruction retired]
    integer_store_retired
         [Integer store instruction retired]
    jal_instruction_retired
         [JAL instruction retired]
    jalr_instruction_retired
         [JALR instruction retired]
    other_fp_retired
         [Other floating-point instruction retired]
    system_instruction_retired
         [System instruction retired]

  memory:
    data_tlb_miss
         [Data TLB miss]
    dcache_miss_mmio_accesses
         [Data cache miss or memory-mapped I/O access]
    dcache_writeback
         [Data cache write-back]
    icache_retired
         [Instruction cache miss]
    inst_tlb_miss
         [Instruction TLB miss]
    utlb_miss
         [UTLB miss]

  microarch:
    addressgen_interlock
         [Address-generation interlock]
    branch_direction_misprediction
         [Branch direction misprediction]
    branch_target_misprediction
         [Branch/jump target misprediction]
    csr_read_interlock
         [CSR read interlock]
    dcache_dtim_busy
         [Data cache/DTIM busy]
    fp_interlock
         [Floating-point interlock]
    icache_itim_busy
         [Instruction cache/ITIM busy]
    integer_multiplication_interlock
         [Integer multiplication interlock]
    longlat_interlock
         [Long-latency interlock]
    pipe_flush_csr_write
         [Pipeline flush from CSR write]
    pipe_flush_other_event
         [Pipeline flush from other event]


perf stat -eexception_taken,integer_load_retired,integer_store_retired,atomic_memory_retired,system_instruction_retired,integer_arithmetic_retired,conditional_branch_retired,jal_instruction_retired,jalr_instruction_retired,integer_multiplication_retired,integer_division_retired,fp_load_retired,fp_store_retired,fp_addition_retired,fp_multiplication_retired,fp_fusedmadd_retired,fp_div_sqrt_retired,other_fp_retired,data_tlb_miss,dcache_miss_mmio_accesses,dcache_writeback,icache_retired,inst_tlb_miss,utlb_miss,addressgen_interlock,longlat_interlock,csr_read_interlock,icache_itim_busy,dcache_dtim_busy,branch_direction_misprediction,branch_target_misprediction,pipe_flush_csr_write,pipe_flush_other_event,integer_multiplication_interlock,fp_interlock stress-ng --cpu 1 --cpu-method trig -t 60s
stress-ng: info:  [500] setting to a 60 second run per stressor
stress-ng: info:  [500] dispatching hogs: 1 cpu
stress-ng: info:  [500] successful run completed in 60.02s (1 min, 0.02 secs)

 Performance counter stats for 'stress-ng --cpu 1 --cpu-method trig -t 60s':

            233185      exception_taken                                               (5.72%)
        5010843878      integer_load_retired                                          (5.73%)
        4437123760      integer_store_retired                                         (5.73%)
            875636      atomic_memory_retired                                         (5.72%)
         787401517      system_instruction_retired                                     (5.73%)
       58176497169      integer_arithmetic_retired                                     (5.74%)
        9689155944      conditional_branch_retired                                     (5.73%)
        2141143732      jal_instruction_retired                                       (5.72%)
         624939326      jalr_instruction_retired                                      (5.72%)
        1900272300      integer_multiplication_retired                                     (5.72%)
          29231344      integer_division_retired                                      (5.72%)
        1672274835      fp_load_retired                                               (5.73%)
         592215149      fp_store_retired                                              (5.73%)
          13592616      fp_addition_retired                                           (5.73%)
          13597374      fp_multiplication_retired                                     (5.72%)
        1331957808      fp_fusedmadd_retired                                          (5.72%)
         592410829      fp_div_sqrt_retired                                           (5.72%)
         227334317      other_fp_retired                                              (5.72%)
          95772283      data_tlb_miss                                                 (5.72%)
           6291088      dcache_miss_mmio_accesses                                     (5.72%)
             55591      dcache_writeback                                              (5.72%)
          47394708      icache_retired                                                (5.72%)
          62778259      inst_tlb_miss                                                 (5.72%)
          10159938      utlb_miss                                                     (5.72%)
         424446212      addressgen_interlock                                          (5.72%)
         849061809      longlat_interlock                                             (5.72%)
                 0      csr_read_interlock                                            (5.71%)
       16924613138      icache_itim_busy                                              (5.71%)
         204163844      dcache_dtim_busy                                              (5.70%)
         175163620      branch_direction_misprediction                                     (5.72%)
         202874026      branch_target_misprediction                                     (5.71%)
         433228932      pipe_flush_csr_write                                          (5.71%)
                 0      pipe_flush_other_event                                        (5.71%)
         429384915      integer_multiplication_interlock                                     (5.71%)
        8635530214      fp_interlock                                                  (5.71%)

      60.044837787 seconds time elapsed

      60.001431000 seconds user
       0.033660000 seconds sys


[1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2 
[2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
[3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
[4] http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
[5] https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
[6] https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/

Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>

This work was developed at INESC-ID, Instituto Superior Técnico, Universidade de Lisboa.

---
Changes in v2:
  - Fix compilation errors and warnings
  - Remove space idents
  - Correct formatting

João Mário Domingos (4):
  RISC-V: Create unique identification for SoC PMU
  RISC-V: Support CPUID for risc-v in perf
  RISC-V: Added generic pmu-events mapfile
  RISC-V: Added HiFive Unmatched PMU events

 arch/riscv/kernel/sbi.c                       |  3 +
 drivers/perf/riscv_pmu.c                      | 18 ++++
 drivers/perf/riscv_pmu_sbi.c                  | 47 ++++++++++
 tools/perf/arch/riscv/util/Build              |  1 +
 tools/perf/arch/riscv/util/header.c           | 66 +++++++++++++
 tools/perf/pmu-events/arch/riscv/mapfile.csv  | 15 +++
 .../pmu-events/arch/riscv/riscv-generic.json  | 20 ++++
 .../arch/riscv/sifive/u74/instructions.json   | 92 +++++++++++++++++++
 .../arch/riscv/sifive/u74/memory.json         | 32 +++++++
 .../arch/riscv/sifive/u74/microarch.json      | 57 ++++++++++++
 10 files changed, 351 insertions(+)
 create mode 100644 tools/perf/arch/riscv/util/header.c
 create mode 100644 tools/perf/pmu-events/arch/riscv/mapfile.csv
 create mode 100644 tools/perf/pmu-events/arch/riscv/riscv-generic.json
 create mode 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/instructions.json
 create mode 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/memory.json
 create mode 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/microarch.json

Comments

Nikita Shubin Nov. 17, 2021, 12:25 p.m. UTC | #1
On Tue, 16 Nov 2021 15:48:08 +0000
João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:

Hello Mario!

Thank you for your patch series.

I have reproduced your test with some u-boot dts tinkering, and got
similar results.

However,

> 
> [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2 

OpenSBI sscofpmf has been merged.

> [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
> [5]
> https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi

Is missing the adaptation for OpenSBI bitmap patch for
"raw-event-to-mhpmcounters".

> [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4

The link is broken.

> [4]
> http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html

> [6]
> https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
> 

There is a version 2 submitted, and it won't apply as require rebasing
and some renaming.

Please share your u-boot dts changes - they should be small and provide
a common base for this series.

Tested-by: Nikita Shubin <n.shubin@yadro.com>

> Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> 
> This work was developed at INESC-ID, Instituto Superior Técnico,
> Universidade de Lisboa.
> 
> ---
> Changes in v2:
>   - Fix compilation errors and warnings
>   - Remove space idents
>   - Correct formatting
> 
> João Mário Domingos (4):
>   RISC-V: Create unique identification for SoC PMU
>   RISC-V: Support CPUID for risc-v in perf
>   RISC-V: Added generic pmu-events mapfile
>   RISC-V: Added HiFive Unmatched PMU events
> 
>  arch/riscv/kernel/sbi.c                       |  3 +
>  drivers/perf/riscv_pmu.c                      | 18 ++++
>  drivers/perf/riscv_pmu_sbi.c                  | 47 ++++++++++
>  tools/perf/arch/riscv/util/Build              |  1 +
>  tools/perf/arch/riscv/util/header.c           | 66 +++++++++++++
>  tools/perf/pmu-events/arch/riscv/mapfile.csv  | 15 +++
>  .../pmu-events/arch/riscv/riscv-generic.json  | 20 ++++
>  .../arch/riscv/sifive/u74/instructions.json   | 92
> +++++++++++++++++++ .../arch/riscv/sifive/u74/memory.json         |
> 32 +++++++ .../arch/riscv/sifive/u74/microarch.json      | 57
> ++++++++++++ 10 files changed, 351 insertions(+)
>  create mode 100644 tools/perf/arch/riscv/util/header.c
>  create mode 100644 tools/perf/pmu-events/arch/riscv/mapfile.csv
>  create mode 100644
> tools/perf/pmu-events/arch/riscv/riscv-generic.json create mode
> 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/instructions.json
> create mode 100644
> tools/perf/pmu-events/arch/riscv/sifive/u74/memory.json create mode
> 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/microarch.json
>
Atish Patra Nov. 18, 2021, 8 a.m. UTC | #2
On Wed, Nov 17, 2021 at 4:25 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> On Tue, 16 Nov 2021 15:48:08 +0000
> João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
>
> Hello Mario!
>
> Thank you for your patch series.
>
> I have reproduced your test with some u-boot dts tinkering, and got
> similar results.
>
> However,
>
> >
> > [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2
>
> OpenSBI sscofpmf has been merged.
>
> > [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
> > [5]
> > https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
>
> Is missing the adaptation for OpenSBI bitmap patch for
> "raw-event-to-mhpmcounters".

My patch was just an example and predates before the bitmap patch
posted by Vincent.
I will update the U-boot patch along with the next kernel version.

>
> > [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
>
> The link is broken.
>

Sorry. This should be
https://github.com/atishp04/linux/tree/sbi_pmu_v4

> > [4]
> > http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
>
> > [6]
> > https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
> >
>
> There is a version 2 submitted, and it won't apply as require rebasing
> and some renaming.
>
> Please share your u-boot dts changes - they should be small and provide
> a common base for this series.
>

Yes. That would be really helpful to have a DT path with all the
entries for easier testing.


> Tested-by: Nikita Shubin <n.shubin@yadro.com>
>
> > Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> >
> > This work was developed at INESC-ID, Instituto Superior Técnico,
> > Universidade de Lisboa.
> >
> > ---
> > Changes in v2:
> >   - Fix compilation errors and warnings
> >   - Remove space idents
> >   - Correct formatting



> >
> > João Mário Domingos (4):
> >   RISC-V: Create unique identification for SoC PMU
> >   RISC-V: Support CPUID for risc-v in perf
> >   RISC-V: Added generic pmu-events mapfile
> >   RISC-V: Added HiFive Unmatched PMU events
> >
> >  arch/riscv/kernel/sbi.c                       |  3 +
> >  drivers/perf/riscv_pmu.c                      | 18 ++++
> >  drivers/perf/riscv_pmu_sbi.c                  | 47 ++++++++++
> >  tools/perf/arch/riscv/util/Build              |  1 +
> >  tools/perf/arch/riscv/util/header.c           | 66 +++++++++++++
> >  tools/perf/pmu-events/arch/riscv/mapfile.csv  | 15 +++
> >  .../pmu-events/arch/riscv/riscv-generic.json  | 20 ++++
> >  .../arch/riscv/sifive/u74/instructions.json   | 92
> > +++++++++++++++++++ .../arch/riscv/sifive/u74/memory.json         |
> > 32 +++++++ .../arch/riscv/sifive/u74/microarch.json      | 57
> > ++++++++++++ 10 files changed, 351 insertions(+)
> >  create mode 100644 tools/perf/arch/riscv/util/header.c
> >  create mode 100644 tools/perf/pmu-events/arch/riscv/mapfile.csv
> >  create mode 100644
> > tools/perf/pmu-events/arch/riscv/riscv-generic.json create mode
> > 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/instructions.json
> > create mode 100644
> > tools/perf/pmu-events/arch/riscv/sifive/u74/memory.json create mode
> > 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/microarch.json
> >
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
João Mário Domingos Nov. 22, 2021, 2:57 p.m. UTC | #3
On Thu, Nov 18, 2021 at 12:00:09AM -0800, Atish Patra wrote:
> On Wed, Nov 17, 2021 at 4:25 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> >
> > On Tue, 16 Nov 2021 15:48:08 +0000
> > João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> >
> > Hello Mario!
> >
> > Thank you for your patch series.
> >
> > I have reproduced your test with some u-boot dts tinkering, and got
> > similar results.
> >
> > However,
> >
> > >
> > > [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2
> >
> > OpenSBI sscofpmf has been merged.
> >
> > > [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
> > > [5]
> > > https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
> >
> > Is missing the adaptation for OpenSBI bitmap patch for
> > "raw-event-to-mhpmcounters".
> 
> My patch was just an example and predates before the bitmap patch
> posted by Vincent.
> I will update the U-boot patch along with the next kernel version.
> 
> >
> > > [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
> >
> > The link is broken.
> >
> 
> Sorry. This should be
> https://github.com/atishp04/linux/tree/sbi_pmu_v4
> 
> > > [4]
> > > http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
> >
> > > [6]
> > > https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
> > >
> >
> > There is a version 2 submitted, and it won't apply as require rebasing
> > and some renaming.
> >
> > Please share your u-boot dts changes - they should be small and provide
> > a common base for this series.
> >
> 
> Yes. That would be really helpful to have a DT path with all the
> entries for easier testing.
> 
> 

As the changes to the U-Boot are short I'm including them here, please tell me if I
should include them in other way. I merged my changes with Atish's own
patch to simplify the process.

diff -u b/arch/riscv/dts/fu740-c000.dtsi b/arch/riscv/dts/fu740-c000.dtsi
--- b/arch/riscv/dts/fu740-c000.dtsi
+++ b/arch/riscv/dts/fu740-c000.dtsi
@@ -141,6 +141,49 @@
                #size-cells = <2>;
                compatible = "sifive,fu740-c000", "sifive,fu740", "simple-bus";
                ranges;
+               pmu {
+                       compatible = "riscv,pmu";
+                       pmu,raw-event-to-mhpmcounters = <0x00000000 0x00000100 0x18
+                                                       0x00000000 0x00000200 0x18
+                                                       0x00000000 0x00000400 0x18
+                                                       0x00000000 0x00000800 0x18
+                                                       0x00000000 0x00001000 0x18
+                                                       0x00000000 0x00002000 0x18
+                                                       0x00000000 0x00004000 0x18
+                                                       0x00000000 0x00008000 0x18
+                                                       0x00000000 0x00010000 0x18
+                                                       0x00000000 0x00020000 0x18
+                                                       0x00000000 0x00040000 0x18
+                                                       0x00000000 0x00080000 0x18
+                                                       0x00000000 0x00100000 0x18
+                                                       0x00000000 0x00200000 0x18
+                                                       0x00000000 0x00400000 0x18
+                                                       0x00000000 0x00800000 0x18
+                                                       0x00000000 0x01000000 0x18
+                                                       0x00000000 0x02000000 0x18
+                                                       0x00000000 0x00000101 0x18
+                                                       0x00000000 0x00000201 0x18
+                                                       0x00000000 0x00000401 0x18
+                                                       0x00000000 0x00000801 0x18
+                                                       0x00000000 0x00001001 0x18
+                                                       0x00000000 0x00002001 0x18
+                                                       0x00000000 0x00004001 0x18
+                                                       0x00000000 0x00008001 0x18
+                                                       0x00000000 0x00010001 0x18
+                                                       0x00000000 0x00020001 0x18
+                                                       0x00000000 0x00040001 0x18
+                                                       0x00000000 0x00000102 0x18
+                                                       0x00000000 0x00000202 0x18
+                                                       0x00000000 0x00000402 0x18
+                                                       0x00000000 0x00000802 0x18
+                                                       0x00000000 0x00001002 0x18
+                                                       0x00000000 0x00002002 0x18>;
+                       pmu,event-to-mhpmcounters = <0x05 0x06 0x18
+                                                    0x10009 0x10009 0x18>;
+                       pmu,event-to-mhpmevent = <0x05 0x00000000 0x4000
+                                                 0x06 0x00000000 0x4001
+                                                 0x10008 0x00000000 0x102>;
+               };
                plic0: interrupt-controller@c000000 {
                        #interrupt-cells = <1>;
                        compatible = "sifive,plic-1.0.0";


> > Tested-by: Nikita Shubin <n.shubin@yadro.com>
> >
> > > Signed-off-by: João Mário Domingos <joao.mario@tecnico.ulisboa.pt>
> > >
> > > This work was developed at INESC-ID, Instituto Superior Técnico,
> > > Universidade de Lisboa.
> > >
> > > ---
> > > Changes in v2:
> > >   - Fix compilation errors and warnings
> > >   - Remove space idents
> > >   - Correct formatting
> 
> 
> 
> > >
> > > João Mário Domingos (4):
> > >   RISC-V: Create unique identification for SoC PMU
> > >   RISC-V: Support CPUID for risc-v in perf
> > >   RISC-V: Added generic pmu-events mapfile
> > >   RISC-V: Added HiFive Unmatched PMU events
> > >
> > >  arch/riscv/kernel/sbi.c                       |  3 +
> > >  drivers/perf/riscv_pmu.c                      | 18 ++++
> > >  drivers/perf/riscv_pmu_sbi.c                  | 47 ++++++++++
> > >  tools/perf/arch/riscv/util/Build              |  1 +
> > >  tools/perf/arch/riscv/util/header.c           | 66 +++++++++++++
> > >  tools/perf/pmu-events/arch/riscv/mapfile.csv  | 15 +++
> > >  .../pmu-events/arch/riscv/riscv-generic.json  | 20 ++++
> > >  .../arch/riscv/sifive/u74/instructions.json   | 92
> > > +++++++++++++++++++ .../arch/riscv/sifive/u74/memory.json         |
> > > 32 +++++++ .../arch/riscv/sifive/u74/microarch.json      | 57
> > > ++++++++++++ 10 files changed, 351 insertions(+)
> > >  create mode 100644 tools/perf/arch/riscv/util/header.c
> > >  create mode 100644 tools/perf/pmu-events/arch/riscv/mapfile.csv
> > >  create mode 100644
> > > tools/perf/pmu-events/arch/riscv/riscv-generic.json create mode
> > > 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/instructions.json
> > > create mode 100644
> > > tools/perf/pmu-events/arch/riscv/sifive/u74/memory.json create mode
> > > 100644 tools/perf/pmu-events/arch/riscv/sifive/u74/microarch.json
> > >
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> -- 
> Regards,
> Atish
Jessica Clarke Nov. 22, 2021, 4:26 p.m. UTC | #4
On 22 Nov 2021, at 14:57, João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> On Thu, Nov 18, 2021 at 12:00:09AM -0800, Atish Patra wrote:
>> On Wed, Nov 17, 2021 at 4:25 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>>> 
>>> On Tue, 16 Nov 2021 15:48:08 +0000
>>> João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
>>> 
>>> Hello Mario!
>>> 
>>> Thank you for your patch series.
>>> 
>>> I have reproduced your test with some u-boot dts tinkering, and got
>>> similar results.
>>> 
>>> However,
>>> 
>>>> 
>>>> [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2
>>> 
>>> OpenSBI sscofpmf has been merged.
>>> 
>>>> [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
>>>> [5]
>>>> https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
>>> 
>>> Is missing the adaptation for OpenSBI bitmap patch for
>>> "raw-event-to-mhpmcounters".
>> 
>> My patch was just an example and predates before the bitmap patch
>> posted by Vincent.
>> I will update the U-boot patch along with the next kernel version.
>> 
>>> 
>>>> [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
>>> 
>>> The link is broken.
>>> 
>> 
>> Sorry. This should be
>> https://github.com/atishp04/linux/tree/sbi_pmu_v4
>> 
>>>> [4]
>>>> http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
>>> 
>>>> [6]
>>>> https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
>>>> 
>>> 
>>> There is a version 2 submitted, and it won't apply as require rebasing
>>> and some renaming.
>>> 
>>> Please share your u-boot dts changes - they should be small and provide
>>> a common base for this series.
>>> 
>> 
>> Yes. That would be really helpful to have a DT path with all the
>> entries for easier testing.
>> 
>> 
> 
> As the changes to the U-Boot are short I'm including them here, please tell me if I
> should include them in other way. I merged my changes with Atish's own
> patch to simplify the process.
> 
> diff -u b/arch/riscv/dts/fu740-c000.dtsi b/arch/riscv/dts/fu740-c000.dtsi
> --- b/arch/riscv/dts/fu740-c000.dtsi
> +++ b/arch/riscv/dts/fu740-c000.dtsi
> @@ -141,6 +141,49 @@
>                #size-cells = <2>;
>                compatible = "sifive,fu740-c000", "sifive,fu740", "simple-bus";
>                ranges;
> +               pmu {
> +                       compatible = "riscv,pmu";
> +                       pmu,raw-event-to-mhpmcounters = <0x00000000 0x00000100 0x18
> +                                                       0x00000000 0x00000200 0x18
> +                                                       0x00000000 0x00000400 0x18
> +                                                       0x00000000 0x00000800 0x18
> +                                                       0x00000000 0x00001000 0x18
> +                                                       0x00000000 0x00002000 0x18
> +                                                       0x00000000 0x00004000 0x18
> +                                                       0x00000000 0x00008000 0x18
> +                                                       0x00000000 0x00010000 0x18
> +                                                       0x00000000 0x00020000 0x18
> +                                                       0x00000000 0x00040000 0x18
> +                                                       0x00000000 0x00080000 0x18
> +                                                       0x00000000 0x00100000 0x18
> +                                                       0x00000000 0x00200000 0x18
> +                                                       0x00000000 0x00400000 0x18
> +                                                       0x00000000 0x00800000 0x18
> +                                                       0x00000000 0x01000000 0x18
> +                                                       0x00000000 0x02000000 0x18
> +                                                       0x00000000 0x00000101 0x18
> +                                                       0x00000000 0x00000201 0x18
> +                                                       0x00000000 0x00000401 0x18
> +                                                       0x00000000 0x00000801 0x18
> +                                                       0x00000000 0x00001001 0x18
> +                                                       0x00000000 0x00002001 0x18
> +                                                       0x00000000 0x00004001 0x18
> +                                                       0x00000000 0x00008001 0x18
> +                                                       0x00000000 0x00010001 0x18
> +                                                       0x00000000 0x00020001 0x18
> +                                                       0x00000000 0x00040001 0x18
> +                                                       0x00000000 0x00000102 0x18
> +                                                       0x00000000 0x00000202 0x18
> +                                                       0x00000000 0x00000402 0x18
> +                                                       0x00000000 0x00000802 0x18
> +                                                       0x00000000 0x00001002 0x18
> +                                                       0x00000000 0x00002002 0x18>;
> +                       pmu,event-to-mhpmcounters = <0x05 0x06 0x18
> +                                                    0x10009 0x10009 0x18>;
> +                       pmu,event-to-mhpmevent = <0x05 0x00000000 0x4000
> +                                                 0x06 0x00000000 0x4001
> +                                                 0x10008 0x00000000 0x102>;
> +               };
>                plic0: interrupt-controller@c000000 {
>                        #interrupt-cells = <1>;
>                        compatible = "sifive,plic-1.0.0”;

I still highly disagree with this. The presence of the SBI PMU
extension is a property of the firmware, not the SoC, so it has no
place in /soc. Moreover its existence can be probed via the
sbi_probe_extension SBI call. It is already discoverable.

See my responses to Atish’s patches for more details.

Jess
Atish Patra Nov. 22, 2021, 9:17 p.m. UTC | #5
On Mon, Nov 22, 2021 at 8:26 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 22 Nov 2021, at 14:57, João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> > On Thu, Nov 18, 2021 at 12:00:09AM -0800, Atish Patra wrote:
> >> On Wed, Nov 17, 2021 at 4:25 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> >>>
> >>> On Tue, 16 Nov 2021 15:48:08 +0000
> >>> João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> >>>
> >>> Hello Mario!
> >>>
> >>> Thank you for your patch series.
> >>>
> >>> I have reproduced your test with some u-boot dts tinkering, and got
> >>> similar results.
> >>>
> >>> However,
> >>>
> >>>>
> >>>> [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2
> >>>
> >>> OpenSBI sscofpmf has been merged.
> >>>
> >>>> [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
> >>>> [5]
> >>>> https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
> >>>
> >>> Is missing the adaptation for OpenSBI bitmap patch for
> >>> "raw-event-to-mhpmcounters".
> >>
> >> My patch was just an example and predates before the bitmap patch
> >> posted by Vincent.
> >> I will update the U-boot patch along with the next kernel version.
> >>
> >>>
> >>>> [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
> >>>
> >>> The link is broken.
> >>>
> >>
> >> Sorry. This should be
> >> https://github.com/atishp04/linux/tree/sbi_pmu_v4
> >>
> >>>> [4]
> >>>> http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
> >>>
> >>>> [6]
> >>>> https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
> >>>>
> >>>
> >>> There is a version 2 submitted, and it won't apply as require rebasing
> >>> and some renaming.
> >>>
> >>> Please share your u-boot dts changes - they should be small and provide
> >>> a common base for this series.
> >>>
> >>
> >> Yes. That would be really helpful to have a DT path with all the
> >> entries for easier testing.
> >>
> >>
> >
> > As the changes to the U-Boot are short I'm including them here, please tell me if I
> > should include them in other way. I merged my changes with Atish's own
> > patch to simplify the process.
> >
> > diff -u b/arch/riscv/dts/fu740-c000.dtsi b/arch/riscv/dts/fu740-c000.dtsi
> > --- b/arch/riscv/dts/fu740-c000.dtsi
> > +++ b/arch/riscv/dts/fu740-c000.dtsi
> > @@ -141,6 +141,49 @@
> >                #size-cells = <2>;
> >                compatible = "sifive,fu740-c000", "sifive,fu740", "simple-bus";
> >                ranges;
> > +               pmu {
> > +                       compatible = "riscv,pmu";
> > +                       pmu,raw-event-to-mhpmcounters = <0x00000000 0x00000100 0x18
> > +                                                       0x00000000 0x00000200 0x18
> > +                                                       0x00000000 0x00000400 0x18
> > +                                                       0x00000000 0x00000800 0x18
> > +                                                       0x00000000 0x00001000 0x18
> > +                                                       0x00000000 0x00002000 0x18
> > +                                                       0x00000000 0x00004000 0x18
> > +                                                       0x00000000 0x00008000 0x18
> > +                                                       0x00000000 0x00010000 0x18
> > +                                                       0x00000000 0x00020000 0x18
> > +                                                       0x00000000 0x00040000 0x18
> > +                                                       0x00000000 0x00080000 0x18
> > +                                                       0x00000000 0x00100000 0x18
> > +                                                       0x00000000 0x00200000 0x18
> > +                                                       0x00000000 0x00400000 0x18
> > +                                                       0x00000000 0x00800000 0x18
> > +                                                       0x00000000 0x01000000 0x18
> > +                                                       0x00000000 0x02000000 0x18
> > +                                                       0x00000000 0x00000101 0x18
> > +                                                       0x00000000 0x00000201 0x18
> > +                                                       0x00000000 0x00000401 0x18
> > +                                                       0x00000000 0x00000801 0x18
> > +                                                       0x00000000 0x00001001 0x18
> > +                                                       0x00000000 0x00002001 0x18
> > +                                                       0x00000000 0x00004001 0x18
> > +                                                       0x00000000 0x00008001 0x18
> > +                                                       0x00000000 0x00010001 0x18
> > +                                                       0x00000000 0x00020001 0x18
> > +                                                       0x00000000 0x00040001 0x18
> > +                                                       0x00000000 0x00000102 0x18
> > +                                                       0x00000000 0x00000202 0x18
> > +                                                       0x00000000 0x00000402 0x18
> > +                                                       0x00000000 0x00000802 0x18
> > +                                                       0x00000000 0x00001002 0x18
> > +                                                       0x00000000 0x00002002 0x18>;
> > +                       pmu,event-to-mhpmcounters = <0x05 0x06 0x18
> > +                                                    0x10009 0x10009 0x18>;
> > +                       pmu,event-to-mhpmevent = <0x05 0x00000000 0x4000
> > +                                                 0x06 0x00000000 0x4001
> > +                                                 0x10008 0x00000000 0x102>;
> > +               };
> >                plic0: interrupt-controller@c000000 {
> >                        #interrupt-cells = <1>;
> >                        compatible = "sifive,plic-1.0.0”;
>
> I still highly disagree with this. The presence of the SBI PMU
> extension is a property of the firmware, not the SoC, so it has no
> place in /soc. Moreover its existence can be probed via the
> sbi_probe_extension SBI call. It is already discoverable.
>
> See my responses to Atish’s patches for more details.
Hi Jessica,
I am working on revising the PMU series based on your feedback.
The diff proposed here will go into the device tree in U-Boot meant to be used
for firmware. Firmware should remove these DT properties before
jumping to the Linux kernel
Currently OpenSBI does this as well.


> Jess
>
Nikita Shubin Nov. 23, 2021, 5:24 a.m. UTC | #6
Hello Mario!

On Mon, 22 Nov 2021 14:57:52 +0000
João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> 
> As the changes to the U-Boot are short I'm including them here,
> please tell me if I should include them in other way. I merged my
> changes with Atish's own patch to simplify the process.
> 
> diff -u b/arch/riscv/dts/fu740-c000.dtsi
> b/arch/riscv/dts/fu740-c000.dtsi --- b/arch/riscv/dts/fu740-c000.dtsi
> +++ b/arch/riscv/dts/fu740-c000.dtsi
> @@ -141,6 +141,49 @@
>                 #size-cells = <2>;
>                 compatible = "sifive,fu740-c000", "sifive,fu740",
> "simple-bus"; ranges;
> +               pmu {
> +                       compatible = "riscv,pmu";
> +                       pmu,raw-event-to-mhpmcounters = <0x00000000
> 0x00000100 0x18
> +                                                       0x00000000
> 0x00000200 0x18
> +                                                       0x00000000
> 0x00000400 0x18
> +                                                       0x00000000
> 0x00000800 0x18
> +                                                       0x00000000
> 0x00001000 0x18
> +                                                       0x00000000
> 0x00002000 0x18
> +                                                       0x00000000
> 0x00004000 0x18
> +                                                       0x00000000
> 0x00008000 0x18
> +                                                       0x00000000
> 0x00010000 0x18
> +                                                       0x00000000
> 0x00020000 0x18
> +                                                       0x00000000
> 0x00040000 0x18
> +                                                       0x00000000
> 0x00080000 0x18
> +                                                       0x00000000
> 0x00100000 0x18
> +                                                       0x00000000
> 0x00200000 0x18
> +                                                       0x00000000
> 0x00400000 0x18
> +                                                       0x00000000
> 0x00800000 0x18
> +                                                       0x00000000
> 0x01000000 0x18
> +                                                       0x00000000
> 0x02000000 0x18
> +                                                       0x00000000
> 0x00000101 0x18
> +                                                       0x00000000
> 0x00000201 0x18
> +                                                       0x00000000
> 0x00000401 0x18
> +                                                       0x00000000
> 0x00000801 0x18
> +                                                       0x00000000
> 0x00001001 0x18
> +                                                       0x00000000
> 0x00002001 0x18
> +                                                       0x00000000
> 0x00004001 0x18
> +                                                       0x00000000
> 0x00008001 0x18
> +                                                       0x00000000
> 0x00010001 0x18
> +                                                       0x00000000
> 0x00020001 0x18
> +                                                       0x00000000
> 0x00040001 0x18
> +                                                       0x00000000
> 0x00000102 0x18
> +                                                       0x00000000
> 0x00000202 0x18
> +                                                       0x00000000
> 0x00000402 0x18
> +                                                       0x00000000
> 0x00000802 0x18
> +                                                       0x00000000
> 0x00001002 0x18
> +                                                       0x00000000
> 0x00002002 0x18>;
> +                       pmu,event-to-mhpmcounters = <0x05 0x06 0x18
> +                                                    0x10009 0x10009
> 0x18>;
> +                       pmu,event-to-mhpmevent = <0x05 0x00000000
> 0x4000
> +                                                 0x06 0x00000000
> 0x4001
> +                                                 0x10008 0x00000000
> 0x102>;
> +               };

Well, i definitely thought it was shorter...

After applying Vincent Chen patches:
https://patchwork.ozlabs.org/project/opensbi/patch/20211110050153.26935-1-vincent.chen@sifive.com/

My looks like:

diff --git a/arch/riscv/dts/fu740-c000.dtsi
b/arch/riscv/dts/fu740-c000.dtsi index 649efe400a..6a155b2b86 100644
--- a/arch/riscv/dts/fu740-c000.dtsi
+++ b/arch/riscv/dts/fu740-c000.dtsi
@@ -141,6 +141,17 @@
                #size-cells = <2>;
                compatible = "sifive,fu740-c000", "sifive,fu740",
"simple-bus"; ranges;
+               pmu {
+                       compatible = "riscv,pmu";
+                       riscv,raw-event-to-mhpmcounters = <0x00000000
0x3ffff00 0x0 0x0 0x18
+                                                        0x00000000
0x7ff01 0x0 0x1 0x18
+                                                        0x00000000
0x3f02 0x0 0x2 0x18>;
+                       riscv,event-to-mhpmcounters = <0x05 0x06 0x18
+                                                    0x10009 0x10009
0x18>;
+                       riscv,event-to-mhpmevent = <0x05 0x00000000
0x4000
+                                                 0x06 0x00000000 0x4001
+                                                 0x10008 0x00000000
0x102>;
+               };
                plic0: interrupt-controller@c000000 {
                        #interrupt-cells = <1>;
                        compatible = "sifive,plic-1.0.0";

Shorter indeed, but surely less readable.
João Mário Domingos Nov. 23, 2021, 5:39 p.m. UTC | #7
On Mon, Nov 22, 2021 at 01:17:33PM -0800, Atish Patra wrote:
> On Mon, Nov 22, 2021 at 8:26 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> >
> > On 22 Nov 2021, at 14:57, João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> > > On Thu, Nov 18, 2021 at 12:00:09AM -0800, Atish Patra wrote:
> > >> On Wed, Nov 17, 2021 at 4:25 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
> > >>>
> > >>> On Tue, 16 Nov 2021 15:48:08 +0000
> > >>> João Mário Domingos <joao.mario@tecnico.ulisboa.pt> wrote:
> > >>>
> > >>> Hello Mario!
> > >>>
> > >>> Thank you for your patch series.
> > >>>
> > >>> I have reproduced your test with some u-boot dts tinkering, and got
> > >>> similar results.
> > >>>
> > >>> However,
> > >>>
> > >>>>
> > >>>> [1] https://github.com/atishp04/opensbi/tree/pmu_sscofpmf_v2
> > >>>
> > >>> OpenSBI sscofpmf has been merged.
> > >>>
> > >>>> [2] https://github.com/atishp04/u-boot/tree/hifive_unmatched_dt_pmu
> > >>>> [5]
> > >>>> https://github.com/atishp04/u-boot/blob/hifive_unmatched_dt_pmu/arch/riscv/dts/fu740-c000.dtsi
> > >>>
> > >>> Is missing the adaptation for OpenSBI bitmap patch for
> > >>> "raw-event-to-mhpmcounters".
> > >>
> > >> My patch was just an example and predates before the bitmap patch
> > >> posted by Vincent.
> > >> I will update the U-boot patch along with the next kernel version.
> > >>
> > >>>
> > >>>> [3] https://github.com/atishp04/linux/tree/riscv_pmu_v4
> > >>>
> > >>> The link is broken.
> > >>>
> > >>
> > >> Sorry. This should be
> > >> https://github.com/atishp04/linux/tree/sbi_pmu_v4
> > >>
> > >>>> [4]
> > >>>> http://lists.infradead.org/pipermail/linux-riscv/2021-October/009408.html
> > >>>
> > >>>> [6]
> > >>>> https://patchwork.ozlabs.org/project/opensbi/patch/20211105013301.27656-1-vincent.chen@sifive.com/
> > >>>>
> > >>>
> > >>> There is a version 2 submitted, and it won't apply as require rebasing
> > >>> and some renaming.
> > >>>
> > >>> Please share your u-boot dts changes - they should be small and provide
> > >>> a common base for this series.
> > >>>
> > >>
> > >> Yes. That would be really helpful to have a DT path with all the
> > >> entries for easier testing.
> > >>
> > >>
> > >
> > > As the changes to the U-Boot are short I'm including them here, please tell me if I
> > > should include them in other way. I merged my changes with Atish's own
> > > patch to simplify the process.
> > >
> > > diff -u b/arch/riscv/dts/fu740-c000.dtsi b/arch/riscv/dts/fu740-c000.dtsi
> > > --- b/arch/riscv/dts/fu740-c000.dtsi
> > > +++ b/arch/riscv/dts/fu740-c000.dtsi
> > > @@ -141,6 +141,49 @@
> > >                #size-cells = <2>;
> > >                compatible = "sifive,fu740-c000", "sifive,fu740", "simple-bus";
> > >                ranges;
> > > +               pmu {
> > > +                       compatible = "riscv,pmu";
> > > +                       pmu,raw-event-to-mhpmcounters = <0x00000000 0x00000100 0x18
> > > +                                                       0x00000000 0x00000200 0x18
> > > +                                                       0x00000000 0x00000400 0x18
> > > +                                                       0x00000000 0x00000800 0x18
> > > +                                                       0x00000000 0x00001000 0x18
> > > +                                                       0x00000000 0x00002000 0x18
> > > +                                                       0x00000000 0x00004000 0x18
> > > +                                                       0x00000000 0x00008000 0x18
> > > +                                                       0x00000000 0x00010000 0x18
> > > +                                                       0x00000000 0x00020000 0x18
> > > +                                                       0x00000000 0x00040000 0x18
> > > +                                                       0x00000000 0x00080000 0x18
> > > +                                                       0x00000000 0x00100000 0x18
> > > +                                                       0x00000000 0x00200000 0x18
> > > +                                                       0x00000000 0x00400000 0x18
> > > +                                                       0x00000000 0x00800000 0x18
> > > +                                                       0x00000000 0x01000000 0x18
> > > +                                                       0x00000000 0x02000000 0x18
> > > +                                                       0x00000000 0x00000101 0x18
> > > +                                                       0x00000000 0x00000201 0x18
> > > +                                                       0x00000000 0x00000401 0x18
> > > +                                                       0x00000000 0x00000801 0x18
> > > +                                                       0x00000000 0x00001001 0x18
> > > +                                                       0x00000000 0x00002001 0x18
> > > +                                                       0x00000000 0x00004001 0x18
> > > +                                                       0x00000000 0x00008001 0x18
> > > +                                                       0x00000000 0x00010001 0x18
> > > +                                                       0x00000000 0x00020001 0x18
> > > +                                                       0x00000000 0x00040001 0x18
> > > +                                                       0x00000000 0x00000102 0x18
> > > +                                                       0x00000000 0x00000202 0x18
> > > +                                                       0x00000000 0x00000402 0x18
> > > +                                                       0x00000000 0x00000802 0x18
> > > +                                                       0x00000000 0x00001002 0x18
> > > +                                                       0x00000000 0x00002002 0x18>;
> > > +                       pmu,event-to-mhpmcounters = <0x05 0x06 0x18
> > > +                                                    0x10009 0x10009 0x18>;
> > > +                       pmu,event-to-mhpmevent = <0x05 0x00000000 0x4000
> > > +                                                 0x06 0x00000000 0x4001
> > > +                                                 0x10008 0x00000000 0x102>;
> > > +               };
> > >                plic0: interrupt-controller@c000000 {
> > >                        #interrupt-cells = <1>;
> > >                        compatible = "sifive,plic-1.0.0”;
> >
> > I still highly disagree with this. The presence of the SBI PMU
> > extension is a property of the firmware, not the SoC, so it has no
> > place in /soc. Moreover its existence can be probed via the
> > sbi_probe_extension SBI call. It is already discoverable.
> >
> > See my responses to Atish’s patches for more details.
> Hi Jessica,
> I am working on revising the PMU series based on your feedback.
> The diff proposed here will go into the device tree in U-Boot meant to be used
> for firmware. Firmware should remove these DT properties before
> jumping to the Linux kernel
> Currently OpenSBI does this as well.
>
Hi Atish and Jessica, I've seen your responses in Atish's patches and I
will update this series in accordance with the upcoming Atish's
changes.
From what I can anticipate the modifications will be minor, if
any, as this relates mainly to U-boot, as Atish stated.
> 
> > Jess
> >
> 
> 
> -- 
> Regards,
> Atish