diff mbox

[v2,4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

Message ID 1500526099-9935-5-git-send-email-rnayak@codeaurora.org (mailing list archive)
State Superseded, archived
Delegated to: Andy Gross
Headers show

Commit Message

Rajendra Nayak July 20, 2017, 4:48 a.m. UTC
we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
this can prevent it from being disabled while its unused, it does not
prevent it from being disabled when a child derived from the clock calls
an explicit enable/disable.
Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
so its never disabled.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/gcc-msm8996.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephen Boyd July 27, 2017, 10:51 p.m. UTC | #1
On 07/20, Rajendra Nayak wrote:
> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> this can prevent it from being disabled while its unused, it does not
> prevent it from being disabled when a child derived from the clock calls
> an explicit enable/disable.

Do you mean the config_noc_clk_src is being disabled? Who is
getting this clk? The bus driver? In the downstream sources I
don't even see this clk exposed, so maybe we should just remove
support for it instead.

> Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
> so its never disabled.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gcc-msm8996.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
> index 8abc200..8872985 100644
> --- a/drivers/clk/qcom/gcc-msm8996.c
> +++ b/drivers/clk/qcom/gcc-msm8996.c
> @@ -1333,7 +1333,7 @@ enum {
>  			.name = "gcc_mmss_noc_cfg_ahb_clk",
>  			.parent_names = (const char *[]){ "config_noc_clk_src" },
>  			.num_parents = 1,
> -			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>  			.ops = &clk_branch2_ops,
>  		},
>  	},
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
Rajendra Nayak July 28, 2017, 7:58 a.m. UTC | #2
On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> On 07/20, Rajendra Nayak wrote:
>> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
>> this can prevent it from being disabled while its unused, it does not
>> prevent it from being disabled when a child derived from the clock calls
>> an explicit enable/disable.
> 
> Do you mean the config_noc_clk_src is being disabled? Who is

I think the issue I saw was after I switched the parent from
config_noc_clk_src to the RPM controlled cnoc_clk. That had to
be kept voted, else some other ahb clock derived from it being
disabled was causing the vote on cnoc_clk to be dropped.

> getting this clk? The bus driver? In the downstream sources I
> don't even see this clk exposed, so maybe we should just remove
> support for it instead.
> 
>> Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
>> so its never disabled.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/clk/qcom/gcc-msm8996.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
>> index 8abc200..8872985 100644
>> --- a/drivers/clk/qcom/gcc-msm8996.c
>> +++ b/drivers/clk/qcom/gcc-msm8996.c
>> @@ -1333,7 +1333,7 @@ enum {
>>  			.name = "gcc_mmss_noc_cfg_ahb_clk",
>>  			.parent_names = (const char *[]){ "config_noc_clk_src" },
>>  			.num_parents = 1,
>> -			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> +			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>  			.ops = &clk_branch2_ops,
>>  		},
>>  	},
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>
Stephen Boyd July 28, 2017, 4:40 p.m. UTC | #3
On 07/28, Rajendra Nayak wrote:
> 
> On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> > On 07/20, Rajendra Nayak wrote:
> >> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> >> this can prevent it from being disabled while its unused, it does not
> >> prevent it from being disabled when a child derived from the clock calls
> >> an explicit enable/disable.
> > 
> > Do you mean the config_noc_clk_src is being disabled? Who is
> 
> I think the issue I saw was after I switched the parent from
> config_noc_clk_src to the RPM controlled cnoc_clk. That had to
> be kept voted, else some other ahb clock derived from it being
> disabled was causing the vote on cnoc_clk to be dropped.

Hmm ok. Maybe we have some sort of bus driver vote from the
multimedia clk driver in downstream code? Let me check.
Stephen Boyd July 28, 2017, 5:02 p.m. UTC | #4
On 07/28, Stephen Boyd wrote:
> On 07/28, Rajendra Nayak wrote:
> > 
> > On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> > > On 07/20, Rajendra Nayak wrote:
> > >> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> > >> this can prevent it from being disabled while its unused, it does not
> > >> prevent it from being disabled when a child derived from the clock calls
> > >> an explicit enable/disable.
> > > 
> > > Do you mean the config_noc_clk_src is being disabled? Who is
> > 
> > I think the issue I saw was after I switched the parent from
> > config_noc_clk_src to the RPM controlled cnoc_clk. That had to
> > be kept voted, else some other ahb clock derived from it being
> > disabled was causing the vote on cnoc_clk to be dropped.
> 
> Hmm ok. Maybe we have some sort of bus driver vote from the
> multimedia clk driver in downstream code? Let me check.
> 

Ah I was looking at the mmss file when I should have been looking
at the gcc file. I see it downstream too, where we call
clk_prepare_enable() during gcc boot. This patch looks fine.
Eventually, it would make more sense to have this handled by the
bus driver, but until we get there this is fine.
diff mbox

Patch

diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 8abc200..8872985 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -1333,7 +1333,7 @@  enum {
 			.name = "gcc_mmss_noc_cfg_ahb_clk",
 			.parent_names = (const char *[]){ "config_noc_clk_src" },
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
 			.ops = &clk_branch2_ops,
 		},
 	},