[PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Danila Tikhonov]

Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
didn't update its configuration" error.

Fixes: 2a1d7eb854bb ("clk: qcom: gcc: Add global clock controller driver for SM8150")
Tested-by: Arseniy Velikanov <adomerlee@gmail.com>
Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
---
 drivers/clk/qcom/gcc-sm8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
index 41ab210875fb..05d115c52dfe 100644
--- a/drivers/clk/qcom/gcc-sm8150.c
+++ b/drivers/clk/qcom/gcc-sm8150.c
@@ -774,7 +774,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
 		.name = "gcc_sdcc2_apps_clk_src",
 		.parent_data = gcc_parents_6,
 		.num_parents = ARRAY_SIZE(gcc_parents_6),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_OPS_PARENT_ENABLE,
 		.ops = &clk_rcg2_floor_ops,
 	},
 };
-- 
2.41.0

Re: [PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Konrad Dybcio]

On 13.09.2023 19:56, Danila Tikhonov wrote:
> Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
> didn't update its configuration" error.
> 
> Fixes: 2a1d7eb854bb ("clk: qcom: gcc: Add global clock controller driver for SM8150")
> Tested-by: Arseniy Velikanov <adomerlee@gmail.com>
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> ---
Would have been nice to mention this is necessary because
the source for the top frequency (GPLL9) is not enabled
by default

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Stephen Boyd]

Quoting Danila Tikhonov (2023-09-13 10:56:11)
> Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
> didn't update its configuration" error.
> 
> Fixes: 2a1d7eb854bb ("clk: qcom: gcc: Add global clock controller driver for SM8150")
> Tested-by: Arseniy Velikanov <adomerlee@gmail.com>
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> ---
>  drivers/clk/qcom/gcc-sm8150.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> index 41ab210875fb..05d115c52dfe 100644
> --- a/drivers/clk/qcom/gcc-sm8150.c
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -774,7 +774,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
>                 .name = "gcc_sdcc2_apps_clk_src",
>                 .parent_data = gcc_parents_6,
>                 .num_parents = ARRAY_SIZE(gcc_parents_6),
> -               .flags = CLK_SET_RATE_PARENT,
> +               .flags = CLK_OPS_PARENT_ENABLE,
>                 .ops = &clk_rcg2_floor_ops,

In what case are we getting the rcg stuck? I thought that you could
write the rcg registers while the parent was off and switch to that
parent if the parent isn't enabled and it wouldn't get stuck.

Re: [PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Konrad Dybcio]

On 14.09.2023 18:20, Stephen Boyd wrote:
> Quoting Danila Tikhonov (2023-09-13 10:56:11)
>> Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
>> didn't update its configuration" error.
>>
>> Fixes: 2a1d7eb854bb ("clk: qcom: gcc: Add global clock controller driver for SM8150")
>> Tested-by: Arseniy Velikanov <adomerlee@gmail.com>
>> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
>> ---
>>  drivers/clk/qcom/gcc-sm8150.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
>> index 41ab210875fb..05d115c52dfe 100644
>> --- a/drivers/clk/qcom/gcc-sm8150.c
>> +++ b/drivers/clk/qcom/gcc-sm8150.c
>> @@ -774,7 +774,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
>>                 .name = "gcc_sdcc2_apps_clk_src",
>>                 .parent_data = gcc_parents_6,
>>                 .num_parents = ARRAY_SIZE(gcc_parents_6),
>> -               .flags = CLK_SET_RATE_PARENT,
>> +               .flags = CLK_OPS_PARENT_ENABLE,
>>                 .ops = &clk_rcg2_floor_ops,
> 
> In what case are we getting the rcg stuck? I thought that you could
> write the rcg registers while the parent was off and switch to that
> parent if the parent isn't enabled and it wouldn't get stuck.
I think the better question here would be "why isn't
OPS_PARENT_ENABLE the default for all qc clocks on all
platforms" :/

Konrad

Re: [PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Bjorn Andersson]

On Wed, 13 Sep 2023 20:56:11 +0300, Danila Tikhonov wrote:
> Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
> didn't update its configuration" error.
> 
> 

Applied, thanks!

[1/1] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src
      commit: 7138c244fb293f24ce8ab782961022eff00a10c4

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH] clk: qcom: gcc-sm8150: Fix gcc_sdcc2_apps_clk_src

[Stephen Boyd]

Quoting Konrad Dybcio (2023-09-15 05:04:41)
> On 14.09.2023 18:20, Stephen Boyd wrote:
> > Quoting Danila Tikhonov (2023-09-13 10:56:11)
> >> Set .flags = CLK_OPS_PARENT_ENABLE to fix "gcc_sdcc2_apps_clk_src: rcg
> >> didn't update its configuration" error.
> >>
> >> Fixes: 2a1d7eb854bb ("clk: qcom: gcc: Add global clock controller driver for SM8150")
> >> Tested-by: Arseniy Velikanov <adomerlee@gmail.com>
> >> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> >> ---
> >>  drivers/clk/qcom/gcc-sm8150.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> >> index 41ab210875fb..05d115c52dfe 100644
> >> --- a/drivers/clk/qcom/gcc-sm8150.c
> >> +++ b/drivers/clk/qcom/gcc-sm8150.c
> >> @@ -774,7 +774,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
> >>                 .name = "gcc_sdcc2_apps_clk_src",
> >>                 .parent_data = gcc_parents_6,
> >>                 .num_parents = ARRAY_SIZE(gcc_parents_6),
> >> -               .flags = CLK_SET_RATE_PARENT,
> >> +               .flags = CLK_OPS_PARENT_ENABLE,
> >>                 .ops = &clk_rcg2_floor_ops,
> > 
> > In what case are we getting the rcg stuck? I thought that you could
> > write the rcg registers while the parent was off and switch to that
> > parent if the parent isn't enabled and it wouldn't get stuck.
> I think the better question here would be "why isn't
> OPS_PARENT_ENABLE the default for all qc clocks on all
> platforms" :/
> 

We don't need that flag because of how the hardware works and how the
clk framework moves the enable of the parent from the old parent to the
new parent when changing rates. The RCGs only get stuck if we change the
parent of an RCG to a disabled parent when the current parent is enabled
and the RCG is enabled. Otherwise we're free to change the parent of the
RCG because it isn't trying to do a glitch free switch of clk frequency.

Is it possible that the clk is running out of boot on a parent that
is enabled in the hardware but doesn't look enabled to the clk framework
because of how we fail to hand off enable state? Maybe the mmc driver
then calls clk_set_rate() to change the rate (rcg is still off) and that
causes problems?