diff mbox series

[2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

Message ID 20181130065259.26497-3-bjorn.andersson@linaro.org (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: qcom: gcc-msm8998: Fixes and clkref clocks | expand

Commit Message

Bjorn Andersson Nov. 30, 2018, 6:52 a.m. UTC
Drop the halt check of the UFS symbol clocks, in accordance with other
platforms. This makes clk_disable_unused() happy and makes it possible
to turn the clocks on again without an error.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/clk/qcom/gcc-msm8998.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Stephen Boyd Nov. 30, 2018, 7:06 a.m. UTC | #1
Quoting Bjorn Andersson (2018-11-29 22:52:58)
> Drop the halt check of the UFS symbol clocks, in accordance with other
> platforms. This makes clk_disable_unused() happy and makes it possible
> to turn the clocks on again without an error.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Someone was supposed to figure out why we needed to SKIP here instead of
doing things in the proper order. Is anyone attempting to figure that
out?
Bjorn Andersson Nov. 30, 2018, 7:27 a.m. UTC | #2
On Thu 29 Nov 23:06 PST 2018, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2018-11-29 22:52:58)
> > Drop the halt check of the UFS symbol clocks, in accordance with other
> > platforms. This makes clk_disable_unused() happy and makes it possible
> > to turn the clocks on again without an error.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Someone was supposed to figure out why we needed to SKIP here instead of
> doing things in the proper order. Is anyone attempting to figure that
> out?
> 

I'm not aware of any such efforts, but looping in Vivek here.

I would be happy to revert this change in 8996, 8998 and 845 once such
fix arrives. But as this is needed to make progress on getting UFS up on
8998 it would be nice to get it merged for now...

Regards,
Bjorn
Stephen Boyd Nov. 30, 2018, 8:13 a.m. UTC | #3
Quoting Bjorn Andersson (2018-11-29 23:27:25)
> On Thu 29 Nov 23:06 PST 2018, Stephen Boyd wrote:
> 
> > Quoting Bjorn Andersson (2018-11-29 22:52:58)
> > > Drop the halt check of the UFS symbol clocks, in accordance with other
> > > platforms. This makes clk_disable_unused() happy and makes it possible
> > > to turn the clocks on again without an error.
> > > 
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Someone was supposed to figure out why we needed to SKIP here instead of
> > doing things in the proper order. Is anyone attempting to figure that
> > out?
> > 
> 
> I'm not aware of any such efforts, but looping in Vivek here.
> 
> I would be happy to revert this change in 8996, 8998 and 845 once such
> fix arrives. But as this is needed to make progress on getting UFS up on
> 8998 it would be nice to get it merged for now...
> 

That's fine. Just doing my periodic ping on this topic.
Marc Gonzalez Nov. 30, 2018, 10:55 a.m. UTC | #4
On 30/11/2018 07:52, Bjorn Andersson wrote:

> Drop the halt check of the UFS symbol clocks, in accordance with other
> platforms. This makes clk_disable_unused() happy and makes it possible
> to turn the clocks on again without an error.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/clk/qcom/gcc-msm8998.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index d89f8e7c2a59..3d232d266d72 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -2403,7 +2403,7 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
>  
>  static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>  	.halt_reg = 0x75014,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x75014,
>  		.enable_mask = BIT(0),
> @@ -2416,7 +2416,7 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>  
>  static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>  	.halt_reg = 0x7605c,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x7605c,
>  		.enable_mask = BIT(0),
> @@ -2429,7 +2429,7 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>  
>  static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
>  	.halt_reg = 0x75010,
> -	.halt_check = BRANCH_HALT,
> +	.halt_check = BRANCH_HALT_SKIP,
>  	.clkr = {
>  		.enable_reg = 0x75010,
>  		.enable_mask = BIT(0),

FWIW, before applying your patch, the kernel printed the following
warnings at boot:

[    1.820756] gcc_ufs_rx_symbol_1_clk status stuck at 'on'
[    1.992977] gcc_ufs_rx_symbol_0_clk status stuck at 'on'
[    2.165113] gcc_gpu_bimc_gfx_clk status stuck at 'on'

https://lore.kernel.org/linux-clk/f4271471-b60a-f056-b453-1cfa593a0fc4@free.fr/

(NB: gcc_ufs_tx_symbol_0_clk not mentioned)

Your patch makes the two ufs_rx warnings go away.

Regards.
Marc Gonzalez Nov. 30, 2018, 1:08 p.m. UTC | #5
On 30/11/2018 11:55, Marc Gonzalez wrote:

> On 30/11/2018 07:52, Bjorn Andersson wrote:
> 
>> Drop the halt check of the UFS symbol clocks, in accordance with other
>> platforms. This makes clk_disable_unused() happy and makes it possible
>> to turn the clocks on again without an error.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>  drivers/clk/qcom/gcc-msm8998.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
>> index d89f8e7c2a59..3d232d266d72 100644
>> --- a/drivers/clk/qcom/gcc-msm8998.c
>> +++ b/drivers/clk/qcom/gcc-msm8998.c
>> @@ -2403,7 +2403,7 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
>>  
>>  static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>>  	.halt_reg = 0x75014,
>> -	.halt_check = BRANCH_HALT,
>> +	.halt_check = BRANCH_HALT_SKIP,
>>  	.clkr = {
>>  		.enable_reg = 0x75014,
>>  		.enable_mask = BIT(0),
>> @@ -2416,7 +2416,7 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>>  
>>  static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>>  	.halt_reg = 0x7605c,
>> -	.halt_check = BRANCH_HALT,
>> +	.halt_check = BRANCH_HALT_SKIP,
>>  	.clkr = {
>>  		.enable_reg = 0x7605c,
>>  		.enable_mask = BIT(0),
>> @@ -2429,7 +2429,7 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>>  
>>  static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
>>  	.halt_reg = 0x75010,
>> -	.halt_check = BRANCH_HALT,
>> +	.halt_check = BRANCH_HALT_SKIP,
>>  	.clkr = {
>>  		.enable_reg = 0x75010,
>>  		.enable_mask = BIT(0),
> 
> FWIW, before applying your patch, the kernel printed the following
> warnings at boot:
> 
> [    1.820756] gcc_ufs_rx_symbol_1_clk status stuck at 'on'
> [    1.992977] gcc_ufs_rx_symbol_0_clk status stuck at 'on'
> [    2.165113] gcc_gpu_bimc_gfx_clk status stuck at 'on'
> 
> https://lore.kernel.org/linux-clk/f4271471-b60a-f056-b453-1cfa593a0fc4@free.fr/

As far as gcc_gpu_bimc_gfx_clk is concerned, downstream flags it as
"no_halt_check_on_disable", and sets CLKFLAG_RETAIN_MEM.

Regards.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index d89f8e7c2a59..3d232d266d72 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2403,7 +2403,7 @@  static struct clk_branch gcc_ufs_phy_aux_clk = {
 
 static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
 	.halt_reg = 0x75014,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x75014,
 		.enable_mask = BIT(0),
@@ -2416,7 +2416,7 @@  static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
 
 static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
 	.halt_reg = 0x7605c,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x7605c,
 		.enable_mask = BIT(0),
@@ -2429,7 +2429,7 @@  static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
 
 static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
 	.halt_reg = 0x75010,
-	.halt_check = BRANCH_HALT,
+	.halt_check = BRANCH_HALT_SKIP,
 	.clkr = {
 		.enable_reg = 0x75010,
 		.enable_mask = BIT(0),