Re: [PATCH 4/4] interconnect: qcom: icc-rpm: Remodel how QoS settings are stored

From: Konrad Dybcio <konrad.dybcio@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Georgi Djakov <djakov@kernel.org>,
	Shawn Guo <shawn.guo@linaro.org>,
	Marijn Suijten <marijn.suijten@somainline.org>,
	linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] interconnect: qcom: icc-rpm: Remodel how QoS settings are stored
Date: Tue, 26 Mar 2024 22:12:16 +0100	[thread overview]
Message-ID: <1e138728-f78e-4bfe-b59b-297e599759ab@linaro.org> (raw)
In-Reply-To: <ZgM2naP4mGLKwbCV@gerhold.net>

On 26.03.2024 9:57 PM, Stephan Gerhold wrote:
> On Tue, Mar 26, 2024 at 08:42:35PM +0100, Konrad Dybcio wrote:
>> Currently, the QoS settings are stored in the node data, even though
>> they're a property of the bus/provider instead. Moreover, they are only
>> needed during the probe step, so they can be easily moved into struct
>> qcom_icc_desc.
>>
>> Reshuffle things around to make it anywhere near readable & comparable
>> with a reference. As a nice bonus, a lot of bytes are shaved off and
>> a few miliseconds are shaved off here and there.
>>
>> As an example, bloat-o-meter reports this on sm6115.o:
>> Total: Before=14799, After=13263, chg -10.38%
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---

[...]

> Nitpick: Why is the u16 const when the other (non-pointer) members are
> not? The u16 also feels a bit like overkill here. The struct would have
> exactly the same size with a full unsigned int because of padding.

That's just my brain performing premature (and as you can see invalid)
optimizations.. I can change it to u32

> 
> Alternatively, you could consider using an empty last entry as sentinel
> instead of adding the count (i.e. with NOC_QOS_MODE_INVALID = 0). Not
> sure what is cleaner here.

Nah, let's keep the counter

> 
> I haven't looked closely at the actual conversion of the definitions in
> the drivers. What is the chance that you made an accidental mistake in
> there? Or was it scripted? :D

By hand. After this change, it should hopefully be very easy and
convenient to check against downstream.

Konrad