| 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: <1ff31bd6-08e0-46da-a1b0-c60dbb1e3cb8@foss.st.com>
Date: Mon, 23 Jun 2025 17:02:22 +0200
From: Clement LE GOFFIC <clement.legoffic@...s.st.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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 6/23/25 11:45, Krzysztof Kozlowski wrote:
> 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.
Ok
>
>> + 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.
Ok will disable_unprepare
>> +
>> + 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