[PATCH] Autodetect target processor architecture

[Carl-Daniel Hailfinger]

Autodetect target processor architecture.
Enable architecture dependent compilation of individual sub-drivers for
the internal programmer.

With this patch, you no longer have to edit the Makefile to compile the
internal driver on MIPS/ARM/...

TODO: arch.h is not suitable for inclusion in a .c/.h file because of
its last line. Any ideas how to change that (move arch.h as "here
document" into the Makefile, use other trickery like more #ifdefs)?

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: flashrom-target_arch/Makefile
===================================================================
--- flashrom-target_arch/Makefile	(Revision 1357)
+++ flashrom-target_arch/Makefile	(Arbeitskopie)
@@ -37,7 +37,10 @@
 CFLAGS += -Werror
 endif
 
-# FIXME We have to differentiate between host and target arch.
+# Determine the destination processor architecture
+ARCH = $(strip $(shell LC_ALL=C gcc -E arch.h|grep -v '^\#'))
+
+# FIXME We have to differentiate between host and target OS architecture.
 OS_ARCH	?= $(shell uname)
 ifneq ($(OS_ARCH), SunOS)
 STRIP_ARGS = -s
@@ -222,8 +225,10 @@
 ifeq ($(CONFIG_INTERNAL), yes)
 FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
 PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
-# FIXME: The PROGRAMMER_OBJS below should only be included on x86.
+ifeq ($(ARCH),"x86")
 PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
+else
+endif
 NEED_PCI := yes
 endif
 
@@ -418,6 +423,9 @@
 		echo "found." || ( echo "not found."; \
 		rm -f .test.c .test$(EXEC_SUFFIX); exit 1)
 	@rm -f .test.c .test$(EXEC_SUFFIX)
+	@printf "ARCH is "
+	@echo $(ARCH)|wc -l|grep -q ^1$ || ( echo "unknown. Aborting."; exit 1)
+	@printf "%s\n" '$(ARCH)'
 
 ifeq ($(CHECK_LIBPCI), yes)
 pciutils: compiler
Index: flashrom-target_arch/arch.h
===================================================================
--- flashrom-target_arch/arch.h	(Revision 0)
+++ flashrom-target_arch/arch.h	(Revision 0)
@@ -0,0 +1,31 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2011 Carl-Daniel Hailfinger
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+/*
+ * Header file for CPU architecture checking.
+ */
+
+#if defined (__i386__) || defined (__x86_64__)
+#define __FLASHROM_ARCH__ "x86"
+#elif defined (__mips) || defined (__mips__) || defined (_mips) || defined (mips)
+#define __FLASHROM_ARCH__ "mips"
+#elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__) || defined(__ppc64__)
+#define __FLASHROM_ARCH__ "ppc"
+#endif
+__FLASHROM_ARCH__


-- 
http://www.hailfinger.org/

[Carl-Daniel Hailfinger]

Autodetect target processor architecture.
Enable architecture dependent compilation of individual sub-drivers for
the internal programmer.

With this patch, you no longer have to edit the Makefile to compile the
internal driver on MIPS/ARM/...

David: This one should take care of the Makefile hacks you needed.


Am 29.06.2011 04:26 schrieb Carl-Daniel Hailfinger:
> TODO: arch.h is not suitable for inclusion in a .c/.h file because of
> its last line. Any ideas how to change that (move arch.h as "here
> document" into the Makefile, use other trickery like more #ifdefs)?

Various solutions have been suggested, and the consensus seemed to be
that we need to make a generic decision about where to store all those
compilation tests in the Makefile, but that it should not happen in this
patch.

Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>

Index: flashrom-target_arch/Makefile
===================================================================
--- flashrom-target_arch/Makefile	(Revision 1357)
+++ flashrom-target_arch/Makefile	(Arbeitskopie)
@@ -37,7 +37,10 @@
 CFLAGS += -Werror
 endif
 
-# FIXME We have to differentiate between host and target arch.
+# Determine the destination processor architecture
+ARCH = $(strip $(shell LC_ALL=C gcc -E arch.h|grep -v '^\#'))
+
+# FIXME We have to differentiate between host and target OS architecture.
 OS_ARCH	?= $(shell uname)
 ifneq ($(OS_ARCH), SunOS)
 STRIP_ARGS = -s
@@ -222,8 +225,10 @@
 ifeq ($(CONFIG_INTERNAL), yes)
 FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
 PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o dmi.o internal.o
-# FIXME: The PROGRAMMER_OBJS below should only be included on x86.
+ifeq ($(ARCH),"x86")
 PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o mcp6x_spi.o
+else
+endif
 NEED_PCI := yes
 endif
 
@@ -418,6 +423,11 @@
 		echo "found." || ( echo "not found."; \
 		rm -f .test.c .test$(EXEC_SUFFIX); exit 1)
 	@rm -f .test.c .test$(EXEC_SUFFIX)
+	@printf "ARCH is "
+	@# FreeBSD wc will output extraneous whitespace.
+	@echo $(ARCH)|wc -l|grep -q '^[[:blank:]]*1[[:blank:]]*$$' ||	\
+		( echo "unknown. Aborting."; exit 1)
+	@printf "%s\n" '$(ARCH)'
 
 ifeq ($(CHECK_LIBPCI), yes)
 pciutils: compiler
Index: flashrom-target_arch/arch.h
===================================================================
--- flashrom-target_arch/arch.h	(Revision 0)
+++ flashrom-target_arch/arch.h	(Revision 0)
@@ -0,0 +1,31 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2011 Carl-Daniel Hailfinger
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+/*
+ * Header file for CPU architecture checking.
+ */
+
+#if defined (__i386__) || defined (__x86_64__)
+#define __FLASHROM_ARCH__ "x86"
+#elif defined (__mips) || defined (__mips__) || defined (_mips) || defined (mips)
+#define __FLASHROM_ARCH__ "mips"
+#elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__) || defined(__ppc64__)
+#define __FLASHROM_ARCH__ "ppc"
+#endif
+__FLASHROM_ARCH__


-- 
http://www.hailfinger.org/

[Peter Lemenkov]

2011/6/29 Carl-Daniel Hailfinger <[e-mail]>:
> Autodetect target processor architecture.
> Enable architecture dependent compilation of individual sub-drivers for
> the internal programmer.

Why not to start adopting autotools as a replacement for ahnd-written
makefiles at this point? They already have (proven) macros for
endianness detection, cpu and architecture and many other good and
useful stuff.
-- 
With best regards, Peter Lemenkov.

[Joerg Mayer]

On Thu, Jun 30, 2011 at 11:58:45AM +0400, Peter Lemenkov wrote:
> 2011/6/29 Carl-Daniel Hailfinger <[e-mail]>:
> > Autodetect target processor architecture.
> > Enable architecture dependent compilation of individual sub-drivers for
> > the internal programmer.
>
> Why not to start adopting autotools as a replacement for ahnd-written
> makefiles at this point? They already have (proven) macros for
> endianness detection, cpu and architecture and many other good and
> useful stuff.

If so, I'd rather see cmake. I what I've read is correct, autotools on Win isn't
a very nice thing. Also, with cmake you only have a single package to install,
with autofoo it's a bit more.
As offered on this list before: I'd be willing to create a cmake config if
there is genuine interest (i.e. I'd like to have a high probability of getting
stuff accepted in the end).

Ciao
   Jörg

-- 
Joerg Mayer                                           <[e-mail]>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.

[Carl-Daniel Hailfinger]

Am 30.06.2011 09:58 schrieb Peter Lemenkov:
> 2011/6/29 Carl-Daniel Hailfinger <[e-mail]>:
> > Autodetect target processor architecture.
> > Enable architecture dependent compilation of individual sub-drivers for
> > the internal programmer.
>
> Why not to start adopting autotools as a replacement for hand-written
> makefiles at this point? They already have (proven) macros for
> endianness detection, cpu and architecture and many other good and
> useful stuff.

Using autoconf would imply at least a fourfold increase in compile time,
and it will break anyway (a few years ago I worked for SUSE, and most of
the packaging effort for my packages went into fixing breakage caused by
autoconf). AFAIK KDE switched from autotoos to CMake with KDE4.

To even consider autotools at all, two basic requirements must be met:
- Zero compile time increase (that includes the configure step in a
pristine tree).
- Total lines of code to be maintained do not increase either.

That said, maybe there is a less buggy autotools alternative which
fulfills the above two requirements. I'd be interested to hear about
such a solution. CMake has been mentioned in the past, but IIRC it would
have resulted in more lines of code to be maintained as well.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[Peter Lemenkov]

2011/6/30 Carl-Daniel Hailfinger <[e-mail]>:
> Using autoconf would imply at least a fourfold increase in compile time,

Does anybody cares? Ok, anyway, we don't need to guess - let's count:

work ~/work/flashrom (git::fedora-next): time autoreconf -if

real    0m3.911s
user    0m2.231s
sys     0m0.244s
work ~/work/flashrom (git::fedora-next):

This would be run very occasionally, so I wouldn't bother about
autoreconf invocation - just for the record. Running ./configure is
more frequent operation but it also requires only when we change
something in configure.ac and Makefile.am:

work ~/work/flashrom (git::fedora-next): time ./configure 2>&1 > /dev/null

real    0m5.572s
user    0m2.202s
sys     0m1.674s
work ~/work/flashrom (git::fedora-next):

And now, the most frequent (but not the last) operation:

work ~/work/flashrom (git::fedora-next): time make 2>&1 > /dev/null

real    0m4.922s
user    0m4.072s
sys     0m0.927s
work ~/work/flashrom (git::fedora-next):

Finally, let's install the resulting binary:

work ~/work/flashrom (git::fedora-next): time make install
DESTDIR=/var/tmp/flashrom-autotools 2>&1 > /dev/null

real    0m0.160s
user    0m0.091s
sys     0m0.054s
work ~/work/flashrom (git::fedora-next):

Now, let's compare with the current buildsystem. Firstly, we will run
"configure + make" target:

work ~/work/flashrom (git::master): time make 2>&1 > /dev/null

real    0m5.385s
user    0m4.282s
sys     0m1.157s
work ~/work/flashrom (git::master):

I'd say it two times faster (assuming that we'll run ./configure every
time we using autotools - and we won't). And lets try to install:

work ~/work/flashrom (git::master): time make install
DESTDIR=/var/tmp/flashrom-custem 2>&1 > /dev/null

real    0m0.494s
user    0m0.295s
sys     0m0.188s
work ~/work/flashrom (git::master):

Still (in)significantly fast but anyway - three times slower :).
> To even consider autotools at all, two basic requirements must be met:
> - Zero compile time increase (that includes the configure step in a
> pristine tree).
> - Total lines of code to be maintained do not increase either.

Yes. LOC number is significatly lower in case of autotools and
handmade Makefile:

sulaco ~/work/flashrom (git::fedora-next): LANG=C wc -l configure.ac
Makefile.am && wc -l Makefile
  241 configure.ac
  186 Makefile.am
  427 total
523 Makefile
sulaco ~/work/flashrom (git::fedora-next):

And there are some cleanups in the sources as well which I didn't
count. So I can answer with confidence - the number of LOCs will be
less than now and the compile time will be almost the same (difference
in 5 seconds).

The CMake-based buildsystem however has one major drawback - it
requires cmake to be installed (aototools requires themselves only for
reconfiguration). And keeping in mind that different major CMake
versions have major incompatibilities I'd rather say that it's better
not to use it at all - it adds more issues than fixes them. I can say
it for sure because I've got very interesting experience of
maintaining compatibility of CMake-based buildsystem for relatively
large project among three major CMake branches - 2.4, 2.6 and 2.8.
-- 
With best regards, Peter Lemenkov.

[Carl-Daniel Hailfinger]

Am 30.06.2011 21:03 schrieb Peter Lemenkov:
> 2011/6/30 Carl-Daniel Hailfinger <[e-mail]>:
> > Using autoconf would imply at least a fourfold increase in compile time,
>
> Does anybody cares? Ok, anyway, we don't need to guess - let's count:
>
> work ~/work/flashrom (git::fedora-next): time autoreconf -if
>
> real    0m3.911s
> user    0m2.231s
> sys     0m0.244s
> work ~/work/flashrom (git::fedora-next):
>
> This would be run very occasionally, so I wouldn't bother about
> autoreconf invocation - just for the record. Running ./configure is
> more frequent operation but it also requires only when we change
> something in configure.ac and Makefile.am:

As a maintainer, I compile pretty much every patch I submit or review in
at least two different configurations. That differs from the usual user
experience, though.
> work ~/work/flashrom (git::fedora-next): time ./configure 2>&1 > /dev/null
>
> real    0m5.572s
> user    0m2.202s
> sys     0m1.674s
> work ~/work/flashrom (git::fedora-next):
>
> And now, the most frequent (but not the last) operation:
>
> work ~/work/flashrom (git::fedora-next): time make 2>&1 > /dev/null
>
> real    0m4.922s
> user    0m4.072s
> sys     0m0.927s
> work ~/work/flashrom (git::fedora-next):

Wow, that machine is really fast! flashrom compilation takes roughly 30
seconds on my machine.
> Finally, let's install the resulting binary:
>
> work ~/work/flashrom (git::fedora-next): time make install
> DESTDIR=/var/tmp/flashrom-autotools 2>&1 > /dev/null
>
> real    0m0.160s
> user    0m0.091s
> sys     0m0.054s
> work ~/work/flashrom (git::fedora-next):
>
> Now, let's compare with the current buildsystem. Firstly, we will run
> "configure + make" target:
>
> work ~/work/flashrom (git::master): time make 2>&1 > /dev/null
>
> real    0m5.385s
> user    0m4.282s
> sys     0m1.157s
> work ~/work/flashrom (git::master):
>
> I'd say it two times faster (assuming that we'll run ./configure every
> time we using autotools - and we won't). And lets try to install:
>
> work ~/work/flashrom (git::master): time make install
> DESTDIR=/var/tmp/flashrom-custem 2>&1 > /dev/null
>
> real    0m0.494s
> user    0m0.295s
> sys     0m0.188s
> work ~/work/flashrom (git::master):
>
> Still (in)significantly fast but anyway - three times slower :).
> > To even consider autotools at all, two basic requirements must be met:
> > - Zero compile time increase (that includes the configure step in a
> > pristine tree).
> > - Total lines of code to be maintained do not increase either.
>
> Yes. LOC number is significatly lower in case of autotools and
> handmade Makefile:
>
> sulaco ~/work/flashrom (git::fedora-next): LANG=C wc -l configure.ac
> Makefile.am && wc -l Makefile
>   241 configure.ac
>   186 Makefile.am
>   427 total
> 523 Makefile
> sulaco ~/work/flashrom (git::fedora-next):

Now that is interesting. Do you have a git tree or a patch set somewhere
so I can take a look?
> And there are some cleanups in the sources as well which I didn't
> count.

I am interested in those as well.
> So I can answer with confidence - the number of LOCs will be
> less than now and the compile time will be almost the same (difference
> in 5 seconds).
>
> The CMake-based buildsystem however has one major drawback - it
> requires cmake to be installed (aototools requires themselves only for
> reconfiguration). And keeping in mind that different major CMake
> versions have major incompatibilities I'd rather say that it's better
> not to use it at all - it adds more issues than fixes them. I can say
> it for sure because I've got very interesting experience of
> maintaining compatibility of CMake-based buildsystem for relatively
> large project among three major CMake branches - 2.4, 2.6 and 2.8.

Hm. My past autotools experience was really abysmal regarding
compatibility between autotools versions, so I doubt CMake can be worse.
The idea behind autotools is good, but the implementation was garbage 7
years ago despite claims to the contrary. Having been burned back then,
I'm very reluctant to consider autotools for anything.

Nowadays most of the changes in the Makefile are either additions for
new programmers or fixups for new gcc/clang versions. Autotools won't
affect the former, but I have no idea if it could prevent the latter. It
would be nice, though.

That said, your makefile/configure file sizes look impressive and I'd
really like to check how they handle all the conditional feature
enabling we have now. Maybe autotools are really more mature now and I
should throw my doubts away.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[Andrew Morgan]

On 29/06/11 03:26, Carl-Daniel Hailfinger wrote:
> Autodetect target processor architecture.
> Enable architecture dependent compilation of individual sub-drivers for
> the internal programmer.
>
> With this patch, you no longer have to edit the Makefile to compile the
> internal driver on MIPS/ARM/...
>
> TODO: arch.h is not suitable for inclusion in a .c/.h file because of
> its last line. Any ideas how to change that (move arch.h as "here
> document" into the Makefile, use other trickery like more #ifdefs)?
> Patch tested on PowerPC Linux. Flashrom still needs to be compiled with 'CONFIG_RAYER_SPI=0 CONFIG_NIC3COM=0 CONFIG_NICREALTEK=0 CONFIG_SATAMV=0 make' but then it works. I assume those variables can be automatically set based on the value of ARCH at a later date.

Checking for a C compiler... found.
ARCH is "ppc"
...

$ uname -a

Linux mini 2.6.38-8-powerpc #42-Ubuntu Mon Apr 11 03:41:36 UTC 2011 ppc ppc ppc GNU/Linux


ps. Your post mentions ARM but arch.h doesn't.

-- 

Andrew.

[Carl-Daniel Hailfinger]

Am 30.06.2011 23:26 schrieb Andrew Morgan:
> On 29/06/11 03:26, Carl-Daniel Hailfinger wrote:
> > Autodetect target processor architecture.
> > Enable architecture dependent compilation of individual sub-drivers for
> > the internal programmer.
> >
> > With this patch, you no longer have to edit the Makefile to compile the
> > internal driver on MIPS/ARM/...
> >
> > TODO: arch.h is not suitable for inclusion in a .c/.h file because of
> > its last line. Any ideas how to change that (move arch.h as "here
> > document" into the Makefile, use other trickery like more #ifdefs)?
> > Patch tested on PowerPC Linux.

Thanks!
> Flashrom still needs to be compiled with 'CONFIG_RAYER_SPI=0
> CONFIG_NIC3COM=0 CONFIG_NICREALTEK=0 CONFIG_SATAMV=0 make' but then it
> works. I assume those variables can be automatically set based on the
> value of ARCH at a later date.

Right, that would make sense.
> Checking for a C compiler... found.
> ARCH is "ppc"
> ...
>
> $ uname -a
>
> Linux mini 2.6.38-8-powerpc #42-Ubuntu Mon Apr 11 03:41:36 UTC 2011
> ppc ppc ppc GNU/Linux
>
> ps. Your post mentions ARM but arch.h doesn't.

Yes, David had the problem of incompatible internal drivers on ARM, and
if this patch gets merged, he can simply extend it with ARM definitions
and have his problems solved.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[David Hendricks]

On Tue, Jun 28, 2011 at 7:26 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Autodetect target processor architecture.
> Enable architecture dependent compilation of individual sub-drivers for
> the internal programmer.
>
> With this patch, you no longer have to edit the Makefile to compile the
> internal driver on MIPS/ARM/...
>
> TODO: arch.h is not suitable for inclusion in a .c/.h file because of
> its last line. Any ideas how to change that (move arch.h as "here
> document" into the Makefile, use other trickery like more #ifdefs)?
>
> Signed-off-by: Carl-Daniel Hailfinger <[e-mail]>
>
> Index: flashrom-target_arch/Makefile
> ===================================================================
> --- flashrom-target_arch/Makefile       (Revision 1357)
> +++ flashrom-target_arch/Makefile       (Arbeitskopie)
> @@ -37,7 +37,10 @@
>  CFLAGS += -Werror
>  endif
>
> -# FIXME We have to differentiate between host and target arch.
> +# Determine the destination processor architecture
> +ARCH = $(strip $(shell LC_ALL=C gcc -E arch.h|grep -v '^\#'))

Use $(CC) instead of "gcc" so stuff like "armv7a-cros-linux-gnueabi-gcc"
gets handled properly. I still think assigning ARCH conditionally is wise
since package managers should set it when cross-compiling, though
using $(CC) should yield the same result.

Aside from that, I was able to cross-compile successfully for ARM (using the
same CONFIG_* settings as before) after adding this minor patch on top of
yours:

diff -Nru a/Makefile b/Makefile
--- a/Makefile  2011-06-30 19:35:53.410049173 -0700
+++ b/Makefile  2011-06-30 19:35:39.140049283 -0700
@@ -38,7 +38,7 @@
 endif

 # Determine the destination processor architecture
-ARCH = $(strip $(shell LC_ALL=C gcc -E arch.h|grep -v '^\#'))
+ARCH = $(strip $(shell LC_ALL=C $(CC) -E arch.h|grep -v '^\#'))

 # FIXME We have to differentiate between host and target OS architecture.
 OS_ARCH        ?= $(shell uname)
@@ -228,6 +228,10 @@
 ifeq ($(ARCH),"x86")
 PROGRAMMER_OBJS += it87spi.o it85spi.o ichspi.o sb600spi.o wbsio_spi.o
mcp6x_spi.o
 else
+ifeq ($(ARCH),"arm")
+PROGRAMMER_OBJS += tegra2_spi.o
+else
+endif
 endif
 NEED_PCI := yes
 endif
diff -Nru a/arch.h b/arch.h
--- a/arch.h    2011-06-30 19:35:50.190944388 -0700
+++ b/arch.h    2011-06-30 19:35:35.280103884 -0700
@@ -27,5 +27,7 @@
 #define __FLASHROM_ARCH__ "mips"
 #elif defined(__powerpc__) || defined(__powerpc64__) || defined(__ppc__) ||
defined(__ppc64__)
 #define __FLASHROM_ARCH__ "ppc"
+#elif defined(__arm__)
+#define __FLASHROM_ARCH__ "arm"
 #endif
 __FLASHROM_ARCH__

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.