Re: [PATCH v1] perf/hx_arm_ni: Support uncore ARM NI-700 PMU
From: Yang Jialong 杨佳龙
Date: Tue Jan 30 2024 - 21:59:12 EST
在 2024/1/30 16:43, Krzysztof Kozlowski 写道:
On 30/01/2024 09:17, JiaLong.Yang wrote:
This code is based on uncore PMUs arm_smmuv3_pmu and arm-cmn.
One ni-700 can have many clock domains. Each of them has only one PMU.
Here one PMU corresponds to one 'struct ni_pmu' instance.
PMU name will be ni_pmu_N_M, which N means different NI-700s and M means
different PMU in one NI-700. If only one NI-700 found in NI-700, name will
be ni_pmu_N.
Node interface event name will be xxni_N_eventname, such as asni_0_rdreq_any.
There are many kinds of type of nodes in one clock domain. Also means that
there are many kinds of that in one PMU. So we distinguish them by xxni string.
Besides, maybe there are many nodes have same type. So we have number N in
event name.
By ni_pmu_0_0/asni_0_rdreq_any/, we can pinpoint accurate bus traffic.
Example1: perf stat -a -e ni_pmu_0_0/asni_0_rdreq_any/,ni_pmu_0_0/cycles/
EXample2: perf stat -a -e ni_pmu_0_0/asni,id=0,event=0x0/
Signed-off-by: JiaLong.Yang <jialong.yang@xxxxxxxxxxxx>
---
If I should send Doc*/bindings/perf/*.yaml seperately?
Checkpatch tells you that, doesn't it?
OK. I will send it seperately.
Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
OK.
.../bindings/perf/hx,c2000-arm-ni.yaml | 58 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 6 +
drivers/perf/Kconfig | 10 +
drivers/perf/Makefile | 1 +
drivers/perf/hx_arm_ni.c | 1308 +++++++++++++++++
6 files changed, 1385 insertions(+)
create mode 100644 Documentation/devicetree/bindings/perf/hx,c2000-arm-ni.yaml
create mode 100644 drivers/perf/hx_arm_ni.c
diff --git a/Documentation/devicetree/bindings/perf/hx,c2000-arm-ni.yaml b/Documentation/devicetree/bindings/perf/hx,c2000-arm-ni.yaml
new file mode 100644
index 000000000000..1b145ecbfa83
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/hx,c2000-arm-ni.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/hx,c2000-arm-ni.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HX-C2000 NI (Network-on_chip Interconnect) Performance Monitors
+
+maintainers:
+ - Jialong Yang <jialong.yang@xxxxxxxxxxxx>
+
+properties:
+ compatible:
+ enum:
+ - hx,c2000-arm-ni
+
+ reg:
+ items:
+ - description: Physical address of the base (PERIPHBASE) and
+ size of the whole NI configuration address space.
+
+ interrupts:
+ minItems: 1
Why?
According to hw, one PMU has one interrupt line. So one NI maybe has
more than one. But actually it depends on hw implementation.
And in C code, I will return error when there is no interrupt.
+ items:
+ - description: Overflow interrupt for clock domain 0
+ - description: Overflow interrupt for clock domain 1
+ - description: Overflow interrupt for clock domain 2
+ description: Generally, one interrupt line for one PMU. But this also
+ support one interrupt line for a NI if merged.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+if:
+ properties:
+ compatible:
+ contains:
+ const: hx,c2000-arm-ni
Drop entire if. What is the point of it?
This attribute is used to identify different NI in my company's product.
But even if I don't give this attribute, nothing will be wrong in code.
However if I do that, I will couldn't know the relation between sysfs
file and hardware NI.
I will drop it. It will be as a normal way to identify NIs manually.
If there is only one NI and not give pccs-id, no thing wrong will happen.
+then:
+ required:
+ - pccs-id
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ ni-pmu@23ff0000 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
OK.
+ compatible = "hx,c2000-arm-ni";
+ #address-cells = <1>;
+ #size-cells = <1>;
Broken indentation.....
(...)
+ ni_pmu->dev = dev;
+ ni_pmu->ni = ni;
+ ni->ni_pmus[cd_idx] = ni_pmu;
+ }
+
+ devm_kfree(dev, cd_arrays);
+
+ pr_info("End discovering hardware registers.");
That's rather useless print. First entire driver must use dev_*. Second,
drop it, no need for end-of-function prints.
OK.
+
+ return 0;
+}
+
+static int ni_pmu_probe(struct platform_device *pdev)
+{
+ int ret, cd_num, idx, irq_num, irq_idx;
+ void __iomem *periphbase;
+ struct global_ni *ni;
+ struct device *dev = &pdev->dev;
+ char *name;
+ static int id;
+ struct ni_pmu *ni_pmu;
+
+ BUILD_BUG_ON(sizeof(long) == 4);
I am sorry, but what?
I only want to ensure 64 bit environment. Maybe there are many other way.
I will ensure that in Kconfig.
+ BUILD_BUG_ON(sizeof(struct ni_hw_perf_event) >
+ offsetof(struct hw_perf_event, target));
+#define NI_PMU_REG_MAP_SIZE 0xE08
+ BUILD_BUG_ON(sizeof(struct ni_pmu_reg_map) != NI_PMU_REG_MAP_SIZE);
+
+ pr_info("Begin probing.");
NAK, drop. Drop all silly entrance/exit messages. EVERYWHERE.
OK.
+
+ periphbase = devm_platform_ioremap_resource(pdev, 0);
+
Drop blank line
OK.
+ if (IS_ERR(periphbase)) {
+ pr_err("%s: ioremap error.", __func__);
dev_err and you need to print something useful, like error code. Or use
return dev_err_probe
OK.
+ return PTR_ERR(periphbase);
+ }
+
+ cd_num = ni_child_number_total(periphbase, periphbase, NI_CD);
+ pr_info("%d clock domains found in NI-700.", cd_num);
Really, what's with all this printing?
I just want to print how many clock domains.
One clock domain correspond to one PMU.
+
+ /* Each clock domain contains one PMU. So cd_num == pmu_num. */
+ ni = devm_kzalloc(dev,
+ struct_size(ni, ni_pmus, cd_num),
+ GFP_KERNEL);
+ if (!ni)
+ return -ENOMEM;
+
+ ni->cd_num = cd_num;
+ ni->base = periphbase;
+ ni->dev = dev;
+ ni->on_cpu = raw_smp_processor_id();
+ platform_set_drvdata(pdev, ni);
+
+ ret = ni_discovery(ni);
+ if (ret) {
+ pr_err("%s: discovery error.", __func__);
+ return ret;
+ }
+
+ irq_num = platform_irq_count(pdev);
+ /* Support that one NI with one irq or one clock domain with one irq. */
+ if (irq_num < 0 || (irq_num != 1 && irq_num != ni->cd_num)) {
+ pr_err("Error in irq number: %d.", irq_num);
+ return -EINVAL;
+ }
+
+ if (irq_num != cd_num) {
+ pr_warn("Only one IRQ found for all PMU.");
+ ret = ni_pmu_irq_setup(ni->ni_pmus[0], 0);
+ if (ret)
+ return ret;
+ }
+
+ ni->irq_num = irq_num;
+
+ for (idx = 0, irq_idx = 0; idx < ni->pmu_num; idx++) {
+ ni_pmu = ni->ni_pmus[idx];
+ ret = ni_pmu_init_attr_groups(ni_pmu);
+ if (ret)
+ return ret;
+
+ if (irq_num == cd_num) {
+ ret = ni_pmu_irq_setup(ni_pmu, irq_idx++);
+ if (ret)
+ return ret;
+ }
+
+ ni_pmu_reset(ni_pmu);
+
+ ni_pmu->pmu = (struct pmu) {
+ .module = THIS_MODULE,
+ .task_ctx_nr = perf_invalid_context,
+ .pmu_enable = ni_pmu_enable,
+ .pmu_disable = ni_pmu_disable,
+ .event_init = ni_pmu_event_init,
+ .add = ni_pmu_event_add,
+ .del = ni_pmu_event_del,
+ .start = ni_pmu_event_start,
+ .stop = ni_pmu_event_stop,
+ .read = ni_pmu_event_read,
+ .attr_groups = ni_pmu->pmu.attr_groups,
+ .capabilities = PERF_PMU_CAP_NO_EXCLUDE | PERF_PMU_CAP_NO_INTERRUPT,
+ };
+
+#ifdef CONFIG_PPC_HX_C2000
+ if (of_property_read_u32(pdev->dev.of_node, "pccs-id", &id))
+ pr_warn("No found pccs_id in dts ni pmu node.");
+#endif
+ if (cd_num > 1)
+ name = devm_kasprintf(dev, GFP_KERNEL, "ni_pmu_%d_%d", id++, idx);
+ else
+ name = devm_kasprintf(dev, GFP_KERNEL, "ni_pmu_%d", id++);
+
+ ret = perf_pmu_register(&ni_pmu->pmu, name, -1);
+ if (ret) {
+ pr_err("Error %d_%d registering PMU", id - 1, idx);
+ return ret;
+ }
+ }
+
+ ret = cpuhp_state_add_instance_nocalls(ni_hp_state,
+ &ni->node);
+ if (ret)
+ return ret;
+ pr_info("End probing.");
No, drop.
+
+ return 0;
+}
+
+static int ni_pmu_remove(struct platform_device *pdev)
+{
+ struct global_ni *ni = platform_get_drvdata(pdev);
+ int idx;
+
+ for (idx = 0; idx < ni->pmu_num; idx++)
+ perf_pmu_unregister(&ni->ni_pmus[idx]->pmu);
+
+ cpuhp_remove_multi_state(ni_hp_state);
+ return 0;
+}
+
+static const struct of_device_id ni_pmu_of_match[] = {
+ { .compatible = "hx,c2000-arm-ni" },
+ {},
+};
+
+static struct platform_driver ni_pmu_driver = {
+ .driver = {
+ .name = "ni-pmu",
+ .of_match_table = of_match_ptr(ni_pmu_of_match),
Drop of_match_ptr. Leads to warnings.
OK.
...
+
+module_init(ni_pmu_init);
+module_exit(ni_pmu_exit);
+/* PLATFORM END */
Useless comment. Please clean your code from debugging or useless markers.
Done.
Best regards,
Krzysztof