diff mbox series

[v0] clk: qcom: gcc: Update disp gpll0 branch for 7180/845

Message ID 1594796050-14511-1-git-send-email-tdas@codeaurora.org (mailing list archive)
State Accepted, archived
Headers show
Series [v0] clk: qcom: gcc: Update disp gpll0 branch for 7180/845 | expand

Commit Message

Taniya Das July 15, 2020, 6:54 a.m. UTC
The display gpll0 branch clock needs to be always left enabled, thus
move the clock ops to _aon branch ops.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/gcc-sc7180.c | 2 +-
 drivers/clk/qcom/gcc-sdm845.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Comments

Stephen Boyd July 21, 2020, 7:45 a.m. UTC | #1
Quoting Taniya Das (2020-07-14 23:54:10)
> The display gpll0 branch clock needs to be always left enabled, thus
> move the clock ops to _aon branch ops.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>

I wanted to apply this but it needs more commit text. Here's the commit
text I wrote.

clk: qcom: gcc: Make disp gpll0 branch aon for sc7180/sdm845

The display gpll0 branch clock inside GCC needs to always be enabled.
Otherwise the AHB clk (disp_cc_mdss_ahb_clk_src) for the display clk
controller (dispcc) will stop clocking while sourcing from gpll0 when
this branch inside GCC is turned off during unused clk disabling. We can
never turn this branch off because the AHB clk for the display subsystem
is needed to read/write any registers inside the display subsystem
including clk related ones. This makes this branch a really easy way to
turn off AHB access to the display subsystem and cause all sorts of
mayhem. Let's just make the clk ops keep the clk enabled forever and
ignore any attempts to disable this clk so that dispcc access keep
working.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Reported-by: Evan Green <evgreen@chromium.org>
Link: https://lore.kernel.org/r/1594796050-14511-1-git-send-email-tdas@codeaurora.org
Fixes: 17269568f726 ("clk: qcom: Add Global Clock controller (GCC) driver for SC7180")
Fixes: 06391eddb60a ("clk: qcom: Add Global Clock controller (GCC) driver for SDM845")

> ---
>  drivers/clk/qcom/gcc-sc7180.c | 2 +-
>  drivers/clk/qcom/gcc-sdm845.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> index ca4383e..538677b 100644
> --- a/drivers/clk/qcom/gcc-sc7180.c
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -1061,7 +1061,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
>                                 .hw = &gpll0.clkr.hw,
>                         },
>                         .num_parents = 1,
> -                       .ops = &clk_branch2_ops,
> +                       .ops = &clk_branch2_aon_ops,

I'm worried that dispcc may probe before GCC and then this branch is
disabled out of boot. Then we'll try to read dispcc registers to
determine clk rates and blow up. I hope that doesn't happen and fixing
it would need this patch plus the usage of PM clks in the dispcc driver.
I suppose if we run into that problem we can use PM clks to make this
branch turn on before accessing any dispcc clks.
Dmitry Baryshkov July 21, 2020, 2:31 p.m. UTC | #2
On 15/07/2020 09:54, Taniya Das wrote:
> The display gpll0 branch clock needs to be always left enabled, thus
> move the clock ops to _aon branch ops.

Does this also apply to sm8250/sm8150?

> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>   drivers/clk/qcom/gcc-sc7180.c | 2 +-
>   drivers/clk/qcom/gcc-sdm845.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> index ca4383e..538677b 100644
> --- a/drivers/clk/qcom/gcc-sc7180.c
> +++ b/drivers/clk/qcom/gcc-sc7180.c
> @@ -1061,7 +1061,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
>   				.hw = &gpll0.clkr.hw,
>   			},
>   			.num_parents = 1,
> -			.ops = &clk_branch2_ops,
> +			.ops = &clk_branch2_aon_ops,
>   		},
>   	},
>   };
> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> index f6ce888..90f7feb 100644
> --- a/drivers/clk/qcom/gcc-sdm845.c
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2018, 2020, The Linux Foundation. All rights reserved.
>    */
> 
>   #include <linux/kernel.h>
> @@ -1344,7 +1344,7 @@ static struct clk_branch gcc_disp_gpll0_clk_src = {
>   				"gpll0",
>   			},
>   			.num_parents = 1,
> -			.ops = &clk_branch2_ops,
> +			.ops = &clk_branch2_aon_ops,
>   		},
>   	},
>   };
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>
Stephen Boyd July 23, 2020, 1:06 a.m. UTC | #3
Quoting Dmitry Baryshkov (2020-07-21 07:31:56)
> On 15/07/2020 09:54, Taniya Das wrote:
> > The display gpll0 branch clock needs to be always left enabled, thus
> > move the clock ops to _aon branch ops.
> 
> Does this also apply to sm8250/sm8150?
> 

Probably.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index ca4383e..538677b 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -1061,7 +1061,7 @@  static struct clk_branch gcc_disp_gpll0_clk_src = {
 				.hw = &gpll0.clkr.hw,
 			},
 			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
 		},
 	},
 };
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
index f6ce888..90f7feb 100644
--- a/drivers/clk/qcom/gcc-sdm845.c
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -1,6 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2018, 2020, The Linux Foundation. All rights reserved.
  */

 #include <linux/kernel.h>
@@ -1344,7 +1344,7 @@  static struct clk_branch gcc_disp_gpll0_clk_src = {
 				"gpll0",
 			},
 			.num_parents = 1,
-			.ops = &clk_branch2_ops,
+			.ops = &clk_branch2_aon_ops,
 		},
 	},
 };