Make Tegra cpuidle driver PSCI compatible

PATCH New Jan Kiszka 4 participants 11 messages Sep 23, 2015 – Nov 12, 2015

Subsystems (3)

Arm (32-bit) Arm sub-architectures Tegra architecture support

lore

Share & tools

lore

Jan Kiszka

author

2015-09-23 06:39 UTC #0 lore raw

v2 cover Make Tegra cpuidle driver PSCI compatible

Seems those two fell under the table. So I'm resending, rebased over current 4.3-rc2, resolving a conflict of patch 1.

Jan

Jan Kiszka (2):
ARM: Availability of psci_smp_available depends on CONFIG_SMP
ARM: tegra: Disable cpuidle driver if PSCI is present

 arch/arm/include/asm/psci.h | 2 +-
 arch/arm/mach-tegra/Kconfig | 1 +
 arch/arm/mach-tegra/tegra.c | 4 +++-
 3 files changed, 5 insertions(+), 2 deletions(-)

Signature · 1 line

2.1.4

Jan Kiszka

author

2015-09-23 06:39 UTC #1 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/include/asm/psci.h +1 -1

index 68ee3ce..ff956f4 100644
@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 
-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }

Signature · 1 line

2.1.4

Thierry Reding

2015-10-06 08:11 UTC #2 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Quoted 13 lines · Jan Kiszka

Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/psci.h +0 -0
index 68ee3ce..ff956f4 100644

arch/arm/include/asm/psci.h line 22

Quoted 8 lines

arch/arm/include/asm/psci.h +1 -1

@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 
-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }

Hi Will,

you had questions about this when this was first submitted back in May[0], but discussion stalled. Can you take another look, please?

I think irrespective of what the series is trying to do this is a correct fix. The arch/arm/kernel/psci_smp.c file is only compiled if both ARM_PSCI and SMP are selected. Builds break if we don't mirror that conditional in the header because for ARM_PSCI && !SMP no dummy will be defined, but the implementation for the prototype won't be available either, leading to a linker error.

Also Cc'ing Sebastian who sent this same patch yesterday.

[0]: http://patchwork.ozlabs.org/patch/469878/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151006/cda6c441/attachment.sig>

Will Deacon

2015-10-06 15:18 UTC #3 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Quoted 17 lines · Thierry Reding

Quoted 16 lines · Jan Kiszka
Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/psci.h +0 -0
index 68ee3ce..ff956f4 100644
@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 

Quoted 7 lines

arch/arm/include/asm/psci.h line 22

Quoted 5 lines

arch/arm/include/asm/psci.h +1 -1

-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }

Hi Will,

Hi Thierry,

Quoted 9 lines

you had questions about this when this was first submitted back in May[0], but discussion stalled. Can you take another look, please?
I think irrespective of what the series is trying to do this is a correct fix. The arch/arm/kernel/psci_smp.c file is only compiled if both ARM_PSCI and SMP are selected. Builds break if we don't mirror that conditional in the header because for ARM_PSCI && !SMP no dummy will be defined, but the implementation for the prototype won't be available either, leading to a linker error.

Sure, I'm fine with this patch in isolation, I just didn't (don't) fully grok what the series is trying to achieve.

Will

Thierry Reding

2015-10-08 16:04 UTC #4 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Quoted 38 lines · Will Deacon

Quoted 17 lines · Thierry Reding
Quoted 16 lines · Jan Kiszka
Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/psci.h +0 -0
index 68ee3ce..ff956f4 100644
@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 
Quoted 7 lines
arch/arm/include/asm/psci.h line 22
Quoted 5 lines
arch/arm/include/asm/psci.h +1 -1
-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }
Hi Will,
Hi Thierry,
Quoted 9 lines
you had questions about this when this was first submitted back in May[0], but discussion stalled. Can you take another look, please?
I think irrespective of what the series is trying to do this is a correct fix. The arch/arm/kernel/psci_smp.c file is only compiled if both ARM_PSCI and SMP are selected. Builds break if we don't mirror that conditional in the header because for ARM_PSCI && !SMP no dummy will be defined, but the implementation for the prototype won't be available either, leading to a linker error.

Quoted 2 lines

Sure, I'm fine with this patch in isolation, I just didn't (don't) fully grok what the series is trying to achieve.

The goal is to prevent the kernel from registering a CPU idle driver if PSCI is going to be used for SMP. This is necessary because both the CPU idle driver and the PSCI implementation (provided by U-Boot in this case) access the same resources. The effect of having both enabled is usually that the system will simply hang sometime during boot.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151008/586b161c/attachment-0001.sig>

Sebastian Andrzej Siewior

2015-11-12 09:43 UTC #5 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

In reply to Will Deacon

Quoted 18 lines · Will Deacon

Quoted 17 lines · Thierry Reding
Quoted 16 lines · Jan Kiszka
Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/psci.h +0 -0
index 68ee3ce..ff956f4 100644
@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 

Quoted 8 lines

Quoted 5 lines
arch/arm/include/asm/psci.h line 22
Quoted 5 lines
arch/arm/include/asm/psci.h +1 -1
-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }
Sure, I'm fine with this patch in isolation, I just didn't (don't) fully grok what the series is trying to achieve.

Will, do you want this patch re-sent or do you just need some time to pick it up?

Quoted 1 line

Will

Sebastian

Will Deacon

2015-11-12 09:49 UTC #6 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Quoted 28 lines · Sebastian Andrzej Siewior

Quoted 18 lines · Will Deacon
Quoted 17 lines · Thierry Reding
Quoted 16 lines · Jan Kiszka
Ensure that we can use psci_smp_available without checking for CONFIG_SMP first.
Signed-off-by Jan Kiszka <[email protected]>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/psci.h +0 -0
index 68ee3ce..ff956f4 100644
@@ -16,7 +16,7 @@
 
 extern struct smp_operations psci_smp_ops;
 
Quoted 8 lines
Quoted 5 lines
arch/arm/include/asm/psci.h line 22
Quoted 5 lines
arch/arm/include/asm/psci.h +1 -1
-#ifdef CONFIG_ARM_PSCI
+#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_SMP)
 bool psci_smp_available(void);
 #else
 static inline bool psci_smp_available(void) { return false; }
Sure, I'm fine with this patch in isolation, I just didn't (don't) fully grok what the series is trying to achieve.

Quoted 2 lines

Will, do you want this patch re-sent or do you just need some time to pick it up?

I wasn't planning to pick this one up as I assumed you'd be taking it via arm-soc along with the second patch that depends on it. I just stopped moaning about not understanding it :)

Will

Sebastian Andrzej Siewior

2015-11-12 10:07 UTC #7 lore raw

PATCH v2 1/2 ARM: Availability of psci_smp_available depends on CONFIG_SMP

Quoted 3 lines · Will Deacon

I wasn't planning to pick this one up as I assumed you'd be taking it via arm-soc along with the second patch that depends on it. I just stopped moaning about not understanding it :)

Ah, okay, thanks for the update. I just reposted it and hope for the best.

Quoted 1 line

Will

Sebastian

Jan Kiszka

author

2015-09-23 06:39 UTC #8 lore raw

PATCH v2 2/2 ARM: tegra: Disable cpuidle driver if PSCI is present

In reply to Jan Kiszka

The firmware takes over the ownership of the flow controller when it provides PSCI support. To avoid clashes, disable the Tegra cpuidle driver if PSCI was detected.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/arm/mach-tegra/Kconfig | 1 +
 arch/arm/mach-tegra/tegra.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

arch/arm/mach-tegra/Kconfig +1 -0

index 0fa4c5f..373768c 100644
@@ -4,6 +4,7 @@ menuconfig ARCH_TEGRA
 	select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
 	select ARM_AMBA
 	select ARM_GIC
+	select ARM_PSCI
 	select CLKSRC_MMIO
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP

arch/arm/mach-tegra/tegra.c +3 -1

index 2378fa56..d97c961 100644
@@ -44,6 +44,7 @@
 #include <asm/mach-types.h>
 #include <asm/setup.h>
 #include <asm/trusted_foundations.h>
+#include <asm/psci.h>
 
 #include "board.h"
 #include "common.h"

@@ -136,7 +137,8 @@ static void __init tegra_dt_init_late(void)
 	int i;
 
 	tegra_init_suspend();
-	tegra_cpuidle_init();
+	if (!psci_smp_available())
+		tegra_cpuidle_init();
 
 	for (i = 0; i < ARRAY_SIZE(board_init_funcs); i++) {
 		if (of_machine_is_compatible(board_init_funcs[i].machine)) {

Signature · 1 line

2.1.4

Thierry Reding

2015-10-06 08:21 UTC #9 lore raw

PATCH v2 2/2 ARM: tegra: Disable cpuidle driver if PSCI is present

Quoted 35 lines · Jan Kiszka

The firmware takes over the ownership of the flow controller when it provides PSCI support. To avoid clashes, disable the Tegra cpuidle driver if PSCI was detected.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/arm/mach-tegra/Kconfig | 1 +
 arch/arm/mach-tegra/tegra.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

arch/arm/mach-tegra/Kconfig +1 -0

index 0fa4c5f..373768c 100644
@@ -4,6 +4,7 @@ menuconfig ARCH_TEGRA
 	select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
 	select ARM_AMBA
 	select ARM_GIC
+	select ARM_PSCI
 	select CLKSRC_MMIO
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP

arch/arm/mach-tegra/tegra.c +1 -0

index 2378fa56..d97c961 100644
@@ -44,6 +44,7 @@
 #include <asm/mach-types.h>
 #include <asm/setup.h>
 #include <asm/trusted_foundations.h>
+#include <asm/psci.h>
 
 #include "board.h"
 #include "common.h"

arch/arm/mach-tegra/tegra.c static void __init tegra_dt_init_late(void) · line 144

Quoted 9 lines

arch/arm/mach-tegra/tegra.c +2 -1

@@ -136,7 +137,8 @@ static void __init tegra_dt_init_late(void)
 	int i;
 
 	tegra_init_suspend();
-	tegra_cpuidle_init();
+	if (!psci_smp_available())
+		tegra_cpuidle_init();
 
 	for (i = 0; i < ARRAY_SIZE(board_init_funcs); i++) {
 		if (of_machine_is_compatible(board_init_funcs[i].machine)) {

The equivalent of this part is already in cpuidle-tegra114.c and I'm not sure this adds anything. cpuidle-tegra114.c is the same code that is used on Tegra124. Anything before Tegra114 is now unlikely to gain PSCI support. Given that they're Cortex-A9 and lack virtualization extensions I don't think it's even possible to support PSCI on them. Anything after Tegra124 will very likely be 64-bit and hence require firmware support (such as PSCI) for CPU idle anyway.

That still leaves the select ARM_PSCI which we don't have in mainline yet. I'm also not sure if select ARM_PSCI is the right approach here. It means that everyone will be forced to have PSCI support in the kernel. But what if for some reason you want to disable PSCI (and instead use the kernel's cpuidle support)? Maybe it'd be better to add this to the tegra_defconfig instead.

Jan Kiszka

author

2015-10-06 08:36 UTC #10 lore raw

PATCH v2 2/2 ARM: tegra: Disable cpuidle driver if PSCI is present

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 28 lines · Thierry Reding

Quoted 27 lines · Jan Kiszka
The firmware takes over the ownership of the flow controller when it provides PSCI support. To avoid clashes, disable the Tegra cpuidle driver if PSCI was detected.
Signed-off-by Jan Kiszka <[email protected]> ---
arch/arm/mach-tegra/Kconfig | 1 + arch/arm/mach-tegra/tegra.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/arch/arm/mach-tegra/Kconfig index 0fa4c5f..373768c 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig
(quoted diff) +0 -0
@@ -4,6 +4,7 @@ menuconfig ARCH_TEGRA select
ARCH_SUPPORTS_TRUSTED_FOUNDATIONS select ARM_AMBA select ARM_GIC
(quoted diff) +1 -0
+	select ARM_PSCI select CLKSRC_MMIO select HAVE_ARM_SCU if SMP 
select HAVE_ARM_TWD if SMP diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c index 2378fa56..d97c961 100644 --- a/arch/arm/mach-tegra/tegra.c +++ b/arch/arm/mach-tegra/tegra.c @@ -44,6 +44,7 @@ #include <asm/mach-types.h> #include <asm/setup.h> #include <asm/trusted_foundations.h> +#include <asm/psci.h>
#include "board.h" #include "common.h" @@ -136,7 +137,8 @@ static void __init tegra_dt_init_late(void) int i;
tegra_init_suspend(); - tegra_cpuidle_init(); + if
(!psci_smp_available()) + tegra_cpuidle_init();

Quoted 11 lines

Quoted 2 lines
for (i = 0; i < ARRAY_SIZE(board_init_funcs); i++) { if (of_machine_is_compatible(board_init_funcs[i].machine)) {
The equivalent of this part is already in cpuidle-tegra114.c and I'm not sure this adds anything. cpuidle-tegra114.c is the same code that is used on Tegra124. Anything before Tegra114 is now unlikely to gain PSCI support. Given that they're Cortex-A9 and lack virtualization extensions I don't think it's even possible to support PSCI on them. Anything after Tegra124 will very likely be 64-bit and hence require firmware support (such as PSCI) for CPU idle anyway.

Oh, missed that change.

Quoted 7 lines

That still leaves the select ARM_PSCI which we don't have in mainline yet. I'm also not sure if select ARM_PSCI is the right approach here. It means that everyone will be forced to have PSCI support in the kernel. But what if for some reason you want to disable PSCI (and instead use the kernel's cpuidle support)? Maybe it'd be better to add this to the tegra_defconfig instead.

Would be fine with me as well.

Jan

- --
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux -----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlYTiAYACgkQitSsb3rl5xSJnQCg7IRy0ju/r0Ka70WCWYRZPGDR Z1MAnRTWStY3BOI4yK8mL7CLYAbumUCD
=uAvw
-----END PGP SIGNATURE-----