Re: [PATCH] perf: ampere: Add support for Ampere SoC PMUs

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

Hi Robin,

On Fri, 28 Apr 2023, Robin Murphy wrote:
> On 2023-04-27 22:53, Ilkka Koskinen wrote:
>> Add support for Ampere SoC PMUs. This driver supports MCU PMU
>> available in the AmpereOne SoC.
>> 
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>   .../admin-guide/perf/ampere-soc-pmu.rst       |  36 +
>>   Documentation/admin-guide/perf/index.rst      |   1 +
>>   drivers/perf/Kconfig                          |  10 +
>>   drivers/perf/Makefile                         |   1 +
>>   drivers/perf/ampere_soc_pmu.c                 | 724 ++++++++++++++++++
>>   5 files changed, 772 insertions(+)
>>   create mode 100644 Documentation/admin-guide/perf/ampere-soc-pmu.rst
>>   create mode 100644 drivers/perf/ampere_soc_pmu.c
>> 
>> diff --git a/Documentation/admin-guide/perf/ampere-soc-pmu.rst 
>> b/Documentation/admin-guide/perf/ampere-soc-pmu.rst
>> new file mode 100644
>> index 000000000000..5161fbd1c548
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/ampere-soc-pmu.rst
>> @@ -0,0 +1,36 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +============================================
>> +Ampere SoC Performance Monitoring Unit (PMU)
>> +============================================
>> +
>> +Ampere SoC PMU is a generic PMU IP. At the first phase it's used for 
>> counting
>> +MCU events on AmpereOne.
>> +
>> +MCU PMU events
>> +--------------
>> +
>> +The PMU driver registers a PMU device for every supported PMU instance on 
>> each
>> +SoC. See /sys/devices/AMPC0100:<nn>/.
>> +
>> +The PMU driver supports setting filters for "rank", "bank", and 
>> "threshold". The
>> +filter settings are device specific and shared between all the relevant 
>> events.
>> +The default value for all the filters is zero. The filters can be modified 
>> by
>> +setting them with the last event of the particular device. All the 
>> previous
>> +settings are overwritten.
>
> Yeah, that doesn't really work... what is the "last event" relative to? Order 
> of arguments to arbitrary tools? Order of perf_event_open syscalls? Order in 
> which events are actually scheduled on the PMU? (which users can't even 
> control - think event rotation)
>
> To be practical I think you'll have to handle this the same way as 
> arm_smmuv3_pmu's global filtering, and only allow events with matching filter 
> configs to be scheduled together.

Thanks for pointing out that to me! I thought my implementation had an 
issue with the shared filters but when I tried to look, if any of 
the supported PMUs was using them the same way, I somehow missed 
arm_smmuv3_pmu. I fix the driver.

>
> [...]
>> +#define SOC_PMEVCNTR0_LO	0x000
>> +#define SOC_PMEVCNTR0_HI	0x004
>> +#define SOC_PMCCNTR_LO		0x0F8
>> +#define SOC_PMCCNTR_HI		0x0FC
>> +
>> +#define SOC_PMEVTYPER0		0x400
>> +
>> +#define SOC_PMELCSR		0xA10
>> +
>> +#define SOC_PMCNTENSET		0xC00
>> +#define SOC_PMCNTENCLR		0xC20
>> +#define SOC_PMINTENSET		0xC40
>> +#define SOC_PMINTENCLR		0xC60
>> +#define SOC_PMOVSCLR		0xC80
>> +#define SOC_PMOVSSET		0xCC0
>> +
>> +#define SOC_PMAUXR0		0xD80
>> +#define SOC_PMAUXR1		0xD84
>> +#define SOC_PMAUXR2		0xD88
>> +#define SOC_PMAUXR3		0xD8C
>> +
>> +#define SOC_PMCFGR		0xE00
>> +#define SOC_PMCR		0xE04
>> +#define PMU_PMCR_E		BIT(0)
>> +#define PMU_PMCR_P		BIT(1)
>> +
>> +#define SOC_PMAUTHSTATUS	0xFB8
>> +#define SOC_PMDEVARCH		0xFBC
>> +#define SOC_PMDEVTYPE		0xFCC
>> +#define SOC_PMPIDR4		0xFD0
>> +#define SOC_PMPIDR0		0xFE0
>> +#define SOC_PMPIDR1		0xFE4
>> +#define SOC_PMPIDR2		0xFE8
>> +#define SOC_PMPIDR3		0xFEC
>> +#define SOC_PMCIDR0		0xFF0
>> +#define SOC_PMCIDR1		0xFF4
>> +#define SOC_PMCIDR2		0xFF8
>> +#define SOC_PMCIDR3		0xFFC
>
> This register map quite clearly follows the Arm CoreSight PMU architecture. 
> Nice to see it being used, but would you mind having a go at hooking up your 
> imp-def bits to the existing arm_cspmu driver?

I didn't think about that. I'll take a look at the coresight pmu driver 
and see how I could do it properly.

Thanks, Ilkka