Received-SPF: pass (google.com: domain of linux-kernel+bounces-29429-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <2cbdbe92-ce36-4147-a64f-7f86ee3761bc@linaro.org>
Date: Wed, 17 Jan 2024 21:38:58 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC 7/7] arm64: dts: qcom: x1e80100: Enable LLCC/DDR dvfs
Content-Language: en-US
To: Sibi Sankar <quic_sibis@quicinc.com>, sudeep.holla@arm.com,
 cristian.marussi@arm.com, andersson@kernel.org, jassisinghbrar@gmail.com,
 robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
 devicetree@vger.kernel.org, quic_rgottimu@quicinc.com,
 quic_kshivnan@quicinc.com, conor+dt@kernel.org
References: <20240117173458.2312669-1-quic_sibis@quicinc.com>
 <20240117173458.2312669-8-quic_sibis@quicinc.com>
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20240117173458.2312669-8-quic_sibis@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 1/17/24 18:34, Sibi Sankar wrote:
> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> index 6856a206f7fc..3dc6f32fbb4c 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
>   				reg = <0x13>;
>   				#clock-cells = <1>;
>   			};
> +
> +			scmi_vendor: protocol@80 {
> +				reg = <0x80>;
> +
> +				memlat {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					memory@0 {
> +						reg = <0x0>; /* Memory Type DDR */

I'm not sure reg is the best property to (ab)use..

You could very well define a new one, like qcom,memory type,
then the subnodes could look like:

memory-0 {
	qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
	[...]
};

> +						freq-table-khz = <200000 4224000>;
> +
> +						monitor-0 {
> +							qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;

I fail to see the usefulness in checking which CPUs make use of
the same DRAM or LLC pool. If that's something that may not be
obvious in future designs like on dual-socket x86 servers,
I think it can be deferred until then and for now, AFAIU you
can just unconditionally assume all CPUs count.

> +							qcom,cpufreq-memfreq-tbl = < 999000 547000 >,
> +										   < 1440000 768000 >,
> +										   < 1671000 1555000 >,
> +										   < 2189000 2092000 >,
> +										   < 2156000 3187000 >,
> +										   < 3860000 4224000 >;

I.. can't seem to think of a future where this doesn't explode.

When you release a different bin/SKU/fuse config of this SoC where
the CPU frequencies are different, this will likely also need to be
updated. We don't want that manual cruft in the devicetree.

Since both previously cpufreq-hw and now cpufreq-scmi generally
operate on levels that map to some frequencies in the firmware,
could it be bound to that instead?

Konrad