[RFC PATCH 0/2] ARM: EXYNOS: Fix Suspend-to-RAM on Exynos5420

[RFC PATCH 0/2] ARM: EXYNOS: Fix Suspend-to-RAM on Exynos5420

[Javier Martinez Canillas]

Hello,

Suspend-to-RAM is currently not working. Abhilash Kesavan traced down
to the MDMA0 DMA controller clock to be disabled during suspend and
that it must stay enaled during suspend or the system is not able to
resume.

This series is an attempt to fix the issue and is composed of patches:

Javier Martinez Canillas (2):
  clk: exynos5420: Add alias for MDMA0 controller clock
  ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled
    during suspend

 arch/arm/mach-exynos/suspend.c       | 15 +++++++++++++++
 drivers/clk/samsung/clk-exynos5420.c |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

Patch #1 just adds an alias fo the MDMA0 clock so a lookup is registered.

Patch #2 enables the MDMA0 clock before entering into a suspend state and
disables it again once the system has been resumed.

The patches are a RFC because I'm not sure if this is the best approach
to solve the issue. So feedback will be highly appreciated.

Best regards,
Javier

[RFC PATCH 1/2] clk: exynos5420: Add alias for MDMA0 controller clock

[Javier Martinez Canillas]

The MDMA0 controller clock needs to be enabled to allow the
system to be resumed when entering into a suspend state.

The clock is disabled as a part of the runtime pm for the
pl330 DMA driver so the system fails to resume. So to allow
the system to grab the clock and make sure that it stays
enabled during suspend, an alias has to be added so a clock
lookup for a clock is registered.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/clk/samsung/clk-exynos5420.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 07d666cc6a29..8b49e8b3b548 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -893,7 +893,7 @@ static struct samsung_div_clock exynos5x_div_clks[] __initdata = {
 
 static struct samsung_gate_clock exynos5x_gate_clks[] __initdata = {
 	/* G2D */
-	GATE(CLK_MDMA0, "mdma0", "aclk266_g2d", GATE_IP_G2D, 1, 0, 0),
+	GATE_A(CLK_MDMA0, "mdma0", "aclk266_g2d", GATE_IP_G2D, 1, 0, 0, "mdma0"),
 	GATE(CLK_SSS, "sss", "aclk266_g2d", GATE_IP_G2D, 2, 0, 0),
 	GATE(CLK_G2D, "g2d", "aclk333_g2d", GATE_IP_G2D, 3, 0, 0),
 	GATE(CLK_SMMU_MDMA0, "smmu_mdma0", "aclk266_g2d", GATE_IP_G2D, 5, 0, 0),
-- 
2.1.4

[RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Javier Martinez Canillas]

Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
Management support v12") added pm support for the pl330 dma driver but
it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
during suspend and this clock needs to remain enabled in order to make
the system resume from a system suspend state.

To make sure that the clock is enabled during suspend, enable it prior
to entering a suspend state and disable it once the system has resumed.

Thanks to Abhilash Kesavan for figuring out that this was the issue.

Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/mach-exynos/suspend.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index 1521eaf99265..6dbc0a6d1bb5 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -16,6 +16,7 @@
 #include <linux/init.h>
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
+#include <linux/clk.h>
 #include <linux/cpu_pm.h>
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -79,6 +80,7 @@ static const struct exynos_pm_data *pm_data;
 
 static int exynos5420_cpu_state;
 static unsigned int exynos_pmu_spare3;
+static struct clk *clk;
 
 /*
  * GIC wake-up support
@@ -374,6 +376,16 @@ static void exynos5420_pm_prepare(void)
 {
 	unsigned int tmp;
 
+	/*
+	 * Exynos5420 requires the MDMA0 controller clock to be
+	 * ungated on suspend in order to be resumed correctly.
+	 */
+	clk = clk_get(NULL, "mdma0");
+	if (IS_ERR(clk))
+		pr_warn("Failed to get mdma0 clk (%ld)\n", PTR_ERR(clk));
+	else
+		clk_prepare_enable(clk);
+
 	/* Set wake-up mask registers */
 	exynos_pm_set_wakeup_mask();
 
@@ -516,6 +528,9 @@ static void exynos5420_pm_resume(void)
 {
 	unsigned long tmp;
 
+	if (!IS_ERR_OR_NULL(clk))
+		clk_disable_unprepare(clk);
+
 	/* Restore the CPU0 low power state register */
 	tmp = pmu_raw_readl(EXYNOS5_ARM_CORE0_SYS_PWR_REG);
 	pmu_raw_writel(tmp | S5P_CORE_LOCAL_PWR_EN,
-- 
2.1.4

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Sylwester Nawrocki]

Hello Javier,

On 27/03/15 15:21, Javier Martinez Canillas wrote:
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this clock needs to remain enabled in order to make
> the system resume from a system suspend state.
> 
> To make sure that the clock is enabled during suspend, enable it prior
> to entering a suspend state and disable it once the system has resumed.
> 
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
> 
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-exynos/suspend.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 1521eaf99265..6dbc0a6d1bb5 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -16,6 +16,7 @@
>  #include <linux/init.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/clk.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> @@ -79,6 +80,7 @@ static const struct exynos_pm_data *pm_data;
>  
>  static int exynos5420_cpu_state;
>  static unsigned int exynos_pmu_spare3;
> +static struct clk *clk;
>  
>  /*
>   * GIC wake-up support
> @@ -374,6 +376,16 @@ static void exynos5420_pm_prepare(void)
>  {
>  	unsigned int tmp;
>  
> +	/*
> +	 * Exynos5420 requires the MDMA0 controller clock to be
> +	 * ungated on suspend in order to be resumed correctly.
> +	 */
> +	clk = clk_get(NULL, "mdma0");
> +	if (IS_ERR(clk))
> +		pr_warn("Failed to get mdma0 clk (%ld)\n", PTR_ERR(clk));

I suppose you want this clk_get() call in exynos_pm_init(), now there
is clk_put() missing and this will cause a memory leak.

> +	else
> +		clk_prepare_enable(clk);
> +
>  	/* Set wake-up mask registers */
>  	exynos_pm_set_wakeup_mask();
>  
> @@ -516,6 +528,9 @@ static void exynos5420_pm_resume(void)
>  {
>  	unsigned long tmp;
>  
> +	if (!IS_ERR_OR_NULL(clk))

This should be just IS_ERR().

> +		clk_disable_unprepare(clk);
> +
>  	/* Restore the CPU0 low power state register */
>  	tmp = pmu_raw_readl(EXYNOS5_ARM_CORE0_SYS_PWR_REG);
>  	pmu_raw_writel(tmp | S5P_CORE_LOCAL_PWR_EN,

--
Regards,
Sylwester

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Javier Martinez Canillas]

Hello Sylwester,

Thanks a lot for your feedback.

On 03/27/2015 03:36 PM, Sylwester Nawrocki wrote:
>>   * GIC wake-up support
>> @@ -374,6 +376,16 @@ static void exynos5420_pm_prepare(void)
>>  {
>>  	unsigned int tmp;
>>  
>> +	/*
>> +	 * Exynos5420 requires the MDMA0 controller clock to be
>> +	 * ungated on suspend in order to be resumed correctly.
>> +	 */
>> +	clk = clk_get(NULL, "mdma0");
>> +	if (IS_ERR(clk))
>> +		pr_warn("Failed to get mdma0 clk (%ld)\n", PTR_ERR(clk));
> 
> I suppose you want this clk_get() call in exynos_pm_init(), now there

Well I wanted to do it in an exynos5420 specific function to avoid
having an of_machine_is_compatible("samsung,exynos5420") but I can
move there if that is preferred.

> is clk_put() missing and this will cause a memory leak.
>

duh, I wonder how I missed that. Thanks for pointing it out.

>> +	else
>> +		clk_prepare_enable(clk);
>> +
>>  	/* Set wake-up mask registers */
>>  	exynos_pm_set_wakeup_mask();
>>  
>> @@ -516,6 +528,9 @@ static void exynos5420_pm_resume(void)
>>  {
>>  	unsigned long tmp;
>>  
>> +	if (!IS_ERR_OR_NULL(clk))
> 
> This should be just IS_ERR().
>

Ok.

>> +		clk_disable_unprepare(clk);
>> +
>>  	/* Restore the CPU0 low power state register */
>>  	tmp = pmu_raw_readl(EXYNOS5_ARM_CORE0_SYS_PWR_REG);
>>  	pmu_raw_writel(tmp | S5P_CORE_LOCAL_PWR_EN,
> 
> --
> Regards,
> Sylwester
> 

Best regards,
Javier

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Krzysztof Kozlowski]

2015-03-27 15:21 GMT+01:00 Javier Martinez Canillas
<javier.martinez@collabora.co.uk>:
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this clock needs to remain enabled in order to make
> the system resume from a system suspend state.
>
> To make sure that the clock is enabled during suspend, enable it prior
> to entering a suspend state and disable it once the system has resumed.
>
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Hmmm... isn't a real problem elsewhere - like some clock hierarchy is
wrong or incomplete? This rather looks like workaround for the
problem.

> ---
>  arch/arm/mach-exynos/suspend.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 1521eaf99265..6dbc0a6d1bb5 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -16,6 +16,7 @@
>  #include <linux/init.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/clk.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> @@ -79,6 +80,7 @@ static const struct exynos_pm_data *pm_data;
>
>  static int exynos5420_cpu_state;
>  static unsigned int exynos_pmu_spare3;
> +static struct clk *clk;
>
>  /*
>   * GIC wake-up support
> @@ -374,6 +376,16 @@ static void exynos5420_pm_prepare(void)
>  {
>         unsigned int tmp;
>
> +       /*
> +        * Exynos5420 requires the MDMA0 controller clock to be
> +        * ungated on suspend in order to be resumed correctly.
> +        */
> +       clk = clk_get(NULL, "mdma0");
> +       if (IS_ERR(clk))
> +               pr_warn("Failed to get mdma0 clk (%ld)\n", PTR_ERR(clk));
> +       else
> +               clk_prepare_enable(clk);
> +
>         /* Set wake-up mask registers */
>         exynos_pm_set_wakeup_mask();
>
> @@ -516,6 +528,9 @@ static void exynos5420_pm_resume(void)
>  {
>         unsigned long tmp;
>
> +       if (!IS_ERR_OR_NULL(clk))
> +               clk_disable_unprepare(clk);
> +

Missing clk_put()

Best regards,
Krzysztof

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for your feedback.

On 03/27/2015 03:38 PM, Krzysztof Kozlowski wrote:
> 2015-03-27 15:21 GMT+01:00 Javier Martinez Canillas
> <javier.martinez@collabora.co.uk>:
>> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
>> Management support v12") added pm support for the pl330 dma driver but
>> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
>> during suspend and this clock needs to remain enabled in order to make
>> the system resume from a system suspend state.
>>
>> To make sure that the clock is enabled during suspend, enable it prior
>> to entering a suspend state and disable it once the system has resumed.
>>
>> Thanks to Abhilash Kesavan for figuring out that this was the issue.
>>
>> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> 
> Hmmm... isn't a real problem elsewhere - like some clock hierarchy is
> wrong or incomplete? This rather looks like workaround for the
> problem.
>

That's a very good question, that's why I sent it as an RFC. I didn't find
anything relevant in the manual but I could certainly be missing something.

>>
>> @@ -516,6 +528,9 @@ static void exynos5420_pm_resume(void)
>>  {
>>         unsigned long tmp;
>>
>> +       if (!IS_ERR_OR_NULL(clk))
>> +               clk_disable_unprepare(clk);
>> +
> 
> Missing clk_put()
>

Yes, Sylwester already pointed it out. Sorry for missing that.
 
> Best regards,
> Krzysztof
> 

Best regards,
Javier

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Chanwoo Choi]

Hi Javier,

On 03/27/2015 11:21 PM, Javier Martinez Canillas wrote:
> Commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power
> Management support v12") added pm support for the pl330 dma driver but
> it makes the clock for the Exynos5420 MDMA0 DMA controller to be gated
> during suspend and this clock needs to remain enabled in order to make
> the system resume from a system suspend state.
> 
> To make sure that the clock is enabled during suspend, enable it prior
> to entering a suspend state and disable it once the system has resumed.
> 
> Thanks to Abhilash Kesavan for figuring out that this was the issue.
> 
> Fixes: ae43b32 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-exynos/suspend.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index 1521eaf99265..6dbc0a6d1bb5 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -16,6 +16,7 @@
>  #include <linux/init.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/clk.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/io.h>
>  #include <linux/irq.h>
> @@ -79,6 +80,7 @@ static const struct exynos_pm_data *pm_data;
>  
>  static int exynos5420_cpu_state;
>  static unsigned int exynos_pmu_spare3;
> +static struct clk *clk;
>  
>  /*
>   * GIC wake-up support
> @@ -374,6 +376,16 @@ static void exynos5420_pm_prepare(void)
>  {
>  	unsigned int tmp;
>  
> +	/*
> +	 * Exynos5420 requires the MDMA0 controller clock to be
> +	 * ungated on suspend in order to be resumed correctly.
> +	 */
> +	clk = clk_get(NULL, "mdma0");
> +	if (IS_ERR(clk))
> +		pr_warn("Failed to get mdma0 clk (%ld)\n", PTR_ERR(clk));
> +	else
> +		clk_prepare_enable(clk);
> +

I faced on the similiar issue. If some clock was disabled,
Exynos SoC could not enter the suspend mode

But, I think it is not prpper method to resolve this issue.
about that that specific clock (e.g., mdma0) is handled in this dirver.
Also, ARM-64bit don't include any '../mach-exynos' directory.

IMHO, some clock issue have to be handled in SoC clk driver or others.

[snip]

Best Regards,
Chanwoo Choi

Re: [RFC PATCH 2/2] ARM: EXYNOS: Make sure that the Exynos5420 MDMA0 clock is enabled during suspend

[Javier Martinez Canillas]

Hello Chanwoo,

Thanks a lot for your feedback.

On 03/30/2015 04:04 AM, Chanwoo Choi wrote:
> 
> I faced on the similiar issue. If some clock was disabled,
> Exynos SoC could not enter the suspend mode
> 
> But, I think it is not prpper method to resolve this issue.
> about that that specific clock (e.g., mdma0) is handled in this dirver.
> Also, ARM-64bit don't include any '../mach-exynos' directory.
> 
> IMHO, some clock issue have to be handled in SoC clk driver or others.
> 

Ok, Abhilash also said that before so I just posted another RFC:

"[RFC PATCH v3 0/2] ARM: EXYNOS: Fix Suspend-to-RAM on Exynos5420"

Please let me know if that is more aligned to what you were 
thinking as the proper solution.

> 
> Best Regards,
> Chanwoo Choi
> 

Best regards,
Javier