On Tue, 09 Sep 2025 12:12:20 +0200
Clément Le Goffic <legoffic.clement@xxxxxxxxx> wrote:
> From: Clément Le Goffic <clement.legoffic@xxxxxxxxxxx>
>
> 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>
> Signed-off-by: Clément Le Goffic <legoffic.clement@xxxxxxxxx>
Hi Clément
A quick drive by review,
J
> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
> new file mode 100644
> index 000000000000..38328663d2c5
> --- /dev/null
> +++ b/drivers/perf/stm32_ddr_pmu.c
> @@ -0,0 +1,897 @@
> +
> +#define MP1_CLR_CNT GENMASK(3, 0)
> +#define MP1_CLR_TIME BIT(31)
> +#define MP2_CLR_CNT GENMASK(7, 0)
> +#define MP2_CLR_TIME BIT(8)
> +
> +/* 4 event counters plus 1 dedicated to time */
> +#define MP1_CNT_NB (4 + 1)
This is never used so I would drop it and rename the MP2_CNT_NB
to indicate it is the max value for any devices supported.
> +/* Index of the time dedicated counter */
> +#define MP1_TIME_CNT_IDX 4
> +
> +/* 8 event counters plus 1 dedicated to time */
> +#define MP2_CNT_NB (8 + 1)
...
> +struct stm32_ddr_pmu {
> + struct pmu pmu;
> + void __iomem *membase;
> + struct device *dev;
> + struct clk *clk;
> + const struct stm32_ddr_pmu_cfg *cfg;
> + struct hrtimer hrtimer;
> + ktime_t poll_period;
> + int selected_set;
> + u32 dram_type;
> + struct list_head counters[];
The absence of a __counted_by() marking made me wonder how
we ensured that this wasn't overrun. I see below that's because
size is always the same. So
struct list_head counters[MP2_CNT_NB];
If you do want to make it dynamic then that is fine but added
a local variable for the size and the __counted_by() marking so
the various analysis tools can check for buffer overruns.
> +};
> +static void stm32_ddr_pmu_event_del(struct perf_event *event, int flags)
> +{
> + struct stm32_ddr_pmu *pmu = to_stm32_ddr_pmu(event->pmu);
> + struct stm32_ddr_cnt *counter = event->pmu_private;
> + bool events = true;
Always set before use, so don't set it here. I'd move this into the
scope of the for loop to make this more obvious.
> +
> + stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> +
> + stm32_ddr_pmu_free_counter(pmu, counter);
> +
> + for (int i = 0; i < pmu->cfg->counters_nb; i++) {
> + events = !list_empty(&pmu->counters[i]);
> + if (events) /* If there is activity nothing to do */
> + return;
> + }
> +
> + hrtimer_cancel(&pmu->hrtimer);
> + stm32_ddr_stop_counters(pmu);
> +
> + pmu->selected_set = -1;
> +
> + clk_disable(pmu->clk);
> +}
> +
> +#define STM32_DDR_PMU_EVENT_ATTR(_name, _id) \
> + PMU_EVENT_ATTR_ID(_name, stm32_ddr_pmu_sysfs_show, _id)
> +
> +static struct attribute *stm32_ddr_pmu_events_attrs_mp[] = {
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_rd, PERF_OP_IS_RD),
Prefixing perf events with perf_ seems unnecessary.
I guess perf_op_is_rd is counting all reads? Is so why not call it simply 'reads'
or something else short like that? If it's cycles when a read is going on then
maybe a more complex is needed, but perf_op_is_rd doesn't convey that to me.
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_wr, PERF_OP_IS_WR),
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_activate, PERF_OP_IS_ACTIVATE),
> + STM32_DDR_PMU_EVENT_ATTR(ctl_idle, CTL_IDLE),
> + STM32_DDR_PMU_EVENT_ATTR(perf_hpr_req_with_no_credit, PERF_HPR_REQ_WITH_NO_CREDIT),
> + STM32_DDR_PMU_EVENT_ATTR(perf_lpr_req_with_no_credit, PERF_LPR_REQ_WITH_NO_CREDIT),
> + STM32_DDR_PMU_EVENT_ATTR(cactive_ddrc, CACTIVE_DDRC),
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_enter_powerdown, PERF_OP_IS_ENTER_POWERDOWN),
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_refresh, PERF_OP_IS_REFRESH),
> + STM32_DDR_PMU_EVENT_ATTR(perf_selfresh_mode, PERF_SELFRESH_MODE),
> + STM32_DDR_PMU_EVENT_ATTR(dfi_lp_req, DFI_LP_REQ),
> + STM32_DDR_PMU_EVENT_ATTR(perf_hpr_xact_when_critical, PERF_HPR_XACT_WHEN_CRITICAL),
> + STM32_DDR_PMU_EVENT_ATTR(perf_lpr_xact_when_critical, PERF_LPR_XACT_WHEN_CRITICAL),
> + STM32_DDR_PMU_EVENT_ATTR(perf_wr_xact_when_critical, PERF_WR_XACT_WHEN_CRITICAL),
> + STM32_DDR_PMU_EVENT_ATTR(dfi_lp_req_cpy, DFI_LP_REQ), /* Suffixed '_cpy' to allow the
> + * choice between sets 2 and 3
> + */
> + STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
> + NULL
> +};
> +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 using a fixed number of counters why not put it in the struct
definition and simplify the code? I agree it is probably not
worth making this dynamic given small sizes but I don't mind
if you do want to do this. The only thing that isn't a good idea
is this dynamic, but not really, current situation.
> + if (!pmu)
> + return -ENOMEM;
> +static DEFINE_SIMPLE_DEV_PM_OPS(stm32_ddr_pmu_pm_ops, NULL, stm32_ddr_pmu_device_resume);
> +
> +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
> + {
> + .compatible = "st,stm32mp131-ddr-pmu",
> + .data = &stm32_ddr_pmu_cfg_mp1
Trivial but if you are spinning again, normal convention is trailing commas
in cases like this because maybe other fields will get set later.
> + },
> + {
> + .compatible = "st,stm32mp251-ddr-pmu",
> + .data = &stm32_ddr_pmu_cfg_mp2
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stm32_ddr_pmu_of_match);
> +
> +static struct platform_driver stm32_ddr_pmu_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .pm = pm_sleep_ptr(&stm32_ddr_pmu_pm_ops),
> + .of_match_table = stm32_ddr_pmu_of_match,
> + },
> + .probe = stm32_ddr_pmu_device_probe,
> + .remove = stm32_ddr_pmu_device_remove,
> +};
> +
> +module_platform_driver(stm32_ddr_pmu_driver);
> +
> +MODULE_AUTHOR("Clément Le Goffic");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 DDR performance monitor driver");
> +MODULE_LICENSE("GPL");
>
