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@xxxxxxxxxxx>
> > > >
> > > > 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
