Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c 6 files changed, 1,008 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/1
diff --git a/src/soc/amd/common/block/include/amdblocks/fch_spi.h b/src/soc/amd/common/block/include/amdblocks/fch_spi.h new file mode 100644 index 0000000..c3ed793 --- /dev/null +++ b/src/soc/amd/common/block/include/amdblocks/fch_spi.h @@ -0,0 +1,103 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _FCH_SPI_H_ +#define _FCH_SPI_H_ + +#include <stdint.h> +#include <stddef.h> + +#define ROM_PROTECT_RANGE0 0x50 +#define ROM_PROTECT_RANGE1 0x54 +#define ROM_PROTECT_RANGE2 0x58 +#define ROM_PROTECT_RANGE3 0x5c +#define ROM_BASE_MASK 0xfffff000 /* bits 31-12 */ +#define ROM_RANGE_WP BIT(10) +#define ROM_RANGE_RP BIT(9) +#define RANGE_UNIT BIT(8) +#define ROM_PROTECT_SKIP (ROM_PROTECT_RANGE1 - ROM_PROTECT_RANGE0) +#define ROM_PROTECT_LIMIT (ROM_PROTECT_RANGE3 + ROM_PROTECT_SKIP) +#define UPPER_16 0xffff0000 /* bits 31-16 */ +#define GRANULARITY_TEST_4k 0x0000f000 /* bits 15-12 */ + +#define SPI_PAGE_WRITE 0x02 +#define SPI_WRITE_ENABLE 0x06 + +/* SPI MMIO registers */ +#define SPI_CNTRL0 0x00 +#define SPI_ACCESS_MAC_ROM_EN BIT(22) +#define SPI_RESTRICTED_CMD1 0x04 +#define SPI_RESTRICTED_CMD2 0x08 +#define SPI_CNTRL1 0x0c +#define SPI_CMD_CODE 0x45 +#define SPI_CMD_TRIGGER 0x47 + +/* Special SST write commands */ +#define CMD_SST_BP 0x02 /* Byte Program */ +#define CMD_SST_AAI_WP 0xAD /* Auto Address Increment Word Program */ + +#define SST_256 0x4b /* Only SST that programs 256 bytes at once */ + +#define VENDOR_ID_ADESTO 0x1f +#define VENDOR_ID_AMIC 0x37 +#define VENDOR_ID_ATMEL 0x1f +#define VENDOR_ID_EON 0x1c +#define VENDOR_ID_GIGADEVICE 0xc8 +#define VENDOR_ID_MACRONIX 0xc2 +#define VENDOR_ID_SPANSION 0x01 +#define VENDOR_ID_SST 0xbf +#define VENDOR_ID_STMICRO 0x20 +#define VENDOR_ID_STMICRO_FF 0xff +#define VENDOR_ID_WINBOND 0xef + +enum special_spi { + ESPECIAL_SPI_NONE = 0, + ESPECIAL_SPI_SST, +}; + +struct spi_data { + const char *name; + u32 size; + u32 sector_size; + u32 page_size; + u8 status_cmd; + u8 erase_cmd; + u8 write_cmd; + u8 write_enable_cmd; + u8 read_cmd; + u8 read_cmd_len; + enum special_spi special; +}; + +struct spi_detail { + u8 vendor_id; + u8 write_cmd; + u8 write_enable_cmd; +}; + +void fch_spi_init(void); +int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in); +int fch_spi_flash_cmd(u8 cmd, void *response, size_t len); +int fch_spi_flash_cmd_write(const u8 *cmd, size_t cmd_len, const void *data, size_t data_len); +int crop_chunk(unsigned int cmd_len, unsigned int buf_len); +int special_spi_write(uint32_t offset, size_t len, const void *buf); +int fch_spi_wait_cmd_ready(unsigned long timeout); + +static inline int fch_spi_enable_write(void) +{ + return fch_spi_flash_cmd(SPI_WRITE_ENABLE, NULL, 0); +} + +#endif /* _FCH_SPI_H_ */ diff --git a/src/soc/amd/common/block/spi/Kconfig b/src/soc/amd/common/block/spi/Kconfig new file mode 100644 index 0000000..40dcb29 --- /dev/null +++ b/src/soc/amd/common/block/spi/Kconfig @@ -0,0 +1,9 @@ +config SOC_AMD_COMMON_BLOCK_SPI + bool + default n + help + Select this option to add SPI controller functions to the build. + +config SOC_AMD_COMMON_BLOCK_SPI_DEBUG + bool + default n diff --git a/src/soc/amd/common/block/spi/Makefile.inc b/src/soc/amd/common/block/spi/Makefile.inc new file mode 100644 index 0000000..df765d8 --- /dev/null +++ b/src/soc/amd/common/block/spi/Makefile.inc @@ -0,0 +1,24 @@ +ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_SPI),y) + +bootblock-y += fch_spi_ctrl.c +bootblock-y += fch_spi_flash.c +bootblock-y += fch_spi_special.c +romstage-y += fch_spi_ctrl.c +romstage-y += fch_spi_flash.c +romstage-y += fch_spi_special.c +verstage-y += fch_spi_ctrl.c +verstage-y += fch_spi_flash.c +verstage-y += fch_spi_special.c +postcar-y += fch_spi_ctrl.c +postcar-y += fch_spi_flash.c +postcar-y += fch_spi_special.c +ramstage-y += fch_spi_ctrl.c +ramstage-y += fch_spi_flash.c +ramstage-y += fch_spi_special.c +ifeq ($(CONFIG_SPI_FLASH_SMM),y) +smm-y += fch_spi_ctrl.c +smm-y += fch_spi_flash.c +smm-y += fch_spi_special.c +endif + +endif diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c new file mode 100644 index 0000000..ac73e6d --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -0,0 +1,510 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi_flash.h> +#include <soc/southbridge.h> +#include <soc/pci_devs.h> +#include <amdblocks/fch_spi.h> +#include <amdblocks/lpc.h> +#include <drivers/spi/spi_flash_internal.h> +#include <device/pci_ops.h> +#include <timer.h> +#include <lib.h> + +extern const struct spi_flash_ops fch_spi_flash_ops; + +struct spi_data ctrl_spi_data; +static uintptr_t spibar; + +/* + * Most SPI vendors use JEDEC defined page write command of 0x02. If a particular SPI vendor + * uses a write command different from 0x02, add it here. Each entry is {vendor_id, write_cmd, + * write_cmd_enable}. This table is terminated by a triple 0x00. + */ +const struct spi_detail vendor_write[] = { + {VENDOR_ID_SST, CMD_SST_AAI_WP, SPI_WRITE_ENABLE}, + {0, 0, 0}, +}; + +static inline uint8_t spi_read8(uint8_t reg) +{ + return read8((void *)(spibar + reg)); +} + +static inline uint32_t spi_read32(uint8_t reg) +{ + return read32((void *)(spibar + reg)); +} + +static inline void spi_write8(uint8_t reg, uint8_t val) +{ + write8((void *)(spibar + reg), val); +} + +static inline void spi_write32(uint8_t reg, uint32_t val) +{ + write32((void *)(spibar + reg), val); +} + +static void dump_state(const char *str, u8 phase) +{ + u8 dump_size; + u32 addr; + if (!CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) + return; + + printk(BIOS_DEBUG, "SPI: %s\n", str); + printk(BIOS_DEBUG, "Cntrl0: %x\n", spi_read32(SPI_CNTRL0)); + printk(BIOS_DEBUG, "Status: %x\n", spi_read32(SPI_STATUS)); + + addr = spibar + SPI_FIFO; + if (phase == 0) { + dump_size = spi_read8(SPI_TX_BYTE_COUNT); + printk(BIOS_DEBUG, "TxByteCount: %x\n", dump_size); + printk(BIOS_DEBUG, "CmdCode: %x\n", spi_read8(SPI_CMD_CODE)); + } else { + dump_size = spi_read8(SPI_RX_BYTE_COUNT); + printk(BIOS_DEBUG, "RxByteCount: %x\n", dump_size); + addr += spi_read8(SPI_TX_BYTE_COUNT); + } + + if (dump_size > 0) + hexdump((void *)addr, dump_size); +} + +static int wait_for_ready(void) +{ + const uint32_t timeout_ms = 500; + struct stopwatch sw; + + stopwatch_init_msecs_expire(&sw, timeout_ms); + + do { + if (!(spi_read32(SPI_STATUS) & SPI_BUSY)) + return 0; + } while (!stopwatch_expired(&sw)); + + return -1; +} + +static int execute_command(void) +{ + dump_state("Before Execute", 0); + + spi_write8(SPI_CMD_TRIGGER, SPI_CMD_TRIGGER_EXECUTE); + + if (wait_for_ready()) + printk(BIOS_DEBUG, + "FCH_SC Error: Timeout executing command\n"); + + dump_state("Transaction finished", 1); + + return 0; +} + +void spi_init(void) +{ + spibar = lpc_get_spibase(); + printk(BIOS_DEBUG, "%s: Spibar at 0x%08x\n", __func__, (u32)spibar); +} + +static int spi_ctrlr_xfer(const void *dout, size_t bytesout, void *din, size_t bytesin) +{ + size_t count; + uint8_t cmd; + uint8_t *bufin = din; + const uint8_t *bufout = dout; + + if (CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) + printk(BIOS_DEBUG, "%s(%zx, %zx)\n", __func__, bytesout, + bytesin); + + /* First byte is cmd which cannot be sent through FIFO */ + cmd = bufout[0]; + bufout++; + bytesout--; + + /* + * Check if this is a write command attempting to transfer more bytes + * than the controller can handle. Iterations for writes are not + * supported here because each SPI write command needs to be preceded + * and followed by other SPI commands, and this sequence is controlled + * by the SPI chip driver. + */ + if (bytesout + bytesin > SPI_FIFO_DEPTH) { + printk(BIOS_DEBUG, "FCH_SC: Too much to transfer, code error!\n"); + return -1; + } + + if (wait_for_ready()) + return -1; + + spi_write8(SPI_CMD_CODE, cmd); + spi_write8(SPI_TX_BYTE_COUNT, bytesout); + spi_write8(SPI_RX_BYTE_COUNT, bytesin); + + for (count = 0; count < bytesout; count++) + spi_write8(SPI_FIFO + count, bufout[count]); + + if (execute_command()) + return -1; + + for (count = 0; count < bytesin; count++) + bufin[count] = spi_read8(SPI_FIFO + count + bytesout); + + return 0; +} + +static int amd_xfer_vectors( struct spi_op vectors[], size_t count) +{ + int ret; + void *din; + size_t bytes_in; + + if (count < 1 || count > 2) + return -1; + + /* SPI flash commands always have a command first... */ + if (!vectors[0].dout || !vectors[0].bytesout) + return -1; + /* And not read any data during the command. */ + if (vectors[0].din || vectors[0].bytesin) + return -1; + + if (count == 2) { + /* If response bytes requested ensure the buffer is valid. */ + if (vectors[1].bytesin && !vectors[1].din) + return -1; + /* No sends can accompany a receive. */ + if (vectors[1].dout || vectors[1].bytesout) + return -1; + din = vectors[1].din; + bytes_in = vectors[1].bytesin; + } else { + din = NULL; + bytes_in = 0; + } + + ret = spi_ctrlr_xfer(vectors[0].dout, vectors[0].bytesout, din, bytes_in); + + if (ret) { + vectors[0].status = SPI_OP_FAILURE; + if (count == 2) + vectors[1].status = SPI_OP_FAILURE; + } else { + vectors[0].status = SPI_OP_SUCCESS; + if (count == 2) + vectors[1].status = SPI_OP_SUCCESS; + } + + return ret; +} + +int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in) +{ + /* + * SPI flash requires command-response kind of behavior. Thus, two + * separate SPI vectors are required -- first to transmit dout and other + * to receive in din. If some specialized SPI flash controllers + * (e.g. x86) can perform both command and response together, it should + * be handled at SPI flash controller driver level. + */ + struct spi_op vectors[] = { + [0] = { .dout = dout, .bytesout = bytes_out, + .din = NULL, .bytesin = 0, }, + [1] = { .dout = NULL, .bytesout = 0, + .din = din, .bytesin = bytes_in }, + }; + size_t count = ARRAY_SIZE(vectors); + if (!bytes_in) + count = 1; + + return amd_xfer_vectors(vectors, count); +} + +int fch_spi_flash_cmd(u8 cmd, void *response, size_t len) +{ + int ret = do_fch_spi_flash_cmd(&cmd, sizeof(cmd), response, len); + if (ret) + printk(BIOS_WARNING, "FCH_SC: Failed to send command %02x: %d\n", cmd, ret); + + return ret; +} + +/* + * The following table holds all device probe functions + * + * shift: number of continuation bytes before the ID + * idcode: the expected IDCODE or 0xff for non JEDEC devices + * probe: the function to call + * + * Non JEDEC devices should be ordered in the table such that + * the probe functions with best detection algorithms come first. + * + * Several matching entries are permitted, they will be tried + * in sequence until a probe function returns non NULL. + * + * IDCODE_CONT_LEN may be redefined if a device needs to declare a + * larger "shift" value. IDCODE_PART_LEN generally shouldn't be + * changed. This is the max number of bytes probe functions may + * examine when looking up part-specific identification info. + * + * Probe functions will be given the idcode buffer starting at their + * manu id byte (the "idcode" in the table below). In other words, + * all of the continuation bytes will be skipped (the "shift" below). + */ +#define IDCODE_CONT_LEN 0 +#define IDCODE_PART_LEN 5 +static struct { + const u8 shift; + const u8 idcode; + int (*probe) (const struct spi_slave *spi, u8 *idcode, + struct spi_flash *flash); +} flashes[] = { + /* Keep it sorted by define name */ +#if CONFIG(SPI_FLASH_ADESTO) + { 0, VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, +#endif +#if CONFIG(SPI_FLASH_AMIC) + { 0, VENDOR_ID_AMIC, spi_flash_probe_amic, }, +#endif +#if CONFIG(SPI_FLASH_ATMEL) + { 0, VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, +#endif +#if CONFIG(SPI_FLASH_EON) + { 0, VENDOR_ID_EON, spi_flash_probe_eon, }, +#endif +#if CONFIG(SPI_FLASH_GIGADEVICE) + { 0, VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, +#endif +#if CONFIG(SPI_FLASH_MACRONIX) + { 0, VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, +#endif +#if CONFIG(SPI_FLASH_SPANSION) + { 0, VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, +#endif +#if CONFIG(SPI_FLASH_SST) + { 0, VENDOR_ID_SST, spi_flash_probe_sst, }, +#endif +#if CONFIG(SPI_FLASH_STMICRO) + { 0, VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, + { 0, VENDOR_ID_STMICRO_FF, spi_flash_probe_stmicro, }, +#endif +#if CONFIG(SPI_FLASH_WINBOND) + { 0, VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, +#endif + /* Keep it sorted by best detection */ +}; +#define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN) + +static void save_flash_data(struct spi_flash *flash) +{ + int i; + u8 vendor, write_enable = SPI_WRITE_ENABLE, cmd = SPI_PAGE_WRITE; + + ctrl_spi_data.name = flash->name; + ctrl_spi_data.size = flash->size; + ctrl_spi_data.sector_size = flash->sector_size; + ctrl_spi_data.page_size = flash->page_size; + ctrl_spi_data.status_cmd = flash->status_cmd; + ctrl_spi_data.erase_cmd = flash->erase_cmd; + for (i = 0; i < (int)ARRAY_SIZE(vendor_write); i++) { + vendor = vendor_write[i].vendor_id; + if (vendor == flash->vendor) { + cmd = vendor_write[i].write_cmd; + write_enable = vendor_write[i].write_enable_cmd; + break; + } + if (!vendor & !cmd) { + cmd = SPI_PAGE_WRITE; + write_enable = SPI_WRITE_ENABLE; + break; + } + } + ctrl_spi_data.write_cmd = cmd; + ctrl_spi_data.write_enable_cmd = write_enable; + if (CONFIG(SPI_FLASH_NO_FAST_READ)) { + ctrl_spi_data.read_cmd_len = 4; + ctrl_spi_data.read_cmd = CMD_READ_ARRAY_SLOW; + } else { + ctrl_spi_data.read_cmd_len = 5; + ctrl_spi_data.read_cmd = CMD_READ_ARRAY_FAST; + } +} + +static int fch_spi_flash_probe(const struct spi_slave *spi, struct spi_flash *flash) +{ + int ret, i, shift; + u8 idcode[IDCODE_LEN], *idp; + + /* Read the ID codes */ + ret = fch_spi_flash_cmd(CMD_READ_ID, idcode, sizeof(idcode)); + if (ret) + return -1; + + if (CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) { + printk(BIOS_SPEW, "SF: Got idcode: "); + for (i = 0; i < sizeof(idcode); i++) + printk(BIOS_SPEW, "%02x ", idcode[i]); + printk(BIOS_SPEW, "\n"); + } + + /* count the number of continuation bytes */ + for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; + ++shift, ++idp) + continue; + + printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + + /* search the table for matches in shift and id */ + for (i = 0; i < (int)ARRAY_SIZE(flashes); ++i) + if (flashes[i].shift == shift && flashes[i].idcode == *idp) { + /* we have a match, call probe */ + if (flashes[i].probe(spi, idp, flash) == 0) { + flash->vendor = idp[0]; + flash->model = (idp[1] << 8) | idp[2]; + save_flash_data(flash); + flash->ops = &fch_spi_flash_ops; + if (idp[0] == VENDOR_ID_SST) { + ctrl_spi_data.special = ESPECIAL_SPI_SST; + if (idp[2] == SST_256) { + ctrl_spi_data.page_size = 256; + ctrl_spi_data.write_cmd = SPI_PAGE_WRITE; + } else + ctrl_spi_data.page_size = 2; + } else + ctrl_spi_data.special = ESPECIAL_SPI_NONE; + return 0; + } + } + + /* No match, return error. */ + return -1; +} + +static int protect_a_range(u32 value) +{ + u32 reg32; + u8 reg; + /* find a free protection register */ + for (reg = ROM_PROTECT_RANGE0; reg < ROM_PROTECT_LIMIT; reg += ROM_PROTECT_SKIP) { + reg32 = pci_read_config32(SOC_LPC_DEV, reg); + if ((reg32 & ROM_BASE_MASK) == 0) + break; + } + if (reg == ROM_PROTECT_LIMIT) + return -1; /* no free range */ + + pci_write_config32(SOC_LPC_DEV, reg, value); + return 0; +} + +/* + * Protect range of SPI flash defined by region using the SPI flash controller. + * + * Note: Up to 4 ranges can be protected, though if a particular region requires more than one + * range, total number of regions decreases accordingly. Each range can be programmed to 4KiB or + * 64KiB granularity. + * + * Know issue: If more than 1 region needs protection, and they need mixed protections (read/ + * write) than start with the region that requires the most protection. After the restricted + * commands have been written, they can't be changed (write once). So if first region is write + * protection and second region is read protection, it's best to define first region as read and + * write protection. + */ +static int fch_spi_flash_protect(const struct spi_flash *flash, const struct region *region, + const enum ctrlr_prot_type type) +{ + int ret; + u32 reg32, rom_base, range_base; + size_t addr, len, gran_value, total_ranges, range; + u8 granularity = 1; /* assume 64k granularity */ + + addr = region->offset; + len = region->size; + + reg32 = pci_read_config32(SOC_LPC_DEV, ROM_ADDRESS_RANGE2_START); + rom_base = (reg32 << 16) & UPPER_16; + range_base = addr % rom_base; + + /* Define granularity to be used */ + if (GRANULARITY_TEST_4k & range_base) + granularity = 0; /* use 4K granularity */ + if (GRANULARITY_TEST_4k & len) + granularity = 0; /* use 4K granularity */ + + /* Define the first range and total number of ranges required */ + if (granularity) { + gran_value = 0x00010000; /* 64 KiB */ + range_base = range_base >> 16; + } else { + gran_value = 0x00001000; /* 4 KiB */ + range_base = range_base >> 12; + } + total_ranges = len / gran_value; + range_base &= 0x000000ff; /* Bits 7-0 */ + + /* Create the value (reg32) to be programed and program required ranges */ + reg32 = rom_base & ROM_BASE_MASK; + reg32 |= range_base; + if (granularity) + reg32 |= RANGE_UNIT; + if (type & WRITE_PROTECT) + reg32 |= ROM_RANGE_WP; + if (type & READ_PROTECT) + reg32 |= ROM_RANGE_RP; + + for (range = 0; range < total_ranges; range++) { + ret = protect_a_range(reg32); + if (ret) + return ret; + reg32++; /* next range */ + } + + /* define commands to be blocked if in range */ + reg32 = 0; + if (type & WRITE_PROTECT) { + reg32 |= (ctrl_spi_data.write_enable_cmd << 24); + reg32 |= (ctrl_spi_data.write_cmd << 16); + reg32 |= (ctrl_spi_data.erase_cmd << 8); + } + if (type & READ_PROTECT) + reg32 |= ctrl_spi_data.read_cmd; + + /* Final steps to protect region */ + pci_write_config32(SOC_LPC_DEV, SPI_RESTRICTED_CMD1, reg32); + reg32 = pci_read_config32(SOC_LPC_DEV, SPI_CNTRL0); + reg32 &= ~SPI_ACCESS_MAC_ROM_EN; + pci_write_config32(SOC_LPC_DEV, SPI_CNTRL0, reg32); + + return 0; +} + +const struct spi_ctrlr fch_spi_flash_ctrlr = { + .max_xfer_size = SPI_FIFO_DEPTH, + .flash_probe = fch_spi_flash_probe, + .flash_protect = fch_spi_flash_protect, +}; + +const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = { + { + .ctrlr = &fch_spi_flash_ctrlr, + .bus_start = 0, + .bus_end = 0, + }, +}; + +const size_t spi_ctrlr_bus_map_count = ARRAY_SIZE(spi_ctrlr_bus_map); diff --git a/src/soc/amd/common/block/spi/fch_spi_flash.c b/src/soc/amd/common/block/spi/fch_spi_flash.c new file mode 100644 index 0000000..c2ff6ea --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_flash.c @@ -0,0 +1,236 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi_flash.h> +#include <spi-generic.h> +#include <soc/southbridge.h> +#include <amdblocks/fch_spi.h> +#include <drivers/spi/spi_flash_internal.h> +#include <timer.h> +#include <string.h> + +extern struct spi_data ctrl_spi_data; + +static void spi_flash_addr(u32 addr, u8 *cmd) +{ + /* cmd[0] is actual command */ + cmd[1] = addr >> 16; + cmd[2] = addr >> 8; + cmd[3] = addr >> 0; +} + +int crop_chunk(unsigned int cmd_len, unsigned int buf_len) +{ + unsigned int ctrlr_max = SPI_FIFO_DEPTH; + + /* + * Assume opcode is always one byte and deduct it from the cmd_len as the hardware has + * a separate register for the opcode. + */ + cmd_len--; + ctrlr_max -= cmd_len; + return min(ctrlr_max, buf_len); +} + +int fch_spi_flash_cmd_write(const u8 *cmd, size_t cmd_len, const void *data, size_t data_len) +{ + int ret; + u8 buff[SPI_FIFO_DEPTH + 1]; + memcpy(buff, cmd, cmd_len); + memcpy(buff + cmd_len, data, data_len); + + ret = do_fch_spi_flash_cmd(buff, cmd_len + data_len, NULL, 0); + if (ret) { + printk(BIOS_WARNING, "FCH_SF: Failed to send write command (%zu bytes): %d\n", + data_len, ret); + } + + return ret; +} + +static int fch_spi_flash_status(const struct spi_flash *flash, uint8_t *reg) +{ + int ret; + u8 status, cmd = CMD_READ_STATUS; + + ret = do_fch_spi_flash_cmd(&cmd, 1, &status, 1); + if (!ret) + *reg = status; + return ret; +} + +int fch_spi_wait_cmd_ready(unsigned long timeout) +{ + struct mono_time current, end; + int ret; + u8 status; + + timer_monotonic_get(¤t); + end = current; + mono_time_add_msecs(&end, timeout); + + do { + ret = fch_spi_flash_status(NULL, &status); + if (ret) + return -1; + if ((status & STATUS_WIP) == 0) + return 0; + timer_monotonic_get(¤t); + } while (!mono_time_after(¤t, &end)); + + printk(BIOS_DEBUG, "FCH_SF: timeout at %ld msec\n", timeout); + return -1; +} + +static int fch_spi_flash_erase(const struct spi_flash *flash, uint32_t offset, size_t len) +{ + u32 start, end, erase_size; + int ret = -1; + u8 cmd[4]; + + erase_size = ctrl_spi_data.sector_size; + if (offset % erase_size || len % erase_size) { + printk(BIOS_WARNING, "%s: Erase offset/length not multiple of erase size\n", + ctrl_spi_data.name); + return -1; + } + if (len == 0) { + printk(BIOS_WARNING, "%s: Erase length cannot be 0\n", ctrl_spi_data.name); + return -1; + } + + cmd[0] = ctrl_spi_data.erase_cmd; + start = offset; + end = start + len; + + while (offset < end) { + spi_flash_addr(offset, cmd); + offset += erase_size; + +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "FCH_SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1], + cmd[2], cmd[3], offset); +#endif + ret = fch_spi_enable_write(); + if (ret) + goto out; + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), NULL, 0); + if (ret) + goto out; + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PAGE_ERASE_TIMEOUT_MS); + if (ret) + goto out; + } + + printk(BIOS_DEBUG, "%s: Successfully erased %zu bytes @ %#x\n", ctrl_spi_data.name, len, + start); + +out: + return ret; +} + +static int fch_spi_flash_read(const struct spi_flash *flash, uint32_t offset, size_t len, + void *buf) +{ + uint8_t *data = buf; + int ret; + size_t xfer_len; + u8 cmd[5]; + + cmd[0] = ctrl_spi_data.read_cmd; + cmd[4] = 0; + while (len) { + xfer_len = crop_chunk(ctrl_spi_data.read_cmd_len, len); + spi_flash_addr(offset, cmd); + ret = do_fch_spi_flash_cmd(cmd, ctrl_spi_data.read_cmd_len, data, xfer_len); + if (ret) { + printk(BIOS_WARNING, + "FCH_SF: Failed to send read command %#.2x(%#x, %#zx): %d\n", + cmd[0], offset, xfer_len, ret); + return ret; + } + offset += xfer_len; + data += xfer_len; + len -= xfer_len; + } + return 0; +} + +static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len, + const void *buf) +{ + unsigned long byte_addr; + unsigned long page_size; + size_t chunk_len; + size_t actual; + int ret = 0; + u8 cmd[4]; + + if (ctrl_spi_data.special != ESPECIAL_SPI_NONE) + return special_spi_write(offset, len, buf); + page_size = ctrl_spi_data.page_size; + + for (actual = 0; actual < len; actual += chunk_len) { + byte_addr = offset % page_size; + chunk_len = min(len - actual, page_size - byte_addr); + chunk_len = crop_chunk(sizeof(cmd), chunk_len); + + cmd[0] = ctrl_spi_data.write_cmd; + cmd[1] = (offset >> 16) & 0xff; + cmd[2] = (offset >> 8) & 0xff; + cmd[3] = offset & 0xff; +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu" + "\n", buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); +#endif + + ret = fch_spi_enable_write(); + if (ret < 0) { + printk(BIOS_WARNING, "%s: Enabling Write failed\n", ctrl_spi_data.name); + goto out; + } + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), buf + actual, chunk_len); + if (ret < 0) { + printk(BIOS_WARNING, "%s: Page Program failed\n", ctrl_spi_data.name); + goto out; + } + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); + if (ret) + goto out; + + offset += chunk_len; + } + +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "%s: Successfully programmed %zu bytes @ 0x%lx\n", + ctrl_spi_data.name, len, (unsigned long)(offset - len)); +#endif + ret = 0; + +out: + return ret; +} + +const struct spi_flash_ops fch_spi_flash_ops = { + .read = fch_spi_flash_read, + .write = fch_spi_flash_write, + .erase = fch_spi_flash_erase, + .status = fch_spi_flash_status, +}; diff --git a/src/soc/amd/common/block/spi/fch_spi_special.c b/src/soc/amd/common/block/spi/fch_spi_special.c new file mode 100644 index 0000000..21d91a0 --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_special.c @@ -0,0 +1,126 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi-generic.h> +#include <amdblocks/fch_spi.h> +#include <string.h> + +extern struct spi_data ctrl_spi_data; + +static int sst_byte_write(u32 offset, const void *buf) +{ + int ret; + u8 cmd[4] = { + CMD_SST_BP, + offset >> 16, + offset >> 8, + offset, + }; + + ret = fch_spi_enable_write(); + if (ret) + return ret; + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), buf, 1); + if (ret) + return ret; + + return fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); +} + +static int special_sst_write_aai(u32 offset, size_t len, const void *buf) +{ + size_t actual, chunk_len, cmd_len; + unsigned long byte_addr; + unsigned long page_size; + int ret = 0; + u8 cmd[4]; + + page_size = ctrl_spi_data.page_size; + + /* If the data is not word aligned, write out leading single byte */ + actual = offset % 2; + if (actual) { + ret = sst_byte_write(offset, buf); + len--; + if (ret) + goto done; + } + offset += actual; + + ret = fch_spi_enable_write(); + if (ret) + goto done; + + cmd[0] = ctrl_spi_data.write_cmd; + cmd_len = sizeof(cmd); + + for (actual = 0; actual < len; actual += chunk_len) { + byte_addr = offset % page_size; + chunk_len = min(len - actual, page_size - byte_addr); + chunk_len = crop_chunk(cmd_len, chunk_len); +/* + * If page_size is 2 (most SST SPI), this section of code is useful only on the first pass, + * as subsequent passes will send only the AAI command without the offset. However, this + * implementation allows for combining 2 implementations into a single one and does not consume + * too much time compared to the actual physical write within the SPI. + */ + cmd[1] = (offset >> 16) & 0xff; + cmd[2] = (offset >> 8) & 0xff; + cmd[3] = offset & 0xff; + +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x }" + " chunk_len = %zu\n", + buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); +#endif + + ret = fch_spi_enable_write(); + if (ret < 0) { + printk(BIOS_WARNING, "SF: Enabling Write failed\n"); + break; + } + + ret = fch_spi_flash_cmd_write(cmd, cmd_len, buf + actual, chunk_len); + if (ret < 0) { + printk(BIOS_WARNING, "SF: SST Page Program failed\n"); + break; + } + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); + if (ret) + break; + + offset += chunk_len; + if (page_size == 2) + cmd_len = 1; + } +done: + return ret; +} + +int special_spi_write(uint32_t offset, size_t len, const void *buf) +{ + int ret = 0; + switch (ctrl_spi_data.special) { + case ESPECIAL_SPI_SST: + ret = special_sst_write_aai(offset, len, buf); + break; + default: + break; + } + return ret; +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... PS1, Line 171: static int amd_xfer_vectors( struct spi_op vectors[], size_t count) space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... PS1, Line 274: int (*probe) (const struct spi_slave *spi, u8 *idcode, Unnecessary space before function pointer arguments
Hello Marshall Dawson, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#2).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c 6 files changed, 1,008 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/2
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... PS1, Line 171: static int amd_xfer_vectors( struct spi_op vectors[], size_t count)
space prohibited after that open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/35018/1/src/soc/amd/common/block/sp... PS1, Line 274: int (*probe) (const struct spi_slave *spi, u8 *idcode,
Unnecessary space before function pointer arguments
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@10 PS2, Line 10: controller within the FCH to its fullest. What is currently not supported?
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@11 PS2, Line 11: Please add the datasheet name and revision.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 5: Select this option to add SPI controller functions to the build. Can you be more specific? Without it generic SPI controller functions would be used, or none at all?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@10 PS2, Line 10: controller within the FCH to its fullest.
What is currently not supported?
Write protection using SPI controller functionality is not supported using current SPI code, but is supported by this patch. Consequently currently SPI write protection works only with some SPI that do implement it using SPI in chip write protection.
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@11 PS2, Line 11:
Please add the datasheet name and revision.
Only datasheet used were Family 15h BKDG revision 3.06 (public) and Family 17h (NDA). The SPI controller is identical in both, except for the size of the FIFO (which is declared in SOC specific southbridge.h). I'll add only the Family 15h which is already public.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 5: Select this option to add SPI controller functions to the build.
Can you be more specific? Without it generic SPI controller functions would be used, or none at all?
From what I understood on Martin's request, the idea is to eventually abandon device/spi/spi_flash.c as much as possible, as Intel currently overrides it with fast_spi. He asked me to specifically override flash_ops structure, just as fast_spi does. So yes, I could be more specific, but when config SPI_FLASH becomes deprecated I would need to remove the extra help.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 2:
(18 comments)
This isn't a 100% review.
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@11 PS2, Line 11:
Only datasheet used were Family 15h BKDG revision 3.06 (public) and Family 17h (NDA). […]
Please update the commit message per Paul's request. The gerrit comment means nothing.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 21: Add a comment that these registers are in D14F3. OTOH why not add them to lpc.h?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 30: #define ROM_PROTECT_SKIP (ROM_PROTECT_RANGE1 - ROM_PROTECT_RANGE0) : #define ROM_PROTECT_LIMIT (ROM_PROTECT_RANGE3 + ROM_PROTECT_SKIP) This really makes the c confusing. Why not just make a macro for ROM_PROTECT_RANGE_REG(n) and a #define for MAX_ROM_PROTECT_RANGES.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 402: /* fin add a blank line
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 405: reg32 & ROM_BASE_MASK) ALIGN_DOWN(reg32, 4 * KiB)
Arguably, you could simply check if (!reg32).
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 422: Know "Known"
Is this really an issue? That makes it sounds like a chipset bug. Why not just state that the registers are write-once, and therefore the implementation naturally follows.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 434: granularity This looks like it's written to essentially be a boolean. If you leave it non-bool, how about renaming it so it's obvious what a value of 1 is, e.g. gran64.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 439: ROM_ADDRESS_RANGE2_START Are you assuming this will contain the base address of the flash device? I don't think you should count on the register having been programmed. Why not simply use CONFIG_COREBOOT_ROMSIZE_KB and calculate it?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: 16 Would prefer to see this as a #define
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: & UPPER_16 can't you skip this?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 441: % Is it possible for a caller to request a region below rom_base? Maybe give it a quick check and return an error if the region is too low.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 444: GRANULARITY_TEST_4k & range_base You're anding range_base and len with 0xf000; won't you get false positives it either of those values are >= 64K? How about comparing (range_base != ALIGN_DOWN(range_base, 4 * KiB).
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 458: /* Bits 7-0 */ I don't think this comment adds much for the reader. I would much rather know _why_ we're anding; I assume because the max is 16MB. FWIW, if you check for legal regions, this could probably go away.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 460: programed sp, but BTW I would reword the whole comment so you don't use two forms of "program". Also, is it necessary to point out that reg32 is the value?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */ Are you sure? Because it looks wrong. Don't we need a completely different reg32? A better comment might help.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 477: /* define commands to be blocked if in range */ Are your certain about this? With no comments, it looks like you're precluding the commands to 100% of the flash if WP or RP, regardless of what the range was. The documentation looks like this blocks the MAC from issuing the commands, which doesn't seem to be quite what you want. (While we'd probably want to ensure the MAC can't write a region we're protecting, I believe this only protects the top 512K what AMD calls "BIOS space".)
Once you write the D14F3x[5c,58,54,50] registers, the SPI controller should ignore the commands from the x86 already.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 488: SPI_RESTRICTED_CMD1 Isn't this an MMIO register?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 489: SPI_CNTRL0 MMIO
Hello Marshall Dawson, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#3).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c 7 files changed, 1,008 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/3
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 3:
(18 comments)
g
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35018/2//COMMIT_MSG@11 PS2, Line 11:
Please update the commit message per Paul's request. The gerrit comment means nothing.
Done, but only the Family 15h public version. Only reason it has not been out was that I was waiting for more reviews for a single patch.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 21:
Add a comment that these registers are in D14F3. OTOH why not add them to lpc. […]
I didn't want to change any file outside what I included. Moved to lpc.h.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 30: #define ROM_PROTECT_SKIP (ROM_PROTECT_RANGE1 - ROM_PROTECT_RANGE0) : #define ROM_PROTECT_LIMIT (ROM_PROTECT_RANGE3 + ROM_PROTECT_SKIP)
This really makes the c confusing. […]
All I really need is the limit, though your suggestion has merit if I also change the code using it. Done.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 402: /* fin
add a blank line
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 405: reg32 & ROM_BASE_MASK)
ALIGN_DOWN(reg32, 4 * KiB) […]
ALIGN_DOWN(reg32, 4 * KiB)? Instead of the single AND operation? I don't like it. Simply (!reg32). I need to double check, but if reserved bits read as 0 (I have found reserved bit set to 1 at reset before) you are correct.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 422: Know
"Known" […]
It's the restricted commands that are the problem, once written without a read command, a read command can't be added... and no, it's not a bug, just a warning. Changed from "Known issue" do "Warning". The one problem is that I don't know if adding the read command as a restricted command will cause problem if the read protect is not enabled for the range. If that is true, than I could simply add all commands and be done with it, without this warning.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 434: granularity
This looks like it's written to essentially be a boolean. […]
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 439: ROM_ADDRESS_RANGE2_START
Are you assuming this will contain the base address of the flash device? I don't think you should c […]
It's definitely programmed with grunt. And if not programmed and I use the calculation as you define, protection would fall on the wrong place due to mismatch between registers.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: 16
Would prefer to see this as a #define
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 440: & UPPER_16
can't you skip this?
Playing safe.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 441: %
Is it possible for a caller to request a region below rom_base? Maybe give it a quick check and ret […]
Good idea, done.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 444: GRANULARITY_TEST_4k & range_base
You're anding range_base and len with 0xf000; won't you get false positives it either of those value […]
No false positives. Even if address or size was greater than 64K, if any of bits 15-12 is set than it has to be 4K granularity. The worst that could happen would be if for example, addr = ((n * 64K) - 4K) and size 68K. This could be solved with a 4K range and a 64K range... but my code is not smart enough to realize it. It would do the first as 4K (as it should), but the remaining 64K would be broken into 4K chunks and there would be not enough regions. OTOH, if any bit 11 through 0 is set, that's an error I'm not checking and results would be unpredictable.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 458: /* Bits 7-0 */
I don't think this comment adds much for the reader. […]
This is needed, it's a mask. Declared the mask value and used it here.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 460: programed
sp, but BTW I would reword the whole comment so you don't use two forms of "program". […]
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */
Are you sure? Because it looks wrong. […]
Completely sure. I'm increasing the range address (lower 8 bits) to point to next range (4K or 64K depending on granularity).
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 477: /* define commands to be blocked if in range */
Are your certain about this? With no comments, it looks like you're precluding the commands to 100% […]
It's all needed. From BKDG: Step 1 enables protection for direct memory access to SPI (Direct memory access is when Host access SPI memory using normal Read / Write commands these access are converted to SPI Read and Write commands by the SPI controller). Step 2 enables protection for indirect memory access to the SPI controller. Indirect memory accesses are when the SPIx00 [SPI_Cntrl0] register is used to send SPI commands to the SPI ROM. Step 3 By programming the SPIx00[SpiAccessMacRomEn] = 0, all SPI commands (defined in register SPIx14 [SPI_CmdValue1] and SPIx18 [SPI_CmdValue2]) and Restricted commands (defined in SPIx04 [SPI_RestrictedCmd] and SPIx08 [SPI_RestrictedCmd2]) are protected.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 488: SPI_RESTRICTED_CMD1
Isn't this an MMIO register?
Correct, fixed.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 489: SPI_CNTRL0
MMIO
Correct, fixed.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 3:
Ping... review please.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 3:
(6 comments)
Sorry, I didn't finish my review. Here's what I had.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 53: #define VENDOR_ID_ADESTO 0x1f : #define VENDOR_ID_AMIC 0x37 : #define VENDOR_ID_ATMEL 0x1f : #define VENDOR_ID_EON 0x1c : #define VENDOR_ID_GIGADEVICE 0xc8 : #define VENDOR_ID_MACRONIX 0xc2 : #define VENDOR_ID_SPANSION 0x01 : #define VENDOR_ID_SST 0xbf : #define VENDOR_ID_STMICRO 0x20 : #define VENDOR_ID_STMICRO_FF 0xff : #define VENDOR_ID_WINBOND 0xef This has nothing to do with AMD, so should probably be moved to a common header.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 65: special_spi non-standard?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 94: crop_chunk stick with spi_crop_chunk?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 110: BIOS_DEBUG Maybe BIOS_ERR?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 147: bytesout + bytesin We really need to fit both bytes in and out into the fifo at the same time?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 148: BIOS_DEBUG Again, maybe a higher level here than debug?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 53: #define VENDOR_ID_ADESTO 0x1f : #define VENDOR_ID_AMIC 0x37 : #define VENDOR_ID_ATMEL 0x1f : #define VENDOR_ID_EON 0x1c : #define VENDOR_ID_GIGADEVICE 0xc8 : #define VENDOR_ID_MACRONIX 0xc2 : #define VENDOR_ID_SPANSION 0x01 : #define VENDOR_ID_SST 0xbf : #define VENDOR_ID_STMICRO 0x20 : #define VENDOR_ID_STMICRO_FF 0xff : #define VENDOR_ID_WINBOND 0xef
This has nothing to do with AMD, so should probably be moved to a common header.
Common header would need to be spi-generic.h, so it will be a separate patch.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 65: special_spi
non-standard?
ok, valid. Done.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 94: crop_chunk
stick with spi_crop_chunk?
Would conflict with same function at drivers/spi/spi_flash.c as both are declared in headers, and both headers (spi_flash.h and fch_spi.h are used by all code files within common/amdblocks/spi. So it must have a different name. If you want spi as part of the name, it must have something else to differentiate. maybe fch_spi_crop_chunk?
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 110: BIOS_DEBUG
Maybe BIOS_ERR?
Ack
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 147: bytesout + bytesin
We really need to fit both bytes in and out into the fifo at the same time?
Yes. Initially I did not, and it was causing a endless loop when writing more bytes than SPI_FIFO_DEPTH.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 148: BIOS_DEBUG
Again, maybe a higher level here than debug?
Ack
Hello Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#5).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c 7 files changed, 995 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/5
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/fch_spi.h:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/in... PS2, Line 94: crop_chunk
Would conflict with same function at drivers/spi/spi_flash. […]
No answer, changed to fch_spi_crop().
Hello Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#6).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c 7 files changed, 997 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/6
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 6:
(9 comments)
Hi Richard, this is my first pass. Please kindly look over my suggestions. The common theme here is to trim off some bulk in repeated loops and cluttering the symbol table with unnecessary global state.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 27: extern const struct spi_flash_ops fch_spi_flash_ops; see below to get rid of this global state.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 29: struct spi_data ctrl_spi_data; can be static with changes suggested below.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 248: * The following table holds all device probe functions Let's move this table into it's own file and expose flashes[] as const and flash_count = ARRAY_SIZE() as const also.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 380: flash->ops = &fch_spi_flash_ops; `fch_spi_flash_ops_init(flash);`
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 173: static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len, If I am not mistaken `non_standard_sst_write_aai()` is almost identical to `fch_spi_flash_write()` here you set `cmd[0] = ctrl_spi_data.write_cmd;` in terms of the main loop with a few random things at the start of `non_standard_sst_write_aai()`.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 184: return non_standard_spi_write(offset, len, buf); I think it best to remove the main loop from `non_standard_sst_write_aai()` and just figure out what things need to be set around this branching here, like `cmd[0]`.
With those changes that should trim down fch_spi_special.c quite some!
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 230: const struct spi_flash_ops fch_spi_flash_ops = { Can now be static
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 236: void fch_spi_flash_ops_init(struct spi_flash *flash) { flash->ops = &fch_spi_flash_ops; }
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 115: int non_standard_spi_write(uint32_t offset, size_t len, const void *buf) This function is unnecessary indirection, just called `non_standard_sst_write_aai()` directly from `fch_spi_flash_write()` move all the branch logic into `non_standard_sst_write_aai()`s preamble and pass a const ref to ctrl_spi_data as an argument so we can drop the extern.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 216: int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in) call this `fch_spi_flash_cmd()`
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 238: int fch_spi_flash_cmd(u8 cmd, void *response, size_t len) It's only used in `fch_spi_flash_probe()` may as well inline it.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 312: const struct spi_data * get_ctrl_spi_data(void) { return &ctrl_spi_data; }
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 379: save_flash_data s/save_flash_data()/set_ctrl_spi_data()/g
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 381: if (idp[0] == VENDOR_ID_SST) { seems like this if block is overriding things that were discovered and stored in flash then passed in to `save_flash_data()`. Maybe store them as local state override them in `flash` and call `save_flash_data()` later on.
At the very least this takes away some global mutations on these branches and delineates those transitions though function calls.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 34: int fch_spi_crop_chunk(unsigned int cmd_len, unsigned int buf_len) static
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 50: u8 buff[SPI_FIFO_DEPTH + 1]; put a check in here that cmd_len+ data_len isn't larger than SPI_FIFO_DEPTH and if it is return -1 to the call site.
`buf` is idiomatic tiny bit more however that's not hugely important.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 6:
(16 comments)
os
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 27: extern const struct spi_flash_ops fch_spi_flash_ops;
see below to get rid of this global state.
Gotcha. Will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 29: struct spi_data ctrl_spi_data;
can be static with changes suggested below.
Would need to create functions to return needed parameters. Why do you have problem with a global?
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 216: int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in)
call this `fch_spi_flash_cmd()`
Will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 238: int fch_spi_flash_cmd(u8 cmd, void *response, size_t len)
It's only used in `fch_spi_flash_probe()` may as well inline it.
Will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 248: * The following table holds all device probe functions
Let's move this table into it's own file and expose flashes[] as const and flash_count = ARRAY_SIZE( […]
On original code (drivers/spi/spi_flash.c) it was done this way. I have no problem either way.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 312:
const struct spi_data * get_ctrl_spi_data(void) […]
understood... if I create a separate file for the table.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 379: save_flash_data
s/save_flash_data()/set_ctrl_spi_data()/g
Will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 380: flash->ops = &fch_spi_flash_ops;
`fch_spi_flash_ops_init(flash);`
Will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 381: if (idp[0] == VENDOR_ID_SST) {
seems like this if block is overriding things that were discovered and stored in flash then passed i […]
non_standard is only set here, no where else. Yes, page_size is overwritten if it's SST, however there's no way for save_flash_data to check 2 possible values on this particular case only (unless I moved this code into save_flash_data).
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 34: int fch_spi_crop_chunk(unsigned int cmd_len, unsigned int buf_len)
static
It's used by fch_spi_special.c
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 50: u8 buff[SPI_FIFO_DEPTH + 1];
put a check in here that cmd_len+ data_len isn't larger than SPI_FIFO_DEPTH and if it is return -1 t […]
check not needed, it'll never be due to fch_spi_crop_chunk being called before this function is called.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 173: static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len,
If I am not mistaken `non_standard_sst_write_aai()` is almost identical to `fch_spi_flash_write()` h […]
It's almost identical, only difference is protection if write address don't start on a word boundary (writes a single byte with a different command before continuing to write with page_size). Also, see what I wrote in your next comment.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 184: return non_standard_spi_write(offset, len, buf);
I think it best to remove the main loop from `non_standard_sst_write_aai()` and just figure out what […]
fch_spi_special was written as it was because I was planning for any weird flash that might show up in the future, though currently SST is the only one with special code. I don't think it's too much, you never know when something new come (though it looks like now there's a "de facto" for flash commands, which only SST does not obeys).
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 230: const struct spi_flash_ops fch_spi_flash_ops = {
Can now be static
will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 236:
void fch_spi_flash_ops_init(struct spi_flash *flash) […]
will do.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 115: int non_standard_spi_write(uint32_t offset, size_t len, const void *buf)
This function is unnecessary indirection, just called `non_standard_sst_write_aai()` directly from ` […]
Done this way if we ever get a new weird flash that also need its own function.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 6:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 27: extern const struct spi_flash_ops fch_spi_flash_ops;
Gotcha. Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 29: struct spi_data ctrl_spi_data;
Would need to create functions to return needed parameters. […]
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 216: int do_fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in)
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 238: int fch_spi_flash_cmd(u8 cmd, void *response, size_t len)
Will do.
It's also used on inline command enable_write (see fch_spi.h). Done anyway.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 248: * The following table holds all device probe functions
On original code (drivers/spi/spi_flash.c) it was done this way. I have no problem either way.
Done, though size is not returned as const.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 312:
understood... if I create a separate file for the table.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 379: save_flash_data
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 380: flash->ops = &fch_spi_flash_ops;
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 381: if (idp[0] == VENDOR_ID_SST) {
non_standard is only set here, no where else. […]
Done the move above.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 230: const struct spi_flash_ops fch_spi_flash_ops = {
will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 236:
will do.
Done
Hello Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#7).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c A src/soc/amd/common/block/spi/fch_spi_table.c 8 files changed, 1,010 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/7
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7:
Patch Set 6:
(9 comments)
Hi Richard, this is my first pass. Please kindly look over my suggestions. The common theme here is to trim off some bulk in repeated loops and cluttering the symbol table with unnecessary global state.
Currently I left 5 of your comments unresolved, as I have a different opinion. However, on a recent conference call with co-workers, they were mostly in favor of your comments, so I'll solve the last 5 comments on my next drop.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 6: This overwrites the structure spi_flash_ops to use FCH SPI code instead of individual I would break this line at 80. I don't believe making it longer improves readability.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */
Completely sure. […]
The way I read it, the Range field is more like a size indicator that's applicable to RomBase. To me, when you say "point to next range", that sounds like an iterator. So it seems to me you're protecting overlapping regions.
Looks like you didn't update the comment.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 53: u32 addr; Maybe add a blank line after your variables.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 93: E Kind of a nit, but I'd make the case match in all the strings in this function. FWIW, I prefer lower case here.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 109: (u32) Why not remove this cast and change the format string instead?
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 137: this sequence is controlled : * by the SPI chip driver. Does this still make sense? Or should it refer to a different file now?
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 214: If some specialized SPI flash controllers : * (e.g. x86) can perform both command and response together, it should : * be handled at SPI flash controller driver level I don't follow what this means. Is this not the SPI flash controller driver? And why mention "specialized SPI flash controllers"?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 40: cmd_len--; Seems like a complex way to just write?
`return min((SPI_FIFO_DEPTH - cmd_len - 1), buf_len);`
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 184: if (spi_data_ptr->non_standard != NON_STANDARD_SPI_NONE) isn't this essentially a duplicate check to the branch in non_standard_spi_write() ?
Although, as an aside, I still believe that fch_spi_special.c could ultimately be dedup into this function however I wont block on that.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 117: int ret = 0; not sure if you missed this one but you can inline `non_standard_sst_write_aai()` here and pass `spi_data_ptr` as an argument so you can get rid of these two `get_ctrl_spi_data()` call sites.
``` int non_standard_spi_write(const struct spi_data *spi_data_ptr, uint32_t offset, size_t len, const void *buf) { if (spi_data_ptr->non_standard != NON_STANDARD_SPI_SST) return 0;
[...non_standard_sst_write_aai()...] } ```
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7: Code-Review+1
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 6: This overwrites the structure spi_flash_ops to use FCH SPI code instead of individual
I would break this line at 80. I don't believe making it longer improves readability.
will do.
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */
The way I read it, the Range field is more like a size indicator that's applicable to RomBase. […]
Nope, I'm protecting 2 adjacent regions. Range points to the start address of a region. The range value must be multiplied by the granularity (which is also the size of the region) to get the actual offset from the SPI start address. Will add the above as a comment.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 53: u32 addr;
Maybe add a blank line after your variables.
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 93: E
Kind of a nit, but I'd make the case match in all the strings in this function. […]
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 109: (u32)
Why not remove this cast and change the format string instead?
Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 137: this sequence is controlled : * by the SPI chip driver.
Does this still make sense? Or should it refer to a different file now?
I believe that I should just remove the section after the comma.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 214: If some specialized SPI flash controllers : * (e.g. x86) can perform both command and response together, it should : * be handled at SPI flash controller driver level
I don't follow what this means. […]
Left over from copy/paste. Will remove.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 34: int fch_spi_crop_chunk(unsigned int cmd_len, unsigned int buf_len)
It's used by fch_spi_special. […]
Missed, next patch
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 50: u8 buff[SPI_FIFO_DEPTH + 1];
check not needed, it'll never be due to fch_spi_crop_chunk being called before this function is call […]
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 173: static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len,
It's almost identical, only difference is protection if write address don't start on a word boundary […]
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 184: return non_standard_spi_write(offset, len, buf);
fch_spi_special was written as it was because I was planning for any weird flash that might show up […]
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 40: cmd_len--;
Seems like a complex way to just write? […]
With an extra parenthesis, but I believe you're right. Will do.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 184: if (spi_data_ptr->non_standard != NON_STANDARD_SPI_NONE)
isn't this essentially a duplicate check to the branch in non_standard_spi_write() ? […]
Modified already, you'll see when I post my next patch.
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 115: int non_standard_spi_write(uint32_t offset, size_t len, const void *buf)
Done this way if we ever get a new weird flash that also need its own function.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 117: int ret = 0;
not sure if you missed this one but you can inline `non_standard_sst_write_aai()` here and pass `spi […]
This code don't exists anymore, you'see on my next patch.
Hello Edward O'Callaghan, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#8).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c A src/soc/amd/common/block/spi/fch_spi_table.c 8 files changed, 987 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/8
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 6: This overwrites the structure spi_flash_ops to use FCH SPI code instead of individual
will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/2/src/soc/amd/common/block/sp... PS2, Line 474: reg32++; /* next range */
Nope, I'm protecting 2 adjacent regions. Range points to the start address of a region. […]
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_ctrl.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 53: u32 addr;
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 93: E
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 109: (u32)
Will do.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 137: this sequence is controlled : * by the SPI chip driver.
I believe that I should just remove the section after the comma.
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 214: If some specialized SPI flash controllers : * (e.g. x86) can perform both command and response together, it should : * be handled at SPI flash controller driver level
Left over from copy/paste. Will remove.
Done
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/6/src/soc/amd/common/block/sp... PS6, Line 34: int fch_spi_crop_chunk(unsigned int cmd_len, unsigned int buf_len)
Missed, next patch
Done
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 40: cmd_len--;
With an extra parenthesis, but I believe you're right. Will do.
Done
Hello Edward O'Callaghan, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35018
to look at the new patch set (#9).
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c A src/soc/amd/common/block/spi/fch_spi_table.c 8 files changed, 981 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/35018/9
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 9: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 9:
Thanks for taking the time and working through the comments Richard, much appreciated !
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
soc/amd/common/block: Create new SPI code
Create a new SPI code that overrides flash operations and uses the SPI controller within the FCH to its fullest.
Reference: Family 15h models 70h-7Fh BKDG revision 3.06 (public)
BUG=b:136595978 TEST=Build and boot grunt using this code, with debug enabled. Check output.
Change-Id: Id293fb9b2da84c4206c7a1341b64e83fc0b8d71d Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35018 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- A src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/include/amdblocks/lpc.h A src/soc/amd/common/block/spi/Kconfig A src/soc/amd/common/block/spi/Makefile.inc A src/soc/amd/common/block/spi/fch_spi_ctrl.c A src/soc/amd/common/block/spi/fch_spi_flash.c A src/soc/amd/common/block/spi/fch_spi_special.c A src/soc/amd/common/block/spi/fch_spi_table.c 8 files changed, 981 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/amd/common/block/include/amdblocks/fch_spi.h b/src/soc/amd/common/block/include/amdblocks/fch_spi.h new file mode 100644 index 0000000..24cfbfc --- /dev/null +++ b/src/soc/amd/common/block/include/amdblocks/fch_spi.h @@ -0,0 +1,87 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _FCH_SPI_H_ +#define _FCH_SPI_H_ + +#include <stdint.h> +#include <stddef.h> + +#define GRANULARITY_TEST_4k 0x0000f000 /* bits 15-12 */ +#define WORD_TO_DWORD_UPPER(x) ((x << 16) & 0xffff0000) +#define SPI_PAGE_WRITE 0x02 +#define SPI_WRITE_ENABLE 0x06 +#define IDCODE_CONT_LEN 0 +#define IDCODE_PART_LEN 5 +#define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN) + +/* SPI MMIO registers */ +#define SPI_CNTRL0 0x00 +#define SPI_ACCESS_MAC_ROM_EN BIT(22) +#define SPI_RESTRICTED_CMD1 0x04 +#define SPI_RESTRICTED_CMD2 0x08 +#define SPI_CNTRL1 0x0c +#define SPI_CMD_CODE 0x45 +#define SPI_CMD_TRIGGER 0x47 + +/* Special SST write commands */ +#define CMD_SST_BP 0x02 /* Byte Program */ +#define CMD_SST_AAI_WP 0xad /* Auto Address Increment Word Program */ + +#define SST_256 0x004b /* Only SST that programs 256 bytes at once */ + +enum non_standard_spi { + NON_STANDARD_SPI_NONE = 0, + NON_STANDARD_SPI_SST, +}; + +struct spi_flash_table { + const u8 shift; + const u8 idcode; + int (*probe)(const struct spi_slave *spi, u8 *idcode, + struct spi_flash *flash); +}; + +struct spi_data { + const char *name; + u32 size; + u32 sector_size; + u32 page_size; + u8 status_cmd; + u8 erase_cmd; + u8 write_cmd; + u8 write_enable_cmd; + u8 read_cmd; + u8 read_cmd_len; + enum non_standard_spi non_standard; +}; + +void fch_spi_init(void); +void fch_spi_flash_ops_init(struct spi_flash *flash); +int fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in); +int fch_spi_flash_cmd_write(const u8 *cmd, size_t cmd_len, const void *data, size_t data_len); +int fch_spi_wait_cmd_ready(unsigned long timeout); +int non_standard_sst_byte_write(u32 offset, const void *buf); +int non_standard_sst_write_aai(u32 offset, size_t len, const void *buf, size_t start); +const struct spi_flash_table *get_spi_flash_table(int *table_size); +const struct spi_data *get_ctrl_spi_data(void); + +static inline int fch_spi_enable_write(void) +{ + u8 cmd_enable = SPI_WRITE_ENABLE; + return fch_spi_flash_cmd(&cmd_enable, 1, NULL, 0); +} + +#endif /* _FCH_SPI_H_ */ diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h index f956ba3..11880eb 100644 --- a/src/soc/amd/common/block/include/amdblocks/lpc.h +++ b/src/soc/amd/common/block/include/amdblocks/lpc.h @@ -90,6 +90,14 @@ #define DECODE_IO_PORT_ENABLE0_H BIT(0)
#define LPC_MEM_PORT1 0x4c +#define ROM_PROTECT_RANGE0 0x50 +#define ROM_BASE_MASK 0xfffff000 /* bits 31-12 */ +#define ROM_RANGE_WP BIT(10) +#define ROM_RANGE_RP BIT(9) +#define RANGE_UNIT BIT(8) +#define RANGE_ADDR_MASK 0x000000ff /* Range defined by bits 7-0 */ +#define ROM_PROTECT_RANGE_REG(n) (ROM_PROTECT_RANGE0 + (4 * n)) +#define MAX_ROM_PROTECT_RANGES 4 #define LPC_MEM_PORT0 0x60
/* Register 0x64 is 32-bit, composed by two 16-bit sub-registers. diff --git a/src/soc/amd/common/block/spi/Kconfig b/src/soc/amd/common/block/spi/Kconfig new file mode 100644 index 0000000..785e6da --- /dev/null +++ b/src/soc/amd/common/block/spi/Kconfig @@ -0,0 +1,11 @@ +config SOC_AMD_COMMON_BLOCK_SPI + bool + default n + help + Select this option to add FCH SPI controller functions to the build. + This overwrites the structure spi_flash_ops to use FCH SPI code + instead of individual SPI specific code. + +config SOC_AMD_COMMON_BLOCK_SPI_DEBUG + bool + default n diff --git a/src/soc/amd/common/block/spi/Makefile.inc b/src/soc/amd/common/block/spi/Makefile.inc new file mode 100644 index 0000000..b94eda4 --- /dev/null +++ b/src/soc/amd/common/block/spi/Makefile.inc @@ -0,0 +1,30 @@ +ifeq ($(CONFIG_SOC_AMD_COMMON_BLOCK_SPI),y) + +bootblock-y += fch_spi_ctrl.c +bootblock-y += fch_spi_flash.c +bootblock-y += fch_spi_special.c +bootblock-y += fch_spi_table.c +romstage-y += fch_spi_ctrl.c +romstage-y += fch_spi_flash.c +romstage-y += fch_spi_special.c +romstage-y += fch_spi_table.c +verstage-y += fch_spi_ctrl.c +verstage-y += fch_spi_flash.c +verstage-y += fch_spi_special.c +verstage-y += fch_spi_table.c +postcar-y += fch_spi_ctrl.c +postcar-y += fch_spi_flash.c +postcar-y += fch_spi_special.c +postcar-y += fch_spi_table.c +ramstage-y += fch_spi_ctrl.c +ramstage-y += fch_spi_flash.c +ramstage-y += fch_spi_special.c +ramstage-y += fch_spi_table.c +ifeq ($(CONFIG_SPI_FLASH_SMM),y) +smm-y += fch_spi_ctrl.c +smm-y += fch_spi_flash.c +smm-y += fch_spi_special.c +smm-y += fch_spi_table.c +endif + +endif diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c new file mode 100644 index 0000000..4299e4f --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -0,0 +1,427 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi_flash.h> +#include <soc/southbridge.h> +#include <soc/pci_devs.h> +#include <amdblocks/fch_spi.h> +#include <amdblocks/lpc.h> +#include <drivers/spi/spi_flash_internal.h> +#include <device/pci_ops.h> +#include <timer.h> +#include <lib.h> + +static struct spi_data ctrl_spi_data; +static uint32_t spibar; + +static inline uint8_t spi_read8(uint8_t reg) +{ + return read8((void *)(spibar + reg)); +} + +static inline uint32_t spi_read32(uint8_t reg) +{ + return read32((void *)(spibar + reg)); +} + +static inline void spi_write8(uint8_t reg, uint8_t val) +{ + write8((void *)(spibar + reg), val); +} + +static inline void spi_write32(uint8_t reg, uint32_t val) +{ + write32((void *)(spibar + reg), val); +} + +static void dump_state(const char *str, u8 phase) +{ + u8 dump_size; + u32 addr; + + if (!CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) + return; + + printk(BIOS_DEBUG, "SPI: %s\n", str); + printk(BIOS_DEBUG, "Cntrl0: %x\n", spi_read32(SPI_CNTRL0)); + printk(BIOS_DEBUG, "Status: %x\n", spi_read32(SPI_STATUS)); + + addr = spibar + SPI_FIFO; + if (phase == 0) { + dump_size = spi_read8(SPI_TX_BYTE_COUNT); + printk(BIOS_DEBUG, "TxByteCount: %x\n", dump_size); + printk(BIOS_DEBUG, "CmdCode: %x\n", spi_read8(SPI_CMD_CODE)); + } else { + dump_size = spi_read8(SPI_RX_BYTE_COUNT); + printk(BIOS_DEBUG, "RxByteCount: %x\n", dump_size); + addr += spi_read8(SPI_TX_BYTE_COUNT); + } + + if (dump_size > 0) + hexdump((void *)addr, dump_size); +} + +static int wait_for_ready(void) +{ + const uint32_t timeout_ms = 500; + struct stopwatch sw; + + stopwatch_init_msecs_expire(&sw, timeout_ms); + + do { + if (!(spi_read32(SPI_STATUS) & SPI_BUSY)) + return 0; + } while (!stopwatch_expired(&sw)); + + return -1; +} + +static int execute_command(void) +{ + dump_state("Before execute", 0); + + spi_write8(SPI_CMD_TRIGGER, SPI_CMD_TRIGGER_EXECUTE); + + if (wait_for_ready()) + printk(BIOS_ERR, + "FCH_SC Error: Timeout executing command\n"); + + dump_state("Transaction finished", 1); + + return 0; +} + +void spi_init(void) +{ + spibar = lpc_get_spibase(); + printk(BIOS_DEBUG, "%s: Spibar at 0x%08x\n", __func__, spibar); +} + +const struct spi_data *get_ctrl_spi_data(void) +{ + return &ctrl_spi_data; +} + +static int spi_ctrlr_xfer(const void *dout, size_t bytesout, void *din, size_t bytesin) +{ + size_t count; + uint8_t cmd; + uint8_t *bufin = din; + const uint8_t *bufout = dout; + + if (CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) + printk(BIOS_DEBUG, "%s(%zx, %zx)\n", __func__, bytesout, + bytesin); + + /* First byte is cmd which cannot be sent through FIFO */ + cmd = bufout[0]; + bufout++; + bytesout--; + + /* + * Check if this is a write command attempting to transfer more bytes + * than the controller can handle. Iterations for writes are not + * supported here because each SPI write command needs to be preceded + * and followed by other SPI commands. + */ + if (bytesout + bytesin > SPI_FIFO_DEPTH) { + printk(BIOS_WARNING, "FCH_SC: Too much to transfer, code error!\n"); + return -1; + } + + if (wait_for_ready()) + return -1; + + spi_write8(SPI_CMD_CODE, cmd); + spi_write8(SPI_TX_BYTE_COUNT, bytesout); + spi_write8(SPI_RX_BYTE_COUNT, bytesin); + + for (count = 0; count < bytesout; count++) + spi_write8(SPI_FIFO + count, bufout[count]); + + if (execute_command()) + return -1; + + for (count = 0; count < bytesin; count++) + bufin[count] = spi_read8(SPI_FIFO + count + bytesout); + + return 0; +} + +static int amd_xfer_vectors(struct spi_op vectors[], size_t count) +{ + int ret; + void *din; + size_t bytes_in; + + if (count < 1 || count > 2) + return -1; + + /* SPI flash commands always have a command first... */ + if (!vectors[0].dout || !vectors[0].bytesout) + return -1; + /* And not read any data during the command. */ + if (vectors[0].din || vectors[0].bytesin) + return -1; + + if (count == 2) { + /* If response bytes requested ensure the buffer is valid. */ + if (vectors[1].bytesin && !vectors[1].din) + return -1; + /* No sends can accompany a receive. */ + if (vectors[1].dout || vectors[1].bytesout) + return -1; + din = vectors[1].din; + bytes_in = vectors[1].bytesin; + } else { + din = NULL; + bytes_in = 0; + } + + ret = spi_ctrlr_xfer(vectors[0].dout, vectors[0].bytesout, din, bytes_in); + + if (ret) { + vectors[0].status = SPI_OP_FAILURE; + if (count == 2) + vectors[1].status = SPI_OP_FAILURE; + } else { + vectors[0].status = SPI_OP_SUCCESS; + if (count == 2) + vectors[1].status = SPI_OP_SUCCESS; + } + + return ret; +} + +int fch_spi_flash_cmd(const void *dout, size_t bytes_out, void *din, size_t bytes_in) +{ + /* + * SPI flash requires command-response kind of behavior. Thus, two + * separate SPI vectors are required -- first to transmit dout and other + * to receive in din. + */ + struct spi_op vectors[] = { + [0] = { .dout = dout, .bytesout = bytes_out, + .din = NULL, .bytesin = 0, }, + [1] = { .dout = NULL, .bytesout = 0, + .din = din, .bytesin = bytes_in }, + }; + size_t count = ARRAY_SIZE(vectors); + if (!bytes_in) + count = 1; + + return amd_xfer_vectors(vectors, count); +} + +static void set_ctrl_spi_data(struct spi_flash *flash) +{ + u8 cmd = SPI_PAGE_WRITE; + + ctrl_spi_data.name = flash->name; + ctrl_spi_data.size = flash->size; + ctrl_spi_data.sector_size = flash->sector_size; + ctrl_spi_data.status_cmd = flash->status_cmd; + ctrl_spi_data.erase_cmd = flash->erase_cmd; + ctrl_spi_data.write_enable_cmd = SPI_WRITE_ENABLE; + + if (flash->vendor == VENDOR_ID_SST) { + ctrl_spi_data.non_standard = NON_STANDARD_SPI_SST; + if ((flash->model & 0x00ff) == SST_256) + ctrl_spi_data.page_size = 256; + else { + ctrl_spi_data.page_size = 2; + cmd = CMD_SST_AAI_WP; + } + } else { + ctrl_spi_data.page_size = flash->page_size; + ctrl_spi_data.non_standard = NON_STANDARD_SPI_NONE; + } + ctrl_spi_data.write_cmd = cmd; + + if (CONFIG(SPI_FLASH_NO_FAST_READ)) { + ctrl_spi_data.read_cmd_len = 4; + ctrl_spi_data.read_cmd = CMD_READ_ARRAY_SLOW; + } else { + ctrl_spi_data.read_cmd_len = 5; + ctrl_spi_data.read_cmd = CMD_READ_ARRAY_FAST; + } +} + +static int fch_spi_flash_probe(const struct spi_slave *spi, struct spi_flash *flash) +{ + int ret, i, shift, table_size; + u8 idcode[IDCODE_LEN], *idp, cmd = CMD_READ_ID; + const struct spi_flash_table *flash_ptr = get_spi_flash_table(&table_size); + + /* Read the ID codes */ + ret = fch_spi_flash_cmd(&cmd, 1, idcode, sizeof(idcode)); + if (ret) + return -1; + + if (CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG)) { + printk(BIOS_SPEW, "SF: Got idcode: "); + for (i = 0; i < sizeof(idcode); i++) + printk(BIOS_SPEW, "%02x ", idcode[i]); + printk(BIOS_SPEW, "\n"); + } + + /* count the number of continuation bytes */ + for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; + ++shift, ++idp) + continue; + + printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + + /* search the table for matches in shift and id */ + for (i = 0; i < table_size; ++i) { + if (flash_ptr->shift == shift && flash_ptr->idcode == *idp) { + /* we have a match, call probe */ + if (flash_ptr->probe(spi, idp, flash) == 0) { + flash->vendor = idp[0]; + flash->model = (idp[1] << 8) | idp[2]; + set_ctrl_spi_data(flash); + fch_spi_flash_ops_init(flash); + return 0; + } + } + flash_ptr++; + } + + /* No match, return error. */ + return -1; +} + +static int protect_a_range(u32 value) +{ + u32 reg32; + u8 n; + + /* find a free protection register */ + for (n = 0; n < MAX_ROM_PROTECT_RANGES; n++) { + reg32 = pci_read_config32(SOC_LPC_DEV, ROM_PROTECT_RANGE_REG(n)); + if (!reg32) + break; + } + if (n == MAX_ROM_PROTECT_RANGES) + return -1; /* no free range */ + + pci_write_config32(SOC_LPC_DEV, ROM_PROTECT_RANGE_REG(n), value); + return 0; +} + +/* + * Protect range of SPI flash defined by region using the SPI flash controller. + * + * Note: Up to 4 ranges can be protected, though if a particular region requires more than one + * range, total number of regions decreases accordingly. Each range can be programmed to 4KiB or + * 64KiB granularity. + * + * Warning: If more than 1 region needs protection, and they need mixed protections (read/write) + * than start with the region that requires the most protection. After the restricted commands + * have been written, they can't be changed (write once). So if first region is write protection + * and second region is read protection, it's best to define first region as read and write + * protection. + */ +static int fch_spi_flash_protect(const struct spi_flash *flash, const struct region *region, + const enum ctrlr_prot_type type) +{ + int ret; + u32 reg32, rom_base, range_base; + size_t addr, len, gran_value, total_ranges, range; + bool granularity_64k = true; /* assume 64k granularity */ + + addr = region->offset; + len = region->size; + + reg32 = pci_read_config32(SOC_LPC_DEV, ROM_ADDRESS_RANGE2_START); + rom_base = WORD_TO_DWORD_UPPER(reg32); + if (addr < rom_base) + return -1; + range_base = addr % rom_base; + + /* Define granularity to be used */ + if (GRANULARITY_TEST_4k & range_base) + granularity_64k = false; /* use 4K granularity */ + if (GRANULARITY_TEST_4k & len) + granularity_64k = false; /* use 4K granularity */ + + /* Define the first range and total number of ranges required */ + if (granularity_64k) { + gran_value = 0x00010000; /* 64 KiB */ + range_base = range_base >> 16; + } else { + gran_value = 0x00001000; /* 4 KiB */ + range_base = range_base >> 12; + } + total_ranges = len / gran_value; + range_base &= RANGE_ADDR_MASK; + + /* Create reg32 to be written into a range register and program required ranges */ + reg32 = rom_base & ROM_BASE_MASK; + reg32 |= range_base; + if (granularity_64k) + reg32 |= RANGE_UNIT; + if (type & WRITE_PROTECT) + reg32 |= ROM_RANGE_WP; + if (type & READ_PROTECT) + reg32 |= ROM_RANGE_RP; + + for (range = 0; range < total_ranges; range++) { + ret = protect_a_range(reg32); + if (ret) + return ret; + /* + * Next range (lower 8 bits). Range points to the start address of a region. + * The range value must be multiplied by the granularity (which is also the + * size of the region) to get the actual offset from the SPI start address. + */ + reg32++; + } + + /* define commands to be blocked if in range */ + reg32 = 0; + if (type & WRITE_PROTECT) { + reg32 |= (ctrl_spi_data.write_enable_cmd << 24); + reg32 |= (ctrl_spi_data.write_cmd << 16); + reg32 |= (ctrl_spi_data.erase_cmd << 8); + } + if (type & READ_PROTECT) + reg32 |= ctrl_spi_data.read_cmd; + + /* Final steps to protect region */ + pci_write_config32(SOC_LPC_DEV, SPI_RESTRICTED_CMD1, reg32); + reg32 = spi_read32(SPI_CNTRL0); + reg32 &= ~SPI_ACCESS_MAC_ROM_EN; + spi_write32(SPI_CNTRL0, reg32); + + return 0; +} + +const struct spi_ctrlr fch_spi_flash_ctrlr = { + .max_xfer_size = SPI_FIFO_DEPTH, + .flash_probe = fch_spi_flash_probe, + .flash_protect = fch_spi_flash_protect, +}; + +const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = { + { + .ctrlr = &fch_spi_flash_ctrlr, + .bus_start = 0, + .bus_end = 0, + }, +}; + +const size_t spi_ctrlr_bus_map_count = ARRAY_SIZE(spi_ctrlr_bus_map); diff --git a/src/soc/amd/common/block/spi/fch_spi_flash.c b/src/soc/amd/common/block/spi/fch_spi_flash.c new file mode 100644 index 0000000..40dd0e2 --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_flash.c @@ -0,0 +1,246 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi_flash.h> +#include <soc/southbridge.h> +#include <amdblocks/fch_spi.h> +#include <drivers/spi/spi_flash_internal.h> +#include <timer.h> +#include <string.h> + +static void spi_flash_addr(u32 addr, u8 *cmd) +{ + /* cmd[0] is actual command */ + cmd[1] = addr >> 16; + cmd[2] = addr >> 8; + cmd[3] = addr >> 0; +} + +static int crop_chunk(unsigned int cmd_len, unsigned int buf_len) +{ + return min((SPI_FIFO_DEPTH - (cmd_len - 1)), buf_len); +} + +int fch_spi_flash_cmd_write(const u8 *cmd, size_t cmd_len, const void *data, size_t data_len) +{ + int ret; + u8 buff[SPI_FIFO_DEPTH + 1]; + + if ((cmd_len + data_len) > SPI_FIFO_DEPTH) + return -1; + memcpy(buff, cmd, cmd_len); + memcpy(buff + cmd_len, data, data_len); + + ret = fch_spi_flash_cmd(buff, cmd_len + data_len, NULL, 0); + if (ret) { + printk(BIOS_WARNING, "FCH_SF: Failed to send write command (%zu bytes): %d\n", + data_len, ret); + } + + return ret; +} + +static int fch_spi_flash_status(const struct spi_flash *flash, uint8_t *reg) +{ + int ret; + u8 status, cmd = CMD_READ_STATUS; + + ret = fch_spi_flash_cmd(&cmd, 1, &status, 1); + if (!ret) + *reg = status; + return ret; +} + +int fch_spi_wait_cmd_ready(unsigned long timeout) +{ + struct mono_time current, end; + int ret; + u8 status; + + timer_monotonic_get(¤t); + end = current; + mono_time_add_msecs(&end, timeout); + + do { + ret = fch_spi_flash_status(NULL, &status); + if (ret) + return -1; + if ((status & STATUS_WIP) == 0) + return 0; + timer_monotonic_get(¤t); + } while (!mono_time_after(¤t, &end)); + + printk(BIOS_DEBUG, "FCH_SF: timeout at %ld msec\n", timeout); + return -1; +} + +static int fch_spi_flash_erase(const struct spi_flash *flash, uint32_t offset, size_t len) +{ + u32 start, end, erase_size; + const struct spi_data *spi_data_ptr = get_ctrl_spi_data(); + int ret = -1; + u8 cmd[4]; + + erase_size = spi_data_ptr->sector_size; + if (offset % erase_size || len % erase_size) { + printk(BIOS_WARNING, "%s: Erase offset/length not multiple of erase size\n", + spi_data_ptr->name); + return -1; + } + if (len == 0) { + printk(BIOS_WARNING, "%s: Erase length cannot be 0\n", spi_data_ptr->name); + return -1; + } + + cmd[0] = spi_data_ptr->erase_cmd; + start = offset; + end = start + len; + + while (offset < end) { + spi_flash_addr(offset, cmd); + offset += erase_size; + +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "FCH_SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1], + cmd[2], cmd[3], offset); +#endif + ret = fch_spi_enable_write(); + if (ret) + goto out; + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), NULL, 0); + if (ret) + goto out; + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PAGE_ERASE_TIMEOUT_MS); + if (ret) + goto out; + } + + printk(BIOS_DEBUG, "%s: Successfully erased %zu bytes @ %#x\n", spi_data_ptr->name, len, + start); + +out: + return ret; +} + +static int fch_spi_flash_read(const struct spi_flash *flash, uint32_t offset, size_t len, + void *buf) +{ + const struct spi_data *spi_data_ptr = get_ctrl_spi_data(); + uint8_t *data = buf; + int ret; + size_t xfer_len; + u8 cmd[5]; + + cmd[0] = spi_data_ptr->read_cmd; + cmd[4] = 0; + while (len) { + xfer_len = crop_chunk(spi_data_ptr->read_cmd_len, len); + spi_flash_addr(offset, cmd); + ret = fch_spi_flash_cmd(cmd, spi_data_ptr->read_cmd_len, data, xfer_len); + if (ret) { + printk(BIOS_WARNING, + "FCH_SF: Failed to send read command %#.2x(%#x, %#zx): %d\n", + cmd[0], offset, xfer_len, ret); + return ret; + } + offset += xfer_len; + data += xfer_len; + len -= xfer_len; + } + return 0; +} + +static int fch_spi_flash_write(const struct spi_flash *flash, uint32_t offset, size_t len, + const void *buf) +{ + unsigned long byte_addr; + unsigned long page_size; + const struct spi_data *spi_data_ptr = get_ctrl_spi_data(); + size_t chunk_len; + size_t actual, start = 0; + int ret = 0; + u8 cmd[4]; + + page_size = spi_data_ptr->page_size; + if (spi_data_ptr->non_standard == NON_STANDARD_SPI_SST) { + if (offset % 2) { + ret = non_standard_sst_byte_write(offset, buf); + len--; + start++; + offset++; + if (ret) + return ret; + } + if (page_size == 2) + return non_standard_sst_write_aai(offset, len, buf, start); + } + + for (actual = start; actual < len; actual += chunk_len) { + byte_addr = offset % page_size; + chunk_len = min(len - actual, page_size - byte_addr); + chunk_len = crop_chunk(sizeof(cmd), chunk_len); + + cmd[0] = spi_data_ptr->write_cmd; + cmd[1] = (offset >> 16) & 0xff; + cmd[2] = (offset >> 8) & 0xff; + cmd[3] = offset & 0xff; +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu" + "\n", buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); +#endif + + ret = fch_spi_enable_write(); + if (ret < 0) { + printk(BIOS_WARNING, "%s: Enabling Write failed\n", spi_data_ptr->name); + goto out; + } + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), buf + actual, chunk_len); + if (ret < 0) { + printk(BIOS_WARNING, "%s: Page Program failed\n", spi_data_ptr->name); + goto out; + } + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); + if (ret) + goto out; + + offset += chunk_len; + } + +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "%s: Successfully programmed %zu bytes @ 0x%lx\n", + spi_data_ptr->name, len, (unsigned long)(offset - len)); +#endif + ret = 0; + +out: + return ret; +} + +static const struct spi_flash_ops fch_spi_flash_ops = { + .read = fch_spi_flash_read, + .write = fch_spi_flash_write, + .erase = fch_spi_flash_erase, + .status = fch_spi_flash_status, +}; + +void fch_spi_flash_ops_init(struct spi_flash *flash) +{ + flash->ops = &fch_spi_flash_ops; +} diff --git a/src/soc/amd/common/block/spi/fch_spi_special.c b/src/soc/amd/common/block/spi/fch_spi_special.c new file mode 100644 index 0000000..456a389 --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_special.c @@ -0,0 +1,89 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <spi-generic.h> +#include <amdblocks/fch_spi.h> +#include <string.h> + +int non_standard_sst_byte_write(u32 offset, const void *buf) +{ + int ret; + u8 cmd[4] = { + CMD_SST_BP, + offset >> 16, + offset >> 8, + offset, + }; + + ret = fch_spi_enable_write(); + if (ret) + return ret; + + ret = fch_spi_flash_cmd_write(cmd, sizeof(cmd), buf, 1); + if (ret) + return ret; + + return fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); +} + +int non_standard_sst_write_aai(u32 offset, size_t len, const void *buf, size_t start) +{ + size_t actual, cmd_len; + int ret = 0; + u8 cmd[4]; + + ret = fch_spi_enable_write(); + if (ret) + goto done; + + cmd_len = 4; + cmd[0] = CMD_SST_AAI_WP; + cmd[1] = offset >> 16; + cmd[2] = offset >> 8; + cmd[3] = offset; + + for (actual = start; actual < len - 1; actual += 2) { +#if CONFIG(SOC_AMD_COMMON_BLOCK_SPI_DEBUG) + printk(BIOS_DEBUG, "PP: 0x%p => cmd = { 0x%02x 0x%06lx }" + " chunk_len = 2\n", + buf + actual, cmd[0], (offset + actual)); +#endif + + ret = fch_spi_enable_write(); + if (ret < 0) { + printk(BIOS_WARNING, "SF: Enabling Write failed\n"); + break; + } + + ret = fch_spi_flash_cmd_write(cmd, cmd_len, buf + actual, 2); + if (ret < 0) { + printk(BIOS_WARNING, "SF: SST word Program failed\n"); + break; + } + + ret = fch_spi_wait_cmd_ready(SPI_FLASH_PROG_TIMEOUT_MS); + if (ret) + break; + + offset += 2; + cmd_len = 1; + } + /* If there is a single trailing byte, write it out */ + if (!ret && actual != len) + ret = non_standard_sst_byte_write(offset, buf + actual); +done: + return ret; +} diff --git a/src/soc/amd/common/block/spi/fch_spi_table.c b/src/soc/amd/common/block/spi/fch_spi_table.c new file mode 100644 index 0000000..8c0108a --- /dev/null +++ b/src/soc/amd/common/block/spi/fch_spi_table.c @@ -0,0 +1,83 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <spi_flash.h> +#include <amdblocks/fch_spi.h> +#include <drivers/spi/spi_flash_internal.h> + +/* + * The following table holds all device probe functions + * + * shift: number of continuation bytes before the ID + * idcode: the expected IDCODE or 0xff for non JEDEC devices + * probe: the function to call + * + * Non JEDEC devices should be ordered in the table such that + * the probe functions with best detection algorithms come first. + * + * Several matching entries are permitted, they will be tried + * in sequence until a probe function returns non NULL. + * + * IDCODE_CONT_LEN may be redefined if a device needs to declare a + * larger "shift" value. IDCODE_PART_LEN generally shouldn't be + * changed. This is the max number of bytes probe functions may + * examine when looking up part-specific identification info. + * + * Probe functions will be given the idcode buffer starting at their + * manu id byte (the "idcode" in the table below). In other words, + * all of the continuation bytes will be skipped (the "shift" below). + */ + +const struct spi_flash_table flashes[] = { + /* Keep it sorted by define name */ +#if CONFIG(SPI_FLASH_ADESTO) + { 0, VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, +#endif +#if CONFIG(SPI_FLASH_AMIC) + { 0, VENDOR_ID_AMIC, spi_flash_probe_amic, }, +#endif +#if CONFIG(SPI_FLASH_ATMEL) + { 0, VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, +#endif +#if CONFIG(SPI_FLASH_EON) + { 0, VENDOR_ID_EON, spi_flash_probe_eon, }, +#endif +#if CONFIG(SPI_FLASH_GIGADEVICE) + { 0, VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, +#endif +#if CONFIG(SPI_FLASH_MACRONIX) + { 0, VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, +#endif +#if CONFIG(SPI_FLASH_SPANSION) + { 0, VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, +#endif +#if CONFIG(SPI_FLASH_SST) + { 0, VENDOR_ID_SST, spi_flash_probe_sst, }, +#endif +#if CONFIG(SPI_FLASH_STMICRO) + { 0, VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, + { 0, VENDOR_ID_STMICRO_FF, spi_flash_probe_stmicro, }, +#endif +#if CONFIG(SPI_FLASH_WINBOND) + { 0, VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, +#endif + /* Keep it sorted by best detection */ +}; + +const struct spi_flash_table *get_spi_flash_table(int *table_size) +{ + *table_size = (int)ARRAY_SIZE(flashes); + return &flashes[0]; +}