* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
@ 2014-12-09 11:48 Krzysztof Kozlowski
2014-12-09 12:29 ` Arnd Bergmann
0 siblings, 1 reply; 7+ messages in thread
From: Krzysztof Kozlowski @ 2014-12-09 11:48 UTC (permalink / raw)
To: linux-arm-kernel
Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
disabling SUSPEND) and CPU_IDLE enabled:
arch/arm64/kernel/psci.c:543:2: error: unknown field ?cpu_suspend? specified in initializer
.cpu_suspend = cpu_psci_cpu_suspend,
^
arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
arch/arm64/kernel/psci.c:543:2: warning: (near initialization for ?cpu_psci_ops.cpu_prepare?) [enabled by default]
make[1]: *** [arch/arm64/kernel/psci.o] Error 1
The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
defined, not CPU_IDLE.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
arch/arm64/kernel/psci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 3425f311c49e..f1dbca7d5c96 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -540,6 +540,8 @@ const struct cpu_operations cpu_psci_ops = {
.name = "psci",
#ifdef CONFIG_CPU_IDLE
.cpu_init_idle = cpu_psci_cpu_init_idle,
+#endif
+#ifdef CONFIG_ARM64_CPU_SUSPEND
.cpu_suspend = cpu_psci_cpu_suspend,
#endif
#ifdef CONFIG_SMP
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-09 11:48 [PATCH] arm64: psci: Fix build breakage without PM_SLEEP Krzysztof Kozlowski
@ 2014-12-09 12:29 ` Arnd Bergmann
2014-12-09 12:38 ` Krzysztof Kozlowski
0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2014-12-09 12:29 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> disabling SUSPEND) and CPU_IDLE enabled:
>
> arch/arm64/kernel/psci.c:543:2: error: unknown field ?cpu_suspend? specified in initializer
> .cpu_suspend = cpu_psci_cpu_suspend,
> ^
> arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> arch/arm64/kernel/psci.c:543:2: warning: (near initialization for ?cpu_psci_ops.cpu_prepare?) [enabled by default]
> make[1]: *** [arch/arm64/kernel/psci.o] Error 1
>
> The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> defined, not CPU_IDLE.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
No objection to fixing this obvious build bug, but why do we even have
an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
because we have a random collection of platform specific drivers that
use the symbols, but that's not the case on ARM64.
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-09 12:29 ` Arnd Bergmann
@ 2014-12-09 12:38 ` Krzysztof Kozlowski
2014-12-09 16:15 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Krzysztof Kozlowski @ 2014-12-09 12:38 UTC (permalink / raw)
To: linux-arm-kernel
On wto, 2014-12-09 at 13:29 +0100, Arnd Bergmann wrote:
> On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> > Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> > disabling SUSPEND) and CPU_IDLE enabled:
> >
> > arch/arm64/kernel/psci.c:543:2: error: unknown field ?cpu_suspend? specified in initializer
> > .cpu_suspend = cpu_psci_cpu_suspend,
> > ^
> > arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> > arch/arm64/kernel/psci.c:543:2: warning: (near initialization for ?cpu_psci_ops.cpu_prepare?) [enabled by default]
> > make[1]: *** [arch/arm64/kernel/psci.o] Error 1
> >
> > The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> > defined, not CPU_IDLE.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >
>
> No objection to fixing this obvious build bug, but why do we even have
> an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
> because we have a random collection of platform specific drivers that
> use the symbols, but that's not the case on ARM64.
I believe because of cpuidle. It's the same as on ARM32: the cpu_suspend
is used by both PM_SLEEP and CPU_IDLE.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-09 12:38 ` Krzysztof Kozlowski
@ 2014-12-09 16:15 ` Catalin Marinas
2014-12-12 15:06 ` Lorenzo Pieralisi
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2014-12-09 16:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 09, 2014 at 12:38:09PM +0000, Krzysztof Kozlowski wrote:
> On wto, 2014-12-09 at 13:29 +0100, Arnd Bergmann wrote:
> > On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> > > Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> > > disabling SUSPEND) and CPU_IDLE enabled:
> > >
> > > arch/arm64/kernel/psci.c:543:2: error: unknown field ?cpu_suspend? specified in initializer
> > > .cpu_suspend = cpu_psci_cpu_suspend,
> > > ^
> > > arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> > > arch/arm64/kernel/psci.c:543:2: warning: (near initialization for ?cpu_psci_ops.cpu_prepare?) [enabled by default]
> > > make[1]: *** [arch/arm64/kernel/psci.o] Error 1
> > >
> > > The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> > > defined, not CPU_IDLE.
> > >
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > >
> >
> > No objection to fixing this obvious build bug, but why do we even have
> > an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
> > because we have a random collection of platform specific drivers that
> > use the symbols, but that's not the case on ARM64.
>
> I believe because of cpuidle. It's the same as on ARM32: the cpu_suspend
> is used by both PM_SLEEP and CPU_IDLE.
I guess at some point we can replace (as a separate patch)
ARM64_CPU_SUSPEND with PM_SLEEP.
But what I don't fully understand, we can enable CPU_IDLE without
ARM64_CPU_SUSPEND. However, the cpuidle-arm64.c driver will fail to
link since it calls cpu_suspend(). Wouldn't it be better if
ARM64_CPU_SUSPEND depends on CPU_PM (or replaced by it) rather than
PM_SLEEP?
Can we allow deeper idle states when CONFIG_SUSPEND is disabled? I see
CONFIG_SUSPEND related to suspend-to-RAM (system standby) rather than
CPU idle, in which case we may want to allow cpu_suspend when only
CPU_IDLE is enabled (which implies CONFIG_CPU_PM).
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-09 16:15 ` Catalin Marinas
@ 2014-12-12 15:06 ` Lorenzo Pieralisi
2014-12-15 17:46 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Lorenzo Pieralisi @ 2014-12-12 15:06 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 09, 2014 at 04:15:11PM +0000, Catalin Marinas wrote:
> On Tue, Dec 09, 2014 at 12:38:09PM +0000, Krzysztof Kozlowski wrote:
> > On wto, 2014-12-09 at 13:29 +0100, Arnd Bergmann wrote:
> > > On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> > > > Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> > > > disabling SUSPEND) and CPU_IDLE enabled:
> > > >
> > > > arch/arm64/kernel/psci.c:543:2: error: unknown field 'cpu_suspend' specified in initializer
> > > > .cpu_suspend = cpu_psci_cpu_suspend,
> > > > ^
> > > > arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> > > > arch/arm64/kernel/psci.c:543:2: warning: (near initialization for 'cpu_psci_ops.cpu_prepare') [enabled by default]
> > > > make[1]: *** [arch/arm64/kernel/psci.o] Error 1
> > > >
> > > > The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> > > > defined, not CPU_IDLE.
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > >
> > >
> > > No objection to fixing this obvious build bug, but why do we even have
> > > an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
> > > because we have a random collection of platform specific drivers that
> > > use the symbols, but that's not the case on ARM64.
> >
> > I believe because of cpuidle. It's the same as on ARM32: the cpu_suspend
> > is used by both PM_SLEEP and CPU_IDLE.
>
> I guess at some point we can replace (as a separate patch)
> ARM64_CPU_SUSPEND with PM_SLEEP.
>
> But what I don't fully understand, we can enable CPU_IDLE without
> ARM64_CPU_SUSPEND. However, the cpuidle-arm64.c driver will fail to
> link since it calls cpu_suspend(). Wouldn't it be better if
> ARM64_CPU_SUSPEND depends on CPU_PM (or replaced by it) rather than
> PM_SLEEP?
I think that ARM64_CPU_SUSPEND should depend on PM_SLEEP || CPU_IDLE,
if CPU_IDLE is enabled it is certainly because some idle states are
expected to be present, true, not all of them lose context (which is
why ARM64_CPU_SUSPEND is needed, to save/restore context and clean
it to RAM), but I think that's too fine grain, making it depend on
CPU_IDLE should be ok.
Having CPU_IDLE enabled without arm64 cpuidle driver enabled (which
selects ARM64_CPU_SUSPEND) is useless at the moment.
As to cpu_ops, I think that the suspend hook should ifdef on CPU_IDLE,
but I have to wait and see how we implement S2R to make it a final
decision.
> Can we allow deeper idle states when CONFIG_SUSPEND is disabled? I see
> CONFIG_SUSPEND related to suspend-to-RAM (system standby) rather than
> CPU idle, in which case we may want to allow cpu_suspend when only
> CPU_IDLE is enabled (which implies CONFIG_CPU_PM).
Yes, deep idle states are enabled even when suspend is disabled, but
both S2R and CPU_IDLE should turn on ARM64_CPU_SUSPEND to save/restore
context, unless we remove ARM64_CPU_SUSPEND entirely and we always
compile that code in.
I will put together a patch first thing next week to clarify this
thread.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-12 15:06 ` Lorenzo Pieralisi
@ 2014-12-15 17:46 ` Catalin Marinas
2014-12-15 18:32 ` Lorenzo Pieralisi
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2014-12-15 17:46 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Dec 12, 2014 at 03:06:08PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Dec 09, 2014 at 04:15:11PM +0000, Catalin Marinas wrote:
> > On Tue, Dec 09, 2014 at 12:38:09PM +0000, Krzysztof Kozlowski wrote:
> > > On wto, 2014-12-09 at 13:29 +0100, Arnd Bergmann wrote:
> > > > On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> > > > > Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> > > > > disabling SUSPEND) and CPU_IDLE enabled:
> > > > >
> > > > > arch/arm64/kernel/psci.c:543:2: error: unknown field 'cpu_suspend' specified in initializer
> > > > > .cpu_suspend = cpu_psci_cpu_suspend,
> > > > > ^
> > > > > arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> > > > > arch/arm64/kernel/psci.c:543:2: warning: (near initialization for 'cpu_psci_ops.cpu_prepare') [enabled by default]
> > > > > make[1]: *** [arch/arm64/kernel/psci.o] Error 1
> > > > >
> > > > > The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> > > > > defined, not CPU_IDLE.
> > > > >
> > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > > >
> > > >
> > > > No objection to fixing this obvious build bug, but why do we even have
> > > > an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
> > > > because we have a random collection of platform specific drivers that
> > > > use the symbols, but that's not the case on ARM64.
> > >
> > > I believe because of cpuidle. It's the same as on ARM32: the cpu_suspend
> > > is used by both PM_SLEEP and CPU_IDLE.
> >
> > I guess at some point we can replace (as a separate patch)
> > ARM64_CPU_SUSPEND with PM_SLEEP.
> >
> > But what I don't fully understand, we can enable CPU_IDLE without
> > ARM64_CPU_SUSPEND. However, the cpuidle-arm64.c driver will fail to
> > link since it calls cpu_suspend(). Wouldn't it be better if
> > ARM64_CPU_SUSPEND depends on CPU_PM (or replaced by it) rather than
> > PM_SLEEP?
>
> I think that ARM64_CPU_SUSPEND should depend on PM_SLEEP || CPU_IDLE,
That's what we do with CPU_PM, we select it if SUSPEND || CPU_IDLE
(PM_SLEEP is default yes if SUSPEND).
> if CPU_IDLE is enabled it is certainly because some idle states are
> expected to be present, true, not all of them lose context (which is
> why ARM64_CPU_SUSPEND is needed, to save/restore context and clean
> it to RAM), but I think that's too fine grain, making it depend on
> CPU_IDLE should be ok.
>
> Having CPU_IDLE enabled without arm64 cpuidle driver enabled (which
> selects ARM64_CPU_SUSPEND) is useless at the moment.
Ah, so we can force a selection even if it doesn't meet its
dependencies like PM_SLEEP (which depends on SUSPEND).
Can we just get rid of ARM64_CPU_SUSPEND altogether and use CPU_PM
instead?
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: psci: Fix build breakage without PM_SLEEP
2014-12-15 17:46 ` Catalin Marinas
@ 2014-12-15 18:32 ` Lorenzo Pieralisi
0 siblings, 0 replies; 7+ messages in thread
From: Lorenzo Pieralisi @ 2014-12-15 18:32 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Dec 15, 2014 at 05:46:22PM +0000, Catalin Marinas wrote:
> On Fri, Dec 12, 2014 at 03:06:08PM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Dec 09, 2014 at 04:15:11PM +0000, Catalin Marinas wrote:
> > > On Tue, Dec 09, 2014 at 12:38:09PM +0000, Krzysztof Kozlowski wrote:
> > > > On wto, 2014-12-09 at 13:29 +0100, Arnd Bergmann wrote:
> > > > > On Tuesday 09 December 2014 12:48:36 Krzysztof Kozlowski wrote:
> > > > > > Fix build failure of defconfig when PM_SLEEP is disabled (e.g. by
> > > > > > disabling SUSPEND) and CPU_IDLE enabled:
> > > > > >
> > > > > > arch/arm64/kernel/psci.c:543:2: error: unknown field 'cpu_suspend' specified in initializer
> > > > > > .cpu_suspend = cpu_psci_cpu_suspend,
> > > > > > ^
> > > > > > arch/arm64/kernel/psci.c:543:2: warning: initialization from incompatible pointer type [enabled by default]
> > > > > > arch/arm64/kernel/psci.c:543:2: warning: (near initialization for 'cpu_psci_ops.cpu_prepare') [enabled by default]
> > > > > > make[1]: *** [arch/arm64/kernel/psci.o] Error 1
> > > > > >
> > > > > > The cpu_operations.cpu_suspend field exists only if ARM64_CPU_SUSPEND is
> > > > > > defined, not CPU_IDLE.
> > > > > >
> > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > > > >
> > > > >
> > > > > No objection to fixing this obvious build bug, but why do we even have
> > > > > an ARM64_CPU_SUSPEND option? On ARM32 we only have the respective option
> > > > > because we have a random collection of platform specific drivers that
> > > > > use the symbols, but that's not the case on ARM64.
> > > >
> > > > I believe because of cpuidle. It's the same as on ARM32: the cpu_suspend
> > > > is used by both PM_SLEEP and CPU_IDLE.
> > >
> > > I guess at some point we can replace (as a separate patch)
> > > ARM64_CPU_SUSPEND with PM_SLEEP.
> > >
> > > But what I don't fully understand, we can enable CPU_IDLE without
> > > ARM64_CPU_SUSPEND. However, the cpuidle-arm64.c driver will fail to
> > > link since it calls cpu_suspend(). Wouldn't it be better if
> > > ARM64_CPU_SUSPEND depends on CPU_PM (or replaced by it) rather than
> > > PM_SLEEP?
> >
> > I think that ARM64_CPU_SUSPEND should depend on PM_SLEEP || CPU_IDLE,
>
> That's what we do with CPU_PM, we select it if SUSPEND || CPU_IDLE
> (PM_SLEEP is default yes if SUSPEND).
>
> > if CPU_IDLE is enabled it is certainly because some idle states are
> > expected to be present, true, not all of them lose context (which is
> > why ARM64_CPU_SUSPEND is needed, to save/restore context and clean
> > it to RAM), but I think that's too fine grain, making it depend on
> > CPU_IDLE should be ok.
> >
> > Having CPU_IDLE enabled without arm64 cpuidle driver enabled (which
> > selects ARM64_CPU_SUSPEND) is useless at the moment.
>
> Ah, so we can force a selection even if it doesn't meet its
> dependencies like PM_SLEEP (which depends on SUSPEND).
>
> Can we just get rid of ARM64_CPU_SUSPEND altogether and use CPU_PM
> instead?
Yes, it looks like the best option to me, I will put together a
patch shortly.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-12-15 18:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-09 11:48 [PATCH] arm64: psci: Fix build breakage without PM_SLEEP Krzysztof Kozlowski
2014-12-09 12:29 ` Arnd Bergmann
2014-12-09 12:38 ` Krzysztof Kozlowski
2014-12-09 16:15 ` Catalin Marinas
2014-12-12 15:06 ` Lorenzo Pieralisi
2014-12-15 17:46 ` Catalin Marinas
2014-12-15 18:32 ` Lorenzo Pieralisi
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).