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

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

Hi John,

在 2021/1/26 20:30, John Garry 写道:
> 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
> 

Ok,

>> +{
>> +    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
> 

Ok,

>> +    }
>> +}
>> +
>> +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

Ok, if it is not useful, let's return -ENOMEM directly.

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

Oops, this shall be set when alloc sllc_pmu.

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

Ok,

Thanks,
Shaokun

>> +    },
>> +    .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