Messages in this thread | | | Subject | Re: [PATCH v3 2/2] drivers/perf: hisi: Add Support for CPA PMU | From | "liuqi (BA)" <> | Date | Wed, 6 Apr 2022 09:57:48 +0800 |
| |
Hi John,
Thanks for the comments. some replies inline.
On 2022/4/5 16:28, John Garry wrote: > On 21/03/2022 07:01, Qi Liu wrote: >> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on > > leave whitespace before '(' > ok, will fix this. >> each SICL (Super I/O Cluster) which implements packet format translation, >> route parsing and traffic statistic. > > statistics will fix this, thanks. > >> >> CPA PMU has 8 PMU counters and interrupt is supported to handle counter >> overflow. Let's support its driver under the framework of HiSilicon PMU >> driver. >> > > Generally this looks ok, just some small comments to consider. > > Thanks, > John > >> Signed-off-by: Qi Liu <liuqi115@huawei.com> >> --- >> drivers/perf/hisilicon/Makefile | 2 +- >> drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++ >> include/linux/cpuhotplug.h | 1 + >> 3 files changed, 402 insertions(+), 1 deletion(-) >> create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c >> >> diff --git a/drivers/perf/hisilicon/Makefile >> b/drivers/perf/hisilicon/Makefile >> index 506ed39e3266..6be83517acaa 100644 >> --- a/drivers/perf/hisilicon/Makefile >> +++ b/drivers/perf/hisilicon/Makefile >> @@ -1,6 +1,6 @@ >> # 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_pa_pmu.o >> + hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o >> obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o >> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c >> b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c >> new file mode 100644 >> index 000000000000..3f1d9c3f99fe >> --- /dev/null >> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c >> @@ -0,0 +1,400 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore hardware event >> counters support > > I am not sure why we even mention uncore for something which is not in a > SCCL >
we metion uncore as it use hisi_uncore_pmu framework, I'll drop this next time. >> + * >> + * Copyright (C) 2022 HiSilicon Limited >> + * Author: Qi Liu <liuqi115@huawei.com> >> + * >> + * This code is based on the uncore PMUs like arm-cci and arm-ccn. >> + */ >> + [...] >> + >> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev, >> + struct hisi_pmu *cpa_pmu) >> +{ >> + if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id", >> + &cpa_pmu->sicl_id)) { >> + dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n"); > > I don't think that you need to mention "cpa_pmu"
will drop it.
> >> + return -EINVAL; >> + } >> + >> + if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id", >> + &cpa_pmu->index_id)) { >> + dev_err(&pdev->dev, "Cannot read idx-id\n"); >> + return -EINVAL; >> + } >> + >> + cpa_pmu->ccl_id = -1; >> + cpa_pmu->sccl_id = -1; >> + cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(cpa_pmu->base)) >> + return PTR_ERR(cpa_pmu->base); >> + >> + cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION); >> + >> + return 0; >> +} >> +
[...]
>> + >> +static int hisi_cpa_pmu_probe(struct platform_device *pdev) >> +{ >> + struct hisi_pmu *cpa_pmu; >> + char *name; >> + int ret; >> + >> + cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL); >> + if (!cpa_pmu) >> + return -ENOMEM; >> + >> + ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu); >> + if (ret) >> + return ret; >> + >> + ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> + >> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1); >> + if (ret) { >> + dev_err(cpa_pmu->dev, "PMU register failed\n"); >> + cpuhp_state_remove_instance_nocalls( >> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, cpa_pmu); >> + >> + /* Power Management should be disabled before using CPA PMU. */ >> + hisi_cpa_pmu_disable_pm(cpa_pmu); > > maybe you can move this earlier in the probe, before registering > got it, will do this.
>> + return ret; >> +} >> + >> +static int hisi_cpa_pmu_remove(struct platform_device *pdev) >> +{ >> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev); >> + >> + hisi_cpa_pmu_enable_pm(cpa_pmu); > > why do we do this?
restore CPA registers after remove CPA PMU driver, as we disable pmu in probe function.
> >> + perf_pmu_unregister(&cpa_pmu->pmu); >> + >> cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, >> + &cpa_pmu->node); >> + return 0; >> +} >> + >> +static struct platform_driver hisi_cpa_pmu_driver = { >> + .driver = { >> + .name = "hisi_cpa_pmu", >> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match), >> + .suppress_bind_attrs = true, >> + }, >> + .probe = hisi_cpa_pmu_probe, >> + .remove = hisi_cpa_pmu_remove, >> +}; >> + >> +static int __init hisi_cpa_pmu_module_init(void) >> +{ >> + int ret; >> + >> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, >> + "AP_PERF_ARM_HISI_CPA_ONLINE", >> + hisi_uncore_pmu_online_cpu, >> + hisi_uncore_pmu_offline_cpu); >> + if (ret) { >> + pr_err("CPA PMU: setup hotplug: %d\n", ret); > > you don't mention failed > > And mostly we use lowercase for the driver name and maybe consider using > pr_fmt > got it, will fix this next time.
Thanks, Qi >> + return ret; >> + } >> + >> + ret = platform_driver_register(&hisi_cpa_pmu_driver); >> + if (ret) >> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE); >> + >> + return ret; >> +} >> +module_init(hisi_cpa_pmu_module_init); >> + >> +static void __exit hisi_cpa_pmu_module_exit(void) >> +{ >> + platform_driver_unregister(&hisi_cpa_pmu_driver); >> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE); >> +} >> +module_exit(hisi_cpa_pmu_module_exit); >> + >> +MODULE_DESCRIPTION("HiSilicon SoC CPA uncore PMU driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>"); >> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h >> index 411a428ace4d..d55063719904 100644 >> --- a/include/linux/cpuhotplug.h >> +++ b/include/linux/cpuhotplug.h >> @@ -220,6 +220,7 @@ enum cpuhp_state { >> CPUHP_AP_PERF_S390_SF_ONLINE, >> CPUHP_AP_PERF_ARM_CCI_ONLINE, >> CPUHP_AP_PERF_ARM_CCN_ONLINE, >> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, >> CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE, >> CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE, >> CPUHP_AP_PERF_ARM_HISI_L3_ONLINE, > > .
| |