Re: [flashrom] [PATCH]Improve libpayload support, provide libflashrom.a target

[Carl-Daniel Hailfinger]

Auf 25.02.2011 14:47, Georgi, Patrick schrieb:
> attached patch contains a couple of build fixes for libpayload builds,
> adds a "libpayload" OS_ARCH (which disables serial and usb programmers
> and sets up the build to use lpgcc), and provides a new make target
> "libflashrom.a" which combines all object files except the CLI_OBJS
> class into libflashrom.a.
>
> For a successful build you also need the libpayload patches on the
> coreboot mailing list until they're checked in.
>
> libflashrom.a still suffers from a variety of exit(1)s here and there,
> so it's not quite a librarified flashrom yet, but it's a start.

Yes indeed, and I'm working on removing those exit() wherever possible.
> Signed-off-by: Patrick Georgi

Thanks for your patch! Review follows.
> Index: Makefile
>
> --- Makefile	(Revision 1262)
> +++ Makefile	(Arbeitskopie)
> @@ -28,6 +28,8 @@
>  MANDIR  ?= $(PREFIX)/share/man
>  CFLAGS  ?= -Os -Wall -Wshadow
>  EXPORTDIR ?= .
> +AR      ?= ar
> +RANLIB  ?= ranlib

Those two changes above probably need special handling for the
cross-compilation case for DOS.
> WARNERROR ?= yes
>
> @@ -82,6 +84,39 @@
>  endif
>  endif
>
> +ifeq ($(OS_ARCH), libpayload)
> +CC:=CC=i386-elf-gcc lpgcc
> +AR:=i386-elf-ar
> +RANLIB:=i386-elf-ranlib
> +CPPFLAGS += -DSTANDALONE
> +ifeq ($(CONFIG_DUMMY), yes)
> +UNSUPPORTED_FEATURES += CONFIG_DUMMY=yes
> +else
> +override CONFIG_DUMMY = no
> +endif
> +ifeq ($(CONFIG_BUSPIRATE_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_BUSPIRATE_SPI=yes
> +else
> +override CONFIG_BUSPIRATE_SPI = no
> +endif
> +ifeq ($(CONFIG_SERPROG), yes)
> +UNSUPPORTED_FEATURES += CONFIG_SERPROG=yes
> +else
> +override CONFIG_SERPROG = no
> +endif
> +# Dediprog and FT2232 are not supported under DOS (missing USB support).

Please remove or change this comment.
> +ifeq ($(CONFIG_DEDIPROG), yes)
> +UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
> +else
> +override CONFIG_DEDIPROG = no
> +endif
> +ifeq ($(CONFIG_FT2232_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_FT2232_SPI=yes
> +else
> +override CONFIG_FT2232_SPI = no
> +endif
> +endif
>
> CHIP_OBJS = jedec.o stm50flw0x0x.o w39.o w29ee011.o \
>    sst28sf040.o m29f400bt.o 82802ab.o pm49fl00x.o \
>    sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o sharplhf00l04.o
> @@ -335,11 +370,16 @@
>  # We could use PULLED_IN_LIBS, but that would be ugly.
>  FEATURE_LIBS += $(shell LC_ALL=C grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz")
>
> -OBJS = $(CHIP_OBJS) $(CLI_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
> +LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
> +OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS)
>
> $(PROGRAM)$(EXEC_SUFFIX): $(OBJS)
>    $(CC) $(LDFLAGS) -o $(PROGRAM)$(EXEC_SUFFIX) $(OBJS) $(FEATURE_LIBS) $(LIBS)
>
> +libflashrom.a: $(LIBFLASHROM_OBJS)
> $(AR) rcs $@ $^
> $(RANLIB) $@

Please check if this breaks native Windows build or cross-compiled DOS
build.
> # TAROPTIONS reduces information leakage from the packager's system.
>  # If other tar programs support command line arguments for setting uid/gid of
>  # stored files, they can be handled here as well.
> @@ -352,7 +392,7 @@
>  # This includes all frontends and libflashrom.
>  # We don't use EXEC_SUFFIX here because we want to clean everything.
>  clean:
> rm -f $(PROGRAM) $(PROGRAM).exe *.o *.d
> rm -f $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d

Does this clean libraries for DOS (cross-compiled under Linux) and
natively under Windows as well, or are the filename extensions different?
> distclean: clean
>    rm -f .features .libdeps

Other than that, it looks good. If you fix the comment and can organize
a test compile for Windows and DOS (Idwer can help with DOS), this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel