Message-ID: <1a9ddd04-7877-4b4b-b746-0f3cf6ce0d8b@gmail.com>
Date: Thu, 11 Sep 2025 11:56:41 +0200
From: Clément Le Goffic <legoffic.clement@...il.com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Gatien Chevallier <gatien.chevallier@...s.st.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Gabriel Fernandez <gabriel.fernandez@...s.st.com>,
 Krzysztof Kozlowski <krzk@...nel.org>, Julius Werner <jwerner@...omium.org>,
 Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Philipp Zabel <p.zabel@...gutronix.de>, Jonathan Corbet <corbet@....net>,
 linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-perf-users@...r.kernel.org, linux-doc@...r.kernel.org,
 Clément Le Goffic <clement.legoffic@...s.st.com>
Subject: Re: [PATCH v6 13/20] perf: stm32: introduce DDRPERFM driver

On 10/09/2025 11:26, Jonathan Cameron wrote:
> On Tue, 09 Sep 2025 12:12:20 +0200
> Clément Le Goffic <legoffic.clement@...il.com> wrote:
> 
>> From: Clément Le Goffic <clement.legoffic@...s.st.com>
>>
>> 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@...s.st.com>
>> Signed-off-by: Clément Le Goffic <legoffic.clement@...il.com>
> Hi Clément
> 
> A quick drive by review,
> 

Hi Jonathan,

Thank you for the review, below are my answers

> 
>> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
>> new file mode 100644
>> index 000000000000..38328663d2c5
>> --- /dev/null
>> +++ b/drivers/perf/stm32_ddr_pmu.c
>> @@ -0,0 +1,897 @@
> 
>> +
>> +#define MP1_CLR_CNT		GENMASK(3, 0)
>> +#define MP1_CLR_TIME		BIT(31)
>> +#define MP2_CLR_CNT		GENMASK(7, 0)
>> +#define MP2_CLR_TIME		BIT(8)
>> +
>> +/* 4 event counters plus 1 dedicated to time */
>> +#define MP1_CNT_NB		(4 + 1)
> 
> This is never used so I would drop it and rename the MP2_CNT_NB
> to indicate it is the max value for any devices supported.

It is used in the stm32_ddr_pmu_cfg_mp1 struct which is the device 
platform data.
> 
> 
>> +/* Index of the time dedicated counter */
>> +#define MP1_TIME_CNT_IDX	4
>> +
>> +/* 8 event counters plus 1 dedicated to time */
>> +#define MP2_CNT_NB		(8 + 1)
> ...
> 
>> +struct stm32_ddr_pmu {
>> +	struct pmu pmu;
>> +	void __iomem *membase;
>> +	struct device *dev;
>> +	struct clk *clk;
>> +	const struct stm32_ddr_pmu_cfg *cfg;
>> +	struct hrtimer hrtimer;
>> +	ktime_t poll_period;
>> +	int selected_set;
>> +	u32 dram_type;
>> +	struct list_head counters[];
> The absence of a __counted_by() marking made me wonder how
> we ensured that this wasn't overrun.  I see below that's because
> size is always the same.  So
> 	struct list_head counters[MP2_CNT_NB];
> If you do want to make it dynamic then that is fine but added
> a local variable for the size and the __counted_by() marking so
> the various analysis tools can check for buffer overruns.

Oh I didn't know about this __counted_by compiler attribute.
I'll have a look and try to use it.
The array shouldn't have the same size basically it should depends on 
the platform counters number.
There is definitely something to rework regarding the allocation.
Thank you for pointing it.

> 
>> +};
> 
> 
> 
>> +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;
> 
> Always set before use, so don't set it here.  I'd move this into the
> scope of the for loop to make this more obvious.

Right, i'll remove the assignation.

> 
>> +
>> +	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]);
>> +		if (events) /* If there is activity nothing to do */
>> +			return;
>> +	}
>> +
>> +	hrtimer_cancel(&pmu->hrtimer);
>> +	stm32_ddr_stop_counters(pmu);
>> +
>> +	pmu->selected_set = -1;
>> +
>> +	clk_disable(pmu->clk);
>> +}
> 
>> +
>> +#define STM32_DDR_PMU_EVENT_ATTR(_name, _id)			\
>> +	PMU_EVENT_ATTR_ID(_name, stm32_ddr_pmu_sysfs_show, _id)
>> +
>> +static struct attribute *stm32_ddr_pmu_events_attrs_mp[] = {
>> +	STM32_DDR_PMU_EVENT_ATTR(perf_op_is_rd, PERF_OP_IS_RD),
> 
> Prefixing perf events with perf_ seems unnecessary.
> 
> I guess perf_op_is_rd is counting all reads?  Is so why not call it simply 'reads'
> or something else short like that?  If it's cycles when a read is going on then
> maybe a more complex is needed, but perf_op_is_rd doesn't convey that to me.

Here I just extracted the name of each event from the datasheet and for 
some of them there are prefixed by "perf_".
I do not have enough knowledge of the HW to just rename it read and let 
other event with their "scientific name".
To me I should stick to a policy either rename all the events with 
understandable names or keep all event names like this.
And I'm unable to find an understandable name for each event.

> 
>> +	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
>> +};
> 
> 
>> +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 using a fixed number of counters why not put it in the struct
> definition and simplify the code?  I agree it is probably not
> worth making this dynamic given small sizes but I don't mind
> if you do want to do this.  The only thing that isn't a good idea
> is this dynamic, but not really, current situation.

Yes something need reworks here as said above in your first comment.
I will try to find the best solution.

> 
>> +	if (!pmu)
>> +		return -ENOMEM;
> 
> 
> 
>> +static DEFINE_SIMPLE_DEV_PM_OPS(stm32_ddr_pmu_pm_ops, NULL, stm32_ddr_pmu_device_resume);
>> +
>> +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
>> +	{
>> +		.compatible = "st,stm32mp131-ddr-pmu",
>> +		.data = &stm32_ddr_pmu_cfg_mp1
> 
> Trivial but if you are spinning again, normal convention is trailing commas
> in cases like this because maybe other fields will get set later.
Yes this is something I should keep in mind each time I init a struct.
I'll fix it for the next version.

> 
>> +	},
>> +	{
>> +		.compatible = "st,stm32mp251-ddr-pmu",
>> +		.data = &stm32_ddr_pmu_cfg_mp2
>> +	},
>> +	{ }
>> +};
>> +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");
>>
> 

Best regards,
Clément

Powered by blists - more mailing lists