- flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add mediatek_i2c_spi interface
by Neill Corlett (Code Review) 26 Jan '22

26 Jan '22

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Neill Corlett has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 ) Change subject: Add mediatek_i2c_spi interface ...................................................................... Patch Set 2: (20 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61288/comment/06a1a6eb_3ddab73a PS1, Line 7: . > nit: no period at the end of commit summaries Done Patchset: PS1: > Hi Neill, thanks for your patch. […] - It definitely looks like there's an opportunity for code sharing with mstarddc_spi, but the ISP process for our specific chip (TSUMOP82JUQ) requires extra steps that aren't included in that driver. - We had two different board designs, one of which connects the TSUM* to SMBus and the other to raw I2C. This driver is tested on both. The finished product will be using I2C. I've left the SMBus code in as a convenience so we can continue to flash our earlier prototypes. - GPIO numbers are specific to TSUMOP82JUQ. My understanding is that these particular GPIO lines, for that chip, are standard for connecting to SPI WP#. Documentation that we received from PanVision just says to poke them and doesn't provide further detail. Patchset: PS2: Thanks for the review! File mediatek_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/61288/comment/475f3f5b_28279868 PS1, Line 29: #define ISP_PORT 0x92 >> 1 : #define DEBUG_PORT 0xb2 >> 1 > Please wrap these in parentheses to avoid operation order problems: […] Done https://review.coreboot.org/c/flashrom/+/61288/comment/d0dbe83f_1f14c121 PS1, Line 36: > nit: double blank line Done https://review.coreboot.org/c/flashrom/+/61288/comment/29bc701b_e2c4601b PS1, Line 55: uint8_t* buf > The `*` unary operator should have a space before, not after: […] Done https://review.coreboot.org/c/flashrom/+/61288/comment/fccf3bbf_fb9ace94 PS1, Line 66: d > with ret value so we know how the ioctl actually failed. Done https://review.coreboot.org/c/flashrom/+/61288/comment/7a94d3e0_17e64427 PS1, Line 96: args.data->block[0] > just 'len' here, avoids needing to do that indirection there to check we are in fact memcpy to a hea […] Done https://review.coreboot.org/c/flashrom/+/61288/comment/299da758_d0f925a0 PS1, Line 96: args.data->block > args. […] This is copying the data out of the i2c_smbus_data, starting at 1, using the size specified by [0]. https://review.coreboot.org/c/flashrom/+/61288/comment/669167ec_40980ef1 PS1, Line 181: 2 > ARRAY_SIZE(data) […] Done https://review.coreboot.org/c/flashrom/+/61288/comment/dfdae254_e4fba6a4 PS1, Line 211: 3 > ARRAY_SIZE(data) Done https://review.coreboot.org/c/flashrom/+/61288/comment/322fbf98_68d6b397 PS1, Line 242: GPIO line > +1 This is specific to the revision of chip that our board uses (TSUMOP82JUQ). https://review.coreboot.org/c/flashrom/+/61288/comment/53248304_ee88dc61 PS1, Line 249: 0x426, 7 > What are these magic numbers? The TSUMOP82JUQ has a GPIO line going to WP# on SPI, and these two registers (0x426 and 0x428) control its state. We're following a ISP guide given to us by PanVision that doesn't provide any additional details. https://review.coreboot.org/c/flashrom/+/61288/comment/0062d36f_660ae9bd PS1, Line 266: { > Please move the brace to the following line Done https://review.coreboot.org/c/flashrom/+/61288/comment/8bad9221_a8d57a63 PS1, Line 269: unsigned char > This assumes that the size of `unsigned char` is always 1 byte. I'd use `uint8_t`. Done https://review.coreboot.org/c/flashrom/+/61288/comment/a63d8c5f_bcbe12f3 PS1, Line 271: sizeof > Along with the changes above, this should then be `ARRAY_SIZE` Done https://review.coreboot.org/c/flashrom/+/61288/comment/7024df48_d34a665a PS1, Line 295: 0x7F > Please use lowercase for hex Done https://review.coreboot.org/c/flashrom/+/61288/comment/6a3bcdaf_82dcf852 PS1, Line 330: { > Please move the brace to the following line Done https://review.coreboot.org/c/flashrom/+/61288/comment/19e4635e_c9418481 PS1, Line 348: } > If both attempts fail, can we assume that entering ISP has failed? Observed behavior is that this can fail if the chip is already in ISP mode. https://review.coreboot.org/c/flashrom/+/61288/comment/75eef19a_33ee7eea PS1, Line 360: { > Please move the brace to the following line Done -- To view, visit https://review.coreboot.org/c/flashrom/+/61288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Gerrit-Change-Number: 61288 Gerrit-PatchSet: 2 Gerrit-Owner: Neill Corlett <corlett(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.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-Comment-Date: Tue, 25 Jan 2022 18:45:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add mediatek_i2c_spi interface
by Neill Corlett (Code Review) 26 Jan '22

26 Jan '22

Attention is currently required from: Edward O'Callaghan, Neill Corlett. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/61288 to look at the new patch set (#2). Change subject: Add mediatek_i2c_spi interface ...................................................................... Add mediatek_i2c_spi interface Add a spi_master interface supporting MediaTek MST9U ISP mode. Autodetect the bus type via I2C_FUNC_I2C, and use the appropriate read/write commands, in case the MST9U is attached to smbus. TEST=Successfully programmed SPI on test hardware. Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Signed-off-by: Neill Corlett <corlett(a)google.com> --- M Makefile A mediatek_i2c_spi.c M meson.build M meson_options.txt M programmer.h M programmer_table.c 6 files changed, 531 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/88/61288/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820 Gerrit-Change-Number: 61288 Gerrit-PatchSet: 2 Gerrit-Owner: Neill Corlett <corlett(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Neill Corlett <corlett(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: meson: sync programmer dependencies from Makefile
by Nico Huber (Code Review) 26 Jan '22

26 Jan '22

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/61287 ) Change subject: meson: sync programmer dependencies from Makefile ...................................................................... meson: sync programmer dependencies from Makefile The Makefile recently gained finer-grained programmer dependency lists allowing it to track which enabled programmers assume various things about the system, like availability of libraries or the CPU architecture. This change implements the same changes in the Meson configuration file. This fixes a number of programmers to correctly build on non-x86 systems, because they were previously misclassified as dependent on x86 architectural features but actually only used PCI. BUG=none TEST=meson build succeeds on both x86 and ARM Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: Iae93111fd48865f3fe8dd0eb637349b9a0c4affc Reviewed-on: https://review.coreboot.org/c/flashrom/+/61287 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Thomas Heijligen <src(a)posteo.de> --- M meson.build 1 file changed, 65 insertions(+), 44 deletions(-) Approvals: build bot (Jenkins): Verified Thomas Heijligen: Looks good to me, but someone else must approve Edward O'Callaghan: Looks good to me, approved diff --git a/meson.build b/meson.build index 4c33bf5..cb34e54 100644 --- a/meson.build +++ b/meson.build @@ -105,8 +105,48 @@ deps = [] srcs = [] -need_raw_access = false -need_serial = false +host_is_x86 = ['x86', 'x86_64'].contains(host_machine.cpu_family()) + +need_serial = [ + config_buspirate_spi, config_pony_spi, config_serprog, +].contains(true) +need_bitbang_spi = [ + config_internal, config_nicintel_spi, config_ogp_spi, + config_pony_spi, config_rayer_spi, +].contains(true) +need_raw_mem_access = [ + config_atapromise, config_drkaiser, config_gfxnvidia, config_internal, + config_it8212, config_nicintel, config_nicintel_eeprom, config_nicintel_spi, + config_ogp_spi, config_satamv, config_satasii, +].contains(true) +# Internal programmer uses x86 features if the system is x86 +need_x86_msr = config_internal and host_is_x86 +need_x86_port_io = [ + config_atahpt, config_atapromise, config_internal and host_is_x86, + config_nic3com, config_nicnatsemi, config_nicrealtek, config_rayer_spi, + config_satamv, +].contains(true) +need_libpci = [ + config_atahpt, config_atapromise, config_atavia, + config_drkaiser, config_gfxnvidia, config_internal, config_it8212, + config_nic3com, config_nicintel, config_nicintel_eeprom, config_nicintel_spi, + config_nicnatsemi, config_nicrealtek, config_ogp_spi, config_satamv, + config_satasii, +].contains(true) +need_libusb1 = [ + config_ch341a_spi, config_dediprog, config_developerbox_spi, + config_digilent_spi, config_pickit2_spi, config_raiden_debug_spi, + config_stlinkv3_spi, +].contains(true) +need_libftdi1 = [ + config_ft2232_spi, config_usbblaster_spi, +].contains(true) +need_libjaylink = config_jlink_spi + +if (need_x86_port_io or need_x86_msr) and not host_is_x86 + error('one or more enabled programmer only supports x86 and target is not') +endif + # check for required symbols if cc.has_function('clock_gettime') @@ -124,38 +164,25 @@ srcs += 'usbdev.c' srcs += 'usb_device.c' deps += dependency('libusb-1.0') -else - config_ch341a_spi = false - config_dediprog = false - config_digilent_spi = false - config_developerbox_spi = false - config_pickit2_spi = false - config_raiden_debug_spi = false +elif need_libusb1 + error('usb is disabled but one or more enabled programmer requires USB access') endif # some programmers require libpci if get_option('pciutils') srcs += 'pcidev.c' deps += dependency('libpci') - need_raw_access = true cargs += '-DNEED_PCI=1' -else - config_atahpt = false - config_atapromise = false - config_atavia = false - config_drkaiser = false - config_gfxnvidia = false - config_internal = false - config_it8212 = false - config_nic3com = false - config_nicintel_eeprom = false - config_nicintel = false - config_nicintel_spi = false - config_nicnatsemi = false - config_nicrealtek = false - config_ogp_spi = false - config_satamv = false - config_satasii = false +elif need_libpci + error('pciutils is disabled but one or more enabled programmer requires PCI access') +endif + +if need_libftdi1 + deps += dependency('libftdi1') +endif + +if need_libjaylink + deps += dependency('libjaylink') endif # set defines for configured programmers @@ -174,7 +201,6 @@ if config_buspirate_spi srcs += 'buspirate_spi.c' cargs += '-DCONFIG_BUSPIRATE_SPI=1' - need_serial = true endif if config_ch341a_spi srcs += 'ch341a_spi.c' @@ -195,7 +221,6 @@ if config_jlink_spi srcs += 'jlink_spi.c' cargs += '-DCONFIG_JLINK_SPI=1' - deps += dependency('libjaylink') endif if config_drkaiser srcs += 'drkaiser.c' @@ -208,7 +233,6 @@ if config_ft2232_spi srcs += 'ft2232_spi.c' cargs += '-DCONFIG_FT2232_SPI=1' - deps += dependency('libftdi1') cargs += '-DHAVE_FT232H=1' endif if config_gfxnvidia @@ -225,7 +249,7 @@ srcs += 'chipset_enable.c' srcs += 'internal.c' srcs += 'processor_enable.c' - if target_machine.cpu_family() == 'x86' or target_machine.cpu_family() == 'x86_64' + if host_is_x86 srcs += 'amd_imc.c' srcs += 'dmi.c' srcs += 'ichspi.c' @@ -235,7 +259,6 @@ srcs += 'sb600spi.c' srcs += 'wbsio_spi.c' endif - config_bitbang_spi = true cargs += '-DCONFIG_INTERNAL=1' if get_option('config_internal_dmi') # Use internal DMI/SMBIOS decoder by default instead of relying on dmidecode. @@ -272,7 +295,6 @@ endif if config_nicintel_spi srcs += 'nicintel_spi.c' - config_bitbang_spi = true cargs += '-DCONFIG_NICINTEL_SPI=1' endif if config_nicnatsemi @@ -284,7 +306,6 @@ cargs += '-DCONFIG_NICREALTEK=1' endif if config_ogp_spi - config_bitbang_spi = true srcs += 'ogp_spi.c' cargs += '-DCONFIG_OGP_SPI=1' endif @@ -294,14 +315,10 @@ endif if config_pony_spi srcs += 'pony_spi.c' - need_serial = true - config_bitbang_spi = true cargs += '-DCONFIG_PONY_SPI=1' endif if config_rayer_spi srcs += 'rayer_spi.c' - config_bitbang_spi = true - need_raw_access = true cargs += '-DCONFIG_RAYER_SPI=1' endif if config_satamv @@ -315,7 +332,6 @@ if config_serprog srcs += 'serprog.c' cargs += '-DCONFIG_SERPROG=1' - need_serial = true endif if config_usbblaster_spi srcs += 'usbblaster_spi.c' @@ -335,7 +351,7 @@ endif # bitbanging SPI infrastructure -if config_bitbang_spi +if need_bitbang_spi srcs += 'bitbang_spi.c' cargs += '-DCONFIG_BITBANG_SPI=1' endif @@ -344,14 +360,19 @@ srcs += 'i2c_helper_linux.c' endif -# raw memory, MSR or PCI port I/O access -if need_raw_access - srcs += 'hwaccess_x86_io.c' - srcs += 'hwaccess_x86_msr.c' +if need_raw_mem_access srcs += 'hwaccess_physmap.c' +endif + +if need_x86_port_io + srcs += 'hwaccess_x86_io.c' cargs += '-D__FLASHROM_HAVE_OUTB__=1' endif +if need_x86_msr + srcs += 'hwaccess_x86_msr.c' +endif + # raw serial IO if need_serial srcs += 'serial.c' 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. -- To view, visit https://review.coreboot.org/c/flashrom/+/61287 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iae93111fd48865f3fe8dd0eb637349b9a0c4affc Gerrit-Change-Number: 61287 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: hwaccess: fix build on non-x86 targets
by Nico Huber (Code Review) 26 Jan '22

26 Jan '22

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/61194 ) Change subject: hwaccess: fix build on non-x86 targets ...................................................................... hwaccess: fix build on non-x86 targets The changes to hwaccess in commit 49d758698a0dd166679c48b1a2785e50e9b0cc83 cause build failure on non-x86 systems because the hwaccess_x86_* headers are included in some files that are built for all platforms (particularly those in the internal programmer) and those headers in turn include <sys/io.h> which only exists on x86. This change avoids including those headers on non-x86 platforms so the internal programmer can be built without errors. The comment on the stub implementation of rget_io_perms() is also modified to remove references to non-x86 platforms, since that file is only built on x86 now. BUG=None TEST=meson build succeeds for both x86 and ARM targets Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e Reviewed-on: https://review.coreboot.org/c/flashrom/+/61194 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Thomas Heijligen <src(a)posteo.de> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M board_enable.c M chipset_enable.c M hwaccess_x86_io.c M internal.c 4 files changed, 16 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Thomas Heijligen: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/board_enable.c b/board_enable.c index c23e257..94cfc9d 100644 --- a/board_enable.c +++ b/board_enable.c @@ -25,11 +25,13 @@ #include <stdlib.h> #include "flash.h" #include "programmer.h" -#include "hwaccess_x86_io.h" -#include "hwaccess_x86_msr.h" #include "platform/pci.h" #if defined(__i386__) || defined(__x86_64__) + +#include "hwaccess_x86_io.h" +#include "hwaccess_x86_msr.h" + /* * Helper functions for many Winbond Super I/Os of the W836xx range. */ diff --git a/chipset_enable.c b/chipset_enable.c index d72300e..fe7ec15 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -33,8 +33,6 @@ #include <errno.h> #include "flash.h" #include "programmer.h" -#include "hwaccess_x86_io.h" -#include "hwaccess_x86_msr.h" #include "hwaccess_physmap.h" #include "platform/pci.h" @@ -42,6 +40,9 @@ #if defined(__i386__) || defined(__x86_64__) +#include "hwaccess_x86_io.h" +#include "hwaccess_x86_msr.h" + static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name) { uint8_t tmp; diff --git a/hwaccess_x86_io.c b/hwaccess_x86_io.c index 3152bfe..fc6ee54 100644 --- a/hwaccess_x86_io.c +++ b/hwaccess_x86_io.c @@ -79,7 +79,6 @@ #else /* DJGPP and libpayload environments have full PCI port I/O permissions by default. */ -/* PCI port I/O support is unimplemented on PPC/MIPS and unavailable on ARM. */ int rget_io_perms(void) { return 0; diff --git a/internal.c b/internal.c index bb606ca..b8e0564 100644 --- a/internal.c +++ b/internal.c @@ -19,10 +19,13 @@ #include <stdlib.h> #include "flash.h" #include "programmer.h" -#include "hwaccess_x86_io.h" #include "hwaccess_physmap.h" #include "platform/pci.h" +#if defined(__i386__) || defined(__x86_64__) +#include "hwaccess_x86_io.h" +#endif + int is_laptop = 0; int laptop_ok = 0; @@ -241,11 +244,6 @@ } free(arg); - if (rget_io_perms()) { - ret = 1; - goto internal_init_exit; - } - /* Default to Parallel/LPC/FWH flash devices. If a known host controller * is found, the host controller init routine sets the * internal_buses_supported bitfield. @@ -271,6 +269,11 @@ } #if defined(__i386__) || defined(__x86_64__) + if (rget_io_perms()) { + ret = 1; + goto internal_init_exit; + } + if ((cb_parse_table(&cb_vendor, &cb_model) == 0) && (board_vendor != NULL) && (board_model != NULL)) { if (strcasecmp(board_vendor, cb_vendor) || strcasecmp(board_model, cb_model)) { msg_pwarn("Warning: The mainboard IDs set by -p internal:mainboard (%s:%s) do not\n" -- To view, visit https://review.coreboot.org/c/flashrom/+/61194 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e Gerrit-Change-Number: 61194 Gerrit-PatchSet: 9 Gerrit-Owner: Peter Marheine <pmarheine(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: hwaccess: fix build on non-x86 targets
by Thomas Heijligen (Code Review) 26 Jan '22

26 Jan '22

Attention is currently required from: Paul Menzel, Edward O'Callaghan, Peter Marheine. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 ) Change subject: hwaccess: fix build on non-x86 targets ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61194 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e Gerrit-Change-Number: 61194 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Marheine <pmarheine(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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 25 Jan 2022 16:36:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Elkhart Lake support
by Werner Zeh (Code Review) 25 Jan '22

25 Jan '22

Attention is currently required from: Nico Huber. Hello build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60711 to look at the new patch set (#3). Change subject: Add Elkhart Lake support ...................................................................... Add Elkhart Lake support Elkhart Lake has a chipset called Mule Creek Canyon which is quite compatible with 300 series chipsets. There are a few differences though, e.g. different encoding for the SPI clock values for read and write in the FLCOMP register. In addition Elkhart Lake has a new PCI device ID for the SPI controller which is added, too. TEST=Read and flash complete flash on Siemens MC EHL1 Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com> --- M chipset_enable.c M ich_descriptors.c M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 5 files changed, 47 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/60711/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/60711 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Gerrit-Change-Number: 60711 Gerrit-PatchSet: 3 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.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: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Add Elkhart Lake support
by Werner Zeh (Code Review) 25 Jan '22

25 Jan '22

Attention is currently required from: Nico Huber. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60711 ) Change subject: Add Elkhart Lake support ...................................................................... Patch Set 2: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/60711/comment/2b446403_aa04df79 PS1, Line 1033: return CHIPSET_500_SERIES_TIGER_POINT; > You nailed it, Nico. […] Turned out the SPI programming guide is wrong and will be updated. But we can rely on the values in the descriptor. I will change this patch accordingly. -- To view, visit https://review.coreboot.org/c/flashrom/+/60711 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I711e39a3ec9cd7098389231eaa1cb864d615a475 Gerrit-Change-Number: 60711 Gerrit-PatchSet: 2 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 25 Jan 2022 11:08:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors.c Invert the meaning of 'dual_output' bit
by Werner Zeh (Code Review) 25 Jan '22

25 Jan '22

Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/61362 to look at the new patch set (#2). Change subject: ich_descriptors.c Invert the meaning of 'dual_output' bit ...................................................................... ich_descriptors.c Invert the meaning of 'dual_output' bit In the Flash Component description register (FLCOMP) bit 30 reports the capability of using dual output for fast read operation on the flash component. According to various SPI Programming Guides (checked for Panther Point, Lewisburg C620, Apollo Lake and Elkhart Lake) the dual output is enabled when this bit is set and disabled if not. Currently the logic displays it the other way around when parsing the descriptor. This patch changes this so now if bit 30 in FLCOMP is not set, dual read support for fast read operation is shown as disabled. Change-Id: If6282ac8326ab0b92e9c70c09dba0299bf0deb6f Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com> --- M ich_descriptors.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/61362/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/61362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If6282ac8326ab0b92e9c70c09dba0299bf0deb6f Gerrit-Change-Number: 61362 Gerrit-PatchSet: 2 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: util/ich_descriptors_tool: Invert the meaning of 'dual_output' bit
by Werner Zeh (Code Review) 25 Jan '22

25 Jan '22

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61362 ) Change subject: util/ich_descriptors_tool: Invert the meaning of 'dual_output' bit ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61362/comment/527a52c5_999fd5e8 PS1, Line 7: util/ich_descriptors_tool > nit: I'd use `ich_descriptors.c` as file path, the code is also used by flashrom itself. Ack https://review.coreboot.org/c/flashrom/+/61362/comment/a0b240ab_b57e9b06 PS1, Line 11: According to the SPI Programming Guide the dual output is : enabled when this bit is set and disabled if not (checked for Panther : Point, Lewisburg C620, Apollo Lake and Elkhart Lake). > Starting with "According to the SPI Programming Guide" makes it seem like you only checked one SPI p […] Fine with me, will adapt. -- To view, visit https://review.coreboot.org/c/flashrom/+/61362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If6282ac8326ab0b92e9c70c09dba0299bf0deb6f Gerrit-Change-Number: 61362 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Tue, 25 Jan 2022 11:07:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: util/ich_descriptors_tool: Invert the meaning of 'dual_output' bit
by Angel Pons (Code Review) 25 Jan '22

25 Jan '22

Attention is currently required from: Werner Zeh. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61362 ) Change subject: util/ich_descriptors_tool: Invert the meaning of 'dual_output' bit ...................................................................... Patch Set 1: Code-Review+2 (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/61362/comment/15303ce1_a9734f79 PS1, Line 7: util/ich_descriptors_tool nit: I'd use `ich_descriptors.c` as file path, the code is also used by flashrom itself. https://review.coreboot.org/c/flashrom/+/61362/comment/82c4ced2_4120f43a PS1, Line 11: According to the SPI Programming Guide the dual output is : enabled when this bit is set and disabled if not (checked for Panther : Point, Lewisburg C620, Apollo Lake and Elkhart Lake). Starting with "According to the SPI Programming Guide" makes it seem like you only checked one SPI programming guide. I'd suggest rephrasing the sentence as follows: According to the SPI Programming Guides for Panther Point, Lewisburg C620, Apollo Lake and Elkhart Lake, the dual output is enabled when this bit is set and disabled if not. Another option would be to move the part in parentheses closer to "SPI Programming Guide": According to various SPI Programming Guides (checked for Panther Point, Lewisburg C620, Apollo Lake and Elkhart Lake), the dual output is enabled when this bit is set and disabled if not. Patchset: PS1: I checked the SPI programming guides for Lynx Point, Wildcat Point, Bay Trail, Skylake-H and Gemini Lake; all agree with this change. -- To view, visit https://review.coreboot.org/c/flashrom/+/61362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If6282ac8326ab0b92e9c70c09dba0299bf0deb6f Gerrit-Change-Number: 61362 Gerrit-PatchSet: 1 Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Comment-Date: Tue, 25 Jan 2022 10:15:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit