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).

Could you update the description as per above suggestion or like?

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 <[email protected]> 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 <[email protected]> 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.

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.

core/drivers/stm32_cpu_opp.c

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


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

Could you squash all fixup commits?

core/drivers/scmi-msg/perf_domain.c

+	if (*domain_id >= domain_count)
+		return SCMI_INVALID_PARAMETERS;
+
+	*domain_id = confine_array_index(*domain_id, domain_count);


etienne-lms · 2025-02-11T16:34:28Z

core/drivers/scmi-msg/perf_domain.c

+	unsigned int domain_id = in_args->domain_id;
+	int32_t status = SCMI_GENERIC_ERROR;
+	struct scmi_perf_attributes_p2a return_values = {
+		.attributes = SCMI_PERF_DOMAIN_ATTRIBUTES_CAN_SET_LEVEL,


etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> for commits:
"drivers: add stm32 CPU DVFS driver",
"plat-stm32mp1: default enable CFG_STM32_CPU_OPP for STM32MP13",
"drivers: stm32_cpu_opp: skip OPP with unsupported voltage",
"plat-stm32mp1: retrieve chip id from syscfg",
"plat-stm32mp1: chip and STM32MP15 platform identification" (with a minor comment addressed or not),

Acked-by: Etienne Carriere <[email protected]> for commit:
"dts: stm32: describe supported-hw on CPU OPP for STM32MP13"

Commits "drivers: scmi-msg: support performance domains for DVFS"
and "plat-stm32mp1: SCMI performance domain for CPU DVFS" LGTM but few minor comments to address.

Maybe default value for CFG_STM32MP_OPP_LATENCY_US should be set from commit "plat-stm32mp1: conf: enable SCMI PERF for stm32mp13".

etienne-lms · 2025-02-13T12:47:24Z

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

+	uint32_t id = STM32MP1_CHIP_ID;
+
+	if (!chip_dev_id)
+		return TEE_ERROR_BAD_PARAMETERS;


minor: assert(chip_dev_id) should be enough.

etienne-lms · 2025-02-13T12:51:01Z

core/drivers/stm32_cpu_opp.c

@@ -341,6 +362,14 @@ static TEE_Result stm32_cpu_opp_get_dt_subnode(const void *fdt, int node)

 		volt_uv = fdt32_to_cpu(*cuint32);

+		/* skip OPP when the SOC does not support it */
+		if (stm32_cpu_opp_is_supported(fdt, subnode) != TEE_SUCCESS) {
+			DMSG("Skip soc OPP %"PRIu64"kHz/%"PRIu32"uV",


s/soc/SoC/ (or maybe simply "Skip OPP ...")

etienne-lms · 2025-02-13T13:05:22Z

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


Could you update the description as per above suggestion or like?

etienne-lms · 2025-02-13T13:05:30Z

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.


etienne-lms · 2025-02-13T13:06:06Z

core/include/drivers/scmi-msg.h

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


etienne-lms · 2025-02-13T13:14:41Z

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

+		.name = (_name), \
+	}
+
+struct stm32_scmi_perfd scmi_performance_domain[] = {


Needs __maybe_unused.

etienne-lms · 2025-02-13T13:20:16Z

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

+	if (!perfd)
+		return SCMI_NOT_FOUND;
+
+	/* Measured latency on STM32MP13 is around 650uS */


The config switch is set to 1000, not 650.

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 <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Describe CPU operating points for STM32MP13 boards. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

Enable CFG_STM32_CPU_OPP for STM32MP13 and increase CFG_STM32MP_OPP_COUNT to 3 OPP. Signed-off-by: Pascal Paillet <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Remove operating points that could not be handled by the regulator supplying the CPU. Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Pascal Paillet <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

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 <[email protected]> Signed-off-by: Pascal Paillet <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

New platform function to get the chip identification using DBGMCU SoC register. Signed-off-by: Lionel Debieve <[email protected]> Signed-off-by: Pascal Paillet <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Replace a test by an assert. Signed-off-by: Pascal Paillet <[email protected]>

Describe supported hardware for each OPP. Signed-off-by: Pascal Paillet <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Use device ID to remove not supported OPP. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

ppaillet · 2025-02-20T14:29:16Z

Maybe default value for CFG_STM32MP_OPP_LATENCY_US should be set from commit "plat-stm32mp1: conf: enable SCMI PERF for stm32mp13".
CFG_STM32MP_OPP_LATENCY_US is set for MP13 in this patch.
If we enable CPU OPP for MP15, we will ahve to define this value.

Improve comment. The Opp is skipped because it is not supported on this SoC. Signed-off-by: Pascal Paillet <[email protected]>

Implement some of the SCMI performance domain management messages in scmi-msg drivers to support basic DVFS scenario. Co-developed-by: Etienne Carriere <[email protected]> Signed-off-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

Update comments. Signed-off-by: Pascal Paillet <[email protected]>

Implement scmi-msg perf protocol platform handlers to drive CPU voltage/frequency scaling support. Co-developed-by: Etienne Carriere <[email protected]> Signed-off-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

Fix compilation without CFG_SCMI_MSG_PERF_DOMAIN Fix comment Signed-off-by: Pascal Paillet <[email protected]>

Enable CFG_SCMI_MSG_PERF_DOMAIN for STM32MP13 that is used to provide CPU OPP to linux. Signed-off-by: Pascal Paillet <[email protected]>

Confine domain id before returning errors. Signed-off-by: Pascal Paillet <[email protected]>

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

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

ppaillet force-pushed the stm32mp1_cpu_opp branch from 1006f41 to 655cc54 Compare February 11, 2025 10:18

etienne-lms reviewed Feb 11, 2025

View reviewed changes

ppaillet force-pushed the stm32mp1_cpu_opp branch from 655cc54 to 69ee915 Compare February 12, 2025 14:53

etienne-lms reviewed Feb 13, 2025

View reviewed changes

Pascal Paillet added 9 commits February 20, 2025 14:38

dts: stm32: describe CPU OPP for STM32MP13

367c392

Describe CPU operating points for STM32MP13 boards. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

plat-stm32mp1: default enable CFG_STM32_CPU_OPP for STM32MP13

7522c9f

Enable CFG_STM32_CPU_OPP for STM32MP13 and increase CFG_STM32MP_OPP_COUNT to 3 OPP. Signed-off-by: Pascal Paillet <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

fixup! plat-stm32mp1: chip and STM32MP15 platform identification

81a7afe

Replace a test by an assert. Signed-off-by: Pascal Paillet <[email protected]>

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

ae79173

Describe supported hardware for each OPP. Signed-off-by: Pascal Paillet <[email protected]> Acked-by: Etienne Carriere <[email protected]>

drivers: stm32_cpu_opp: skip OPP unsupported by SOC

83eb156

Use device ID to remove not supported OPP. Acked-by: Etienne Carriere <[email protected]> Signed-off-by: Pascal Paillet <[email protected]>

Pascal Paillet added 4 commits February 20, 2025 15:33

fixup! drivers: stm32_cpu_opp: skip OPP unsupported by SOC

8c0c265

Improve comment. The Opp is skipped because it is not supported on this SoC. Signed-off-by: Pascal Paillet <[email protected]>

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

fcfc0e1

Update comments. Signed-off-by: Pascal Paillet <[email protected]>

Pascal Paillet added 3 commits February 20, 2025 15:33

fixup! plat-stm32mp1: SCMI performance domain for CPU DVFS

a5f0d19

Fix compilation without CFG_SCMI_MSG_PERF_DOMAIN Fix comment Signed-off-by: Pascal Paillet <[email protected]>

plat-stm32mp1: conf: enable SCMI PERF for stm32mp13

5566f8f

Enable CFG_SCMI_MSG_PERF_DOMAIN for STM32MP13 that is used to provide CPU OPP to linux. Signed-off-by: Pascal Paillet <[email protected]>

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

471eb1f

Confine domain id before returning errors. Signed-off-by: Pascal Paillet <[email protected]>

ppaillet force-pushed the stm32mp1_cpu_opp branch from 69ee915 to 471eb1f Compare February 20, 2025 14:48

		@@ -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

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

This comment was marked as resolved.

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 left a 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

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

ppaillet commented Feb 20, 2025

etienne-lms Jan 15, 2025 •

edited

Loading