diff mbox series

[06/14] clk: qcom: clk-branch: Add support for BRANCH_HALT_POLL flag

Message ID 20241017-sar2130p-clocks-v1-6-f75e740f0a8d@linaro.org (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: qcom: add support for clock controllers on the SAR2130P platform | expand

Commit Message

Dmitry Baryshkov Oct. 17, 2024, 4:56 p.m. UTC 
From: Kalpak Kawadkar <quic_kkawadka@quicinc.com>

On some platforms branch clock will be enabled before Linux.
It is expectated from the clock provider is to poll on the clock
to ensure it is indeed enabled and not HW gated, thus add
the BRANCH_HALT_POLL flag.

Signed-off-by: Kalpak Kawadkar <quic_kkawadka@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-branch.c | 7 ++++++-
 drivers/clk/qcom/clk-branch.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Stephen Boyd Oct. 17, 2024, 6:03 p.m. UTC | #1 
Quoting Dmitry Baryshkov (2024-10-17 09:56:56)
> From: Kalpak Kawadkar <quic_kkawadka@quicinc.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.

>                    (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      ^

> +       }
> +
Dmitry Baryshkov Oct. 17, 2024, 10:05 p.m. UTC | #2 
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@quicinc.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.

> 
> >                    (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      ^
> 
> > +       }
> > +
Taniya Das Oct. 18, 2024, 11:02 a.m. UTC | #3 
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@quicinc.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.

>>
>>>                     (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      ^
>>
>>> +       }
>>> +
>
Dmitry Baryshkov Oct. 18, 2024, 11:12 a.m. UTC | #4 
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@quicinc.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.
diff mbox series

Patch

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
@@ -1,7 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2013, The Linux Foundation. All rights reserved.
- * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2022, 2023 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <linux/kernel.h>
@@ -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 ||
 		   (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) {
+		return  clk_branch_wait(br, en, check_halt);
+	}
+
 	if (en) {
 		ret = clk_enable_regmap(hw);
 		if (ret)
diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h
index 292756435f53648640717734af198442a315272e..47bf59a671c3c8516a57c283fce548a6e5f16619 100644
--- a/drivers/clk/qcom/clk-branch.h
+++ b/drivers/clk/qcom/clk-branch.h
@@ -34,6 +34,7 @@  struct clk_branch {
 #define BRANCH_HALT_ENABLE_VOTED	(BRANCH_HALT_ENABLE | BRANCH_VOTED)
 #define BRANCH_HALT_DELAY		2 /* No bit to check; just delay */
 #define BRANCH_HALT_SKIP		3 /* Don't check halt bit */
+#define BRANCH_HALT_POLL		4 /* Don't enable the clock, poll for halt */
 
 	struct clk_regmap clkr;
 };