Re: [PATCH v2 09/16] perf: stm32: introduce DDRPERFM driver

From: Clement LE GOFFIC
Date: Tue Jul 15 2025 - 05:53:56 EST

Hi Jonathan,

On 7/11/25 18:04, Jonathan Cameron wrote:
On Fri, 11 Jul 2025 16:49:01 +0200
Clément Le Goffic <clement.legoffic@xxxxxxxxxxx> wrote:

Introduce the driver for the DDR Performance Monitor available on
STM32MPU SoC.

On STM32MP2 platforms, the DDRPERFM allows to monitor up to 8 DDR events
that come from the DDR Controller such as read or write events.

On STM32MP1 platforms, the DDRPERFM cannot monitor any event on any
counter, there is a notion of set of events.
Events from different sets cannot be monitored at the same time.
The first chosen event selects the set.
The set is coded in the first two bytes of the config value which is on 4
bytes.

On STM32MP25x series, the DDRPERFM clock is shared with the DDR controller
and may be secured by bootloaders.
Access controllers allow to check access to a resource. Use the access
controller defined in the devicetree to know about the access to the
DDRPERFM clock.

Signed-off-by: Clément Le Goffic <clement.legoffic@xxxxxxxxxxx>

Hi Clément,

A quick drive by review as it's Friday afternoon and I was curious..

Mostly superficial stuff. I didn't look closely at the perf logic.

Thank you for the review.
The perf logic is new to me so if you have any suggestion, you're welcome.



diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
new file mode 100644
index 000000000000..1be5bbe12978
--- /dev/null
+++ b/drivers/perf/stm32_ddr_pmu.c
@@ -0,0 +1,910 @@

+#define EVENT_NUMBER(group, index) (((group) << 8) | (index))
+#define GROUP_VALUE(event_number) ((event_number) >> 8)
+#define EVENT_INDEX(event_number) ((event_number) & 0xFF)

Prefix these macro names with something driver specific. They are
very likely to clash with something in a header in future otherwise.

Ok


+
+enum stm32_ddr_pmu_memory_type {
+ STM32_DDR_PMU_LPDDR4,
+ STM32_DDR_PMU_LPDDR3,
+ STM32_DDR_PMU_DDR4,
+ STM32_DDR_PMU_DDR3

This should have a trailing comma as might well be more
added in future if this IP gets used in more devices.

Ok

+};



+
+static const struct attribute_group *stm32_ddr_pmu_attr_groups_mp2[] = {
+ &stm32_ddr_pmu_events_attrs_group_mp2,
+ &stm32_ddr_pmu_format_attr_group,
+ NULL,

No comma needed on terminating entries.

Ok, will also be fixed for `stm32_ddr_pmu_attr_groups_mp1[]` and
`stm32_ddr_pmu_format_attrs[]`

+};
+
+static int stm32_ddr_pmu_device_probe(struct platform_device *pdev)
+{
+ struct stm32_firewall firewall;
+ struct stm32_ddr_pmu *pmu;
+ struct reset_control *rst;
+ struct resource *res;
+ int ret;
+
+ pmu = devm_kzalloc(&pdev->dev, struct_size(pmu, counters, MP2_CNT_NB), GFP_KERNEL);
+ if (!pmu)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pmu);
+ pmu->dev = &pdev->dev;
+
+ pmu->cfg = device_get_match_data(&pdev->dev);
+
+ pmu->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+ if (IS_ERR(pmu->membase))
+ return PTR_ERR(pmu->membase);
+
+ if (of_property_present(pmu->dev->of_node, "access-controllers")) {
+ ret = stm32_firewall_get_firewall(pmu->dev->of_node, &firewall, 1);
+ if (ret)
+ return dev_err_probe(pmu->dev, ret, "Failed to get firewall\n");
+ ret = stm32_firewall_grant_access_by_id(&firewall, firewall.firewall_id);
+ if (ret)
+ return dev_err_probe(pmu->dev, ret, "Failed to grant access\n");
+ }
+
+ pmu->clk = devm_clk_get_optional_prepared(pmu->dev, NULL);

Given there are quite a few uses of pmu->dev, maybe worth a local
struct device *dev = &pdev->dev; at the top and use dev to replace all these.

As I need pmu->dev elsewhere in the driver I'll stick to it and replace all &pdev->dev


+ if (IS_ERR(pmu->clk))
+ return dev_err_probe(pmu->dev, PTR_ERR(pmu->clk), "Failed to get prepare clock\n");
+
+ clk_enable(pmu->clk);
+
+ rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

You mix and match between pdev->dev, and pmu->dev. Good to pick one or use local
variable as suggested above.

Ok

+ if (IS_ERR(rst)) {
+ clk_disable_unprepare(pmu->clk);
Given use of _prepared() get above. This doesn't look right - the unprepare
should be handled by devm unwinding. clk_disable()

Oh you're right, I can fix this unwinding issue by using `devm_clk_get_optional_enabled()` instead of `devm_clk_get_optional_prepared()` and remove the `clk_enable()` so all `clk_disable_unprepare()` disappear from the probe


+ return dev_err_probe(&pdev->dev, PTR_ERR(rst), "Failed to get reset\n");
+ }
+
+ reset_control_assert(rst);
+ reset_control_deassert(rst);
+
+ pmu->poll_period = ms_to_ktime(POLL_MS);
+ hrtimer_setup(&pmu->hrtimer, stm32_ddr_pmu_poll, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+
+ for (int i = 0; i < MP2_CNT_NB; i++)
+ INIT_LIST_HEAD(&pmu->counters[i]);
+
+ pmu->selected_set = -1;
+
+ pmu->pmu = (struct pmu) {
+ .task_ctx_nr = perf_invalid_context,
+ .start = stm32_ddr_pmu_event_start,
+ .stop = stm32_ddr_pmu_event_stop,
+ .add = stm32_ddr_pmu_event_add,
+ .del = stm32_ddr_pmu_event_del,
+ .read = stm32_ddr_pmu_event_read,
+ .event_init = stm32_ddr_pmu_event_init,
+ .attr_groups = pmu->cfg->attribute,
+ .module = THIS_MODULE,
+ };
+
+ ret = perf_pmu_register(&pmu->pmu, DRIVER_NAME, -1);

Calling this exposes user interfaces etc. Does it really make sense to
do that and then write another register? I'd normally expect this
last in probe.

Indeed, will move it at the end of the probe


+ if (ret) {
+ clk_disable_unprepare(pmu->clk);

As above.

Ok

+ return dev_err_probe(&pdev->dev, ret,
+ "Couldn't register DDRPERFM driver as a PMU\n");
+ }
+
+ if (pmu->cfg->regs->dram_inf.reg) {
+ ret = stm32_ddr_pmu_get_memory_type(pmu);
+ if (ret) {
+ perf_pmu_unregister(&pmu->pmu);
+ clk_disable_unprepare(pmu->clk);
+ return dev_err_probe(&pdev->dev, ret, "Failed to get memory type\n");
+ }
+
+ writel_relaxed(pmu->dram_type, pmu->membase + pmu->cfg->regs->dram_inf.reg);
+ }
+
+ clk_disable(pmu->clk);
+
+ return 0;
+}

+static const struct stm32_ddr_pmu_regspec stm32_ddr_pmu_regspec_mp2 = {
+ .stop = { DDRPERFM_CTRL, CTRL_STOP },
+ .start = { DDRPERFM_CTRL, CTRL_START },
+ .status = { DDRPERFM_MP2_STATUS, MP2_STATUS_BUSY },
+ .clear_cnt = { DDRPERFM_CLR, MP2_CLR_CNT},
+ .clear_time = { DDRPERFM_CLR, MP2_CLR_TIME},

Spaces before } are missing
There are a few others above that I'll not mention directly.

Ok thanks



+ .cfg0 = { DDRPERFM_MP2_CFG0 },
+ .cfg1 = { DDRPERFM_MP2_CFG1 },
+ .enable = { DDRPERFM_MP2_CFG5 },
+ .dram_inf = { DDRPERFM_MP2_DRAMINF },
+ .counter_time = { DDRPERFM_MP2_TCNT },
+ .counter_evt = {
+ { DDRPERFM_MP2_EVCNT(0) },
Somewhat unusual formatting though neat I guess so fine if you
really like it!.
.counter_evt = {
{ DDRPERFM_MP2_EVCNT(0) },

would be what I'd normally expect.

I'll stick to normality, don't wanna be special here

+ { DDRPERFM_MP2_EVCNT(1) },
+ { DDRPERFM_MP2_EVCNT(2) },
+ { DDRPERFM_MP2_EVCNT(3) },
+ { DDRPERFM_MP2_EVCNT(4) },
+ { DDRPERFM_MP2_EVCNT(5) },
+ { DDRPERFM_MP2_EVCNT(6) },
+ { DDRPERFM_MP2_EVCNT(7) },
+ },
+};
+
+static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp1 = {
+ .regs = &stm32_ddr_pmu_regspec_mp1,
+ .attribute = stm32_ddr_pmu_attr_groups_mp1,
+ .counters_nb = MP1_CNT_NB,
+ .evt_counters_nb = MP1_CNT_NB - 1, /* Time counter is not an event counter */
+ .time_cnt_idx = MP1_TIME_CNT_IDX,
+ .get_counter = stm32_ddr_pmu_get_event_counter_mp1,
+};
+
+static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp2 = {
+ .regs = &stm32_ddr_pmu_regspec_mp2,
+ .attribute = stm32_ddr_pmu_attr_groups_mp2,
+ .counters_nb = MP2_CNT_NB,
+ .evt_counters_nb = MP2_CNT_NB - 1, /* Time counter is an event counter */
+ .time_cnt_idx = MP2_TIME_CNT_IDX,
+ .get_counter = stm32_ddr_pmu_get_event_counter_mp2,
+};
+
+static const struct dev_pm_ops stm32_ddr_pmu_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(NULL, stm32_ddr_pmu_device_resume)
+};

static DEFINE_SIMPLE_DEV_PM_OPS() looks appropriate here.

Indeed, Thank you



+
+static const struct of_device_id stm32_ddr_pmu_of_match[] = {
+ {
+ .compatible = "st,stm32mp131-ddr-pmu",
+ .data = &stm32_ddr_pmu_cfg_mp1
+ },
+ {
+ .compatible = "st,stm32mp251-ddr-pmu",
+ .data = &stm32_ddr_pmu_cfg_mp2
+ },
+ { },

No comma need after terminating entry. Nice to make it hard
to accidentally add entries after one of these!

Yes, I'll fix it

Best regards,
Clément