Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Thu, 30 Nov 2023 12:05:06 +0000
From:   Sudeep Holla <sudeep.holla@arm.com>
To:     Sibi Sankar <quic_sibis@quicinc.com>
Cc:     <cristian.marussi@arm.com>, <linux-arm-msm@vger.kernel.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        <linux-kernel@vger.kernel.org>, <quic_mdtipton@quicinc.com>,
        <linux-arm-kernel@lists.infradead.org>, <quic_asartor@quicinc.com>,
        <quic_lingutla@quicinc.com>
Subject: Re: [PATCH 2/3] firmware: arm_scmi: Fix freq/power truncation in the
 perf protocol
Message-ID: <ZWh6cuApg-sRbA2s@bogus>
References: <20231129065748.19871-1-quic_sibis@quicinc.com>
 <20231129065748.19871-3-quic_sibis@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231129065748.19871-3-quic_sibis@quicinc.com>
Precedence: bulk

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;