[v2 1/3] ARM: tegra: Unify tegra{20,30,114}_init_early()

[v2 1/3] ARM: tegra: Unify tegra{20,30,114}_init_early()

[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>
---
Update:
Used IS_ENABLED() instead of ifdefs as Arnd/Felipe suggested.
---
 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           |   23 +++++++++--------------
 arch/arm/mach-tegra/sleep.h             |   10 +++++-----
 7 files changed, 21 insertions(+), 50 deletions(-)

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..8da9f78 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,13 @@ 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;
-}
-#endif
+	if (!IS_ENABLED(CONFIG_HOTPLUG_CPU))
+		return;
 
-#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 (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;
 }
-#endif
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);
-- 
1.7.9.5

[v2 2/3] ARM: tegra: Rename board-dt-tegra20.c to tegra.c

[Hiroshi Doyu]

This is the preparation to unify "board-dt-tegra{20,30,114}.c" to a
single file "tegra.c".

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mach-tegra/Makefile                       |    2 +-
 .../arm/mach-tegra/{board-dt-tegra20.c => tegra.c} |    0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename arch/arm/mach-tegra/{board-dt-tegra20.c => tegra.c} (100%)

diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index f6b46ae..c6d6be2 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -27,7 +27,7 @@ obj-$(CONFIG_HOTPLUG_CPU)               += hotplug.o
 obj-$(CONFIG_CPU_FREQ)                  += cpu-tegra.o
 obj-$(CONFIG_TEGRA_PCI)			+= pcie.o
 
-obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= board-dt-tegra20.o
+obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra.o
 obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= board-dt-tegra30.o
 obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= board-dt-tegra114.o
 ifeq ($(CONFIG_CPU_IDLE),y)
diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/tegra.c
similarity index 100%
rename from arch/arm/mach-tegra/board-dt-tegra20.c
rename to arch/arm/mach-tegra/tegra.c
-- 
1.7.9.5

[v2 3/3] ARM: tegra: Unify Device tree board files

[Hiroshi Doyu]

Unify board-dt-tegra{30,114} to the Tegra20 DT board file, "tegra.c".

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mach-tegra/Makefile            |    5 ++-
 arch/arm/mach-tegra/board-dt-tegra114.c |   46 ------------------------
 arch/arm/mach-tegra/board-dt-tegra30.c  |   60 -------------------------------
 arch/arm/mach-tegra/tegra.c             |   24 +++++++------
 4 files changed, 16 insertions(+), 119 deletions(-)
 delete mode 100644 arch/arm/mach-tegra/board-dt-tegra114.c
 delete mode 100644 arch/arm/mach-tegra/board-dt-tegra30.c

diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index c6d6be2..ee866337 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -27,9 +27,8 @@ obj-$(CONFIG_HOTPLUG_CPU)               += hotplug.o
 obj-$(CONFIG_CPU_FREQ)                  += cpu-tegra.o
 obj-$(CONFIG_TEGRA_PCI)			+= pcie.o
 
-obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra.o
-obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= board-dt-tegra30.o
-obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= board-dt-tegra114.o
+obj-y					+= tegra.o
+
 ifeq ($(CONFIG_CPU_IDLE),y)
 obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= cpuidle-tegra114.o
 endif
diff --git a/arch/arm/mach-tegra/board-dt-tegra114.c b/arch/arm/mach-tegra/board-dt-tegra114.c
deleted file mode 100644
index 08e8294..0000000
--- a/arch/arm/mach-tegra/board-dt-tegra114.c
+++ /dev/null
@@ -1,46 +0,0 @@
-/*
- * NVIDIA Tegra114 device tree board support
- *
- * Copyright (C) 2013 NVIDIA Corporation
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/of.h>
-#include <linux/of_platform.h>
-#include <linux/clocksource.h>
-
-#include <asm/mach/arch.h>
-
-#include "board.h"
-#include "common.h"
-
-static void __init tegra114_dt_init(void)
-{
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-}
-
-static const char * const tegra114_dt_board_compat[] = {
-	"nvidia,tegra114",
-	NULL,
-};
-
-DT_MACHINE_START(TEGRA114_DT, "NVIDIA Tegra114 (Flattened Device Tree)")
-	.smp		= smp_ops(tegra_smp_ops),
-	.map_io		= tegra_map_common_io,
-	.init_early	= tegra_init_early,
-	.init_irq	= tegra_dt_init_irq,
-	.init_time	= clocksource_of_init,
-	.init_machine	= tegra114_dt_init,
-	.init_late	= tegra_init_late,
-	.restart	= tegra_assert_system_reset,
-	.dt_compat	= tegra114_dt_board_compat,
-MACHINE_END
diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c
deleted file mode 100644
index 63f8139..0000000
--- a/arch/arm/mach-tegra/board-dt-tegra30.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * arch/arm/mach-tegra/board-dt-tegra30.c
- *
- * NVIDIA Tegra30 device tree board support
- *
- * Copyright (C) 2011, 2013, NVIDIA Corporation
- *
- * Derived from:
- *
- * arch/arm/mach-tegra/board-dt-tegra20.c
- *
- * Copyright (C) 2010 Secret Lab Technologies, Ltd.
- * Copyright (C) 2010 Google, Inc.
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/clocksource.h>
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_fdt.h>
-#include <linux/of_irq.h>
-#include <linux/of_platform.h>
-
-#include <asm/mach/arch.h>
-
-#include "board.h"
-#include "common.h"
-#include "iomap.h"
-
-static void __init tegra30_dt_init(void)
-{
-	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-}
-
-static const char *tegra30_dt_board_compat[] = {
-	"nvidia,tegra30",
-	NULL
-};
-
-DT_MACHINE_START(TEGRA30_DT, "NVIDIA Tegra30 (Flattened Device Tree)")
-	.smp		= smp_ops(tegra_smp_ops),
-	.map_io		= tegra_map_common_io,
-	.init_early	= tegra_init_early,
-	.init_irq	= tegra_dt_init_irq,
-	.init_time	= clocksource_of_init,
-	.init_machine	= tegra30_dt_init,
-	.init_late	= tegra_init_late,
-	.restart	= tegra_assert_system_reset,
-	.dt_compat	= tegra30_dt_board_compat,
-MACHINE_END
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index fca18e9..00debef 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -1,6 +1,7 @@
 /*
- * nVidia Tegra device tree board support
+ * NVIDIA Tegra SoC device tree board support
  *
+ * Copyright (C) 2011, 2013, NVIDIA Corporation
  * Copyright (C) 2010 Secret Lab Technologies, Ltd.
  * Copyright (C) 2010 Google, Inc.
  *
@@ -67,13 +68,15 @@ static struct tegra_ehci_platform_data tegra_ehci3_pdata = {
 	.vbus_gpio = -1,
 };
 
-static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
+static struct of_dev_auxdata tegra_auxdata_lookup[] __initdata = {
+#ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5000000, "tegra-ehci.0",
 		       &tegra_ehci1_pdata),
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5004000, "tegra-ehci.1",
 		       &tegra_ehci2_pdata),
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5008000, "tegra-ehci.2",
 		       &tegra_ehci3_pdata),
+#endif
 	{}
 };
 
@@ -84,29 +87,25 @@ static void __init tegra_dt_init(void)
 	 * devices
 	 */
 	of_platform_populate(NULL, of_default_bus_match_table,
-				tegra20_auxdata_lookup, NULL);
+				tegra_auxdata_lookup, NULL);
 }
 
 static void __init trimslice_init(void)
 {
-#ifdef CONFIG_TEGRA_PCI
 	int ret;
 
 	ret = tegra_pcie_init(true, true);
 	if (ret)
 		pr_err("tegra_pci_init() failed: %d\n", ret);
-#endif
 }
 
 static void __init harmony_init(void)
 {
-#ifdef CONFIG_TEGRA_PCI
 	int ret;
 
 	ret = harmony_pcie_init();
 	if (ret)
 		pr_err("harmony_pcie_init() failed: %d\n", ret);
-#endif
 }
 
 static void __init paz00_init(void)
@@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
 
 	tegra_init_late();
 
+	if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(board_init_funcs); i++) {
 		if (of_machine_is_compatible(board_init_funcs[i].machine)) {
 			board_init_funcs[i].init();
@@ -137,12 +139,14 @@ static void __init tegra_dt_init_late(void)
 	}
 }
 
-static const char *tegra20_dt_board_compat[] = {
+static const char * const tegra_dt_board_compat[] = {
 	"nvidia,tegra20",
+	"nvidia,tegra30",
+	"nvidia,tegra114",
 	NULL
 };
 
-DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)")
+DT_MACHINE_START(TEGRA_DT, "NVIDIA Tegra SoC (Flattened Device Tree)")
 	.map_io		= tegra_map_common_io,
 	.smp		= smp_ops(tegra_smp_ops),
 	.init_early	= tegra_init_early,
@@ -151,5 +155,5 @@ DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)")
 	.init_machine	= tegra_dt_init,
 	.init_late	= tegra_dt_init_late,
 	.restart	= tegra_assert_system_reset,
-	.dt_compat	= tegra20_dt_board_compat,
+	.dt_compat	= tegra_dt_board_compat,
 MACHINE_END
-- 
1.7.9.5

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Stephen Warren]

On 02/10/2013 11:05 PM, Hiroshi Doyu wrote:
> Unify board-dt-tegra{30,114} to the Tegra20 DT board file, "tegra.c".

> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile

>  obj-$(CONFIG_CPU_FREQ)                  += cpu-tegra.o
>  obj-$(CONFIG_TEGRA_PCI)			+= pcie.o
>  
> -obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra.o
> -obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= board-dt-tegra30.o
> -obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= board-dt-tegra114.o
> +obj-y					+= tegra.o
> +

I think I'd be tempted to move that line together with all the others
that don't depend on some config option.

> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c

> -static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
> +static struct of_dev_auxdata tegra_auxdata_lookup[] __initdata = {
> +#ifdef CONFIG_ARCH_TEGRA_2x_SOC
>  	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5000000, "tegra-ehci.0",
>  		       &tegra_ehci1_pdata),
>  	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5004000, "tegra-ehci.1",
>  		       &tegra_ehci2_pdata),
>  	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5008000, "tegra-ehci.2",
>  		       &tegra_ehci3_pdata),
> +#endif
>  	{}
>  };

Why ifdef? This is certainly small enough that it shouldn't matter for
any Tegra30- or Tegra114-kernel, and it's hopefully going away very
soon, so I'd prefer to drop the addition of the ifdefs to avoid any
conflicts with any other changes to that table.

>  static void __init trimslice_init(void)
>  {
> -#ifdef CONFIG_TEGRA_PCI
>  	int ret;
>  
>  	ret = tegra_pcie_init(true, true);
>  	if (ret)
>  		pr_err("tegra_pci_init() failed: %d\n", ret);
> -#endif
>  }
>  
>  static void __init harmony_init(void)
>  {
> -#ifdef CONFIG_TEGRA_PCI
>  	int ret;
>  
>  	ret = harmony_pcie_init();
>  	if (ret)
>  		pr_err("harmony_pcie_init() failed: %d\n", ret);
> -#endif
>  }

Why drop those ifdefs? Does the code still compile (link) if built for
Tegra30-only or Tegra114-only, where the Tegra PCI driver won't be
enabled, and hence those functions don't exist?

>  static void __init paz00_init(void)
> @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
>  
>  	tegra_init_late();
>  
> +	if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> +		return;

I don't think that's going to help any link issues, so I'd drop it and
keep this function simple. After all, what if someone wants to add some
non-PCI-related entry into board_init_funcs[]?

> -static const char *tegra20_dt_board_compat[] = {
> +static const char * const tegra_dt_board_compat[] = {
>  	"nvidia,tegra20",
> +	"nvidia,tegra30",
> +	"nvidia,tegra114",
>  	NULL
>  };

It'd be best to add the newer values first. It shouldn't matter, but
currently does at least for device compatible matching, so we may as
well be consistent here.

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 12 Feb 2013 00:54:03 +0100:

> > -obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra.o
> > -obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= board-dt-tegra30.o
> > -obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= board-dt-tegra114.o
> > +obj-y					+= tegra.o
> > +
> 
> I think I'd be tempted to move that line together with all the others
> that don't depend on some config option.

In arch/arm/mach-tegra/Makefile, we have:

obj-y                                   += common.o
obj-y                                   += io.o
obj-y                                   += irq.o
obj-y					+= fuse.o
obj-y					+= pmc.o
.....

Should we have a single entry for them as well?

obj-y:= common.o io.o irq.o fuse.o pmc.o flowctrl.o powergate.o apbio.o pm.o \
	reset.o reset-handler.o sleep.o tegra.o

I think that this moval could be done another patch.

> >  static void __init harmony_init(void)
> >  {
> > -#ifdef CONFIG_TEGRA_PCI
> >  	int ret;
> >  
> >  	ret = harmony_pcie_init();
> >  	if (ret)
> >  		pr_err("harmony_pcie_init() failed: %d\n", ret);
> > -#endif
> >  }
> 
> Why drop those ifdefs? Does the code still compile (link) if built for
> Tegra30-only or Tegra114-only, where the Tegra PCI driver won't be
> enabled, and hence those functions don't exist?

This function itself will be dropped by the following IS_ENABLED().

> >  static void __init paz00_init(void)
> > @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
> >  
> >  	tegra_init_late();
> >  
> > +	if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> > +		return;
> 
> I don't think that's going to help any link issues, so I'd drop it and
> keep this function simple.

As explained in the above, a complier will drop unnecessary functions
automatically with this IS_ENABLED(), which could save many ifdefs.

> After all, what if someone wants to add some
> non-PCI-related entry into board_init_funcs[]?

I originally thought that one should try to solve those board specific
problems with DT basically and this tegra specific board_init_funcs[]
was left only for the legacy support during DT transition.

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Stephen Warren]

On 02/11/2013 09:12 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 12 Feb 2013 00:54:03 +0100:
> 
>>> -obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra.o
>>> -obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= board-dt-tegra30.o
>>> -obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= board-dt-tegra114.o
>>> +obj-y					+= tegra.o
>>> +
>>
>> I think I'd be tempted to move that line together with all the others
>> that don't depend on some config option.
> 
> In arch/arm/mach-tegra/Makefile, we have:
> 
> obj-y                                   += common.o
> obj-y                                   += io.o
> obj-y                                   += irq.o
> obj-y					+= fuse.o
> obj-y					+= pmc.o
> .....
> 
> Should we have a single entry for them as well?
> 
> obj-y:= common.o io.o irq.o fuse.o pmc.o flowctrl.o powergate.o apbio.o pm.o \
> 	reset.o reset-handler.o sleep.o tegra.o
> 
> I think that this moval could be done another patch.

I just meant put the lines next to each-other. We definitely shouldn't
merge the lines together or it'll make it more painful to change the
list of files later.

>>>  static void __init harmony_init(void)
>>>  {
>>> -#ifdef CONFIG_TEGRA_PCI
>>>  	int ret;
>>>  
>>>  	ret = harmony_pcie_init();
>>>  	if (ret)
>>>  		pr_err("harmony_pcie_init() failed: %d\n", ret);
>>> -#endif
>>>  }
>>
>> Why drop those ifdefs? Does the code still compile (link) if built for
>> Tegra30-only or Tegra114-only, where the Tegra PCI driver won't be
>> enabled, and hence those functions don't exist?
> 
> This function itself will be dropped by the following IS_ENABLED().
> 
>>>  static void __init paz00_init(void)
>>> @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
>>>  
>>>  	tegra_init_late();
>>>  
>>> +	if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
>>> +		return;
>>
>> I don't think that's going to help any link issues, so I'd drop it and
>> keep this function simple.
> 
> As explained in the above, a complier will drop unnecessary functions
> automatically with this IS_ENABLED(), which could save many ifdefs.

That sounds extremely brittle. Have you validated this on a wide variety
of compiler versions?

>> After all, what if someone wants to add some
>> non-PCI-related entry into board_init_funcs[]?
> 
> I originally thought that one should try to solve those board specific
> problems with DT basically and this tegra specific board_init_funcs[]
> was left only for the legacy support during DT transition.

That's the intent right now, but who knows what else might end up
getting added there. It seems simplest just to maintain the ifdefs since
they're already there.

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 12 Feb 2013 05:47:20 +0100:

> >>>  static void __init harmony_init(void)
> >>>  {
> >>> -#ifdef CONFIG_TEGRA_PCI
> >>>  	int ret;
> >>>  
> >>>  	ret = harmony_pcie_init();
> >>>  	if (ret)
> >>>  		pr_err("harmony_pcie_init() failed: %d\n", ret);
> >>> -#endif
> >>>  }
> >>
> >> Why drop those ifdefs? Does the code still compile (link) if built for
> >> Tegra30-only or Tegra114-only, where the Tegra PCI driver won't be
> >> enabled, and hence those functions don't exist?
> > 
> > This function itself will be dropped by the following IS_ENABLED().
> > 
> >>>  static void __init paz00_init(void)
> >>> @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
> >>>  
> >>>  	tegra_init_late();
> >>>  
> >>> +	if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> >>> +		return;
> >>
> >> I don't think that's going to help any link issues, so I'd drop it and
> >> keep this function simple.
> > 
> > As explained in the above, a complier will drop unnecessary functions
> > automatically with this IS_ENABLED(), which could save many ifdefs.
> 
> That sounds extremely brittle. Have you validated this on a wide variety
> of compiler versions?

I verified with gcc-4.6.
IIRC, that's the point of IS_ENABLED() being introduced. Arnd?

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Arnd Bergmann]

On Tuesday 12 February 2013, Hiroshi Doyu wrote:
> > >>>  static void __init paz00_init(void)
> > >>> @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
> > >>>  
> > >>>   tegra_init_late();
> > >>>  
> > >>> + if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> > >>> +         return;
> > >>
> > >> I don't think that's going to help any link issues, so I'd drop it and
> > >> keep this function simple.
> > > 
> > > As explained in the above, a complier will drop unnecessary functions
> > > automatically with this IS_ENABLED(), which could save many ifdefs.
> > 
> > That sounds extremely brittle. Have you validated this on a wide variety
> > of compiler versions?
> 
> I verified with gcc-4.6.
> IIRC, that's the point of IS_ENABLED() being introduced. Arnd?

It's certainly expected to work with all compilers, yes. If a compiler
cannot remove conditional function calls that depend on a constant
expression, we have a lot of other problems alredy.

However, from what I see above, you have the logic reversed (it
should return if neither TEGRA_2x nor PCI are enabled, rather
than return if one of them is enabled). and it becomes a little
confusing to read.

If you want to get rid of the #ifdef here, I would suggest you put those
into the functions directly, like

static void __init harmony_init(void)
{
        int ret = 0;

        if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
		ret = harmony_pcie_init();
        if (ret)
                pr_err("harmony_pcie_init() failed: %d\n", ret);
}

which will turn into an empty function if one of the two is disabled.

Since we are not going to add any other board specfic init functions, you
can also unroll the loop and put everything into tegra_dt_init_late:

/* Board specific fixups, try not add any new ones here */
static void __init tegra_dt_init_late(void)
{
	tegra_powergate_debugfs_init();

	/* so far, these all apply only to tegra2x */
	if (!IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
		return;

	if (of_machine_is_compatible("compal,paz00"))
		tegra_paz00_wifikill_init();
	if (IS_ENABLED(CONFIG_PCI) && of_machine_is_compatible("nvidia,harmony")
		harmony_pcie_init();
	if (IS_ENABLED(CONFIG_PCI) && of_machine_is_compatible("compulab,trimslice")
		tegra_pcie_init(true, true);
}

	Arnd

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Stephen Warren]

On 02/12/2013 06:50 AM, Arnd Bergmann wrote:
> On Tuesday 12 February 2013, Hiroshi Doyu wrote:
>>>>>>  static void __init paz00_init(void)
>>>>>> @@ -129,6 +128,9 @@ static void __init tegra_dt_init_late(void)
>>>>>>  
>>>>>>   tegra_init_late();
>>>>>>  
>>>>>> + if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
>>>>>> +         return;
>>>>>
>>>>> I don't think that's going to help any link issues, so I'd drop it and
>>>>> keep this function simple.
>>>>
>>>> As explained in the above, a complier will drop unnecessary functions
>>>> automatically with this IS_ENABLED(), which could save many ifdefs.
>>>
>>> That sounds extremely brittle. Have you validated this on a wide variety
>>> of compiler versions?
>>
>> I verified with gcc-4.6.
>> IIRC, that's the point of IS_ENABLED() being introduced. Arnd?
> 
> It's certainly expected to work with all compilers, yes. If a compiler
> cannot remove conditional function calls that depend on a constant
> expression, we have a lot of other problems alredy.

Removing the function calls isn't guaranteed to remove the body of the
functions though. Those functions aren't implemented in some separate
file that's optionally included, but rather are implemented in the same
object file, now unconditionally, and they in turn reference (with no
IS_ENABLED logic) other functions that are implemented in conditionally
linked files.

> However, from what I see above, you have the logic reversed (it
> should return if neither TEGRA_2x nor PCI are enabled, rather
> than return if one of them is enabled). and it becomes a little
> confusing to read.
> 
> If you want to get rid of the #ifdef here, I would suggest you put those
> into the functions directly, like
> 
> static void __init harmony_init(void)
> {
>         int ret = 0;
> 
>         if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> 		ret = harmony_pcie_init();
>         if (ret)
>                 pr_err("harmony_pcie_init() failed: %d\n", ret);
> }
> 
> which will turn into an empty function if one of the two is disabled.

That would work.

However I'd like to avoid changing the body of those two functions at
all if possible, since I hope the PCIe driver rework will be merged in
3.10, and that will allow the Harmony and TrimSlice init functions to be
removed entirely. I'd rather not have conflicts with the removal patch.

> Since we are not going to add any other board specfic init functions, you
> can also unroll the loop and put everything into tegra_dt_init_late:

That's not necessarily true. While we certainly don't plan to, I don't
think we can rule it out; after all, we don't have rfkill bindings and
yet other boards will need them.

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Arnd Bergmann]

On Tuesday 12 February 2013, Stephen Warren wrote:
> >>>>> I don't think that's going to help any link issues, so I'd drop it and
> >>>>> keep this function simple.
> >>>>
> >>>> As explained in the above, a complier will drop unnecessary functions
> >>>> automatically with this IS_ENABLED(), which could save many ifdefs.
> >>>
> >>> That sounds extremely brittle. Have you validated this on a wide variety
> >>> of compiler versions?
> >>
> >> I verified with gcc-4.6.
> >> IIRC, that's the point of IS_ENABLED() being introduced. Arnd?
> > 
> > It's certainly expected to work with all compilers, yes. If a compiler
> > cannot remove conditional function calls that depend on a constant
> > expression, we have a lot of other problems alredy.
> 
> Removing the function calls isn't guaranteed to remove the body of the
> functions though. Those functions aren't implemented in some separate
> file that's optionally included, but rather are implemented in the same
> object file, now unconditionally, and they in turn reference (with no
> IS_ENABLED logic) other functions that are implemented in conditionally
> linked files.

I think gcc should remove all of that in this case, since board_init_funcs
becomes unused when the only code that references it cannot be reached,
and when that array is gone, the now unused functions would be removed
as well.

We have had bugs with prerelease gcc versions that did not handle cases
like this in the way we want it, but I think all releases get it right.
I don't think it's mandated anywhere in the C99 standard that this is
what happens, but we do rely on it anyway AFAIK.

> > static void __init harmony_init(void)
> > {
> >         int ret = 0;
> > 
> >         if (IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
> >               ret = harmony_pcie_init();
> >         if (ret)
> >                 pr_err("harmony_pcie_init() failed: %d\n", ret);
> > }
> > 
> > which will turn into an empty function if one of the two is disabled.
> 
> That would work.
> 
> However I'd like to avoid changing the body of those two functions at
> all if possible, since I hope the PCIe driver rework will be merged in
> 3.10, and that will allow the Harmony and TrimSlice init functions to be
> removed entirely. I'd rather not have conflicts with the removal patch.

Yes, makes sense.

> > Since we are not going to add any other board specfic init functions, you
> > can also unroll the loop and put everything into tegra_dt_init_late:
> 
> That's not necessarily true. While we certainly don't plan to, I don't
> think we can rule it out; after all, we don't have rfkill bindings and
> yet other boards will need them.

Ok.

	Arnd

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Stephen Warren]

On 02/12/2013 10:32 AM, Arnd Bergmann wrote:
> On Tuesday 12 February 2013, Stephen Warren wrote:
>>>>>>> I don't think that's going to help any link issues, so I'd drop it and
>>>>>>> keep this function simple.
>>>>>>
>>>>>> As explained in the above, a complier will drop unnecessary functions
>>>>>> automatically with this IS_ENABLED(), which could save many ifdefs.
>>>>>
>>>>> That sounds extremely brittle. Have you validated this on a wide variety
>>>>> of compiler versions?
>>>>
>>>> I verified with gcc-4.6.
>>>> IIRC, that's the point of IS_ENABLED() being introduced. Arnd?
>>>
>>> It's certainly expected to work with all compilers, yes. If a compiler
>>> cannot remove conditional function calls that depend on a constant
>>> expression, we have a lot of other problems alredy.
>>
>> Removing the function calls isn't guaranteed to remove the body of the
>> functions though. Those functions aren't implemented in some separate
>> file that's optionally included, but rather are implemented in the same
>> object file, now unconditionally, and they in turn reference (with no
>> IS_ENABLED logic) other functions that are implemented in conditionally
>> linked files.
> 
> I think gcc should remove all of that in this case, since board_init_funcs
> becomes unused when the only code that references it cannot be reached,
> and when that array is gone, the now unused functions would be removed
> as well.
> 
> We have had bugs with prerelease gcc versions that did not handle cases
> like this in the way we want it, but I think all releases get it right.
> I don't think it's mandated anywhere in the C99 standard that this is
> what happens, but we do rely on it anyway AFAIK.

Hmmm. Very surprisingly (to me), this does indeed work fine, even with
an older gcc-4.4.1 I had lying around (after fixing the test inversion
in tegra_dt_init_late).

I believe U-Boot enabled -ffunction-sections -fdata-sections or similar
(recently?) to get this kind of behaviour. I wonder why the kernel
didn't need that. Perhaps -O2 is more aggressive (within a file at
least) than I thought.

I did note a few warnings due to the ifdefs in tegra_auxdata_lookup[]:

arch/arm/mach-tegra/tegra.c:47: warning: 'tegra_ehci1_pdata' defined but
not used
arch/arm/mach-tegra/tegra.c:58: warning: 'tegra_ehci2_pdata' defined but
not used
arch/arm/mach-tegra/tegra.c:65: warning: 'tegra_ehci3_pdata' defined but
not used

(I built a tegra_defconfig kernel and modified it to enable
Tegra30+Tegra114, disable Tegra20, disable DRM)

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Arnd Bergmann]

On Tuesday 12 February 2013, Stephen Warren wrote:
> I believe U-Boot enabled -ffunction-sections -fdata-sections or similar
> (recently?) to get this kind of behaviour. I wonder why the kernel
> didn't need that. Perhaps -O2 is more aggressive (within a file at
> least) than I thought.

-ffunction-sections works across files, while the trick I described
only works on file static symbols. David Woodhouse at some point
had a working kernel build with -ffunction-sections but for some
reason that never got merged.

	Arnd

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 12 Feb 2013 17:35:56 +0100:

> However I'd like to avoid changing the body of those two functions at
> all if possible, since I hope the PCIe driver rework will be merged in
> 3.10, and that will allow the Harmony and TrimSlice init functions to be
> removed entirely. I'd rather not have conflicts with the removal patch.
> 
> > Since we are not going to add any other board specfic init functions, you
> > can also unroll the loop and put everything into tegra_dt_init_late:
> 
> That's not necessarily true. While we certainly don't plan to, I don't
> think we can rule it out; after all, we don't have rfkill bindings and
> yet other boards will need them.

Considering the above points,

- The Harmony and TrimSlice init functions will be removed entirely
  so soon that we want to avoid those merge conflicts as much as
  possible.
- We may still need board specific init, especially for rfkill.
- We want to build any combination of Tegra{20,30,114}, 2^3 - 1 == 7.

The patch for "tegra.c" would be as below.

Should I still keep the name, "tegra20_auxdata_lookup" to avoid
any conflicts or "tegra_auxdata_lookup" would be ok for merge?

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index fca18e9..fd4412d 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -1,6 +1,7 @@
 /*
- * nVidia Tegra device tree board support
+ * NVIDIA Tegra SoC device tree board support
  *
+ * Copyright (C) 2011, 2013, NVIDIA Corporation
  * Copyright (C) 2010 Secret Lab Technologies, Ltd.
  * Copyright (C) 2010 Google, Inc.
  *
@@ -67,7 +68,7 @@ static struct tegra_ehci_platform_data tegra_ehci3_pdata = {
 	.vbus_gpio = -1,
 };
 
-static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
+static struct of_dev_auxdata tegra_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5000000, "tegra-ehci.0",
 		       &tegra_ehci1_pdata),
 	OF_DEV_AUXDATA("nvidia,tegra20-ehci", 0xC5004000, "tegra-ehci.1",
@@ -84,7 +85,7 @@ static void __init tegra_dt_init(void)
 	 * devices
 	 */
 	of_platform_populate(NULL, of_default_bus_match_table,
-				tegra20_auxdata_lookup, NULL);
+				tegra_auxdata_lookup, NULL);
 }
 
 static void __init trimslice_init(void)
@@ -111,7 +112,8 @@ static void __init harmony_init(void)
 
 static void __init paz00_init(void)
 {
-	tegra_paz00_wifikill_init();
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC))
+		tegra_paz00_wifikill_init();
 }
 
 static struct {
@@ -137,12 +139,14 @@ static void __init tegra_dt_init_late(void)
 	}
 }
 
-static const char *tegra20_dt_board_compat[] = {
+static const char * const tegra_dt_board_compat[] = {
+	"nvidia,tegra114",
+	"nvidia,tegra30",
 	"nvidia,tegra20",
 	NULL
 };
 
-DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)")
+DT_MACHINE_START(TEGRA_DT, "NVIDIA Tegra SoC (Flattened Device Tree)")
 	.map_io		= tegra_map_common_io,
 	.smp		= smp_ops(tegra_smp_ops),
 	.init_early	= tegra_init_early,
@@ -151,5 +155,5 @@ DT_MACHINE_START(TEGRA_DT, "nVidia Tegra20 (Flattened Device Tree)")
 	.init_machine	= tegra_dt_init,
 	.init_late	= tegra_dt_init_late,
 	.restart	= tegra_assert_system_reset,
-	.dt_compat	= tegra20_dt_board_compat,
+	.dt_compat	= tegra_dt_board_compat,
 MACHINE_END
-- 
1.7.9.5

Re: [v2 3/3] ARM: tegra: Unify Device tree board files

[Stephen Warren]

On 02/12/2013 11:12 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 12 Feb 2013 17:35:56 +0100:
> 
>> However I'd like to avoid changing the body of those two functions at
>> all if possible, since I hope the PCIe driver rework will be merged in
>> 3.10, and that will allow the Harmony and TrimSlice init functions to be
>> removed entirely. I'd rather not have conflicts with the removal patch.
>>
>>> Since we are not going to add any other board specfic init functions, you
>>> can also unroll the loop and put everything into tegra_dt_init_late:
>>
>> That's not necessarily true. While we certainly don't plan to, I don't
>> think we can rule it out; after all, we don't have rfkill bindings and
>> yet other boards will need them.
> 
> Considering the above points,
> 
> - The Harmony and TrimSlice init functions will be removed entirely
>   so soon that we want to avoid those merge conflicts as much as
>   possible.
> - We may still need board specific init, especially for rfkill.
> - We want to build any combination of Tegra{20,30,114}, 2^3 - 1 == 7.
> 
> The patch for "tegra.c" would be as below.
> 
> Should I still keep the name, "tegra20_auxdata_lookup" to avoid
> any conflicts or "tegra_auxdata_lookup" would be ok for merge?

Yes, avoiding the rename of the auxdata table might be a good idea.

Aside from that, I think the patch you gave looks fine.