December 2021 - flashrom-gerrit

Change in flashrom[master]: Makefile: Make pkg-config mandatory to find libftdi1
by Nico Huber (Code Review) 22 Dec '21

22 Dec '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/58623 ) Change subject: Makefile: Make pkg-config mandatory to find libftdi1 ...................................................................... Makefile: Make pkg-config mandatory to find libftdi1 Use `make HAS_LIBFTDI1=yes/no` to override the pkg-config detection and `CONFIG_LIBFTDI1_CFLAGS` and `CONFIG_LIBFTDI1_LDFLAGS` to set cflags and ldflags manually. Change-Id: I41f5186d9f3e063c12c8c6eea888d0b0bf534259 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/58623 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M Makefile A Makefile.d/ft232h_test.c M Makefile.include 3 files changed, 57 insertions(+), 59 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/Makefile b/Makefile index b196d98..1db75aa 100644 --- a/Makefile +++ b/Makefile @@ -14,8 +14,6 @@ # GNU General Public License for more details. # -include Makefile.include - PROGRAM = flashrom ############################################################################### @@ -55,12 +53,17 @@ # make CONFIG_DEFAULT_PROGRAMMER_NAME=serprog CONFIG_DEFAULT_PROGRAMMER_ARGS="dev=/dev/ttyUSB0:1500000" # would make executing './flashrom' (almost) equivialent to './flashrom -p serprog:dev=/dev/ttyUSB0:1500000'. +# The user can provide CPP, C and LDFLAGS and the Makefile will extend these +override CPPFLAGS := $(CPPFLAGS) +override CFLAGS := $(CFLAGS) +override LDFLAGS := $(LDFLAGS) + # If your compiler spits out excessive warnings, run make WARNERROR=no # You shouldn't have to change this flag. WARNERROR ?= yes ifeq ($(WARNERROR), yes) -CFLAGS += -Werror +override CFLAGS += -Werror endif ifdef LIBS_BASE @@ -87,6 +90,8 @@ debug_shell = $(shell export LC_ALL=C ; { echo 'exec: export LC_ALL=C ; { $(subst ','\'',$(1)) ; }' >&2; \ { $(1) ; } | tee -a $(BUILD_DETAILS_FILE) ; echo >&2 ; } 2>>$(BUILD_DETAILS_FILE)) +include Makefile.include + ############################################################################### # Dependency handling. @@ -129,7 +134,7 @@ CONFIG_RAIDEN_DEBUG_SPI \ CONFIG_STLINKV3_SPI \ -DEPENDS_ON_LIBFTDI := \ +DEPENDS_ON_LIBFTDI1 := \ CONFIG_FT2232_SPI \ CONFIG_USBBLASTER_SPI \ @@ -160,6 +165,10 @@ CC_WORKING := $(call c_compile_test, Makefile.d/cc_test.c) +# Configs for dependencies. Can be overwritten by commandline +CONFIG_LIBFTDI1_CFLAGS := $(call dependency_cflags, libftdi1) +CONFIG_LIBFTDI1_LDFLAGS := $(call dependency_ldflags, libftdi1) + # Determine the destination OS, architecture and endian # IMPORTANT: The following lines must be placed before TARGET_OS, ARCH or ENDIAN # is ever used (of course), but should come after any lines setting CC because @@ -168,6 +177,10 @@ override ARCH := $(call c_macro_test, Makefile.d/arch_test.h) override ENDIAN := $(call c_macro_test, Makefile.d/endian_test.h) + +HAS_LIBFTDI1 := $(call find_dependency, libftdi1) + +HAS_FT232H := $(call c_compile_test, Makefile.d/ft232h_test.c, $(CONFIG_LIBFTDI1_CFLAGS)) HAS_UTSNAME := $(call c_compile_test, Makefile.d/utsname_test.c) HAS_CLOCK_GETTIME := $(call c_compile_test, Makefile.d/clock_gettime_test.c) HAS_LINUX_MTD := $(call c_compile_test, Makefile.d/linux_mtd_test.c) @@ -197,7 +210,7 @@ # Missing serial support. $(call mark_unsupported,$(DEPENDS_ON_SERIAL)) # Libraries not available for DOS -$(call mark_unsupported,$(DEPENDS_ON_LIBUSB1) $(DEPENDS_ON_LIBFTDI) $(DEPENDS_ON_LIBJAYLINK)) +$(call mark_unsupported,$(DEPENDS_ON_LIBUSB1) $(DEPENDS_ON_LIBJAYLINK)) endif ifeq ($(TARGET_OS), $(filter $(TARGET_OS), MinGW Cygwin)) @@ -239,7 +252,7 @@ # Bus Pirate, Serprog and PonyProg are not supported with libpayload (missing serial support). $(call mark_unsupported,CONFIG_BUSPIRATE_SPI CONFIG_SERPROG CONFIG_PONY_SPI) # Dediprog, Developerbox, USB-Blaster, PICkit2, CH341A and FT2232 are not supported with libpayload (missing libusb support). -$(call mark_unsupported,$(DEPENDS_ON_LIBUSB1) $(DEPENDS_ON_LIBFTDI) $(DEPENDS_ON_LIBJAYLINK)) +$(call mark_unsupported,$(DEPENDS_ON_LIBUSB1) $(DEPENDS_ON_LIBJAYLINK)) endif ifeq ($(HAS_LINUX_MTD), no) @@ -264,6 +277,10 @@ $(call mark_unsupported,CONFIG_INTERNAL) endif +ifeq ($(HAS_LIBFTDI1), no) +$(call mark_unsupported,$(DEPENDS_ON_LIBFTDI1)) +endif + ifeq ($(ENDIAN), little) FEATURE_CFLAGS += -D'__FLASHROM_LITTLE_ENDIAN__=1' endif @@ -591,14 +608,12 @@ endif ifeq ($(CONFIG_FT2232_SPI), yes) -# This is a totally ugly hack. -FEATURE_CFLAGS += $(call debug_shell,grep -q "FTDISUPPORT := yes" .libdeps && printf "%s" "-D'CONFIG_FT2232_SPI=1'") +FEATURE_CFLAGS += -D'CONFIG_FT2232_SPI=1' PROGRAMMER_OBJS += ft2232_spi.o endif ifeq ($(CONFIG_USBBLASTER_SPI), yes) -# This is a totally ugly hack. -FEATURE_CFLAGS += $(call debug_shell,grep -q "FTDISUPPORT := yes" .libdeps && printf "%s" "-D'CONFIG_USBBLASTER_SPI=1'") +FEATURE_CFLAGS += -D'CONFIG_USBBLASTER_SPI=1' PROGRAMMER_OBJS += usbblaster_spi.o endif @@ -822,15 +837,13 @@ endif endif -NEED_LIBFTDI := $(call filter_deps,$(DEPENDS_ON_LIBFTDI)) -ifneq ($(NEED_LIBFTDI), ) -FTDILIBS := $(call debug_shell,[ -n "$(PKG_CONFIG_LIBDIR)" ] && export PKG_CONFIG_LIBDIR="$(PKG_CONFIG_LIBDIR)" ; $(PKG_CONFIG) --libs libftdi1 || $(PKG_CONFIG) --libs libftdi || printf "%s" "-lftdi -lusb") -FEATURE_CFLAGS += $(call debug_shell,grep -q "FT232H := yes" .libdeps && printf "%s" "-D'HAVE_FT232H=1'") -FTDI_INCLUDES := $(call debug_shell,[ -n "$(PKG_CONFIG_LIBDIR)" ] && export PKG_CONFIG_LIBDIR="$(PKG_CONFIG_LIBDIR)" ; $(PKG_CONFIG) --cflags-only-I libftdi1) -FEATURE_CFLAGS += $(FTDI_INCLUDES) -FEATURE_LIBS += $(call debug_shell,grep -q "FTDISUPPORT := yes" .libdeps && printf "%s" "$(FTDILIBS)") -# We can't set NEED_LIBUSB1 here because that would transform libftdi auto-enabling -# into a hard requirement for libusb, defeating the purpose of auto-enabling. +USE_LIBFTDI1 := $(if $(call filter_deps,$(DEPENDS_ON_LIBFTDI1)),yes,no) +ifeq ($(USE_LIBFTDI1), yes) +override CFLAGS += $(CONFIG_LIBFTDI1_CFLAGS) +override LDFLAGS += $(CONFIG_LIBFTDI1_LDFLAGS) +ifeq ($(HAS_FT232H), yes) +FEATURE_CFLAGS += -D'HAVE_FT232H=1' +endif endif NEED_LIBJAYLINK := $(call filter_deps,$(DEPENDS_ON_LIBJAYLINK)) @@ -866,7 +879,7 @@ endif $(PROGRAM)$(EXEC_SUFFIX): $(OBJS) - $(CC) $(LDFLAGS) -o $(PROGRAM)$(EXEC_SUFFIX) $(OBJS) $(LIBS) $(PCILIBS) $(FEATURE_LIBS) $(USBLIBS) $(USB1LIBS) $(JAYLINKLIBS) $(NI845X_LIBS) + $(CC) -o $(PROGRAM)$(EXEC_SUFFIX) $(OBJS) $(LDFLAGS) $(LIBS) $(PCILIBS) $(FEATURE_LIBS) $(USBLIBS) $(USB1LIBS) $(JAYLINKLIBS) $(NI845X_LIBS) libflashrom.a: $(LIBFLASHROM_OBJS) $(AR) rcs $@ $^ @@ -1004,23 +1017,12 @@ rm -f .test.c .test.o; exit 1; }; } 2>>$(BUILD_DETAILS_FILE); echo $? >&3 ; } | tee -a $(BUILD_DETAILS_FILE) >&4; } 3>&1;} | { read rc ; exit ${rc}; } } 4>&1 @rm -f .test.c .test.o .test$(EXEC_SUFFIX) endif -ifneq ($(NEED_LIBFTDI), ) - @printf "Checking for FTDI support... " | tee -a $(BUILD_DETAILS_FILE) - @echo "$$FTDI_TEST" > .test.c - @printf "\nexec: %s\n" "$(CC) $(CPPFLAGS) $(CFLAGS) $(FTDI_INCLUDES) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) $(FTDILIBS) $(LIBS)" >>$(BUILD_DETAILS_FILE) - @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(FTDI_INCLUDES) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) $(FTDILIBS) $(LIBS) >&2 && \ - ( echo "found."; echo "FTDISUPPORT := yes" >> .libdeps ; \ - printf "Checking for FT232H support in libftdi... " ; \ - echo "$$FTDI_232H_TEST" >> .test.c ; \ - printf "\nexec: %s\n" "$(CC) $(CPPFLAGS) $(CFLAGS) $(FTDI_INCLUDES) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) $(FTDILIBS) $(LIBS)" >>$(BUILD_DETAILS_FILE) ; \ - { $(CC) $(CPPFLAGS) $(CFLAGS) $(FTDI_INCLUDES) $(LDFLAGS) .test.c -o .test$(EXEC_SUFFIX) $(FTDILIBS) $(LIBS) >&2 && \ - ( echo "found."; echo "FT232H := yes" >> .libdeps ) || \ - ( echo "not found."; echo "FT232H := no" >> .libdeps ) } \ - ) || \ - ( echo "not found."; echo "FTDISUPPORT := no" >> .libdeps ) } \ - 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) - @rm -f .test.c .test.o .test$(EXEC_SUFFIX) -endif + @echo Dependency libftdi1 found: $(HAS_LIBFTDI1) + @if [ $(HAS_LIBFTDI1) = yes ]; then \ + echo " Checking for \"TYPE_232H\" in \"enum ftdi_chip_type\": $(HAS_FT232H)"; \ + echo " CFLAGS: $(CONFIG_LIBFTDI1_CFLAGS)"; \ + echo " LDFLAGS: $(CONFIG_LIBFTDI1_LDFLAGS)"; \ + fi # If a user does not explicitly request a non-working feature, we should # silently disable it. However, if a non-working (does not compile) feature diff --git a/Makefile.d/ft232h_test.c b/Makefile.d/ft232h_test.c new file mode 100644 index 0000000..21bc995 --- /dev/null +++ b/Makefile.d/ft232h_test.c @@ -0,0 +1,3 @@ +#include <ftdi.h> + +enum ftdi_chip_type type = TYPE_232H; \ No newline at end of file diff --git a/Makefile.include b/Makefile.include index dccc69d..cd8ae7d 100644 --- a/Makefile.include +++ b/Makefile.include @@ -33,11 +33,23 @@ # Run the C Preprocessor with file $1 and return the last line, removing quotes. define c_macro_test -$(strip $(shell $(CC) -E $1 2>/dev/null | tail -1 | tr -d '"')) +$(strip $(call debug_shell, $(CC) -E $1 | tail -1 | tr -d '"')) endef -define c_compile_test -$(shell $(CC) -c -Wall -Werror -o /dev/null $1 2>/dev/null && echo yes || echo no) +define c_compile_test # $1: files to compile, $2: cflags +$(call debug_shell, $(CC) -c -Wall -Werror $2 $1 -o /dev/null && echo yes || echo no) +endef + +define find_dependency +$(call debug_shell, $(if $(PKG_CONFIG_LIBDIR),PKG_CONFIG_LIBDIR=$(PKG_CONFIG_LIBDIR),) $(PKG_CONFIG) --exists $1 && echo yes || echo no) +endef + +define dependency_cflags +$(call debug_shell, $(if $(PKG_CONFIG_LIBDIR),PKG_CONFIG_LIBDIR=$(PKG_CONFIG_LIBDIR),) $(PKG_CONFIG) --cflags $1 2>/dev/null) +endef + +define dependency_ldflags +$(call debug_shell, $(if $(PKG_CONFIG_LIBDIR),PKG_CONFIG_LIBDIR=$(PKG_CONFIG_LIBDIR),) $(PKG_CONFIG) --libs $1 2>/dev/null) endef define LIBPCI_TEST @@ -111,25 +123,6 @@ endef export LIBJAYLINK_TEST -define FTDI_TEST -#include <stdlib.h> -#include <ftdi.h> -struct ftdi_context *ftdic = NULL; -int main(int argc, char **argv) -{ - (void) argc; - (void) argv; - return ftdi_init(ftdic); -} -endef -export FTDI_TEST - -define FTDI_232H_TEST -#include <ftdi.h> -enum ftdi_chip_type type = TYPE_232H; -endef -export FTDI_232H_TEST - define NI845X_TEST #include <ni845x.h> -- To view, visit https://review.coreboot.org/c/flashrom/+/58623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41f5186d9f3e063c12c8c6eea888d0b0bf534259 Gerrit-Change-Number: 58623 Gerrit-PatchSet: 10 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: physmap: rename to hwaccess_physmap, create own header
by Thomas Heijligen (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Angel Pons. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60113 ) Change subject: physmap: rename to hwaccess_physmap, create own header ...................................................................... Patch Set 5: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/60113/comment/a8e47254_e1331b11 PS4, Line 41: #if CONFIG_INTERNAL == 1 > Is it contagious? :# Jop, to many #if and friends around here. ;) -- To view, visit https://review.coreboot.org/c/flashrom/+/60113 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieba6f4e94cfc3e668fcb8b3c978de5908aed2592 Gerrit-Change-Number: 60113 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 12:23:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: physmap: rename to hwaccess_physmap, create own header
by Nico Huber (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Thomas Heijligen, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60113 ) Change subject: physmap: rename to hwaccess_physmap, create own header ...................................................................... Patch Set 5: Code-Review+2 (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/60113/comment/6d57cd60_8d4ed72a PS4, Line 41: #if CONFIG_INTERNAL == 1 > This isn't needed. […] Is it contagious? :# -- To view, visit https://review.coreboot.org/c/flashrom/+/60113 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieba6f4e94cfc3e668fcb8b3c978de5908aed2592 Gerrit-Change-Number: 60113 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 12:22:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess physmap: move x86 msr related code into own files
by Nico Huber (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Thomas Heijligen, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60111 ) Change subject: hwaccess physmap: move x86 msr related code into own files ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/60111 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idc9ce9df3ea1e291ad469de59467646b294119c4 Gerrit-Change-Number: 60111 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 12:18:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: hwaccess: move x86 port I/O related code into own files
by Nico Huber (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Thomas Heijligen, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60110 ) Change subject: hwaccess: move x86 port I/O related code into own files ...................................................................... Patch Set 6: Code-Review+2 (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/60110/comment/c01c5b9e_04616157 PS5, Line 786: x86 > Done Thanks! And good idea to shuffle the if's btw. :) -- To view, visit https://review.coreboot.org/c/flashrom/+/60110 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I372b4a409f036da766c42bc406b596bc41b0f75a Gerrit-Change-Number: 60110 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 12:17:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Remove unneeded line breaks, add useful ones
by Angel Pons (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60272 ) Change subject: ichspi: Remove unneeded line breaks, add useful ones ...................................................................... Patch Set 2: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/60272/comment/4e76dba8_c67e5ec4 PS1, Line 1720: > Ah! I think I understand this now. […] Yes, this is visual alignment of the printed messages -- To view, visit https://review.coreboot.org/c/flashrom/+/60272 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7ca2902b7caaa95418b828b068c661afafdcd171 Gerrit-Change-Number: 60272 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 10:39:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Split very long init function into two
by Angel Pons (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 ) Change subject: ichspi: Split very long init function into two ...................................................................... Patch Set 3: Code-Review+1 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/58735/comment/714ccfec_6abd0111 PS3, Line 20: TEST=1) probe-read-verify-erase section-write-reboot : on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E : 2) probe and read on Panther Point (7 series PCH) > Personally, I don't really like these "tags" (here TEST or in other commits sometimes TESTED), becau […] If you still need a `TEST=` tag for some reason, you could do something like this: TEST=Check that the following scenarios still behave properly: 1) Probe-read-verify-erase section-write-reboot on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E 2) Probe and read on Panther Point (7 series PCH) Note that the text following `TEST=` provides information about how the test results were interpreted. Without this part, it might be that both tests failed spectacularly, but the commit message wouldn't be lying: the change *was* tested, but without success! 😄 And yes, manually adding Tested-by tags is perfectly fine. I only use them when someone else has tested my changes. I feel a Tested-by tag is redundant when I've tested my own changes. File ichspi.c: https://review.coreboot.org/c/flashrom/+/58735/comment/10efe72b_6a8e8ae0 PS2, Line 1822: arg = extract_programmer_param("ich_spi_mode"); > Wow! this is great idea, I might use it in future, thank you!! > I would leave this commit as is, hope it's fine. Sure, no problem! https://review.coreboot.org/c/flashrom/+/58735/comment/f33a90f0_5a50853e PS2, Line 1769: > Yes this was accidental, I removed it. Thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/58735 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b Gerrit-Change-Number: 58735 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 10:27:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Extract handling programmer param into a function
by Felix Singer (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58736 ) Change subject: ichspi: Extract handling programmer param into a function ...................................................................... Patch Set 3: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/58736/comment/db443982_9209e503 PS1, Line 1761: if (arg && !strcmp(arg, "hwseq")) { > Nice to see you here Felix :) All good that was not blocking. […] Well, it's about reworking (rather rewriting) the arguments parser. I don't think people will like this since this will cause a lot of review work 😄 I will look at the current state and see how it is currently done. Maybe it's not even possible. -- To view, visit https://review.coreboot.org/c/flashrom/+/58736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20e2379a6fd58c9346f0a2d6daf2b8decf1f6976 Gerrit-Change-Number: 58736 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 07:38:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Extract handling programmer param into a function
by Felix Singer (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58736 ) Change subject: ichspi: Extract handling programmer param into a function ...................................................................... Patch Set 3: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/58736/comment/eeb7369f_06396c50 PS2, Line 1764: arg > Just checking: this idea can go in a separate commit, right? I think so. I would do that in a separate commit so that changes don't get mixed up. -- To view, visit https://review.coreboot.org/c/flashrom/+/58736 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20e2379a6fd58c9346f0a2d6daf2b8decf1f6976 Gerrit-Change-Number: 58736 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 07:26:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Split very long init function into two
by Felix Singer (Code Review) 22 Dec '21

22 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 ) Change subject: ichspi: Split very long init function into two ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/58735/comment/3c599105_133c98b5 PS3, Line 20: TEST=1) probe-read-verify-erase section-write-reboot : on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E : 2) probe and read on Panther Point (7 series PCH) > Did I do the right by adding test info into commit message? Do I need to added Tested-By tags? I don […] Personally, I don't really like these "tags" (here TEST or in other commits sometimes TESTED), because this way you are not able to write multiple lines which still fit to the style of the rest of the commit message. So, what people do is, they write half broken sentences (to shorten the characters count) and sometimes I have no idea what they are trying to tell me or what the meaning is. However, I would only use tags if the text behind it fits in the same line. Otherwise, like in your case, I would write it as follow and I would move it before that BUG tag, to make it part of the "actual" commit summary (since I think it is relevant information): Tests done: 1) Probe-read-verify-erase section-write-reboot on Intel octopus board with GD25LQ128C/GD25LQ128D/GD25LQ128E 2) Probe and read on Panther Point (7 series PCH) Of course you can add a Tested-by tag for another person. Even if it is not you, it adds useful information for the readers, since they are able to contact these persons then. There might be cases, where there are multiple ways to test things. Everyone might use a different way and each one might give another experience (not only in knowledge, but also in usability and meaningfulness). So this is definitely useful :) -- To view, visit https://review.coreboot.org/c/flashrom/+/58735 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b Gerrit-Change-Number: 58735 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 22 Dec 2021 07:07:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021