linux-kernel - Re: [PATCH 06/14] clk: qcom: clk-branch: Add support for BRANCH_HALT

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: <gmw3fkvtcoorsdqxdqgr4ioew6wuvtuuvy44qv6fpyrs6wgbv3@jqyayktyxfvn>
Date: Fri, 18 Oct 2024 14:12:11 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Taniya Das <quic_tdas@...cinc.com>
Cc: Stephen Boyd <sboyd@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>, linux-arm-msm@...r.kernel.org, 
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Kalpak Kawadkar <quic_kkawadka@...cinc.com>
Subject: Re: [PATCH 06/14] clk: qcom: clk-branch: Add support for
 BRANCH_HALT_POLL flag

On Fri, Oct 18, 2024 at 04:32:47PM +0530, Taniya Das wrote:
> 
> 
> On 10/18/2024 3:35 AM, Dmitry Baryshkov wrote:
> > On Thu, Oct 17, 2024 at 11:03:20AM -0700, Stephen Boyd wrote:
> > > Quoting Dmitry Baryshkov (2024-10-17 09:56:56)
> > > > From: Kalpak Kawadkar <quic_kkawadka@...cinc.com>
> > > > 
> > > > On some platforms branch clock will be enabled before Linux.
> > > > It is expectated from the clock provider is to poll on the clock
> > > 
> > > Unfortunately 'expectated' is not a word. The sentence is also
> > > grammatically incorrect.
> > > 
> > > > to ensure it is indeed enabled and not HW gated, thus add
> > > > the BRANCH_HALT_POLL flag.
> > > [...]
> > > > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c
> > > > index 229480c5b075a0e70dc05b1cb15b88d29fd475ce..c4c7bd565cc9a3926e24bb12ed6355ec6ddd19fb 100644
> > > > --- a/drivers/clk/qcom/clk-branch.c
> > > > +++ b/drivers/clk/qcom/clk-branch.c
> > > > @@ -76,6 +76,7 @@ static int clk_branch_wait(const struct clk_branch *br, bool enabling,
> > > >                  udelay(10);
> > > >          } else if (br->halt_check == BRANCH_HALT_ENABLE ||
> > > >                     br->halt_check == BRANCH_HALT ||
> > > > +                  br->halt_check == BRANCH_HALT_POLL ||
> > > 
> > > The name is confusing. The halt check is already "polling", i.e. this
> > > isn't a different type of halt check. This is really something like
> > > another branch flag that doesn't have to do with the halt checking and
> > > only to do with skipping writing the enable bit. Maybe we should
> > > introduce another clk_ops for these types of clks instead.
> > 
> > SGTM. All clocks with this flag use clk_branch2_aon_ops, so it is easy
> > to switch to a separate clk_ops.
> > 
> 
> The basic requirement here is to just poll in both enable/disable, but HLOS
> cannot update the CLK_ENABLE bit. The clock could be gated by the bandwidth
> vote and thus to ensure the clock is in good state before the consumers
> start using the subsystem.
> 
> We can definitely think for a different ops, I think it is better we have a
> good name to the flag.

Granted that we end up using just clk_branch_wait() in enable and
clk_is_enabled_regmap() in .is_enabled, I think that separate ops might
make sense. Anyway, as this concerns only 4 camera clocks, I can drop
this patch for now.

> 
> > > 
> > > >                     (enabling && voted)) {
> > > >                  int count = 200;
> > > > @@ -97,6 +98,10 @@ static int clk_branch_toggle(struct clk_hw *hw, bool en,
> > > >          struct clk_branch *br = to_clk_branch(hw);
> > > >          int ret;
> > > > +       if (br->halt_check == BRANCH_HALT_POLL) {
> > > 
> > > Remove braces
> > > 
> > > > +               return  clk_branch_wait(br, en, check_halt);
> > > 
> > > Remove extra space      ^
> > > 
> > > > +       }
> > > > +
> > 
> 
> -- 
> Thanks & Regards,
> Taniya Das.

-- 
With best wishes
Dmitry