Received-SPF: pass (google.com: domain of linux-kernel+bounces-171471-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <3a3c4279-a254-48d0-91ad-70b7f1e3eb77@linaro.org>
Date: Tue, 7 May 2024 15:51:04 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] clk: qcom: gcc-sm8450: set OPS_PARENT_ENABLE on
 gcc_sdcc2_apps_clk_src
To: Stephen Boyd <sboyd@kernel.org>, Bjorn Andersson <andersson@kernel.org>,
 Michael Turquette <mturquette@baylibre.com>, Vinod Koul <vkoul@kernel.org>
Cc: Marijn Suijten <marijn.suijten@somainline.org>,
 linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
 linux-kernel@vger.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
References: <20240427-topic-8450sdc2-v1-1-631cbb59e0e5@linaro.org>
 <2337ba58adb3fb127710bead9b8665a9.sboyd@kernel.org>
 <11bd7146-30cd-4b71-b2ca-d76875763731@linaro.org>
 <6ba2967c6c9d24e3f1c9b76496176010.sboyd@kernel.org>
Content-Language: en-US
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <6ba2967c6c9d24e3f1c9b76496176010.sboyd@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/30/24 23:26, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-04-30 03:46:52)
>> On 30.04.2024 2:21 AM, Stephen Boyd wrote:
>>> Quoting Konrad Dybcio (2024-04-27 05:01:07)
>>>> Similar to how it works on other SoCs, the top frequency of the SDHCI2
>>>> core clock is generated by a separate PLL (peculiar design choice) that
>>>> is not guaranteed to be enabled (why does the clock framework not handle
>>>> this by default?).
>>>>
>>>> Add the CLK_OPS_PARENT_ENABLE flag to make sure we're not muxing the
>>>> RCG input to a dormant source.
>>>
>>> The RCG2 hardware hasn't required the parent to be enabled for clk
>>> operations besides for the glitch-free source switch. What scenario is
>>> happening here that's requiring this flag? Is the RCG forcibly enabled
>>> perhaps because the bootloader has left the root enable bit set
>>> (CMD_ROOT_EN)? Or are we changing the parent while the clk framework
>>> thinks the clk is off when it is actually on?
>>>
>>> TL;DR: This is papering over a bigger bug.
>>
>> Definitely.
>>
>>
>> Take a look at:
>>
>> static const struct freq_tbl ftbl_gcc_sdcc2_apps_clk_src[] = {
>>          F(400000, P_BI_TCXO, 12, 1, 4),
>>          F(25000000, P_GCC_GPLL0_OUT_EVEN, 12, 0, 0),
>>          F(50000000, P_GCC_GPLL0_OUT_EVEN, 6, 0, 0),
>>          F(100000000, P_GCC_GPLL0_OUT_EVEN, 3, 0, 0),
>>          F(202000000, P_GCC_GPLL9_OUT_MAIN, 4, 0, 0),
>>          { }
>> };
>>
>> XO and GPLL0 are more or less always on, but GPLL9 is described to only
>> be used for this specific clock for this specific frequency (perhaps it
>> feeds something else on the soc but that's besides the point).
>>
>> Then, the parent input is changed during set_rate, but GPLL9 seems to
>> never be enabled:
> 
> Is the sdcc2 RCG enabled during the set_rate?

without PARENT_OPS_ENABLE:

[    3.326891] sdhci_msm 8804000.mmc: Got CD GPIO
[    3.336839] scsi host0: ufshcd
[    3.337105] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
[    3.346339] ------------[ cut here ]------------
[    3.351093] gcc_sdcc2_apps_clk_src: rcg didn't update its configuration.
[    3.351114] WARNING: CPU: 1 PID: 11 at drivers/clk/qcom/clk-rcg2.c:133 update_config+0xc8/0xd8

[...]

[    3.610523] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate


with PARENT_OPS_ENABLE:

[    3.331419] sdhci_msm 8804000.mmc: Got CD GPIO
[    3.336569] gcc_sdcc2_apps_clk_src is DISABLED @ set_rate
[    3.344795] scsi host0: ufshcd
[    3.355122] qcrypto 1dfa000.crypto: Adding to iommu group 5
[    3.363567] remoteproc remoteproc0: 2400000.remoteproc is available
[    3.364729] gcc_sdcc2_apps_clk_src is ENABLED @ set_rate

after testing it both ways, I realized it wasn't supposed to make a
difference in this regard, but I suppose I can paste both results anyway..

Konrad