[PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[Marek Szyprowski]

Commit 6edfa11cb396 ("clk: samsung: Add enable/disable operation for
PLL36XX clocks") added enable/disable operations to PLL clocks. Prior that
VPLL and EPPL clocks were always enabled because the enable bit was never
touched. Those clocks have to be enabled during suspend/resume cycle,
because otherwise board fails to enter sleep mode. This patch enables them
unconditionally before entering system suspend state. System restore
function will set them to the previous state saved in the register cache
done before that unconditional enable.

Fixes: 6edfa11cb396 ("clk: samsung: Add enable/disable operation for PLL36XX clocks")
CC: stable@vger.kernel.org # v4.13
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changelog:
v2:
- wait until PLL is properly locked
---
 drivers/clk/samsung/clk-exynos4.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index e40b77583c47..d8d3cb67b402 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -294,6 +294,18 @@ enum exynos4_plls {
 #define PLL_ENABLED	(1 << 31)
 #define PLL_LOCKED	(1 << 29)
 
+static void exynos4_clk_enable_pll(u32 reg)
+{
+	u32 pll_con = readl(reg_base + reg);
+	pll_con |= PLL_ENABLED;
+	writel(pll_con, reg_base + reg);
+
+	while (!(pll_con & PLL_LOCKED)) {
+		cpu_relax();
+		pll_con = readl(reg_base + reg);
+	}
+}
+
 static void exynos4_clk_wait_for_pll(u32 reg)
 {
 	u32 pll_con;
@@ -315,6 +327,9 @@ static int exynos4_clk_suspend(void)
 	samsung_clk_save(reg_base, exynos4_save_pll,
 				ARRAY_SIZE(exynos4_clk_pll_regs));
 
+	exynos4_clk_enable_pll(EPLL_CON0);
+	exynos4_clk_enable_pll(VPLL_CON0);
+
 	if (exynos4_soc == EXYNOS4210) {
 		samsung_clk_save(reg_base, exynos4_save_soc,
 					ARRAY_SIZE(exynos4210_clk_save));
-- 
1.9.1

Re: [PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[Chanwoo Choi]

Hi Marek,

On 2017년 09월 15일 18:41, Marek Szyprowski wrote:
> Commit 6edfa11cb396 ("clk: samsung: Add enable/disable operation for
> PLL36XX clocks") added enable/disable operations to PLL clocks. Prior that
> VPLL and EPPL clocks were always enabled because the enable bit was never
> touched. Those clocks have to be enabled during suspend/resume cycle,
> because otherwise board fails to enter sleep mode. This patch enables them
> unconditionally before entering system suspend state. System restore
> function will set them to the previous state saved in the register cache
> done before that unconditional enable.
> 
> Fixes: 6edfa11cb396 ("clk: samsung: Add enable/disable operation for PLL36XX clocks")
> CC: stable@vger.kernel.org # v4.13
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Changelog:
> v2:
> - wait until PLL is properly locked
> ---
>  drivers/clk/samsung/clk-exynos4.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index e40b77583c47..d8d3cb67b402 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -294,6 +294,18 @@ enum exynos4_plls {
>  #define PLL_ENABLED	(1 << 31)
>  #define PLL_LOCKED	(1 << 29)
>  
> +static void exynos4_clk_enable_pll(u32 reg)
> +{
> +	u32 pll_con = readl(reg_base + reg);
> +	pll_con |= PLL_ENABLED;
> +	writel(pll_con, reg_base + reg);
> +
> +	while (!(pll_con & PLL_LOCKED)) {
> +		cpu_relax();
> +		pll_con = readl(reg_base + reg);
> +	}
> +}
> +
>  static void exynos4_clk_wait_for_pll(u32 reg)
>  {
>  	u32 pll_con;
> @@ -315,6 +327,9 @@ static int exynos4_clk_suspend(void)
>  	samsung_clk_save(reg_base, exynos4_save_pll,
>  				ARRAY_SIZE(exynos4_clk_pll_regs));
>  
> +	exynos4_clk_enable_pll(EPLL_CON0);
> +	exynos4_clk_enable_pll(VPLL_CON0);
> +

To support runtime pm for Exynos5433 on your patch[1],
You get the clock from device-tree and enable the clock
on the suspend function (exynos5433_cmu_suspend) as following:

[1] [PATCH v9 3/5] clk: samsung: exynos5433: Add support for runtime PM
- https://patchwork.kernel.org/patch/9911753/
------------
static int exynos5433_cmu_suspend(struct device *dev)                                                          
{                                                                                                              
	struct exynos5433_cmu_data *data = dev_get_drvdata(dev);                                               
	int i;                                                                                                 
                                                                                                            
	samsung_clk_save(data->ctx->reg_base, data->clk_save,                                                  
				data->nr_clk_save);                                                                   

	for (i = 0; i < data->nr_pclks; i++)                                                                   
		clk_prepare_enable(data->pclks[i]);                                                            

	samsung_clk_restore(data->ctx->reg_base, data->clk_suspend,                                            
				data->nr_clk_suspend);                                                             

	return 0;
}
------------

IMHO, I think that you better to get the PLL clock from device-tree
instead of adding the duplicate code for enabling PLL(EPLL_CON0/VPLL_CON0).
But, Maybe many -EPROBE_DEFER happen on device driver
if using the 'platform_driver' method instead of CLK_OF_DECLARE macro.

I'm not sure what is more important on below:
- Remove the duplicate code
- Prevent the -EPROBE_DEFER

Anyway, I will follow Sylwester's opinion.


>  	if (exynos4_soc == EXYNOS4210) {
>  		samsung_clk_save(reg_base, exynos4_save_soc,
>  					ARRAY_SIZE(exynos4210_clk_save));
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[Chanwoo Choi]

On 2017년 09월 15일 19:10, Chanwoo Choi wrote:
> Hi Marek,
> 
> On 2017년 09월 15일 18:41, Marek Szyprowski wrote:
>> Commit 6edfa11cb396 ("clk: samsung: Add enable/disable operation for
>> PLL36XX clocks") added enable/disable operations to PLL clocks. Prior that
>> VPLL and EPPL clocks were always enabled because the enable bit was never
>> touched. Those clocks have to be enabled during suspend/resume cycle,
>> because otherwise board fails to enter sleep mode. This patch enables them
>> unconditionally before entering system suspend state. System restore
>> function will set them to the previous state saved in the register cache
>> done before that unconditional enable.
>>
>> Fixes: 6edfa11cb396 ("clk: samsung: Add enable/disable operation for PLL36XX clocks")
>> CC: stable@vger.kernel.org # v4.13
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> Changelog:
>> v2:
>> - wait until PLL is properly locked
>> ---
>>  drivers/clk/samsung/clk-exynos4.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
>> index e40b77583c47..d8d3cb67b402 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -294,6 +294,18 @@ enum exynos4_plls {
>>  #define PLL_ENABLED	(1 << 31)
>>  #define PLL_LOCKED	(1 << 29)
>>  
>> +static void exynos4_clk_enable_pll(u32 reg)
>> +{
>> +	u32 pll_con = readl(reg_base + reg);
>> +	pll_con |= PLL_ENABLED;
>> +	writel(pll_con, reg_base + reg);
>> +
>> +	while (!(pll_con & PLL_LOCKED)) {
>> +		cpu_relax();
>> +		pll_con = readl(reg_base + reg);
>> +	}
>> +}
>> +
>>  static void exynos4_clk_wait_for_pll(u32 reg)
>>  {
>>  	u32 pll_con;
>> @@ -315,6 +327,9 @@ static int exynos4_clk_suspend(void)
>>  	samsung_clk_save(reg_base, exynos4_save_pll,
>>  				ARRAY_SIZE(exynos4_clk_pll_regs));
>>  
>> +	exynos4_clk_enable_pll(EPLL_CON0);
>> +	exynos4_clk_enable_pll(VPLL_CON0);
>> +
> 
> To support runtime pm for Exynos5433 on your patch[1],
> You get the clock from device-tree and enable the clock
> on the suspend function (exynos5433_cmu_suspend) as following:
> 
> [1] [PATCH v9 3/5] clk: samsung: exynos5433: Add support for runtime PM
> - https://patchwork.kernel.org/patch/9911753/
> ------------
> static int exynos5433_cmu_suspend(struct device *dev)                                                          
> {                                                                                                              
> 	struct exynos5433_cmu_data *data = dev_get_drvdata(dev);                                               
> 	int i;                                                                                                 
>                                                                                                             
> 	samsung_clk_save(data->ctx->reg_base, data->clk_save,                                                  
> 				data->nr_clk_save);                                                                   
> 
> 	for (i = 0; i < data->nr_pclks; i++)                                                                   
> 		clk_prepare_enable(data->pclks[i]);                                                            
> 
> 	samsung_clk_restore(data->ctx->reg_base, data->clk_suspend,                                            
> 				data->nr_clk_suspend);                                                             
> 
> 	return 0;
> }
> ------------
> 
> IMHO, I think that you better to get the PLL clock from device-tree
> instead of adding the duplicate code for enabling PLL(EPLL_CON0/VPLL_CON0).
> But, Maybe many -EPROBE_DEFER happen on device driver
> if using the 'platform_driver' method instead of CLK_OF_DECLARE macro.
> 
> I'm not sure what is more important on below:
> - Remove the duplicate code
> - Prevent the -EPROBE_DEFER
> 
> Anyway, I will follow Sylwester's opinion.

Ah, it is my mistake. the clk-exynos4.c have to use the CLK_OF_DECLARE macro
in order to support the timer using CLK_MCT.

Please ignore my comment. Feel free to add my reviewed-by tag
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


ditto.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[Krzysztof Kozlowski]

On Fri, Sep 15, 2017 at 11:41 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Commit 6edfa11cb396 ("clk: samsung: Add enable/disable operation for
> PLL36XX clocks") added enable/disable operations to PLL clocks. Prior that
> VPLL and EPPL clocks were always enabled because the enable bit was never
> touched. Those clocks have to be enabled during suspend/resume cycle,
> because otherwise board fails to enter sleep mode. This patch enables them
> unconditionally before entering system suspend state. System restore
> function will set them to the previous state saved in the register cache
> done before that unconditional enable.
>
> Fixes: 6edfa11cb396 ("clk: samsung: Add enable/disable operation for PLL36XX clocks")
> CC: stable@vger.kernel.org # v4.13
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Changelog:
> v2:
> - wait until PLL is properly locked
> ---
>  drivers/clk/samsung/clk-exynos4.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index e40b77583c47..d8d3cb67b402 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -294,6 +294,18 @@ enum exynos4_plls {
>  #define PLL_ENABLED    (1 << 31)
>  #define PLL_LOCKED     (1 << 29)
>
> +static void exynos4_clk_enable_pll(u32 reg)
> +{
> +       u32 pll_con = readl(reg_base + reg);
> +       pll_con |= PLL_ENABLED;
> +       writel(pll_con, reg_base + reg);
> +
> +       while (!(pll_con & PLL_LOCKED)) {
> +               cpu_relax();
> +               pll_con = readl(reg_base + reg);
> +       }

Looks good:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

As a follow up for the fix (separate patch) we could consider adding
timeouts to both busy-loops (waiting on PLL_LOCKED). Also the busy
loop itself could be factored out (less code duplicate). However both
should not be part of this fix.

Best regards,
Krzysztof

Re: [PATCH v2] clk: samsung: exynos4: Enable VPLL and EPLL clocks for suspend/resume cycle

[Sylwester Nawrocki]

On 09/15/2017 11:41 AM, Marek Szyprowski wrote:
> Commit 6edfa11cb396 ("clk: samsung: Add enable/disable operation for
> PLL36XX clocks") added enable/disable operations to PLL clocks. Prior that
> VPLL and EPPL clocks were always enabled because the enable bit was never
> touched. Those clocks have to be enabled during suspend/resume cycle,
> because otherwise board fails to enter sleep mode. This patch enables them
> unconditionally before entering system suspend state. System restore
> function will set them to the previous state saved in the register cache
> done before that unconditional enable.
> 
> Fixes: 6edfa11cb396 ("clk: samsung: Add enable/disable operation for PLL36XX clocks")
> CC:stable@vger.kernel.org  # v4.13
> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thanks, can you please resend so the patchwork catches the patch
and with clk maintainers in Cc?

-- 
Regards,
Sylwester