[PATCH 0/2] Prepare Exynos5433 clocks driver for system suspend/resume

[PATCH 0/2] Prepare Exynos5433 clocks driver for system suspend/resume

[Marek Szyprowski]

Dear All

This patchset adds remaining bits to Exynos5433 clocks driver needed
to make system suspend/resume working without marking dozen of clocks as
'critical'. It must be applied on top of 'Cleanup suspend/resume code in
Samsung clock drivers' patchset sent a few minutes ago, otherwise
CLK_OF_DECLARE part of the clocks driver will not handle
samsung_cmu_info->suspend_regs entries.

Best regards
Marek Szyprowski


Patch summary:

Marek Szyprowski (2):
  clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume
  clk: samsung: exynos5433: Add suspend state for TOP, CPIF & PERIC CMUs

 drivers/clk/samsung/clk-exynos5433.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Marek Szyprowski]

Clocks should be suspended as late as possible and resumed as early as
possible to let other drivers do their own suspend/resume tasks. NOIRQ
callbacks better suit this requirement.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos5433.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 162de44df099..426980514e67 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -5630,7 +5630,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {
 static const struct dev_pm_ops exynos5433_cmu_pm_ops = {
 	SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,
 			   NULL)
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				     pm_runtime_force_resume)
 };
 
-- 
2.17.1

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Chanwoo Choi]

Hi,

On 2018년 08월 30일 01:00, Marek Szyprowski wrote:
> Clocks should be suspended as late as possible and resumed as early as
> possible to let other drivers do their own suspend/resume tasks. NOIRQ
> callbacks better suit this requirement.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index 162de44df099..426980514e67 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -5630,7 +5630,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {
>  static const struct dev_pm_ops exynos5433_cmu_pm_ops = {
>  	SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,
>  			   NULL)
> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>  				     pm_runtime_force_resume)
>  };
>  
> 

The suspend_noirq callback for devices are called right before
entering the suspend and then resume_noirq callback for device
are called right after wakeup from suspend.

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Clocks should be suspended as late as possible and resumed as early as
> possible to let other drivers do their own suspend/resume tasks. NOIRQ
> callbacks better suit this requirement.

I think that's not a good reason to use the noirq versions. These are
to solve the races with interrupt handlers, not to manually order
callbacks.

Best regards,
Krzysztof


> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index 162de44df099..426980514e67 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -5630,7 +5630,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {
>  static const struct dev_pm_ops exynos5433_cmu_pm_ops = {
>         SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,
>                            NULL)
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>                                      pm_runtime_force_resume)
>  };
>
> --
> 2.17.1
>

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Marek Szyprowski]

Hi Krzysztof,

On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
> On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Clocks should be suspended as late as possible and resumed as early as
>> possible to let other drivers do their own suspend/resume tasks. NOIRQ
>> callbacks better suit this requirement.
> I think that's not a good reason to use the noirq versions. These are
> to solve the races with interrupt handlers, not to manually order
> callbacks.

Then please tell me which other solution should I use to make clock 
available
on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
access its clocks in NOIRQ resume.

 > ...

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

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Krzysztof Kozlowski]

On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
> > On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> Clocks should be suspended as late as possible and resumed as early as
> >> possible to let other drivers do their own suspend/resume tasks. NOIRQ
> >> callbacks better suit this requirement.
> > I think that's not a good reason to use the noirq versions. These are
> > to solve the races with interrupt handlers, not to manually order
> > callbacks.
>
> Then please tell me which other solution should I use to make clock
> available
> on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
> access its clocks in NOIRQ resume.

Indeed I found the usage of noirq in the dw-mmc driver which made me
wondering why it is there... and if you look at explanation, the noirq
is only for the purpose of clearing wakeup interrupt in CLKSEL
register.

Further code refactoring moved more and more code to suspend_noirq,
including the runtime PM part. This probably should not be part of
suspend_noirq but regular suspend. Then all the need of manual
ordering would go away. So the answer to your question - try fixing
buggy dw-mmc driver. :)

Best regards,
Krzysztof

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Marek Szyprowski]

Hi Krzysztof,

On 2018-08-30 12:25, Krzysztof Kozlowski wrote:
> On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
>>> On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>>> Clocks should be suspended as late as possible and resumed as early as
>>>> possible to let other drivers do their own suspend/resume tasks. NOIRQ
>>>> callbacks better suit this requirement.
>>> I think that's not a good reason to use the noirq versions. These are
>>> to solve the races with interrupt handlers, not to manually order
>>> callbacks.
>> Then please tell me which other solution should I use to make clock
>> available
>> on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
>> access its clocks in NOIRQ resume.
> Indeed I found the usage of noirq in the dw-mmc driver which made me
> wondering why it is there... and if you look at explanation, the noirq
> is only for the purpose of clearing wakeup interrupt in CLKSEL
> register.
>
> Further code refactoring moved more and more code to suspend_noirq,
> including the runtime PM part. This probably should not be part of
> suspend_noirq but regular suspend. Then all the need of manual
> ordering would go away. So the answer to your question - try fixing
> buggy dw-mmc driver. :)

It is not the bug in dw-mmc driver. It clearly needs to access some its
registers during NOIRQ phase. However accessing registers requires to
have clocks enabled, which is being handled by runtime PM. There is
nothing broken here.

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

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Krzysztof Kozlowski]

On Thu, 30 Aug 2018 at 12:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Hi Krzysztof,
>
> On 2018-08-30 12:25, Krzysztof Kozlowski wrote:
> > On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
> >>> On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >>>> Clocks should be suspended as late as possible and resumed as early as
> >>>> possible to let other drivers do their own suspend/resume tasks. NOIRQ
> >>>> callbacks better suit this requirement.
> >>> I think that's not a good reason to use the noirq versions. These are
> >>> to solve the races with interrupt handlers, not to manually order
> >>> callbacks.
> >> Then please tell me which other solution should I use to make clock
> >> available
> >> on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
> >> access its clocks in NOIRQ resume.
> > Indeed I found the usage of noirq in the dw-mmc driver which made me
> > wondering why it is there... and if you look at explanation, the noirq
> > is only for the purpose of clearing wakeup interrupt in CLKSEL
> > register.
> >
> > Further code refactoring moved more and more code to suspend_noirq,
> > including the runtime PM part. This probably should not be part of
> > suspend_noirq but regular suspend. Then all the need of manual
> > ordering would go away. So the answer to your question - try fixing
> > buggy dw-mmc driver. :)
>
> It is not the bug in dw-mmc driver. It clearly needs to access some its
> registers during NOIRQ phase. However accessing registers requires to
> have clocks enabled, which is being handled by runtime PM. There is
> nothing broken here.

Ah I missed that point and I see you fixed that case in "mmc:
dw_mmc-exynos: fix potential external abort in resume_noirq()". The
true reasoning for this patch is that soc clk driver should suspend
after every other suspend callback which is using clocks... It would
be nice to explain this particular scenario in commit msg.

However both dw-mmc and clk will be now in suspend noirq phase so do
you have any guarantees that dw-mmc will be suspended after clk?

Best regards,
Krzysztof

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Marek Szyprowski]

On 2018-08-30 12:45, Krzysztof Kozlowski wrote:
> On Thu, 30 Aug 2018 at 12:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Hi Krzysztof,
>>
>> On 2018-08-30 12:25, Krzysztof Kozlowski wrote:
>>> On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>>> On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
>>>>> On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>>>>> Clocks should be suspended as late as possible and resumed as early as
>>>>>> possible to let other drivers do their own suspend/resume tasks. NOIRQ
>>>>>> callbacks better suit this requirement.
>>>>> I think that's not a good reason to use the noirq versions. These are
>>>>> to solve the races with interrupt handlers, not to manually order
>>>>> callbacks.
>>>> Then please tell me which other solution should I use to make clock
>>>> available
>>>> on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
>>>> access its clocks in NOIRQ resume.
>>> Indeed I found the usage of noirq in the dw-mmc driver which made me
>>> wondering why it is there... and if you look at explanation, the noirq
>>> is only for the purpose of clearing wakeup interrupt in CLKSEL
>>> register.
>>>
>>> Further code refactoring moved more and more code to suspend_noirq,
>>> including the runtime PM part. This probably should not be part of
>>> suspend_noirq but regular suspend. Then all the need of manual
>>> ordering would go away. So the answer to your question - try fixing
>>> buggy dw-mmc driver. :)
>> It is not the bug in dw-mmc driver. It clearly needs to access some its
>> registers during NOIRQ phase. However accessing registers requires to
>> have clocks enabled, which is being handled by runtime PM. There is
>> nothing broken here.
> Ah I missed that point and I see you fixed that case in "mmc:
> dw_mmc-exynos: fix potential external abort in resume_noirq()". The
> true reasoning for this patch is that soc clk driver should suspend
> after every other suspend callback which is using clocks... It would
> be nice to explain this particular scenario in commit msg.
>
> However both dw-mmc and clk will be now in suspend noirq phase so do
> you have any guarantees that dw-mmc will be suspended after clk?

Frankly speaking this works now, because the devices are populated in
the order of their presence in device-tree. When the order would be
reverse, dw-mmc driver will defer probe until clocks are registered.
This would be enough to ensure proper suspend/resume order, because all
deferred devices are moved to the end of dpm_list (the list of device
used for system suspend/resume calls), see deferred_probe_work_func()
and comments there.

This is however still not fully resolved problem that has to be
addressed one day. Andrzej Hajda will have a speech on this topic at
Open Source Summit:

https://osseu18.sched.com/event/FxYd/deferred-problem-issues-with-complex-dependencies-between-devices-in-linux-kernel-andrzej-hajda-samsung

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

Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Krzysztof Kozlowski]

On Thu, 30 Aug 2018 at 13:12, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
>
>
> On 2018-08-30 12:45, Krzysztof Kozlowski wrote:
> > Ah I missed that point and I see you fixed that case in "mmc:
> > dw_mmc-exynos: fix potential external abort in resume_noirq()". The
> > true reasoning for this patch is that soc clk driver should suspend
> > after every other suspend callback which is using clocks... It would
> > be nice to explain this particular scenario in commit msg.
> >
> > However both dw-mmc and clk will be now in suspend noirq phase so do
> > you have any guarantees that dw-mmc will be suspended after clk?
>
> Frankly speaking this works now, because the devices are populated in
> the order of their presence in device-tree. When the order would be
> reverse, dw-mmc driver will defer probe until clocks are registered.
> This would be enough to ensure proper suspend/resume order, because all
> deferred devices are moved to the end of dpm_list (the list of device
> used for system suspend/resume calls), see deferred_probe_work_func()
> and comments there.

Looks quite fragile... but I understand now that your patch is a
logical solution for existing infrastructure with existing dw-mmc
behavior during suspend.

I wonder whether the device links should be used here (in case of
dw-mmc and clk drivers) and in all clk consumers in general. Something
similar to regulators_get().

Can you extend the commit msg with this clk--dw_mmc scenario?

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

Best regards,
Krzysztof

[PATCH 2/2] clk: samsung: exynos5433: Add suspend state for TOP, CPIF & PERIC CMUs

[Marek Szyprowski]

Before entering system suspend, one has to ensure that all TOP ACLK clocks
are enabled, so do this by storing 0x67ECFFED value to ENABLE_ACLK_TOP
register. In case of PERIC CMU, keep all UART PCLK and SCLK clocks as well
as all GPIO PCLK clocks enabled.

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

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 426980514e67..d34e645aba49 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -177,6 +177,12 @@ static const unsigned long top_clk_regs[] __initconst = {
 	ENABLE_CMU_TOP_DIV_STAT,
 };
 
+static const struct samsung_clk_reg_dump top_suspend_regs[] = {
+	{ ENABLE_ACLK_TOP, 0x67ECFFED }, /* all clocks enabled */
+	{ ISP_PLL_CON0, 0x85CC0502 },	/* reset value + ENABLE bit */
+	{ AUD_PLL_CON0, 0x84830202 },	/* reset value + ENABLE bit */
+};
+
 /* list of all parent clock list */
 PNAME(mout_aud_pll_p)		= { "oscclk", "fout_aud_pll", };
 PNAME(mout_isp_pll_p)		= { "oscclk", "fout_isp_pll", };
@@ -792,6 +798,8 @@ static const struct samsung_cmu_info top_cmu_info __initconst = {
 	.nr_clk_ids		= TOP_NR_CLK,
 	.clk_regs		= top_clk_regs,
 	.nr_clk_regs		= ARRAY_SIZE(top_clk_regs),
+	.suspend_regs		= top_suspend_regs,
+	.nr_suspend_regs	= ARRAY_SIZE(top_suspend_regs),
 };
 
 static void __init exynos5433_cmu_top_init(struct device_node *np)
@@ -822,6 +830,11 @@ static const unsigned long cpif_clk_regs[] __initconst = {
 	ENABLE_SCLK_CPIF,
 };
 
+static const struct samsung_clk_reg_dump cpif_suspend_regs[] = {
+	{ ENABLE_SCLK_CPIF, 0x3FF },	/* all clocks enabled */
+	{ MPHY_PLL_CON0, 0x81C70601 },	/* reset value + ENABLE bit */
+};
+
 /* list of all parent clock list */
 PNAME(mout_mphy_pll_p)		= { "oscclk", "fout_mphy_pll", };
 
@@ -862,6 +875,8 @@ static const struct samsung_cmu_info cpif_cmu_info __initconst = {
 	.nr_clk_ids		= CPIF_NR_CLK,
 	.clk_regs		= cpif_clk_regs,
 	.nr_clk_regs		= ARRAY_SIZE(cpif_clk_regs),
+	.suspend_regs		= cpif_suspend_regs,
+	.nr_suspend_regs	= ARRAY_SIZE(cpif_suspend_regs),
 };
 
 static void __init exynos5433_cmu_cpif_init(struct device_node *np)
@@ -1547,6 +1562,11 @@ static const unsigned long peric_clk_regs[] __initconst = {
 	ENABLE_IP_PERIC2,
 };
 
+static const struct samsung_clk_reg_dump peric_suspend_regs[] = {
+	{ ENABLE_PCLK_PERIC0, 0xE00FF000 },
+	{ ENABLE_SCLK_PERIC, 0x7 },
+};
+
 static const struct samsung_div_clock peric_div_clks[] __initconst = {
 	/* DIV_PERIC */
 	DIV(CLK_DIV_SCLK_SCI, "div_sclk_sci", "oscclk", DIV_PERIC, 4, 4),
@@ -1705,6 +1725,8 @@ static const struct samsung_cmu_info peric_cmu_info __initconst = {
 	.nr_clk_ids		= PERIC_NR_CLK,
 	.clk_regs		= peric_clk_regs,
 	.nr_clk_regs		= ARRAY_SIZE(peric_clk_regs),
+	.suspend_regs		= peric_suspend_regs,
+	.nr_suspend_regs	= ARRAY_SIZE(peric_suspend_regs),
 };
 
 static void __init exynos5433_cmu_peric_init(struct device_node *np)
-- 
2.17.1

Re: [PATCH 2/2] clk: samsung: exynos5433: Add suspend state for TOP, CPIF & PERIC CMUs

[Chanwoo Choi]

Hi Marek,

On 2018년 08월 30일 01:00, Marek Szyprowski wrote:
> Before entering system suspend, one has to ensure that all TOP ACLK clocks
> are enabled, so do this by storing 0x67ECFFED value to ENABLE_ACLK_TOP
> register. In case of PERIC CMU, keep all UART PCLK and SCLK clocks as well
> as all GPIO PCLK clocks enabled.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index 426980514e67..d34e645aba49 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -177,6 +177,12 @@ static const unsigned long top_clk_regs[] __initconst = {
>  	ENABLE_CMU_TOP_DIV_STAT,
>  };
>  
> +static const struct samsung_clk_reg_dump top_suspend_regs[] = {
> +	{ ENABLE_ACLK_TOP, 0x67ECFFED }, /* all clocks enabled */
> +	{ ISP_PLL_CON0, 0x85CC0502 },	/* reset value + ENABLE bit */
> +	{ AUD_PLL_CON0, 0x84830202 },	/* reset value + ENABLE bit */

When you sent the patch[1], you used the 'small letter'
instead of capital letter. Even if it's trivial, you better to keep one style
either small or capital letter.
[1] commit 523d3de41f02 ("clk: samsung: exynos5433: Add support for runtime PM")

> +};
> +
>  /* list of all parent clock list */
>  PNAME(mout_aud_pll_p)		= { "oscclk", "fout_aud_pll", };
>  PNAME(mout_isp_pll_p)		= { "oscclk", "fout_isp_pll", };
> @@ -792,6 +798,8 @@ static const struct samsung_cmu_info top_cmu_info __initconst = {
>  	.nr_clk_ids		= TOP_NR_CLK,
>  	.clk_regs		= top_clk_regs,
>  	.nr_clk_regs		= ARRAY_SIZE(top_clk_regs),
> +	.suspend_regs		= top_suspend_regs,
> +	.nr_suspend_regs	= ARRAY_SIZE(top_suspend_regs),
>  };
>  
>  static void __init exynos5433_cmu_top_init(struct device_node *np)
> @@ -822,6 +830,11 @@ static const unsigned long cpif_clk_regs[] __initconst = {
>  	ENABLE_SCLK_CPIF,
>  };
>  
> +static const struct samsung_clk_reg_dump cpif_suspend_regs[] = {
> +	{ ENABLE_SCLK_CPIF, 0x3FF },	/* all clocks enabled */
> +	{ MPHY_PLL_CON0, 0x81C70601 },	/* reset value + ENABLE bit */
> +};
> +
>  /* list of all parent clock list */
>  PNAME(mout_mphy_pll_p)		= { "oscclk", "fout_mphy_pll", };
>  
> @@ -862,6 +875,8 @@ static const struct samsung_cmu_info cpif_cmu_info __initconst = {
>  	.nr_clk_ids		= CPIF_NR_CLK,
>  	.clk_regs		= cpif_clk_regs,
>  	.nr_clk_regs		= ARRAY_SIZE(cpif_clk_regs),
> +	.suspend_regs		= cpif_suspend_regs,
> +	.nr_suspend_regs	= ARRAY_SIZE(cpif_suspend_regs),
>  };
>  
>  static void __init exynos5433_cmu_cpif_init(struct device_node *np)
> @@ -1547,6 +1562,11 @@ static const unsigned long peric_clk_regs[] __initconst = {
>  	ENABLE_IP_PERIC2,
>  };
>  
> +static const struct samsung_clk_reg_dump peric_suspend_regs[] = {
> +	{ ENABLE_PCLK_PERIC0, 0xE00FF000 },
> +	{ ENABLE_SCLK_PERIC, 0x7 },

How about adding the comment of meaning of hex value as top/cpif_suspend_regs?

> +};
> +
>  static const struct samsung_div_clock peric_div_clks[] __initconst = {
>  	/* DIV_PERIC */
>  	DIV(CLK_DIV_SCLK_SCI, "div_sclk_sci", "oscclk", DIV_PERIC, 4, 4),
> @@ -1705,6 +1725,8 @@ static const struct samsung_cmu_info peric_cmu_info __initconst = {
>  	.nr_clk_ids		= PERIC_NR_CLK,
>  	.clk_regs		= peric_clk_regs,
>  	.nr_clk_regs		= ARRAY_SIZE(peric_clk_regs),
> +	.suspend_regs		= peric_suspend_regs,
> +	.nr_suspend_regs	= ARRAY_SIZE(peric_suspend_regs),
>  };
>  
>  static void __init exynos5433_cmu_peric_init(struct device_node *np)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] clk: samsung: exynos5433: Add suspend state for TOP, CPIF & PERIC CMUs

[Krzysztof Kozlowski]

On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> Before entering system suspend, one has to ensure that all TOP ACLK clocks
> are enabled, so do this by storing 0x67ECFFED value to ENABLE_ACLK_TOP
> register. In case of PERIC CMU, keep all UART PCLK and SCLK clocks as well
> as all GPIO PCLK clocks enabled.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5433.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
> index 426980514e67..d34e645aba49 100644
> --- a/drivers/clk/samsung/clk-exynos5433.c
> +++ b/drivers/clk/samsung/clk-exynos5433.c
> @@ -177,6 +177,12 @@ static const unsigned long top_clk_regs[] __initconst = {
>         ENABLE_CMU_TOP_DIV_STAT,
>  };
>
> +static const struct samsung_clk_reg_dump top_suspend_regs[] = {
> +       { ENABLE_ACLK_TOP, 0x67ECFFED }, /* all clocks enabled */
> +       { ISP_PLL_CON0, 0x85CC0502 },   /* reset value + ENABLE bit */
> +       { AUD_PLL_CON0, 0x84830202 },   /* reset value + ENABLE bit */
> +};
> +
>  /* list of all parent clock list */
>  PNAME(mout_aud_pll_p)          = { "oscclk", "fout_aud_pll", };
>  PNAME(mout_isp_pll_p)          = { "oscclk", "fout_isp_pll", };
> @@ -792,6 +798,8 @@ static const struct samsung_cmu_info top_cmu_info __initconst = {
>         .nr_clk_ids             = TOP_NR_CLK,
>         .clk_regs               = top_clk_regs,
>         .nr_clk_regs            = ARRAY_SIZE(top_clk_regs),
> +       .suspend_regs           = top_suspend_regs,
> +       .nr_suspend_regs        = ARRAY_SIZE(top_suspend_regs),
>  };
>
>  static void __init exynos5433_cmu_top_init(struct device_node *np)
> @@ -822,6 +830,11 @@ static const unsigned long cpif_clk_regs[] __initconst = {
>         ENABLE_SCLK_CPIF,
>  };
>
> +static const struct samsung_clk_reg_dump cpif_suspend_regs[] = {
> +       { ENABLE_SCLK_CPIF, 0x3FF },    /* all clocks enabled */
> +       { MPHY_PLL_CON0, 0x81C70601 },  /* reset value + ENABLE bit */
> +};
> +
>  /* list of all parent clock list */
>  PNAME(mout_mphy_pll_p)         = { "oscclk", "fout_mphy_pll", };
>
> @@ -862,6 +875,8 @@ static const struct samsung_cmu_info cpif_cmu_info __initconst = {
>         .nr_clk_ids             = CPIF_NR_CLK,
>         .clk_regs               = cpif_clk_regs,
>         .nr_clk_regs            = ARRAY_SIZE(cpif_clk_regs),
> +       .suspend_regs           = cpif_suspend_regs,
> +       .nr_suspend_regs        = ARRAY_SIZE(cpif_suspend_regs),
>  };
>
>  static void __init exynos5433_cmu_cpif_init(struct device_node *np)
> @@ -1547,6 +1562,11 @@ static const unsigned long peric_clk_regs[] __initconst = {
>         ENABLE_IP_PERIC2,
>  };
>
> +static const struct samsung_clk_reg_dump peric_suspend_regs[] = {
> +       { ENABLE_PCLK_PERIC0, 0xE00FF000 },
> +       { ENABLE_SCLK_PERIC, 0x7 },

It would be nice to see comment about values here as well.

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

Best regards,
Krzysztof