linux-kernel - Re: [PATCH 2/2] clk: qcom: gcc-sm8550: Don't park the USB RCG at registration time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1db8780a-d7ff-4eb7-b6dd-835327e55d21@oss.qualcomm.com>
Date: Fri, 3 Jan 2025 14:17:09 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson
 <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        patches@...ts.linux.dev, linux-clk@...r.kernel.org,
        Taniya Das <quic_tdas@...cinc.com>,
        Amit Pundir <amit.pundir@...aro.org>
Subject: Re: [PATCH 2/2] clk: qcom: gcc-sm8550: Don't park the USB RCG at
 registration time

On 30.08.2024 7:59 PM, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2024-08-30 05:24:20)
>> On 27.08.2024 8:12 PM, Stephen Boyd wrote:
>>> Quoting Stephen Boyd (2024-08-19 16:36:27)
>>>> Amit Pundir reports that audio and USB-C host mode stops working if the
>>>> gcc_usb30_prim_master_clk_src clk is registered and
>>>> clk_rcg2_shared_init() parks it on XO. Skip parking this clk at
>>>> registration time to fix those issues.
>>>>
>>>> Partially revert commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon
>>>> registration") by skipping the parking bit for this clk, but keep the
>>>> part where we cache the config register. That's still necessary to
>>>> figure out the true parent of the clk at registration time.
>>>>
>>>> Fixes: 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration")
>>>> Fixes: 929c75d57566 ("clk: qcom: gcc-sm8550: Mark RCGs shared where applicable")
>>>> Cc: Konrad Dybcio <konradybcio@...nel.org>
>>>> Cc: Bjorn Andersson <andersson@...nel.org>
>>>> Cc: Taniya Das <quic_tdas@...cinc.com>
>>>> Reported-by: Amit Pundir <amit.pundir@...aro.org>
>>>> Closes: https://lore.kernel.org/CAMi1Hd1KQBE4kKUdAn8E5FV+BiKzuv+8FoyWQrrTHPDoYTuhgA@mail.gmail.com
>>>> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
>>>> ---
>>>>  drivers/clk/qcom/clk-rcg.h    |  1 +
>>>>  drivers/clk/qcom/clk-rcg2.c   | 30 ++++++++++++++++++++++++++++++
>>>>  drivers/clk/qcom/gcc-sm8550.c |  2 +-
>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
>>>> index d7414361e432..8e0f3372dc7a 100644
>>>> --- a/drivers/clk/qcom/clk-rcg.h
>>>> +++ b/drivers/clk/qcom/clk-rcg.h
>>>> @@ -198,6 +198,7 @@ extern const struct clk_ops clk_byte2_ops;
>>>>  extern const struct clk_ops clk_pixel_ops;
>>>>  extern const struct clk_ops clk_gfx3d_ops;
>>>>  extern const struct clk_ops clk_rcg2_shared_ops;
>>>> +extern const struct clk_ops clk_rcg2_shared_no_init_park_ops;
>>>
>>> I'm considering inverting these two rcg2_shared clk_ops so that only a
>>> few clks are parked at clk registration time, to minimize the impact of
>>> commit 01a0a6cc8cfd ("clk: qcom: Park shared RCGs upon registration").
>>> We're up to three or four band-aids, that we can probably wait on
>>> applying if we make all the shared RCGs determine the correct parent at
>>> registration time but skip the parking, except for the display clks on
>>> sc7180 where that exposes another problem with shared parents getting
>>> turned off during probe. It's possible that other SoCs will want to park
>>> their display clks as well to avoid that secondary problem, but it can
>>> be an opt-in case instead of a change to all shared RCGs.
>>
>> Are all cases that need the parking obvious like it was the case on 7180,
>> i.e. some downstream branch is stuck and there's complaining in dmesg?
>>
> 
> I'm under the impression that we need to park the clk when it is shared
> by a remoteproc/firmware or is associated with a GDSC. It seems that on
> older generations of hardware the GDSC would get unstuck eventually, but
> newer generations stay broken and cause all sorts of havoc.

I heard newer GDSCs are funky..

> Note that in my statement earlier in this thread I'm talking about
> parking the clk at registration time. That's done to avoid a problem
> where a shared RCG turns off their parent PLL and another shared RCG is
> also using that PLL but hasn't parked yet. The solution was to park at
> registration time to fix that. It's mostly a workaround for the fact
> that the clk framework doesn't have a good way to track dependencies for
> all the child clks that are enable at registration time which want to
> keep the parent PLL enabled. The problem is that it breaks things like
> USB that has strict frequency requirements for the link.

Should we just do something like .sync_state, where top-level parents
aren't turned off until all clocks have been registered?

Konrad