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 Date: Fri, 9 Jun 2023 09:53:38 +0100 [thread overview] Message-ID: <ZILokkt17/yCPQQ2@FVFF77S0Q05N> (raw) In-Reply-To: <20230609075608.36559-2-hejunhao3@huawei.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
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 Date: Fri, 9 Jun 2023 09:53:38 +0100 [thread overview] Message-ID: <ZILokkt17/yCPQQ2@FVFF77S0Q05N> (raw) In-Reply-To: <20230609075608.36559-2-hejunhao3@huawei.com> 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
next prev parent reply other threads:[~2023-06-09 8:54 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-06-09 7:56 [PATCH v4 0/3] Add support for HiSilicon SoC uncore PMU Junhao He 2023-06-09 7:56 ` Junhao He 2023-06-09 7:56 ` [PATCH v4 1/3] drivers/perf: hisi: Add support for HiSilicon H60PA and PAv3 PMU driver Junhao He 2023-06-09 7:56 ` Junhao He 2023-06-09 8:53 ` Mark Rutland [this message] 2023-06-09 8:53 ` Mark Rutland 2023-06-15 11:44 ` hejunhao 2023-06-15 11:44 ` hejunhao 2023-06-09 7:56 ` [PATCH v4 2/3] drivers/perf: hisi: Add support for HiSilicon UC " Junhao He 2023-06-09 7:56 ` Junhao He 2023-06-09 8:59 ` Mark Rutland 2023-06-09 8:59 ` Mark Rutland 2023-06-09 7:56 ` [PATCH v4 3/3] docs: perf: Add new description for HiSilicon UC PMU Junhao He 2023-06-09 7:56 ` Junhao He 2023-06-09 9:00 ` Mark Rutland 2023-06-09 9:00 ` Mark Rutland
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ZILokkt17/yCPQQ2@FVFF77S0Q05N \ --to=mark.rutland@arm.com \ --cc=hejunhao3@huawei.com \ --cc=jonathan.cameron@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=prime.zeng@hisilicon.com \ --cc=shenyang39@huawei.com \ --cc=will@kernel.org \ --cc=yangyicong@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.