Message-ID: <20250711170415.00001901@huawei.com>
Date: Fri, 11 Jul 2025 17:04:15 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Clément Le Goffic <clement.legoffic@...s.st.com>
CC: Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>, "Rob
 Herring" <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Philipp Zabel
	<p.zabel@...gutronix.de>, Jonathan Corbet <corbet@....net>, "Gatien
 Chevallier" <gatien.chevallier@...s.st.com>, Michael Turquette
	<mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, "Gabriel
 Fernandez" <gabriel.fernandez@...s.st.com>, Krzysztof Kozlowski
	<krzk@...nel.org>, Le Goffic <legoffic.clement@...il.com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-perf-users@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-stm32@...md-mailman.stormreply.com>,
	<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-clk@...r.kernel.org>
Subject: Re: [PATCH v2 09/16] perf: stm32: introduce DDRPERFM driver

On Fri, 11 Jul 2025 16:49:01 +0200
Clément Le Goffic <clement.legoffic@...s.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@...s.st.com>

Hi Clément,

A quick drive by review as it's Friday afternoon and I was curious..

Mostly superficial stuff. I didn't look closely at the perf logic.

Jonathan

> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
> new file mode 100644
> index 000000000000..1be5bbe12978
> --- /dev/null
> +++ b/drivers/perf/stm32_ddr_pmu.c
> @@ -0,0 +1,910 @@

> +#define EVENT_NUMBER(group, index)	(((group) << 8) | (index))
> +#define GROUP_VALUE(event_number)		((event_number) >> 8)
> +#define EVENT_INDEX(event_number)	((event_number) & 0xFF)

Prefix these macro names with something driver specific.  They are
very likely to clash with something in a header in future otherwise.

> +
> +enum stm32_ddr_pmu_memory_type {
> +	STM32_DDR_PMU_LPDDR4,
> +	STM32_DDR_PMU_LPDDR3,
> +	STM32_DDR_PMU_DDR4,
> +	STM32_DDR_PMU_DDR3

This should have a trailing comma as might well be more
added in future if this IP gets used in more devices.

> +};
>


> +
> +static const struct attribute_group *stm32_ddr_pmu_attr_groups_mp2[] = {
> +	&stm32_ddr_pmu_events_attrs_group_mp2,
> +	&stm32_ddr_pmu_format_attr_group,
> +	NULL,

No comma needed on terminating entries.

> +};
> +
> +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(&pdev->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);
> +		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_prepared(pmu->dev, NULL);

Given there are quite a few uses of pmu->dev, maybe worth a local
struct device *dev = &pdev->dev; at the top and use dev to replace all these.

> +	if (IS_ERR(pmu->clk))
> +		return dev_err_probe(pmu->dev, PTR_ERR(pmu->clk), "Failed to get prepare clock\n");
> +
> +	clk_enable(pmu->clk);
> +
> +	rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

You mix and match between pdev->dev, and pmu->dev. Good to pick one or use local
variable as suggested above.

> +	if (IS_ERR(rst)) {
> +		clk_disable_unprepare(pmu->clk);
Given use of _prepared() get above. This doesn't look right - the unprepare
should be handled by devm unwinding. clk_disable() 
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rst), "Failed to get reset\n");
> +	}
> +
> +	reset_control_assert(rst);
> +	reset_control_deassert(rst);
> +
> +	pmu->poll_period = ms_to_ktime(POLL_MS);
> +	hrtimer_setup(&pmu->hrtimer, stm32_ddr_pmu_poll, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +
> +	for (int i = 0; i < MP2_CNT_NB; i++)
> +		INIT_LIST_HEAD(&pmu->counters[i]);
> +
> +	pmu->selected_set = -1;
> +
> +	pmu->pmu = (struct pmu) {
> +		.task_ctx_nr = perf_invalid_context,
> +		.start = stm32_ddr_pmu_event_start,
> +		.stop = stm32_ddr_pmu_event_stop,
> +		.add = stm32_ddr_pmu_event_add,
> +		.del = stm32_ddr_pmu_event_del,
> +		.read = stm32_ddr_pmu_event_read,
> +		.event_init = stm32_ddr_pmu_event_init,
> +		.attr_groups = pmu->cfg->attribute,
> +		.module = THIS_MODULE,
> +	};
> +
> +	ret = perf_pmu_register(&pmu->pmu, DRIVER_NAME, -1);

Calling this exposes user interfaces etc.  Does it really make sense to
do that and then write another register?  I'd normally expect this
last in probe.

> +	if (ret) {
> +		clk_disable_unprepare(pmu->clk);

As above.

> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Couldn't register DDRPERFM driver as a PMU\n");
> +	}
> +
> +	if (pmu->cfg->regs->dram_inf.reg) {
> +		ret = stm32_ddr_pmu_get_memory_type(pmu);
> +		if (ret) {
> +			perf_pmu_unregister(&pmu->pmu);
> +			clk_disable_unprepare(pmu->clk);
> +			return dev_err_probe(&pdev->dev, ret, "Failed to get memory type\n");
> +		}
> +
> +		writel_relaxed(pmu->dram_type, pmu->membase + pmu->cfg->regs->dram_inf.reg);
> +	}
> +
> +	clk_disable(pmu->clk);
> +
> +	return 0;
> +}

> +static const struct stm32_ddr_pmu_regspec stm32_ddr_pmu_regspec_mp2 = {
> +	.stop =		{ DDRPERFM_CTRL, CTRL_STOP },
> +	.start =	{ DDRPERFM_CTRL, CTRL_START },
> +	.status =	{ DDRPERFM_MP2_STATUS, MP2_STATUS_BUSY },
> +	.clear_cnt =	{ DDRPERFM_CLR, MP2_CLR_CNT},
> +	.clear_time =	{ DDRPERFM_CLR, MP2_CLR_TIME},

Spaces before } are missing
There are a few others above that I'll not mention directly.


> +	.cfg0 =		{ DDRPERFM_MP2_CFG0 },
> +	.cfg1 =		{ DDRPERFM_MP2_CFG1 },
> +	.enable =	{ DDRPERFM_MP2_CFG5 },
> +	.dram_inf =	{ DDRPERFM_MP2_DRAMINF },
> +	.counter_time =	{ DDRPERFM_MP2_TCNT },
> +	.counter_evt =	{
> +				{ DDRPERFM_MP2_EVCNT(0) },
Somewhat unusual formatting though neat I guess so fine if you
really like it!.
	.counter_evt =	{
		{ DDRPERFM_MP2_EVCNT(0) },

would be what I'd normally expect.
> +				{ DDRPERFM_MP2_EVCNT(1) },
> +				{ DDRPERFM_MP2_EVCNT(2) },
> +				{ DDRPERFM_MP2_EVCNT(3) },
> +				{ DDRPERFM_MP2_EVCNT(4) },
> +				{ DDRPERFM_MP2_EVCNT(5) },
> +				{ DDRPERFM_MP2_EVCNT(6) },
> +				{ DDRPERFM_MP2_EVCNT(7) },
> +	},
> +};
> +
> +static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp1 = {
> +	.regs = &stm32_ddr_pmu_regspec_mp1,
> +	.attribute = stm32_ddr_pmu_attr_groups_mp1,
> +	.counters_nb = MP1_CNT_NB,
> +	.evt_counters_nb = MP1_CNT_NB - 1, /* Time counter is not an event counter */
> +	.time_cnt_idx = MP1_TIME_CNT_IDX,
> +	.get_counter = stm32_ddr_pmu_get_event_counter_mp1,
> +};
> +
> +static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp2 = {
> +	.regs = &stm32_ddr_pmu_regspec_mp2,
> +	.attribute = stm32_ddr_pmu_attr_groups_mp2,
> +	.counters_nb = MP2_CNT_NB,
> +	.evt_counters_nb = MP2_CNT_NB - 1, /* Time counter is an event counter */
> +	.time_cnt_idx = MP2_TIME_CNT_IDX,
> +	.get_counter = stm32_ddr_pmu_get_event_counter_mp2,
> +};
> +
> +static const struct dev_pm_ops stm32_ddr_pmu_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, stm32_ddr_pmu_device_resume)
> +};

static DEFINE_SIMPLE_DEV_PM_OPS() looks appropriate here.


> +
> +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
> +	{
> +		.compatible = "st,stm32mp131-ddr-pmu",
> +		.data = &stm32_ddr_pmu_cfg_mp1
> +	},
> +	{
> +		.compatible = "st,stm32mp251-ddr-pmu",
> +		.data = &stm32_ddr_pmu_cfg_mp2
> +	},
> +	{ },

No comma need after terminating entry.  Nice to make it hard
to accidentally add entries after one of these!

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