Re: [PATCH 2/3] firmware: arm_scmi: Fix freq/power truncation in the perf protocol

Sudeep Holla
2023-11-30T12:05:53Z

On Wed, Nov 29, 2023 at 12:27:47PM +0530, Sibi Sankar wrote:
> Fix frequency and power truncation seen in the performance protocol by
> casting it with the correct type.
>

While I always remembered to handle this when reviewing the spec, seem to
have forgotten when it came to handling in the implementation :(. Thanks
for spotting this.

However I don't like the ugly type casting. I think we can do better. Also
looking at the code around the recently added level index mode, I think we
can simplify things like below patch.

Cristian,
What do you think ?

Regards,
Sudeep

-->8

drivers/firmware/arm_scmi/perf.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index a648521e04a3..2e828b29efab 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -268,13 +268,14 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
dom_info->sustained_perf_level =
le32_to_cpu(attr->sustained_perf_level);
if (!dom_info->sustained_freq_khz ||
- !dom_info->sustained_perf_level)
+ !dom_info->sustained_perf_level ||
+ dom_info->level_indexing_mode)
/* CPUFreq converts to kHz, hence default 1000 */
dom_info->mult_factor = 1000;
else
dom_info->mult_factor =
- (dom_info->sustained_freq_khz * 1000) /
- dom_info->sustained_perf_level;
+ (dom_info->sustained_freq_khz * 1000UL)
+ / dom_info->sustained_perf_level;
strscpy(dom_info->info.name, attr->name,
SCMI_SHORT_NAME_MAX_SIZE);
}
@@ -804,9 +805,10 @@ static int scmi_dvfs_device_opps_add(const struct scmi_protocol_handle *ph,

for (idx = 0; idx < dom->opp_count; idx++) {
if (!dom->level_indexing_mode)
- freq = dom->opp[idx].perf * dom->mult_factor;
+ freq = dom->opp[idx].perf;
else
- freq = dom->opp[idx].indicative_freq * 1000;
+ freq = dom->opp[idx].indicative_freq;
+ freq *= dom->mult_factor;

data.level = dom->opp[idx].perf;
data.freq = freq;
@@ -879,7 +881,7 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
return ret;

if (!dom->level_indexing_mode) {
- *freq = level * dom->mult_factor;
+ *freq = level;
} else {
struct scmi_opp *opp;

@@ -887,8 +889,9 @@ static int scmi_dvfs_freq_get(const struct scmi_protocol_handle *ph, u32 domain,
if (!opp)
return -EIO;

- *freq = opp->indicative_freq * 1000;
+ *freq = opp->indicative_freq;
}
+ freq *= dom->mult_factor;

return ret;
}
@@ -908,9 +911,10 @@ static int scmi_dvfs_est_power_get(const struct scmi_protocol_handle *ph,

for (opp = dom->opp, idx = 0; idx < dom->opp_count; idx++, opp++) {
if (!dom->level_indexing_mode)
- opp_freq = opp->perf * dom->mult_factor;
+ opp_freq = opp->perf;
else
- opp_freq = opp->indicative_freq * 1000;
+ opp_freq = opp->indicative_freq;
+ opp_freq *= dom->mult_factor;

if (opp_freq < *freq)
continue;