On Thu, Nov 30, 2023 at 12:05:06PM +0000, Sudeep Holla wrote:
> 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 ?
>
Hi
the cleanup seems nice in general to compact the mult_factor multipliers
in one place, and regarding addressing the problem of truncation without
the need of the explicit casting, should not be enough to change to
additionally also change mult_factor to be an u64 ?
Not tested so I could miss something...
Thanks,
Cristian
> 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;
>
