Re: [PATCH v3 12/19] perf: stm32: introduce DDRPERFM driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 22 Jul 2025 16:03:29 +0200
Clément Le Goffic <clement.legoffic@foss.st.com> 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@foss.st.com>
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@foss.st.com> 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");

> +}