* Re: [PATCH] ARM: imx: build v7_cpu_resume() unconditionally [not found] <20200116141849.73955-1-r.czerwinski@pengutronix.de> @ 2020-03-22 18:50 ` Clemens Gruber 2020-03-22 20:22 ` Sasha Levin 0 siblings, 1 reply; 4+ messages in thread From: Clemens Gruber @ 2020-03-22 18:50 UTC (permalink / raw) To: Rouven Czerwinski Cc: s.hauer, shawnguo, Ahmad Fatoum, linux-kernel, linux-imx, kernel, festevam, linux-arm-kernel, Lucas Stach, stable, Greg Kroah-Hartman Hi, On Thu, Jan 16, 2020 at 03:18:49PM +0100, Rouven Czerwinski wrote: > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > > This function is not only needed by the platform suspend code, but is also > reused as the CPU resume function when the ARM cores can be powered down > completely in deep idle, which is the case on i.MX6SX and i.MX6UL(L). > > Providing the static inline stub whenever CONFIG_SUSPEND is disabled means > that those platforms will hang on resume from cpuidle if suspend is disabled. > > So there are two problems: > > - The static inline stub masks the linker error > - The function is not available where needed > > Fix both by just building the function unconditionally, when > CONFIG_SOC_IMX6 is enabled. The actual code is three instructions long, > so it's arguably ok to just leave it in for all i.MX6 kernel configurations. > > Fixes: 05136f0897b5 ("ARM: imx: support arm power off in cpuidle for i.mx6sx") > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de> > --- > arch/arm/mach-imx/Makefile | 2 ++ > arch/arm/mach-imx/common.h | 4 ++-- > arch/arm/mach-imx/resume-imx6.S | 24 ++++++++++++++++++++++++ > arch/arm/mach-imx/suspend-imx6.S | 14 -------------- > 4 files changed, 28 insertions(+), 16 deletions(-) > create mode 100644 arch/arm/mach-imx/resume-imx6.S > > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile > index 35ff620537e6..03506ce46149 100644 > --- a/arch/arm/mach-imx/Makefile > +++ b/arch/arm/mach-imx/Makefile > @@ -91,6 +91,8 @@ AFLAGS_suspend-imx6.o :=-Wa,-march=armv7-a > obj-$(CONFIG_SOC_IMX6) += suspend-imx6.o > obj-$(CONFIG_SOC_IMX53) += suspend-imx53.o > endif > +AFLAGS_resume-imx6.o :=-Wa,-march=armv7-a > +obj-$(CONFIG_SOC_IMX6) += resume-imx6.o > obj-$(CONFIG_SOC_IMX6) += pm-imx6.o > > obj-$(CONFIG_SOC_IMX1) += mach-imx1.o > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h > index 912aeceb4ff8..5aa5796cff0e 100644 > --- a/arch/arm/mach-imx/common.h > +++ b/arch/arm/mach-imx/common.h > @@ -109,17 +109,17 @@ void imx_cpu_die(unsigned int cpu); > int imx_cpu_kill(unsigned int cpu); > > #ifdef CONFIG_SUSPEND > -void v7_cpu_resume(void); > void imx53_suspend(void __iomem *ocram_vbase); > extern const u32 imx53_suspend_sz; > void imx6_suspend(void __iomem *ocram_vbase); > #else > -static inline void v7_cpu_resume(void) {} > static inline void imx53_suspend(void __iomem *ocram_vbase) {} > static const u32 imx53_suspend_sz; > static inline void imx6_suspend(void __iomem *ocram_vbase) {} > #endif > > +void v7_cpu_resume(void); > + > void imx6_pm_ccm_init(const char *ccm_compat); > void imx6q_pm_init(void); > void imx6dl_pm_init(void); > diff --git a/arch/arm/mach-imx/resume-imx6.S b/arch/arm/mach-imx/resume-imx6.S > new file mode 100644 > index 000000000000..5bd1ba7ef15b > --- /dev/null > +++ b/arch/arm/mach-imx/resume-imx6.S > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright 2014 Freescale Semiconductor, Inc. > + */ > + > +#include <linux/linkage.h> > +#include <asm/assembler.h> > +#include <asm/asm-offsets.h> > +#include <asm/hardware/cache-l2x0.h> > +#include "hardware.h" > + > +/* > + * The following code must assume it is running from physical address > + * where absolute virtual addresses to the data section have to be > + * turned into relative ones. > + */ > + > +ENTRY(v7_cpu_resume) > + bl v7_invalidate_l1 > +#ifdef CONFIG_CACHE_L2X0 > + bl l2c310_early_resume > +#endif > + b cpu_resume > +ENDPROC(v7_cpu_resume) > diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S > index 062391ff13da..1eabf2d2834b 100644 > --- a/arch/arm/mach-imx/suspend-imx6.S > +++ b/arch/arm/mach-imx/suspend-imx6.S > @@ -327,17 +327,3 @@ resume: > > ret lr > ENDPROC(imx6_suspend) > - > -/* > - * The following code must assume it is running from physical address > - * where absolute virtual addresses to the data section have to be > - * turned into relative ones. > - */ > - > -ENTRY(v7_cpu_resume) > - bl v7_invalidate_l1 > -#ifdef CONFIG_CACHE_L2X0 > - bl l2c310_early_resume > -#endif > - b cpu_resume > -ENDPROC(v7_cpu_resume) > -- > 2.25.0 This patch broke the build for our i.MX6 kernel. I am referring to commits 512a928aff in mainline and 7199cb65bb in linux-stable. In our kernel, neither CONFIG_PM nor CONFIG_SUSPEND are set. Therefore, ARM_CPU_SUSPEND is also unset, which means that sleep.S (containing cpu_resume) is not built. With this patch, ld reports the following error: arch/arm/mach-imx/resume-imx6.o: in function `v7_cpu_resume': (.text+0x8): undefined reference to `cpu_resume' Best regards, Clemens ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ARM: imx: build v7_cpu_resume() unconditionally 2020-03-22 18:50 ` [PATCH] ARM: imx: build v7_cpu_resume() unconditionally Clemens Gruber @ 2020-03-22 20:22 ` Sasha Levin 2020-03-22 23:09 ` Clemens Gruber 0 siblings, 1 reply; 4+ messages in thread From: Sasha Levin @ 2020-03-22 20:22 UTC (permalink / raw) To: 20200116141849.73955-1-r.czerwinski Cc: Rouven Czerwinski, s.hauer, shawnguo, Ahmad Fatoum, linux-kernel, linux-imx, kernel, festevam, linux-arm-kernel, Lucas Stach, stable, Greg Kroah-Hartman On Sun, Mar 22, 2020 at 07:50:22PM +0100, Clemens Gruber wrote: >Hi, > >On Thu, Jan 16, 2020 at 03:18:49PM +0100, Rouven Czerwinski wrote: >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> >> >> This function is not only needed by the platform suspend code, but is also >> reused as the CPU resume function when the ARM cores can be powered down >> completely in deep idle, which is the case on i.MX6SX and i.MX6UL(L). >> >> Providing the static inline stub whenever CONFIG_SUSPEND is disabled means >> that those platforms will hang on resume from cpuidle if suspend is disabled. >> >> So there are two problems: >> >> - The static inline stub masks the linker error >> - The function is not available where needed >> >> Fix both by just building the function unconditionally, when >> CONFIG_SOC_IMX6 is enabled. The actual code is three instructions long, >> so it's arguably ok to just leave it in for all i.MX6 kernel configurations. >> >> Fixes: 05136f0897b5 ("ARM: imx: support arm power off in cpuidle for i.mx6sx") >> Signed-off-by: Lucas Stach <l.stach@pengutronix.de> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de> >> --- >> arch/arm/mach-imx/Makefile | 2 ++ >> arch/arm/mach-imx/common.h | 4 ++-- >> arch/arm/mach-imx/resume-imx6.S | 24 ++++++++++++++++++++++++ >> arch/arm/mach-imx/suspend-imx6.S | 14 -------------- >> 4 files changed, 28 insertions(+), 16 deletions(-) >> create mode 100644 arch/arm/mach-imx/resume-imx6.S >> >> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile >> index 35ff620537e6..03506ce46149 100644 >> --- a/arch/arm/mach-imx/Makefile >> +++ b/arch/arm/mach-imx/Makefile >> @@ -91,6 +91,8 @@ AFLAGS_suspend-imx6.o :=-Wa,-march=armv7-a >> obj-$(CONFIG_SOC_IMX6) += suspend-imx6.o >> obj-$(CONFIG_SOC_IMX53) += suspend-imx53.o >> endif >> +AFLAGS_resume-imx6.o :=-Wa,-march=armv7-a >> +obj-$(CONFIG_SOC_IMX6) += resume-imx6.o >> obj-$(CONFIG_SOC_IMX6) += pm-imx6.o >> >> obj-$(CONFIG_SOC_IMX1) += mach-imx1.o >> diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h >> index 912aeceb4ff8..5aa5796cff0e 100644 >> --- a/arch/arm/mach-imx/common.h >> +++ b/arch/arm/mach-imx/common.h >> @@ -109,17 +109,17 @@ void imx_cpu_die(unsigned int cpu); >> int imx_cpu_kill(unsigned int cpu); >> >> #ifdef CONFIG_SUSPEND >> -void v7_cpu_resume(void); >> void imx53_suspend(void __iomem *ocram_vbase); >> extern const u32 imx53_suspend_sz; >> void imx6_suspend(void __iomem *ocram_vbase); >> #else >> -static inline void v7_cpu_resume(void) {} >> static inline void imx53_suspend(void __iomem *ocram_vbase) {} >> static const u32 imx53_suspend_sz; >> static inline void imx6_suspend(void __iomem *ocram_vbase) {} >> #endif >> >> +void v7_cpu_resume(void); >> + >> void imx6_pm_ccm_init(const char *ccm_compat); >> void imx6q_pm_init(void); >> void imx6dl_pm_init(void); >> diff --git a/arch/arm/mach-imx/resume-imx6.S b/arch/arm/mach-imx/resume-imx6.S >> new file mode 100644 >> index 000000000000..5bd1ba7ef15b >> --- /dev/null >> +++ b/arch/arm/mach-imx/resume-imx6.S >> @@ -0,0 +1,24 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright 2014 Freescale Semiconductor, Inc. >> + */ >> + >> +#include <linux/linkage.h> >> +#include <asm/assembler.h> >> +#include <asm/asm-offsets.h> >> +#include <asm/hardware/cache-l2x0.h> >> +#include "hardware.h" >> + >> +/* >> + * The following code must assume it is running from physical address >> + * where absolute virtual addresses to the data section have to be >> + * turned into relative ones. >> + */ >> + >> +ENTRY(v7_cpu_resume) >> + bl v7_invalidate_l1 >> +#ifdef CONFIG_CACHE_L2X0 >> + bl l2c310_early_resume >> +#endif >> + b cpu_resume >> +ENDPROC(v7_cpu_resume) >> diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S >> index 062391ff13da..1eabf2d2834b 100644 >> --- a/arch/arm/mach-imx/suspend-imx6.S >> +++ b/arch/arm/mach-imx/suspend-imx6.S >> @@ -327,17 +327,3 @@ resume: >> >> ret lr >> ENDPROC(imx6_suspend) >> - >> -/* >> - * The following code must assume it is running from physical address >> - * where absolute virtual addresses to the data section have to be >> - * turned into relative ones. >> - */ >> - >> -ENTRY(v7_cpu_resume) >> - bl v7_invalidate_l1 >> -#ifdef CONFIG_CACHE_L2X0 >> - bl l2c310_early_resume >> -#endif >> - b cpu_resume >> -ENDPROC(v7_cpu_resume) >> -- >> 2.25.0 > >This patch broke the build for our i.MX6 kernel. > >I am referring to commits 512a928aff in mainline and >7199cb65bb in linux-stable. > >In our kernel, neither CONFIG_PM nor CONFIG_SUSPEND are set. Therefore, >ARM_CPU_SUSPEND is also unset, which means that sleep.S (containing >cpu_resume) is not built. > >With this patch, ld reports the following error: >arch/arm/mach-imx/resume-imx6.o: in function `v7_cpu_resume': >(.text+0x8): undefined reference to `cpu_resume' Is this a problem with Linus's tree as well? -- Thanks, Sasha ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ARM: imx: build v7_cpu_resume() unconditionally 2020-03-22 20:22 ` Sasha Levin @ 2020-03-22 23:09 ` Clemens Gruber 2020-03-23 8:22 ` Ahmad Fatoum 0 siblings, 1 reply; 4+ messages in thread From: Clemens Gruber @ 2020-03-22 23:09 UTC (permalink / raw) To: Sasha Levin Cc: Ahmad Fatoum, festevam, s.hauer, linux-kernel, stable, linux-imx, kernel, Greg Kroah-Hartman, Rouven Czerwinski, shawnguo, linux-arm-kernel, Lucas Stach, Clemens Gruber On Sun, Mar 22, 2020 at 04:22:24PM -0400, Sasha Levin wrote: > On Sun, Mar 22, 2020 at 07:50:22PM +0100, Clemens Gruber wrote: > > Hi, > > > > On Thu, Jan 16, 2020 at 03:18:49PM +0100, Rouven Czerwinski wrote: > > > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > > > > > > This function is not only needed by the platform suspend code, but is also > > > reused as the CPU resume function when the ARM cores can be powered down > > > completely in deep idle, which is the case on i.MX6SX and i.MX6UL(L). > > > > > > Providing the static inline stub whenever CONFIG_SUSPEND is disabled means > > > that those platforms will hang on resume from cpuidle if suspend is disabled. > > > > > > So there are two problems: > > > > > > - The static inline stub masks the linker error > > > - The function is not available where needed > > > > > > Fix both by just building the function unconditionally, when > > > CONFIG_SOC_IMX6 is enabled. The actual code is three instructions long, > > > so it's arguably ok to just leave it in for all i.MX6 kernel configurations. > > > > > > Fixes: 05136f0897b5 ("ARM: imx: support arm power off in cpuidle for i.mx6sx") > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > > Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > > --- > > > arch/arm/mach-imx/Makefile | 2 ++ > > > arch/arm/mach-imx/common.h | 4 ++-- > > > arch/arm/mach-imx/resume-imx6.S | 24 ++++++++++++++++++++++++ > > > arch/arm/mach-imx/suspend-imx6.S | 14 -------------- > > > 4 files changed, 28 insertions(+), 16 deletions(-) > > > create mode 100644 arch/arm/mach-imx/resume-imx6.S > > > > > > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile > > > index 35ff620537e6..03506ce46149 100644 > > > --- a/arch/arm/mach-imx/Makefile > > > +++ b/arch/arm/mach-imx/Makefile > > > @@ -91,6 +91,8 @@ AFLAGS_suspend-imx6.o :=-Wa,-march=armv7-a > > > obj-$(CONFIG_SOC_IMX6) += suspend-imx6.o > > > obj-$(CONFIG_SOC_IMX53) += suspend-imx53.o > > > endif > > > +AFLAGS_resume-imx6.o :=-Wa,-march=armv7-a > > > +obj-$(CONFIG_SOC_IMX6) += resume-imx6.o > > > obj-$(CONFIG_SOC_IMX6) += pm-imx6.o > > > > > > obj-$(CONFIG_SOC_IMX1) += mach-imx1.o > > > diff --git a/arch/arm/mach-imx/common.h b/arch/arm/mach-imx/common.h > > > index 912aeceb4ff8..5aa5796cff0e 100644 > > > --- a/arch/arm/mach-imx/common.h > > > +++ b/arch/arm/mach-imx/common.h > > > @@ -109,17 +109,17 @@ void imx_cpu_die(unsigned int cpu); > > > int imx_cpu_kill(unsigned int cpu); > > > > > > #ifdef CONFIG_SUSPEND > > > -void v7_cpu_resume(void); > > > void imx53_suspend(void __iomem *ocram_vbase); > > > extern const u32 imx53_suspend_sz; > > > void imx6_suspend(void __iomem *ocram_vbase); > > > #else > > > -static inline void v7_cpu_resume(void) {} > > > static inline void imx53_suspend(void __iomem *ocram_vbase) {} > > > static const u32 imx53_suspend_sz; > > > static inline void imx6_suspend(void __iomem *ocram_vbase) {} > > > #endif > > > > > > +void v7_cpu_resume(void); > > > + > > > void imx6_pm_ccm_init(const char *ccm_compat); > > > void imx6q_pm_init(void); > > > void imx6dl_pm_init(void); > > > diff --git a/arch/arm/mach-imx/resume-imx6.S b/arch/arm/mach-imx/resume-imx6.S > > > new file mode 100644 > > > index 000000000000..5bd1ba7ef15b > > > --- /dev/null > > > +++ b/arch/arm/mach-imx/resume-imx6.S > > > @@ -0,0 +1,24 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright 2014 Freescale Semiconductor, Inc. > > > + */ > > > + > > > +#include <linux/linkage.h> > > > +#include <asm/assembler.h> > > > +#include <asm/asm-offsets.h> > > > +#include <asm/hardware/cache-l2x0.h> > > > +#include "hardware.h" > > > + > > > +/* > > > + * The following code must assume it is running from physical address > > > + * where absolute virtual addresses to the data section have to be > > > + * turned into relative ones. > > > + */ > > > + > > > +ENTRY(v7_cpu_resume) > > > + bl v7_invalidate_l1 > > > +#ifdef CONFIG_CACHE_L2X0 > > > + bl l2c310_early_resume > > > +#endif > > > + b cpu_resume > > > +ENDPROC(v7_cpu_resume) > > > diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S > > > index 062391ff13da..1eabf2d2834b 100644 > > > --- a/arch/arm/mach-imx/suspend-imx6.S > > > +++ b/arch/arm/mach-imx/suspend-imx6.S > > > @@ -327,17 +327,3 @@ resume: > > > > > > ret lr > > > ENDPROC(imx6_suspend) > > > - > > > -/* > > > - * The following code must assume it is running from physical address > > > - * where absolute virtual addresses to the data section have to be > > > - * turned into relative ones. > > > - */ > > > - > > > -ENTRY(v7_cpu_resume) > > > - bl v7_invalidate_l1 > > > -#ifdef CONFIG_CACHE_L2X0 > > > - bl l2c310_early_resume > > > -#endif > > > - b cpu_resume > > > -ENDPROC(v7_cpu_resume) > > > -- > > > 2.25.0 > > > > This patch broke the build for our i.MX6 kernel. > > > > I am referring to commits 512a928aff in mainline and > > 7199cb65bb in linux-stable. > > > > In our kernel, neither CONFIG_PM nor CONFIG_SUSPEND are set. Therefore, > > ARM_CPU_SUSPEND is also unset, which means that sleep.S (containing > > cpu_resume) is not built. > > > > With this patch, ld reports the following error: > > arch/arm/mach-imx/resume-imx6.o: in function `v7_cpu_resume': > > (.text+0x8): undefined reference to `cpu_resume' > > Is this a problem with Linus's tree as well? Yes, it affects Linus's tree and linux-stable. Reverting 512a928aff in mainline (7199cb65bb in linux-stable) fixes the build error for us. Thanks, Clemens ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ARM: imx: build v7_cpu_resume() unconditionally 2020-03-22 23:09 ` Clemens Gruber @ 2020-03-23 8:22 ` Ahmad Fatoum 0 siblings, 0 replies; 4+ messages in thread From: Ahmad Fatoum @ 2020-03-23 8:22 UTC (permalink / raw) To: Sasha Levin Cc: festevam, s.hauer, linux-kernel, stable, linux-imx, kernel, Greg Kroah-Hartman, Rouven Czerwinski, shawnguo, linux-arm-kernel, Lucas Stach, Clemens Gruber Hello Clemens On 3/23/20 12:09 AM, Clemens Gruber wrote: >> Is this a problem with Linus's tree as well? > > Yes, it affects Linus's tree and linux-stable. > > Reverting 512a928aff in mainline (7199cb65bb in linux-stable) fixes the > build error for us. Thanks for the report and sorry for the inconvenience. I just sent out a patch[1] to fix the linker error. [1]: https://lore.kernel.org/linux-arm-kernel/20200323081933.31497-1-a.fatoum@pengutronix.de/T/#u Cheers Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-23 8:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200116141849.73955-1-r.czerwinski@pengutronix.de>
2020-03-22 18:50 ` [PATCH] ARM: imx: build v7_cpu_resume() unconditionally Clemens Gruber
2020-03-22 20:22 ` Sasha Levin
2020-03-22 23:09 ` Clemens Gruber
2020-03-23 8:22 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).