From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3835CC7EE2E for ; Fri, 9 Jun 2023 08:54:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=9OJeO0U/BtLXJtEMjhfP8fcEBvgqpqSi2vhFlpDhibo=; b=xcmGlADtHTvNpN zY2Q3TyS18IWBUFrTY5xATZ2OQw3llb1UpPI3b/+z2qkUFaUZZwgrUDuJcDoxYSpOP1E889pj9sGO J7HiBL58D5OoUNcCXtktqkX13ssfM3MWwQWz76ToCtHI1vzT0FZirLVNAktG2GSKcpTEC7xCvDk2X lizNq+o1sskjhlL/28rXmUu5X+zM6VSGi5ji57S3RyHp0nDpAVAq6iOSeTXr9XSyHtQTgF9wRnxVW JkNPkBWAVbTwleJoGqqzR3doov8qMqhGNHhoyU/qRG5PsH9kT7ULOte2gpjxgC1dSW0Mi3aQ57Ph0 iNRujyGKA/RdaWD1tEXA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q7XsT-00CJPt-1y; Fri, 09 Jun 2023 08:53:49 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q7XsQ-00CJLg-2d for linux-arm-kernel@lists.infradead.org; Fri, 09 Jun 2023 08:53:48 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4012FAB6; Fri, 9 Jun 2023 01:54:28 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.25.215]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 501D23F71E; Fri, 9 Jun 2023 01:53:41 -0700 (PDT) Date: Fri, 9 Jun 2023 09:53:38 +0100 From: Mark Rutland To: Junhao He 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: References: <20230609075608.36559-1-hejunhao3@huawei.com> <20230609075608.36559-2-hejunhao3@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230609075608.36559-2-hejunhao3@huawei.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230609_015346_953053_1729AA73 X-CRM114-Status: GOOD ( 32.86 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > Reviewed-by: Jonathan Cameron > Reviewed-by: Yicong Yang > --- > 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