diff mbox series

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

Message ID 20180829155046.29359-9-m.szyprowski@samsung.com
State Superseded
Headers show
Series Cleanup suspend/resume code in Samsung clock drivers | expand

Commit Message

Marek Szyprowski Aug. 29, 2018, 3:50 p.m. UTC
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(-)

-- 
2.17.1

Comments

Chanwoo Choi Aug. 30, 2018, 1:36 a.m. UTC | #1
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,

>
Krzysztof Kozlowski Aug. 31, 2018, 2:59 p.m. UTC | #2
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
Marek Szyprowski Sept. 6, 2018, 1:25 p.m. UTC | #3
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
Krzysztof Kozlowski Sept. 6, 2018, 4:15 p.m. UTC | #4
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
diff mbox series

Patch

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,