Re: [PATCH] ARM: Samsung: Select ARM_CPU_SUSPEND when required
On 04/08/2013 01:00 PM, Russell King - ARM Linux wrote: > On Mon, Apr 08, 2013 at 12:27:34PM +0200, Sylwester Nawrocki wrote: >> On 04/08/2013 11:57 AM, Kukjin Kim wrote: [...] > > Sigh. This stuff looks rather screwed up now: > > $ grep -B1 'ENTRY.*do_suspend' arch/arm/mm/proc*.S > arch/arm/mm/proc-arm920.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-arm920.S:ENTRY(cpu_arm920_do_suspend) > -- > arch/arm/mm/proc-arm926.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-arm926.S:ENTRY(cpu_arm926_do_suspend) > -- > arch/arm/mm/proc-mohawk.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-mohawk.S:ENTRY(cpu_mohawk_do_suspend) > -- > arch/arm/mm/proc-sa1100.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-sa1100.S:ENTRY(cpu_sa1100_do_suspend) > -- > arch/arm/mm/proc-v6.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-v6.S:ENTRY(cpu_v6_do_suspend) > -- > arch/arm/mm/proc-v7.S-#ifdef CONFIG_ARM_CPU_SUSPEND > arch/arm/mm/proc-v7.S:ENTRY(cpu_v7_do_suspend) > -- > arch/arm/mm/proc-xsc3.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-xsc3.S:ENTRY(cpu_xsc3_do_suspend) > -- > arch/arm/mm/proc-xscale.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-xscale.S:ENTRY(cpu_xscale_do_suspend) > > Now, CONFIG_PM_SLEEP is fine if this stuff only ever gets used when PM_SLEEP > is enabled - that's what it was designed for in the first place. However, > as we can see from the earlier patches in this thread, the cpu_suspend > stuff is being selected when PM is enabled (which is arguably wrong), and Yes, presumably this needs to be cleaned up. I was considering replacing CONFIG_PM with CONFIG_PM_SLEEP, but that might involve some not trivial changes. I'm going to have a look at it eventually. > also in some cases when CPU_IDLE is enabled. > > Therefore, making this code depend on ARM_CPU_SUSPEND seems sensible. > However, So, how did proc-v7.S and only that file end up doing something > different? > > commit 15e0d9e37c7fe9711b60f47221c394d45553ad8c > Author: Arnd Bergmann > Date: Sat Oct 1 21:09:39 2011 +0200 > > ARM: pm: let platforms select cpu_suspend support > > Support for the cpu_suspend functions is only built-in > when CONFIG_PM_SLEEP is enabled, but omap3/4, exynos4 > and pxa always call cpu_suspend when CONFIG_PM is enabled. > > Signed-off-by: Arnd Bergmann > > ... > diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S > index a30e785..591accd 100644 > --- a/arch/arm/mm/proc-v7.S > +++ b/arch/arm/mm/proc-v7.S > @@ -217,7 +217,7 @@ ENDPROC(cpu_v7_set_pte_ext) > /* Suspend/resume support: derived from arch/arm/mach-s5pv210/sleep.S */ > .globl cpu_v7_suspend_size > .equ cpu_v7_suspend_size, 4 * 9 > -#ifdef CONFIG_PM_SLEEP > +#ifdef CONFIG_ARM_CPU_SUSPEND > ENTRY(cpu_v7_do_suspend) > ... > > As far as this commit goes, it looks sane at the time that it was written, > but as soon as we have *any* other selections of ARM_CPU_SUSPEND, the > whole idea becomes extremely fragile - hence the reason for your build > errors. > > Moreover, with the above commit, there is _no_ sense what so ever in not > applying the same change to all proc-*.S files, thereby entirely avoiding > this fragility. I would argue that the original commit should have made > the same change to _all_ proc-*.S files. > > Let's do the job properly - hence this is now queued for -rc: Thanks, that seems a most reasonable fix. I was considering something like that, just was afraid a bit to do this wide change and that it might cause some other issues. Anyway that looks fine. > 8<=== > From: Russell King > Subject: [PATCH] ARM: Do 15e0d9e37c (ARM: pm: let platforms select > cpu_suspend support) properly > > Let's do the changes properly and fix the same problem everywhere, not > just for one case. > > Cc: # kernels containing 15e0d9e37c7fe or equivalent > Signed-off-by: Russell King > --- > arch/arm/mm/proc-arm920.S |2 +- > arch/arm/mm/proc-arm926.S |2 +- > arch/arm/mm/proc-mohawk.S |2 +- > arch/arm/mm/proc-sa1100.S |2 +- > arch/arm/mm/proc-v6.S |2 +- > arch/arm/mm/proc-xsc3.S |2 +- > arch/arm/mm/proc-xscale.S |2 +- > 7 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/mm/proc-arm920.S b/arch/arm/mm/proc-arm920.S > index 2c3b942..2556cf1 100644 > --- a/arch/arm/mm/proc-arm920.S > +++ b/arch/arm/mm/proc-arm920.S > @@ -387,7 +387,7 @@ ENTRY(cpu_arm920_set_pte_ext) > /* Suspend/resume support: taken from arch/arm/plat-s3c24xx/sleep.S */ > .globl cpu_arm920_suspend_size > .equ cpu_arm920_suspend_size, 4 * 3 > -#ifdef CONFIG_PM_SLEEP > +#ifdef CONFIG_ARM_CPU_SUSPEND > ENTRY(cpu_arm920_do_suspend) > stmfd sp!, {r4 - r6, lr} > mrc p15, 0, r4, c13, c0, 0 @ PID [...] -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: Samsung: Select ARM_CPU_SUSPEND when required
On Mon, Apr 08, 2013 at 12:27:34PM +0200, Sylwester Nawrocki wrote: > On 04/08/2013 11:57 AM, Kukjin Kim wrote: > Yes, this looks better. However after posting this patch I noticed linker > errors in some builds due to undefined cpu_arm920_do_suspend, > cpu_arm920_do_resume routines. > > It seems it is because various cpu_*_do_suspend routines are selected by > CONFIG_PM_SLEEP. And the PM code in arch/arm/mach-s3c24xx, arch/arm/mach- > s3c64xx is selected by CONFIG_PM. > > $ git grep -1 "ENTRY(cpu_.*_do_suspend" > arch/arm/mm/proc-arm920.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-arm920.S:ENTRY(cpu_arm920_do_suspend) > arch/arm/mm/proc-arm920.S- stmfd sp!, {r4 - r6, lr} > -- > arch/arm/mm/proc-arm926.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-arm926.S:ENTRY(cpu_arm926_do_suspend) > arch/arm/mm/proc-arm926.S- stmfd sp!, {r4 - r6, lr} > -- > arch/arm/mm/proc-mohawk.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-mohawk.S:ENTRY(cpu_mohawk_do_suspend) > arch/arm/mm/proc-mohawk.S- stmfd sp!, {r4 - r9, lr} > -- > arch/arm/mm/proc-sa1100.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-sa1100.S:ENTRY(cpu_sa1100_do_suspend) > arch/arm/mm/proc-sa1100.S- stmfd sp!, {r4 - r6, lr} > -- > arch/arm/mm/proc-v6.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-v6.S:ENTRY(cpu_v6_do_suspend) > arch/arm/mm/proc-v6.S- stmfd sp!, {r4 - r9, lr} > -- > arch/arm/mm/proc-v7.S-#ifdef CONFIG_ARM_CPU_SUSPEND > arch/arm/mm/proc-v7.S:ENTRY(cpu_v7_do_suspend) > arch/arm/mm/proc-v7.S- stmfd sp!, {r4 - r10, lr} > -- > arch/arm/mm/proc-xsc3.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-xsc3.S:ENTRY(cpu_xsc3_do_suspend) > arch/arm/mm/proc-xsc3.S-stmfd sp!, {r4 - r9, lr} > -- > arch/arm/mm/proc-xscale.S-#ifdef CONFIG_PM_SLEEP > arch/arm/mm/proc-xscale.S:ENTRY(cpu_xscale_do_suspend) > arch/arm/mm/proc-xscale.S- stmfd sp!, {r4 - r9, lr} > > However I can't reproduce it now :-/ Anyway the $subject patch fixes > the main issue, which I can easily reproduce here as well. So I'll > prepare another patch if needed when I get back to this later. Sigh. This stuff looks rather screwed up now: $ grep -B1 'ENTRY.*do_suspend' arch/arm/mm/proc*.S arch/arm/mm/proc-arm920.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-arm920.S:ENTRY(cpu_arm920_do_suspend) -- arch/arm/mm/proc-arm926.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-arm926.S:ENTRY(cpu_arm926_do_suspend) -- arch/arm/mm/proc-mohawk.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-mohawk.S:ENTRY(cpu_mohawk_do_suspend) -- arch/arm/mm/proc-sa1100.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-sa1100.S:ENTRY(cpu_sa1100_do_suspend) -- arch/arm/mm/proc-v6.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-v6.S:ENTRY(cpu_v6_do_suspend) -- arch/arm/mm/proc-v7.S-#ifdef CONFIG_ARM_CPU_SUSPEND arch/arm/mm/proc-v7.S:ENTRY(cpu_v7_do_suspend) -- arch/arm/mm/proc-xsc3.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-xsc3.S:ENTRY(cpu_xsc3_do_suspend) -- arch/arm/mm/proc-xscale.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-xscale.S:ENTRY(cpu_xscale_do_suspend) Now, CONFIG_PM_SLEEP is fine if this stuff only ever gets used when PM_SLEEP is enabled - that's what it was designed for in the first place. However, as we can see from the earlier patches in this thread, the cpu_suspend stuff is being selected when PM is enabled (which is arguably wrong), and also in some cases when CPU_IDLE is enabled. Therefore, making this code depend on ARM_CPU_SUSPEND seems sensible. However, So, how did proc-v7.S and only that file end up doing something different? commit 15e0d9e37c7fe9711b60f47221c394d45553ad8c Author: Arnd Bergmann Date: Sat Oct 1 21:09:39 2011 +0200 ARM: pm: let platforms select cpu_suspend support Support for the cpu_suspend functions is only built-in when CONFIG_PM_SLEEP is enabled, but omap3/4, exynos4 and pxa always call cpu_suspend when CONFIG_PM is enabled. Signed-off-by: Arnd Bergmann ... diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S index a30e785..591accd 100644 --- a/arch/arm/mm/proc-v7.S +++ b/arch/arm/mm/proc-v7.S @@ -217,7 +217,7 @@ ENDPROC(cpu_v7_set_pte_ext) /* Suspend/resume support: derived from arch/arm/mach-s5pv210/sleep.S */ .globl cpu_v7_suspend_size .equ cpu_v7_suspend_size, 4 * 9 -#ifdef CONFIG_PM_SLEEP +#ifdef CONFIG_ARM_CPU_SUSPEND ENTRY(cpu_v7_do_suspend) ... As far as this commit goes, it looks sane at the time that it was written, but as soon as we have *any* other selections of ARM_CPU_SUSPEND, the whole idea becomes extremely fragile - hence the reason for your build errors. Moreover, with the above commit, there is _no_ sense what so ever in not applying the same change to all proc-*.S files, thereby entirely avoiding this fragility. I would argue that the original commit should have made the same change to _all_ proc-*.S files. Let's do the job properly - hence this is now queued for -rc: 8<=== From: Russell King Subject: [PATCH] ARM: Do 15e0d9e37c (ARM: pm: let platforms select cpu_suspend support) properly Let's do the cha
Re: [PATCH] ARM: Samsung: Select ARM_CPU_SUSPEND when required
On 04/08/2013 11:57 AM, Kukjin Kim wrote: > Sylwester Nawrocki wrote: [...] > Yes, right. The pm.c in plat-samsung should be built with > arch/arm/kernel/sleep.S and suspend.c. > > BTW it should be shown in alphabetical order and we don't need more > following in mach-exynos. > > 8<8< > diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig > index 2f45906..bc0a8b2 100644 > --- a/arch/arm/mach-exynos/Kconfig > +++ b/arch/arm/mach-exynos/Kconfig > @@ -31,7 +31,6 @@ config CPU_EXYNOS4210 > bool "SAMSUNG EXYNOS4210" > default y > depends on ARCH_EXYNOS4 > - select ARM_CPU_SUSPEND if PM > select PM_GENERIC_DOMAINS > select S5P_PM if PM > select S5P_SLEEP if PM > diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig > index b708b3e..30a976d 100644 > --- a/arch/arm/plat-samsung/Kconfig > +++ b/arch/arm/plat-samsung/Kconfig > @@ -8,6 +8,7 @@ config PLAT_SAMSUNG > Bool > depends on PLAT_S3C24XX || ARCH_S3C64XX || PLAT_S5P > default y > + select ARM_CPU_SUSPEND if PM > select GENERIC_IRQ_CHIP > select NO_IOPORT > help > 8<8< > > If you have any objections, let me know. Yes, this looks better. However after posting this patch I noticed linker errors in some builds due to undefined cpu_arm920_do_suspend, cpu_arm920_do_resume routines. It seems it is because various cpu_*_do_suspend routines are selected by CONFIG_PM_SLEEP. And the PM code in arch/arm/mach-s3c24xx, arch/arm/mach- s3c64xx is selected by CONFIG_PM. $ git grep -1 "ENTRY(cpu_.*_do_suspend" arch/arm/mm/proc-arm920.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-arm920.S:ENTRY(cpu_arm920_do_suspend) arch/arm/mm/proc-arm920.S- stmfd sp!, {r4 - r6, lr} -- arch/arm/mm/proc-arm926.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-arm926.S:ENTRY(cpu_arm926_do_suspend) arch/arm/mm/proc-arm926.S- stmfd sp!, {r4 - r6, lr} -- arch/arm/mm/proc-mohawk.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-mohawk.S:ENTRY(cpu_mohawk_do_suspend) arch/arm/mm/proc-mohawk.S- stmfd sp!, {r4 - r9, lr} -- arch/arm/mm/proc-sa1100.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-sa1100.S:ENTRY(cpu_sa1100_do_suspend) arch/arm/mm/proc-sa1100.S- stmfd sp!, {r4 - r6, lr} -- arch/arm/mm/proc-v6.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-v6.S:ENTRY(cpu_v6_do_suspend) arch/arm/mm/proc-v6.S- stmfd sp!, {r4 - r9, lr} -- arch/arm/mm/proc-v7.S-#ifdef CONFIG_ARM_CPU_SUSPEND arch/arm/mm/proc-v7.S:ENTRY(cpu_v7_do_suspend) arch/arm/mm/proc-v7.S- stmfd sp!, {r4 - r10, lr} -- arch/arm/mm/proc-xsc3.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-xsc3.S:ENTRY(cpu_xsc3_do_suspend) arch/arm/mm/proc-xsc3.S-stmfd sp!, {r4 - r9, lr} -- arch/arm/mm/proc-xscale.S-#ifdef CONFIG_PM_SLEEP arch/arm/mm/proc-xscale.S:ENTRY(cpu_xscale_do_suspend) arch/arm/mm/proc-xscale.S- stmfd sp!, {r4 - r9, lr} However I can't reproduce it now :-/ Anyway the $subject patch fixes the main issue, which I can easily reproduce here as well. So I'll prepare another patch if needed when I get back to this later. Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: Samsung: Select ARM_CPU_SUSPEND when required
Sylwester Nawrocki wrote: > > The power management code of S3C24XX, S3C64XX, S5PV210 platform in > arch/arm/plat-samsung/, arch/arm/mach-s3c24xx/, arch/arm/mach-s3c64xx/ > directories uses generic cpu_suspend routine. Make sure it is compiled > in when building with power management support. Without this patch > compilation fails with errors as below. It can be reproduced by using > default config files with CONFIG_SUSPEND disabled and CONFIG_PM_RUNTIME > enabled. > > - s5pv210_defconfig > arch/arm/plat-samsung/built-in.o: In function `s3c_pm_enter': > arch/arm/plat-samsung/pm.c:304: undefined reference to `cpu_suspend' > arch/arm/plat-samsung/built-in.o: In function `s3c_cpu_resume': > arch/arm/plat-samsung/s5p-sleep.S:74: undefined reference to `cpu_resume' > > - s3c24xx_defconfig > arch/arm/mach-s3c24xx/built-in.o: In function `s3c_cpu_resume': > arch/arm/mach-s3c24xx/sleep.S:83: undefined reference to `cpu_resume' > arch/arm/plat-samsung/built-in.o: In function `s3c_pm_enter': > arch/arm/plat-samsung/pm.c:304: undefined reference to `cpu_suspend' > > - s3c64xx_defconfig > arch/arm/mach-s3c64xx/built-in.o: In function `s3c_cpu_resume': > arch/arm/mach-s3c64xx/sleep.S:72: undefined reference to `cpu_resume' > arch/arm/plat-samsung/built-in.o: In function `s3c_pm_enter': > arch/arm/plat-samsung/pm.c:304: undefined reference to `cpu_suspend' > > To fix this issue select ARM_CPU_SUSPEND for PLAT_SAMSUNG when PM is set. > > The build break occurs for kernels at least back to v3.0, however this > patch applies without conflicts only back to v3.7. > > Signed-off-by: Sylwester Nawrocki > Cc: sta...@vger.kernel.org > --- > arch/arm/plat-samsung/Kconfig |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig > index 54d1861..02355ba 100644 > --- a/arch/arm/plat-samsung/Kconfig > +++ b/arch/arm/plat-samsung/Kconfig > @@ -10,6 +10,7 @@ config PLAT_SAMSUNG > default y > select GENERIC_IRQ_CHIP > select NO_IOPORT > + select ARM_CPU_SUSPEND if PM > help > Base platform code for all Samsung SoC based systems > > -- > 1.7.4.1 Yes, right. The pm.c in plat-samsung should be built with arch/arm/kernel/sleep.S and suspend.c. BTW it should be shown in alphabetical order and we don't need more following in mach-exynos. 8<8< diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig index 2f45906..bc0a8b2 100644 --- a/arch/arm/mach-exynos/Kconfig +++ b/arch/arm/mach-exynos/Kconfig @@ -31,7 +31,6 @@ config CPU_EXYNOS4210 bool "SAMSUNG EXYNOS4210" default y depends on ARCH_EXYNOS4 - select ARM_CPU_SUSPEND if PM select PM_GENERIC_DOMAINS select S5P_PM if PM select S5P_SLEEP if PM diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig index b708b3e..30a976d 100644 --- a/arch/arm/plat-samsung/Kconfig +++ b/arch/arm/plat-samsung/Kconfig @@ -8,6 +8,7 @@ config PLAT_SAMSUNG Bool depends on PLAT_S3C24XX || ARCH_S3C64XX || PLAT_S5P default y + select ARM_CPU_SUSPEND if PM select GENERIC_IRQ_CHIP select NO_IOPORT help 8<8< If you have any objections, let me know. Thanks. - Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html