[PATCH v2 0/2] Make Tegra cpuidle driver PSCI compatible

[PATCH v2 0/2] Make Tegra cpuidle driver PSCI compatible

[Jan Kiszka]

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(-)

-- 
2.1.4

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

[Jan Kiszka]

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/include/asm/psci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index 68ee3ce..ff956f4 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -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; }
-- 
2.1.4

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

[Thierry Reding]

On Wed, Sep 23, 2015 at 08:39:43AM +0200, Jan Kiszka wrote:
> Ensure that we can use psci_smp_available without checking for
> CONFIG_SMP first.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/include/asm/psci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> index 68ee3ce..ff956f4 100644
> --- a/arch/arm/include/asm/psci.h
> +++ b/arch/arm/include/asm/psci.h
> @@ -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>

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

[Will Deacon]

On Tue, Oct 06, 2015 at 10:11:24AM +0200, Thierry Reding wrote:
> On Wed, Sep 23, 2015 at 08:39:43AM +0200, Jan Kiszka wrote:
> > Ensure that we can use psci_smp_available without checking for
> > CONFIG_SMP first.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  arch/arm/include/asm/psci.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > index 68ee3ce..ff956f4 100644
> > --- a/arch/arm/include/asm/psci.h
> > +++ b/arch/arm/include/asm/psci.h
> > @@ -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,

Hi Thierry,

> 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

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

[Thierry Reding]

On Tue, Oct 06, 2015 at 04:18:58PM +0100, Will Deacon wrote:
> On Tue, Oct 06, 2015 at 10:11:24AM +0200, Thierry Reding wrote:
> > On Wed, Sep 23, 2015 at 08:39:43AM +0200, Jan Kiszka wrote:
> > > Ensure that we can use psci_smp_available without checking for
> > > CONFIG_SMP first.
> > > 
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > >  arch/arm/include/asm/psci.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > > index 68ee3ce..ff956f4 100644
> > > --- a/arch/arm/include/asm/psci.h
> > > +++ b/arch/arm/include/asm/psci.h
> > > @@ -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,
> 
> Hi Thierry,
> 
> > 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.

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>

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

[Sebastian Andrzej Siewior]

On 2015-10-06 16:18:58 [+0100], Will Deacon wrote:
> On Tue, Oct 06, 2015 at 10:11:24AM +0200, Thierry Reding wrote:
> > On Wed, Sep 23, 2015 at 08:39:43AM +0200, Jan Kiszka wrote:
> > > Ensure that we can use psci_smp_available without checking for
> > > CONFIG_SMP first.
> > > 
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > >  arch/arm/include/asm/psci.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > > index 68ee3ce..ff956f4 100644
> > > --- a/arch/arm/include/asm/psci.h
> > > +++ b/arch/arm/include/asm/psci.h
> > > @@ -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; }
> 
> 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?

> Will

Sebastian

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

[Will Deacon]

On Thu, Nov 12, 2015 at 10:43:33AM +0100, Sebastian Andrzej Siewior wrote:
> On 2015-10-06 16:18:58 [+0100], Will Deacon wrote:
> > On Tue, Oct 06, 2015 at 10:11:24AM +0200, Thierry Reding wrote:
> > > On Wed, Sep 23, 2015 at 08:39:43AM +0200, Jan Kiszka wrote:
> > > > Ensure that we can use psci_smp_available without checking for
> > > > CONFIG_SMP first.
> > > > 
> > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > ---
> > > >  arch/arm/include/asm/psci.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > > > index 68ee3ce..ff956f4 100644
> > > > --- a/arch/arm/include/asm/psci.h
> > > > +++ b/arch/arm/include/asm/psci.h
> > > > @@ -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; }
> > 
> > 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?

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

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

[Sebastian Andrzej Siewior]

On 2015-11-12 09:49:20 [+0000], Will Deacon wrote:
> 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.

> Will

Sebastian

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

[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 <jan.kiszka@siemens.com>
---
 arch/arm/mach-tegra/Kconfig | 1 +
 arch/arm/mach-tegra/tegra.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index 0fa4c5f..373768c 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -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
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();
 
 	for (i = 0; i < ARRAY_SIZE(board_init_funcs); i++) {
 		if (of_machine_is_compatible(board_init_funcs[i].machine)) {
-- 
2.1.4

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

[Thierry Reding]

On Wed, Sep 23, 2015 at 08:39:44AM +0200, Jan Kiszka wrote:
> 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 <jan.kiszka@siemens.com>
> ---
>  arch/arm/mach-tegra/Kconfig | 1 +
>  arch/arm/mach-tegra/tegra.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index 0fa4c5f..373768c 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -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
> 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();
>  
>  	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.

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/20151006/1816c6a9/attachment.sig>

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

[Jan Kiszka]

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

On 2015-10-06 09:21, Thierry Reding wrote:
> On Wed, Sep 23, 2015 at 08:39:44AM +0200, Jan Kiszka wrote:
>> 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 <jan.kiszka@siemens.com> --- 
>> arch/arm/mach-tegra/Kconfig | 1 + arch/arm/mach-tegra/tegra.c | 4
>> +++- 2 files changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm/mach-tegra/Kconfig
>> b/arch/arm/mach-tegra/Kconfig index 0fa4c5f..373768c 100644 ---
>> a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig 
>> @@ -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 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();
>> 
>> 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.

> 
> 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-----