* 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).