Message-ID: <20250910102627.00007a40@huawei.com>
Date: Wed, 10 Sep 2025 10:26:27 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Clément Le Goffic <legoffic.clement@...il.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 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,

J

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


> +/* 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.

> +};



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

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

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


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

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

Powered by blists - more mailing lists