Re: [PATCH 6/8] drivers/perf: hisi: Add support for HiSilicon SLLC PMU driver

[John Garry <john.garry@huawei.com>]

On 31/12/2020 06:19, Shaokun Zhang wrote:
> HiSilicon's Hip09 is comprised by multi-dies that can be connected by SLLC
> module (Skyros Link Layer Controller), its has separate PMU registers which
> the driver can program it freely and interrupt is supported to handle
> counter overflow. Let's support its driver under the framework of HiSilicon
> uncore PMU driver.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Co-developed-by: Qi Liu <liuqi115@huawei.com>
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>

layout looks consistent with other hisi uncore PMU drivers, but some 
small comments, below

Reviewed-by: John Garry <john.garry@huawei.com>

> ---
>   drivers/perf/hisilicon/Makefile               |   2 +-
>   drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 530 ++++++++++++++++++++++++++
>   include/linux/cpuhotplug.h                    |   1 +
>   3 files changed, 532 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> 
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index e8377061845f..6600a9d45dd8 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -1,3 +1,3 @@
>   # 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_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> new file mode 100644
> index 000000000000..6911c388a5ac
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon SLLC uncore Hardware event counters support
> + *
> + * Copyright (C) 2020 Hisilicon Limited
> + * Author: Shaokun Zhang <zhangshaokun@hisilicon.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + */
> +#include <linux/acpi.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/list.h>
> +#include <linux/smp.h>
> +
> +#include "hisi_uncore_pmu.h"
> +
> +/* SLLC register definition */
> +#define SLLC_INT_MASK			0x0814
> +#define SLLC_INT_STATUS			0x0818
> +#define SLLC_INT_CLEAR			0x081C
> +#define SLLC_PERF_CTRL			0x1C00
> +#define SLLC_SRCID_CTRL			0x1C04
> +#define SLLC_TGTID_CTRL			0x1C08
> +#define SLLC_EVENT_CTRL			0x1C14
> +#define SLLC_EVENT_TYPE0		0x1C18
> +#define SLLC_VERSION			0x1CF0
> +#define SLLC_EVENT_CNT0_L		0x1D00
> +
> +#define SLLC_EVTYPE_MASK		0xFF
> +#define SLLC_PERF_CTRL_EN		BIT(0)
> +#define SLLC_FILT_EN			BIT(1)
> +#define SLLC_TRACETAG_EN		BIT(2)
> +#define SLLC_SRCID_EN			BIT(4)
> +#define SLLC_SRCID_NONE			0x0
> +#define SLLC_TGTID_EN			BIT(5)
> +#define SLLC_TGTID_NONE			0x0
> +#define SLLC_TGTID_LO_SHIFT		1
> +#define SLLC_TGTID_HI_SHIFT		12
> +#define SLLC_SRCID_CMD_SHIFT		1
> +#define SLLC_SRCID_MSK_SHIFT		12
> +#define SLLC_NR_EVENTS			0x80
> +
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tgtid_lo, config1, 10, 0);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tgtid_hi, config1, 21, 11);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_cmd, config1, 32, 22);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_msk, config1, 43, 33);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tracetag_en, config1, 44, 44);
> +
> +static inline bool tgtid_is_valid(u32 hi, u32 lo)

nit: no need for inline

> +{
> +	return lo > 0 && hi >= lo;
> +}
> +
> +static inline void hisi_sllc_pmu_enable_tracetag(struct perf_event *event)
> +{
> +	struct hisi_pmu *sllc_pmu = to_hisi_pmu(event->pmu);
> +	u32 tt_en = hisi_get_tracetag_en(event);
> +
> +	if (tt_en) {
> +		u32 val;
> +
> +		val = readl(sllc_pmu->base + SLLC_PERF_CTRL);
> +		val |= SLLC_TRACETAG_EN | SLLC_FILT_EN;
> +		writel(val, sllc_pmu->base + SLLC_PERF_CTRL);
> +	}
> +}
> +
> +static inline void hisi_sllc_pmu_disable_tracetag(struct perf_event *event)
> +{
> +	struct hisi_pmu *sllc_pmu = to_hisi_pmu(event->pmu);
> +	u32 tt_en = hisi_get_tracetag_en(event);
> +
> +	if (tt_en) {
> +		u32 val;
> +
> +		val = readl(sllc_pmu->base + SLLC_PERF_CTRL);
> +		val &= ~(SLLC_TRACETAG_EN | SLLC_FILT_EN);
> +		writel(val, sllc_pmu->base + SLLC_PERF_CTRL);

note: i think that all these can be _relaxed variant, but that is for 
existing drivers as well, so can be reviewed later

> +	}
> +}
> +
> +static inline void hisi_sllc_pmu_config_tgtid(struct perf_event *event)
> +{
> +	struct hisi_pmu *sllc_pmu = to_hisi_pmu(event->pmu);
> +	u32 lo = hisi_get_tgtid_lo(event);
> +	u32 hi = hisi_get_tgtid_hi(event);
> +
> +	if (tgtid_is_valid(hi, lo)) {
> +		u32 val = (hi << SLLC_TGTID_HI_SHIFT) | (lo << SLLC_TGTID_LO_SHIFT);
> +
> +		writel(val, sllc_pmu->base + SLLC_TGTID_CTRL);
> +
> +		/* Enable the tgtid */
> +		val = readl(sllc_pmu->base + SLLC_PERF_CTRL);
> +		val |= SLLC_TGTID_EN | SLLC_FILT_EN;
> +		writel(val, sllc_pmu->base + SLLC_PERF_CTRL);
> +	}
> +}
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sccl%u_sllc%u",
> +			      sllc_pmu->sccl_id, sllc_pmu->index_id);
> +	if (!name) {
> +		dev_err(&pdev->dev, "failed to allocate name for PMU\n");

maybe this message is not so useful

> +		return -ENOMEM;
> +	}
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
> +				       &sllc_pmu->node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> +		return ret;
> +	}
> +
> +	sllc_pmu->pmu = (struct pmu) {
> +		.module		= THIS_MODULE,
> +		.task_ctx_nr	= perf_invalid_context,
> +		.event_init	= hisi_uncore_pmu_event_init,
> +		.pmu_enable	= hisi_uncore_pmu_enable,
> +		.pmu_disable	= hisi_uncore_pmu_disable,
> +		.add		= hisi_uncore_pmu_add,
> +		.del		= hisi_uncore_pmu_del,
> +		.start		= hisi_uncore_pmu_start,
> +		.stop		= hisi_uncore_pmu_stop,
> +		.read		= hisi_uncore_pmu_read,
> +		.attr_groups    = sllc_pmu->pmu_events.attr_groups,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +	};
> +
> +	ret = perf_pmu_register(&sllc_pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(sllc_pmu->dev, "PMU register failed, ret = %d\n", ret);
> +		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
> +					    &sllc_pmu->node);
> +		irq_set_affinity_hint(sllc_pmu->irq, NULL);
> +	}
> +
> +	platform_set_drvdata(pdev, sllc_pmu);

strange that we still do this for an error

> +
> +	return ret;
> +}
> +
> +static int hisi_sllc_pmu_remove(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *sllc_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&sllc_pmu->pmu);
> +	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
> +					    &sllc_pmu->node);
> +	irq_set_affinity_hint(sllc_pmu->irq, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hisi_sllc_pmu_driver = {
> +	.driver = {
> +		.name = "hisi_sllc_pmu",
> +		.acpi_match_table = hisi_sllc_pmu_acpi_match,

please set unbind unsupported

> +	},
> +	.probe = hisi_sllc_pmu_probe,
> +	.remove = hisi_sllc_pmu_remove,
> +};
> +
> +static int __init hisi_sllc_pmu_module_init(void)
> +{
> +	int ret;
> +
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
> +				      "AP_PERF_ARM_HISI_SLLC_ONLINE",
> +				      hisi_uncore_pmu_online_cpu,
> +				      hisi_uncore_pmu_offline_cpu);
> +	if (ret) {
> +		pr_err("SLLC PMU: cpuhp state setup failed, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&hisi_sllc_pmu_driver);
> +	if (ret)
> +		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE);
> +
> +	return ret;
> +}
> +module_init(hisi_sllc_pmu_module_init);
> +
> +static void __exit hisi_sllc_pmu_module_exit(void)
> +{
> +	platform_driver_unregister(&hisi_sllc_pmu_driver);
> +	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE);
> +}
> +module_exit(hisi_sllc_pmu_module_exit);
> +
> +MODULE_DESCRIPTION("HiSilicon SLLC uncore PMU driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Shaokun Zhang <zhangshaokun@hisilicon.com>");
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 0042ef362511..b5e04f9b68f1 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -174,6 +174,7 @@ enum cpuhp_state {
>   	CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
>   	CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE,
>   	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +	CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
>   	CPUHP_AP_PERF_ARM_L2X0_ONLINE,
>   	CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
>   	CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel