diff mbox

[06/11] ARM: tegra114: hook tegra_tear_down_cpu function

Message ID 1372152475-18617-7-git-send-email-josephl@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joseph Lo June 25, 2013, 9:27 a.m. UTC
Hooking tegra_tear_down_cpu for Tegra114 for supporting cluster power
down when CPU cluster suspneded in LP2.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/pm.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Thierry Reding June 27, 2013, 10:13 a.m. UTC | #1
On Tue, Jun 25, 2013 at 05:27:50PM +0800, Joseph Lo wrote:
> Hooking tegra_tear_down_cpu for Tegra114 for supporting cluster power
> down when CPU cluster suspneded in LP2.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
>  arch/arm/mach-tegra/pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> index 94e69be..a0668a2 100644
> --- a/arch/arm/mach-tegra/pm.c
> +++ b/arch/arm/mach-tegra/pm.c
> @@ -52,6 +52,7 @@ static void tegra_tear_down_cpu_init(void)
>  			tegra_tear_down_cpu = tegra20_tear_down_cpu;
>  		break;
>  	case TEGRA30:
> +	case TEGRA114:
>  		if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC))
>  			tegra_tear_down_cpu = tegra30_tear_down_cpu;

This is getting a little weird. Suppose I want to build a Tegra114 only
kernel. With the above code it means the tegra_tear_down_cpu won't be
hooked because ARCH_TEGRA_3x_SOC isn't selected.

Perhaps tegra30_tear_down_cpu() should be built unconditionally so that
it's always available? I suspect that something similar will need to be
done for future chips too, further complicating matters.

There are other alternatives like adding another Kconfig symbol which
doesn't cover all of Tegra30 but only code shared with Tegra114 (and
possible future chips) or building sleep-tegra30.S if either one of the
ARCH_TEGRA_3x_SOC or ARCH_TEGRA_114_SOC symbols is selected.

Thierry
Joseph Lo June 27, 2013, 10:22 a.m. UTC | #2
On Thu, 2013-06-27 at 18:13 +0800, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 25, 2013 at 05:27:50PM +0800, Joseph Lo wrote:
> > Hooking tegra_tear_down_cpu for Tegra114 for supporting cluster power
> > down when CPU cluster suspneded in LP2.
> > 
> > Signed-off-by: Joseph Lo <josephl@nvidia.com>
> > ---
> >  arch/arm/mach-tegra/pm.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> > index 94e69be..a0668a2 100644
> > --- a/arch/arm/mach-tegra/pm.c
> > +++ b/arch/arm/mach-tegra/pm.c
> > @@ -52,6 +52,7 @@ static void tegra_tear_down_cpu_init(void)
> >  			tegra_tear_down_cpu = tegra20_tear_down_cpu;
> >  		break;
> >  	case TEGRA30:
> > +	case TEGRA114:
> >  		if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC))
> >  			tegra_tear_down_cpu = tegra30_tear_down_cpu;
> 
> This is getting a little weird. Suppose I want to build a Tegra114 only
> kernel. With the above code it means the tegra_tear_down_cpu won't be
> hooked because ARCH_TEGRA_3x_SOC isn't selected.
> 
Oops. You are right. Will fix.

> Perhaps tegra30_tear_down_cpu() should be built unconditionally so that
> it's always available?
The tegra30_tear_down_cpu will be built for both Tegra30 or Tegra114.

> 
> There are other alternatives like adding another Kconfig symbol which
> doesn't cover all of Tegra30 but only code shared with Tegra114 (and
> possible future chips) or building sleep-tegra30.S if either one of the
> ARCH_TEGRA_3x_SOC or ARCH_TEGRA_114_SOC symbols is selected.
> 
Yes, we building "sleep-tegra30.S" when either one of the
ARCH_TEGRA_3x_SOC or ARCH_TEGRA_114_SOC symbols is selected.

Thanks,
Joseph
Stephen Warren June 27, 2013, 5:48 p.m. UTC | #3
On 06/27/2013 04:13 AM, Thierry Reding wrote:
> On Tue, Jun 25, 2013 at 05:27:50PM +0800, Joseph Lo wrote:
>> Hooking tegra_tear_down_cpu for Tegra114 for supporting cluster
>> power down when CPU cluster suspneded in LP2.
>> 
>> Signed-off-by: Joseph Lo <josephl@nvidia.com> --- 
>> arch/arm/mach-tegra/pm.c | 1 + 1 file changed, 1 insertion(+)
>> 
>> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c 
>> index 94e69be..a0668a2 100644 --- a/arch/arm/mach-tegra/pm.c +++
>> b/arch/arm/mach-tegra/pm.c @@ -52,6 +52,7 @@ static void
>> tegra_tear_down_cpu_init(void) tegra_tear_down_cpu =
>> tegra20_tear_down_cpu; break; case TEGRA30: +	case TEGRA114: if
>> (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC)) tegra_tear_down_cpu =
>> tegra30_tear_down_cpu;
> 
> This is getting a little weird. Suppose I want to build a Tegra114
> only kernel. With the above code it means the tegra_tear_down_cpu
> won't be hooked because ARCH_TEGRA_3x_SOC isn't selected.
> 
> Perhaps tegra30_tear_down_cpu() should be built unconditionally so
> that it's always available? I suspect that something similar will
> need to be done for future chips too, further complicating
> matters.
> 
> There are other alternatives like adding another Kconfig symbol
> which doesn't cover all of Tegra30 but only code shared with
> Tegra114 (and possible future chips) or building sleep-tegra30.S if
> either one of the ARCH_TEGRA_3x_SOC or ARCH_TEGRA_114_SOC symbols
> is selected.

To be honest, I wonder if we should just get rid of ARCH_TEGRA_*_SOC,
and build everything if ARCH_TEGRA is defined. tegra_defconfig enables
all ARCH_TEGRA_*_SOC anyway, and I'm afraid I don't test other
configurations very often, and I assume that distros will enable
everything...
Thierry Reding June 28, 2013, 6:34 p.m. UTC | #4
On Thu, Jun 27, 2013 at 11:48:49AM -0600, Stephen Warren wrote:
> On 06/27/2013 04:13 AM, Thierry Reding wrote:
> > On Tue, Jun 25, 2013 at 05:27:50PM +0800, Joseph Lo wrote:
> >> Hooking tegra_tear_down_cpu for Tegra114 for supporting cluster
> >> power down when CPU cluster suspneded in LP2.
> >> 
> >> Signed-off-by: Joseph Lo <josephl@nvidia.com> --- 
> >> arch/arm/mach-tegra/pm.c | 1 + 1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c 
> >> index 94e69be..a0668a2 100644 --- a/arch/arm/mach-tegra/pm.c +++
> >> b/arch/arm/mach-tegra/pm.c @@ -52,6 +52,7 @@ static void
> >> tegra_tear_down_cpu_init(void) tegra_tear_down_cpu =
> >> tegra20_tear_down_cpu; break; case TEGRA30: +	case TEGRA114: if
> >> (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC)) tegra_tear_down_cpu =
> >> tegra30_tear_down_cpu;
> > 
> > This is getting a little weird. Suppose I want to build a Tegra114
> > only kernel. With the above code it means the tegra_tear_down_cpu
> > won't be hooked because ARCH_TEGRA_3x_SOC isn't selected.
> > 
> > Perhaps tegra30_tear_down_cpu() should be built unconditionally so
> > that it's always available? I suspect that something similar will
> > need to be done for future chips too, further complicating
> > matters.
> > 
> > There are other alternatives like adding another Kconfig symbol
> > which doesn't cover all of Tegra30 but only code shared with
> > Tegra114 (and possible future chips) or building sleep-tegra30.S if
> > either one of the ARCH_TEGRA_3x_SOC or ARCH_TEGRA_114_SOC symbols
> > is selected.
> 
> To be honest, I wonder if we should just get rid of ARCH_TEGRA_*_SOC,
> and build everything if ARCH_TEGRA is defined. tegra_defconfig enables
> all ARCH_TEGRA_*_SOC anyway, and I'm afraid I don't test other
> configurations very often, and I assume that distros will enable
> everything...

The same thing had occurred to me as well. Obviously there could be some
savings in executable code for people that really only need one specific
generation. But, without having any concrete numbers, I suspect that all
the ARCH_TEGRA code is less than 100 KiB in total so I don't think it
matters that much given that there aren't any devices with less than 256
MiB of RAM (that I've heard of at least).

Thierry
diff mbox

Patch

diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 94e69be..a0668a2 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -52,6 +52,7 @@  static void tegra_tear_down_cpu_init(void)
 			tegra_tear_down_cpu = tegra20_tear_down_cpu;
 		break;
 	case TEGRA30:
+	case TEGRA114:
 		if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC))
 			tegra_tear_down_cpu = tegra30_tear_down_cpu;
 		break;