| Message ID | 1427466097-7287-3-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(+)