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

Jonathan Cameron
2025-03-04T10:11:56Z

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