Nico Huber has uploaded this change for review. ( https://review.coreboot.org/22396
Change subject: spi: Move ICH BBAR quirk out of the way ......................................................................
spi: Move ICH BBAR quirk out of the way
Get rid of the layering violations around ICH's BBAR. Move all the weird address handling into (surprise, surprise) `ichspi.c`. Might fix writes for the `BBAR != 0` case by accident.
Drop the weird alignment fixup in spi_chip_read(). It looks broken (always sets `addrbase` below the valid base address) and there is no indication in the CL that it was ever tested.
Background: Some ICHs have a BBAR (BIOS Base Address Configuration Register) that, if set, limits the valid address range to [BBAR, 2^24). Current code lifted addresses for REMS, RES and READ operations by BBAR, now we do it for all addresses in ichspi.
Change-Id: Icbac513c5339e8aff624870252133284ef85ab73 Signed-off-by: Nico Huber nico.h@gmx.de --- M flash.h M ichspi.c M programmer.h M spi.c M spi25.c 5 files changed, 18 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/22396/1
diff --git a/flash.h b/flash.h index 4f0ab15..bab4873 100644 --- a/flash.h +++ b/flash.h @@ -371,7 +371,6 @@ #define NULL_SPI_CMD { 0, 0, NULL, NULL, } int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds); -uint32_t spi_get_valid_read_addr(struct flashctx *flash);
enum chipbustype get_buses_supported(void); #endif /* !__FLASH_H__ */ diff --git a/ichspi.c b/ichspi.c index 859d55f..9a4cf5d 100644 --- a/ichspi.c +++ b/ichspi.c @@ -229,7 +229,7 @@ static int ichspi_lock = 0;
static enum ich_chipset ich_generation = CHIPSET_ICH_UNKNOWN; -uint32_t ichspi_bbar = 0; +static uint32_t ichspi_bbar;
static void *ich_spibar = NULL;
@@ -1150,19 +1150,6 @@ return SPI_INVALID_LENGTH; }
- /* if opcode-type requires an address */ - if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS || - opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) { - addr = (writearr[1] << 16) | - (writearr[2] << 8) | (writearr[3] << 0); - if (addr < ichspi_bbar) { - msg_perr("%s: Address 0x%06x below allowed " - "range 0x%06x-0xffffff\n", __func__, - addr, ichspi_bbar); - return SPI_INVALID_ADDRESS; - } - } - /* Translate read/write array/count. * The maximum data length is identical for the maximum read length and * for the maximum write length excluding opcode and address. Opcode and @@ -1181,6 +1168,20 @@ count = readcnt; }
+ /* if opcode-type requires an address */ + if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS || + opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) { + addr = writearr[1] << 16 | writearr[2] << 8 | writearr[3]; + if (count > UINT32_MAX - addr || /* just an integer overflow check */ + ichspi_bbar > UINT32_MAX - addr - count || /* another integer overflow check */ + ichspi_bbar + addr + count > 1 << 24) { /* last address must be in 24-bit space */ + msg_perr("%s: Addresses 0x%06x-0x%06x not in allowed range 0x000000-0x%06x\n", + __func__, addr, addr + count - 1, (1 << 24) - ichspi_bbar - 1); + return SPI_INVALID_ADDRESS; + } + addr += ichspi_bbar; + } + result = run_opcode(flash, *opcode, addr, count, data); if (result) { msg_pdbg("Running OPCODE 0x%02x failed ", opcode->opcode); diff --git a/programmer.h b/programmer.h index a98b713..b390a53 100644 --- a/programmer.h +++ b/programmer.h @@ -661,7 +661,6 @@
/* ichspi.c */ #if CONFIG_INTERNAL == 1 -extern uint32_t ichspi_bbar; int ich_init_spi(void *spibar, enum ich_chipset ich_generation); int via_init_spi(uint32_t mmio_base);
diff --git a/spi.c b/spi.c index 0a4a618..56f1fdf 100644 --- a/spi.c +++ b/spi.c @@ -103,31 +103,7 @@ int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - unsigned int addrbase = 0; - - /* Check if the chip fits between lowest valid and highest possible - * address. Highest possible address with the current SPI implementation - * means 0xffffff, the highest unsigned 24bit number. - */ - addrbase = spi_get_valid_read_addr(flash); - /* Show flash chip size warning if flash chip doesn't support - 4-Bytes Addressing mode and last address excedes 24 bits */ - if (!(flash->chip->feature_bits & FEATURE_4BA_SUPPORT) && - addrbase + flash->chip->total_size * 1024 > (1 << 24)) { - msg_perr("Flash chip size exceeds the allowed access window. "); - msg_perr("Read will probably fail.\n"); - /* Try to get the best alignment subject to constraints. */ - addrbase = (1 << 24) - flash->chip->total_size * 1024; - } - /* Check if alignment is native (at least the largest power of two which - * is a factor of the mapped size of the chip). - */ - if (ffs(flash->chip->total_size * 1024) > (ffs(addrbase) ? : 33)) { - msg_perr("Flash chip is not aligned natively in the allowed " - "access window.\n"); - msg_perr("Read will probably return garbage.\n"); - } - return flash->mst->spi.read(flash, buf, addrbase + start, len); + return flash->mst->spi.read(flash, buf, start, len); }
/* @@ -140,27 +116,6 @@ int spi_chip_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { return flash->mst->spi.write_256(flash, buf, start, len); -} - -/* - * Get the lowest allowed address for read accesses. This often happens to - * be the lowest allowed address for all commands which take an address. - * This is a master limitation. - */ -uint32_t spi_get_valid_read_addr(struct flashctx *flash) -{ - switch (flash->mst->spi.type) { -#if CONFIG_INTERNAL == 1 -#if defined(__i386__) || defined(__x86_64__) - case SPI_CONTROLLER_ICH7: - case SPI_CONTROLLER_ICH9: - /* Return BBAR for ICH chipsets. */ - return ichspi_bbar; -#endif -#endif - default: - return 0; - } }
int spi_aai_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) diff --git a/spi25.c b/spi25.c index df0ebbe..06eea9f 100644 --- a/spi25.c +++ b/spi25.c @@ -49,21 +49,11 @@
static int spi_rems(struct flashctx *flash, unsigned char *readarr) { - unsigned char cmd[JEDEC_REMS_OUTSIZE] = { JEDEC_REMS, 0, 0, 0 }; - uint32_t readaddr; + const unsigned char cmd[JEDEC_REMS_OUTSIZE] = { JEDEC_REMS, 0, 0, 0 }; int ret;
ret = spi_send_command(flash, sizeof(cmd), JEDEC_REMS_INSIZE, cmd, readarr); - if (ret == SPI_INVALID_ADDRESS) { - /* Find the lowest even address allowed for reads. */ - readaddr = (spi_get_valid_read_addr(flash) + 1) & ~1; - cmd[1] = (readaddr >> 16) & 0xff, - cmd[2] = (readaddr >> 8) & 0xff, - cmd[3] = (readaddr >> 0) & 0xff, - ret = spi_send_command(flash, sizeof(cmd), JEDEC_REMS_INSIZE, - cmd, readarr); - } if (ret) return ret; msg_cspew("REMS returned 0x%02x 0x%02x. ", readarr[0], readarr[1]); @@ -72,20 +62,11 @@
static int spi_res(struct flashctx *flash, unsigned char *readarr, int bytes) { - unsigned char cmd[JEDEC_RES_OUTSIZE] = { JEDEC_RES, 0, 0, 0 }; - uint32_t readaddr; + const unsigned char cmd[JEDEC_RES_OUTSIZE] = { JEDEC_RES, 0, 0, 0 }; int ret; int i;
ret = spi_send_command(flash, sizeof(cmd), bytes, cmd, readarr); - if (ret == SPI_INVALID_ADDRESS) { - /* Find the lowest even address allowed for reads. */ - readaddr = (spi_get_valid_read_addr(flash) + 1) & ~1; - cmd[1] = (readaddr >> 16) & 0xff, - cmd[2] = (readaddr >> 8) & 0xff, - cmd[3] = (readaddr >> 0) & 0xff, - ret = spi_send_command(flash, sizeof(cmd), bytes, cmd, readarr); - } if (ret) return ret; msg_cspew("RES returned");
-
Nico Huber (Code Review)