September 2017 - flashrom-gerrit

Change in flashrom[stable]: ichspi: "Fix" access permission reporting for regions > 7
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21818 Change subject: ichspi: "Fix" access permission reporting for regions > 7 ...................................................................... ichspi: "Fix" access permission reporting for regions > 7 Can't find bits that tell us the actual permissions in charge. So report them as unknown. Original-Change-Id: Ib73f95e0348f5c6d89988e3ea3529af0ec3b23a6 Original-Reviewed-on: https://review.coreboot.org/21106 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Change-Id: I59cce146c96e3c1f33b51265084c1352602ea5ac Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M ichspi.c 1 file changed, 18 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/18/21818/1 diff --git a/ichspi.c b/ichspi.c index 5f3e394..9ee8044 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1543,6 +1543,7 @@ static const char *const access_names[4] = { "locked", "read-only", "write-only", "read-write" }; + const int rwperms_unknown = ARRAY_SIZE(access_names); static const char *const region_names[5] = { "Flash Descriptor", "BIOS", "Management Engine", "Gigabit Ethernet", "Platform Data" @@ -1550,10 +1551,20 @@ const char *const region_name = i < ARRAY_SIZE(region_names) ? region_names[i] : "unknown"; uint32_t base, limit; - int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | - (((ICH_BRRA(frap) >> i) & 1) << 0); + int rwperms; int offset = ICH9_REG_FREG0 + i * 4; uint32_t freg = mmio_readl(ich_spibar + offset); + + if (i < 8) { + rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) | + (((ICH_BRRA(frap) >> i) & 1) << 0); + } else { + /* Datasheets don't define any access bits for regions > 7. We + can't rely on the actual descriptor settings either as there + are several overrides for them (those by other masters are + not even readable by us, *shrug*). */ + rwperms = rwperms_unknown; + } base = ICH_FREG_BASE(freg); limit = ICH_FREG_LIMIT(freg); @@ -1569,6 +1580,11 @@ region_name, base, limit, access_names[rwperms]); return 0; } + if (rwperms == rwperms_unknown) { + msg_pdbg("FREG%i: %s region (0x%08x-0x%08x) has unknown permissions.\n", + i, region_name, base, limit); + return 0; + } msg_pwarn("FREG%i: Warning: %s region (0x%08x-0x%08x) is %s.\n", i, region_name, base, limit, access_names[rwperms]); -- To view, visit https://review.coreboot.org/21818 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I59cce146c96e3c1f33b51265084c1352602ea5ac Gerrit-Change-Number: 21818 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[stable]: Add a convenient libflashrom interface
by build bot (Jenkins) (Code Review) 30 Sep '17

30 Sep '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21788 ) Change subject: Add a convenient libflashrom interface ...................................................................... Patch Set 1: Verified-1 Build Failed https://qa.coreboot.org/job/flashrom-customrules/583/ : FAILURE https://qa.coreboot.org/job/flashrom_gerrit/528/ : SUCCESS -- To view, visit https://review.coreboot.org/21788 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: comment Gerrit-Change-Id: Ibe14c89c96e45ed7679332b0f5cf44b1c9f1d9ee Gerrit-Change-Number: 21788 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 30 Sep 2017 20:49:06 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[stable]: chipset_enable: Mark Braswell as tested
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Hello David Hendricks, I'd like you to do a code review. Please visit https://review.coreboot.org/21817 to review the following change. Change subject: chipset_enable: Mark Braswell as tested ...................................................................... chipset_enable: Mark Braswell as tested Reported by Uwe Vieweg: https://mail.coreboot.org/pipermail/flashrom/2017-August/015059.html Original-Change-Id: Iaf7558af8737af36401f577ca7aba9fd7114a3df Original-Reviewed-on: https://review.coreboot.org/20923 Original-Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Original-Reviewed-by: Nico Huber <nico.h(a)gmx.de> Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Change-Id: I77c49c89ca552e8483022e3cd2a1db10b52554b1 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M chipset_enable.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/21817/1 diff --git a/chipset_enable.c b/chipset_enable.c index 6a93d0d..36e2838 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1740,7 +1740,7 @@ {0x8086, 0x1f39, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, {0x8086, 0x1f3a, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, {0x8086, 0x1f3b, NT, "Intel", "Avoton/Rangeley", enable_flash_silvermont}, - {0x8086, 0x229c, NT, "Intel", "Braswell", enable_flash_silvermont}, + {0x8086, 0x229c, OK, "Intel", "Braswell", enable_flash_silvermont}, {0x8086, 0x2310, NT, "Intel", "DH89xxCC (Cave Creek)", enable_flash_pch7}, {0x8086, 0x2390, NT, "Intel", "Coleto Creek", enable_flash_pch7}, {0x8086, 0x2410, OK, "Intel", "ICH", enable_flash_ich0}, -- To view, visit https://review.coreboot.org/21817 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I77c49c89ca552e8483022e3cd2a1db10b52554b1 Gerrit-Change-Number: 21817 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>

1 0

Change in flashrom[stable]: Include <sys/types.h> wherever ssize_t is used
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21816 Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Include <sys/types.h> wherever ssize_t is used `ssize_t` is a POSIX type (cf. IEEE Std 1003.1). Original-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Original-Reviewed-on: https://review.coreboot.org/21015 Original-Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Original-Reviewed-by: Urja Rannikko <urjaman(a)gmail.com> Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Change-Id: Iffabf97a2e3f3540e5969e214f9c49ac06fa2597 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reported-by: Urja Rannikko <urjaman(a)gmail.com> --- M dediprog.c M ich_descriptors.c M ich_descriptors.h 3 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/16/21816/1 diff --git a/dediprog.c b/dediprog.c index 6f82772..2f5b441 100644 --- a/dediprog.c +++ b/dediprog.c @@ -21,6 +21,7 @@ #include "platform.h" +#include <sys/types.h> #include <stdlib.h> #include <stdio.h> #include <string.h> diff --git a/ich_descriptors.c b/ich_descriptors.c index a2f8edf..7916152 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -34,6 +34,7 @@ #define UPPER_MAP_OFFSET (4096 - 256 - 4) #define getVTBA(flumap) (((flumap)->FLUMAP1 << 4) & 0x00000ff0) +#include <sys/types.h> #include <string.h> #include "flash.h" /* for msg_* */ #include "programmer.h" diff --git a/ich_descriptors.h b/ich_descriptors.h index 574bc5f..4ddc710 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -22,6 +22,7 @@ #ifndef __ICH_DESCRIPTORS_H__ #define __ICH_DESCRIPTORS_H__ 1 +#include <sys/types.h> #include <stdint.h> #include "programmer.h" /* for enum ich_chipset */ -- To view, visit https://review.coreboot.org/21816 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: Iffabf97a2e3f3540e5969e214f9c49ac06fa2597 Gerrit-Change-Number: 21816 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[stable]: ich_descriptors: Modify limits for C620/Lewisburg PCH
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21815 Change subject: ich_descriptors: Modify limits for C620/Lewisburg PCH ...................................................................... ich_descriptors: Modify limits for C620/Lewisburg PCH Original-Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Original-Reviewed-on: https://review.coreboot.org/20936 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Original-Reviewed-by: Nico Huber <nico.h(a)gmx.de> Change-Id: Id755183cd2d86d050b722f150450eb67cd90b1d4 Signed-off-by: David Hendricks <dhendricks(a)fb.com> --- M ich_descriptors.h 1 file changed, 3 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/21815/1 diff --git a/ich_descriptors.h b/ich_descriptors.h index 8c313d1..574bc5f 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -165,7 +165,7 @@ }; }; -#define MAX_NUM_FLREGS 10 /* 16 on unsupported Lewisburg PCH */ +#define MAX_NUM_FLREGS 16 struct ich_desc_region { /* * Number of entries and width differ on various generations: @@ -174,6 +174,7 @@ * ICH8 .. Panther Point/7 5 13 * Lynx Point/8 .. Wildcat Point/9 7 15 * Sunrise Point/100 .. 10 15 + * Lewisburg/100 .. 16 15 */ union { uint32_t FLREGs[MAX_NUM_FLREGS]; /* Flash Descriptor Regions */ @@ -188,7 +189,7 @@ }; }; -#define MAX_NUM_MASTERS 5 /* 6 on unsupported Lewisburg PCH */ +#define MAX_NUM_MASTERS 6 /* 5 prior to C620/Lewisburg PCH */ struct ich_desc_master { union { uint32_t FLMSTRs[MAX_NUM_MASTERS]; /* Flash Masters */ -- To view, visit https://review.coreboot.org/21815 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: Id755183cd2d86d050b722f150450eb67cd90b1d4 Gerrit-Change-Number: 21815 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[stable]: rpci: Use pci_dev struct pointer to avoid API breaks
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21814 Change subject: rpci: Use pci_dev struct pointer to avoid API breaks ...................................................................... rpci: Use pci_dev struct pointer to avoid API breaks The pci_dev structure is never meant to be used as is, but always as a pointer. By using the struct itself in undo_pci_write_data, we are risking data corruption, or buffer overflows if the structure size changes. This is especially apparent on my system where flashrom segfaults because I compile it with pciutils 3.3.0 and I run it on a system with pciutils 3.5.2. The struture size is different and causes a struct with the wrong size to be sent to the library, with invalid internal field values. This has been discovered and discussed in Change ID 18925 [1] [1] https://review.coreboot.org/#/c/18925/ Original-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Original-Reviewed-on: https://review.coreboot.org/20784 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: Nico Huber <nico.h(a)gmx.de> Change-Id: I58dc4f37c5ea29cbad30777ebc0e6c2e3c6ad446 Signed-off-by: Youness Alaoui <kakaroto(a)kakaroto.homelinux.net> --- M chipset_enable.c M pcidev.c M programmer.h 3 files changed, 25 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/21814/1 diff --git a/chipset_enable.c b/chipset_enable.c index 20d2662..6a93d0d 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -843,6 +843,7 @@ * straints (e.g. on PCI domains, extended PCIe config space). */ struct pci_access *const pci_acc = pci_alloc(); + struct pci_access *const saved_pacc = pacc; if (!pci_acc) { msg_perr("Can't allocate PCI accessor.\n"); return ret; @@ -857,6 +858,9 @@ return ret; } + /* Modify pacc so the rpci_write can register the undo callback with a + * device using the correct pci_access */ + pacc = pci_acc; enable_flash_ich_report_gcs(spi_dev, pch_generation, NULL); const int ret_bc = enable_flash_ich_bios_cntl_config_space(spi_dev, pch_generation, 0xdc); @@ -880,6 +884,7 @@ _freepci_ret: pci_free_dev(spi_dev); + pacc = saved_pacc; return ret; } diff --git a/pcidev.c b/pcidev.c index 2c78063..f4e5542 100644 --- a/pcidev.c +++ b/pcidev.c @@ -160,6 +160,7 @@ return 1; } pci_cleanup(pacc); + pacc = NULL; return 0; } @@ -259,7 +260,7 @@ }; struct undo_pci_write_data { - struct pci_dev dev; + struct pci_dev *dev; int reg; enum pci_write_type type; union { @@ -272,22 +273,23 @@ int undo_pci_write(void *p) { struct undo_pci_write_data *data = p; - if (pacc == NULL) { - msg_perr("%s: Tried to undo PCI writes without a valid PCI context!\n" - "Please report a bug at flashrom(a)flashrom.org\n", __func__); + if (pacc == NULL || data->dev == NULL) { + msg_perr("%s: Tried to undo PCI writes without a valid PCI %s!\n" + "Please report a bug at flashrom(a)flashrom.org\n", + __func__, data->dev == NULL ? "device" : "context"); return 1; } msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n", - data->dev.bus, data->dev.dev, data->dev.func, data->reg); + data->dev->bus, data->dev->dev, data->dev->func, data->reg); switch (data->type) { case pci_write_type_byte: - pci_write_byte(&data->dev, data->reg, data->bytedata); + pci_write_byte(data->dev, data->reg, data->bytedata); break; case pci_write_type_word: - pci_write_word(&data->dev, data->reg, data->worddata); + pci_write_word(data->dev, data->reg, data->worddata); break; case pci_write_type_long: - pci_write_long(&data->dev, data->reg, data->longdata); + pci_write_long(data->dev, data->reg, data->longdata); break; } /* p was allocated in register_undo_pci_write. */ @@ -303,7 +305,11 @@ msg_gerr("Out of memory!\n"); \ exit(1); \ } \ - undo_pci_write_data->dev = *a; \ + if (pacc) \ + undo_pci_write_data->dev = pci_get_dev(pacc, \ + a->domain, a->bus, a->dev, a->func); \ + else \ + undo_pci_write_data->dev = NULL; \ undo_pci_write_data->reg = b; \ undo_pci_write_data->type = pci_write_type_##c; \ undo_pci_write_data->c##data = pci_read_##c(dev, reg); \ diff --git a/programmer.h b/programmer.h index ec00bd9..e58fd32 100644 --- a/programmer.h +++ b/programmer.h @@ -195,6 +195,11 @@ struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. + * To clone the pci_dev instances internally, the `pacc` global + * variable has to reference a pci_access method that is compatible + * with the given pci_dev handle. The referenced pci_access (not + * the variable) has to stay valid until the shutdown handlers are + * finished. */ int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data); int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data); -- To view, visit https://review.coreboot.org/21814 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I58dc4f37c5ea29cbad30777ebc0e6c2e3c6ad446 Gerrit-Change-Number: 21814 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[stable]: Enable continuous SPI reads
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Hello Urja Rannikko, I'd like you to do a code review. Please visit https://review.coreboot.org/21813 to review the following change. Change subject: Enable continuous SPI reads ...................................................................... Enable continuous SPI reads Previous unnecessary page-by-page reading is repurposed to read by big naturally aligned areas (now chip size limited to 16MB for future-proofing of 4 byte addressed multi-die chips) and serprog hack for continuous reads is removed. Original-Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Original-Reviewed-on: https://review.coreboot.org/20223 Original-Reviewed-by: Nico Huber <nico.h(a)gmx.de> Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Change-Id: I8fdf4621847b1973e5d7344b369d63da5c62e16f Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> --- M serprog.c M spi25.c 2 files changed, 15 insertions(+), 35 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/21813/1 diff --git a/serprog.c b/serprog.c index 98aac83..25c9944 100644 --- a/serprog.c +++ b/serprog.c @@ -303,15 +303,13 @@ unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); -static int serprog_spi_read(struct flashctx *flash, uint8_t *buf, - unsigned int start, unsigned int len); static struct spi_master spi_master_serprog = { .type = SPI_CONTROLLER_SERPROG, .max_data_read = MAX_DATA_READ_UNLIMITED, .max_data_write = MAX_DATA_WRITE_UNLIMITED, .command = serprog_spi_send_command, .multicommand = default_spi_send_multicommand, - .read = serprog_spi_read, + .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, }; @@ -931,25 +929,6 @@ readarr); free(parmbuf); return ret; -} - -/* FIXME: This function is optimized so that it does not split each transaction - * into chip page_size long blocks unnecessarily like spi_read_chunked. This has - * the advantage that it is much faster for most chips, but breaks those with - * non-continuous reads. When spi_read_chunked is fixed this method can be removed. */ -static int serprog_spi_read(struct flashctx *flash, uint8_t *buf, - unsigned int start, unsigned int len) -{ - unsigned int i, cur_len; - const unsigned int max_read = spi_master_serprog.max_data_read; - for (i = 0; i < len; i += cur_len) { - int ret; - cur_len = min(max_read, (len - i)); - ret = spi_nbyte_read(flash, start + i, buf + i, cur_len); - if (ret) - return ret; - } - return 0; } void *serprog_map(const char *descr, uintptr_t phys_addr, size_t len) diff --git a/spi25.c b/spi25.c index af4b6db..76242be 100644 --- a/spi25.c +++ b/spi25.c @@ -940,30 +940,31 @@ /* * Read a part of the flash chip. * FIXME: Use the chunk code from Michael Karcher instead. - * Each page is read separately in chunks with a maximum size of chunksize. + * Each naturally aligned area is read separately in chunks with a maximum size of chunksize. */ int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize) { int rc = 0; unsigned int i, j, starthere, lenhere, toread; - unsigned int page_size = flash->chip->page_size; + /* Limit for multi-die 4-byte-addressing chips. */ + unsigned int area_size = min(flash->chip->total_size * 1024, 16 * 1024 * 1024); /* Warning: This loop has a very unusual condition and body. - * The loop needs to go through each page with at least one affected - * byte. The lowest page number is (start / page_size) since that - * division rounds down. The highest page number we want is the page + * The loop needs to go through each area with at least one affected + * byte. The lowest area number is (start / area_size) since that + * division rounds down. The highest area number we want is the area * where the last byte of the range lives. That last byte has the - * address (start + len - 1), thus the highest page number is - * (start + len - 1) / page_size. Since we want to include that last - * page as well, the loop condition uses <=. + * address (start + len - 1), thus the highest area number is + * (start + len - 1) / area_size. Since we want to include that last + * area as well, the loop condition uses <=. */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { - /* Byte position of the first byte in the range in this page. */ + for (i = start / area_size; i <= (start + len - 1) / area_size; i++) { + /* Byte position of the first byte in the range in this area. */ /* starthere is an offset to the base address of the chip. */ - starthere = max(start, i * page_size); - /* Length of bytes in the range in this page. */ - lenhere = min(start + len, (i + 1) * page_size) - starthere; + starthere = max(start, i * area_size); + /* Length of bytes in the range in this area. */ + lenhere = min(start + len, (i + 1) * area_size) - starthere; for (j = 0; j < lenhere; j += chunksize) { toread = min(chunksize, lenhere - j); rc = spi_nbyte_read(flash, starthere + j, buf + starthere - start + j, toread); -- To view, visit https://review.coreboot.org/21813 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I8fdf4621847b1973e5d7344b369d63da5c62e16f Gerrit-Change-Number: 21813 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com>

1 0

Change in flashrom[stable]: udelay: Use clock_gettime() if available and precise
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21812 Change subject: udelay: Use clock_gettime() if available and precise ...................................................................... udelay: Use clock_gettime() if available and precise Instead of calibrating our busy loop against a coarse clock, check if a precise clock is available and loop against that. The former is unre- liable by definition on any modern system that may dynamically reclock the processor. v2: Apparently _POSIX_MONOTONIC_CLOCK being defined only means that the library knows about CLOCK_MONOTONIC. So check for its support at runtime and fall back to CLOCK_REALTIME if it's missing. TEST=Manually added a 10s loop and compared to real time. Run on Linux RPi3, Linux x86 and my original use case Linux in VirtualBox (Linux host). Original-Change-Id: I85ad359823875237ada9cd027af3017d62e9a235 Original-Reviewed-on: https://review.coreboot.org/19391 Original-Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Original-Reviewed-by: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Original-Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Change-Id: I08df071915b282e829214610958e4512f5d71b01 Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M Makefile M udelay.c 2 files changed, 78 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/21812/1 diff --git a/Makefile b/Makefile index 44aa24c..6806e14 100644 --- a/Makefile +++ b/Makefile @@ -1016,6 +1016,9 @@ # We could use PULLED_IN_LIBS, but that would be ugly. FEATURE_LIBS += $(call debug_shell,grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz") +FEATURE_CFLAGS += $(call debug_shell,grep -q "CLOCK_GETTIME := yes" .features && printf "%s" "-D'HAVE_CLOCK_GETTIME=1'") +FEATURE_LIBS += $(call debug_shell,grep -q "CLOCK_GETTIME := yes" .features && printf "%s" "-lrt") + LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS) OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS) @@ -1312,6 +1315,18 @@ endef export LINUX_I2C_TEST +define CLOCK_GETTIME_TEST +#include <time.h> + +int main(int argc, char **argv) +{ + struct timespec res; + clock_gettime(CLOCK_REALTIME, &res); + return 0; +} +endef +export CLOCK_GETTIME_TEST + features: compiler @echo "FEATURES := yes" > .features.tmp ifneq ($(NEED_LIBFTDI), ) @@ -1354,6 +1369,13 @@ @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \ ( echo "found."; echo "UTSNAME := yes" >> .features.tmp ) || \ ( echo "not found."; echo "UTSNAME := no" >> .features.tmp ) } 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) + @printf "Checking for clock_gettime support... " | tee -a $(BUILD_DETAILS_FILE) + @echo "$$CLOCK_GETTIME_TEST" >.featuretest.c + @printf "\nexec: %s\n" "$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -lrt .featuretest.c -o .featuretest$(EXEC_SUFFIX)" >>$(BUILD_DETAILS_FILE) + @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) -lrt .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \ + ( echo "found."; echo "CLOCK_GETTIME := yes" >>.features.tmp ) || \ + ( echo "not found."; echo "CLOCK_GETTIME := no" >>.features.tmp ) } \ + 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features @rm -f .featuretest.c .featuretest$(EXEC_SUFFIX) diff --git a/udelay.c b/udelay.c index 7c6961d..3a44c16 100644 --- a/udelay.c +++ b/udelay.c @@ -21,12 +21,63 @@ #ifndef __LIBPAYLOAD__ +#include <stdbool.h> #include <unistd.h> +#include <errno.h> #include <time.h> #include <sys/time.h> #include <stdlib.h> #include <limits.h> #include "flash.h" + +static bool use_clock_gettime = false; + +#if HAVE_CLOCK_GETTIME == 1 + +#ifdef _POSIX_MONOTONIC_CLOCK +static clockid_t clock_id = CLOCK_MONOTONIC; +#else +static clockid_t clock_id = CLOCK_REALTIME; +#endif + +static void clock_usec_delay(int usecs) +{ + struct timespec now; + clock_gettime(clock_id, &now); + + const long end_nsec = now.tv_nsec + usecs * 1000L; + const struct timespec end = { + end_nsec / (1000 * 1000 * 1000) + now.tv_sec, + end_nsec % (1000 * 1000 * 1000) + }; + do { + clock_gettime(clock_id, &now); + } while (now.tv_sec < end.tv_sec || (now.tv_sec == end.tv_sec && now.tv_nsec < end.tv_nsec)); +} + +static int clock_check_res(void) +{ + struct timespec res; + if (!clock_getres(clock_id, &res)) { + if (res.tv_sec == 0 && res.tv_nsec <= 100) { + msg_pinfo("Using clock_gettime for delay loops (clk_id: %d, resolution: %ldns).\n", + (int)clock_id, res.tv_nsec); + use_clock_gettime = true; + return 1; + } + } else if (clock_id != CLOCK_REALTIME && errno == EINVAL) { + /* Try again with CLOCK_REALTIME. */ + clock_id = CLOCK_REALTIME; + return clock_check_res(); + } + return 0; +} +#else + +static inline void clock_usec_delay(int usecs) {} +static inline int clock_check_res(void) { return 0; } + +#endif /* HAVE_CLOCK_GETTIME == 1 */ /* loops per microsecond */ static unsigned long micro = 1; @@ -87,6 +138,9 @@ void myusec_calibrate_delay(void) { + if (clock_check_res()) + return; + unsigned long count = 1000; unsigned long timeusec, resolution; int i, tries = 0; @@ -189,6 +243,8 @@ /* If the delay is >1 s, use internal_sleep because timing does not need to be so precise. */ if (usecs > 1000000) { internal_sleep(usecs); + } else if (use_clock_gettime) { + clock_usec_delay(usecs); } else { myusec_delay(usecs); } -- To view, visit https://review.coreboot.org/21812 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I08df071915b282e829214610958e4512f5d71b01 Gerrit-Change-Number: 21812 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[stable]: Move register decodes into enable_flash_ich_handle_gcs()
by build bot (Jenkins) (Code Review) 30 Sep '17

30 Sep '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21785 ) Change subject: Move register decodes into enable_flash_ich_handle_gcs() ...................................................................... Patch Set 1: Verified-1 Build Failed https://qa.coreboot.org/job/flashrom-customrules/580/ : FAILURE https://qa.coreboot.org/job/flashrom_gerrit/525/ : SUCCESS -- To view, visit https://review.coreboot.org/21785 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: comment Gerrit-Change-Id: Ia4b77af1b135ec0caee9405684f18f6314715722 Gerrit-Change-Number: 21785 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 30 Sep 2017 20:48:55 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[stable]: ich_descriptors: Pretty print an assumed chipset
by Nico Huber (Code Review) 30 Sep '17

30 Sep '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21811 Change subject: ich_descriptors: Pretty print an assumed chipset ...................................................................... ich_descriptors: Pretty print an assumed chipset Original-Change-Id: Id28cb3abc45c6e7f4c4accfc019579c7448c45d7 Original-Reviewed-on: https://review.coreboot.org/20247 Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Original-Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Original-Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Original-Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Change-Id: I031356e253569712045770bbec2be1d451336894 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M ich_descriptors.c M ich_descriptors.h 2 files changed, 19 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/21811/1 diff --git a/ich_descriptors.c b/ich_descriptors.c index a2aa940..a2f8edf 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -90,6 +90,21 @@ #define getFISBA(cont) (((cont)->FLMAP1 >> 12) & 0x00000ff0) #define getFMSBA(cont) (((cont)->FLMAP2 << 4) & 0x00000ff0) +void prettyprint_ich_chipset(enum ich_chipset cs) +{ + static const char *const chipset_names[] = { + "Unknown ICH", "ICH8", "ICH9", "ICH10", + "5 series Ibex Peak", "6 series Cougar Point", "7 series Panther Point", + "8 series Lynx Point", "Baytrail", "8 series Lynx Point LP", "8 series Wellsburg", + "9 series Wildcat Point", "9 series Wildcat Point LP", "100 series Sunrise Point", + }; + if (cs < CHIPSET_ICH8 || cs - CHIPSET_ICH8 + 1 >= ARRAY_SIZE(chipset_names)) + cs = 0; + else + cs = cs - CHIPSET_ICH8 + 1; + msg_pdbg2("Assuming chipset '%s'.\n", chipset_names[cs]); +} + void prettyprint_ich_descriptors(enum ich_chipset cs, const struct ich_descriptors *desc) { prettyprint_ich_descriptor_content(cs, &desc->content); @@ -881,8 +896,10 @@ desc->component.FLILL = dump[(getFCBA(&desc->content) >> 2) + 1]; desc->component.FLPB = dump[(getFCBA(&desc->content) >> 2) + 2]; - if (*cs == CHIPSET_ICH_UNKNOWN) + if (*cs == CHIPSET_ICH_UNKNOWN) { *cs = guess_ich_chipset(&desc->content, &desc->component); + prettyprint_ich_chipset(*cs); + } /* region */ const ssize_t nr = ich_number_of_regions(*cs, &desc->content); diff --git a/ich_descriptors.h b/ich_descriptors.h index bccea27..8c313d1 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -567,6 +567,7 @@ ssize_t ich_number_of_regions(enum ich_chipset cs, const struct ich_desc_content *content); ssize_t ich_number_of_masters(enum ich_chipset cs, const struct ich_desc_content *content); +void prettyprint_ich_chipset(enum ich_chipset cs); void prettyprint_ich_descriptors(enum ich_chipset cs, const struct ich_descriptors *desc); void prettyprint_ich_descriptor_content(enum ich_chipset cs, const struct ich_desc_content *content); -- To view, visit https://review.coreboot.org/21811 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: stable Gerrit-MessageType: newchange Gerrit-Change-Id: I031356e253569712045770bbec2be1d451336894 Gerrit-Change-Number: 21811 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2017