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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/06/2025 11:27, Clément Le Goffic wrote:
> +	if (of_property_present(pmu->dev->of_node, "access-controllers")) {
> +		ret = stm32_firewall_get_firewall(pmu->dev->of_node, &firewall, 1);
> +		if (ret) {
> +			dev_err(pmu->dev, "Failed to get firewall\n");
> +			return ret;
> +		}
> +		ret = stm32_firewall_grant_access_by_id(&firewall, firewall.firewall_id);
> +		if (ret) {
> +			dev_err(pmu->dev, "Failed to grant access\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (of_property_present(pmu->dev->of_node, "clocks")) {

No, don't open-code get clk optional.

> +		pmu->clk = devm_clk_get_prepared(pmu->dev, NULL);
> +		if (IS_ERR(pmu->clk)) {
> +			dev_err(pmu->dev, "Failed to get clock\n");
> +			return PTR_ERR(pmu->clk);
> +		}
> +	}
> +
> +	clk_enable(pmu->clk);
> +
> +	if (of_property_present(pdev->dev.of_node, "resets")) {
> +		rst = devm_reset_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(rst)) {
> +			dev_err(&pdev->dev, "Failed to get reset\n");
> +			ret = PTR_ERR(rst);
> +			goto err_clk;
> +		}
> +		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);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't register DDRPERFM driver as a PMU\n");
> +		goto err_clk;
> +	}
> +
> +	if (pmu->cfg->regs->dram_inf.reg) {
> +		ret = of_property_read_u32(pdev->dev.of_node, "st,dram-type", &pmu->dram_type);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Missing device-tree property 'st,dram-type'\n");
> +			perf_pmu_unregister(&pmu->pmu);
> +
> +			return ret;
> +		}
> +
> +		writel_relaxed(pmu->dram_type, pmu->membase + pmu->cfg->regs->dram_inf.reg);
> +	}
> +
> +	clk_disable(pmu->clk);

Why do you keep clock prepared? This device does not know what sort of
clock it gets, so you end up with clock always active for example and
this being no-op.

> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(pmu->clk);
> +
> +	return ret;
> +}
> +
> +static void stm32_ddr_pmu_device_remove(struct platform_device *pdev)
> +{
> +	struct stm32_ddr_pmu *stm32_ddr_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&stm32_ddr_pmu->pmu);
> +}
> +
> +static int __maybe_unused stm32_ddr_pmu_device_resume(struct device *dev)
> +{
> +	struct stm32_ddr_pmu *pmu = dev_get_drvdata(dev);
> +
> +	clk_enable(pmu->clk);
> +	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_mp1 = {
> +	.stop =		{ DDRPERFM_CTRL, CTRL_STOP },
> +	.start =	{ DDRPERFM_CTRL, CTRL_START },
> +	.enable =	{ DDRPERFM_CFG },
> +	.cfg =		{ DDRPERFM_CFG },
> +	.status =	{ DDRPERFM_STATUS, MP1_STATUS_BUSY },
> +	.clear_cnt =	{ DDRPERFM_CLR, MP1_CLR_CNT},
> +	.clear_time =	{ DDRPERFM_CLR, MP1_CLR_TIME},
> +	.counter_time =	{ DDRPERFM_TCNT },
> +	.counter_evt =	{
> +				{ DDRPERFM_EVCNT(0) },
> +				{ DDRPERFM_EVCNT(1) },
> +				{ DDRPERFM_EVCNT(2) },
> +				{ DDRPERFM_EVCNT(3) },
> +	},
> +};
> +
> +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},
> +	.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) },
> +				{ 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 const struct of_device_id stm32_ddr_pmu_of_match[] = {
> +	{
> +		.compatible = "st,stm32mp131-ddr-pmu",
> +		.data = &stm32_ddr_pmu_cfg_mp1
> +	},
> +	{
> +		.compatible = "st,stm32mp151-ddr-pmu",
> +		.data = &stm32_ddr_pmu_cfg_mp1

So devices are compatible, thus express it correctly and drop this.

> +	},
> +	{
> +		.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 = &stm32_ddr_pmu_pm_ops,
> +		.of_match_table = of_match_ptr(stm32_ddr_pmu_of_match),

Drop of_match_ptr, you have here warnings.



Best regards,
Krzysztof