Re: [PATCH 06/13] perf: stm32: introduce DDRPERFM driver

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

Hi Philip

On 6/30/25 10:38, Philipp Zabel wrote:
> On Mo, 2025-06-23 at 11:27 +0200, Clément Le Goffic 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>
>> ---
>>   drivers/perf/Kconfig         |  11 +
>>   drivers/perf/Makefile        |   1 +
>>   drivers/perf/stm32_ddr_pmu.c | 893 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 905 insertions(+)
>>
> [...]
>> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
>> new file mode 100644
>> index 000000000000..c0bce1f446a0
>> --- /dev/null
>> +++ b/drivers/perf/stm32_ddr_pmu.c
>> @@ -0,0 +1,893 @@
> [...]
>> +	if (of_property_present(pdev->dev.of_node, "resets")) {
>> +		rst = devm_reset_control_get(&pdev->dev, NULL);
> 
> Use devm_reset_control_get_optional_exclusive() instead. It returns
> NULL if the device tree doesn't contain a resets property.

Ok I will have a look, thank you

> 
>> +		if (IS_ERR(rst)) {
>> +			dev_err(&pdev->dev, "Failed to get reset\n");
> 
> Please consider using dev_err_probe() instead.

Ok

>> +			ret = PTR_ERR(rst);
>> +			goto err_clk;
>> +		}
>> +		reset_control_assert(rst);
>> +		reset_control_deassert(rst);
> 
> These can be done unconditionally, as they are no-ops for rst == NULL.

Indeed

Best regards,
Clément