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@xxxxxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Reviewed-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> ---
> 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
