Question: Multiple board support is broken

Question: Multiple board support is broken

[Alexander Shiyan]

Hello.

Today I was tested multiple boards (not multiplatform) in the kernel
and found problems with booting. Exacly, if multiple boards defined
in. config, we can proceed to boot only the last (determined by mach number).
This problem is the result of a redefine "machine_arch_type" variable in
the mach-types.h more than once for each machine. As a result, we can not
use machine_is_xx () macros.
The following is an attempt to make mach-types.h generator simple and
solve this problem.

Please comment and show me if I think is wrong.
Thanks!

>From 7d4a967edb7bcb840d6d526b8de4a40d63a8ea82 Mon Sep 17 00:00:00 2001
From: Alexander Shiyan <shc_work@mail.ru>
Date: Fri, 7 Dec 2012 19:01:26 +0400
Subject: [PATCH] Simplify mach-types.h


Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/boot/compressed/misc.c |    5 ++---
 arch/arm/kernel/devtree.c       |    2 +-
 arch/arm/kernel/head-common.S   |    2 +-
 arch/arm/kernel/setup.c         |    4 ++--
 arch/arm/mach-pxa/em-x270.c     |    2 --
 arch/arm/mach-pxa/trizeps4.c    |    4 ++--
 arch/arm/tools/gen-mach-types   |   11 +----------
 7 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/compressed/misc.c b/arch/arm/boot/compressed/misc.c
index df89983..86a2b0d 100644
--- a/arch/arm/boot/compressed/misc.c
+++ b/arch/arm/boot/compressed/misc.c
@@ -16,8 +16,6 @@
  *  This allows for a much quicker boot time.
  */
 
-unsigned int __machine_arch_type;
-
 #include <linux/compiler.h>	/* for inline */
 #include <linux/types.h>
 #include <linux/linkage.h>
@@ -112,6 +110,7 @@ unsigned char *output_data;
 
 unsigned long free_mem_ptr;
 unsigned long free_mem_end_ptr;
+unsigned int machine_arch_type;
 
 #ifndef arch_error
 #define arch_error(x)
@@ -146,7 +145,7 @@ decompress_kernel(unsigned long output_start, unsigned long free_mem_ptr_p,
 	output_data		= (unsigned char *)output_start;
 	free_mem_ptr		= free_mem_ptr_p;
 	free_mem_end_ptr	= free_mem_ptr_end_p;
-	__machine_arch_type	= arch_id;
+	machine_arch_type	= arch_id;
 
 	arch_decomp_setup();
 
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 70f1bde..bbb9a02 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -232,7 +232,7 @@ struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 
 	/* Change machine number to match the mdesc we're using */
-	__machine_arch_type = mdesc_best->nr;
+	machine_arch_type = mdesc_best->nr;
 
 	return mdesc_best;
 }
diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 2f560c5..6e02251 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -112,7 +112,7 @@ __mmap_switched_data:
 	.long	__bss_start			@ r6
 	.long	_end				@ r7
 	.long	processor_id			@ r4
-	.long	__machine_arch_type		@ r5
+	.long	machine_arch_type		@ r5
 	.long	__atags_pointer			@ r6
 #ifdef CONFIG_CPU_CP15
 	.long	cr_alignment			@ r7
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 86d1429..dc3c4b0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -78,8 +78,8 @@ extern void setup_dma_zone(struct machine_desc *desc);
 
 unsigned int processor_id;
 EXPORT_SYMBOL(processor_id);
-unsigned int __machine_arch_type __read_mostly;
-EXPORT_SYMBOL(__machine_arch_type);
+unsigned int machine_arch_type __read_mostly;
+EXPORT_SYMBOL(machine_arch_type);
 unsigned int cacheid __read_mostly;
 EXPORT_SYMBOL(cacheid);
 
diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
index 1b64114..939079a 100644
--- a/arch/arm/mach-pxa/em-x270.c
+++ b/arch/arm/mach-pxa/em-x270.c
@@ -1272,8 +1272,6 @@ static void __init em_x270_init(void)
 		em_x270_module_init();
 	else if (machine_is_exeda())
 		em_x270_exeda_init();
-	else
-		panic("Unsupported machine: %d\n", machine_arch_type);
 
 	em_x270_init_da9030();
 	em_x270_init_dm9000();
diff --git a/arch/arm/mach-pxa/trizeps4.c b/arch/arm/mach-pxa/trizeps4.c
index fbbcbed..7988e8b 100644
--- a/arch/arm/mach-pxa/trizeps4.c
+++ b/arch/arm/mach-pxa/trizeps4.c
@@ -544,11 +544,11 @@ static void __init trizeps4_map_io(void)
 
 	if ((__raw_readl(MSC0) & 0x8) && (__raw_readl(BOOT_DEF) & 0x1)) {
 		/* if flash is 16 bit wide its a Trizeps4 WL */
-		__machine_arch_type = MACH_TYPE_TRIZEPS4WL;
+		machine_arch_type = MACH_TYPE_TRIZEPS4WL;
 		trizeps4_flash_data[0].width = 2;
 	} else {
 		/* if flash is 32 bit wide its a Trizeps4 */
-		__machine_arch_type = MACH_TYPE_TRIZEPS4;
+		machine_arch_type = MACH_TYPE_TRIZEPS4;
 		trizeps4_flash_data[0].width = 4;
 	}
 }
diff --git a/arch/arm/tools/gen-mach-types b/arch/arm/tools/gen-mach-types
index 04fef71..d36642d 100644
--- a/arch/arm/tools/gen-mach-types
+++ b/arch/arm/tools/gen-mach-types
@@ -30,7 +30,7 @@ END	{
 	  printf("#define __ASM_ARM_MACH_TYPE_H\n\n");
 	  printf("#ifndef __ASSEMBLY__\n");
 	  printf("/* The type of machine we're running on */\n");
-	  printf("extern unsigned int __machine_arch_type;\n");
+	  printf("extern unsigned int machine_arch_type;\n");
 	  printf("#endif\n\n");
 
 	  printf("/* see arch/arm/kernel/arch.c for a description of these */\n");
@@ -43,12 +43,6 @@ END	{
 	  for (i = 0; i < nr; i++)
 	    if (num[i] ~ /..*/) {
 	      printf("#ifdef %s\n", config[i]);
-	      printf("# ifdef machine_arch_type\n");
-	      printf("#  undef machine_arch_type\n");
-	      printf("#  define machine_arch_type\t__machine_arch_type\n");
-	      printf("# else\n");
-	      printf("#  define machine_arch_type\t%s\n", mach_type[i]);
-	      printf("# endif\n");
 	      printf("# define %s()\t(machine_arch_type == %s)\n", machine_is[i], mach_type[i]);
 	      printf("#else\n");
 	      printf("# define %s()\t(0)\n", machine_is[i]);
@@ -65,8 +59,5 @@ END	{
 	      printf("#define %s()\t(0)\n", machine_is[i]);
 	    }
 
-	  printf("\n#ifndef machine_arch_type\n");
-	  printf("#define machine_arch_type\t__machine_arch_type\n");
-	  printf("#endif\n\n");
 	  printf("#endif\n");
 	}
-- 
1.7.8.6

-- 
Alexander Shiyan <shc_work@mail.ru>

Question: Multiple board support is broken

[Russell King - ARM Linux]

On Fri, Dec 07, 2012 at 07:31:43PM +0400, Alexander Shiyan wrote:
> Hello.
> 
> Today I was tested multiple boards (not multiplatform) in the kernel
> and found problems with booting.

Err, I test this almost constantly and it works, and it's been working
100% correctly for about the last 12 years.  There is no problem here.

> Exacly, if multiple boards defined
> in. config, we can proceed to boot only the last (determined by mach number).

The mach-types file doesn't have anything to do with which board gets chosen.
That file just provides the IDs, and a bunch of _optimized_ macros to
allow the compiler to perform optimizations.

Let's look at how this works today - let me pull out two entries:

#ifdef CONFIG_ARCH_EBSA285
# ifdef machine_arch_type
#  undef machine_arch_type
#  define machine_arch_type     __machine_arch_type
# else
#  define machine_arch_type     MACH_TYPE_EBSA285
# endif
# define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
#else
# define machine_is_ebsa285()   (0)
#endif

#ifdef CONFIG_ARCH_NETWINDER
# ifdef machine_arch_type
#  undef machine_arch_type
#  define machine_arch_type     __machine_arch_type
# else
#  define machine_arch_type     MACH_TYPE_NETWINDER
# endif
# define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
#else
# define machine_is_netwinder() (0)
#endif

#ifndef machine_arch_type
#define machine_arch_type       __machine_arch_type
#endif

Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
through what the preprocessor does.  In the first case, machine_arch_type
is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
That means machine_arch_type gets defined as MACH_TYPE_EBSA285.

machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
and when all the macro subsitutions occur (which happens where it's used)
this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
always true, so any code inside an if (machine_is_ebsa285()) {} block will
be compiled into the kernel and will be executed unconditionally - which
is exactly what we want.

Now, machine_is_netwinder() gets defined as constant (0).  This is always
false, so any code inside an if (machine_is_netwinder()) {} block will
be optimized away - exactly what we want.  This isn't affected by the
machine_arch_type definition.

Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.

Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
This time machine_arch_type is already defined.

So, what happens is machine_arch_type first gets undefined to avoid any
compiler warnings about multiple definitions.  We then define
machine_arch_type to be the C variable __machine_arch_type.

This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
become a runtime interpreted condition, which occurs for any machine type
that has its config symbol enabled.  So, machine_is_ebsa285() becomes
(__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
platforms machine_is_xxx() macros remain defined to constant 0.

So, this all works exactly as we want.  There is no bug here.

Question: Multiple board support is broken

[Alexander Shiyan]

On Fri, 7 Dec 2012 16:17:54 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > Today I was tested multiple boards (not multiplatform) in the kernel
> > and found problems with booting.
> 
> Err, I test this almost constantly and it works, and it's been working
> 100% correctly for about the last 12 years.  There is no problem here.
> 
> > Exacly, if multiple boards defined
> > in. config, we can proceed to boot only the last (determined by mach number).
> 
> The mach-types file doesn't have anything to do with which board gets chosen.
> That file just provides the IDs, and a bunch of _optimized_ macros to
> allow the compiler to perform optimizations.
> 
> Let's look at how this works today - let me pull out two entries:
> 
> #ifdef CONFIG_ARCH_EBSA285
> # ifdef machine_arch_type
> #  undef machine_arch_type
> #  define machine_arch_type     __machine_arch_type
> # else
> #  define machine_arch_type     MACH_TYPE_EBSA285
> # endif
> # define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
> #else
> # define machine_is_ebsa285()   (0)
> #endif
> 
> #ifdef CONFIG_ARCH_NETWINDER
> # ifdef machine_arch_type
> #  undef machine_arch_type
> #  define machine_arch_type     __machine_arch_type
> # else
> #  define machine_arch_type     MACH_TYPE_NETWINDER
> # endif
> # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> #else
> # define machine_is_netwinder() (0)
> #endif
> 
> #ifndef machine_arch_type
> #define machine_arch_type       __machine_arch_type
> #endif
> 
> Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
> through what the preprocessor does.  In the first case, machine_arch_type
> is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
> That means machine_arch_type gets defined as MACH_TYPE_EBSA285.
> 
> machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
> and when all the macro subsitutions occur (which happens where it's used)
> this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
> always true, so any code inside an if (machine_is_ebsa285()) {} block will
> be compiled into the kernel and will be executed unconditionally - which
> is exactly what we want.
> 
> Now, machine_is_netwinder() gets defined as constant (0).  This is always
> false, so any code inside an if (machine_is_netwinder()) {} block will
> be optimized away - exactly what we want.  This isn't affected by the
> machine_arch_type definition.
> 
> Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
> CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.
> 
> Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
> In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
> When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
> This time machine_arch_type is already defined.
> 
> So, what happens is machine_arch_type first gets undefined to avoid any
> compiler warnings about multiple definitions.  We then define
> machine_arch_type to be the C variable __machine_arch_type.
> 
> This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
> become a runtime interpreted condition, which occurs for any machine type
> that has its config symbol enabled.  So, machine_is_ebsa285() becomes
> (__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
> becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
> platforms machine_is_xxx() macros remain defined to constant 0.
> 
> So, this all works exactly as we want.  There is no bug here.

Apparently the problem is that I have a custom symbol for their own board,
but I specify an existing ID in the MACHINE_START. The initial boot is fine,
because support for the ID is in the kernel, but the mach_desc is taken from
a different machine. Below is a test program to demonstrate. In this test I
boot kernel with ID=4 and I have a valid machines with ID=4 and 5, but symbol
associated with ID=4 is missing. This case will a result boot a machine ID=5.
How to solve the problem, I do not know yet...

<<< test.c
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

//#define CONFIG_ARCH_EBSA285 1
#define CONFIG_ARCH_NETWINDER 1

#define MACH_TYPE_EBSA285 4
#define MACH_TYPE_NETWINDER 5

#ifdef CONFIG_ARCH_EBSA285
# ifdef machine_arch_type
# undef machine_arch_type
# define machine_arch_type __machine_arch_type
# else
# define machine_arch_type MACH_TYPE_EBSA285
# endif
# define machine_is_ebsa285() (machine_arch_type == MACH_TYPE_EBSA285)
#else
# define machine_is_ebsa285() (0)
#endif

#ifdef CONFIG_ARCH_NETWINDER
# ifdef machine_arch_type
# undef machine_arch_type
# define machine_arch_type __machine_arch_type
# else
# define machine_arch_type MACH_TYPE_NETWINDER
# endif
# define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
#else
# define machine_is_netwinder() (0)
#endif

#ifndef machine_arch_type
#define machine_arch_type __machine_arch_type
#endif

unsigned int __machine_arch_type = MACH_TYPE_EBSA285; //From bootloader

int main(int argc, char** argv)
{
	if (machine_is_ebsa285())
		printf("machine_is_ebsa285()\n");
	if (machine_is_netwinder())
		printf("machine_is_netwinder()\n");
	printf("  machine_arch_type=%u\n", machine_arch_type);
	printf("__machine_arch_type=%u\n", __machine_arch_type);

	return 0;
}
>>>

shc at shc /home/git/test $ ./test 
machine_is_netwinder()
  machine_arch_type=5
__machine_arch_type=4

-- 
Alexander Shiyan <shc_work@mail.ru>

Question: Multiple board support is broken

[Russell King - ARM Linux]

On Sat, Dec 08, 2012 at 12:28:11PM +0400, Alexander Shiyan wrote:
> On Fri, 7 Dec 2012 16:17:54 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > Today I was tested multiple boards (not multiplatform) in the kernel
> > > and found problems with booting.
> > 
> > Err, I test this almost constantly and it works, and it's been working
> > 100% correctly for about the last 12 years.  There is no problem here.
> > 
> > > Exacly, if multiple boards defined
> > > in. config, we can proceed to boot only the last (determined by mach number).
> > 
> > The mach-types file doesn't have anything to do with which board gets chosen.
> > That file just provides the IDs, and a bunch of _optimized_ macros to
> > allow the compiler to perform optimizations.
> > 
> > Let's look at how this works today - let me pull out two entries:
> > 
> > #ifdef CONFIG_ARCH_EBSA285
> > # ifdef machine_arch_type
> > #  undef machine_arch_type
> > #  define machine_arch_type     __machine_arch_type
> > # else
> > #  define machine_arch_type     MACH_TYPE_EBSA285
> > # endif
> > # define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
> > #else
> > # define machine_is_ebsa285()   (0)
> > #endif
> > 
> > #ifdef CONFIG_ARCH_NETWINDER
> > # ifdef machine_arch_type
> > #  undef machine_arch_type
> > #  define machine_arch_type     __machine_arch_type
> > # else
> > #  define machine_arch_type     MACH_TYPE_NETWINDER
> > # endif
> > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > #else
> > # define machine_is_netwinder() (0)
> > #endif
> > 
> > #ifndef machine_arch_type
> > #define machine_arch_type       __machine_arch_type
> > #endif
> > 
> > Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
> > through what the preprocessor does.  In the first case, machine_arch_type
> > is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
> > That means machine_arch_type gets defined as MACH_TYPE_EBSA285.
> > 
> > machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
> > and when all the macro subsitutions occur (which happens where it's used)
> > this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
> > always true, so any code inside an if (machine_is_ebsa285()) {} block will
> > be compiled into the kernel and will be executed unconditionally - which
> > is exactly what we want.
> > 
> > Now, machine_is_netwinder() gets defined as constant (0).  This is always
> > false, so any code inside an if (machine_is_netwinder()) {} block will
> > be optimized away - exactly what we want.  This isn't affected by the
> > machine_arch_type definition.
> > 
> > Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
> > CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.
> > 
> > Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
> > In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
> > When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
> > This time machine_arch_type is already defined.
> > 
> > So, what happens is machine_arch_type first gets undefined to avoid any
> > compiler warnings about multiple definitions.  We then define
> > machine_arch_type to be the C variable __machine_arch_type.
> > 
> > This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
> > become a runtime interpreted condition, which occurs for any machine type
> > that has its config symbol enabled.  So, machine_is_ebsa285() becomes
> > (__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
> > becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
> > platforms machine_is_xxx() macros remain defined to constant 0.
> > 
> > So, this all works exactly as we want.  There is no bug here.
> 
> Apparently the problem is that I have a custom symbol for their own board,
> but I specify an existing ID in the MACHINE_START. The initial boot is fine,
> because support for the ID is in the kernel, but the mach_desc is taken from
> a different machine. Below is a test program to demonstrate. In this test I
> boot kernel with ID=4 and I have a valid machines with ID=4 and 5, but symbol
> associated with ID=4 is missing. This case will a result boot a machine ID=5.
> How to solve the problem, I do not know yet...
> 
> <<< test.c
> #include <stdio.h>
> #include <string.h>
> #include <stdlib.h>
> 
> //#define CONFIG_ARCH_EBSA285 1
> #define CONFIG_ARCH_NETWINDER 1
> 
> #define MACH_TYPE_EBSA285 4
> #define MACH_TYPE_NETWINDER 5
> 
> #ifdef CONFIG_ARCH_EBSA285
> # ifdef machine_arch_type
> # undef machine_arch_type
> # define machine_arch_type __machine_arch_type
> # else
> # define machine_arch_type MACH_TYPE_EBSA285
> # endif
> # define machine_is_ebsa285() (machine_arch_type == MACH_TYPE_EBSA285)
> #else
> # define machine_is_ebsa285() (0)
> #endif
> 
> #ifdef CONFIG_ARCH_NETWINDER
> # ifdef machine_arch_type
> # undef machine_arch_type
> # define machine_arch_type __machine_arch_type
> # else
> # define machine_arch_type MACH_TYPE_NETWINDER
> # endif
> # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> #else
> # define machine_is_netwinder() (0)
> #endif
> 
> #ifndef machine_arch_type
> #define machine_arch_type __machine_arch_type
> #endif
> 
> unsigned int __machine_arch_type = MACH_TYPE_EBSA285; //From bootloader
> 
> int main(int argc, char** argv)
> {
> 	if (machine_is_ebsa285())
> 		printf("machine_is_ebsa285()\n");
> 	if (machine_is_netwinder())
> 		printf("machine_is_netwinder()\n");
> 	printf("  machine_arch_type=%u\n", machine_arch_type);
> 	printf("__machine_arch_type=%u\n", __machine_arch_type);
> 
> 	return 0;
> }
> >>>
> 
> shc at shc /home/git/test $ ./test 
> machine_is_netwinder()
>   machine_arch_type=5
> __machine_arch_type=4

That is working as designed, nothing wrong there.  You have EBSA285
support disabled (you commented out CONFIG_ARCH_EBSA285) and you have
Netwinder support enabled (CONFIG_ARCH_NETWINDER).  So,
machine_is_ebsa285() is constant false and machine_is_netwinder() is
constant true.

However, such a kernel won't boot when passed ID 5 because it won't
find the platform record in the kernel, and we explicitly error out
in that case.

Question: Multiple board support is broken

[Alexander Shiyan]

On Sat, 8 Dec 2012 09:26:18 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Sat, Dec 08, 2012 at 12:28:11PM +0400, Alexander Shiyan wrote:
> > On Fri, 7 Dec 2012 16:17:54 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 
> > > > Today I was tested multiple boards (not multiplatform) in the kernel
> > > > and found problems with booting.
> > > 
> > > Err, I test this almost constantly and it works, and it's been working
> > > 100% correctly for about the last 12 years.  There is no problem here.
> > > 
> > > > Exacly, if multiple boards defined
> > > > in. config, we can proceed to boot only the last (determined by mach number).
> > > 
> > > The mach-types file doesn't have anything to do with which board gets chosen.
> > > That file just provides the IDs, and a bunch of _optimized_ macros to
> > > allow the compiler to perform optimizations.
> > > 
> > > Let's look at how this works today - let me pull out two entries:
> > > 
> > > #ifdef CONFIG_ARCH_EBSA285
> > > # ifdef machine_arch_type
> > > #  undef machine_arch_type
> > > #  define machine_arch_type     __machine_arch_type
> > > # else
> > > #  define machine_arch_type     MACH_TYPE_EBSA285
> > > # endif
> > > # define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
> > > #else
> > > # define machine_is_ebsa285()   (0)
> > > #endif
> > > 
> > > #ifdef CONFIG_ARCH_NETWINDER
> > > # ifdef machine_arch_type
> > > #  undef machine_arch_type
> > > #  define machine_arch_type     __machine_arch_type
> > > # else
> > > #  define machine_arch_type     MACH_TYPE_NETWINDER
> > > # endif
> > > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > > #else
> > > # define machine_is_netwinder() (0)
> > > #endif
> > > 
> > > #ifndef machine_arch_type
> > > #define machine_arch_type       __machine_arch_type
> > > #endif
> > > 
> > > Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
> > > through what the preprocessor does.  In the first case, machine_arch_type
> > > is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
> > > That means machine_arch_type gets defined as MACH_TYPE_EBSA285.
> > > 
> > > machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
> > > and when all the macro subsitutions occur (which happens where it's used)
> > > this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
> > > always true, so any code inside an if (machine_is_ebsa285()) {} block will
> > > be compiled into the kernel and will be executed unconditionally - which
> > > is exactly what we want.
> > > 
> > > Now, machine_is_netwinder() gets defined as constant (0).  This is always
> > > false, so any code inside an if (machine_is_netwinder()) {} block will
> > > be optimized away - exactly what we want.  This isn't affected by the
> > > machine_arch_type definition.
> > > 
> > > Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
> > > CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.
> > > 
> > > Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
> > > In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
> > > When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
> > > This time machine_arch_type is already defined.
> > > 
> > > So, what happens is machine_arch_type first gets undefined to avoid any
> > > compiler warnings about multiple definitions.  We then define
> > > machine_arch_type to be the C variable __machine_arch_type.
> > > 
> > > This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
> > > become a runtime interpreted condition, which occurs for any machine type
> > > that has its config symbol enabled.  So, machine_is_ebsa285() becomes
> > > (__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
> > > becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
> > > platforms machine_is_xxx() macros remain defined to constant 0.
> > > 
> > > So, this all works exactly as we want.  There is no bug here.
> > 
> > Apparently the problem is that I have a custom symbol for their own board,
> > but I specify an existing ID in the MACHINE_START. The initial boot is fine,
> > because support for the ID is in the kernel, but the mach_desc is taken from
> > a different machine. Below is a test program to demonstrate. In this test I
> > boot kernel with ID=4 and I have a valid machines with ID=4 and 5, but symbol
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> > associated with ID=4 is missing. This case will a result boot a machine ID=5.
> > How to solve the problem, I do not know yet...
> > 
> > <<< test.c
> > #include <stdio.h>
> > #include <string.h>
> > #include <stdlib.h>
> > 
> > //#define CONFIG_ARCH_EBSA285 1
> > #define CONFIG_ARCH_NETWINDER 1
> > 
> > #define MACH_TYPE_EBSA285 4
> > #define MACH_TYPE_NETWINDER 5
> > 
> > #ifdef CONFIG_ARCH_EBSA285
> > # ifdef machine_arch_type
> > # undef machine_arch_type
> > # define machine_arch_type __machine_arch_type
> > # else
> > # define machine_arch_type MACH_TYPE_EBSA285
> > # endif
> > # define machine_is_ebsa285() (machine_arch_type == MACH_TYPE_EBSA285)
> > #else
> > # define machine_is_ebsa285() (0)
> > #endif
> > 
> > #ifdef CONFIG_ARCH_NETWINDER
> > # ifdef machine_arch_type
> > # undef machine_arch_type
> > # define machine_arch_type __machine_arch_type
> > # else
> > # define machine_arch_type MACH_TYPE_NETWINDER
> > # endif
> > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > #else
> > # define machine_is_netwinder() (0)
> > #endif
> > 
> > #ifndef machine_arch_type
> > #define machine_arch_type __machine_arch_type
> > #endif
> > 
> > unsigned int __machine_arch_type = MACH_TYPE_EBSA285; //From bootloader
> > 
> > int main(int argc, char** argv)
> > {
> > 	if (machine_is_ebsa285())
> > 		printf("machine_is_ebsa285()\n");
> > 	if (machine_is_netwinder())
> > 		printf("machine_is_netwinder()\n");
> > 	printf("  machine_arch_type=%u\n", machine_arch_type);
> > 	printf("__machine_arch_type=%u\n", __machine_arch_type);
> > 
> > 	return 0;
> > }
> > >>>
> > 
> > shc at shc /home/git/test $ ./test 
> > machine_is_netwinder()
> >   machine_arch_type=5
> > __machine_arch_type=4
> 
> That is working as designed, nothing wrong there.  You have EBSA285
> support disabled (you commented out CONFIG_ARCH_EBSA285) and you have
> Netwinder support enabled (CONFIG_ARCH_NETWINDER).  So,
> machine_is_ebsa285() is constant false and machine_is_netwinder() is
> constant true.
> 
> However, such a kernel won't boot when passed ID 5 because it won't
> find the platform record in the kernel, and we explicitly error out
> in that case.
Why not? As I say before that I have a two valid machines with ID 4 and 5.
I can boot machine ID 5 by specifying this ID in the bootloader.

In any case, it's my fault that I do not specify the correct symbol,
associated with the ID. But even such issues can be avoided if we make
a comparison with the ID passed from the bootloader and not redefine it, ie:
#ifdef CONFIG_ARCH_NETWINDER
# define machine_is_netwinder() (__machine_arch_type == MACH_TYPE_NETWINDER)
#else
# define machine_is_netwinder() (0)
#endif

In this case, if we pass the ID 4 to bootloader, we will never have a macro
machine_is_xx() (that's right), but boot the correct machine.

Look at my solution for "gen_mach_types" in the first message of this thread.
Fixme, but on my opinion it will not affect the optimization.

-- 
Alexander Shiyan <shc_work@mail.ru>

Question: Multiple board support is broken

[Russell King - ARM Linux]

On Sat, Dec 08, 2012 at 01:58:45PM +0400, Alexander Shiyan wrote:
> On Sat, 8 Dec 2012 09:26:18 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Sat, Dec 08, 2012 at 12:28:11PM +0400, Alexander Shiyan wrote:
> > > On Fri, 7 Dec 2012 16:17:54 +0000
> > > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > > 
> > > > > Today I was tested multiple boards (not multiplatform) in the kernel
> > > > > and found problems with booting.
> > > > 
> > > > Err, I test this almost constantly and it works, and it's been working
> > > > 100% correctly for about the last 12 years.  There is no problem here.
> > > > 
> > > > > Exacly, if multiple boards defined
> > > > > in. config, we can proceed to boot only the last (determined by mach number).
> > > > 
> > > > The mach-types file doesn't have anything to do with which board gets chosen.
> > > > That file just provides the IDs, and a bunch of _optimized_ macros to
> > > > allow the compiler to perform optimizations.
> > > > 
> > > > Let's look at how this works today - let me pull out two entries:
> > > > 
> > > > #ifdef CONFIG_ARCH_EBSA285
> > > > # ifdef machine_arch_type
> > > > #  undef machine_arch_type
> > > > #  define machine_arch_type     __machine_arch_type
> > > > # else
> > > > #  define machine_arch_type     MACH_TYPE_EBSA285
> > > > # endif
> > > > # define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
> > > > #else
> > > > # define machine_is_ebsa285()   (0)
> > > > #endif
> > > > 
> > > > #ifdef CONFIG_ARCH_NETWINDER
> > > > # ifdef machine_arch_type
> > > > #  undef machine_arch_type
> > > > #  define machine_arch_type     __machine_arch_type
> > > > # else
> > > > #  define machine_arch_type     MACH_TYPE_NETWINDER
> > > > # endif
> > > > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > > > #else
> > > > # define machine_is_netwinder() (0)
> > > > #endif
> > > > 
> > > > #ifndef machine_arch_type
> > > > #define machine_arch_type       __machine_arch_type
> > > > #endif
> > > > 
> > > > Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
> > > > through what the preprocessor does.  In the first case, machine_arch_type
> > > > is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
> > > > That means machine_arch_type gets defined as MACH_TYPE_EBSA285.
> > > > 
> > > > machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
> > > > and when all the macro subsitutions occur (which happens where it's used)
> > > > this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
> > > > always true, so any code inside an if (machine_is_ebsa285()) {} block will
> > > > be compiled into the kernel and will be executed unconditionally - which
> > > > is exactly what we want.
> > > > 
> > > > Now, machine_is_netwinder() gets defined as constant (0).  This is always
> > > > false, so any code inside an if (machine_is_netwinder()) {} block will
> > > > be optimized away - exactly what we want.  This isn't affected by the
> > > > machine_arch_type definition.
> > > > 
> > > > Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
> > > > CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.
> > > > 
> > > > Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
> > > > In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
> > > > When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
> > > > This time machine_arch_type is already defined.
> > > > 
> > > > So, what happens is machine_arch_type first gets undefined to avoid any
> > > > compiler warnings about multiple definitions.  We then define
> > > > machine_arch_type to be the C variable __machine_arch_type.
> > > > 
> > > > This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
> > > > become a runtime interpreted condition, which occurs for any machine type
> > > > that has its config symbol enabled.  So, machine_is_ebsa285() becomes
> > > > (__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
> > > > becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
> > > > platforms machine_is_xxx() macros remain defined to constant 0.
> > > > 
> > > > So, this all works exactly as we want.  There is no bug here.
> > > 
> > > Apparently the problem is that I have a custom symbol for their own board,
> > > but I specify an existing ID in the MACHINE_START. The initial boot is fine,
> > > because support for the ID is in the kernel, but the mach_desc is taken from
> > > a different machine. Below is a test program to demonstrate. In this test I
> > > boot kernel with ID=4 and I have a valid machines with ID=4 and 5, but symbol
>                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> > > associated with ID=4 is missing. This case will a result boot a machine ID=5.
> > > How to solve the problem, I do not know yet...
> > > 
> > > <<< test.c
> > > #include <stdio.h>
> > > #include <string.h>
> > > #include <stdlib.h>
> > > 
> > > //#define CONFIG_ARCH_EBSA285 1
> > > #define CONFIG_ARCH_NETWINDER 1
> > > 
> > > #define MACH_TYPE_EBSA285 4
> > > #define MACH_TYPE_NETWINDER 5
> > > 
> > > #ifdef CONFIG_ARCH_EBSA285
> > > # ifdef machine_arch_type
> > > # undef machine_arch_type
> > > # define machine_arch_type __machine_arch_type
> > > # else
> > > # define machine_arch_type MACH_TYPE_EBSA285
> > > # endif
> > > # define machine_is_ebsa285() (machine_arch_type == MACH_TYPE_EBSA285)
> > > #else
> > > # define machine_is_ebsa285() (0)
> > > #endif
> > > 
> > > #ifdef CONFIG_ARCH_NETWINDER
> > > # ifdef machine_arch_type
> > > # undef machine_arch_type
> > > # define machine_arch_type __machine_arch_type
> > > # else
> > > # define machine_arch_type MACH_TYPE_NETWINDER
> > > # endif
> > > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > > #else
> > > # define machine_is_netwinder() (0)
> > > #endif
> > > 
> > > #ifndef machine_arch_type
> > > #define machine_arch_type __machine_arch_type
> > > #endif
> > > 
> > > unsigned int __machine_arch_type = MACH_TYPE_EBSA285; //From bootloader
> > > 
> > > int main(int argc, char** argv)
> > > {
> > > 	if (machine_is_ebsa285())
> > > 		printf("machine_is_ebsa285()\n");
> > > 	if (machine_is_netwinder())
> > > 		printf("machine_is_netwinder()\n");
> > > 	printf("  machine_arch_type=%u\n", machine_arch_type);
> > > 	printf("__machine_arch_type=%u\n", __machine_arch_type);
> > > 
> > > 	return 0;
> > > }
> > > >>>
> > > 
> > > shc at shc /home/git/test $ ./test 
> > > machine_is_netwinder()
> > >   machine_arch_type=5
> > > __machine_arch_type=4
> > 
> > That is working as designed, nothing wrong there.  You have EBSA285
> > support disabled (you commented out CONFIG_ARCH_EBSA285) and you have
> > Netwinder support enabled (CONFIG_ARCH_NETWINDER).  So,
> > machine_is_ebsa285() is constant false and machine_is_netwinder() is
> > constant true.
> > 
> > However, such a kernel won't boot when passed ID 5 because it won't
> > find the platform record in the kernel, and we explicitly error out
> > in that case.
>
> Why not? As I say before that I have a two valid machines with ID 4 and 5.
> I can boot machine ID 5 by specifying this ID in the bootloader.

If you have two machines with ID 4 and ID 5, and you configure out of the
kernel support for ID 4, then pass ID 4 to the kernel, the kernel should
error out.

> In any case, it's my fault that I do not specify the correct symbol,
> associated with the ID.

That is why we have the machine database, to keep things straight.

> But even such issues can be avoided if we make
> a comparison with the ID passed from the bootloader and not redefine it, ie:
> #ifdef CONFIG_ARCH_NETWINDER
> # define machine_is_netwinder() (__machine_arch_type == MACH_TYPE_NETWINDER)
> #else
> # define machine_is_netwinder() (0)
> #endif

No, you haven't listened to what I've said.  Go back and read my
explanation about how this works, particularly the part where these
macros end up being constant false and constant true.

> In this case, if we pass the ID 4 to bootloader, we will never have a macro
> machine_is_xx() (that's right), but boot the correct machine.

Again, you're not listening to what I've said.

> Look at my solution for "gen_mach_types" in the first message of this thread.
> Fixme, but on my opinion it will not affect the optimization.

You're wrong.  You don't understand what's going on here and why it is
the way that it is.  There is nothing wrong here, at all.  It is all
working as designed.  Your changes make it work sub-optimally by getting
rid of the constant-true optimization.

Question: Multiple board support is broken

[Alexander Shiyan]

On Sat, 8 Dec 2012 10:38:12 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > > > > > Today I was tested multiple boards (not multiplatform) in the kernel
> > > > > > and found problems with booting.
> > > > > 
> > > > > Err, I test this almost constantly and it works, and it's been working
> > > > > 100% correctly for about the last 12 years.  There is no problem here.
> > > > > 
> > > > > > Exacly, if multiple boards defined
> > > > > > in. config, we can proceed to boot only the last (determined by mach number).
> > > > > 
> > > > > The mach-types file doesn't have anything to do with which board gets chosen.
> > > > > That file just provides the IDs, and a bunch of _optimized_ macros to
> > > > > allow the compiler to perform optimizations.
> > > > > 
> > > > > Let's look at how this works today - let me pull out two entries:
> > > > > 
> > > > > #ifdef CONFIG_ARCH_EBSA285
> > > > > # ifdef machine_arch_type
> > > > > #  undef machine_arch_type
> > > > > #  define machine_arch_type     __machine_arch_type
> > > > > # else
> > > > > #  define machine_arch_type     MACH_TYPE_EBSA285
> > > > > # endif
> > > > > # define machine_is_ebsa285()   (machine_arch_type == MACH_TYPE_EBSA285)
> > > > > #else
> > > > > # define machine_is_ebsa285()   (0)
> > > > > #endif
> > > > > 
> > > > > #ifdef CONFIG_ARCH_NETWINDER
> > > > > # ifdef machine_arch_type
> > > > > #  undef machine_arch_type
> > > > > #  define machine_arch_type     __machine_arch_type
> > > > > # else
> > > > > #  define machine_arch_type     MACH_TYPE_NETWINDER
> > > > > # endif
> > > > > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > > > > #else
> > > > > # define machine_is_netwinder() (0)
> > > > > #endif
> > > > > 
> > > > > #ifndef machine_arch_type
> > > > > #define machine_arch_type       __machine_arch_type
> > > > > #endif
> > > > > 
> > > > > Now, if CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=n, then follow
> > > > > through what the preprocessor does.  In the first case, machine_arch_type
> > > > > is _not_ defined as a pre-processor symbol, so the "ifdef" is false.
> > > > > That means machine_arch_type gets defined as MACH_TYPE_EBSA285.
> > > > > 
> > > > > machine_is_ebsa285() gets defined as (machine_arch_type == MACH_TYPE_EBSA285)
> > > > > and when all the macro subsitutions occur (which happens where it's used)
> > > > > this ends up becoming (MACH_TYPE_EBSA285 == MACH_TYPE_EBSA285).  This is
> > > > > always true, so any code inside an if (machine_is_ebsa285()) {} block will
> > > > > be compiled into the kernel and will be executed unconditionally - which
> > > > > is exactly what we want.
> > > > > 
> > > > > Now, machine_is_netwinder() gets defined as constant (0).  This is always
> > > > > false, so any code inside an if (machine_is_netwinder()) {} block will
> > > > > be optimized away - exactly what we want.  This isn't affected by the
> > > > > machine_arch_type definition.
> > > > > 
> > > > > Next, you can do the same thing for the CONFIG_ARCH_EBSA285=n and
> > > > > CONFIG_ARCH_NETWINDER=y case, and you'll end up with similar results.
> > > > > 
> > > > > Finally for the case where CONFIG_ARCH_EBSA285=y and CONFIG_ARCH_NETWINDER=y.
> > > > > In this case, it starts off just like the CONFIG_ARCH_EBSA285=y case above.
> > > > > When we hit the CONFIG_ARCH_NETWINDER block a very important change happens.
> > > > > This time machine_arch_type is already defined.
> > > > > 
> > > > > So, what happens is machine_arch_type first gets undefined to avoid any
> > > > > compiler warnings about multiple definitions.  We then define
> > > > > machine_arch_type to be the C variable __machine_arch_type.
> > > > > 
> > > > > This makes any references to (machine_arch_type == MACH_TYPE_WHATEVER)
> > > > > become a runtime interpreted condition, which occurs for any machine type
> > > > > that has its config symbol enabled.  So, machine_is_ebsa285() becomes
> > > > > (__machine_arch_type == MACH_TYPE_EBSA285) and machine_is_netwinder()
> > > > > becomes (__machine_arch_type == MACH_TYPE_NETWINDER), while other
> > > > > platforms machine_is_xxx() macros remain defined to constant 0.
> > > > > 
> > > > > So, this all works exactly as we want.  There is no bug here.
> > > > 
> > > > Apparently the problem is that I have a custom symbol for their own board,
> > > > but I specify an existing ID in the MACHINE_START. The initial boot is fine,
> > > > because support for the ID is in the kernel, but the mach_desc is taken from
> > > > a different machine. Below is a test program to demonstrate. In this test I
> > > > boot kernel with ID=4 and I have a valid machines with ID=4 and 5, but symbol
> >                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > > > associated with ID=4 is missing. This case will a result boot a machine ID=5.
> > > > How to solve the problem, I do not know yet...
> > > > 
> > > > <<< test.c
> > > > #include <stdio.h>
> > > > #include <string.h>
> > > > #include <stdlib.h>
> > > > 
> > > > //#define CONFIG_ARCH_EBSA285 1
> > > > #define CONFIG_ARCH_NETWINDER 1
> > > > 
> > > > #define MACH_TYPE_EBSA285 4
> > > > #define MACH_TYPE_NETWINDER 5
> > > > 
> > > > #ifdef CONFIG_ARCH_EBSA285
> > > > # ifdef machine_arch_type
> > > > # undef machine_arch_type
> > > > # define machine_arch_type __machine_arch_type
> > > > # else
> > > > # define machine_arch_type MACH_TYPE_EBSA285
> > > > # endif
> > > > # define machine_is_ebsa285() (machine_arch_type == MACH_TYPE_EBSA285)
> > > > #else
> > > > # define machine_is_ebsa285() (0)
> > > > #endif
> > > > 
> > > > #ifdef CONFIG_ARCH_NETWINDER
> > > > # ifdef machine_arch_type
> > > > # undef machine_arch_type
> > > > # define machine_arch_type __machine_arch_type
> > > > # else
> > > > # define machine_arch_type MACH_TYPE_NETWINDER
> > > > # endif
> > > > # define machine_is_netwinder() (machine_arch_type == MACH_TYPE_NETWINDER)
> > > > #else
> > > > # define machine_is_netwinder() (0)
> > > > #endif
> > > > 
> > > > #ifndef machine_arch_type
> > > > #define machine_arch_type __machine_arch_type
> > > > #endif
> > > > 
> > > > unsigned int __machine_arch_type = MACH_TYPE_EBSA285; //From bootloader
> > > > 
> > > > int main(int argc, char** argv)
> > > > {
> > > > 	if (machine_is_ebsa285())
> > > > 		printf("machine_is_ebsa285()\n");
> > > > 	if (machine_is_netwinder())
> > > > 		printf("machine_is_netwinder()\n");
> > > > 	printf("  machine_arch_type=%u\n", machine_arch_type);
> > > > 	printf("__machine_arch_type=%u\n", __machine_arch_type);
> > > > 
> > > > 	return 0;
> > > > }
> > > > >>>
> > > > 
> > > > shc at shc /home/git/test $ ./test 
> > > > machine_is_netwinder()
> > > >   machine_arch_type=5
> > > > __machine_arch_type=4
> > > 
> > > That is working as designed, nothing wrong there.  You have EBSA285
> > > support disabled (you commented out CONFIG_ARCH_EBSA285) and you have
> > > Netwinder support enabled (CONFIG_ARCH_NETWINDER).  So,
> > > machine_is_ebsa285() is constant false and machine_is_netwinder() is
> > > constant true.
> > > 
> > > However, such a kernel won't boot when passed ID 5 because it won't
> > > find the platform record in the kernel, and we explicitly error out
> > > in that case.
> >
> > Why not? As I say before that I have a two valid machines with ID 4 and 5.
> > I can boot machine ID 5 by specifying this ID in the bootloader.
> 
> If you have two machines with ID 4 and ID 5, and you configure out of the
> kernel support for ID 4, then pass ID 4 to the kernel, the kernel should
> error out.
> 
> > In any case, it's my fault that I do not specify the correct symbol,
> > associated with the ID.
> 
> That is why we have the machine database, to keep things straight.
> 
> > But even such issues can be avoided if we make
> > a comparison with the ID passed from the bootloader and not redefine it, ie:
> > #ifdef CONFIG_ARCH_NETWINDER
> > # define machine_is_netwinder() (__machine_arch_type == MACH_TYPE_NETWINDER)
> > #else
> > # define machine_is_netwinder() (0)
> > #endif
> 
> No, you haven't listened to what I've said.  Go back and read my
> explanation about how this works, particularly the part where these
> macros end up being constant false and constant true.
> 
> > In this case, if we pass the ID 4 to bootloader, we will never have a macro
> > machine_is_xx() (that's right), but boot the correct machine.
> 
> Again, you're not listening to what I've said.
> 
> > Look at my solution for "gen_mach_types" in the first message of this thread.
> > Fixme, but on my opinion it will not affect the optimization.
> 
> You're wrong.  You don't understand what's going on here and why it is
> the way that it is.  There is nothing wrong here, at all.  It is all
> working as designed.  Your changes make it work sub-optimally by getting
> rid of the constant-true optimization.

I understand that optimization occurs only if one machine is defined. With
two or more machines, the constant is replaced by the variable and processing
is done at runtime.
For my case, when only one character is specified, the processing at runtime
does not work...

Well, I understand your position on optimization. Can we at least replace
definition machine_arch_type by variable __machine_arch_type, which is passed
to the function setup_machine_tags() in setup_arch() procedure? Or have I
misunderstood something again?

-- 
Alexander Shiyan <shc_work@mail.ru>

Question: Multiple board support is broken

[Russell King - ARM Linux]

On Sat, Dec 08, 2012 at 03:18:59PM +0400, Alexander Shiyan wrote:
> Well, I understand your position on optimization. Can we at least replace
> definition machine_arch_type by variable __machine_arch_type, which is passed
> to the function setup_machine_tags() in setup_arch() procedure? Or have I
> misunderstood something again?

Err, no I don't think you have.  Looks like the initial erroring out got
broken by the addition of DT support and all the changes that happened
after that.

Given that the initial assembly code which errored out on non-supported
platforms seems to have been deleted, that does need to be changed.
Please send a patch just for that.

Question: Multiple board support is broken

[Alexander Shiyan]

On Sat, 8 Dec 2012 11:23:53 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Sat, Dec 08, 2012 at 03:18:59PM +0400, Alexander Shiyan wrote:
> > Well, I understand your position on optimization. Can we at least replace
> > definition machine_arch_type by variable __machine_arch_type, which is passed
> > to the function setup_machine_tags() in setup_arch() procedure? Or have I
> > misunderstood something again?
> 
> Err, no I don't think you have.  Looks like the initial erroring out got
> broken by the addition of DT support and all the changes that happened
> after that.
> 
> Given that the initial assembly code which errored out on non-supported
> platforms seems to have been deleted, that does need to be changed.
> Please send a patch just for that.

Do it now, please check the following patch.

-- 
Alexander Shiyan <shc_work@mail.ru>

[PATCH] ARM: Do not use definition "machine_arch_type" outside mach-types.h

[Alexander Shiyan]

This patch removes the use of the definition "machine_arch_type"
outside mach-types.h file. Using variable "__machine_arch_type"
that passed from the bootloader.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/kernel/machine_kexec.c |    2 +-
 arch/arm/kernel/setup.c         |    2 +-
 arch/arm/mach-pxa/em-x270.c     |    2 +-
 arch/arm/tools/gen-mach-types   |    3 ---
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 8ef8c93..f154d6b 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -145,7 +145,7 @@ void machine_kexec(struct kimage *image)
 	/* Prepare parameters for reboot_code_buffer*/
 	kexec_start_address = image->start;
 	kexec_indirection_page = page_list;
-	kexec_mach_type = machine_arch_type;
+	kexec_mach_type = __machine_arch_type;
 	if (!kexec_boot_atags)
 		kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
 
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 86d1429..95fcaa0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -744,7 +744,7 @@ void __init setup_arch(char **cmdline_p)
 	setup_processor();
 	mdesc = setup_machine_fdt(__atags_pointer);
 	if (!mdesc)
-		mdesc = setup_machine_tags(__atags_pointer, machine_arch_type);
+		mdesc = setup_machine_tags(__atags_pointer, __machine_arch_type);
 	machine_desc = mdesc;
 	machine_name = mdesc->name;
 
diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
index 1b64114..601ffc21 100644
--- a/arch/arm/mach-pxa/em-x270.c
+++ b/arch/arm/mach-pxa/em-x270.c
@@ -1273,7 +1273,7 @@ static void __init em_x270_init(void)
 	else if (machine_is_exeda())
 		em_x270_exeda_init();
 	else
-		panic("Unsupported machine: %d\n", machine_arch_type);
+		panic("Unsupported machine: %d\n", __machine_arch_type);
 
 	em_x270_init_da9030();
 	em_x270_init_dm9000();
diff --git a/arch/arm/tools/gen-mach-types b/arch/arm/tools/gen-mach-types
index 04fef71..4a85086 100644
--- a/arch/arm/tools/gen-mach-types
+++ b/arch/arm/tools/gen-mach-types
@@ -65,8 +65,5 @@ END	{
 	      printf("#define %s()\t(0)\n", machine_is[i]);
 	    }
 
-	  printf("\n#ifndef machine_arch_type\n");
-	  printf("#define machine_arch_type\t__machine_arch_type\n");
-	  printf("#endif\n\n");
 	  printf("#endif\n");
 	}
-- 
1.7.8.6

[PATCH] ARM: Do not use definition "machine_arch_type" outside mach-types.h

[Russell King - ARM Linux]

On Sat, Dec 08, 2012 at 04:02:05PM +0400, Alexander Shiyan wrote:
> This patch removes the use of the definition "machine_arch_type"
> outside mach-types.h file. Using variable "__machine_arch_type"
> that passed from the bootloader.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  arch/arm/kernel/machine_kexec.c |    2 +-
>  arch/arm/kernel/setup.c         |    2 +-
>  arch/arm/mach-pxa/em-x270.c     |    2 +-
>  arch/arm/tools/gen-mach-types   |    3 ---
>  4 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index 8ef8c93..f154d6b 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -145,7 +145,7 @@ void machine_kexec(struct kimage *image)
>  	/* Prepare parameters for reboot_code_buffer*/
>  	kexec_start_address = image->start;
>  	kexec_indirection_page = page_list;
> -	kexec_mach_type = machine_arch_type;
> +	kexec_mach_type = __machine_arch_type;

Pointless.

>  	if (!kexec_boot_atags)
>  		kexec_boot_atags = image->start - KEXEC_ARM_ZIMAGE_OFFSET + KEXEC_ARM_ATAGS_OFFSET;
>  
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 86d1429..95fcaa0 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -744,7 +744,7 @@ void __init setup_arch(char **cmdline_p)
>  	setup_processor();
>  	mdesc = setup_machine_fdt(__atags_pointer);
>  	if (!mdesc)
> -		mdesc = setup_machine_tags(__atags_pointer, machine_arch_type);
> +		mdesc = setup_machine_tags(__atags_pointer, __machine_arch_type);

This is fine.

>  	machine_desc = mdesc;
>  	machine_name = mdesc->name;
>  
> diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
> index 1b64114..601ffc21 100644
> --- a/arch/arm/mach-pxa/em-x270.c
> +++ b/arch/arm/mach-pxa/em-x270.c
> @@ -1273,7 +1273,7 @@ static void __init em_x270_init(void)
>  	else if (machine_is_exeda())
>  		em_x270_exeda_init();
>  	else
> -		panic("Unsupported machine: %d\n", machine_arch_type);
> +		panic("Unsupported machine: %d\n", __machine_arch_type);
>  
>  	em_x270_init_da9030();
>  	em_x270_init_dm9000();

Pointless.

> diff --git a/arch/arm/tools/gen-mach-types b/arch/arm/tools/gen-mach-types
> index 04fef71..4a85086 100644
> --- a/arch/arm/tools/gen-mach-types
> +++ b/arch/arm/tools/gen-mach-types
> @@ -65,8 +65,5 @@ END	{
>  	      printf("#define %s()\t(0)\n", machine_is[i]);
>  	    }
>  
> -	  printf("\n#ifndef machine_arch_type\n");
> -	  printf("#define machine_arch_type\t__machine_arch_type\n");
> -	  printf("#endif\n\n");

This is still wrong.

[PATCH] ARM: Pass real "__machine_arch_type" variable to setup_machine_tags() procedure

[Alexander Shiyan]

This modification is needed to proper boot the custom machines with
the IDs that are not described in the mach-types.h table.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/kernel/setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 86d1429..95fcaa0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -744,7 +744,7 @@ void __init setup_arch(char **cmdline_p)
 	setup_processor();
 	mdesc = setup_machine_fdt(__atags_pointer);
 	if (!mdesc)
-		mdesc = setup_machine_tags(__atags_pointer, machine_arch_type);
+		mdesc = setup_machine_tags(__atags_pointer, __machine_arch_type);
 	machine_desc = mdesc;
 	machine_name = mdesc->name;
 
-- 
1.7.8.6