Received-SPF: pass (google.com: domain of linux-kernel+bounces-119963-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 26 Mar 2024 21:57:01 +0100
From: Stephan Gerhold <stephan@gerhold.net>
To: Konrad Dybcio <konrad.dybcio@linaro.org>
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
Message-ID: <ZgM2naP4mGLKwbCV@gerhold.net>
References: <20240326-topic-rpm_icc_qos_cleanup-v1-0-357e736792be@linaro.org>
 <20240326-topic-rpm_icc_qos_cleanup-v1-4-357e736792be@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326-topic-rpm_icc_qos_cleanup-v1-4-357e736792be@linaro.org>
Content-Transfer-Encoding: 7bit

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>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 123 +++++----
>  drivers/interconnect/qcom/icc-rpm.h |  13 +-
>  drivers/interconnect/qcom/msm8909.c | 268 ++++++++++---------
>  drivers/interconnect/qcom/msm8916.c | 153 ++++++-----
>  drivers/interconnect/qcom/msm8939.c | 157 ++++++-----
>  drivers/interconnect/qcom/msm8996.c | 517 +++++++++++++++++-------------------
>  drivers/interconnect/qcom/qcm2290.c | 416 +++++++++++++++++------------
>  drivers/interconnect/qcom/sdm660.c  | 393 +++++++++++++--------------
>  drivers/interconnect/qcom/sm6115.c  | 239 ++++++++++++-----
>  9 files changed, 1224 insertions(+), 1055 deletions(-)
> 
> [...]
> @@ -70,20 +68,18 @@ struct qcom_icc_provider {
>  };
>  
>  /**
> - * struct qcom_icc_qos - Qualcomm specific interconnect QoS parameters
> + * struct qcom_icc_qos_data - Qualcomm specific interconnect QoS parameters
>   * @areq_prio: node requests priority
>   * @prio_level: priority level for bus communication
>   * @limit_commands: activate/deactivate limiter mode during runtime
> - * @ap_owned: indicates if the node is owned by the AP or by the RPM
>   * @qos_mode: default qos mode for this node
>   * @qos_port: qos port number for finding qos registers of this node
>   * @urg_fwd_en: enable urgent forwarding
>   */
> -struct qcom_icc_qos {
> +struct qcom_icc_qos_data {
>  	u32 areq_prio;
>  	u32 prio_level;
>  	bool limit_commands;
> -	bool ap_owned;
>  	int qos_mode;
>  	int qos_port;
>  	bool urg_fwd_en;

Side note: There is a potential for more micro-optimization here: You
could save 4 bytes of padding if you move all bools together at the end
of the struct. :D

> [...]
> @@ -134,6 +131,8 @@ struct qcom_icc_desc {
>  	bool keep_alive;
>  	enum qcom_icc_type type;
>  	const struct regmap_config *regmap_cfg;
> +	const struct qcom_icc_qos_data * const qos_data;
> +	const u16 qos_data_num;
>  	unsigned int qos_offset;

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.

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.

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

Thanks,
Stephan