| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9cb1575e-ae27-4a78-adb7-8a9e7072375e@kernel.org>
Date: Mon, 23 Jun 2025 11:45:55 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Clément Le Goffic <clement.legoffic@...s.st.com>,
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>
Cc: 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 06/13] perf: stm32: introduce DDRPERFM driver
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
Powered by blists - more mailing lists