Nico Huber has submitted this change and it was merged. ( 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.
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. Special care has to be taken if the BBAR is not aligned by the flash chip's size. In this case, the lower part of the chip (from BBAR aligned down, up to BBAR) is inacces- sible (this seems to be the original intend behind BBAR) and has to be left out in the address offset calculation.
Change-Id: Icbac513c5339e8aff624870252133284ef85ab73 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/22396 Reviewed-by: David Hendricks david.hendricks@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M flash.h M ichspi.c M programmer.h M spi.c M spi25.c 5 files changed, 29 insertions(+), 85 deletions(-)
Approvals: David Hendricks: Looks good to me, approved build bot (Jenkins): Verified
diff --git a/flash.h b/flash.h index f310e23..a14c302 100644 --- a/flash.h +++ b/flash.h @@ -375,7 +375,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 c7bda92..5fe25f6 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,30 @@ count = readcnt; }
+ /* if opcode-type requires an address */ + if (cmd == JEDEC_REMS || cmd == JEDEC_RES) { + addr = ichspi_bbar; + } else if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS || + opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) { + /* BBAR may cut part of the chip off at the lower end. */ + const uint32_t valid_base = ichspi_bbar & ((flash->chip->total_size * 1024) - 1); + const uint32_t addr_offset = ichspi_bbar - valid_base; + /* Highest address we can program is (2^24 - 1). */ + const uint32_t valid_end = (1 << 24) - addr_offset; + + addr = writearr[1] << 16 | writearr[2] << 8 | writearr[3]; + const uint32_t addr_end = addr + count; + + if (addr < valid_base || + addr_end < addr || /* integer overflow check */ + addr_end > valid_end) { + msg_perr("%s: Addressed region 0x%06x-0x%06x not in allowed range 0x%06x-0x%06x\n", + __func__, addr, addr_end - 1, valid_base, valid_end - 1); + return SPI_INVALID_ADDRESS; + } + addr += addr_offset; + } + 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); }
/* @@ -142,27 +118,6 @@ 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) { return flash->mst->spi.write_aai(flash, buf, start, len); diff --git a/spi25.c b/spi25.c index c49f251..dcc7641 100644 --- a/spi25.c +++ b/spi25.c @@ -49,21 +49,10 @@
static int spi_rems(struct flashctx *flash, unsigned char *readarr) { - unsigned char cmd[JEDEC_REMS_OUTSIZE] = { JEDEC_REMS, 0, 0, 0 }; - uint32_t readaddr; + static const unsigned char cmd[JEDEC_REMS_OUTSIZE] = { JEDEC_REMS, }; 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); - } + 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 +61,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; + static const unsigned char cmd[JEDEC_RES_OUTSIZE] = { JEDEC_RES, }; 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)