| Message ID | 1360308574-19658-2-git-send-email-hdoyu@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Feb 08, 2013 at 09:29:31AM +0200, Hiroshi Doyu wrote: > @@ -56,18 +56,21 @@ int tegra_cpu_disable(unsigned int cpu) > return cpu == 0 ? -EPERM : 0; > } > > -#ifdef CONFIG_ARCH_TEGRA_2x_SOC > -extern void tegra20_hotplug_shutdown(void); > -void __init tegra20_hotplug_init(void) > +void __init tegra_hotplug_init(void) > { > - tegra_hotplug_shutdown = tegra20_hotplug_shutdown; > -} > + switch (tegra_chip_id) { > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > + case TEGRA20: > + tegra_hotplug_shutdown = tegra20_hotplug_shutdown; > + break; > #endif > - > -#ifdef CONFIG_ARCH_TEGRA_3x_SOC > -extern void tegra30_hotplug_shutdown(void); > -void __init tegra30_hotplug_init(void) > -{ > - tegra_hotplug_shutdown = tegra30_hotplug_shutdown; > -} > +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) how about using: #if IS_BUILTIN(CONFIG_ARCH_TEGRA_3x_SOC) instead ? > diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h > index 4ffae54..970ebd5 100644 > --- a/arch/arm/mach-tegra/sleep.h > +++ b/arch/arm/mach-tegra/sleep.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2010-2012, NVIDIA Corporation. All rights reserved. > + * Copyright (c) 2010-2013, NVIDIA Corporation. All rights reserved. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > @@ -124,11 +124,11 @@ int tegra_sleep_cpu_finish(unsigned long); > void tegra_disable_clean_inv_dcache(void); > > #ifdef CONFIG_HOTPLUG_CPU > -void tegra20_hotplug_init(void); > -void tegra30_hotplug_init(void); > +void tegra20_hotplug_shutdown(void); > +void tegra30_hotplug_shutdown(void); aren't these two called only by tegra_hotplug_init() now ? That means they don't need to be exposed.
Hi Felipe, Felipe Balbi <balbi@ti.com> wrote @ Fri, 8 Feb 2013 08:47:20 +0100: > > +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) > > how about using: > > #if IS_BUILTIN(CONFIG_ARCH_TEGRA_3x_SOC) > > instead ? Why is IS_BUILTIN() prefered? > > -void tegra20_hotplug_init(void); > > -void tegra30_hotplug_init(void); > > +void tegra20_hotplug_shutdown(void); > > +void tegra30_hotplug_shutdown(void); > > aren't these two called only by tegra_hotplug_init() now ? Yes > That means they don't need to be exposed. tegra{20,30}_hotplug_shutdown are defined in sleep-tegra{20,30}.S and used in hotplug.c So I think that the above are necessary here.
Hiroshi, Am Freitag, 8. Februar 2013, 09:29:31 schrieb Hiroshi Doyu: > Refactored tegra{20,30,114}_init_early() so that we have the unified > tegra_init_early(). > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> [...] > diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c > index a599f6e..9bcecb8 100644 > --- a/arch/arm/mach-tegra/hotplug.c > +++ b/arch/arm/mach-tegra/hotplug.c > @@ -1,8 +1,7 @@ > /* > - * > * Copyright (C) 2002 ARM Ltd. > * All Rights Reserved > - * Copyright (c) 2010, 2012 NVIDIA Corporation. All rights reserved. > + * Copyright (c) 2010, 2012-2013, NVIDIA Corporation. All rights reserved. > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -15,6 +14,7 @@ > #include <asm/cacheflush.h> > #include <asm/smp_plat.h> > > +#include "fuse.h" > #include "sleep.h" > > static void (*tegra_hotplug_shutdown)(void); > @@ -56,18 +56,21 @@ int tegra_cpu_disable(unsigned int cpu) > return cpu == 0 ? -EPERM : 0; > } > > -#ifdef CONFIG_ARCH_TEGRA_2x_SOC > -extern void tegra20_hotplug_shutdown(void); > -void __init tegra20_hotplug_init(void) > +void __init tegra_hotplug_init(void) > { > - tegra_hotplug_shutdown = tegra20_hotplug_shutdown; > -} > + switch (tegra_chip_id) { > +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > + case TEGRA20: > + tegra_hotplug_shutdown = tegra20_hotplug_shutdown; > + break; > #endif > - > -#ifdef CONFIG_ARCH_TEGRA_3x_SOC > -extern void tegra30_hotplug_shutdown(void); > -void __init tegra30_hotplug_init(void) > -{ > - tegra_hotplug_shutdown = tegra30_hotplug_shutdown; > -} > +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) > + case TEGRA30: > + tegra_hotplug_shutdown = tegra30_hotplug_shutdown; > + break; > #endif > + default: > + BUG_ON(IS_ENABLED(CONFIG_HOTPLUG_CPU)); > + break; > + } > +} are these ifdefs really needed? Multisoc kernels will enable them all anyway and there is a case structure which protects the assignments. Also the hotplug functions are very tiny, so there shouldn't be a big loss. Marc
On 02/08/2013 05:29 AM, Marc Dietrich wrote: > Hiroshi, > > Am Freitag, 8. Februar 2013, 09:29:31 schrieb Hiroshi Doyu: >> Refactored tegra{20,30,114}_init_early() so that we have the unified >> tegra_init_early(). >> diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c >> +void __init tegra_hotplug_init(void) >> { >> + switch (tegra_chip_id) { >> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC >> + case TEGRA20: >> + tegra_hotplug_shutdown = tegra20_hotplug_shutdown; >> + break; >> #endif >> +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) >> + case TEGRA30: >> + tegra_hotplug_shutdown = tegra30_hotplug_shutdown; >> + break; >> #endif >> + default: >> + BUG_ON(IS_ENABLED(CONFIG_HOTPLUG_CPU)); >> + break; >> + } >> +} > > are these ifdefs really needed? Multisoc kernels will enable them all anyway > and there is a case structure which protects the assignments. Also the hotplug > functions are very tiny, so there shouldn't be a big loss. The files that contain/implement those functions are separate for each SoC and only included in the build when the individual SoCs are enabled. While multi-platform SoCs do make sense for distros, we also very specifically want to support the case where only Tegra, and only a single Tegra SoC, is enabled, hence this separation. (and Tegra doesn't support multi-platform yet; maybe in another kernel release or two)
On Friday 08 February 2013, Hiroshi Doyu wrote: > > > +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) > > > > how about using: > > > > #if IS_BUILTIN(CONFIG_ARCH_TEGRA_3x_SOC) > > > > instead ? > > Why is IS_BUILTIN() prefered? > Inside of a function, if(IS_ENABLED(CONFIG_FOO)) or the respective IS_BUILTIN is preferred over #ifdef because it provides better compile coverage and better readability. Also, IS_ENABLED() is nice when you want to check if something is builtin or module, without having to write a complex expression. If you just replace the #ifdef with #if IS_BUILTIN as Felipe suggested, I see no real benefit, but it would be nice to write this as void __init tegra_hotplug_init(void) { if (!IS_ENABLED(CONFIG_HOTPLUG_CPU)) return; if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC)) && tegra_chip_id == TEGRA20) tegra_hotplug_shutdown = tegra20_hotplug_shutdown; if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC)) && tegra_chip_id == TEGRA30) tegra_hotplug_shutdown = tegra30_hotplug_shutdown; } which completely avoids all preprocessor conditionals and replaces them with compile-time choices based on constant expressions to eliminate the code paths for disabled platforms. Arnd
On Friday 08 February 2013 10:09:10 Stephen Warren wrote: > On 02/08/2013 05:29 AM, Marc Dietrich wrote: > > Hiroshi, > > > > Am Freitag, 8. Februar 2013, 09:29:31 schrieb Hiroshi Doyu: > >> Refactored tegra{20,30,114}_init_early() so that we have the unified > >> tegra_init_early(). > >> > >> diff --git a/arch/arm/mach-tegra/hotplug.c > >> b/arch/arm/mach-tegra/hotplug.c > >> > >> +void __init tegra_hotplug_init(void) > >> > >> { > >> > >> + switch (tegra_chip_id) { > >> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC > >> + case TEGRA20: > >> + tegra_hotplug_shutdown = tegra20_hotplug_shutdown; > >> + break; > >> > >> #endif > >> > >> +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) > >> + case TEGRA30: > >> + tegra_hotplug_shutdown = tegra30_hotplug_shutdown; > >> + break; > >> > >> #endif > >> > >> + default: > >> + BUG_ON(IS_ENABLED(CONFIG_HOTPLUG_CPU)); > >> + break; > >> + } > >> +} > > > > are these ifdefs really needed? Multisoc kernels will enable them all > > anyway and there is a case structure which protects the assignments. Also > > the hotplug functions are very tiny, so there shouldn't be a big loss. > > The files that contain/implement those functions are separate for each > SoC and only included in the build when the individual SoCs are enabled. > > While multi-platform SoCs do make sense for distros, we also very > specifically want to support the case where only Tegra, and only a > single Tegra SoC, is enabled, hence this separation. Huh? so tegra_defconfig is not supported? grep "TEGRA_.*_SOC" tegra_defconfig: CONFIG_ARCH_TEGRA_2x_SOC=y CONFIG_ARCH_TEGRA_3x_SOC=y Marc
1On 02/10/2013 10:28 AM, Marc Dietrich wrote: > On Friday 08 February 2013 10:09:10 Stephen Warren wrote: >> On 02/08/2013 05:29 AM, Marc Dietrich wrote: >>> Hiroshi, >>> >>> Am Freitag, 8. Februar 2013, 09:29:31 schrieb Hiroshi Doyu: >>>> Refactored tegra{20,30,114}_init_early() so that we have the unified >>>> tegra_init_early(). ... >>> are these ifdefs really needed? Multisoc kernels will enable them all >>> anyway and there is a case structure which protects the assignments. Also >>> the hotplug functions are very tiny, so there shouldn't be a big loss. >> >> The files that contain/implement those functions are separate for each >> SoC and only included in the build when the individual SoCs are enabled. >> >> While multi-platform SoCs do make sense for distros, we also very >> specifically want to support the case where only Tegra, and only a >> single Tegra SoC, is enabled, hence this separation. > > Huh? so tegra_defconfig is not supported? > > grep "TEGRA_.*_SOC" tegra_defconfig: > > CONFIG_ARCH_TEGRA_2x_SOC=y > CONFIG_ARCH_TEGRA_3x_SOC=y I don't understand the question. But to be clear. There are now 3 variants of Tegra supported. (Tegra20, Tegra30, Tegra114). We want to be able to build a minimal-size kernel (e.g. for embedded applications) that supports just one, any combination of two, or all three Tegra variants.
On Sunday 10 February 2013 13:20:49 Stephen Warren wrote: > 1On 02/10/2013 10:28 AM, Marc Dietrich wrote: > > On Friday 08 February 2013 10:09:10 Stephen Warren wrote: > >> On 02/08/2013 05:29 AM, Marc Dietrich wrote: > >>> Hiroshi, > >>> > >>> Am Freitag, 8. Februar 2013, 09:29:31 schrieb Hiroshi Doyu: > >>>> Refactored tegra{20,30,114}_init_early() so that we have the unified > >>>> tegra_init_early(). > > ... > > >>> are these ifdefs really needed? Multisoc kernels will enable them all > >>> anyway and there is a case structure which protects the assignments. > >>> Also > >>> the hotplug functions are very tiny, so there shouldn't be a big loss. > >> > >> The files that contain/implement those functions are separate for each > >> SoC and only included in the build when the individual SoCs are enabled. > >> > >> While multi-platform SoCs do make sense for distros, we also very > >> specifically want to support the case where only Tegra, and only a > >> single Tegra SoC, is enabled, hence this separation. > > > > Huh? so tegra_defconfig is not supported? > > > > grep "TEGRA_.*_SOC" tegra_defconfig: > > > > CONFIG_ARCH_TEGRA_2x_SOC=y > > CONFIG_ARCH_TEGRA_3x_SOC=y > > I don't understand the question. > > But to be clear. There are now 3 variants of Tegra supported. (Tegra20, > Tegra30, Tegra114). We want to be able to build a minimal-size kernel > (e.g. for embedded applications) that supports just one, any combination > of two, or all three Tegra variants. ah, ok - I just skipped the "also" in your sentence above. But still, the #ifdefs look strange to me and save only a few byte of code. Just me few cents. Marc
Hi Marc, Marc Dietrich <marvin24@gmx.de> wrote @ Sun, 10 Feb 2013 22:16:14 +0100: > ah, ok - I just skipped the "also" in your sentence above. But still, the > #ifdefs look strange to me and save only a few byte of code. Just me few > cents. What about the following as Arnd suggested[1]? void __init tegra_hotplug_init(void) { if (!IS_ENABLED(CONFIG_HOTPLUG_CPU)) return; if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC)) && tegra_chip_id == TEGRA20) tegra_hotplug_shutdown = tegra20_hotplug_shutdown; if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC)) && tegra_chip_id == TEGRA30) tegra_hotplug_shutdown = tegra30_hotplug_shutdown; } [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/148632.html
diff --git a/arch/arm/mach-tegra/board-dt-tegra114.c b/arch/arm/mach-tegra/board-dt-tegra114.c index 085d636..08e8294 100644 --- a/arch/arm/mach-tegra/board-dt-tegra114.c +++ b/arch/arm/mach-tegra/board-dt-tegra114.c @@ -36,7 +36,7 @@ static const char * const tegra114_dt_board_compat[] = { DT_MACHINE_START(TEGRA114_DT, "NVIDIA Tegra114 (Flattened Device Tree)") .smp = smp_ops(tegra_smp_ops), .map_io = tegra_map_common_io, - .init_early = tegra114_init_early, + .init_early = tegra_init_early, .init_irq = tegra_dt_init_irq, .init_time = clocksource_of_init, .init_machine = tegra114_dt_init, diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c index a0edf25..fca18e9 100644 --- a/arch/arm/mach-tegra/board-dt-tegra20.c +++ b/arch/arm/mach-tegra/board-dt-tegra20.c @@ -145,7 +145,7 @@ static const char *tegra20_dt_board_compat[] = { DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)") .map_io = tegra_map_common_io, .smp = smp_ops(tegra_smp_ops), - .init_early = tegra20_init_early, + .init_early = tegra_init_early, .init_irq = tegra_dt_init_irq, .init_time = clocksource_of_init, .init_machine = tegra_dt_init, diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index bf68567..63f8139 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -3,7 +3,7 @@ * * NVIDIA Tegra30 device tree board support * - * Copyright (C) 2011 NVIDIA Corporation + * Copyright (C) 2011, 2013, NVIDIA Corporation * * Derived from: * @@ -50,7 +50,7 @@ static const char *tegra30_dt_board_compat[] = { DT_MACHINE_START(TEGRA30_DT, "NVIDIA Tegra30 (Flattened Device Tree)") .smp = smp_ops(tegra_smp_ops), .map_io = tegra_map_common_io, - .init_early = tegra30_init_early, + .init_early = tegra_init_early, .init_irq = tegra_dt_init_irq, .init_time = clocksource_of_init, .init_machine = tegra30_dt_init, diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h index 86851c8..60431de 100644 --- a/arch/arm/mach-tegra/board.h +++ b/arch/arm/mach-tegra/board.h @@ -26,9 +26,7 @@ void tegra_assert_system_reset(char mode, const char *cmd); -void __init tegra20_init_early(void); -void __init tegra30_init_early(void); -void __init tegra114_init_early(void); +void __init tegra_init_early(void); void __init tegra_map_common_io(void); void __init tegra_init_irq(void); void __init tegra_dt_init_irq(void); diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c index 5449a3f..f0315c9 100644 --- a/arch/arm/mach-tegra/common.c +++ b/arch/arm/mach-tegra/common.c @@ -94,7 +94,7 @@ static void __init tegra_init_cache(void) } -static void __init tegra_init_early(void) +void __init tegra_init_early(void) { tegra_cpu_reset_handler_init(); tegra_apb_io_init(); @@ -102,31 +102,9 @@ static void __init tegra_init_early(void) tegra_init_cache(); tegra_pmc_init(); tegra_powergate_init(); + tegra_hotplug_init(); } -#ifdef CONFIG_ARCH_TEGRA_2x_SOC -void __init tegra20_init_early(void) -{ - tegra_init_early(); - tegra20_hotplug_init(); -} -#endif - -#ifdef CONFIG_ARCH_TEGRA_3x_SOC -void __init tegra30_init_early(void) -{ - tegra_init_early(); - tegra30_hotplug_init(); -} -#endif - -#ifdef CONFIG_ARCH_TEGRA_114_SOC -void __init tegra114_init_early(void) -{ - tegra_init_early(); -} -#endif - void __init tegra_init_late(void) { tegra_powergate_debugfs_init(); diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c index a599f6e..9bcecb8 100644 --- a/arch/arm/mach-tegra/hotplug.c +++ b/arch/arm/mach-tegra/hotplug.c @@ -1,8 +1,7 @@ /* - * * Copyright (C) 2002 ARM Ltd. * All Rights Reserved - * Copyright (c) 2010, 2012 NVIDIA Corporation. All rights reserved. + * Copyright (c) 2010, 2012-2013, NVIDIA Corporation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -15,6 +14,7 @@ #include <asm/cacheflush.h> #include <asm/smp_plat.h> +#include "fuse.h" #include "sleep.h" static void (*tegra_hotplug_shutdown)(void); @@ -56,18 +56,21 @@ int tegra_cpu_disable(unsigned int cpu) return cpu == 0 ? -EPERM : 0; } -#ifdef CONFIG_ARCH_TEGRA_2x_SOC -extern void tegra20_hotplug_shutdown(void); -void __init tegra20_hotplug_init(void) +void __init tegra_hotplug_init(void) { - tegra_hotplug_shutdown = tegra20_hotplug_shutdown; -} + switch (tegra_chip_id) { +#ifdef CONFIG_ARCH_TEGRA_2x_SOC + case TEGRA20: + tegra_hotplug_shutdown = tegra20_hotplug_shutdown; + break; #endif - -#ifdef CONFIG_ARCH_TEGRA_3x_SOC -extern void tegra30_hotplug_shutdown(void); -void __init tegra30_hotplug_init(void) -{ - tegra_hotplug_shutdown = tegra30_hotplug_shutdown; -} +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) + case TEGRA30: + tegra_hotplug_shutdown = tegra30_hotplug_shutdown; + break; #endif + default: + BUG_ON(IS_ENABLED(CONFIG_HOTPLUG_CPU)); + break; + } +} diff --git a/arch/arm/mach-tegra/sleep.h b/arch/arm/mach-tegra/sleep.h index 4ffae54..970ebd5 100644 --- a/arch/arm/mach-tegra/sleep.h +++ b/arch/arm/mach-tegra/sleep.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010-2012, NVIDIA Corporation. All rights reserved. + * Copyright (c) 2010-2013, NVIDIA Corporation. All rights reserved. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -124,11 +124,11 @@ int tegra_sleep_cpu_finish(unsigned long); void tegra_disable_clean_inv_dcache(void); #ifdef CONFIG_HOTPLUG_CPU -void tegra20_hotplug_init(void); -void tegra30_hotplug_init(void); +void tegra20_hotplug_shutdown(void); +void tegra30_hotplug_shutdown(void); +void tegra_hotplug_init(void); #else -static inline void tegra20_hotplug_init(void) {} -static inline void tegra30_hotplug_init(void) {} +static inline void tegra_hotplug_init(void) {} #endif void tegra20_cpu_shutdown(int cpu);
Refactored tegra{20,30,114}_init_early() so that we have the unified tegra_init_early(). Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- arch/arm/mach-tegra/board-dt-tegra114.c | 2 +- arch/arm/mach-tegra/board-dt-tegra20.c | 2 +- arch/arm/mach-tegra/board-dt-tegra30.c | 4 ++-- arch/arm/mach-tegra/board.h | 4 +--- arch/arm/mach-tegra/common.c | 26 ++------------------------ arch/arm/mach-tegra/hotplug.c | 31 +++++++++++++++++-------------- arch/arm/mach-tegra/sleep.h | 10 +++++----- 7 files changed, 29 insertions(+), 50 deletions(-)