Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <eba3fc5a-d106-4420-8350-c4a783bc79f9@quicinc.com>
Date:   Wed, 22 Nov 2023 11:39:05 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] clk: qcom: gcc-msm8953: fix stuck gcc_usb30_master_clk
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Stephen Boyd <sboyd@kernel.org>,
        =?UTF-8?B?QmFybmFiw6FzIEN6w6ltw6Fu?= <trabarni@gmail.com>,
        Bjorn Andersson <andersson@kernel.org>
References: <20231002170021.192740-1-trabarni@gmail.com>
 <0eebfc14-dbcd-4987-9e94-ea5630b6c268@linaro.org>
 <07937184481af74c65108bae26526605.sboyd@kernel.org>
 <64a9d171-377c-48df-bdcd-1ac6d13c2da6@linaro.org>
Content-Language: en-US
CC:     <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <64a9d171-377c-48df-bdcd-1ac6d13c2da6@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 11/18/2023 6:18 AM, Konrad Dybcio wrote:
> On 24.10.2023 04:59, Stephen Boyd wrote:
>> Quoting Konrad Dybcio (2023-10-06 16:50:18)
>>> On 2.10.2023 19:00, Barnabás Czémán wrote:
>>>> According to downstream dwc3-msm source this clock has FSM dependency on
>>>> gcc_pcnoc_usb30_clk so enabling it would fail if latter isn't enabled.
>>>> This patch add works around this issue by changing parent of
>>>> gcc_usb30_master_clk to gcc_pcnoc_usb30_clk. This is acceptable because
>>>> both clocks have same parent and are branches/gates.
>>>>
>>>> Signed-off-by: Barnabás Czémán <trabarni@gmail.com>
>>>> ---
>>> "meh"
>>>
>>> There are multiple cases, especially with qcom, where there are some
>>> magic "dependencies" without parent-child relationship. The common
>>> clock framework doesn't currently have any good way to handle this,
>>> other than some mind gymnastics like you had to do here with matching
>>> them against a common parent/ancestor..
>>>
>>> Stephen, what do you say?
>>>
>>
>> You can't change the parent to be not the actual parent. The consumer of
>> the branch probably wants to call clk_set_rate() on the branch and have
>> it propagate up to the parent to set the actual rate. Can the axi clk
>> simply be left enabled all the time? That seems simpler. Otherwise we
>> probably need to leave the axi clk control to the interconnect driver
>> and make sure drivers enable interconnects before enabling this branch.
> Yeah I'm almost inclined to think adding even more ifs to the icc driver
> will consume more power than just leaving the AXI hanging..
> 
>>
>> When things start to get this tangled I tend to think that we need to
>> remove control of the clk from the general drivers and put the logic to
>> control interconnects and clks into some SoC glue driver and expose a
>> single interface, like genpd power_on/power_off so that general drivers
>> can't get the sequence of steps wrong. Instead all they can do is "power
>> on" their device, and the SoC glue driver can do the proper sequence of
>> framework calls to power up the device.
> That too, given the structure of qcom SoCs, it should almost look like:
> 
> xyznoc-bus {
> 	compatible = "simple-pm-bus";
> 	clocks = <&gcc xyznoc_ahb>,
> 		 <&gcc xyznoc_axi>;
> 	...
> 
> 	xyznoc-node@abcd {};
> }
> 
> etc.
> 
> I've actually discussed this with Bjorn, but we came to a conclusion
> that it's not trivial to determine which peripheral lives on which NoC
> + many of them seem to sorta overlap more than one..

Are we seeing the clk getting stuck during suspend/resume or during 
clk_prepare_enable in probe ?

Regards,
Krishna,