linux-kernel - Re: [PATCH v2 3/4] clk: qcom: ipq5332: Use icc-clk for enabling NoC related clocks

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zpt9Jb0aIg96yKN3@hu-varada-blr.qualcomm.com>
Date: Sat, 20 Jul 2024 14:32:29 +0530
From: Varadarajan Narayanan <quic_varada@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <andersson@...nel.org>, <mturquette@...libre.com>, <sboyd@...nel.org>,
        <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <gregkh@...uxfoundation.org>, <konrad.dybcio@...aro.org>,
        <djakov@...nel.org>, <quic_wcheng@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-usb@...r.kernel.org>, <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] clk: qcom: ipq5332: Use icc-clk for enabling NoC
 related clocks

On Thu, Jul 18, 2024 at 01:47:32PM +0300, Dmitry Baryshkov wrote:
> On Thu, 18 Jul 2024 at 13:42, Varadarajan Narayanan
> <quic_varada@...cinc.com> wrote:
> >
> > On Sat, Jul 13, 2024 at 07:21:29PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Jul 11, 2024 at 05:02:38PM GMT, Varadarajan Narayanan wrote:
> > > > Use the icc-clk framework to enable few clocks to be able to
> > > > create paths and use the peripherals connected on those NoCs.
> > > >
> > > > Signed-off-by: Varadarajan Narayanan <quic_varada@...cinc.com>
> > > > ---
> > > >  drivers/clk/qcom/gcc-ipq5332.c | 36 +++++++++++++++++++++++++++++-----
> > > >  1 file changed, 31 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
> > > > index f98591148a97..6d7672cae0f7 100644
> > > > --- a/drivers/clk/qcom/gcc-ipq5332.c
> > > > +++ b/drivers/clk/qcom/gcc-ipq5332.c
> > > > @@ -4,12 +4,14 @@
> > > >   */
> > > >
> > > >  #include <linux/clk-provider.h>
> > > > +#include <linux/interconnect-provider.h>
> > > >  #include <linux/mod_devicetable.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/regmap.h>
> > > >
> > > >  #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > > > +#include <dt-bindings/interconnect/qcom,ipq5332.h>
> > > >
> > > >  #include "clk-alpha-pll.h"
> > > >  #include "clk-branch.h"
> > > > @@ -131,12 +133,14 @@ static struct clk_alpha_pll gpll4_main = {
> > > >                      * (will be added soon), so the clock framework
> > > >                      * disables this source. But some of the clocks
> > > >                      * initialized by boot loaders uses this source. So we
> > > > -                    * need to keep this clock ON. Add the
> > > > -                    * CLK_IGNORE_UNUSED flag so the clock will not be
> > > > -                    * disabled. Once the consumer in kernel is added, we
> > > > -                    * can get rid of this flag.
> > > > +                    * need to keep this clock ON.
> > > > +                    *
> > > > +                    * After initial bootup, when the ICC framework turns
> > > > +                    * off unused paths, as part of the icc-clk dependencies
> > > > +                    * this clock gets disabled resulting in a hang. Marking
> > > > +                    * it as critical to ensure it is not turned off.
> > >
> > > Previous comment was pretty clear: there are missing consumers, the flag
> > > will be removed once they are added. Current comment doesn't make sense.
> > > What is the reason for the device hang if we have all the consumers in
> > > place?
> >
> > Earlier, since there were no consumers for this clock, it got
> > disabled via clk_disable_unused() and CLK_IGNORE_UNUSED helped
> > prevent that.
> >
> > Now, since this clk is getting used indirectly via icc-clk
> > framework, it doesn't qualify for being disabled by
> > clk_disable_unused(). However, when icc_sync_state is called, if
> > it sees there are no consumers for a path and that path gets
> > disabled, the relevant clocks get disabled and eventually this
> > clock also gets disabled. To avoid this have changed 'flags' from
> > CLK_IGNORE_UNUSED -> CLK_IS_CRITICAL.
>
> You don't seem to be answering my question: "What is the reason for
> the device hang if we have all the consumers in place?"
> Could you please answer it rather than describing the CCF / icc-clk behaviour?

Sorry if I hadn't expressed myself clearly. All the consumers are
not there in place yet.

> Are there any actual consumers for GPLL4 at this point? If not, why do
> you want to keep it running? Usually all PLLs are shut down when there
> are no consumers and then restarted when required. This is the
> expected and correct behaviour.

There are consumers for GPLL4, but they are getting disabled by
clk_disable_unused (this is expected). There seems to be some
consumer that got enabled in boot loader itself but not accounted
in Linux because of which we are relying on CLK_IGNORE_UNUSED.

If missing consumer(s) is identified, we can do away with this
flag. Till that is done, was hoping CLK_IS_CRITICAL could help.

Thanks
Varada