Re: [RFC 1/3] powerpc/perf: Register PMU from device tree and expose events

[Krzysztof Kozlowski <krzk@kernel.org>]

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