Re: [PATCH RFC v7 7/9] PM / devfreq: Introduce the QCOM SCMI Memlat devfreq driver
From: Pragnesh Papaniya
Date: Mon Jul 13 2026 - 15:44:45 EST
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 <sibi.sankar@xxxxxxxxxxxxxxxx>
>>
>> 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 <amir.vajid@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Amir Vajid <amir.vajid@xxxxxxxxxxxxxxxx>
>> Co-developed-by: Ramakrishna Gottimukkula <ramakrishna.gottimukkula@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Ramakrishna Gottimukkula <ramakrishna.gottimukkula@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Sibi Sankar <sibi.sankar@xxxxxxxxxxxxxxxx>
>> Co-developed-by: Pragnesh Papaniya <pragnesh.papaniya@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Pragnesh Papaniya <pragnesh.papaniya@xxxxxxxxxxxxxxxx>
>> ---
>> 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 <pragnesh.papaniya@xxxxxxxxxxxxxxxx>");
>> +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
>>