[PATCH 7/8] drivers/perf: hisi: Add support for HiSilicon PA PMU driver
Shaokun Zhang
zhangshaokun at hisilicon.com
Wed Jan 27 03:41:20 EST 2021
Hi John,
在 2021/1/26 20:45, John Garry 写道:
> 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 at arm.com>
>> Cc: Will Deacon <will at kernel.org>
>> Cc: John Garry <john.garry at huawei.com>
>> Cc: Jonathan Cameron <Jonathan.Cameron at huawei.com>
>> Co-developed-by: Qi Liu <liuqi115 at huawei.com>
>> Signed-off-by: Qi Liu <liuqi115 at huawei.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun at hisilicon.com>
>
> nit: please stop using inline when not needed
Ok,
>
> Apart from minor comments, below:
> Reviewed-by: John Garry <john.garry at huawei.com>
>
> note: we do internal review, but tags not given - maybe we should in future...
Sure, will do this in the next version.
>
>> ---
>> 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 at 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
Ok,
>
>> +#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
This is used to check the cpu core in SCCL affinity when online/offline cpu core.
But for PMU name, we show it using SICL as follow.
> 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?
Any CPU core can do this, but I think it's better to choose the nearest SCCL's
CPU core to do this that we can avoid cross SCCL operations.
>
>> 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
Apologies and will fix this.
>
>> + 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
>
Ok,
>> + },
>> + .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)"
>
Sure,
Thanks,
Shaokun
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Shaokun Zhang <zhangshaokun at 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,
>>
>
> .
More information about the linux-arm-kernel
mailing list