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

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

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@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

Ok,

> 
> 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...

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@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@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