Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 9 Jun 2023 09:53:38 +0100
From:   Mark Rutland <mark.rutland@arm.com>
To:     Junhao He <hejunhao3@huawei.com>
Cc:     will@kernel.org, jonathan.cameron@huawei.com,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-doc@vger.kernel.org, linuxarm@huawei.com,
        yangyicong@huawei.com, shenyang39@huawei.com,
        prime.zeng@hisilicon.com
Subject: Re: [PATCH v4 1/3] drivers/perf: hisi: Add support for HiSilicon
 H60PA and PAv3 PMU driver
Message-ID: <ZILokkt17/yCPQQ2@FVFF77S0Q05N>
References: <20230609075608.36559-1-hejunhao3@huawei.com>
 <20230609075608.36559-2-hejunhao3@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230609075608.36559-2-hejunhao3@huawei.com>
Precedence: bulk

Hi,

This generally looks ok, but I have a few minor comments.

On Fri, Jun 09, 2023 at 03:56:06PM +0800, Junhao He wrote:
> Compared to the original PA device, H60PA offers higher bandwidth.
> The H60PA is a new device and we use HID to differentiate them.
> 
> The events supported by PAv3 and PAv2 are different. They use the
> same HID.

That's a bit unfortunate -- doesn't that mean an older kernel that knows about
v2 will try to probe v3 as v2? Presumably things will go wrong if it pokes the
MMIO registers?

I appreciate it may be too late to change that now, but it seems like something
to consider in future (e.g. any changes not backwards compatible with v3 should
use a new HID).

> The PMU version register is used in the driver to
> distinguish different versions.
> 
> For each H60PA PMU, except for the overflow interrupt register, other
> functions of the H60PA PMU are the same as the original PA PMU module.
> It has 8-programable counters and each counter is free-running.
> Interrupt is supported to handle counter (64-bits) overflow.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 142 +++++++++++++++++---
>  drivers/perf/hisilicon/hisi_uncore_pmu.h    |   9 ++
>  2 files changed, 136 insertions(+), 15 deletions(-)

> @@ -284,6 +302,15 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
>  
>  	pa_pmu->identifier = readl(pa_pmu->base + PA_PMU_VERSION);
>  
> +	/* When running on v3 or later, returns the largest version supported */
> +	if (pa_pmu->identifier >= HISI_PMU_V3)
> +		pa_pmu->dev_info = &pa_pmu_info[2];
> +	else if (pa_pmu->identifier == HISI_PMU_V2)
> +		pa_pmu->dev_info = &pa_pmu_info[1];
> +
> +	if (!pa_pmu->dev_info || !pa_pmu->dev_info->name)
> +		return -EINVAL;
> +

Why does this use indices '2' and '1'? What happened to '0'?

It would be a bit clearer with something like:

	enum pmu_dev_info_idx {
		HISI_PMU_DEV_INFO_V2,
		HISI_PMU_DEV_INFO_V3,
		NR_HISI_PMU_DEV_INFO
	}

Then the above can be:

	if (pa_pmu->identifier >= HISI_PMU_V3)
		pa_pmu->dev_info = &pa_pmu_info[PMU_DEV_INFO_V3];
	else if (pa_pmu->identifier == HISI_PMU_V2)
		pa_pmu->dev_info = &pa_pmu_info[PMU_DEV_INFO_V2];
	else
		return -EINVAL;
	
	if (!pa_pmu->dev_info->name)
		return -EINVAL;

... and when you define the dev_info instances:

> +static const struct hisi_pmu_dev_info hisi_h32pa[] = {
> +	[1] = {
> +		.name = "pa",
> +		.attr_groups = hisi_pa_pmu_v2_attr_groups,
> +		.private = &hisi_pa_pmu_regs,
> +	},
> +	[2] = {
> +		.name = "pa",
> +		.attr_groups = hisi_pa_pmu_v3_attr_groups,
> +		.private = &hisi_pa_pmu_regs,
> +	},
> +	{}
> +};

... you could have:

	static const struct hisi_pmu_dev_info hisi_h32pa[NR_HISI_PMU_DEV_INFO] = {
		[HISI_PMU_DEV_INFO_V2] = {
			.name = "pa",
			.attr_groups = hisi_pa_pmu_v2_attr_groups,
			.private = &hisi_pa_pmu_regs,
		},
		[HISI_PMU_DEV_INFO_V3] = {
			.name = "pa",
			.attr_groups = hisi_pa_pmu_v3_attr_groups,
			.private = &hisi_pa_pmu_regs,
		},
	};

... which would clearly match up with the probe path, and would ensure the
arrays are always correctly sized if there's a v4, etc.

> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> index 07890a8e96ca..a8d6d6905f3f 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -24,6 +24,7 @@
>  #define pr_fmt(fmt)     "hisi_pmu: " fmt
>  
>  #define HISI_PMU_V2		0x30
> +#define HISI_PMU_V3		0x40
>  #define HISI_MAX_COUNTERS 0x10
>  #define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))
>  
> @@ -62,6 +63,13 @@ struct hisi_uncore_ops {
>  	void (*disable_filter)(struct perf_event *event);
>  };
>  
> +/* Describes the HISI PMU chip features information */
> +struct hisi_pmu_dev_info {
> +	const char *name;
> +	const struct attribute_group **attr_groups;
> +	void *private;
> +};
> +
>  struct hisi_pmu_hwevents {
>  	struct perf_event *hw_events[HISI_MAX_COUNTERS];
>  	DECLARE_BITMAP(used_mask, HISI_MAX_COUNTERS);
> @@ -72,6 +80,7 @@ struct hisi_pmu_hwevents {
>  struct hisi_pmu {
>  	struct pmu pmu;
>  	const struct hisi_uncore_ops *ops;
> +	const struct hisi_pmu_dev_info *dev_info;
>  	struct hisi_pmu_hwevents pmu_events;
>  	/* associated_cpus: All CPUs associated with the PMU */
>  	cpumask_t associated_cpus;

Will other hisi pmu drivers use the hisi_pmu_dev_info field in future?

I ask because otherwise you could make this local to hisi_uncore_pa_pmu.c if
you structured this as:

struct hisi_pa_pmu {
	struct hisi_pmu;
	const char *name;
	const struct attribute_group **attr_groups;
	const struct hisi_pa_pmu_int_regs *regs;
}

... which would give you some additional type-safety.

Thanks,
Mark