Received-SPF: pass (google.com: domain of linux-kernel+bounces-4409-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <15d2bd66-29f3-435b-8494-d82ec4036413@linaro.org>
Date: Mon, 18 Dec 2023 22:31:57 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/12] arm64: dts: qcom: sm8550: Switch UFS from
 opp-table-hz to opp-v2
To: Nitin Rawat <quic_nitirawa@quicinc.com>,
 Bjorn Andersson <andersson@kernel.org>, Georgi Djakov <djakov@kernel.org>,
 Abel Vesa <abel.vesa@linaro.org>, Michael Turquette
 <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>,
 Taniya Das <quic_tdas@quicinc.com>, Jagadeesh Kona <quic_jkona@quicinc.com>,
 Neil Armstrong <neil.armstrong@linaro.org>, Rob Herring
 <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Sai Prakash Ranjan <quic_saipraka@quicinc.com>
Cc: Marijn Suijten <marijn.suijten@somainline.org>,
 linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
 devicetree@vger.kernel.org
References: <20231218-topic-8550_fixes-v1-0-ce1272d77540@linaro.org>
 <20231218-topic-8550_fixes-v1-10-ce1272d77540@linaro.org>
 <62f0c623-3819-f6be-115f-6b471ab79a58@quicinc.com>
Content-Language: en-US
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <62f0c623-3819-f6be-115f-6b471ab79a58@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 12/18/23 17:35, Nitin Rawat wrote:
> 
> 
> On 12/18/2023 9:32 PM, Konrad Dybcio wrote:
>> Now that the non-legacy form of OPP is supported within the UFS driver,
>> go ahead and switch to it, adding support for more intermediate freq/power
>> states.
>>
>> In doing so, add the CX RPMhPD under GCC to make sure at least some of
>> the power state requirements are *actually* propagated up the stack.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 50 +++++++++++++++++++++++++++++-------
>>   1 file changed, 41 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> index d707d15cea5b..d6edd54f3ad3 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> @@ -1930,6 +1930,7 @@ ufs_mem_hc: ufs@1d84000 {
>>               iommus = <&apps_smmu 0x60 0x0>;
>>               dma-coherent;
>> +            operating-points-v2 = <&ufs_opp_table>;
>>               interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI1 0>,
>>                       <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
>> @@ -1950,18 +1951,49 @@ ufs_mem_hc: ufs@1d84000 {
>>                    <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
>>                    <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
>>                    <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
>> -            freq-table-hz =
>> -                <75000000 300000000>,
>> -                <0 0>,
>> -                <0 0>,
>> -                <75000000 300000000>,
>> -                <100000000 403000000>,
>> -                <0 0>,
>> -                <0 0>,
>> -                <0 0>;
>>               qcom,ice = <&ice>;
>>               status = "disabled";
>> +
>> +            ufs_opp_table: opp-table {
>> +                compatible = "operating-points-v2";
>> +
>> +                opp-75000000 {
>> +                    opp-hz = /bits/ 64 <75000000>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <75000000>,
>> +                         /bits/ 64 <100000000>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>;
>> +                    required-opps = <&rpmhpd_opp_low_svs>;
>> +                };
>> +
>> +                opp-150000000 {
>> +                    opp-hz = /bits/ 64 <150000000>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <150000000>,
>> +                         /bits/ 64 <100000000> > +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>;
>> +                    required-opps = <&rpmhpd_opp_svs>;
>> +                };
>> +
>> +                opp-300000000 {
>> +                    opp-hz = /bits/ 64 <300000000>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <0>,
>> +                         /bits/ 64 <300000000>,
>> +                         /bits/ 64 <100000000>,
> Hi Konrad,
> 
> This entry is for ICE clock ? Shouldn't the entry be 403000000 ?
> Same for svs as well ?
Hi Nitin,

this entry is for the TCSR_UFS_PAD_CLKREF_EN/"ref_clk" clock,
which doesn't support ratesetting, so it should probably be 0
(or its actual value if we know it - I assumed it was 100 MHz
as it was there before).

The ICE clock is handled separately by the crypto@1d88000 node.

Thinking about it again, the original submission probably included
the ICE clock within the UFS node and when TCSRCC was created,
somebody might have omitted the wrong rate value.

Konrad