| Message ID | 20180829155046.29359-9-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Cleanup suspend/resume code in Samsung clock drivers | expand |
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(®_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,
>
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
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
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 --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(®_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,
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(-)