On 29/06/2026 08:10, Shivani Nittor wrote:
> Introduce a DTS-based PMU driver that discovers PMU information
> from the device tree and registers a PowerPC PMU instance at
> runtime.
>
> The driver parses basic PMU properties such as the number of
> counters and MMCR register definitions, creates sysfs event
> entries from the device tree event descriptions, and registers
> the PMU with the perf subsystem.
>
> To support dynamic PMU registration, add PMU unregistration
> support and create a platform device for PMU nodes described
> in the device tree.
>
> This forms the foundation for moving PMU descriptions out of
> architecture-specific kernel code and into device tree data.
>
> The driver implements the following functionality:
>
> Device Tree Parsing:
> - Reads PMU properties including counter count (nr_pmc), PMU name,
> version, and platform information
> - Parses MMCR (Monitor Mode Control Register) definitions from the
> sprs/mmcr node hierarchy, storing the MMCR register SPRNs
> - Extracts event definitions from the events node, supporting both
> 32-bit and 64-bit event codes
>
> Dynamic Event Registration:
> - Creates sysfs event entries dynamically from device tree event
> descriptions
> - Generates event attributes with format "event=0x<code>" for each
> discovered event
> - Exposes events under /sys/devices/<pmu-name>/events/
>
> PMU Registration:
> - Registers a power_pmu instance with the perf subsystem using the
> device tree-provided PMU name
> - Adds PMU unregistration support to enable proper cleanup
> - Creates platform device for "ibm,power-pmu" compatible nodes
>
> Sysfs Interface:
> - Provides format attributes (event, pmcsel) under
> /sys/devices/<pmu-name>/format/
> - Exposes device tree debug information (nr_pmc) under
> /sys/devices/<pmu-name>/dt/
Where did you document the ABI?
...
> +static struct power_pmu dts_pmu = {
> + .name = "cpu_dts",
> + .n_counter = MAX_PMU_COUNTERS,
> + .attr_groups = pmu_dts_attr_groups,
> +};
> +
> +/* Device Tree match */
That's obvious. Can of_device_id be something else than DT match?
> +static const struct of_device_id pmu_dts_of_match[] = {
> + { .compatible = "ibm,power-pmu" },
Where is ABI documented?
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pmu_dts_of_match);
> +
> +/* Probe function */
> +static int pmu_dts_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node, *child, *events_np;
> + struct device_node *sprs_np, *mmcr_np, *mmcr_child;
> + struct pmu_dts_event *evt;
> + u64 code;
> + u32 code32, sprn;
> + u32 code64[2];
> + u32 code128[4];
> + int cells;
> + const char *str;
> +
> + pr_info("PMU DTS probe node = %s\n", np->full_name);
This does not look like useful printk message. Drivers should be silent
on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
> +
> + if (!of_property_present(np, "nr_pmc")) {
> +
> + for_each_child_of_node(np, child) {
> + pr_info("child node = %s\n", child->full_name);
Why are you sending debugging code?
> +
> + if (of_property_present(child, "nr_pmc")) {
> + np = child;
> + break;
> + }
> + }
> + }
> +
> + if (of_property_read_u32(np, "nr_pmc", &pmu_dts_nr_pmc)) {
> + pr_err("pmu_dts: nr_pmc not found in %s\n", np->full_name);
Why isn't this using dev interface?
> + return -EINVAL;
> + }
> +
> + if (!of_property_read_string(np, "pmu-name", &str))
> + pr_info("PMU Name: %s\n", str);
> +
> + if (!of_property_read_string(np, "pmu-version", &str))
> + pr_info("PMU Version: %s\n", str);
> +
> + if (!of_property_read_string(np, "platform", &str))
No, you cannot just add bunch of undocumented ABI and call it a day.
> + pr_info("Platform: %s\n", str);
> +
> + sprs_np = of_get_child_by_name(np, "sprs");
> + if (!sprs_np) {
> + pr_err("pmu_dts: no sprs node\n");
> + return -EINVAL;
> + }
> +
> + mmcr_np = of_get_child_by_name(sprs_np, "mmcr");
> + if (!mmcr_np) {
> + pr_err("pmu_dts: no mmcr node\n");
> + return -EINVAL;
> + }
> +
> + mmcr_count = 0;
> + for_each_child_of_node(mmcr_np, mmcr_child) {
> +
> + if (of_property_read_u32(mmcr_child, "sprn", &sprn))
> + continue;
> +
> + mmcr_regs_sprs[mmcr_count++] = sprn;
> + pr_info("pmu_dts: MMCR[%d] = %u (%s)\n", mmcr_count - 1,
> + sprn, mmcr_child->name);
> +
> + if (mmcr_count >= MAX_MMCR)
> + break;
Where is cleanup?
> + }
> +
> + if (!mmcr_count) {
> + pr_err("pmu_dts: no MMCR SPRs found\n");
> + return -EINVAL;
> + }
> +
> + /* Parse events */
> + events_np = of_get_child_by_name(np, "events");
> + if (!events_np) {
> + pr_err("pmu_dts: no events node found\n");
> + return -EINVAL;
> + }
> +
> + dts_event_count = 0;
> + for_each_child_of_node(events_np, child) {
> + if (!of_device_is_available(child))
Why are you open-coding available variant of loop?
> + continue;
> +
> + cells = of_property_count_u32_elems(child, "event_code");
> + if (cells == 1) {
> + if (of_property_read_u32(child, "event_code", &code32))
> + continue;
> +
> + code = code32;
> +
> + } else if (cells == 2) {
> + if (of_property_read_u32_array(child, "event_code",
> code64, 2))
> + continue;
> + code = ((u64)code64[0] << 32) | code64[1];
> +
> + } else if (cells == 4) {
> + if (of_property_read_u32_array(child, "event_code",
> code128, 4))
> + continue;
> + code = ((u64)code128[1] << 32) | code128[3];
> +
> + } else {
> + pr_warn("pmu_dts: invalid event_code for %s\n",
> child->name);
> + continue;
> + }
> +
> + evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> + if (!evt)
> + continue;
> +
> + snprintf(evt->name, sizeof(evt->name), "%s", child->name);
> + snprintf(evt->config, sizeof(evt->config), "event=0x%llx",
> code);
> +
> + sysfs_attr_init(&evt->attr.attr);
> + evt->attr.attr.name = evt->name;
> + evt->attr.attr.mode = 0444;
> + evt->attr.show = pmu_dts_event_show;
> + dts_events[dts_event_count] = evt;
> + pmu_dts_events_attrs[dts_event_count] = &evt->attr.attr;
> + dts_event_count++;
> +
> + if (dts_event_count >= MAX_DTS_EVENTS)
> + break;
And node cleanup?
> + }
> + pmu_dts_events_attrs[dts_event_count] = NULL;
> +
> + /* Register PMU */
No.
> + pr_info("pmu_dts: registering PMU\n");
No.
> + return register_power_pmu(&dts_pmu);
> +}
> +
> +/* Platform driver */
This is ridicilous comment.
> +static struct platform_driver pmu_dts_driver = {
> + .probe = pmu_dts_probe,
> + .driver = {
> + .name = "pmu_dts",
> + .of_match_table = pmu_dts_of_match,
> + },
> +};
> +
> +static int __init pmu_dts_init(void)
> +{
> + pr_info("pmu_dts: init\n");
NAK, init functions do not print anything. RFC does not mean you can
send debugging code with poor quality and...
> + return platform_driver_register(&pmu_dts_driver);
> +}
> +
> +static void __exit pmu_dts_exit(void)
> +{
> + pr_info("pmu_dts: exit\n");
> + platform_driver_unregister(&pmu_dts_driver);
> + unregister_power_pmu(&dts_pmu);
> +}
> +module_init(pmu_dts_init);
> +module_exit(pmu_dts_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Shivani Nittor");
> +MODULE_DESCRIPTION("PMU DTS driver");
> diff --git a/arch/powerpc/perf/internal.h b/arch/powerpc/perf/internal.h
> index a70ac471a5a5..11154ee31f8e 100644
> --- a/arch/powerpc/perf/internal.h
> +++ b/arch/powerpc/perf/internal.h
> @@ -2,6 +2,7 @@
> //
> // Copyright 2019 Madhavan Srinivasan, IBM Corporation.
>
> +void unregister_power_pmu(struct power_pmu *pmu);
> int __init init_ppc970_pmu(void);
> int __init init_power5_pmu(void);
> int __init init_power5p_pmu(void);
> diff --git a/arch/powerpc/perf/isa207-common.c
> b/arch/powerpc/perf/isa207-common.c
> index 2b3547fdba4a..e11d1bbbc27b 100644
> --- a/arch/powerpc/perf/isa207-common.c
> +++ b/arch/powerpc/perf/isa207-common.c
> @@ -7,6 +7,7 @@
> * Copyright 2016 Madhavan Srinivasan, IBM Corporation.
> */
> #include "isa207-common.h"
> +#include <asm/dts_pmu.h>
>
> PMU_FORMAT_ATTR(event, "config:0-49");
> PMU_FORMAT_ATTR(pmcxsel, "config:0-7");
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c
> index 1946dbdc9fa1..35be67eef2c6 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -947,6 +947,18 @@ static void __init opal_imc_init_dev(void)
>
> of_node_put(np);
> }
> +#define PMU_DTB "ibm,power-pmu"
> +
> +static void __init opal_pmus_init_dev(void)
> +{
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, PMU_DTB);
> + if (np)
> + of_platform_device_create(np, NULL, NULL);
> +
> + of_node_put(np);
> +}
>
> static int kopald(void *unused)
> {
> @@ -1032,6 +1044,9 @@ static int __init opal_init(void)
> /* Detect In-Memory Collection counters and create devices*/
> opal_imc_init_dev();
>
> + /*Detect PMU node and create device*/
.... with completely broken indentation style.
> + opal_pmus_init_dev();
> +
> /* Create leds platform devices */
> leds = of_find_node_by_path("/ibm,opal/leds");
> if (leds) {
Best regards,
Krzysztof
