Re: [PATCH 6/9] drivers/perf: hisi: Relax the event number check of v2 PMUs

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 18 Feb 2025 17:19:57 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> The supported event number range of each Uncore PMUs is provided by
> each driver in hisi_pmu::check_event and out of range events
> will be rejected. A later version with expanded event number range
> needs to register the PMU with updated hisi_pmu::check_event
> even if it's the only update, which means the expanded events
> cannot be used unless the driver's updated. However the unsupported
> events won't be counted by the hardware so we can relax the event
> number check to allow the use the expanded events.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 2 +-
>  drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 7 +++----
>  drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 2 +-
>  drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 3 +--
>  4 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 26eaa6d20c00..21c494881ca0 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -53,7 +53,7 @@
>  #define DDRC_V1_PERF_CTRL_EN	0x2
>  #define DDRC_V2_PERF_CTRL_EN	0x1
>  #define DDRC_V1_NR_EVENTS	0x7
> -#define DDRC_V2_NR_EVENTS	0x90
> +#define DDRC_V2_NR_EVENTS	0xFF
>  
>  #define DDRC_EVENT_CNTn(base, n)	((base) + (n) * 8)
>  #define DDRC_EVENT_TYPEn(base, n)	((base) + (n) * 4)
> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> index ca609db86046..78cd6d67f209 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> @@ -47,9 +47,8 @@
>  #define HHA_SRCID_CMD		GENMASK(16, 6)
>  #define HHA_SRCID_MSK		GENMASK(30, 20)
>  #define HHA_DATSRC_SKT_EN	BIT(23)
> -#define HHA_EVTYPE_NONE		0xff
> +#define HHA_EVTYPE_MASK		GENMASK(7, 0)
Using something called mask in places where we previously
had something called nr_events seems a little odd.

renaming EVTYPE_NONE to EVTYPE_MASK seems valid given the
useage but I'd have a different define for the number
of events and not make both changes in one patch.

>  #define HHA_V1_NR_EVENT		0x65
> -#define HHA_V2_NR_EVENT		0xCE
>  
>  HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_cmd, config1, 10, 0);
>  HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_msk, config1, 21, 11);
> @@ -197,7 +196,7 @@ static void hisi_hha_pmu_write_evtype(struct hisi_pmu *hha_pmu, int idx,
>  
>  	/* Write event code to HHA_EVENT_TYPEx register */
>  	val = readl(hha_pmu->base + reg);
> -	val &= ~(HHA_EVTYPE_NONE << shift);
> +	val &= ~(HHA_EVTYPE_MASK << shift);
>  	val |= (type << shift);
>  	writel(val, hha_pmu->base + reg);
>  }
> @@ -453,7 +452,7 @@ static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
>  
>  	if (hha_pmu->identifier >= HISI_PMU_V2) {
>  		hha_pmu->counter_bits = 64;
> -		hha_pmu->check_event = HHA_V2_NR_EVENT;
> +		hha_pmu->check_event = HHA_EVTYPE_MASK;
To me this makes little sense.  Should be HHA_MAX_NR_EVENT
or something like that.

>  		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v2_attr_groups;
>  		hha_pmu->num_counters = HHA_V2_NR_COUNTERS;
>  	} else {

Jonathan