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.
Jonathan
> 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.
> +
> +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.
> +};
>
> +
> +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.
> +};
> +
> +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.
> + 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.
> + 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()
> + 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.
> + if (ret) {
> + clk_disable_unprepare(pmu->clk);
As above.
> + 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.
> + .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.
> + { 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.
> +
> +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!
> +};
> +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");
>
