Message-ID: <be66d939-fcc1-4148-6fd8-96a5ab578f5b@huawei.com>
Date:   Sat, 27 May 2023 17:37:10 +0800
From:   hejunhao <hejunhao3@...wei.com>
To:     Yicong Yang <yangyicong@...wei.com>, <will@...nel.org>,
        <jonathan.cameron@...wei.com>, <linux-kernel@...r.kernel.org>,
        <mark.rutland@....com>
CC:     <yangyicong@...ilicon.com>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-doc@...r.kernel.org>, <linuxarm@...wei.com>,
        <shenyang39@...wei.com>, <prime.zeng@...ilicon.com>
Subject: Re: [PATCH 2/3] drivers/perf: hisi: Add support for HiSilicon UC PMU
 driver

Hi Yicong,


On 2023/5/25 21:39, Yicong Yang wrote:
> Hi Junhao,
>
> On 2023/5/23 21:18, Junhao He wrote:
>> On HiSilicon Hip09 platform, there is a UC (unified cache) module
>> on each chip SCCL (Super CPU Cluster). UC is a cache that provides
>> coherence between NUMA and UMA domains. It is located between L2
>> and Memory System. While PA uncore PMU model is the same as other
>> Hip09 PMU modules and many PMU events are supported. Let's support
>> the PMU driver using the HiSilicon uncore PMU framework.
>>
>> * rd_req_en : rd_req_en is the abbreviation of read request tracetag enable
>> and allows user to count only read operations.
>> details are listed in the hisi-pmu document.
>>
>> * srcid_en & srcid: allows user to filter statistics that come from
>> specific CPU/ICL by configuration source ID.
>>
>> * uring_channel: Allows users to filter statistical information based on
>> the specified tx request uring channel.
>> uring_channel only supported events: [0x47 ~ 0x59].
>>
> Just a quick glance and some minor comments below.

Thanks for your comments.

>> Signed-off-by: Junhao He <hejunhao3@...wei.com>
>> ---
>>   drivers/perf/hisilicon/Makefile             |   2 +-
>>   drivers/perf/hisilicon/hisi_uncore_pmu.c    |   5 +-
>>   drivers/perf/hisilicon/hisi_uncore_pmu.h    |   6 +
>>   drivers/perf/hisilicon/hisi_uncore_uc_pmu.c | 581 ++++++++++++++++++++
>>   4 files changed, 592 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>>
>> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
>> index 4d2c9abe3372..48dcc8381ea7 100644
>> --- a/drivers/perf/hisilicon/Makefile
>> +++ b/drivers/perf/hisilicon/Makefile
>> @@ -1,7 +1,7 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>>   			  hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
>> -			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
>> +			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o hisi_uncore_uc_pmu.o
>>   
>>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
>>   obj-$(CONFIG_HNS3_PMU) += hns3_pmu.o
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> index 2823f381930d..c5ac1c1a9fc1 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -20,7 +20,6 @@
>>   
>>   #include "hisi_uncore_pmu.h"
>>   
>> -#define HISI_GET_EVENTID(ev) (ev->hw.config_base & 0xff)
>>   #define HISI_MAX_PERIOD(nr) (GENMASK_ULL((nr) - 1, 0))
>>   
>>   /*
>> @@ -226,6 +225,10 @@ int hisi_uncore_pmu_event_init(struct perf_event *event)
>>   	hwc->idx		= -1;
>>   	hwc->config_base	= event->attr.config;
>>   
>> +	if (hisi_pmu->ops->check_format)
>> +		if (hisi_pmu->ops->check_format(event))
> It can be merged into one if.

Yes, I will do that.

>> +			return -EINVAL;
>> +
>>   	/* Enforce to use the same CPU for all events in this PMU */
>>   	event->cpu = hisi_pmu->on_cpu;
>>   
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> index 73574cc0a243..af6529e99c13 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -49,9 +49,15 @@ enum hisi_pmu_version {
>>   	HISI_PMU_MAX
>>   };
>>   
>> +#define HISI_GET_EVENTID(ev) (ev->hw.config_base & 0xff)
>> +
>> +#define HISI_PMU_EVTYPE_BITS		8
>> +#define HISI_PMU_EVTYPE_SHIFT(idx)	((idx) % 4 * HISI_PMU_EVTYPE_BITS)
>> +
>>   struct hisi_pmu;
>>   
>>   struct hisi_uncore_ops {
>> +	int (*check_format)(struct perf_event *event);
>>   	void (*write_evtype)(struct hisi_pmu *, int, u32);
>>   	int (*get_event_idx)(struct perf_event *);
>>   	u64 (*read_counter)(struct hisi_pmu *, struct hw_perf_event *);
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> new file mode 100644
>> index 000000000000..385966eab816
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> @@ -0,0 +1,581 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * HiSilicon SoC UC (unified cache) uncore Hardware event counters support
>> + *
>> + * Copyright (C) 2023 HiSilicon Limited
>> + *
>> + * This code is based on the uncore PMUs like hisi_uncore_l3c_pmu.
>> + */
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/list.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/property.h>
>> +
>> +#include "hisi_uncore_pmu.h"
>> +
>> +/* Dynamic CPU hotplug state used by UC PMU */
>> +static enum cpuhp_state hisi_uc_pmu_online;
>> +
>> +/* UC register definition */
>> +#define HISI_UC_INT_MASK_REG		0x0800
>> +#define HISI_UC_INT_STS_REG		0x0808
>> +#define HISI_UC_INT_CLEAR_REG		0x080c
>> +#define HISI_UC_TRACETAG_CTRL_REG	0x1b2c
>> +#define HISI_UC_TRACETAG_REQ_MSK	GENMASK(9, 7)
>> +#define HISI_UC_TRACETAG_MARK_EN	BIT(0)
>> +#define HISI_UC_TRACETAG_REQ_EN		(HISI_UC_TRACETAG_MARK_EN | BIT(2))
>> +#define HISI_UC_TRACETAG_SRCID_EN	BIT(3)
>> +#define HISI_UC_SRCID_CTRL_REG		0x1b40
>> +#define HISI_UC_SRCID_MSK		GENMASK(14, 1)
>> +#define HISI_UC_EVENT_CTRL_REG		0x1c00
>> +#define HISI_UC_EVENT_TRACETAG_EN	BIT(29)
>> +#define HISI_UC_EVENT_URING_MSK		GENMASK(28, 27)
>> +#define HISI_UC_EVENT_GLB_EN		BIT(26)
>> +#define HISI_UC_VERSION_REG		0x1cf0
>> +#define HISI_UC_EVTYPE0_REG		0x1d00
>> +#define HISI_UC_EVTYPE_REG(n)		(HISI_UC_EVTYPE0_REG + (n) * 4)
>> +#define HISI_UC_EVTYPE_MASK		GENMASK(7, 0)
>> +#define HISI_UC_CNTR0_REG		0x1e00
>> +#define HISI_UC_CNTR_REG(n)		(HISI_UC_CNTR0_REG + (n) * 8)
>> +
>> +#define HISI_UC_NR_COUNTERS		0x8
>> +#define HISI_UC_V2_NR_EVENTS		0xFF
>> +#define HISI_UC_CNTR_REG_BITS		64
>> +
>> +#define HISI_UC_RD_REQ_TRACETAG		0x4
>> +#define HISI_UC_URING_EVENT_MIN		0x47
>> +#define HISI_UC_URING_EVENT_MAX		0x59
>> +
>> +HISI_PMU_EVENT_ATTR_EXTRACTOR(rd_req_en, config1, 0, 0);
>> +HISI_PMU_EVENT_ATTR_EXTRACTOR(uring_channel, config1, 5, 4);
>> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid, config1, 19, 6);
>> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_en, config1, 20, 20);
>> +
>> +static int hisi_uc_pmu_check_format(struct perf_event *event)
> check_config or check_event will be clearer.

Sure, Will fix in next version.

>
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	if (hisi_get_srcid_en(event) && !hisi_get_rd_req_en(event)) {
>> +		dev_err(uc_pmu->dev,
>> +			"Failed to set srcid: depending on read request enabled!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!hisi_get_uring_channel(event))
>> +		return 0;
>> +
>> +	if ((HISI_GET_EVENTID(event) < HISI_UC_URING_EVENT_MIN) ||
>> +	    (HISI_GET_EVENTID(event) > HISI_UC_URING_EVENT_MAX))
>> +		dev_warn(uc_pmu->dev,
>> +			 "Only events: [%#x ~ %#x] support channel filtering!",
>> +			 HISI_UC_URING_EVENT_MIN, HISI_UC_URING_EVENT_MAX);
>> +
>> +	return 0;
>> +}
>> +
>> +static void hisi_uc_pmu_config_req_tracetag(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 val;
>> +
>> +	if (!hisi_get_rd_req_en(event))
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	/* The request-type has been configured */
>> +	if (FIELD_GET(HISI_UC_TRACETAG_REQ_MSK, val) == HISI_UC_RD_REQ_TRACETAG)
>> +		return;
>> +
>> +	/* Set request-type for tracetag, only read request is supported! */
>> +	val &= ~HISI_UC_TRACETAG_REQ_MSK;
>> +	val |= FIELD_PREP(HISI_UC_TRACETAG_REQ_MSK, HISI_UC_RD_REQ_TRACETAG);
>> +	val |= HISI_UC_TRACETAG_REQ_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_clear_req_tracetag(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 val;
>> +
>> +	if (!hisi_get_rd_req_en(event))
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	/* Do nothing, the request-type tracetag has been cleaned up */
>> +	if (FIELD_GET(HISI_UC_TRACETAG_REQ_MSK, val) == 0)
>> +		return;
>> +
>> +	/* Clear request-type */
>> +	val &= ~HISI_UC_TRACETAG_REQ_MSK;
>> +	val &= ~HISI_UC_TRACETAG_REQ_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_config_srcid_tracetag(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 val;
>> +
>> +	if (!hisi_get_srcid_en(event))
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	/* Do nothing, the source id has been configured */
>> +	if (FIELD_GET(HISI_UC_TRACETAG_SRCID_EN, val))
>> +		return;
>> +
>> +	/* Enable source id tracetag */
>> +	val |= HISI_UC_TRACETAG_SRCID_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_SRCID_CTRL_REG);
>> +	val &= ~HISI_UC_SRCID_MSK;
>> +	val |= FIELD_PREP(HISI_UC_SRCID_MSK, hisi_get_srcid(event));
>> +	writel(val, uc_pmu->base + HISI_UC_SRCID_CTRL_REG);
>> +
>> +	/* Depend on request-type tracetag enabled */
>> +	hisi_uc_pmu_config_req_tracetag(event);
>> +}
>> +
>> +static void hisi_uc_pmu_clear_srcid_tracetag(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 val;
>> +
>> +	if (!hisi_get_srcid_en(event))
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	/* Do nothing, the source id has been cleaned up */
>> +	if (FIELD_GET(HISI_UC_TRACETAG_SRCID_EN, val) == 0)
>> +		return;
>> +
>> +	hisi_uc_pmu_clear_req_tracetag(event);
>> +
>> +	/* Disable source id tracetag */
>> +	val &= ~HISI_UC_TRACETAG_SRCID_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_TRACETAG_CTRL_REG);
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_SRCID_CTRL_REG);
>> +	val &= ~HISI_UC_SRCID_MSK;
>> +	writel(val, uc_pmu->base + HISI_UC_SRCID_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_config_uring_channel(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 uring_channel = hisi_get_uring_channel(event);
>> +	u32 val;
>> +
>> +	/* Do nothing if not being set or is set explicitly to zero (default) */
>> +	if (uring_channel == 0)
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +
>> +	/* Do nothing, the uring_channel has been configured */
>> +	if (uring_channel == FIELD_GET(HISI_UC_EVENT_URING_MSK, val))
>> +		return;
>> +
>> +	val &= ~HISI_UC_EVENT_URING_MSK;
>> +	val |= FIELD_PREP(HISI_UC_EVENT_URING_MSK, uring_channel);
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_clear_uring_channel(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *uc_pmu = to_hisi_pmu(event->pmu);
>> +	u32 val;
>> +
>> +	/* Do nothing if not being set or is set explicitly to zero (default) */
>> +	if (hisi_get_uring_channel(event) == 0)
>> +		return;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +
>> +	/* Do nothing, the uring_channel has been cleaned up */
>> +	if (FIELD_GET(HISI_UC_EVENT_URING_MSK, val) == 0)
>> +		return;
>> +
>> +	val &= ~HISI_UC_EVENT_URING_MSK;
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_enable_filter(struct perf_event *event)
>> +{
>> +	if (event->attr.config1 == 0)
>> +		return;
>> +
>> +	hisi_uc_pmu_config_uring_channel(event);
>> +	hisi_uc_pmu_config_req_tracetag(event);
>> +	hisi_uc_pmu_config_srcid_tracetag(event);
>> +}
>> +
>> +static void hisi_uc_pmu_disable_filter(struct perf_event *event)
>> +{
>> +	if (event->attr.config1 == 0)
>> +		return;
>> +
>> +	hisi_uc_pmu_clear_srcid_tracetag(event);
>> +	hisi_uc_pmu_clear_req_tracetag(event);
>> +	hisi_uc_pmu_clear_uring_channel(event);
>> +}
>> +
>> +static void hisi_uc_pmu_write_evtype(struct hisi_pmu *uc_pmu, int idx, u32 type)
>> +{
>> +	u32 val;
>> +
>> +	/*
>> +	 * Select the appropriate event select register.
>> +	 * There are 2 32-bit event select registers for the
>> +	 * 8 hardware counters, each event code is 8-bit wide.
>> +	 */
>> +	val = readl(uc_pmu->base + HISI_UC_EVTYPE_REG(idx / 4));
>> +	val &= ~(HISI_UC_EVTYPE_MASK << HISI_PMU_EVTYPE_SHIFT(idx));
>> +	val |= (type << HISI_PMU_EVTYPE_SHIFT(idx));
>> +	writel(val, uc_pmu->base + HISI_UC_EVTYPE_REG(idx / 4));
>> +}
>> +
>> +static void hisi_uc_pmu_start_counters(struct hisi_pmu *uc_pmu)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +	val |= HISI_UC_EVENT_GLB_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_stop_counters(struct hisi_pmu *uc_pmu)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +	val &= ~HISI_UC_EVENT_GLB_EN;
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_enable_counter(struct hisi_pmu *uc_pmu,
>> +					struct hw_perf_event *hwc)
>> +{
>> +	u32 val;
>> +
>> +	/* Enable counter index */
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +	val |= (1 << hwc->idx);
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_disable_counter(struct hisi_pmu *uc_pmu,
>> +					struct hw_perf_event *hwc)
>> +{
>> +	u32 val;
>> +
>> +	/* Clear counter index */
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +	val &= ~(1 << hwc->idx);
>> +	writel(val, uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +}
>> +
>> +static u64 hisi_uc_pmu_read_counter(struct hisi_pmu *uc_pmu,
>> +				    struct hw_perf_event *hwc)
>> +{
>> +	return readq(uc_pmu->base + HISI_UC_CNTR_REG(hwc->idx));
>> +}
>> +
>> +static void hisi_uc_pmu_write_counter(struct hisi_pmu *uc_pmu,
>> +				      struct hw_perf_event *hwc, u64 val)
>> +{
>> +	hisi_uc_pmu_start_counters(uc_pmu);
>> +	writeq(val, uc_pmu->base + HISI_UC_CNTR_REG(hwc->idx));
>> +}
>> +
>> +static void hisi_uc_pmu_enable_counter_int(struct hisi_pmu *uc_pmu,
>> +					   struct hw_perf_event *hwc)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_INT_MASK_REG);
>> +	/* Write 0 to enable interrupt */
> The comment here and below maybe redundant. The code is self explanatory

Ok, i will drop this

>
>> +	val &= ~(1 << hwc->idx);
>> +	writel(val, uc_pmu->base + HISI_UC_INT_MASK_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_disable_counter_int(struct hisi_pmu *uc_pmu,
>> +					    struct hw_perf_event *hwc)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_INT_MASK_REG);
>> +	/* Write 1 to mask interrupt */
>> +	val |= (1 << hwc->idx);
>> +	writel(val, uc_pmu->base + HISI_UC_INT_MASK_REG);
>> +}
>> +
>> +static u32 hisi_uc_pmu_get_int_status(struct hisi_pmu *uc_pmu)
>> +{
>> +	return readl(uc_pmu->base + HISI_UC_INT_STS_REG);
>> +}
>> +
>> +static void hisi_uc_pmu_clear_int_status(struct hisi_pmu *uc_pmu, int idx)
>> +{
>> +	writel(1 << idx, uc_pmu->base + HISI_UC_INT_CLEAR_REG);
>> +}
>> +
>> +static int hisi_uc_pmu_init_data(struct platform_device *pdev,
>> +				 struct hisi_pmu *uc_pmu)
>> +{
>> +	/*
>> +	 * Use the SCCL_ID and CCL_ID to identify the UC PMU, while
>> +	 * SCCL_ID is in MPIDR[aff2] and CCL_ID is in MPIDR[aff1].
>> +	 */
> The comment doesn't match what you've done here, these information
> is gotten from device properties rather than MPIDR.
>
> Thanks.

Will update the comment
Thanks.


Best regards,
Junhao.

Powered by blists - more mailing lists