On Tue, 22 Jul 2025 16:03:29 +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,
Minor comments inline.,
Thanks,
Jonathan
> --- /dev/null
> +++ b/drivers/perf/stm32_ddr_pmu.c
> @@ -0,0 +1,896 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025, STMicroelectronics - All Rights Reserved
> + * Author: Clément Le Goffic <clement.legoffic@xxxxxxxxxxx> for STMicroelectronics.
> + */
> +
> +#include <linux/bus/stm32_firewall_device.h>
> +#include <linux/clk.h>
> +#include <linux/hrtimer.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
Why?
Looks like of.h is needed so you should include that directly.
Check all your headers. mod_devicetable.h should be here
for instance.
> +#include <linux/perf_event.h>
> +#include <linux/reset.h>
> +
> +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;
> +
> + 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]);
What is this trying to do? It seems to be only setting
events = !list_empty(&pmu->counters[pmu->cfg_counters_nb - 1]);
If so just check that but my guess it you care if there is anything
in any of them lists.
> +
> + /* If there is activity nothing to do */
> + if (events)
> + return;
> +
> + hrtimer_cancel(&pmu->hrtimer);
> + stm32_ddr_stop_counters(pmu);
> +
> + pmu->selected_set = -1;
> +
> + clk_disable(pmu->clk);
> +}
> +static int stm32_ddr_pmu_get_memory_type(struct stm32_ddr_pmu *pmu)
> +{
> + struct platform_device *pdev = to_platform_device(pmu->dev);
> + struct device_node *memchan;
> +
> + memchan = of_parse_phandle(pdev->dev.of_node, "memory-channel", 0);
> + if (!memchan)
> + return dev_err_probe(&pdev->dev, -EINVAL,
> + "Missing device-tree property 'memory-channel'\n");
> +
> + if (of_device_is_compatible(memchan, "jedec,lpddr4-channel"))
Random thought, feel free to ignore.
I wonder if it's worth using an of_device_id match table here?
> + pmu->dram_type = STM32_DDR_PMU_LPDDR4;
> + else if (of_device_is_compatible(memchan, "jedec,lpddr3-channel"))
> + pmu->dram_type = STM32_DDR_PMU_LPDDR3;
> + else if (of_device_is_compatible(memchan, "jedec,ddr4-channel"))
> + pmu->dram_type = STM32_DDR_PMU_DDR4;
> + else if (of_device_is_compatible(memchan, "jedec,ddr3-channel"))
> + pmu->dram_type = STM32_DDR_PMU_DDR3;
> + else
> + return dev_err_probe(&pdev->dev, -EINVAL, "Unsupported memory channel type\n");
> +
> + if (pmu->dram_type == STM32_DDR_PMU_LPDDR3)
> + dev_warn(&pdev->dev,
> + "LPDDR3 supported by DDRPERFM but not supported by DDRCTRL/DDRPHY\n");
> +
> + return 0;
> +}
> +static struct attribute *stm32_ddr_pmu_events_attrs_mp[] = {
> + STM32_DDR_PMU_EVENT_ATTR(perf_op_is_rd, PERF_OP_IS_RD),
> + 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,
No trailing comma for a terminating entry like this. You got the other cases
so I guess this one just got missed.
> +};
> +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(pmu->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);
Jiri is busy driving dev_fwnode() thorugh to get rid of all the directly references
to of_node. Probably better to use that here from the start.
> + 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_enabled(pmu->dev, NULL);
> + if (IS_ERR(pmu->clk))
> + return dev_err_probe(pmu->dev, PTR_ERR(pmu->clk), "Failed to get prepare clock\n");
Comment doesn't match code. This is going to enabled, not just prepared.
> +
> + rst = devm_reset_control_get_optional_exclusive(pmu->dev, NULL);
> + if (IS_ERR(rst))
> + return dev_err_probe(pmu->dev, PTR_ERR(rst), "Failed to get reset\n");
> +}
