Stm32mp1 cpu opp #7219

ppaillet · 2025-01-14T16:59:03Z

The goal of this patch-set is to be able to use SCMI performance protocol to drive the CPU frequency of STM32MP1:

add a CPU OPP driver for STM32MP,
add support for SCMI Performance to SCMI-MSG,
connect SCMI performance to the new STM32MP CPU OPP driver.

etienne-lms

Hello Pascal, here is a round of comments for this series.
Could you remove the Change-Id tags from the commit messages.

core/drivers/stm32_cpu_opp.c

core/arch/arm/plat-stm32mp1/scmi_server.c

core/drivers/scmi-msg/perf_domain.c

core/arch/arm/plat-stm32mp1/scmi_server.c

core/arch/arm/plat-stm32mp1/stm32_util.h

core/include/drivers/scmi-msg.h

+ * @channel_id: SCMI channel ID
+ * @domain_id: SCMI performance domain ID
+ * @level: Target performance level
+ * @latency: Output latency value (microsecond) for the target level


etienne-lms · 2025-01-15T08:11:23Z

core/include/drivers/scmi-msg.h

+ * @channel_id: SCMI channel ID
+ * @domain_id: SCMI performance domain ID
+ * @start_index: Level index to start from.
+ * @elt: If NULL, function returns, else output level array


* @elt: Array where to store levels or NULL if querying only @nb_elts * @nb_elts: [in] @elt array size, [out] number of levels * * When @elt is NULL, @nb_elt output value gives full number of levels * remaining starting from @start_index. When @elt is not NULL, * @nb_elt output value gives the number of levels stored in @elt.

Nitpicking: can ~~return~~ remove the terminal dot at lines 453 and 455 (and at line 441).

core/drivers/scmi-msg/perf_domain.c

+int32_t __weak plat_scmi_perf_level_power_cost(unsigned int channel_id __unused,
+					       unsigned int domain_id __unused,
+					       unsigned int level __unused,
+					       unsigned int *cost __unused)


etienne-lms · 2025-01-15T08:15:21Z

core/arch/arm/plat-stm32mp1/main.c

+		return false;
+
+	if (get_part_number(&part_number)) {
+		DMSG("Cannot get part number\n");


Can drop trailing \n.
Ditto at line 742.

etienne-lms · 2025-01-28T13:06:25Z

core/drivers/stm32_cpu_opp.c

+ * @opp_count: Number of cells of @dvfs
+ * @clock: CPU clock handle
+ * @regul: CPU regulator supply handle
+ * @dvfs: Arrays of the supported CPU operating points


s/Arrays/Array/

etienne-lms

Changes in commit "plat-stm32mp1: SCMI performance domain for CPU DVFS" related to stm32_cpu_opp.c and stm32_cpu_opp.h (pm + sustained level) should be squashed in commit "drivers: add stm32 CPU DVFS driver".

etienne-lms · 2025-01-23T10:18:44Z

core/drivers/stm32_cpu_opp.c

+
+	/* nothing to do if OPP is managed by Linux and not by SCMI */
+	if (!IS_ENABLED(CFG_SCMI_MSG_PERF_DOMAIN))
+		return TEE_SUCCESS;


Prefer no call register_pm_driver_cb() if build config tells the PM sequences are not needed.

etienne-lms

Comments for commit "drivers: add stm32 CPU DVFS driver".
In the commit message: s/cpu_opp.c/stm32_cpu_opp.c/ + s/ah higher/a higher/

etienne-lms · 2025-01-28T13:02:27Z

core/drivers/stm32_cpu_opp.c

+#include <drivers/clk_dt.h>
+#include <drivers/regulator.h>
+#include <drivers/stm32_cpu_opp.h>
+#ifdef CFG_STM32MP13


could remove this #ifdef / #endif

This is for compatibility issue with MP2

etienne-lms · 2025-01-28T13:02:56Z

core/drivers/stm32_cpu_opp.c

+#include <trace.h>
+
+/*
+ * struct cpu_dvfs - CPU DVFS registered operating points


s/points/point/

etienne-lms · 2025-01-28T13:06:25Z

core/drivers/stm32_cpu_opp.c

+ * @opp_count: Number of cells of @dvfs
+ * @clock: CPU clock handle
+ * @regul: CPU regulator supply handle
+ * @dvfs: Arrays of the supported CPU operating points


s/Arrays/Array/

core/drivers/stm32_cpu_opp.c

+	TEE_Result res = TEE_ERROR_GENERIC;
+	unsigned int opp = 0;
+
+	DMSG("Set OPP to %ukHz", level);


etienne-lms · 2025-01-28T13:09:26Z

core/drivers/stm32_cpu_opp.c

+}
+
+TEE_Result stm32_cpu_opp_read_level(unsigned int *level)
+{


For consistency, here also we should check the OPPs are defined:

if (!cpu_opp.opp_count) { EMSG("No CPU OPP defined"); return TEE_ERROR_GENERIC; }

My bad: I may have been confusing. Testing whether OPP support was initialized or not, we should test a unique pattern. This function check cpu_opp.opp_count != 0 whereas stm32_cpu_opp_set_level() checks cpu_opp.dvfs != NULL. On the ohter hand, stm32_cpu_opp_get_dt_subnode() only resets cpu_opp.opp_count, not cpu_opp_dvfs upon failure. Could you make all these more consistent?

core/drivers/stm32_cpu_opp.c

+
+	return TEE_SUCCESS;
+
+err:


etienne-lms · 2025-01-28T13:16:13Z

core/drivers/stm32_cpu_opp.c

+	return TEE_SUCCESS;
+}
+
+driver_init(stm32_cpu_initcall);


Use early_init()?

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit:

"dts: stm32: describe CPU OPP for STM32MP13"
*"plat-stm32mp1: default enable CFG_STM32_CPU_OPP for STM32MP13" (s/OPP/OPPs/ in commit message)
"drivers: stm32_cpu_opp: skip OPP with unsupported voltage" (with a minor comment).

etienne-lms · 2025-01-28T13:24:07Z

core/drivers/stm32_cpu_opp.c

+ */
+static bool opp_voltage_is_supported(struct regulator *regul, uint32_t *volt_uv)
+{
+	const int target_volt_uv = *volt_uv;


I think their should be a cast here to convert the unsigned value into a signed value.

etienne-lms

Comments for commits "plat-stm32mp1: retrieve chip id from syscfg"
and "plat-stm32mp1: chip and STM32MP15 platform identification":

For consistency of these 2 commit messages, I propose to squash them into a single commit "plat-stm32mp1: chip identification and supported features".
Replace "id" with "ID" in the former commit message (header line and body).

etienne-lms · 2025-01-28T14:54:26Z

core/arch/arm/plat-stm32mp1/main.c

+
+bool stm32mp_supports_second_core(void)
+{
+	uint32_t __maybe_unused part_number = 0;


remove __maybe_unused

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
"dts: stm32: describe supported-hw on CPU OPP for STM32MP13" and
"drivers: stm32_cpu_opp: skip OPP unsupported by SOC".

etienne-lms

For commit "plat-stm32mp1: SCMI performance domain for CPU DVFS":
could you move all changes on stm32_cpu_opp.c|.h from this commit back into 1st commit "drivers: add stm32 CPU DVFS driver" where, I think, they belong.

etienne-lms · 2025-01-28T15:17:28Z

core/drivers/scmi-msg/perf_domain.c

@@ -0,0 +1,402 @@
+/* SPDX-License-Identifier: BSD-3-Clause */


Please address this comment.

drivers/cpu_opp.c implements dynamic voltage and frequency scaling for the CPU. It is used at boot time to set an higher operating point than the one used to boot. It will be used by the SCMI performance service. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

- Move PM callback from last patches - Check OPP is well initialized in several functions. - Register driver in early init. - Fix Comments. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Describe CPU operating points for STM32MP13 boards. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Enable CFG_STM32_CPU_OPP for STM32MP13 and increase CFG_STM32MP_OPP_COUNT to 3 OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Remove operating points that could not be handled by the regulator supplying the CPU. Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Cast unsigned int to int Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Chip ID is read from SYSCFG. Add the associated read function and new CHIP IDs. Use the chip id to dynamically detect the CRYPTO hardware support, the second CPU core, and CPU OPP. Signed-off-by: Lionel Debieve <lionel.debieve@st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Remove __maybe_unused Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

New platform function to get the chip identification using DBGMCU SoC register. Signed-off-by: Lionel Debieve <lionel.debieve@foss.st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Describe supported hardware for each OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Use device ID to remove not supported OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Implement some of the SCMI performance domain management messages in scmi-msg drivers to support basic DVFS scenario. Co-developed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

- Fix License issue. - Return Default latency value. - Fix indentation issue. - Improve comments Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Implement scmi-msg perf protocol platform handlers to drive CPU voltage/frequency scaling support. Co-developed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

Enable CFG_SCMI_MSG_PERF_DOMAIN for STM32MP13 that is used to provide CPU OPP to linux. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

etienne-lms · 2025-02-04T16:59:07Z

core/drivers/stm32_cpu_opp.c

+{
+	if (opp >= cpu_opp.opp_count) {
+		EMSG("Bad OPP number");
+		return TEE_ERROR_BAD_PARAMETERS;


Remove these lines (current function does not return a TEE_Result value). The assertion below is enough.

etienne-lms · 2025-02-04T17:04:30Z

core/drivers/stm32_cpu_opp.c

+	DMSG("Set OPP to %ukHz", level);
+
+	if (!cpu_opp.dvfs)
+		return TEE_ERROR_GENERIC;


Either the assertion at line 226 or this test, we don't really need both.
I think we should prefer this test since stm32_cpu_opp_set_level() is called straight from the SCMI client interface.

etienne-lms · 2025-02-04T17:08:31Z

core/drivers/stm32_cpu_opp.c

+}
+
+TEE_Result stm32_cpu_opp_read_level(unsigned int *level)
+{


My bad: I may have been confusing. Testing whether OPP support was initialized or not, we should test a unique pattern. This function check cpu_opp.opp_count != 0 whereas stm32_cpu_opp_set_level() checks cpu_opp.dvfs != NULL. On the ohter hand, stm32_cpu_opp_get_dt_subnode() only resets cpu_opp.opp_count, not cpu_opp_dvfs upon failure. Could you make all these more consistent?

etienne-lms · 2025-02-04T17:12:20Z

core/drivers/stm32_cpu_opp.c

+	int subnode = 0;
+	TEE_Result res = TEE_ERROR_GENERIC;
+
+	cpu_opp.dvfs = calloc(CFG_STM32MP_OPP_COUNT, sizeof(*cpu_opp.dvfs));


Since we always allocate CFG_STM32MP_OPP_COUNT cells, maybe worth to make the array size statically defined in the struct:

struct cpu_opp { unsigned int current_opp; unsigned int opp_count; struct clk *clock; struct regulator *regul; - struct cpu_dvfs *dvfs; + struct cpu_dvfs dvfs[CFG_STM32MP_OPP_COUNT]; unsigned int sustained_freq_khz; };

etienne-lms · 2025-02-04T17:48:03Z

core/include/drivers/scmi-msg.h

+
+/*
+ * Get power cost value related to target performance level.
+ * A default (weak) implementation output latency of 0.


s/latency/power cost/

etienne-lms · 2025-02-04T17:48:45Z

core/include/drivers/scmi-msg.h

+
+/*
+ * Get latency is microseconds for transition to target performance level.
+ * A default (weak) implementation output cost value of 0.


s/cost value of 0/latency of 1 microsecond/

etienne-lms reviewed Jan 15, 2025

View reviewed changes

ppaillet force-pushed the stm32mp1_cpu_opp branch 2 times, most recently from 904abd3 to ada6a12 Compare January 23, 2025 08:33

etienne-lms reviewed Jan 23, 2025

View reviewed changes

ppaillet force-pushed the stm32mp1_cpu_opp branch from ada6a12 to 1b6525b Compare January 27, 2025 15:58

etienne-lms reviewed Jan 28, 2025

View reviewed changes

Pascal Paillet added 15 commits January 29, 2025 11:51

fixup! drivers: add stm32 CPU DVFS driver

fe231dc

- Move PM callback from last patches - Check OPP is well initialized in several functions. - Register driver in early init. - Fix Comments. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

dts: stm32: describe CPU OPP for STM32MP13

5a99783

Describe CPU operating points for STM32MP13 boards. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

plat-stm32mp1: default enable CFG_STM32_CPU_OPP for STM32MP13

b2980b8

Enable CFG_STM32_CPU_OPP for STM32MP13 and increase CFG_STM32MP_OPP_COUNT to 3 OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

drivers: stm32_cpu_opp: skip OPP with unsupported voltage

56df978

Remove operating points that could not be handled by the regulator supplying the CPU. Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

fixup! drivers: stm32_cpu_opp: skip OPP with unsupported voltage

c2d6fd3

Cast unsigned int to int Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

fixup! plat-stm32mp1: retrieve chip id from syscfg

756e113

Remove __maybe_unused Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

plat-stm32mp1: chip and STM32MP15 platform identification

f791424

New platform function to get the chip identification using DBGMCU SoC register. Signed-off-by: Lionel Debieve <lionel.debieve@foss.st.com> Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

dts: stm32: describe supported-hw on CPU OPP for STM32MP13

e65ccdd

Describe supported hardware for each OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

drivers: stm32_cpu_opp: skip OPP unsupported by SOC

52b94f6

Use device ID to remove not supported OPP. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

fixup! drivers: scmi-msg: support performance domains for DVFS

b77e503

- Fix License issue. - Return Default latency value. - Fix indentation issue. - Improve comments Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

plat-stm32mp1: conf: enable SCMI PERF for stm32mp13

1006f41

Enable CFG_SCMI_MSG_PERF_DOMAIN for STM32MP13 that is used to provide CPU OPP to linux. Signed-off-by: Pascal Paillet <p.paillet@foss.st.com>

ppaillet force-pushed the stm32mp1_cpu_opp branch from 1b6525b to 1006f41 Compare January 30, 2025 10:52

etienne-lms reviewed Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stm32mp1 cpu opp #7219

Stm32mp1 cpu opp #7219

ppaillet commented Jan 14, 2025

etienne-lms left a comment

This comment was marked as resolved.

etienne-lms Jan 15, 2025 •

edited

Loading

This comment was marked as resolved.

etienne-lms Jan 15, 2025

This comment was marked as off-topic.

etienne-lms Jan 28, 2025

etienne-lms left a comment

etienne-lms Jan 23, 2025

etienne-lms left a comment

etienne-lms Jan 28, 2025

ppaillet Jan 29, 2025

etienne-lms Jan 28, 2025

etienne-lms Jan 28, 2025

This comment was marked as resolved.

etienne-lms Jan 28, 2025

etienne-lms Feb 4, 2025

This comment was marked as resolved.

etienne-lms Jan 28, 2025

etienne-lms left a comment

etienne-lms Jan 28, 2025

etienne-lms left a comment

etienne-lms Jan 28, 2025

etienne-lms left a comment

etienne-lms left a comment

etienne-lms Jan 28, 2025

etienne-lms Feb 4, 2025

etienne-lms Feb 4, 2025

etienne-lms Feb 4, 2025

etienne-lms Feb 4, 2025

etienne-lms Feb 4, 2025

etienne-lms Feb 4, 2025

		@@ -0,0 +1,402 @@
		/* SPDX-License-Identifier: BSD-3-Clause */

Stm32mp1 cpu opp #7219

Are you sure you want to change the base?

Stm32mp1 cpu opp #7219

Conversation

ppaillet commented Jan 14, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

etienne-lms Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Jan 15, 2025 •

edited

Loading