[PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Paweł Chmiel]

This clock must be always enabled to allow access to any registers in
fsys1 CMU. Until proper solution based on runtime PM is applied
(similar to what was done for Exynos5433), mark that clock as critical
so it won't be disabled.

It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
UFS module is probed before pmic used to power that device.
In this case defer probe was happening and that clock was disabled by
UFS driver, causing whole boot to hang on next CMU access.

Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/clk/samsung/clk-exynos7.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
index c1ff715e960c..1048d83f097b 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[] __initconst = {
 		ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
 		CLK_IS_CRITICAL, 0),
 	GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
-		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
+		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
+		CLK_IS_CRITICAL, 0),
 
 	GATE(CLK_SCLK_PHY_FSYS1_26M, "sclk_phy_fsys1_26m",
 		"dout_sclk_phy_fsys1_26m", ENABLE_SCLK_TOP1_FSYS11,
-- 
2.25.1

Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Chanwoo Choi]

Hi Paweł Chmiel,

On 10/25/20 12:43 AM, Paweł Chmiel wrote:
> This clock must be always enabled to allow access to any registers in
> fsys1 CMU. Until proper solution based on runtime PM is applied
> (similar to what was done for Exynos5433), mark that clock as critical
> so it won't be disabled.
> 
> It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
> UFS module is probed before pmic used to power that device.
> In this case defer probe was happening and that clock was disabled by
> UFS driver, causing whole boot to hang on next CMU access.
> 
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  drivers/clk/samsung/clk-exynos7.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
> index c1ff715e960c..1048d83f097b 100644
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[] __initconst = {
>  		ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
>  		CLK_IS_CRITICAL, 0),
>  	GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
> -		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
> +		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
> +		CLK_IS_CRITICAL, 0),

As you commented, in order to keep the always on state,
we can use CLK_IS_CRITICAL. Instead, you can enable the specific clock
with detailed comment on clk-exynos7.c as following merged patches[1][2]:
[1] 67f96ff7c8f0 ("clk: samsung: exynos5420: Keep top G3D clocks enabled")
[2] 0212a0483b0a ("clk: samsung: Keep top BPLL mux on Exynos542x enabled")

The patches[1][2] enable the clock with clk_prepare_enable()
instead of adding CLK_IS_CRITICAL. You can refer to it.

>  
>  	GATE(CLK_SCLK_PHY_FSYS1_26M, "sclk_phy_fsys1_26m",
>  		"dout_sclk_phy_fsys1_26m", ENABLE_SCLK_TOP1_FSYS11,
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Krzysztof Kozlowski]

On Sat, Oct 24, 2020 at 05:43:46PM +0200, Paweł Chmiel wrote:
> This clock must be always enabled to allow access to any registers in
> fsys1 CMU. Until proper solution based on runtime PM is applied
> (similar to what was done for Exynos5433), mark that clock as critical
> so it won't be disabled.
> 
> It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
> UFS module is probed before pmic used to power that device.
> In this case defer probe was happening and that clock was disabled by
> UFS driver, causing whole boot to hang on next CMU access.
> 
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  drivers/clk/samsung/clk-exynos7.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Stephen Boyd]

Not sure why this wasn't picked up in the samsung PR. Can you resend?

> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
> index c1ff715e960c..1048d83f097b 100644
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[] __initconst = {
>                 ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
>                 CLK_IS_CRITICAL, 0),
>         GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
> -               ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
> +               ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
> +               CLK_IS_CRITICAL, 0),
>  

Please put a comment in the code why a clk is critical.

Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Paweł Chmiel]

On 17.12.2020 11:14, Stephen Boyd wrote:
> Not sure why this wasn't picked up in the samsung PR. Can you resend?
Hi
There was v2
(https://patchwork.kernel.org/project/linux-samsung-soc/patch/20201107121456.25562-1-pawel.mikolaj.chmiel@gmail.com/)
but it did receive some request for changes comments and i didn't yet
had time to it.

> 
>> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
>> index c1ff715e960c..1048d83f097b 100644
>> --- a/drivers/clk/samsung/clk-exynos7.c
>> +++ b/drivers/clk/samsung/clk-exynos7.c
>> @@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[] __initconst = {
>>                 ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
>>                 CLK_IS_CRITICAL, 0),
>>         GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
>> -               ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
>> +               ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
>> +               CLK_IS_CRITICAL, 0),
>>  
> 
> Please put a comment in the code why a clk is critica>

Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical

[Sylwester Nawrocki]

On 24.10.2020 17:43, Paweł Chmiel wrote:
> This clock must be always enabled to allow access to any registers in
> fsys1 CMU. Until proper solution based on runtime PM is applied
> (similar to what was done for Exynos5433), mark that clock as critical
> so it won't be disabled.
> 
> It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
> UFS module is probed before pmic used to power that device.
> In this case defer probe was happening and that clock was disabled by
> UFS driver, causing whole boot to hang on next CMU access.
> 
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>   drivers/clk/samsung/clk-exynos7.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos7.c b/drivers/clk/samsung/clk-exynos7.c
> index c1ff715e960c..1048d83f097b 100644
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[] __initconst = {
>   		ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
>   		CLK_IS_CRITICAL, 0),

As this patch can be backported up to the commit that introduced regression
I have applied it instead of your v3, with a comment as below.

+       /*
+        * This clock is required for the CMU_FSYS1 registers access, keep it
+        * enabled permanently until proper runtime PM support is added.
+        */

>   	GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
> -		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
> +		ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
> +		CLK_IS_CRITICAL, 0),
>   
>   	GATE(CLK_SCLK_PHY_FSYS1_26M, "sclk_phy_fsys1_26m",
>   		"dout_sclk_phy_fsys1_26m", ENABLE_SCLK_TOP1_FSYS11,
  
--
Regards,
Sylwester