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

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

On 31/12/2020 06:19, Shaokun Zhang wrote:
> On HiSilicon Hip09 platform, there is a PA (Protocol Adapter) module on
> each chip SICL (Super I/O Cluster) which incorporates three Hydra interface
> and facilitates the cache coherency between the dies on the chip. 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.
> 
> 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>

nit: please stop using inline when not needed

Apart from minor comments, below:
Reviewed-by: John Garry <john.garry@huawei.com>

note: we do internal review, but tags not given - maybe we should in 
future...

> ---
>   drivers/perf/hisilicon/Makefile             |   3 +-
>   drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 498 ++++++++++++++++++++++++++++
>   include/linux/cpuhotplug.h                  |   1 +
>   3 files changed, 501 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> 
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index 6600a9d45dd8..7643c9f93e36 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -1,3 +1,4 @@
>   # 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_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
> +			  hisi_uncore_pa_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> new file mode 100644
> index 000000000000..eec12f5daf71
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> @@ -0,0 +1,498 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon PA 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"
> +
> +/* PA register definition */
> +#define PA_PERF_CTRL			0x1C00

nit: generally lower case used for hex, but not so important

> +#define PA_EVENT_CTRL			0x1C04
> +#define PA_TT_CTRL			0x1C08
> +#define PA_TGTID_CTRL			0x1C14
> +#define PA_SRCID_CTRL			0x1C18
> +#define PA_INT_MASK			0x1C70
> +#define PA_INT_STATUS			0x1C78
> +#define PA_INT_CLEAR			0x1C7C
> +#define PA_EVENT_TYPE0			0x1C80
> +#define PA_PMU_VERSION			0x1CF0
> +#define PA_EVENT_CNT0_L			0x1D00
> +

...

> +
> +static int hisi_pa_pmu_init_data(struct platform_device *pdev,
> +				   struct hisi_pmu *pa_pmu)
> +{
> +	/*
> +	 * Use the SCCL_ID and the index ID to identify the PA PMU,
> +	 * while SCCL_ID is the nearst SCCL_ID from this SICL.
> +	 */
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> +				     &pa_pmu->sccl_id)) {

hmmm... I do wonder if we should make it clearer in the driver that this 
is the SICL. I know I mentioned this before--sorry

> +		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> +				     &pa_pmu->index_id)) {
> +		dev_err(&pdev->dev, "Cannot read idx-id!\n");
> +		return -EINVAL;
> +	}
> +
> +	pa_pmu->ccl_id = -1;
> +
> +	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pa_pmu->base)) {
> +		dev_err(&pdev->dev, "ioremap failed for pa_pmu resource.\n");
> +		return PTR_ERR(pa_pmu->base);
> +	}
> +
> +	pa_pmu->identifier = readl(pa_pmu->base + PA_PMU_VERSION);
> +
> +	return 0;
> +}
> +

...

> +static int hisi_pa_pmu_probe(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *pa_pmu;
> +	char *name;
> +	int ret;
> +
> +	pa_pmu = devm_kzalloc(&pdev->dev, sizeof(*pa_pmu), GFP_KERNEL);
> +	if (!pa_pmu)
> +		return -ENOMEM;
> +
> +	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * PA is attached in SICL and the CPU core is chosen to manage this
> +	 * PMU which is the nearest SCCL,

do we really need to do this? Can any CPU in that chip do it?

> while its SCCL_ID is greater than
> +	 * one with the SICL_ID.
> +	 */
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
> +			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
> +	if (!name) {
> +		dev_err(&pdev->dev, "failed to allocate name for PMU\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
> +				       &pa_pmu->node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> +		return ret;
> +	}
> +
> +	pa_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    = pa_pmu->pmu_events.attr_groups,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +	};
> +
> +	ret = perf_pmu_register(&pa_pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(pa_pmu->dev, "PMU register failed, ret = %d\n", ret);
> +		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
> +					    &pa_pmu->node);
> +		irq_set_affinity_hint(pa_pmu->irq, NULL);
> +	}
> +
> +	platform_set_drvdata(pdev, pa_pmu);

as before

> +	return ret;
> +}
> +
> +static int hisi_pa_pmu_remove(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *pa_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&pa_pmu->pmu);
> +	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
> +					    &pa_pmu->node);
> +	irq_set_affinity_hint(pa_pmu->irq, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hisi_pa_pmu_driver = {
> +	.driver = {
> +		.name = "hisi_pa_pmu",
> +		.acpi_match_table = hisi_pa_pmu_acpi_match,

some small comments on earlier patch reviews apply here

> +	},
> +	.probe = hisi_pa_pmu_probe,
> +	.remove = hisi_pa_pmu_remove,
> +};
> +
> +static int __init hisi_pa_pmu_module_init(void)
> +{
> +	int ret;
> +
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
> +				      "AP_PERF_ARM_HISI_PA_ONLINE",
> +				      hisi_uncore_pmu_online_cpu,
> +				      hisi_uncore_pmu_offline_cpu);
> +	if (ret) {
> +		pr_err("PA PMU: cpuhp state setup failed, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&hisi_pa_pmu_driver);
> +	if (ret)
> +		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE);
> +
> +	return ret;
> +}
> +module_init(hisi_pa_pmu_module_init);
> +
> +static void __exit hisi_pa_pmu_module_exit(void)
> +{
> +	platform_driver_unregister(&hisi_pa_pmu_driver);
> +	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE);
> +}
> +module_exit(hisi_pa_pmu_module_exit);
> +
> +MODULE_DESCRIPTION("HiSilicon PA uncore PMU driver");

nit maybe have "PA (Protocol Adapter)"

> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Shaokun Zhang <zhangshaokun@hisilicon.com>");
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index b5e04f9b68f1..37f591ed80d0 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_PA_ONLINE,
>   	CPUHP_AP_PERF_ARM_HISI_SLLC_ONLINE,
>   	CPUHP_AP_PERF_ARM_L2X0_ONLINE,
>   	CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
> 


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