[PATCH 00/10] Cleanup suspend/resume code in Samsung clock drivers

[PATCH 00/10] Cleanup suspend/resume code in Samsung clock drivers

[Marek Szyprowski]

Dear All

This patchset consolidates common suspend/resume code in almost all
Samsung clock drivers. This cleanup is a side-effect of a preparation
to add suspend/resume support for Exynos5433 SoCs.

Best regards
Marek Szyprowski


Patch summary:

Marek Szyprowski (10):
  clk: samsung: Remove excessive include
  clk: samsung: s3c2410: Use generic helper for handling suspend/resume
  clk: samsung: s3c2412: Use generic helper for handling suspend/resume
  clk: samsung: s3c2443: Use generic helper for handling suspend/resume
  clk: samsung: s3c64xx: Use generic helper for handling suspend/resume
  clk: samsung: s5pv210: Use generic helper for handling suspend/resume
  clk: samsung: exynos5250: Use generic helper for handling
    suspend/resume
  clk: samsung: Add support for setting registers state before suspend
  clk: samsung: exynos4: Use generic helper for handling suspend/resume
  clk: samsung: exynos5420: Use generic helper for handling
    suspend/resume

 drivers/clk/samsung/clk-exynos-audss.c |   1 -
 drivers/clk/samsung/clk-exynos3250.c   |   1 -
 drivers/clk/samsung/clk-exynos4.c      | 147 +++----------------------
 drivers/clk/samsung/clk-exynos5250.c   |  42 +------
 drivers/clk/samsung/clk-exynos5420.c   |  73 ++----------
 drivers/clk/samsung/clk-s3c2410.c      |  43 +-------
 drivers/clk/samsung/clk-s3c2412.c      |  43 +-------
 drivers/clk/samsung/clk-s3c2443.c      |  43 +-------
 drivers/clk/samsung/clk-s3c64xx.c      |  66 +----------
 drivers/clk/samsung/clk-s5pv210.c      |  41 +------
 drivers/clk/samsung/clk.c              |  23 ++--
 drivers/clk/samsung/clk.h              |  18 ++-
 12 files changed, 68 insertions(+), 473 deletions(-)

-- 
2.17.1

[PATCH 01/10] clk: samsung: Remove excessive include

[Marek Szyprowski]

Exynos Audio SubSystem and Exynos3250 clock drivers don't use any syscore
function, so don't include linux/syscore_ops.h in their code.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos-audss.c | 1 -
 drivers/clk/samsung/clk-exynos3250.c   | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index f659c5cbf1d5..8f8a0f9fc842 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -15,7 +15,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
-#include <linux/syscore_ops.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
diff --git a/drivers/clk/samsung/clk-exynos3250.c b/drivers/clk/samsung/clk-exynos3250.c
index 27c9d23657b3..0e9a41a4cac8 100644
--- a/drivers/clk/samsung/clk-exynos3250.c
+++ b/drivers/clk/samsung/clk-exynos3250.c
@@ -12,7 +12,6 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
-#include <linux/syscore_ops.h>
 
 #include <dt-bindings/clock/exynos3250.h>
 
-- 
2.17.1

Re: [PATCH 01/10] clk: samsung: Remove excessive include

[Chanwoo Choi]

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Exynos Audio SubSystem and Exynos3250 clock drivers don't use any syscore
> function, so don't include linux/syscore_ops.h in their code.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 1 -
>  drivers/clk/samsung/clk-exynos3250.c   | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index f659c5cbf1d5..8f8a0f9fc842 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -15,7 +15,6 @@
>  #include <linux/clk-provider.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> -#include <linux/syscore_ops.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> diff --git a/drivers/clk/samsung/clk-exynos3250.c b/drivers/clk/samsung/clk-exynos3250.c
> index 27c9d23657b3..0e9a41a4cac8 100644
> --- a/drivers/clk/samsung/clk-exynos3250.c
> +++ b/drivers/clk/samsung/clk-exynos3250.c
> @@ -12,7 +12,6 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> -#include <linux/syscore_ops.h>
>  
>  #include <dt-bindings/clock/exynos3250.h>
>  
> 

I tested kernel build. There are no warning and break.

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 01/10] clk: samsung: Remove excessive include

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Exynos Audio SubSystem and Exynos3250 clock drivers don't use any syscore
> function, so don't include linux/syscore_ops.h in their code.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 1 -
>  drivers/clk/samsung/clk-exynos3250.c   | 1 -
>  2 files changed, 2 deletions(-)

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

Best regards,
Krzysztof

[PATCH 02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-s3c2410.c | 43 ++-----------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/samsung/clk-s3c2410.c b/drivers/clk/samsung/clk-s3c2410.c
index a9c887475054..8cb868f06257 100644
--- a/drivers/clk/samsung/clk-s3c2410.c
+++ b/drivers/clk/samsung/clk-s3c2410.c
@@ -11,7 +11,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include <dt-bindings/clock/s3c2410.h>
 
@@ -40,9 +39,6 @@ enum s3c2410_plls {
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s3c2410_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -57,42 +53,6 @@ static unsigned long s3c2410_clk_regs[] __initdata = {
 	CAMDIVN,
 };
 
-static int s3c2410_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s3c2410_save,
-				ARRAY_SIZE(s3c2410_clk_regs));
-
-	return 0;
-}
-
-static void s3c2410_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s3c2410_save,
-				ARRAY_SIZE(s3c2410_clk_regs));
-}
-
-static struct syscore_ops s3c2410_clk_syscore_ops = {
-	.suspend = s3c2410_clk_suspend,
-	.resume = s3c2410_clk_resume,
-};
-
-static void __init s3c2410_clk_sleep_init(void)
-{
-	s3c2410_save = samsung_clk_alloc_reg_dump(s3c2410_clk_regs,
-						ARRAY_SIZE(s3c2410_clk_regs));
-	if (!s3c2410_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	register_syscore_ops(&s3c2410_clk_syscore_ops);
-	return;
-}
-#else
-static void __init s3c2410_clk_sleep_init(void) {}
-#endif
-
 PNAME(fclk_p) = { "mpll", "div_slow" };
 
 static struct samsung_mux_clock s3c2410_common_muxes[] __initdata = {
@@ -461,7 +421,8 @@ void __init s3c2410_common_clk_init(struct device_node *np, unsigned long xti_f,
 			ARRAY_SIZE(s3c244x_common_aliases));
 	}
 
-	s3c2410_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, s3c2410_clk_regs,
+			       ARRAY_SIZE(s3c2410_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 }
-- 
2.17.1

Re: [PATCH 02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

[Chanwoo Choi]

Hi Marek,

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c2410.c | 43 ++-----------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)
> 

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


> diff --git a/drivers/clk/samsung/clk-s3c2410.c b/drivers/clk/samsung/clk-s3c2410.c
> index a9c887475054..8cb868f06257 100644
> --- a/drivers/clk/samsung/clk-s3c2410.c
> +++ b/drivers/clk/samsung/clk-s3c2410.c
> @@ -11,7 +11,6 @@
>  #include <linux/clk-provider.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> -#include <linux/syscore_ops.h>
>  
>  #include <dt-bindings/clock/s3c2410.h>
>  
> @@ -40,9 +39,6 @@ enum s3c2410_plls {
>  
>  static void __iomem *reg_base;
>  
> -#ifdef CONFIG_PM_SLEEP
> -static struct samsung_clk_reg_dump *s3c2410_save;
> -
>  /*
>   * list of controller registers to be saved and restored during a
>   * suspend/resume cycle.
> @@ -57,42 +53,6 @@ static unsigned long s3c2410_clk_regs[] __initdata = {
>  	CAMDIVN,
>  };
>  
> -static int s3c2410_clk_suspend(void)
> -{
> -	samsung_clk_save(reg_base, s3c2410_save,
> -				ARRAY_SIZE(s3c2410_clk_regs));
> -
> -	return 0;
> -}
> -
> -static void s3c2410_clk_resume(void)
> -{
> -	samsung_clk_restore(reg_base, s3c2410_save,
> -				ARRAY_SIZE(s3c2410_clk_regs));
> -}
> -
> -static struct syscore_ops s3c2410_clk_syscore_ops = {
> -	.suspend = s3c2410_clk_suspend,
> -	.resume = s3c2410_clk_resume,
> -};
> -
> -static void __init s3c2410_clk_sleep_init(void)
> -{
> -	s3c2410_save = samsung_clk_alloc_reg_dump(s3c2410_clk_regs,
> -						ARRAY_SIZE(s3c2410_clk_regs));
> -	if (!s3c2410_save) {
> -		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
> -			__func__);
> -		return;
> -	}
> -
> -	register_syscore_ops(&s3c2410_clk_syscore_ops);
> -	return;
> -}
> -#else
> -static void __init s3c2410_clk_sleep_init(void) {}
> -#endif
> -
>  PNAME(fclk_p) = { "mpll", "div_slow" };
>  
>  static struct samsung_mux_clock s3c2410_common_muxes[] __initdata = {
> @@ -461,7 +421,8 @@ void __init s3c2410_common_clk_init(struct device_node *np, unsigned long xti_f,
>  			ARRAY_SIZE(s3c244x_common_aliases));
>  	}
>  
> -	s3c2410_clk_sleep_init();
> +	samsung_clk_sleep_init(reg_base, s3c2410_clk_regs,
> +			       ARRAY_SIZE(s3c2410_clk_regs));
>  
>  	samsung_clk_of_add_provider(np, ctx);
>  }
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.

There is functional change in case alloc fails (kzalloc(),
samsung_clk_alloc_reg_dump()). Previously it would be warned and
ignored. Now it will panic. You could mention this but anyway the
change looks nice! Thanks!

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

Best regards,
Krzysztof

Re: [PATCH 02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Hi Krzysztof,

On 2018-08-30 08:39, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Replace common suspend/resume handling code by generic helper.
>> No functional change.
> There is functional change in case alloc fails (kzalloc(),
> samsung_clk_alloc_reg_dump()). Previously it would be warned and
> ignored. Now it will panic. You could mention this but anyway the
> change looks nice! Thanks!

No problem to replace panic() in samsung_clk_sleep_init() with a pair
of warn and return error as this is probably now a preferred behavior.
This is still truly hypothetical discussion because system, which
fails to allocate memory at boot is useless anyway.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 02/10] clk: samsung: s3c2410: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Thu, 30 Aug 2018 at 13:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 2018-08-30 08:39, Krzysztof Kozlowski wrote:
> > On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> Replace common suspend/resume handling code by generic helper.
> >> No functional change.
> > There is functional change in case alloc fails (kzalloc(),
> > samsung_clk_alloc_reg_dump()). Previously it would be warned and
> > ignored. Now it will panic. You could mention this but anyway the
> > change looks nice! Thanks!
>
> No problem to replace panic() in samsung_clk_sleep_init() with a pair
> of warn and return error as this is probably now a preferred behavior.
> This is still truly hypothetical discussion because system, which
> fails to allocate memory at boot is useless anyway.

I do not mind keeping this as is with small notice in commit msg about
change in case of error path. Indeed it is truly hypothetical case so
there is no point to rewrite samsung_clk_sleep_init for this.

Best regards,
Krzysztof

[PATCH 03/10] clk: samsung: s3c2412: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-s3c2412.c | 43 ++-----------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/samsung/clk-s3c2412.c b/drivers/clk/samsung/clk-s3c2412.c
index 6bc94d3aff78..dd1159050a5a 100644
--- a/drivers/clk/samsung/clk-s3c2412.c
+++ b/drivers/clk/samsung/clk-s3c2412.c
@@ -11,7 +11,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 
 #include <dt-bindings/clock/s3c2412.h>
@@ -29,9 +28,6 @@
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s3c2412_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -45,42 +41,6 @@ static unsigned long s3c2412_clk_regs[] __initdata = {
 	CLKSRC,
 };
 
-static int s3c2412_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s3c2412_save,
-				ARRAY_SIZE(s3c2412_clk_regs));
-
-	return 0;
-}
-
-static void s3c2412_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s3c2412_save,
-				ARRAY_SIZE(s3c2412_clk_regs));
-}
-
-static struct syscore_ops s3c2412_clk_syscore_ops = {
-	.suspend = s3c2412_clk_suspend,
-	.resume = s3c2412_clk_resume,
-};
-
-static void __init s3c2412_clk_sleep_init(void)
-{
-	s3c2412_save = samsung_clk_alloc_reg_dump(s3c2412_clk_regs,
-						ARRAY_SIZE(s3c2412_clk_regs));
-	if (!s3c2412_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	register_syscore_ops(&s3c2412_clk_syscore_ops);
-	return;
-}
-#else
-static void __init s3c2412_clk_sleep_init(void) {}
-#endif
-
 static struct clk_div_table divxti_d[] = {
 	{ .val = 0, .div = 1 },
 	{ .val = 1, .div = 2 },
@@ -278,7 +238,8 @@ void __init s3c2412_common_clk_init(struct device_node *np, unsigned long xti_f,
 	samsung_clk_register_alias(ctx, s3c2412_aliases,
 				   ARRAY_SIZE(s3c2412_aliases));
 
-	s3c2412_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, s3c2412_clk_regs,
+			       ARRAY_SIZE(s3c2412_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 
-- 
2.17.1

Re: [PATCH 03/10] clk: samsung: s3c2412: Use generic helper for handling suspend/resume

[Chanwoo Choi]

Hi Marek,

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c2412.c | 43 ++-----------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 03/10] clk: samsung: s3c2412: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c2412.c | 43 ++-----------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-s3c2412.c b/drivers/clk/samsung/clk-s3c2412.c
> index 6bc94d3aff78..dd1159050a5a 100644
> --- a/drivers/clk/samsung/clk-s3c2412.c
> +++ b/drivers/clk/samsung/clk-s3c2412.c
> @@ -11,7 +11,6 @@
>  #include <linux/clk-provider.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> -#include <linux/syscore_ops.h>
>  #include <linux/reboot.h>
>
>  #include <dt-bindings/clock/s3c2412.h>
> @@ -29,9 +28,6 @@
>
>  static void __iomem *reg_base;
>
> -#ifdef CONFIG_PM_SLEEP
> -static struct samsung_clk_reg_dump *s3c2412_save;
> -
>  /*
>   * list of controller registers to be saved and restored during a
>   * suspend/resume cycle.
> @@ -45,42 +41,6 @@ static unsigned long s3c2412_clk_regs[] __initdata = {
>         CLKSRC,

Did you try building without PM_SLEEP? Now this will be included
always so you might have to mark it __maybe_unused.

Best regards,
Krzysztof

>  };
>
> -static int s3c2412_clk_suspend(void)
> -{
> -       samsung_clk_save(reg_base, s3c2412_save,
> -                               ARRAY_SIZE(s3c2412_clk_regs));
> -
> -       return 0;
> -}
> -
> -static void s3c2412_clk_resume(void)
> -{
> -       samsung_clk_restore(reg_base, s3c2412_save,
> -                               ARRAY_SIZE(s3c2412_clk_regs));
> -}
> -
> -static struct syscore_ops s3c2412_clk_syscore_ops = {
> -       .suspend = s3c2412_clk_suspend,
> -       .resume = s3c2412_clk_resume,
> -};
> -
> -static void __init s3c2412_clk_sleep_init(void)
> -{
> -       s3c2412_save = samsung_clk_alloc_reg_dump(s3c2412_clk_regs,
> -                                               ARRAY_SIZE(s3c2412_clk_regs));
> -       if (!s3c2412_save) {
> -               pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
> -                       __func__);
> -               return;
> -       }
> -
> -       register_syscore_ops(&s3c2412_clk_syscore_ops);
> -       return;
> -}
> -#else
> -static void __init s3c2412_clk_sleep_init(void) {}
> -#endif
> -
>  static struct clk_div_table divxti_d[] = {
>         { .val = 0, .div = 1 },
>         { .val = 1, .div = 2 },
> @@ -278,7 +238,8 @@ void __init s3c2412_common_clk_init(struct device_node *np, unsigned long xti_f,
>         samsung_clk_register_alias(ctx, s3c2412_aliases,
>                                    ARRAY_SIZE(s3c2412_aliases));
>
> -       s3c2412_clk_sleep_init();
> +       samsung_clk_sleep_init(reg_base, s3c2412_clk_regs,
> +                              ARRAY_SIZE(s3c2412_clk_regs));
>
>         samsung_clk_of_add_provider(np, ctx);
>
> --
> 2.17.1
>

Re: [PATCH 03/10] clk: samsung: s3c2412: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Hi Krzysztof,

On 2018-08-30 08:42, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Replace common suspend/resume handling code by generic helper.
>> No functional change.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/clk/samsung/clk-s3c2412.c | 43 ++-----------------------------
>>   1 file changed, 2 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-s3c2412.c b/drivers/clk/samsung/clk-s3c2412.c
>> index 6bc94d3aff78..dd1159050a5a 100644
>> --- a/drivers/clk/samsung/clk-s3c2412.c
>> +++ b/drivers/clk/samsung/clk-s3c2412.c
>> @@ -11,7 +11,6 @@
>>   #include <linux/clk-provider.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>> -#include <linux/syscore_ops.h>
>>   #include <linux/reboot.h>
>>
>>   #include <dt-bindings/clock/s3c2412.h>
>> @@ -29,9 +28,6 @@
>>
>>   static void __iomem *reg_base;
>>
>> -#ifdef CONFIG_PM_SLEEP
>> -static struct samsung_clk_reg_dump *s3c2412_save;
>> -
>>   /*
>>    * list of controller registers to be saved and restored during a
>>    * suspend/resume cycle.
>> @@ -45,42 +41,6 @@ static unsigned long s3c2412_clk_regs[] __initdata = {
>>          CLKSRC,
> Did you try building without PM_SLEEP? Now this will be included
> always so you might have to mark it __maybe_unused.

Nope, there is no such need, because *_regs arrays are passed to a
samsung_clk_sleep_init() noop function not a noop marco.

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 03/10] clk: samsung: s3c2412: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Thu, 30 Aug 2018 at 14:50, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 2018-08-30 08:42, Krzysztof Kozlowski wrote:
> > On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> Replace common suspend/resume handling code by generic helper.
> >> No functional change.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>   drivers/clk/samsung/clk-s3c2412.c | 43 ++-----------------------------
> >>   1 file changed, 2 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk-s3c2412.c b/drivers/clk/samsung/clk-s3c2412.c
> >> index 6bc94d3aff78..dd1159050a5a 100644
> >> --- a/drivers/clk/samsung/clk-s3c2412.c
> >> +++ b/drivers/clk/samsung/clk-s3c2412.c
> >> @@ -11,7 +11,6 @@
> >>   #include <linux/clk-provider.h>
> >>   #include <linux/of.h>
> >>   #include <linux/of_address.h>
> >> -#include <linux/syscore_ops.h>
> >>   #include <linux/reboot.h>
> >>
> >>   #include <dt-bindings/clock/s3c2412.h>
> >> @@ -29,9 +28,6 @@
> >>
> >>   static void __iomem *reg_base;
> >>
> >> -#ifdef CONFIG_PM_SLEEP
> >> -static struct samsung_clk_reg_dump *s3c2412_save;
> >> -
> >>   /*
> >>    * list of controller registers to be saved and restored during a
> >>    * suspend/resume cycle.
> >> @@ -45,42 +41,6 @@ static unsigned long s3c2412_clk_regs[] __initdata = {
> >>          CLKSRC,
> > Did you try building without PM_SLEEP? Now this will be included
> > always so you might have to mark it __maybe_unused.
>
> Nope, there is no such need, because *_regs arrays are passed to a
> samsung_clk_sleep_init() noop function not a noop marco.

Right.

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

Best regards,
Krzysztof

[PATCH 04/10] clk: samsung: s3c2443: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-s3c2443.c | 43 ++-----------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/drivers/clk/samsung/clk-s3c2443.c b/drivers/clk/samsung/clk-s3c2443.c
index c46e6d5bc9bc..884067e4f1a1 100644
--- a/drivers/clk/samsung/clk-s3c2443.c
+++ b/drivers/clk/samsung/clk-s3c2443.c
@@ -11,7 +11,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 
 #include <dt-bindings/clock/s3c2443.h>
@@ -43,9 +42,6 @@ enum supported_socs {
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s3c2443_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -65,42 +61,6 @@ static unsigned long s3c2443_clk_regs[] __initdata = {
 	SCLKCON,
 };
 
-static int s3c2443_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s3c2443_save,
-				ARRAY_SIZE(s3c2443_clk_regs));
-
-	return 0;
-}
-
-static void s3c2443_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s3c2443_save,
-				ARRAY_SIZE(s3c2443_clk_regs));
-}
-
-static struct syscore_ops s3c2443_clk_syscore_ops = {
-	.suspend = s3c2443_clk_suspend,
-	.resume = s3c2443_clk_resume,
-};
-
-static void __init s3c2443_clk_sleep_init(void)
-{
-	s3c2443_save = samsung_clk_alloc_reg_dump(s3c2443_clk_regs,
-						ARRAY_SIZE(s3c2443_clk_regs));
-	if (!s3c2443_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	register_syscore_ops(&s3c2443_clk_syscore_ops);
-	return;
-}
-#else
-static void __init s3c2443_clk_sleep_init(void) {}
-#endif
-
 PNAME(epllref_p) = { "mpllref", "mpllref", "xti", "ext" };
 PNAME(esysclk_p) = { "epllref", "epll" };
 PNAME(mpllref_p) = { "xti", "mdivclk" };
@@ -450,7 +410,8 @@ void __init s3c2443_common_clk_init(struct device_node *np, unsigned long xti_f,
 		break;
 	}
 
-	s3c2443_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, s3c2443_clk_regs,
+			       ARRAY_SIZE(s3c2443_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 
-- 
2.17.1

Re: [PATCH 04/10] clk: samsung: s3c2443: Use generic helper for handling suspend/resume

[Chanwoo Choi]

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c2443.c | 43 ++-----------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 04/10] clk: samsung: s3c2443: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c2443.c | 43 ++-----------------------------
>  1 file changed, 2 insertions(+), 41 deletions(-)
>

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

Best regards,
Krzysztof

[PATCH 05/10] clk: samsung: s3c64xx: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-s3c64xx.c | 66 +++----------------------------
 1 file changed, 6 insertions(+), 60 deletions(-)

diff --git a/drivers/clk/samsung/clk-s3c64xx.c b/drivers/clk/samsung/clk-s3c64xx.c
index 6db01cf5ab83..329a449f78c3 100644
--- a/drivers/clk/samsung/clk-s3c64xx.c
+++ b/drivers/clk/samsung/clk-s3c64xx.c
@@ -12,7 +12,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include <dt-bindings/clock/samsung,s3c64xx-clock.h>
 
@@ -59,10 +58,6 @@
 static void __iomem *reg_base;
 static bool is_s3c6400;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s3c64xx_save_common;
-static struct samsung_clk_reg_dump *s3c64xx_save_soc;
-
 /*
  * List of controller registers to be saved and restored during
  * a suspend/resume cycle.
@@ -89,60 +84,6 @@ static unsigned long s3c6410_clk_regs[] __initdata = {
 	MEM0_GATE,
 };
 
-static int s3c64xx_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s3c64xx_save_common,
-				ARRAY_SIZE(s3c64xx_clk_regs));
-
-	if (!is_s3c6400)
-		samsung_clk_save(reg_base, s3c64xx_save_soc,
-					ARRAY_SIZE(s3c6410_clk_regs));
-
-	return 0;
-}
-
-static void s3c64xx_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s3c64xx_save_common,
-				ARRAY_SIZE(s3c64xx_clk_regs));
-
-	if (!is_s3c6400)
-		samsung_clk_restore(reg_base, s3c64xx_save_soc,
-					ARRAY_SIZE(s3c6410_clk_regs));
-}
-
-static struct syscore_ops s3c64xx_clk_syscore_ops = {
-	.suspend = s3c64xx_clk_suspend,
-	.resume = s3c64xx_clk_resume,
-};
-
-static void __init s3c64xx_clk_sleep_init(void)
-{
-	s3c64xx_save_common = samsung_clk_alloc_reg_dump(s3c64xx_clk_regs,
-						ARRAY_SIZE(s3c64xx_clk_regs));
-	if (!s3c64xx_save_common)
-		goto err_warn;
-
-	if (!is_s3c6400) {
-		s3c64xx_save_soc = samsung_clk_alloc_reg_dump(s3c6410_clk_regs,
-						ARRAY_SIZE(s3c6410_clk_regs));
-		if (!s3c64xx_save_soc)
-			goto err_soc;
-	}
-
-	register_syscore_ops(&s3c64xx_clk_syscore_ops);
-	return;
-
-err_soc:
-	kfree(s3c64xx_save_common);
-err_warn:
-	pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-		__func__);
-}
-#else
-static void __init s3c64xx_clk_sleep_init(void) {}
-#endif
-
 /* List of parent clocks common for all S3C64xx SoCs. */
 PNAME(spi_mmc_p)	= { "mout_epll", "dout_mpll", "fin_pll", "clk27m" };
 PNAME(uart_p)		= { "mout_epll", "dout_mpll" };
@@ -508,7 +449,12 @@ void __init s3c64xx_clk_init(struct device_node *np, unsigned long xtal_f,
 
 	samsung_clk_register_alias(ctx, s3c64xx_clock_aliases,
 					ARRAY_SIZE(s3c64xx_clock_aliases));
-	s3c64xx_clk_sleep_init();
+
+	samsung_clk_sleep_init(reg_base, s3c64xx_clk_regs,
+			       ARRAY_SIZE(s3c64xx_clk_regs));
+        if (!is_s3c6400)
+		samsung_clk_sleep_init(reg_base, s3c6410_clk_regs,
+				       ARRAY_SIZE(s3c6410_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 
-- 
2.17.1

Re: [PATCH 05/10] clk: samsung: s3c64xx: Use generic helper for handling suspend/resume

[Chanwoo Choi]

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c64xx.c | 66 +++----------------------------
>  1 file changed, 6 insertions(+), 60 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 05/10] clk: samsung: s3c64xx: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s3c64xx.c | 66 +++----------------------------
>  1 file changed, 6 insertions(+), 60 deletions(-)

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

Best regards,
Krzysztof

[PATCH 06/10] clk: samsung: s5pv210: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-s5pv210.c | 41 ++-----------------------------
 1 file changed, 2 insertions(+), 39 deletions(-)

diff --git a/drivers/clk/samsung/clk-s5pv210.c b/drivers/clk/samsung/clk-s5pv210.c
index fd2725710a6f..41d2337fe030 100644
--- a/drivers/clk/samsung/clk-s5pv210.c
+++ b/drivers/clk/samsung/clk-s5pv210.c
@@ -14,7 +14,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include "clk.h"
 #include "clk-pll.h"
@@ -83,9 +82,6 @@ enum {
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *s5pv210_clk_dump;
-
 /* List of registers that need to be preserved across suspend/resume. */
 static unsigned long s5pv210_clk_regs[] __initdata = {
 	CLK_SRC0,
@@ -132,40 +128,6 @@ static unsigned long s5pv210_clk_regs[] __initdata = {
 	CLK_OUT,
 };
 
-static int s5pv210_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, s5pv210_clk_dump,
-				ARRAY_SIZE(s5pv210_clk_regs));
-	return 0;
-}
-
-static void s5pv210_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, s5pv210_clk_dump,
-				ARRAY_SIZE(s5pv210_clk_regs));
-}
-
-static struct syscore_ops s5pv210_clk_syscore_ops = {
-	.suspend = s5pv210_clk_suspend,
-	.resume = s5pv210_clk_resume,
-};
-
-static void s5pv210_clk_sleep_init(void)
-{
-	s5pv210_clk_dump =
-		samsung_clk_alloc_reg_dump(s5pv210_clk_regs,
-					   ARRAY_SIZE(s5pv210_clk_regs));
-	if (!s5pv210_clk_dump) {
-		pr_warn("%s: Failed to allocate sleep save data\n", __func__);
-		return;
-	}
-
-	register_syscore_ops(&s5pv210_clk_syscore_ops);
-}
-#else
-static inline void s5pv210_clk_sleep_init(void) { }
-#endif
-
 /* Mux parent lists. */
 static const char *const fin_pll_p[] __initconst = {
 	"xxti",
@@ -822,7 +784,8 @@ static void __init __s5pv210_clk_init(struct device_node *np,
 	samsung_clk_register_alias(ctx, s5pv210_aliases,
 						ARRAY_SIZE(s5pv210_aliases));
 
-	s5pv210_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, s5pv210_clk_regs,
+			       ARRAY_SIZE(s5pv210_clk_regs));
 
 	samsung_clk_of_add_provider(np, ctx);
 
-- 
2.17.1

Re: [PATCH 06/10] clk: samsung: s5pv210: Use generic helper for handling suspend/resume

[Chanwoo Choi]

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s5pv210.c | 41 ++-----------------------------
>  1 file changed, 2 insertions(+), 39 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 06/10] clk: samsung: s5pv210: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-s5pv210.c | 41 ++-----------------------------
>  1 file changed, 2 insertions(+), 39 deletions(-)

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

Best regards,
Krzysztof

[PATCH 07/10] clk: samsung: exynos5250: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos5250.c | 42 ++--------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index 347fd80c351b..f14139bcb0c1 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -14,7 +14,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include "clk.h"
 #include "clk-cpu.h"
@@ -111,9 +110,6 @@ enum exynos5250_plls {
 
 static void __iomem *reg_base;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *exynos5250_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -172,41 +168,6 @@ static const unsigned long exynos5250_clk_regs[] __initconst = {
 	GATE_IP_ISP1,
 };
 
-static int exynos5250_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, exynos5250_save,
-				ARRAY_SIZE(exynos5250_clk_regs));
-
-	return 0;
-}
-
-static void exynos5250_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, exynos5250_save,
-				ARRAY_SIZE(exynos5250_clk_regs));
-}
-
-static struct syscore_ops exynos5250_clk_syscore_ops = {
-	.suspend = exynos5250_clk_suspend,
-	.resume = exynos5250_clk_resume,
-};
-
-static void __init exynos5250_clk_sleep_init(void)
-{
-	exynos5250_save = samsung_clk_alloc_reg_dump(exynos5250_clk_regs,
-					ARRAY_SIZE(exynos5250_clk_regs));
-	if (!exynos5250_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	register_syscore_ops(&exynos5250_clk_syscore_ops);
-}
-#else
-static void __init exynos5250_clk_sleep_init(void) {}
-#endif
-
 /* list of all parent clock list */
 PNAME(mout_apll_p)	= { "fin_pll", "fout_apll", };
 PNAME(mout_cpu_p)	= { "mout_apll", "mout_mpll", };
@@ -882,7 +843,8 @@ static void __init exynos5250_clk_init(struct device_node *np)
 		PWR_CTRL2_CORE2_UP_RATIO | PWR_CTRL2_CORE1_UP_RATIO);
 	__raw_writel(tmp, reg_base + PWR_CTRL2);
 
-	exynos5250_clk_sleep_init();
+	samsung_clk_sleep_init(reg_base, exynos5250_clk_regs,
+			       ARRAY_SIZE(exynos5250_clk_regs));
 	exynos5_subcmus_init(ctx, 1, &exynos5250_disp_subcmu);
 
 	samsung_clk_of_add_provider(np, ctx);
-- 
2.17.1

Re: [PATCH 07/10] clk: samsung: exynos5250: Use generic helper for handling suspend/resume

[Chanwoo Choi]

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> No functional change.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5250.c | 42 ++--------------------------
>  1 file changed, 2 insertions(+), 40 deletions(-)

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 07/10] clk: samsung: exynos5250: Use generic helper for handling suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Replace common suspend/resume handling code by generic helper.
> No functional change.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5250.c | 42 ++--------------------------
>  1 file changed, 2 insertions(+), 40 deletions(-)
>

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

Best regards,
Krzysztof

[PATCH 08/10] clk: samsung: Add support for setting registers state before suspend

[Marek Szyprowski]

Some registers of clock controller have to be set to certain values before
entering system suspend state. Till now drivers did that on their own,
but it will be easier to handle it by generic code and let drivers simply
to provide the list of registers and their state.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk.c | 23 +++++++++++++----------
 drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 8634884aa11c..ab467a7f973a 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
 {
 	struct samsung_clock_reg_cache *reg_cache;
 
-	list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
+	list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
 		samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
 				reg_cache->rd_num);
+		samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
+				reg_cache->rsuspend_num);
+	}
 	return 0;
 }
 
@@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
 	.resume = samsung_clk_resume,
 };
 
-void samsung_clk_sleep_init(void __iomem *reg_base,
+void samsung_clk_sleep_init2(void __iomem *reg_base,
 			const unsigned long *rdump,
-			unsigned long nr_rdump)
+			unsigned long nr_rdump,
+			const struct samsung_clk_reg_dump *rsuspend,
+			unsigned long nr_rsuspend)
 {
 	struct samsung_clock_reg_cache *reg_cache;
 
@@ -330,13 +335,10 @@ void samsung_clk_sleep_init(void __iomem *reg_base,
 
 	reg_cache->reg_base = reg_base;
 	reg_cache->rd_num = nr_rdump;
+	reg_cache->rsuspend = rsuspend;
+	reg_cache->rsuspend_num = nr_rsuspend;
 	list_add_tail(&reg_cache->node, &clock_reg_cache_list);
 }
-
-#else
-void samsung_clk_sleep_init(void __iomem *reg_base,
-			const unsigned long *rdump,
-			unsigned long nr_rdump) {}
 #endif
 
 /*
@@ -380,8 +382,9 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
 		samsung_clk_register_fixed_factor(ctx, cmu->fixed_factor_clks,
 			cmu->nr_fixed_factor_clks);
 	if (cmu->clk_regs)
-		samsung_clk_sleep_init(reg_base, cmu->clk_regs,
-			cmu->nr_clk_regs);
+		samsung_clk_sleep_init2(reg_base,
+				       cmu->clk_regs, cmu->nr_clk_regs,
+				       cmu->suspend_regs, cmu->nr_suspend_regs);
 
 	samsung_clk_of_add_provider(np, ctx);
 
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 3880d2f9d582..854d0b52ef5f 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -279,6 +279,8 @@ struct samsung_clock_reg_cache {
 	void __iomem *reg_base;
 	struct samsung_clk_reg_dump *rdump;
 	unsigned int rd_num;
+	const struct samsung_clk_reg_dump *rsuspend;
+	unsigned int rsuspend_num;
 };
 
 struct samsung_cmu_info {
@@ -358,9 +360,21 @@ extern struct samsung_clk_provider __init *samsung_cmu_register_one(
 
 extern unsigned long _get_rate(const char *clk_name);
 
-extern void samsung_clk_sleep_init(void __iomem *reg_base,
+#ifdef CONFIG_PM_SLEEP
+extern void samsung_clk_sleep_init2(void __iomem *reg_base,
 			const unsigned long *rdump,
-			unsigned long nr_rdump);
+			unsigned long nr_rdump,
+			const struct samsung_clk_reg_dump *rsuspend,
+			unsigned long nr_rsuspend);
+#else
+void samsung_clk_sleep_init2(void __iomem *reg_base,
+			const unsigned long *rdump,
+			unsigned long nr_rdump,
+			const struct samsung_clk_reg_dump *rsuspend,
+			unsigned long nr_rsuspend) {}
+#endif
+#define samsung_clk_sleep_init(reg_base, rdump, nr_rdump) \
+	samsung_clk_sleep_init2(reg_base, rdump, nr_rdump, NULL, 0)
 
 extern void samsung_clk_save(void __iomem *base,
 			struct samsung_clk_reg_dump *rd,
-- 
2.17.1

Re: [PATCH 08/10] clk: samsung: Add support for setting registers state before suspend

[Chanwoo Choi]

Hi Marek,

I don't have any object of this patch.

But, I think that it is not good to make the separate function as following:
- void samsung_clk_sleep_init(...)
- void samsung_clk_sleep_init2(...)

Instead, how about adding new structure like 'struct samsung_cmu_info' as following:?
If we use the structure, we can support it by using only one function.

struct samsung_clk_sleep_info {
	const unsigned long *rdump;
	unsigned long nr_rdump;
	unsigned long nr_rdump;
	const struct samsung_clk_reg_dump *rsuspendl;
	unsigned long nr_rsuspend;
};

void samsung_clk_sleep_init(void __iomem *reg_base,
	const struct samsung_clk_sleep_info *info)

Regards,
Chanwoo Choi

On 2018년 08월 30일 00:50, Marek Szyprowski wrote:
> Some registers of clock controller have to be set to certain values before
> entering system suspend state. Till now drivers did that on their own,
> but it will be easier to handle it by generic code and let drivers simply
> to provide the list of registers and their state.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk.c | 23 +++++++++++++----------
>  drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 8634884aa11c..ab467a7f973a 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
>  {
>  	struct samsung_clock_reg_cache *reg_cache;
>  
> -	list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
> +	list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
>  		samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
>  				reg_cache->rd_num);
> +		samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
> +				reg_cache->rsuspend_num);
> +	}
>  	return 0;
>  }
>  
> @@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
>  	.resume = samsung_clk_resume,
>  };
>  
> -void samsung_clk_sleep_init(void __iomem *reg_base,
> +void samsung_clk_sleep_init2(void __iomem *reg_base,
>  			const unsigned long *rdump,
> -			unsigned long nr_rdump)
> +			unsigned long nr_rdump,
> +			const struct samsung_clk_reg_dump *rsuspend,
> +			unsigned long nr_rsuspend)
>  {
>  	struct samsung_clock_reg_cache *reg_cache;
>  
> @@ -330,13 +335,10 @@ void samsung_clk_sleep_init(void __iomem *reg_base,
>  
>  	reg_cache->reg_base = reg_base;
>  	reg_cache->rd_num = nr_rdump;
> +	reg_cache->rsuspend = rsuspend;
> +	reg_cache->rsuspend_num = nr_rsuspend;
>  	list_add_tail(&reg_cache->node, &clock_reg_cache_list);
>  }
> -
> -#else
> -void samsung_clk_sleep_init(void __iomem *reg_base,
> -			const unsigned long *rdump,
> -			unsigned long nr_rdump) {}
>  #endif
>  
>  /*
> @@ -380,8 +382,9 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>  		samsung_clk_register_fixed_factor(ctx, cmu->fixed_factor_clks,
>  			cmu->nr_fixed_factor_clks);
>  	if (cmu->clk_regs)
> -		samsung_clk_sleep_init(reg_base, cmu->clk_regs,
> -			cmu->nr_clk_regs);
> +		samsung_clk_sleep_init2(reg_base,
> +				       cmu->clk_regs, cmu->nr_clk_regs,
> +				       cmu->suspend_regs, cmu->nr_suspend_regs);
>  
>  	samsung_clk_of_add_provider(np, ctx);
>  
> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index 3880d2f9d582..854d0b52ef5f 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -279,6 +279,8 @@ struct samsung_clock_reg_cache {
>  	void __iomem *reg_base;
>  	struct samsung_clk_reg_dump *rdump;
>  	unsigned int rd_num;
> +	const struct samsung_clk_reg_dump *rsuspend;
> +	unsigned int rsuspend_num;
>  };
>  
>  struct samsung_cmu_info {
> @@ -358,9 +360,21 @@ extern struct samsung_clk_provider __init *samsung_cmu_register_one(
>  
>  extern unsigned long _get_rate(const char *clk_name);
>  
> -extern void samsung_clk_sleep_init(void __iomem *reg_base,
> +#ifdef CONFIG_PM_SLEEP
> +extern void samsung_clk_sleep_init2(void __iomem *reg_base,
>  			const unsigned long *rdump,
> -			unsigned long nr_rdump);
> +			unsigned long nr_rdump,
> +			const struct samsung_clk_reg_dump *rsuspend,
> +			unsigned long nr_rsuspend);
> +#else
> +void samsung_clk_sleep_init2(void __iomem *reg_base,
> +			const unsigned long *rdump,
> +			unsigned long nr_rdump,
> +			const struct samsung_clk_reg_dump *rsuspend,
> +			unsigned long nr_rsuspend) {}
> +#endif
> +#define samsung_clk_sleep_init(reg_base, rdump, nr_rdump) \
> +	samsung_clk_sleep_init2(reg_base, rdump, nr_rdump, NULL, 0)
>  
>  extern void samsung_clk_save(void __iomem *base,
>  			struct samsung_clk_reg_dump *rd,
> 

Re: [PATCH 08/10] clk: samsung: Add support for setting registers state before suspend

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Some registers of clock controller have to be set to certain values before
> entering system suspend state. Till now drivers did that on their own,
> but it will be easier to handle it by generic code and let drivers simply
> to provide the list of registers and their state.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk.c | 23 +++++++++++++----------
>  drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
>  2 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 8634884aa11c..ab467a7f973a 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
>  {
>         struct samsung_clock_reg_cache *reg_cache;
>
> -       list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
> +       list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
>                 samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
>                                 reg_cache->rd_num);
> +               samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
> +                               reg_cache->rsuspend_num);
> +       }
>         return 0;
>  }
>
> @@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
>         .resume = samsung_clk_resume,
>  };
>
> -void samsung_clk_sleep_init(void __iomem *reg_base,
> +void samsung_clk_sleep_init2(void __iomem *reg_base,

Like Chanwoo, I also do not like the init2. Quite frankly, I do not
see what problem you want to solve it by adding "2" suffix - not
touching Exynos5433 code? In such case more meaningful prefix would be
better. But I think this should be avoided especially that in patch
9/10 you use both of them.

Separate topic - It is getting confusing. The existing Exynos5433 code
has support for suspend_regs (used in its device-level runtime PM) and
here you are extending generic code on syscore level. Maybe this could
be unified somehow?

Best regards,
Krzysztof

Re: [PATCH 08/10] clk: samsung: Add support for setting registers state before suspend

[Marek Szyprowski]

Hi Krzysztof,

On 2018-08-31 16:59, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Some registers of clock controller have to be set to certain values before
>> entering system suspend state. Till now drivers did that on their own,
>> but it will be easier to handle it by generic code and let drivers simply
>> to provide the list of registers and their state.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/clk/samsung/clk.c | 23 +++++++++++++----------
>>   drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
>>   2 files changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> index 8634884aa11c..ab467a7f973a 100644
>> --- a/drivers/clk/samsung/clk.c
>> +++ b/drivers/clk/samsung/clk.c
>> @@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
>>   {
>>          struct samsung_clock_reg_cache *reg_cache;
>>
>> -       list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
>> +       list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
>>                  samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
>>                                  reg_cache->rd_num);
>> +               samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
>> +                               reg_cache->rsuspend_num);
>> +       }
>>          return 0;
>>   }
>>
>> @@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
>>          .resume = samsung_clk_resume,
>>   };
>>
>> -void samsung_clk_sleep_init(void __iomem *reg_base,
>> +void samsung_clk_sleep_init2(void __iomem *reg_base,
> Like Chanwoo, I also do not like the init2. Quite frankly, I do not
> see what problem you want to solve it by adding "2" suffix - not
> touching Exynos5433 code?

I didn't want to change Exynos5433 and clock drivers cleaned in patches
2-7, as I see no point adding "NULL, 0" parameters there.

> In such case more meaningful prefix would be
> better. But I think this should be avoided especially that in patch
> 9/10 you use both of them.

Okay, so it is just a matter of name. What about
samsung_clk_extended_sleep_init() ?

I don't want to add another temporary structure just to pass some
arguments to that function...

> Separate topic - It is getting confusing. The existing Exynos5433 code
> has support for suspend_regs (used in its device-level runtime PM) and
> here you are extending generic code on syscore level. Maybe this could
> be unified somehow?

Well, you can consider this patch as a first step of unification.
Implementing device based suspend/resume for OF_CLK_DECLARE() based
drivers is quite complicated now (mainly because that initialization
is done much before platform bus and dt-based devices are registered),
but I hope one day this can be also unified.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 08/10] clk: samsung: Add support for setting registers state before suspend

[Krzysztof Kozlowski]

On Thu, Sep 06, 2018 at 03:25:29PM +0200, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 2018-08-31 16:59, Krzysztof Kozlowski wrote:
> > On Wed, 29 Aug 2018 at 17:51, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> Some registers of clock controller have to be set to certain values before
> >> entering system suspend state. Till now drivers did that on their own,
> >> but it will be easier to handle it by generic code and let drivers simply
> >> to provide the list of registers and their state.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>   drivers/clk/samsung/clk.c | 23 +++++++++++++----------
> >>   drivers/clk/samsung/clk.h | 18 ++++++++++++++++--
> >>   2 files changed, 29 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> >> index 8634884aa11c..ab467a7f973a 100644
> >> --- a/drivers/clk/samsung/clk.c
> >> +++ b/drivers/clk/samsung/clk.c
> >> @@ -290,9 +290,12 @@ static int samsung_clk_suspend(void)
> >>   {
> >>          struct samsung_clock_reg_cache *reg_cache;
> >>
> >> -       list_for_each_entry(reg_cache, &clock_reg_cache_list, node)
> >> +       list_for_each_entry(reg_cache, &clock_reg_cache_list, node) {
> >>                  samsung_clk_save(reg_cache->reg_base, reg_cache->rdump,
> >>                                  reg_cache->rd_num);
> >> +               samsung_clk_restore(reg_cache->reg_base, reg_cache->rsuspend,
> >> +                               reg_cache->rsuspend_num);
> >> +       }
> >>          return 0;
> >>   }
> >>
> >> @@ -310,9 +313,11 @@ static struct syscore_ops samsung_clk_syscore_ops = {
> >>          .resume = samsung_clk_resume,
> >>   };
> >>
> >> -void samsung_clk_sleep_init(void __iomem *reg_base,
> >> +void samsung_clk_sleep_init2(void __iomem *reg_base,
> > Like Chanwoo, I also do not like the init2. Quite frankly, I do not
> > see what problem you want to solve it by adding "2" suffix - not
> > touching Exynos5433 code?
> 
> I didn't want to change Exynos5433 and clock drivers cleaned in patches
> 2-7, as I see no point adding "NULL, 0" parameters there.
> 
> > In such case more meaningful prefix would be
> > better. But I think this should be avoided especially that in patch
> > 9/10 you use both of them.
> 
> Okay, so it is just a matter of name. What about
> samsung_clk_extended_sleep_init() ?
> 
> I don't want to add another temporary structure just to pass some
> arguments to that function...

Yes, I am fine with this approach.

> > Separate topic - It is getting confusing. The existing Exynos5433 code
> > has support for suspend_regs (used in its device-level runtime PM) and
> > here you are extending generic code on syscore level. Maybe this could
> > be unified somehow?
> 
> Well, you can consider this patch as a first step of unification.
> Implementing device based suspend/resume for OF_CLK_DECLARE() based
> drivers is quite complicated now (mainly because that initialization
> is done much before platform bus and dt-based devices are registered),
> but I hope one day this can be also unified.

Indeed... so I will keep my fingers crossed for that work :)

Best regards,
Krzysztof

[PATCH 09/10] clk: samsung: exynos4: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
Handling of PLLs is a bit different in generic code, as they are handled
in the same way as other clock registers. Such approach was already used
on later Exynos SoCs and worked fine. Tests have shown that it works also
on Exynos4 SoCs and significantly simplifies the code.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos4.c | 147 ++++--------------------------
 1 file changed, 16 insertions(+), 131 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index 0421960eb963..d3bd9ffd8a09 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -16,7 +16,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include "clk.h"
 #include "clk-cpu.h"
@@ -157,14 +156,6 @@ enum exynos4_plls {
 static void __iomem *reg_base;
 static enum exynos4_soc exynos4_soc;
 
-/*
- * Support for CMU save/restore across system suspends
- */
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *exynos4_save_common;
-static struct samsung_clk_reg_dump *exynos4_save_soc;
-static struct samsung_clk_reg_dump *exynos4_save_pll;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -192,7 +183,7 @@ static const unsigned long exynos4x12_clk_save[] __initconst = {
 	E4X12_PWR_CTRL2,
 };
 
-static const unsigned long exynos4_clk_pll_regs[] __initconst = {
+static const unsigned long exynos4_clk_regs[] __initconst = {
 	EPLL_LOCK,
 	VPLL_LOCK,
 	EPLL_CON0,
@@ -201,9 +192,6 @@ static const unsigned long exynos4_clk_pll_regs[] __initconst = {
 	VPLL_CON0,
 	VPLL_CON1,
 	VPLL_CON2,
-};
-
-static const unsigned long exynos4_clk_regs[] __initconst = {
 	SRC_LEFTBUS,
 	DIV_LEFTBUS,
 	GATE_IP_LEFTBUS,
@@ -276,6 +264,8 @@ static const unsigned long exynos4_clk_regs[] __initconst = {
 };
 
 static const struct samsung_clk_reg_dump src_mask_suspend[] = {
+	{ .offset = VPLL_CON0,			.value = 0x80600302, },
+	{ .offset = EPLL_CON0,			.value = 0x806F0302, },
 	{ .offset = SRC_MASK_TOP,		.value = 0x00000001, },
 	{ .offset = SRC_MASK_CAM,		.value = 0x11111111, },
 	{ .offset = SRC_MASK_TV,		.value = 0x00000111, },
@@ -291,123 +281,6 @@ static const struct samsung_clk_reg_dump src_mask_suspend_e4210[] = {
 	{ .offset = E4210_SRC_MASK_LCD1,	.value = 0x00001111, },
 };
 
-#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;
-
-	pll_con = readl(reg_base + reg);
-	if (!(pll_con & PLL_ENABLED))
-		return;
-
-	while (!(pll_con & PLL_LOCKED)) {
-		cpu_relax();
-		pll_con = readl(reg_base + reg);
-	}
-}
-
-static int exynos4_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, exynos4_save_common,
-				ARRAY_SIZE(exynos4_clk_regs));
-	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));
-		samsung_clk_restore(reg_base, src_mask_suspend_e4210,
-					ARRAY_SIZE(src_mask_suspend_e4210));
-	} else {
-		samsung_clk_save(reg_base, exynos4_save_soc,
-					ARRAY_SIZE(exynos4x12_clk_save));
-	}
-
-	samsung_clk_restore(reg_base, src_mask_suspend,
-					ARRAY_SIZE(src_mask_suspend));
-
-	return 0;
-}
-
-static void exynos4_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, exynos4_save_pll,
-				ARRAY_SIZE(exynos4_clk_pll_regs));
-
-	exynos4_clk_wait_for_pll(EPLL_CON0);
-	exynos4_clk_wait_for_pll(VPLL_CON0);
-
-	samsung_clk_restore(reg_base, exynos4_save_common,
-				ARRAY_SIZE(exynos4_clk_regs));
-
-	if (exynos4_soc == EXYNOS4210)
-		samsung_clk_restore(reg_base, exynos4_save_soc,
-					ARRAY_SIZE(exynos4210_clk_save));
-	else
-		samsung_clk_restore(reg_base, exynos4_save_soc,
-					ARRAY_SIZE(exynos4x12_clk_save));
-}
-
-static struct syscore_ops exynos4_clk_syscore_ops = {
-	.suspend = exynos4_clk_suspend,
-	.resume = exynos4_clk_resume,
-};
-
-static void __init exynos4_clk_sleep_init(void)
-{
-	exynos4_save_common = samsung_clk_alloc_reg_dump(exynos4_clk_regs,
-					ARRAY_SIZE(exynos4_clk_regs));
-	if (!exynos4_save_common)
-		goto err_warn;
-
-	if (exynos4_soc == EXYNOS4210)
-		exynos4_save_soc = samsung_clk_alloc_reg_dump(
-					exynos4210_clk_save,
-					ARRAY_SIZE(exynos4210_clk_save));
-	else
-		exynos4_save_soc = samsung_clk_alloc_reg_dump(
-					exynos4x12_clk_save,
-					ARRAY_SIZE(exynos4x12_clk_save));
-	if (!exynos4_save_soc)
-		goto err_common;
-
-	exynos4_save_pll = samsung_clk_alloc_reg_dump(exynos4_clk_pll_regs,
-					ARRAY_SIZE(exynos4_clk_pll_regs));
-	if (!exynos4_save_pll)
-		goto err_soc;
-
-	register_syscore_ops(&exynos4_clk_syscore_ops);
-	return;
-
-err_soc:
-	kfree(exynos4_save_soc);
-err_common:
-	kfree(exynos4_save_common);
-err_warn:
-	pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-		__func__);
-}
-#else
-static void __init exynos4_clk_sleep_init(void) {}
-#endif
-
 /* list of all parent clock list */
 PNAME(mout_apll_p)	= { "fin_pll", "fout_apll", };
 PNAME(mout_mpll_p)	= { "fin_pll", "fout_mpll", };
@@ -1532,7 +1405,19 @@ static void __init exynos4_clk_init(struct device_node *np,
 
 	if (soc == EXYNOS4X12)
 		exynos4x12_core_down_clock();
-	exynos4_clk_sleep_init();
+
+	samsung_clk_sleep_init2(reg_base, exynos4_clk_regs,
+			       ARRAY_SIZE(exynos4_clk_regs),
+			       src_mask_suspend,
+			       ARRAY_SIZE(src_mask_suspend));
+	if (exynos4_soc == EXYNOS4210)
+		samsung_clk_sleep_init2(reg_base, exynos4210_clk_save,
+				       ARRAY_SIZE(exynos4210_clk_save),
+				       src_mask_suspend_e4210,
+				       ARRAY_SIZE(src_mask_suspend_e4210));
+	else
+		samsung_clk_sleep_init(reg_base, exynos4x12_clk_save,
+				       ARRAY_SIZE(exynos4x12_clk_save));
 
 	samsung_clk_of_add_provider(np, ctx);
 
-- 
2.17.1

Re: [PATCH 09/10] clk: samsung: exynos4: Use generic helper for handling suspend/resume

[Sylwester Nawrocki]

Hi Marek,

On 08/29/2018 05:50 PM, Marek Szyprowski wrote:
> Replace common suspend/resume handling code by generic helper.
> Handling of PLLs is a bit different in generic code, as they are handled
> in the same way as other clock registers. Such approach was already used
> on later Exynos SoCs and worked fine. Tests have shown that it works also
> on Exynos4 SoCs and significantly simplifies the code.

I was going to ask whether it is safe to drop that PLL state polling code after 
looking at the diff but then I found it all explained in the commit message. 
Thank you for this clean up series, I have applied it to the samsung-clk tree.
Thanks Chanwoo and Krzysztof for review and testing.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/clk/samsung/clk-exynos4.c | 147 ++++--------------------------
>   1 file changed, 16 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index 0421960eb963..d3bd9ffd8a09 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -16,7 +16,6 @@

> -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;
> -
> -	pll_con = readl(reg_base + reg);
> -	if (!(pll_con & PLL_ENABLED))
> -		return;
> -
> -	while (!(pll_con & PLL_LOCKED)) {
> -		cpu_relax();
> -		pll_con = readl(reg_base + reg);
> -	}
> -}
> -
> -static int exynos4_clk_suspend(void)
> -{

> -	exynos4_clk_enable_pll(EPLL_CON0);
> -	exynos4_clk_enable_pll(VPLL_CON0);

> -	return 0;
> -}

--
Regards,
Sylwester

Re: [PATCH 09/10] clk: samsung: exynos4: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Hi Sylwester,

On 2018-09-06 21:49, Sylwester Nawrocki wrote:
> On 08/29/2018 05:50 PM, Marek Szyprowski wrote:
>> Replace common suspend/resume handling code by generic helper.
>> Handling of PLLs is a bit different in generic code, as they are handled
>> in the same way as other clock registers. Such approach was already used
>> on later Exynos SoCs and worked fine. Tests have shown that it works also
>> on Exynos4 SoCs and significantly simplifies the code.
> I was going to ask whether it is safe to drop that PLL state polling code after
> looking at the diff but then I found it all explained in the commit message.
> Thank you for this clean up series, I have applied it to the samsung-clk tree.
> Thanks Chanwoo and Krzysztof for review and testing.

One more note which was lost in v2. I've tested this patch with system
suspend/resume on following Exynos4 boards: Trats (4210), Origen (4210),
OdroidU3 (4412) and Trats2 (4412). In all cases it worked fine.

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>    drivers/clk/samsung/clk-exynos4.c | 147 ++++--------------------------
>>    1 file changed, 16 insertions(+), 131 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
>> index 0421960eb963..d3bd9ffd8a09 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -16,7 +16,6 @@
>> -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;
>> -
>> -	pll_con = readl(reg_base + reg);
>> -	if (!(pll_con & PLL_ENABLED))
>> -		return;
>> -
>> -	while (!(pll_con & PLL_LOCKED)) {
>> -		cpu_relax();
>> -		pll_con = readl(reg_base + reg);
>> -	}
>> -}
>> -
>> -static int exynos4_clk_suspend(void)
>> -{
>> -	exynos4_clk_enable_pll(EPLL_CON0);
>> -	exynos4_clk_enable_pll(VPLL_CON0);
>> -	return 0;
>> -}
> --
> Regards,
> Sylwester
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 10/10] clk: samsung: exynos5420: Use generic helper for handling suspend/resume

[Marek Szyprowski]

Replace common suspend/resume handling code by generic helper.
No functional change.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos5420.c | 73 +++-------------------------
 1 file changed, 7 insertions(+), 66 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 95e1bf69449b..9f2f53d7be70 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -15,7 +15,6 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/syscore_ops.h>
 
 #include "clk.h"
 #include "clk-cpu.h"
@@ -156,10 +155,6 @@ enum exynos5x_plls {
 static void __iomem *reg_base;
 static enum exynos5x_soc exynos5x_soc;
 
-#ifdef CONFIG_PM_SLEEP
-static struct samsung_clk_reg_dump *exynos5x_save;
-static struct samsung_clk_reg_dump *exynos5800_save;
-
 /*
  * list of controller registers to be saved and restored during a
  * suspend/resume cycle.
@@ -283,66 +278,6 @@ static const struct samsung_clk_reg_dump exynos5420_set_clksrc[] = {
 	{ .offset = GATE_IP_PERIC,		.value = 0xffffffff, },
 };
 
-static int exynos5420_clk_suspend(void)
-{
-	samsung_clk_save(reg_base, exynos5x_save,
-				ARRAY_SIZE(exynos5x_clk_regs));
-
-	if (exynos5x_soc == EXYNOS5800)
-		samsung_clk_save(reg_base, exynos5800_save,
-				ARRAY_SIZE(exynos5800_clk_regs));
-
-	samsung_clk_restore(reg_base, exynos5420_set_clksrc,
-				ARRAY_SIZE(exynos5420_set_clksrc));
-
-	return 0;
-}
-
-static void exynos5420_clk_resume(void)
-{
-	samsung_clk_restore(reg_base, exynos5x_save,
-				ARRAY_SIZE(exynos5x_clk_regs));
-
-	if (exynos5x_soc == EXYNOS5800)
-		samsung_clk_restore(reg_base, exynos5800_save,
-				ARRAY_SIZE(exynos5800_clk_regs));
-}
-
-static struct syscore_ops exynos5420_clk_syscore_ops = {
-	.suspend = exynos5420_clk_suspend,
-	.resume = exynos5420_clk_resume,
-};
-
-static void __init exynos5420_clk_sleep_init(void)
-{
-	exynos5x_save = samsung_clk_alloc_reg_dump(exynos5x_clk_regs,
-					ARRAY_SIZE(exynos5x_clk_regs));
-	if (!exynos5x_save) {
-		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-			__func__);
-		return;
-	}
-
-	if (exynos5x_soc == EXYNOS5800) {
-		exynos5800_save =
-			samsung_clk_alloc_reg_dump(exynos5800_clk_regs,
-					ARRAY_SIZE(exynos5800_clk_regs));
-		if (!exynos5800_save)
-			goto err_soc;
-	}
-
-	register_syscore_ops(&exynos5420_clk_syscore_ops);
-	return;
-err_soc:
-	kfree(exynos5x_save);
-	pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
-		__func__);
-	return;
-}
-#else
-static void __init exynos5420_clk_sleep_init(void) {}
-#endif
-
 /* list of all parent clocks */
 PNAME(mout_mspll_cpu_p) = {"mout_sclk_cpll", "mout_sclk_dpll",
 				"mout_sclk_mpll", "mout_sclk_spll"};
@@ -1540,7 +1475,13 @@ static void __init exynos5x_clk_init(struct device_node *np,
 		mout_kfc_p[0], mout_kfc_p[1], 0x28200,
 		exynos5420_kfcclk_d, ARRAY_SIZE(exynos5420_kfcclk_d), 0);
 
-	exynos5420_clk_sleep_init();
+	samsung_clk_sleep_init2(reg_base, exynos5x_clk_regs,
+			       ARRAY_SIZE(exynos5x_clk_regs),
+			       exynos5420_set_clksrc,
+			       ARRAY_SIZE(exynos5420_set_clksrc));
+	if (soc == EXYNOS5800)
+		samsung_clk_sleep_init(reg_base, exynos5800_clk_regs,
+				       ARRAY_SIZE(exynos5800_clk_regs));
 	exynos5_subcmus_init(ctx, ARRAY_SIZE(exynos5x_subcmus),
 			     exynos5x_subcmus);
 
-- 
2.17.1