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

[Clement LE GOFFIC <clement.legoffic@foss.st.com>]

Hi Jonathan

On 7/25/25 12:56, Jonathan Cameron wrote:
> 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?
For of_device_id and platform_device structs
> Looks like of.h is needed so you should include that directly.
> 
> Check all your headers.  mod_devicetable.h should be here
> for instance.

mod_devicetable.h is already included in of_platform.h but imagine it 
should be directly include as I do not use any symbol from of_platform.h

Thank you for pointing this out.
I'll have a look on other includes.

Do you have any method to include directly what you need ?
As of here symbols were available through other include but this is not 
correct.
Is there any tool nor tips to find the right include directly and easily ?

>> +#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.

Indeed the test must be in the for loop, thanks.
The idea here is to loop over the counters and check if one of them is 
still active so we don't stop the HW.

I'll do:
	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;
	}

> 
>> +
>> +	/* 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?
I don't think it would be worth it but if wanted I can switch.
>
>> +		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.

Ack.

>> +};
> 
>> +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.
Indeed.

>> +
>> +	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");
> 
>> +}

Best regards,
Clément