Re: [PATCH RFC v7 7/9] PM / devfreq: Introduce the QCOM SCMI Memlat devfreq driver

On 02-Jul-26 10:51 PM, Bjorn Andersson wrote:
> On Wed, Jun 10, 2026 at 02:21:34PM +0530, Pragnesh Papaniya wrote:
>> From: Sibi Sankar <[email protected]>
>>
>> On Qualcomm Glymur, Mahua and X1E/X1P (Hamoa) SoCs, the memlat governor and
>> the mechanism to control the various caches and RAM is hosted on the CPU
>> Control Processor (CPUCP), and configuration and control of this governor
>> is exposed through the QCOM SCMI Generic Extension Protocol, addressed via
>> the "MEMLAT" algorithm string.
>>
> 
> This explains that there's a bunch of functionality running on CPUCP and
> there's a "MEMLAT" string.
> 

CPUCP does all the real work: it samples CPU perf counters, computes IPM/stall,
and votes the DDR/LLCC/DDR_QOS buses on its own timer. The Linux driver only
pushes static configuration (freq maps, ceilings) once at probe and
starts/stops the CPUCP timer. I'll rewrite the message to say this plainly.

>> Introduce a devfreq SCMI client driver that uses the MEMLAT algorithm
>> string to detect memory-latency-bound workloads and control the
>> frequency/level of the memory buses (DDR, LLCC and DDR_QOS).
> 
> You established that there's stuff running in the firmware, now we're
> introducing a client driver to control memory buses.
> 
> But where did you explain how these two "facts" are related? Why is
> there a client driver, what is the actual distribution of roles in this
> dance?
> 

At runtime the driver is not in the control loop, CPUCP is. devfreq is used so
each bus shows up as a real device with trans_stat and the remote governor's
parameters like sample_ms and ipm_ceil are user-configurable. I'll make that
reasoning explicit in the commit text.

>> Model each bus
>> as a devfreq device using the remote devfreq governor. This provides basic
>> insight into device operation via trans_stat and allows further tuning of
>> the remote governor's parameters from userspace.
>>
> 
> Does this mean that the driver is "optional", and only exists to give
> insight and a way to tune the firmware operation?
> 
> If that's the case, why is it a devfreq driver?
> 
> Please write your commit messages (and patches) such that people outside
> your immediate team can understand why the patches are needed and what
> the code is supposed to do.
> 

Ack

>> Co-developed-by: Amir Vajid <[email protected]>
>> Signed-off-by: Amir Vajid <[email protected]>
>> Co-developed-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Ramakrishna Gottimukkula <[email protected]>
>> Signed-off-by: Sibi Sankar <[email protected]>
>> Co-developed-by: Pragnesh Papaniya <[email protected]>
>> Signed-off-by: Pragnesh Papaniya <[email protected]>
>> ---
>>  drivers/devfreq/Kconfig                    |  13 +
>>  drivers/devfreq/Makefile                   |   1 +
>>  drivers/devfreq/scmi-qcom-memlat-cfg.h     | 573 +++++++++++++++++++++++++++
>>  drivers/devfreq/scmi-qcom-memlat-devfreq.c | 616 +++++++++++++++++++++++++++++
>>  4 files changed, 1203 insertions(+)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 2caa87554914..98b5a50d3189 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -169,6 +169,19 @@ config ARM_SUN8I_A33_MBUS_DEVFREQ
>>  	  This adds the DEVFREQ driver for the MBUS controller in some
>>  	  Allwinner sun8i (A33 through H3) and sun50i (A64 and H5) SoCs.
>>  
>> +config SCMI_QCOM_MEMLAT_DEVFREQ
>> +	tristate "Qualcomm Technologies Inc. SCMI client driver"
>> +	depends on QCOM_SCMI_GENERIC_EXT || COMPILE_TEST
>> +	select DEVFREQ_GOV_REMOTE
>> +	help
>> +	  This driver uses the MEMLAT (memory latency) algorithm string
> 
> Is "driver uses X algorithm string" idiomatic SCMI terms?
> 

No, "algorithm string" is an internal term. I'll drop the jargon and describe
it in plain SCMI vendor-protocol terms.

>> +	  hosted on QCOM SCMI Vendor Protocol to detect memory latency
>> +	  workloads and control frequency/level of the various memory
>> +	  buses (DDR/LLCC/DDR_QOS).
>> +
>> +	  This driver defines/documents the parameter IDs used while configuring
>> +	  the memory buses.
> 
> Imagine an person outside your team, sitting there in menuconfig
> wondering if they should enable this driver or not.
> 
> There's a sentence in the middle ("control frequency/level of various
> memory buses" - that sounds like something I want. But "detect memory
> latency", is it just monitoring or does that part relate to the
> controlling part? "This driver defines" so what are those parameters
> used for, do I need some other driver for the control part? Is this last
> paragraph adding value to my understanding for that
> CONFIG_SCMI_QCOM_MEMLAT_DEVFREQ does?
> 

I'll rewrite it to say what you get (memory-bus scaling on these Qualcomm
SoCs), that CPUCP does the actual scaling, and that nothing else is required
to enable it. The parameter-ID paragraph will go.

>> +
>>  source "drivers/devfreq/event/Kconfig"
>>  
>>  endif # PM_DEVFREQ
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index cde57c8cda76..b11f94e2f485 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ)	+= mtk-cci-devfreq.o
>>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>>  obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ)	+= sun8i-a33-mbus.o
>>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
>> +obj-$(CONFIG_SCMI_QCOM_MEMLAT_DEVFREQ)	+= scmi-qcom-memlat-devfreq.o
>>  
>>  # DEVFREQ Event Drivers
>>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
>> diff --git a/drivers/devfreq/scmi-qcom-memlat-cfg.h b/drivers/devfreq/scmi-qcom-memlat-cfg.h
>> new file mode 100644
>> index 000000000000..1ab8b61ea271
>> --- /dev/null
>> +++ b/drivers/devfreq/scmi-qcom-memlat-cfg.h
> 
> Are the entities declared in this header file used by anything other
> than scmi-qcom-memlat-devfreq.c? If not why is it a separate header file?
> 

No, only scmi-qcom-memlat-devfreq.c uses it. I split it out just to keep the
large config tables out of the driver logic. Happy either way: do you prefer
I fold it back into the .c, or keep it as a header?

>> @@ -0,0 +1,573 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#ifndef __DRIVERS_DEVFREQ_SCMI_QCOM_MEMLAT_CONFIG_H__
>> +#define __DRIVERS_DEVFREQ_SCMI_QCOM_MEMLAT_CONFIG_H__
>> +
>> +/*
>> + * Memlat Effective Frequency Calculation Method
>> + * CPUCP_EFFECTIVE_FREQ_METHOD_0 - Uses CPU Cycles and CONST Cycles to calculate
>> + * CPUCP_EFFECTIVE_FREQ_METHOD_1 - Uses CPU Cycles and time period
>> + */
>> +#define CPUCP_EFFECTIVE_FREQ_CALC_METHOD_0	0
>> +#define CPUCP_EFFECTIVE_FREQ_CALC_METHOD_1	1
> 
> #define ZERO 0
> #define ONE  1
> 
> Can these be given real names, or could the struct member even be a
> boolean?
> 
> That said, "ZERO" isn't used in your patch...
> 

I'll make cpucp_freq_method a bool. Method 0 is a valid firmware method and
will be used too, so I'll keep both and select via the bool.

>> +
>> +#define EV_CPU_CYCLES		0
>> +#define EV_CNT_CYCLES		1
>> +#define EV_INST_RETIRED		2
>> +#define EV_STALL_BACKEND_MEM	3
>> +#define EV_L2_D_RFILL		5
>> +#define INVALID_IDX		0xff
> 
> The names of these defines aren't awesome names to put in include
> files...
> 

Ack, I'll namespace them (MEMLAT_EV_*) and align INVALID_IDX with the
firmware's 0xFF.

>> +
>> +#define MEMLAT_ALGO_STR		0x4D454D4C4154ULL /* MEMLAT */
> 
> As you've seen a thousand times on LKML already, we want lower-case hex
> digits...
> 

Firmware expects upper-case string only.

>> +
>> +struct scmi_qcom_map_table {
>> +	unsigned int cpu_freq;
>> +	unsigned int mem_freq;
> 
> Unit soup
> 

I'll suffix them (cpu_freq_mhz / mem_freq_mhz).

>> +};
>> +
>> +struct scmi_qcom_opp_data {
>> +	unsigned long freq;
>> +	unsigned int level;
> 
> The arrays of opp_data is all static const, and you seem to only define
> "level" for ddr_qos, which if I read the code correctly you later
> treat in some special way anyways.
> 

Yes, level is only set for DDR_QOS and unused elsewhere. I'll drop the level
field entirely and document that the DDR_QOS OPP frequencies (1 and 100) are
not real frequencies; they stand in for the 0/1 level.

>> +};
>> +
>> +struct scmi_qcom_memory_range {
>> +	unsigned int min_freq;
>> +	unsigned int max_freq;
>> +};
>> +
>> +enum common_ev_idx {
>> +	INST_IDX,
>> +	CYC_IDX,
>> +	CONST_CYC_IDX,
>> +	FE_STALL_IDX,
>> +	BE_STALL_IDX,
>> +	NUM_COMMON_EVS
>> +};
>> +
>> +enum grp_ev_idx {
>> +	MISS_IDX,
>> +	WB_IDX,
>> +	ACC_IDX,
>> +	NUM_GRP_EVS
>> +};
>> +
>> +/*
>> + * CPUCP firmware identifies memory groups by a small integer (the hw_type
>> + * carried in node_msg / scalar_param_msg / map_param_msg / ev_map_msg).
> 
> Ok, sounds reasonable.
> 
>> The
>> + * encoding is shared between the cfg tables below and scmi_qcom_devfreq_get_cur_freq()
>> + * which special-cases DDR_QOS as a level-based bus rather than a frequency-scaled one.
> 
> The constants are shared between A and B, both being described as
> in-driver users. So is this constants shared between the first (as you
> said above) or an enumeration within the driver?
> 

It's a protocol ABI, shared with the first (firmware). I'll reword the comment
to say it's a firmware-defined encoding, not a driver-internal enum.

>> + */
>> +enum scmi_qcom_memlat_hw_type {
>> +	MEMLAT_HW_DDR			= 0,
>> +	MEMLAT_HW_LLCC			= 1,
>> +	MEMLAT_HW_DDR_QOS_COMPUTE	= 2,
> 
> Please use "enum" for enumerations and #define for constants. (These
> aren't enumerations, as they are defined constants)
> 

Ack, will keep it as enum (as you suggested below).

>> +};
>> +
>> +struct scmi_qcom_monitor_cfg {
>> +	const struct scmi_qcom_map_table *table;
>> +	const char *name;
>> +	u32 be_stall_floor;
> 
> What is a "be stall floor"? Also, it seems to be 1 in all your cases. Is
> it boolean? Is it constant?
> 

It's a back-end-stall percentage threshold. It happens to be 1 in all current
configs (meaning almost any stall qualifies). I'll document it as a percent.

>> +	u32 cpu_mask;
>> +	u32 ipm_ceil;
>> +	int table_len;
> 
> Why is this signed?
> 

Ack

>> +};
>> +
>> +struct scmi_qcom_memory_cfg {
>> +	const struct scmi_qcom_monitor_cfg *monitor_cfg;
>> +	const struct scmi_qcom_opp_data *mem_table;
>> +	struct scmi_qcom_memory_range memory_range;
>> +	const u32 *grp_ev;
>> +	const char *name;
>> +	u32 memory_type;
> 
> As the memory type isn't an enumeration, I asked you above to make it
> constants. But in the form you wrote it, there's no reason for this not
> to have its enum type.
> 

Ack, will keep it as enum.

>> +	int monitor_cnt;
>> +	int num_opps;
> 
> Aren't these unsigned?
> 

Ack

>> +};
>> +
>> +struct scmi_qcom_memlat_cfg_data {
>> +	const struct scmi_qcom_memory_cfg *memory_cfg;
>> +	const u32 *common_ev;
>> +	u32 cpucp_freq_method;
>> +	u32 cpucp_sample_ms;
>> +	int memory_cnt;
> 
> Unsigned...
> 

Ack

>> +};
>> +
>> +static const u32 glymur_common_ev[NUM_COMMON_EVS] = {
>> +	[INST_IDX]      = EV_INST_RETIRED,
>> +	[CYC_IDX]       = EV_CPU_CYCLES,
>> +	[CONST_CYC_IDX] = EV_CNT_CYCLES,
>> +	[FE_STALL_IDX]  = INVALID_IDX,
>> +	[BE_STALL_IDX]  = EV_STALL_BACKEND_MEM,
>> +};
>> +
>> +static const u32 glymur_ddr_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const u32 glymur_llcc_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const u32 glymur_ddr_qos_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const u32 hamoa_common_ev[NUM_COMMON_EVS] = {
>> +	[INST_IDX]      = EV_INST_RETIRED,
>> +	[CYC_IDX]       = EV_CPU_CYCLES,
>> +	[CONST_CYC_IDX] = EV_CNT_CYCLES,
>> +	[FE_STALL_IDX]  = INVALID_IDX,
>> +	[BE_STALL_IDX]  = EV_STALL_BACKEND_MEM,
>> +};
>> +
>> +static const u32 hamoa_ddr_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const u32 hamoa_llcc_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const u32 hamoa_ddr_qos_grp_ev[NUM_GRP_EVS] = {
>> +	[MISS_IDX] = EV_L2_D_RFILL,
>> +	[WB_IDX]   = INVALID_IDX,
>> +	[ACC_IDX]  = INVALID_IDX,
>> +};
>> +
>> +static const struct scmi_qcom_opp_data glymur_llcc_table[] = {
>> +	{ .freq = 315000000 },
>> +	{ .freq = 479000000 },
>> +	{ .freq = 545000000 },
>> +	{ .freq = 725000000 },
>> +	{ .freq = 840000000 },
>> +	{ .freq = 959000000 },
>> +	{ .freq = 1090000000 },
>> +	{ .freq = 1211000000 },
>> +};
>> +
>> +static const struct scmi_qcom_opp_data hamoa_llcc_table[] = {
>> +	{ .freq = 300000000 },
>> +	{ .freq = 466000000 },
>> +	{ .freq = 600000000 },
>> +	{ .freq = 806000000 },
>> +	{ .freq = 933000000 },
>> +	{ .freq = 1066000000 },
>> +};
>> +
>> +static const struct scmi_qcom_opp_data glymur_ddr_table[] = {
>> +	{ .freq = 200000000 },
>> +	{ .freq = 547000000 },
>> +	{ .freq = 1353000000 },
>> +	{ .freq = 1555000000 },
>> +	{ .freq = 1708000000 },
>> +	{ .freq = 2092000000 },
>> +	{ .freq = 2736000000 },
>> +	{ .freq = 3187000000 },
>> +	{ .freq = 3686000000 },
>> +	{ .freq = 4224000000 },
>> +	{ .freq = 4761000000 },
>> +};
>> +
>> +static const struct scmi_qcom_opp_data hamoa_ddr_table[] = {
>> +	{ .freq = 200000000 },
>> +	{ .freq = 547000000 },
>> +	{ .freq = 768000000 },
>> +	{ .freq = 1555000000 },
>> +	{ .freq = 1708000000 },
>> +	{ .freq = 2092000000 },
>> +	{ .freq = 2736000000 },
>> +	{ .freq = 3187000000 },
>> +	{ .freq = 3686000000 },
>> +	{ .freq = 4224000000 },
>> +};
>> +
>> +/*
>> + * DDR_QOS is a level-based bus (0 = nominal, 1 = boost), not a
>> + * frequency-scaled one.
> 
> "level-based"? Looks more like "boolean"?
> 
> So I presume "not a frequency-scaled one" should be interpreted as the
> .freq is bogus?
> 

Yes, the .freq values there are not real frequencies. Removing the
level field and rewording the comment will make that clear.

>> The OPP entries below use synthetic frequencies
>> + * (1 / 100) purely as distinct devfreq keys so trans_stat can show
>> + * level transitions.
> 
> 1/100? 0.01?
> 

Its not division, its an 'or'. Will drop '/'.

> Why are the frequencies 1 and 100, why not 0 and 1, or 0 and 100?
> 

0 can't be used, dev_pm_opp_add() rejects a 0 frequency. The two keys just
need to be non-zero and distinct, hence 1 and 100. I'll spell this out in the
comment.

>> scmi_qcom_devfreq_get_cur_freq() maps the firmware
>> + * level back to the matching key.
> 
> What is the "key" here?
> 

"key" means the OPP/devfreq frequency used for lookup. I'll say "OPP
frequency" instead of "key".

>> + */
>> +static const struct scmi_qcom_opp_data glymur_ddr_qos_table[] = {
>> +	{ .freq = 1, .level = 0 },
>> +	{ .freq = 100, .level = 1 },
>> +};
>> +
>> +static const struct scmi_qcom_memory_cfg glymur_memory_cfg[] = {
>> +	{
>> +		.memory_type = MEMLAT_HW_DDR,
>> +		.name = "ddr",
>> +		.mem_table = glymur_ddr_table,
>> +		.num_opps = ARRAY_SIZE(glymur_ddr_table),
>> +		.grp_ev = glymur_ddr_grp_ev,
>> +		.monitor_cnt = 4,
>> +		.memory_range = { .min_freq = 547000, .max_freq = 4761000},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0x3f,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 8,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 960, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1133, .mem_freq = 1353000 },
>> +					{ .cpu_freq = 1594, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 1920, .mem_freq = 1708000 },
>> +					{ .cpu_freq = 2228, .mem_freq = 2736000 },
>> +					{ .cpu_freq = 2362, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 2650, .mem_freq = 3686000 },
>> +					{ .cpu_freq = 2938, .mem_freq = 4761000 },
> 
> Why are these tables hard coded in the driver? Are they constant?
> 

These tables can be either in DT (like in earlier re-spins of the series) or in
the driver. For the former to work well with the existing OPP framework, we
would need a clock provider created for DDR/LLCC/DDR-QOS just to derive the
cpufreq to memfreq map tables. Having it in the driver simplifies the overall
implementation. 

>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xfc0,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 8,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 356, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1018, .mem_freq = 1353000 },
>> +					{ .cpu_freq = 1536, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 1748, .mem_freq = 1708000 },
>> +					{ .cpu_freq = 2324, .mem_freq = 2736000 },
>> +					{ .cpu_freq = 2496, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 2900, .mem_freq = 3686000 },
>> +					{ .cpu_freq = 3514, .mem_freq = 4761000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0x3f000,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 8,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 356, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1018, .mem_freq = 1353000 },
>> +					{ .cpu_freq = 1536, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 1748, .mem_freq = 1708000 },
>> +					{ .cpu_freq = 2324, .mem_freq = 2736000 },
>> +					{ .cpu_freq = 2496, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 2900, .mem_freq = 3686000 },
>> +					{ .cpu_freq = 3514, .mem_freq = 4761000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_3",
>> +				.cpu_mask = 0x3ffff,
>> +				.table_len = 4,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2823, .mem_freq = 547000 },
>> +					{ .cpu_freq = 3034, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 3226, .mem_freq = 1708000 },
>> +					{ .cpu_freq = 5012, .mem_freq = 2092000 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +	{
>> +		.memory_type = MEMLAT_HW_LLCC,
>> +		.name = "llcc",
>> +		.mem_table = glymur_llcc_table,
>> +		.num_opps = ARRAY_SIZE(glymur_llcc_table),
>> +		.grp_ev = glymur_llcc_grp_ev,
>> +		.monitor_cnt = 3,
>> +		.memory_range = { .min_freq = 315000, .max_freq = 1211000},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0x3f,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 7,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 960, .mem_freq = 315000 },
>> +					{ .cpu_freq = 1113, .mem_freq = 479000 },
>> +					{ .cpu_freq = 1594, .mem_freq = 545000 },
>> +					{ .cpu_freq = 1920, .mem_freq = 725000 },
>> +					{ .cpu_freq = 2362, .mem_freq = 840000 },
>> +					{ .cpu_freq = 2650, .mem_freq = 959000 },
>> +					{ .cpu_freq = 2938, .mem_freq = 1211000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xfc0,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 7,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 356, .mem_freq = 315000 },
>> +					{ .cpu_freq = 1018, .mem_freq = 479000 },
>> +					{ .cpu_freq = 1536, .mem_freq = 545000 },
>> +					{ .cpu_freq = 1748, .mem_freq = 725000 },
>> +					{ .cpu_freq = 2496, .mem_freq = 840000 },
>> +					{ .cpu_freq = 2900, .mem_freq = 959000 },
>> +					{ .cpu_freq = 3514, .mem_freq = 1211000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0x3f000,
>> +				.ipm_ceil = 60000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 7,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 356, .mem_freq = 315000 },
>> +					{ .cpu_freq = 1018, .mem_freq = 479000 },
>> +					{ .cpu_freq = 1536, .mem_freq = 545000 },
>> +					{ .cpu_freq = 1748, .mem_freq = 725000 },
>> +					{ .cpu_freq = 2496, .mem_freq = 840000 },
>> +					{ .cpu_freq = 2900, .mem_freq = 959000 },
>> +					{ .cpu_freq = 3514, .mem_freq = 1211000 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +	{
>> +		.memory_type = MEMLAT_HW_DDR_QOS_COMPUTE,
>> +		.name = "ddr-qos",
>> +		.monitor_cnt = 3,
>> +		.mem_table = glymur_ddr_qos_table,
>> +		.num_opps = ARRAY_SIZE(glymur_ddr_qos_table),
>> +		.grp_ev = glymur_ddr_qos_grp_ev,
>> +		.memory_range = { .min_freq = 0, .max_freq = 1},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0x3f,
>> +				.ipm_ceil = 80000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2362, .mem_freq = 0 },
>> +					{ .cpu_freq = 2938, .mem_freq = 1 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xfc0,
>> +				.ipm_ceil = 80000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2496, .mem_freq = 0 },
>> +					{ .cpu_freq = 3514, .mem_freq = 1 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0x3f000,
>> +				.ipm_ceil = 80000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2496, .mem_freq = 0 },
>> +					{ .cpu_freq = 3514, .mem_freq = 1 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +};
>> +
>> +static const struct scmi_qcom_memory_cfg hamoa_memory_cfg[] = {
>> +	{
>> +		.memory_type = MEMLAT_HW_DDR,
>> +		.name = "ddr",
>> +		.mem_table = hamoa_ddr_table,
>> +		.num_opps = ARRAY_SIZE(hamoa_ddr_table),
>> +		.grp_ev = hamoa_ddr_grp_ev,
>> +		.monitor_cnt = 4,
>> +		.memory_range = { .min_freq = 200000, .max_freq = 4224000},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0xf,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 768000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 2092000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 4224000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xf0,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 768000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 2092000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 4224000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0xf00,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 547000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 768000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 2092000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 3187000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 4224000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_3",
>> +				.cpu_mask = 0xfff,
>> +				.table_len = 4,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 1440, .mem_freq = 547000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 768000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 1555000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 2092000 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +	{
>> +		.memory_type = MEMLAT_HW_LLCC,
>> +		.name = "llcc",
>> +		.mem_table = hamoa_llcc_table,
>> +		.num_opps = ARRAY_SIZE(hamoa_llcc_table),
>> +		.grp_ev = hamoa_llcc_grp_ev,
>> +		.monitor_cnt = 3,
>> +		.memory_range = { .min_freq = 300000, .max_freq = 1066000},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0xf,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 300000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 466000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 600000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 806000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 933000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1066000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xf0,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 300000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 466000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 600000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 806000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 933000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1066000 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0xf00,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 6,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 999, .mem_freq = 300000 },
>> +					{ .cpu_freq = 1440, .mem_freq = 466000 },
>> +					{ .cpu_freq = 1671, .mem_freq = 600000 },
>> +					{ .cpu_freq = 2189, .mem_freq = 806000 },
>> +					{ .cpu_freq = 2516, .mem_freq = 933000 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1066000 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +	{
>> +		.memory_type = MEMLAT_HW_DDR_QOS_COMPUTE,
>> +		.name = "ddr-qos",
>> +		.monitor_cnt = 3,
>> +		.mem_table = glymur_ddr_qos_table,
>> +		.num_opps = ARRAY_SIZE(glymur_ddr_qos_table),
>> +		.grp_ev = hamoa_ddr_qos_grp_ev,
>> +		.memory_range = { .min_freq = 0, .max_freq = 1},
>> +		.monitor_cfg = (const struct scmi_qcom_monitor_cfg[]) {
>> +			{
>> +				.name = "mon_0",
>> +				.cpu_mask = 0xf,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2189, .mem_freq = 0 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_1",
>> +				.cpu_mask = 0xf0,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2189, .mem_freq = 0 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1 },
>> +				}
>> +			},
>> +			{
>> +				.name = "mon_2",
>> +				.cpu_mask = 0xf00,
>> +				.ipm_ceil = 20000000,
>> +				.be_stall_floor = 1,
>> +				.table_len = 2,
>> +				.table = (const struct scmi_qcom_map_table[]) {
>> +					{ .cpu_freq = 2189, .mem_freq = 0 },
>> +					{ .cpu_freq = 3860, .mem_freq = 1 },
>> +				}
>> +			},
>> +		},
>> +	},
>> +};
>> +
>> +static const struct scmi_qcom_memlat_cfg_data glymur_memlat_data = {
>> +	.memory_cfg = glymur_memory_cfg,
>> +	.common_ev = glymur_common_ev,
>> +	.cpucp_freq_method = CPUCP_EFFECTIVE_FREQ_CALC_METHOD_1,
>> +	.cpucp_sample_ms = 4,
>> +	.memory_cnt = ARRAY_SIZE(glymur_memory_cfg),
>> +};
>> +
>> +static const struct scmi_qcom_memlat_cfg_data hamoa_memlat_data = {
>> +	.memory_cfg = hamoa_memory_cfg,
>> +	.common_ev = hamoa_common_ev,
>> +	.cpucp_freq_method = CPUCP_EFFECTIVE_FREQ_CALC_METHOD_1,
>> +	.cpucp_sample_ms = 4,
>> +	.memory_cnt = ARRAY_SIZE(hamoa_memory_cfg),
>> +};
> 
> It would be wonderful to declare scmi_qcom_memlat_configs[] in the
> vicinity of these two definitions.
> 

Ack

>> +
>> +#endif
>> diff --git a/drivers/devfreq/scmi-qcom-memlat-devfreq.c b/drivers/devfreq/scmi-qcom-memlat-devfreq.c
>> new file mode 100644
>> index 000000000000..99ae229acfdc
>> --- /dev/null
>> +++ b/drivers/devfreq/scmi-qcom-memlat-devfreq.c
>> @@ -0,0 +1,616 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/devfreq.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/scmi_protocol.h>
>> +#include <linux/scmi_qcom_protocol.h>
>> +#include <linux/units.h>
>> +
>> +#define MAX_NAME_LEN				20
>> +#define MAX_MAP_ENTRIES				10
> 
> The names used for these defines are a bit generic...
> 

I'll prefix them (MEMLAT_MAX_*).

>> +
>> +#include "scmi-qcom-memlat-cfg.h"
>> +
>> +/**
>> + * enum scmi_memlat_protocol_cmd - parameter_ids supported by the "MEMLAT" algo_str hosted
>> + *                                 by the Qualcomm Generic Vendor Protocol on the SCMI controller.
> 
> This is supposed to be the short description, yet it's the same length
> as the long description on the very next line.
> 

Ack, will make it single line.

>> + *
>> + * MEMLAT (Memory Latency) monitors the counters to detect memory latency bound workloads
>> + * and scales the frequency/levels of the memory buses accordingly.
>> + *
>> + * @MEMLAT_SET_MEM_GROUP: initializes the frequency/level scaling functions for the memory bus.
>> + * @MEMLAT_SET_MONITOR: configures the monitor to work on a specific memory bus.
>> + * @MEMLAT_SET_COMMON_EV_MAP: set up common counters used to monitor the cpu frequency.
>> + * @MEMLAT_SET_GRP_EV_MAP: set up any specific counters used to monitor the memory bus.
>> + * @MEMLAT_IPM_CEIL: set the IPM (Instruction Per Misses) ceiling per monitor.
>> + * @MEMLAT_BE_STALL_FLOOR: set the back-end stall floor per monitor.
>> + * @MEMLAT_SAMPLE_MS: set the sampling period for all the monitors.
>> + * @MEMLAT_MON_FREQ_MAP: setup the cpufreq to memfreq map.
>> + * @MEMLAT_SET_MIN_FREQ: set the min frequency of the memory bus.
>> + * @MEMLAT_SET_MAX_FREQ: set the max frequency of the memory bus.
>> + * @MEMLAT_GET_CUR_FREQ: query the current frequency/level of the memory bus.
>> + * @MEMLAT_START_TIMER: start all the monitors with the requested sampling period.
>> + * @MEMLAT_STOP_TIMER: stop all the running monitors.
>> + * @MEMLAT_SET_EFFECTIVE_FREQ_METHOD: set the method used to determine cpu frequency.
>> + */
>> +enum scmi_memlat_protocol_cmd {
> 
> Don't use "enum" when you don't mean "enumeration".
> 

Ack, these are fixed SCMI parameter IDs with explicit values and gaps that must
match the firmware, so I'll switch them to #define.

>> +	MEMLAT_SET_MEM_GROUP = 16,
>> +	MEMLAT_SET_MONITOR,
>> +	MEMLAT_SET_COMMON_EV_MAP,
>> +	MEMLAT_SET_GRP_EV_MAP,
>> +	MEMLAT_IPM_CEIL = 23,
>> +	MEMLAT_BE_STALL_FLOOR = 25,
>> +	MEMLAT_SAMPLE_MS = 31,
>> +	MEMLAT_MON_FREQ_MAP,
>> +	MEMLAT_SET_MIN_FREQ,
>> +	MEMLAT_SET_MAX_FREQ,
>> +	MEMLAT_GET_CUR_FREQ,
>> +	MEMLAT_START_TIMER = 36,
>> +	MEMLAT_STOP_TIMER,
>> +	MEMLAT_SET_EFFECTIVE_FREQ_METHOD = 39,
>> +};
>> +
>> +struct cpucp_map_table {
>> +	__le16 v1;
>> +	__le16 v2;
> 
> v1 v2?
> 
> As far as I can tell these two numbers are cpu and memory frequency.
> 

v1 is CPU freq and v2 is memory freq (both in MHz). I'll rename them
accordingly.

>> +};
>> +
>> +struct map_param_msg {
>> +	__le32 hw_type;
>> +	__le32 mon_idx;
>> +	__le32 nr_rows;
>> +	struct cpucp_map_table tbl[MAX_MAP_ENTRIES];
>> +} __packed;
> 
> Does this really need the __packed? And if so, why is the only message
> that you explicitly mark as packed?
> 

It doesn't. I'll drop the __packed so all the message structs are consistent.

>> +
>> +struct node_msg {
>> +	__le32 cpumask;
>> +	__le32 hw_type;
>> +	__le32 mon_type;
>> +	__le32 mon_idx;
>> +	char mon_name[MAX_NAME_LEN];
>> +};
>> +
>> +struct scalar_param_msg {
>> +	__le32 hw_type;
>> +	__le32 mon_idx;
>> +	__le32 val;
>> +};
>> +
>> +struct ev_map_msg {
>> +	__le32 num_evs;
>> +	__le32 hw_type;
>> +	__le32 cid[NUM_COMMON_EVS];
>> +};
>> +
>> +struct scmi_qcom_memlat_map {
>> +	unsigned int cpufreq_mhz;
>> +	unsigned int memfreq_khz;
> 
> unit soup
> 

Same fix as above, will make both MHz.

>> +};
>> +
>> +struct scmi_qcom_monitor_info {
>> +	struct scmi_qcom_memlat_map *freq_map;
>> +	char name[MAX_NAME_LEN];
>> +	u32 mon_idx;
>> +	u32 mon_type;
>> +	u32 ipm_ceil;
>> +	u32 be_stall_floor;
>> +	u32 mask;
>> +	u32 freq_map_len;
> 
> Most of these values are merely copied from const tables to heap on
> probe - verbatim - and then put in messages. Would it be possible to
> just reference the static configuration, or are the any dynamic aspects
> that I'm missing?
> 

Apart from sample_ms, there's nothing dynamic here, it really is just a copy.
I'll build the messages directly from the const config instead of staging it
in heap structs, which also removes most of the allocation and cleanup below.

>> +};
>> +
>> +struct scmi_qcom_memory_info {
>> +	struct scmi_qcom_monitor_info **monitor;
>> +	u32 hw_type;
>> +	int monitor_cnt;
> 
> Why signed?
> 

Ack, will make unsigned.

>> +	u32 min_freq;
>> +	u32 max_freq;
> 
> Throughout the driver we see frequencies expressed in mhz, khz, and hz.
> Suffix these to help the reader.
> 

Ack

>> +	struct devfreq_dev_profile profile;
>> +	struct devfreq *devfreq;
>> +	struct platform_device *pdev;
>> +	struct scmi_protocol_handle *ph;
>> +	const struct qcom_generic_ext_ops *ops;
>> +};
>> +
>> +struct scmi_qcom_memlat_info {
>> +	struct scmi_protocol_handle *ph;
>> +	const struct qcom_generic_ext_ops *ops;
>> +	const struct scmi_qcom_memlat_cfg_data *cfg_data;
>> +	struct scmi_qcom_memory_info **memory;
>> +	u32 cpucp_freq_method;
>> +	u32 cpucp_sample_ms;
>> +	int memory_cnt;
> 
> Sounds like an unsigned to me
> 

Ack

>> +};
>> +
>> +static int configure_cpucp_common_events(struct scmi_qcom_memlat_info *info,
>> +					 const struct scmi_qcom_memlat_cfg_data *cfg_data)
>> +{
>> +	const struct qcom_generic_ext_ops *ops = info->ops;
>> +	struct ev_map_msg msg = {};
>> +	int i;
>> +
>> +	msg.num_evs = cpu_to_le32(NUM_COMMON_EVS);
>> +	/* Common events apply to all groups; INVALID_IDX flags "no specific group". */
> 
> Does hw_type of INVALID_IDX mean "all groups", or "any (no specific)
> group"?
> 
> Is the protocol defined that you pass "invalid index" for both hw_type
> and cid? Or do we just reuse the INVALID_IDX define because it happens
> to have the right value?
> 
> Why not "#define HW_TYPE_ALL 0xff"? Then you can skip the second half of
> the comment above, which only purpose is to clarify why the code looks
> "wrong".
> 

Common events apply to every group, so hw_type is a don't-care here.
Reusing INVALID_IDX was just convenient. I'll add a named #define
HW_TYPE_ALL 0xff and drop the apologetic comment.

>> +	msg.hw_type = cpu_to_le32(INVALID_IDX);
>> +	for (i = 0; i < NUM_COMMON_EVS; i++)
>> +		msg.cid[i] = cpu_to_le32(cfg_data->common_ev[i]);
>> +
>> +	return ops->set_param(info->ph, &msg, sizeof(msg), MEMLAT_ALGO_STR,
> 
> This is always info->ops->set_param(info->ph, ..., ..., MEMLAT_ALGO_STR,
> ...), why not create a helper unction that takes info, the command,
> message and size instead?
> 
> Would make the code less noisy.
> 

Ack, I'll add a small wrapper (memlat_set_param(info, cmd, msg, size)) and use
it throughout.

>> +			      MEMLAT_SET_COMMON_EV_MAP);
>> +}
>> +
>> +static int configure_cpucp_grp(struct device *dev, struct scmi_qcom_memlat_info *info,
>> +			       const struct scmi_qcom_memlat_cfg_data *cfg_data,
>> +			       int memory_index)
>> +{
>> +	const u32 *grp_ev = cfg_data->memory_cfg[memory_index].grp_ev;
>> +	struct scmi_qcom_memory_info *memory = info->memory[memory_index];
>> +	const struct qcom_generic_ext_ops *ops = info->ops;
>> +	struct ev_map_msg ev_msg = {};
>> +	struct node_msg msg = {};
>> +	int ret;
>> +	int i;
>> +
>> +	msg.cpumask = cpu_to_le32(*cpumask_bits(cpu_possible_mask));
>> +	msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	msg.mon_type = 0;
>> +	msg.mon_idx = 0;
>> +	ret = ops->set_param(info->ph, &msg, sizeof(msg), MEMLAT_ALGO_STR, MEMLAT_SET_MEM_GROUP);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to configure mem type %d\n",
>> +				     memory->hw_type);
>> +
>> +	ev_msg.num_evs = cpu_to_le32(NUM_GRP_EVS);
>> +	ev_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	for (i = 0; i < NUM_GRP_EVS; i++)
>> +		ev_msg.cid[i] = cpu_to_le32(grp_ev[i]);
>> +
>> +	ret = ops->set_param(info->ph, &ev_msg, sizeof(ev_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_SET_GRP_EV_MAP);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to configure event map for mem type %d\n",
>> +				     memory->hw_type);
>> +
>> +	return ret;
>> +}
>> +
>> +static int configure_cpucp_mon(struct device *dev, struct scmi_qcom_memlat_info *info,
>> +			       int memory_index, int monitor_index)
>> +{
>> +	const struct qcom_generic_ext_ops *ops = info->ops;
>> +	struct scmi_qcom_memory_info *memory = info->memory[memory_index];
>> +	struct scmi_qcom_monitor_info *monitor = memory->monitor[monitor_index];
>> +	struct scalar_param_msg scalar_msg = {};
>> +	struct map_param_msg map_msg = {};
>> +	struct node_msg msg = {};
> 
> So you have scalar_msg, map_msg, and msg... Why is "msg" just "msg" and
> not "node_msg", is it special?
> 
> If you made scalar_param_msg the "msg" instead, this function would be
> significantly less characters :)
> 
> In fact, is it significant that these local variables are denoted to be
> messages? If you name them "scalar", "map" and "node" you will reduce
> the noise below quite a bit.
> 
> Same goes for all other functions that has multiple messages - where the
> message type is the more significant fact, and you choose to name the
> variable "this is a message".
> 

Ack, I'll rename to scalar/map/node (dropping the _msg suffix) here and in the
other functions that hold several or maybe drop this entirely.

>> +	int ret;
>> +	int i;
>> +
>> +	msg.cpumask = cpu_to_le32(monitor->mask);
>> +	msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	msg.mon_type = cpu_to_le32(monitor->mon_type);
>> +	msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	strscpy(msg.mon_name, monitor->name, sizeof(msg.mon_name));
>> +	ret = ops->set_param(info->ph, &msg, sizeof(msg), MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to configure monitor %s\n",
>> +				     monitor->name);
>> +
>> +	scalar_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	scalar_msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	scalar_msg.val = cpu_to_le32(monitor->ipm_ceil);
>> +	ret = ops->set_param(info->ph, &scalar_msg, sizeof(scalar_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_IPM_CEIL);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to set ipm ceil for %s\n",
>> +				     monitor->name);
>> +
>> +	scalar_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	scalar_msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	scalar_msg.val = cpu_to_le32(monitor->be_stall_floor);
>> +	ret = ops->set_param(info->ph, &scalar_msg, sizeof(scalar_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_BE_STALL_FLOOR);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to set be_stall_floor for %s\n",
>> +				     monitor->name);
>> +
>> +	map_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	map_msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	map_msg.nr_rows = cpu_to_le32(monitor->freq_map_len);
>> +	for (i = 0; i < monitor->freq_map_len; i++) {
>> +		map_msg.tbl[i].v1 = cpu_to_le16(monitor->freq_map[i].cpufreq_mhz);
>> +
>> +		/*
>> +		 * Wire format v2 is u16 in MHz; convert from kHz.
> 
> Why is the freq_map cpufreq defined in MHz, but the memfreq in kHz - and
> then we convert the memory frequency to MHz before we write it?
> 
> Why are they not both defined in MHz?
> 

Ack, Firmware expects both values in MHz so I'll make memfreq as MHz in header
as well.

>> For DDR_QOS
>> +		 * the table holds level indices (0 / 1) rather than real
>> +		 * frequencies, so pass them through unchanged.
> 
> "holds level indices", "pass them through unchanged"? What are you
> trying to say?
> 

I meant: for DDR_QOS these are 0/1 levels, not frequencies, so don't divide
them. Won't need this anyway.

>> +		 */
>> +		if (monitor->freq_map[i].memfreq_khz > 1)
> 
> If you change memfreq_khz to memfreq_mhz you can remove the whole
> comment, the condition, as well as the divide. There might be a reason
> for you to do this, but the comment is poor, so I simply can't tell.
> 

Ack

>> +			map_msg.tbl[i].v2 = cpu_to_le16(monitor->freq_map[i].memfreq_khz / 1000);
>> +		else
>> +			map_msg.tbl[i].v2 = cpu_to_le16(monitor->freq_map[i].memfreq_khz);
>> +	}
>> +	ret = ops->set_param(info->ph, &map_msg, sizeof(map_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_MON_FREQ_MAP);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to configure freq_map for %s\n",
>> +				     monitor->name);
>> +
>> +	scalar_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	scalar_msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	scalar_msg.val = cpu_to_le32(memory->min_freq);
>> +	ret = ops->set_param(info->ph, &scalar_msg, sizeof(scalar_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_SET_MIN_FREQ);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to set min_freq for %s\n",
>> +				     monitor->name);
>> +
>> +	scalar_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +	scalar_msg.mon_idx = cpu_to_le32(monitor->mon_idx);
>> +	scalar_msg.val = cpu_to_le32(memory->max_freq);
>> +	ret = ops->set_param(info->ph, &scalar_msg, sizeof(scalar_msg), MEMLAT_ALGO_STR,
>> +			     MEMLAT_SET_MAX_FREQ);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to set max_freq for %s\n", monitor->name);
>> +
>> +	return ret;
>> +}
>> +
>> +static int scmi_qcom_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>> +{
>> +	struct scmi_qcom_memory_info *memory = dev_get_drvdata(dev);
>> +	const struct qcom_generic_ext_ops *ops = memory->ops;
>> +	struct scalar_param_msg scalar_msg = {};
>> +	u32 max_freq_khz = 0;
>> +	__le32 cur_freq;
>> +	int ret, i;
>> +
>> +	/*
>> +	 * MEMLAT_GET_CUR_FREQ returns target_freq for a single (hw_type,
>> +	 * mon_idx) tuple. The bus's actual voted frequency is the max across
>> +	 * all configured monitors in the group, so query each one and pick
>> +	 * the highest vote.
> 
> You can say this with fewer words.
> 

Ack

>> +	 */
>> +	for (i = 0; i < memory->monitor_cnt; i++) {
>> +		scalar_msg.hw_type = cpu_to_le32(memory->hw_type);
>> +		scalar_msg.mon_idx = cpu_to_le32(memory->monitor[i]->mon_idx);
>> +		scalar_msg.val = 0;
>> +
>> +		ret = ops->get_param(memory->ph, &scalar_msg, sizeof(scalar_msg),
>> +				     MEMLAT_ALGO_STR, MEMLAT_GET_CUR_FREQ,
>> +				     sizeof(cur_freq));
>> +		if (ret < 0) {
>> +			dev_err(dev, "failed to get current frequency for %s\n",
>> +				memory->monitor[i]->name);
>> +			return ret;
> 
> We're grabbing a bunch of these, is it really fatal if one fails?
> 

No, it's a poll callback, so bailing on one failed monitor query is too
harsh. I'll skip the failing monitor and keep aggregating the rest.

>> +		}
>> +
>> +		/* qcom_scmi_common_xfer() returns the response into the same tx buffer. */
> 
> What is qcom_scmi_common_xfer()?
> 
> You can express in C the fact that msg is a in/out - by declaring it as
> a union.
> 
> That way you don't need a comment to describe why you're reinterpreting
> the first 4 bytes of msg as the result of the query.
> 

It's the internal transport that reuses the tx buffer for the response. I'll
try to express the in/out with a union (request struct plus a __le32
response) so the reinterpret is self-documenting and the memcpy and comment
can go.

>> +		memcpy(&cur_freq, &scalar_msg, sizeof(cur_freq));
>> +		if (le32_to_cpu(cur_freq) > max_freq_khz)
>> +			max_freq_khz = le32_to_cpu(cur_freq);
> 
> And you could then le32_to_cpu() out of the union to a local variable
> once, to avoid the double le32_to_cpu().
> 

Ack

>> +	}
>> +
>> +	/*
>> +	 * Frequency-scaled buses (DDR/LLCC) report cur_freq in kHz; convert
>> +	 * to Hz to match the devfreq OPP table units. Level-based buses
>> +	 * (e.g. DDR_QOS_COMPUTE) configure max_freq == 1 because the firmware
>> +	 * reports a 0/1 level rather than a frequency, and the matching
>> +	 * synthetic OPP keys (1 / 100) live in glymur_ddr_qos_table.
> 
> You surely don't need 5 lines of solid text to express this.
> 

Ack, will cut it down to a line or two.

>> +	 */
>> +	if (memory->max_freq > 1)
>> +		*freq = max_freq_khz * 1000UL;
>> +	else
>> +		*freq = max_freq_khz ? 100 : 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static void scmi_qcom_memlat_unwind(struct scmi_qcom_memlat_info *info, int count)
>> +{
>> +	for (int i = 0; i < count; i++) {
>> +		struct scmi_qcom_memory_info *memory = info->memory[i];
>> +
>> +		if (IS_ERR_OR_NULL(memory) || IS_ERR_OR_NULL(memory->pdev))
> 
> I don't get it, how can "memory" be IS_ERR()? Isn't it the result of the
> devm_kcalloc() in scmi_qcom_memlat_parse_cfg()?
> 

Ack, I'll use a plain !memory check (or drop it once the cleanup is
restructured).

>> +			continue;
>> +
>> +		dev_pm_opp_remove_all_dynamic(&memory->pdev->dev);
>> +		platform_device_unregister(memory->pdev);
>> +	}
>> +}
>> +
>> +static int scmi_qcom_memlat_configure_events(struct scmi_device *sdev,
>> +					     struct scmi_qcom_memlat_info *info)
>> +{
>> +	const struct qcom_generic_ext_ops *ops = info->ops;
>> +	struct scmi_protocol_handle *ph = info->ph;
>> +	__le32 sample_ms, freq_method;
>> +	int i, j, ret;
>> +
>> +	/* Configure common events ids */
>> +	ret = configure_cpucp_common_events(info, info->cfg_data);
>> +	if (ret < 0)
>> +		return dev_err_probe(&sdev->dev, ret, "failed to configure common events\n");
>> +
>> +	for (i = 0; i < info->memory_cnt; i++) {
>> +		/* Configure per group parameters */
>> +		ret = configure_cpucp_grp(&sdev->dev, info, info->cfg_data, i);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		for (j = 0; j < info->memory[i]->monitor_cnt; j++) {
>> +			/* Configure per monitor parameters */
>> +			ret = configure_cpucp_mon(&sdev->dev, info, i, j);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	/* Set loop sampling time */
>> +	sample_ms = cpu_to_le32(info->cpucp_sample_ms);
>> +	ret = ops->set_param(ph, &sample_ms, sizeof(sample_ms),
>> +			     MEMLAT_ALGO_STR, MEMLAT_SAMPLE_MS);
>> +	if (ret < 0)
>> +		return dev_err_probe(&sdev->dev, ret, "failed to set sample_ms\n");
>> +
>> +	/* Set the effective cpu frequency calculation method */
>> +	freq_method = cpu_to_le32(info->cpucp_freq_method);
>> +	ret = ops->set_param(ph, &freq_method, sizeof(freq_method),
>> +			     MEMLAT_ALGO_STR, MEMLAT_SET_EFFECTIVE_FREQ_METHOD);
>> +	if (ret < 0)
>> +		return dev_err_probe(&sdev->dev, ret,
>> +				     "failed to set effective frequency calc method\n");
>> +
>> +	/* Start sampling and voting timer */
>> +	ret = ops->start_activity(ph, NULL, 0, MEMLAT_ALGO_STR, MEMLAT_START_TIMER);
>> +	if (ret < 0)
>> +		return dev_err_probe(&sdev->dev, ret, "failed to start memory group timer\n");
>> +
>> +	for (i = 0; i < info->memory_cnt; i++) {
>> +		struct scmi_qcom_memory_info *memory = info->memory[i];
>> +		struct platform_device *pdev = memory->pdev;
>> +		struct devfreq_dev_profile *profile = &memory->profile;
>> +
>> +		/* sampling time should be double the devfreq observing time */
> 
> That's interesting, tell me more...
> 

This follows Lukasz's earlier point on Nyquist criterion: sample about 2x
faster than the changes you want to observe. CPUCP updates every
cpucp_sample_ms, so the devfreq poll runs at half that (sample_ms / 2) to
actually catch the transitions in trans_stat.

>> +		profile->polling_ms = max(1U, info->cpucp_sample_ms / 2);
>> +		profile->get_cur_freq = scmi_qcom_devfreq_get_cur_freq;
>> +		profile->initial_freq = memory->min_freq > 1 ?
>> +					(memory->min_freq * 1000UL) : memory->min_freq;
> 
> What is the purpose of this conditional?
> 
> You support memories with an actual frequency, and what you call
> "level". But for any memory with a "level" resource other than a boolean
> this logic fails.
> 
> For min_freq == 0 we get initial_freq of 0, both for frequency-based and
> level-based memories.
> 
> So the only case where this actually does anything is if you have a
> boolean resource with minimum value of 1 (i.e. a constant). Do we have
> those?
> 

The guard just separates DDR_QOS (level-based) from the frequency buses: for
DDR/LLCC, devfreq wants Hz, so initial_freq = min_freq * 1000, while
DDR_QOS passes its value through as-is.

>> +
>> +		platform_set_drvdata(pdev, memory);
>> +
>> +		memory->devfreq = devm_devfreq_add_device(&pdev->dev, profile,
>> +							  DEVFREQ_GOV_REMOTE, NULL);
>> +		if (IS_ERR(memory->devfreq)) {
>> +			dev_err(&sdev->dev, "failed to add devfreq device\n");
> 
> I personally don't like all the dev_err_probe() that you have sprinkled
> throughout the driver, but I know that many others do - and this one
> isn't consistent.
> 

Ack, I'll use dev_err_probe uniformly on the probe paths.

>> +			/* Stop sampling and voting timer */
>> +			ret = ops->stop_activity(ph, NULL, 0, MEMLAT_ALGO_STR, MEMLAT_STOP_TIMER);
>> +			if (ret < 0)
>> +				dev_err_probe(&sdev->dev, ret,
>> +					      "failed to stop memory group timer\n");
> 
> The significant event is that adding the devfreq device failed, but the
> error message that you retain is "failed to stop memory group timer"...
> 

I am already capturing devfreq failure just above. This is inside that scope.
Do you want me to drop this? 

>> +			return PTR_ERR(memory->devfreq);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct scmi_qcom_memlat_map *
>> +scmi_qcom_parse_memlat_map(struct device *dev, const struct scmi_qcom_monitor_cfg *mon_cfg)
>> +{
>> +	struct scmi_qcom_memlat_map *map_table;
>> +	const struct scmi_qcom_map_table *table;
>> +
>> +	if (mon_cfg->table_len > MAX_MAP_ENTRIES)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	map_table = devm_kcalloc(dev, mon_cfg->table_len, sizeof(*map_table),
>> +				 GFP_KERNEL);
>> +	if (!map_table)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (int i = 0; i < mon_cfg->table_len; i++) {
>> +		table = &mon_cfg->table[i];
>> +
>> +		map_table[i].cpufreq_mhz = table->cpu_freq;
>> +		map_table[i].memfreq_khz = table->mem_freq;
>> +	}
>> +
>> +	return map_table;
>> +}
>> +
>> +static const struct of_device_id scmi_qcom_memlat_configs[] = {
>> +	{ .compatible = "qcom,glymur", .data = &glymur_memlat_data},
>> +	{ .compatible = "qcom,mahua", .data = &glymur_memlat_data},
>> +	{ .compatible = "qcom,x1e80100", .data = &hamoa_memlat_data},
>> +	{ .compatible = "qcom,x1p42100", .data = &hamoa_memlat_data},
>> +	{ }
>> +};
>> +
>> +static int scmi_qcom_memlat_parse_cfg(struct scmi_device *sdev, struct scmi_qcom_memlat_info *info)
>> +{
>> +	const struct scmi_qcom_memlat_cfg_data *cfg_data;
>> +	struct scmi_qcom_monitor_info *monitor;
>> +	struct scmi_qcom_memory_info *memory;
>> +	int ret, i, j;
>> +
>> +	cfg_data = of_machine_get_match_data(scmi_qcom_memlat_configs);
>> +	if (!cfg_data) {
>> +		/*
>> +		 * The SCMI generic-ext protocol can bind on Qualcomm SoCs that
>> +		 * do not ship CPUCP memlat. Bail out quietly in that case rather
>> +		 * than printing an error on every such system.
>> +		 */
>> +		dev_dbg(&sdev->dev, "no memlat config data for this platform\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	info->memory = devm_kcalloc(&sdev->dev, cfg_data->memory_cnt,
>> +				    sizeof(*info->memory), GFP_KERNEL);
>> +	if (!info->memory)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < cfg_data->memory_cnt; i++) {
>> +		const struct scmi_qcom_memory_cfg *memory_cfg = &cfg_data->memory_cfg[i];
>> +		struct platform_device_info pdevinfo = { 0 };
>> +
>> +		pdevinfo.parent = &sdev->dev;
>> +		pdevinfo.name = memory_cfg->name;
>> +		pdevinfo.id = PLATFORM_DEVID_NONE;
>> +
>> +		memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
>> +		if (!memory)
>> +			return -ENOMEM;
>> +
>> +		memory->ops = info->ops;
>> +		memory->ph = info->ph;
>> +		memory->hw_type = memory_cfg->memory_type;
>> +		memory->monitor_cnt = memory_cfg->monitor_cnt;
>> +		memory->min_freq = memory_cfg->memory_range.min_freq;
>> +		memory->max_freq = memory_cfg->memory_range.max_freq;
>> +
>> +		memory->pdev = platform_device_register_full(&pdevinfo);
> 
> Why are there platform_devices in here?
> 
> I'm guessing that you want the struct devices to hang the opp tables
> off? But why a whole platform_device?
> 

Yes, I'll try to explore a lighter alternative.

>> +		if (IS_ERR(memory->pdev))
>> +			return dev_err_probe(&sdev->dev, PTR_ERR(memory->pdev),
>> +					     "failed to register platform device\n");
>> +
>> +		info->memory[i] = memory;
>> +		/* Track progress so probe() can unwind on a later failure. */
> 
> Please clean up your own mess. If this function becomes too long for you
> to be able to get things in order, that's just another indication that
> you need to refactor this function.
> 

Ack, I'll refactor so each function unwinds its own partial work on error, and
drop the external progress counter.

>> +		info->memory_cnt = i + 1;
>> +
>> +		for (j = 0; j < memory_cfg->num_opps; j++) {
>> +			const struct scmi_qcom_opp_data *table = &memory_cfg->mem_table[j];
>> +			struct platform_device *pdev = memory->pdev;
>> +			struct dev_pm_opp_data data = {};
>> +
>> +			data.freq = table->freq;
>> +			data.level = table->level;
>> +
>> +			ret = dev_pm_opp_add_dynamic(&pdev->dev, &data);
>> +			if (ret)
>> +				return dev_err_probe(&sdev->dev, ret, "failed to add OPP\n");
>> +		}
>> +
>> +		memory->monitor = devm_kcalloc(&sdev->dev, memory_cfg->monitor_cnt,
>> +					       sizeof(*memory->monitor), GFP_KERNEL);
>> +		if (!memory->monitor)
>> +			return -ENOMEM;
>> +
>> +		for (j = 0; j < memory_cfg->monitor_cnt; j++) {
>> +			const struct scmi_qcom_monitor_cfg *mon_cfg = &memory_cfg->monitor_cfg[j];
>> +
>> +			monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor), GFP_KERNEL);
>> +			if (!monitor)
>> +				return -ENOMEM;
>> +
>> +			monitor->ipm_ceil = mon_cfg->ipm_ceil;
>> +			/* mon_type 0 = IPM-based latency monitor; 1 = stall-only (compute) */
>> +			monitor->mon_type = monitor->ipm_ceil ? 0 : 1;
> 
> So the actual mon_type in the message is just a 0 or 1? Is this 0 the
> same as the 0 in configure_cpucp_grp()? Should we have a couple of
> defines for these - instead of the comment?
> 

It's just latency(0)/compute(1). I'll make it a bool is_compute in the
driver too and drop the magic 0/1 and the comment. The mon_type/mon_idx
in configure_cpucp_grp is a don't-care, I'll stop reusing a bare 0 there.

>> +			monitor->be_stall_floor = mon_cfg->be_stall_floor;
>> +			monitor->mask = mon_cfg->cpu_mask;
>> +			monitor->freq_map_len = mon_cfg->table_len;
>> +
>> +			monitor->freq_map = scmi_qcom_parse_memlat_map(&sdev->dev, mon_cfg);
> 
> It might be a good idea to do this before you're starting to register
> platform_devices and opps, as it would simplify the cleanup.
> 

Ack

>> +			if (IS_ERR(monitor->freq_map))
>> +				return dev_err_probe(&sdev->dev, PTR_ERR(monitor->freq_map),
>> +						     "failed to populate cpufreq-memfreq map\n");
> 
> There are two ways scmi_qcom_parse_memlat_map() can fail, either the
> driver data is wrong and you get EINVAL (a developer BUG) or you get
> ENOMEM, which is already printed...
> 

Ack, will drop this.

>> +
>> +			strscpy(monitor->name, mon_cfg->name, sizeof(monitor->name));
> 
> Do you really need a copy? As far as I can see the next access is
> another copy out of this struct.
> 

No, it's redundant. Once messages are built from the const config directly,
I'll reference mon_cfg->name and drop the intermediate copy.

>> +			monitor->mon_idx = j;
>> +			memory->monitor[j] = monitor;
>> +		}
>> +	}
>> +
>> +	info->cfg_data = cfg_data;
>> +	info->cpucp_freq_method = cfg_data->cpucp_freq_method;
>> +	info->cpucp_sample_ms = cfg_data->cpucp_sample_ms;
> 
> So much code just moving data between two representations...
> 

Ack, Building messages straight from the const config collapses the two
representations and removes most of this function.

>> +
>> +	return 0;
>> +}
>> +
>> +static int scmi_qcom_devfreq_memlat_probe(struct scmi_device *sdev)
>> +{
>> +	const struct scmi_handle *handle = sdev->handle;
>> +	const struct qcom_generic_ext_ops *ops;
>> +	struct scmi_qcom_memlat_info *info;
>> +	struct scmi_protocol_handle *ph;
>> +	int ret;
>> +
>> +	if (!handle)
>> +		return -ENODEV;
>> +
>> +	info = devm_kzalloc(&sdev->dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_QCOM_GENERIC, &ph);
>> +	if (IS_ERR(ops))
>> +		return PTR_ERR(ops);
>> +
>> +	info->ops = ops;
>> +	info->ph = ph;
>> +
>> +	ret = scmi_qcom_memlat_parse_cfg(sdev, info);
>> +	if (ret) {
>> +		scmi_qcom_memlat_unwind(info, info->memory_cnt);
> 
> scmi_qcom_memlat_unwind() unwinds the work done by
> scmi_qcom_memlat_parse_cfg(), this isn't really idiomatic. Please have
> scmi_qcom_memlat_parse_cfg() clean up its own mess on failure.
> 

Ack. The non-devm bits (platform_device_register_full and
dev_pm_opp_add_dynamic) are why the external unwind exists, I'll handle those
inside the function's own error paths.

>> +		return ret;
>> +	}
>> +
>> +	ret = scmi_qcom_memlat_configure_events(sdev, info);
>> +	if (ret) {
>> +		scmi_qcom_memlat_unwind(info, info->memory_cnt);
>> +		return ret;
>> +	}
>> +
>> +	dev_set_drvdata(&sdev->dev, info);
>> +
>> +	return ret;
>> +}
>> +
>> +static void scmi_qcom_devfreq_memlat_remove(struct scmi_device *sdev)
>> +{
>> +	struct scmi_qcom_memlat_info *info = dev_get_drvdata(&sdev->dev);
>> +	struct scmi_protocol_handle *ph;
>> +	const struct qcom_generic_ext_ops *ops;
>> +	int ret;
>> +
>> +	if (!info)
> 
> How is that possible?
> 

It isn't, probe always sets drvdata to a valid info on success. I'll drop the
!info check.

>> +		return;
>> +
>> +	ph = info->ph;
>> +	ops = info->ops;
>> +
>> +	ret = ops->stop_activity(ph, NULL, 0, MEMLAT_ALGO_STR, MEMLAT_STOP_TIMER);
>> +	if (ret < 0)
>> +		dev_err(&sdev->dev, "failed to stop memory group timer\n");
>> +
>> +	scmi_qcom_memlat_unwind(info, info->memory_cnt);
>> +}
>> +
>> +static const struct scmi_device_id scmi_id_table[] = {
>> +	{ SCMI_PROTOCOL_QCOM_GENERIC, "qcom-generic-ext" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>> +
>> +static struct scmi_driver scmi_qcom_devfreq_memlat_driver = {
>> +	.name		= "scmi-qcom-devfreq-memlat",
>> +	.probe		= scmi_qcom_devfreq_memlat_probe,
>> +	.remove		= scmi_qcom_devfreq_memlat_remove,
>> +	.id_table	= scmi_id_table,
>> +};
>> +module_scmi_driver(scmi_qcom_devfreq_memlat_driver);
>> +
>> +MODULE_AUTHOR("Pragnesh Papaniya <[email protected]>");
>> +MODULE_DESCRIPTION("SCMI QCOM DEVFREQ MEMLAT driver");
> 
> Let go of the shift key!
> 
> s/QCOM/Qualcomm/ at least.
> 
> Regards,
> Bjorn
> 

Ack, I'll change it to sentence case: "Qualcomm SCMI memlat devfreq driver".
Thanks for reviewing this!

-Pragnesh

>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> 2.34.1
>>