Am 20.08.2011 12:39 schrieb Stefan Tauner:
todo:
- revise hwseq activation: when should hwseq be used, when swseq? in the current version i have added a programmer parameter to tell flashrom when one wants to use hwseq _if it is available_ (i.e. descriptor valid), example:
flashrom -p internal:laptop=force_I_want_a_brick,hwseq=yes -VV this needs to be documented in the man page if it gets merged like that.
- commit message (is a link to my blog post a good idea (additionally)?)
We have no limits for the commit message size, but maybe we should start to package additional documentation with flashrom?
- revise (levels of) prints
- revise timeout values (for optimal performance)
- build upon the upcoming opaque interface framework instead
I see all those TODOs, and most of them should be addressed before enabling the code. That's why I'd like to get most of this patch merged really soon except for the few places where the new code is hooked up. That way we get a much smaller remaining patch and the discussion about user interfaces and infrastructure are not attached to a megapatch, but rather to a small "hook up hwseq" patch.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
chipdrivers.h | 6 + flashchips.c | 23 ++++- flashchips.h | 2 + ich_descriptors.c | 35 +++++ ich_descriptors.h | 3 +- ichspi.c | 357 +++++++++++++++++++++++++++++++++++++++++++++++++++-- programmer.h | 1 + 7 files changed, 414 insertions(+), 13 deletions(-)
diff --git a/chipdrivers.h b/chipdrivers.h index 3f5b503..f3781ae 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -86,6 +86,12 @@ void print_status_82802ab(uint8_t status); int unlock_82802ab(struct flashchip *flash); int unlock_28f004s5(struct flashchip *flash);
+/* ichspi.c */ +int ich_hwseq_probe(struct flashchip *flash); +int ich_hwseq_read(struct flashchip *flash, uint8_t *buf, int start, int len); +int ich_hwseq_block_erase(struct flashchip *flash, unsigned int addr, unsigned int blocklen); +int ich_hwseq_write_256(struct flashchip *flash, uint8_t *buf, int start, int len);
Postpone this hunk for the hookup patch.
/* jedec.c */ uint8_t oddparity(uint8_t val); void toggle_ready_jedec(chipaddr dst); diff --git a/flashchips.c b/flashchips.c index 4d741a0..20f7197 100644 --- a/flashchips.c +++ b/flashchips.c @@ -8782,7 +8782,27 @@ const struct flashchip flashchips[] = { .read = read_memmapped, .voltage = {3000, 3600}, /* Also has 12V fast program */ },
+#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
- {
.vendor = "Intel",.name = "Hardware Sequencing",.bustype = BUS_SPI,.manufacture_id = INTEL_ID,.model_id = INTEL_HWSEQ,.total_size = 0,.page_size = 256,.tested = TEST_OK_PREW,.probe = ich_hwseq_probe,.block_erasers ={{ /* erase blocks will be set by the probing function */.block_erase = ich_hwseq_block_erase,}},.write = ich_hwseq_write_256,.read = ich_hwseq_read,- },
+#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
Postpone this hunk for the hookup patch.
{ .vendor = "AMIC", .name = "unknown AMIC SPI chip", @@ -8915,6 +8935,7 @@ const struct flashchip flashchips[] = { .probe = probe_spi_rdid, .write = NULL, },
- { .vendor = "Generic", .name = "unknown SPI chip (REMS)",
diff --git a/flashchips.h b/flashchips.h index ff49d31..1374b2f 100644 --- a/flashchips.h +++ b/flashchips.h @@ -346,6 +346,8 @@ #define SHARP_LH28F008SA 0xA2 /* Sharp chip, Intel Vendor ID */ #define SHARP_LH28F008SC 0xA6 /* Sharp chip, Intel Vendor ID */
+#define INTEL_HWSEQ 0xFFFE /* dummy ID for hardware sequencing */
Postpone...
#define ISSI_ID 0xD5 /* ISSI Integrated Silicon Solutions */
/* diff --git a/ich_descriptors.c b/ich_descriptors.c index ce40d9f..8076b09 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -214,6 +214,41 @@ void prettyprint_ich_descriptor_master(const struct ich_desc_master *mstr) msg_pdbg2("\n"); }
+/** Returns the integer representation of the component density with index +idx in bytes or 0 if a correct size can not be determined. */ +int getFCBA_component_density(uint8_t idx, const struct ich_descriptors *desc)
I'd prefer to have the order (desc, idx) for the parameters here.
+{
- uint8_t size_enc;
- static const int const dec_mem[6] = {
512 * 1024,1 * 1024 * 1024,2 * 1024 * 1024,4 * 1024 * 1024,8 * 1024 * 1024,16 * 1024 * 1024,- };
Kill dec_mem and replace dec_mem[size_enc] with (1<<(19+size_enc)).
- switch(idx) {
- case 0:
size_enc = desc->component.comp1_density;break;- case 1:
if (desc->content.NC == 0)
If NC is shorthand for "not connected", this looks like inverted logic.
return 0;size_enc = desc->component.comp2_density;break;- default:
msg_perr("Only component index 0 or 1 are supported yet.\n");
More specific error message: "Only ICH SPI component..."
return 0;- }
- if (size_enc > 5) {
msg_perr("Density of component with index %d is invalid. "
Same error message change here.
"Encoded density is 0x%x.\n", idx, size_enc);return 0;- }
- return dec_mem[size_enc];
+}
static uint32_t read_descriptor_reg(uint8_t section, uint16_t offset, void *spibar) { uint32_t control = 0; diff --git a/ich_descriptors.h b/ich_descriptors.h index b1da914..8ab798f 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -255,7 +255,8 @@ void prettyprint_ich_descriptor_region(const struct ich_descriptors *desc); void prettyprint_ich_descriptor_master(const struct ich_desc_master *master);
int read_ich_descriptors_via_fdo(void *spibar, struct ich_descriptors *desc);
+int getFCBA_component_density(uint8_t idx, const struct ich_descriptors *desc);
#endif /* BITFIELDS == 1 */ #endif /* __ICH_DESCRIPTORS_H__ */ #endif /* defined(__i386__) || defined(__x86_64__) */ diff --git a/ichspi.c b/ichspi.c index 2e25d34..61c0825 100644 --- a/ichspi.c +++ b/ichspi.c @@ -26,7 +26,10 @@ #if defined(__i386__) || defined(__x86_64__)
#include <string.h> +#include <stdlib.h> #include "flash.h" +#include "flashchips.h" +#include "chipdrivers.h"
Including flashchips.h or chipdrivers.h from a programmer driver is a sign that something went wrong during the design phase.
#include "programmer.h" #include "spi.h" #include "ich_descriptors.h" @@ -1084,6 +1087,298 @@ static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt, return result; }
+static struct hwseq {
- uint32_t size_comp0;
- uint32_t size_comp1;
+} hwseq;
+static void ich_hwseq_set_addr(uint32_t addr) +{
- uint32_t addr_old = REGREAD32(ICH9_REG_FADDR) & ~0x01FFFFFF;
- REGWRITE32(ICH9_REG_FADDR, (addr & 0x01FFFFFF) | addr_old);
Can this write fail somehow?
+}
+/* Sets FADDR.FLA to 'addr' and returns the erase block size in bytes
- of the block containing this address. */
Is it really possible to encode per-region block sizes in a descriptor?
+static uint32_t ich_hwseq_get_erase_block_size(unsigned int addr) +{
- uint8_t enc_berase;
- static const uint32_t const dec_berase[4] = {
decode_ instead of dec_. dec_ sounds too much like decrement. Or you pick a completely different name, e.g. eraseblock_sizes.
256,4 * 1024,8 * 1024,64 * 1024- };
- ich_hwseq_set_addr(addr);
- enc_berase = (REGREAD16(ICH9_REG_HSFS) & HSFS_BERASE) >>
HSFS_BERASE only has two adjacent bits set, correct?
HSFS_BERASE_OFF;- return dec_berase[enc_berase];
+}
+/* Polls for Cycle Done Status, Flash Cycle Error or timeout in 10 us intervals.
Not something to be addressed in this patch, but we should really think about exponential backoff with maximum wait for such wait-for-hardware calls.
- Resets all error flags in HSFS.
- Returns 0 if the cycle completes successfully without errors within
- timeout us, 1 on errors. */
+static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
Can we specify the timeout in usecs instead and use programmer_delay(8) to avoid division overhead (use shift instead)?
unsigned int len)+{
- uint16_t hsfs;
- uint32_t addr;
- while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) &
(HSFS_FDONE | HSFS_FCERR)) == 0) &&
Can we really ignore AEL here?
--timeout) {programmer_delay(10);- }
- REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
This clears all errors. OK.
- if (!timeout) {
addr = REGREAD32(ICH9_REG_FADDR) & 0x01FFFFFF;msg_perr("Timeout error between offset 0x%08x and ""0x%08x + %d (=0x%08x)!\n",
Please don't mix hex and decimal encoding.
addr, addr, len - 1, addr + len - 1);prettyprint_ich9_reg_hsfs(hsfs);prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));return 1;- }
- if (hsfs & HSFS_FCERR) {
addr = REGREAD32(ICH9_REG_FADDR) & 0x01FFFFFF;msg_perr("Transaction error between offset 0x%08x and ""0x%08x (=0x%08x + %d)!\n",
hex/decimal mixture.
addr, addr + len - 1, addr, len - 1);prettyprint_ich9_reg_hsfs(hsfs);prettyprint_ich9_reg_hsfc(REGREAD16(ICH9_REG_HSFC));return 1;- }
- return 0;
+}
ich_hwseq_probe, ich_hwseq_send_command, ich_hwseq_block_erase, ich_hwseq_read and ich_hwseq_write256 use an interface which does not fit well with flashrom. Could you cut out those functions and post them separately, preferably as part of the hookup patch I mentioned above?
+int ich_hwseq_probe(struct flashchip *flash) +{
- uint32_t total_size, boundary;
- uint32_t erase_size_low, size_low, erase_size_high, size_high;
- struct block_eraser *eraser;
- if (flash->manufacture_id != INTEL_ID ||
flash->model_id != INTEL_HWSEQ) {msg_cerr("This chip (%s) is not supported in hardware""sequencing mode and should never have been probed.\n",flash->name);msg_cerr("%s: Please report a bug at flashrom@flashrom.org\n",__func__);return 0;- }
- msg_cdbg("Prerequisites for Intel Hardware Sequencing are ");
- if (spi_programmer->type != SPI_CONTROLLER_ICH_HWSEQ) {
msg_cdbg("not met.\n");return 0;- }
- msg_cdbg("met.\n");
- total_size = hwseq.size_comp0 + hwseq.size_comp1;
- msg_cdbg("Found %d attached SPI flash chip",
(hwseq.size_comp1 != 0) ? 2 : 1);- if (hwseq.size_comp1 != 0)
msg_cdbg("s with a combined");- else
msg_cdbg(" with a");- msg_cdbg(" density of %d kB.\n", total_size / 1024);
- flash->total_size = total_size / 1024;
- eraser = &(flash->block_erasers[0]);
- boundary = (REGREAD32(ICH9_REG_FPB) & FPB_FPBA) << 12;
- size_high = total_size - boundary;
- erase_size_high = ich_hwseq_get_erase_block_size(boundary);
- if (boundary == 0) {
msg_cdbg("There is only one partition containing the whole ""address space (0x%06x - 0x%06x).\n", 0, size_high-1);eraser->eraseblocks[0].size = erase_size_high;eraser->eraseblocks[0].count = size_high / erase_size_high;msg_cdbg("There are %d erase blocks with %d B each.\n",size_high / erase_size_high, erase_size_high);- } else {
msg_cdbg("The flash address space (0x%06x - 0x%06x) is divided ""at address 0x%06x in two partitions.\n",0, size_high-1, boundary);size_low = total_size - size_high;erase_size_low = ich_hwseq_get_erase_block_size(0);eraser->eraseblocks[0].size = erase_size_low;eraser->eraseblocks[0].count = size_low / erase_size_low;msg_cdbg("The first partition ranges from 0x%06x to 0x%06x.\n",0, size_low-1);msg_cdbg("In that range are %d erase blocks with %d B each.\n",size_low / erase_size_low, erase_size_low);eraser->eraseblocks[1].size = erase_size_high;eraser->eraseblocks[1].count = size_high / erase_size_high;msg_cdbg("The second partition ranges from 0x%06x to 0x%06x.\n",boundary, size_high-1);msg_cdbg("In that range are %d erase blocks with %d B each.\n",size_high / erase_size_high, erase_size_high);- }
- return 1;
+}
+static int ich_hwseq_send_command(unsigned int writecnt,
unsigned int readcnt,const unsigned char *writearr,unsigned char *readarr)+{
- msg_pdbg("skipped. Intel Hardware Sequencing does not support sending "
"arbitrary commands.");- return -1;
+}
+int ich_hwseq_block_erase(struct flashchip *flash,
unsigned int addr,unsigned int len)+{
- uint32_t erase_block;
- uint16_t hsfc;
- uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */
- if (flash->manufacture_id != INTEL_ID ||
flash->model_id != INTEL_HWSEQ) {msg_perr("This chip (%s) is not supported in hardware""sequencing mode\n", flash->name);return -1;- }
- erase_block = ich_hwseq_get_erase_block_size(addr);
- if (len != erase_block) {
msg_cerr("Erase block size for address 0x%06x is %d B, ""but requested erase block size is %d B. ""Not erasing anything.\n", addr, erase_block, len);return -1;- }
- /* Although the hardware supports this (it would erase the whole block
* containing the address) we play safe here. */- if (addr % erase_block != 0) {
msg_cerr("Erase address 0x%06x is not aligned to the erase ""block boundary (any multiple of %d). ""Not erasing anything.\n", addr, erase_block);return -1;- }
- if (addr + len > flash->total_size * 1024) {
msg_perr("Request to erase some inaccessible memory address(es)"" (addr=0x%x, len=%d). ""Not erasing anything.\n", addr, len);return -1;- }
- msg_pdbg("Erasing %d bytes starting at 0x%06x.\n", len, addr);
- /* make sure FDONE, FCERR, AEL are cleared by writing 1 to them */
- REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
- hsfc = REGREAD16(ICH9_REG_HSFC);
- hsfc &= ~HSFC_FCYCLE; /* clear operation */
- hsfc |= (0x3 << HSFC_FCYCLE_OFF); /* set erase operation */
- hsfc |= HSFC_FGO; /* start */
- msg_pdbg("HSFC used for block erasing: ");
- prettyprint_ich9_reg_hsfc(hsfc);
- REGWRITE16(ICH9_REG_HSFC, hsfc);
- if (ich_hwseq_wait_for_cycle_complete(timeout, len))
return -1;- return 0;
+}
+int ich_hwseq_read(struct flashchip *flash, uint8_t *buf, int addr, int len) +{
- uint16_t hsfc;
- uint16_t timeout = 100 * 60;
- uint8_t block_len;
- if (flash->manufacture_id != INTEL_ID ||
flash->model_id != INTEL_HWSEQ) {msg_perr("This chip (%s) is not supported in hardware""sequencing mode.\n", flash->name);return -1;- }
- if (addr < 0 || addr + len > flash->total_size * 1024) {
msg_perr("Request to read from an inaccessible memory address ""(addr=0x%x, len=%d).\n", addr, len);return -1;- }
- msg_pdbg("Reading %d bytes starting at 0x%06x.\n", len, addr);
- /* clear FDONE, FCERR, AEL by writing 1 to them (if they are set) */
- REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
- while (len > 0) {
block_len = min(len, spi_programmer->max_data_read);ich_hwseq_set_addr(addr);hsfc = REGREAD16(ICH9_REG_HSFC);hsfc &= ~HSFC_FCYCLE; /* set read operation */hsfc &= ~HSFC_FDBC; /* clear byte count *//* set byte count */hsfc |= (((block_len - 1) << HSFC_FDBC_OFF) & HSFC_FDBC);hsfc |= HSFC_FGO; /* start */REGWRITE16(ICH9_REG_HSFC, hsfc);if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))return 1;ich_read_data(buf, block_len, ICH9_REG_FDATA0);addr += block_len;buf += block_len;len -= block_len;- }
- return 0;
+}
+int ich_hwseq_write_256(struct flashchip *flash, uint8_t *buf, int addr, int len) +{
- uint16_t hsfc;
- uint16_t timeout = 100 * 60;
- uint8_t block_len;
- if (flash->manufacture_id != INTEL_ID ||
flash->model_id != INTEL_HWSEQ) {msg_perr("This chip (%s) is not supported in hardware""sequencing mode\n", flash->name);return -1;- }
- if (addr < 0 || addr + len > flash->total_size * 1024) {
msg_perr("Request to write to an inaccessible memory address ""(addr=0x%x, len=%d).\n", addr, len);return -1;- }
- msg_pdbg("Writing %d bytes starting at 0x%06x.\n", len, addr);
- /* clear FDONE, FCERR, AEL by writing 1 to them (if they are set) */
- REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
- while (len > 0) {
ich_hwseq_set_addr(addr);block_len = min(len, spi_programmer->max_data_write);ich_fill_data(buf, block_len, ICH9_REG_FDATA0);hsfc = REGREAD16(ICH9_REG_HSFC);hsfc &= ~HSFC_FCYCLE; /* clear operation */hsfc |= (0x2 << HSFC_FCYCLE_OFF); /* set write operation */hsfc &= ~HSFC_FDBC; /* clear byte count *//* set byte count */hsfc |= (((block_len - 1) << HSFC_FDBC_OFF) & HSFC_FDBC);hsfc |= HSFC_FGO; /* start */REGWRITE16(ICH9_REG_HSFC, hsfc);if (ich_hwseq_wait_for_cycle_complete(timeout, block_len))return -1;addr += block_len;buf += block_len;len -= block_len;- }
- return 0;
+}
static int ich_spi_send_multicommand(struct spi_command *cmds) { int ret = 0; @@ -1207,6 +1502,16 @@ static const struct spi_programmer spi_programmer_ich9 = { .write_256 = default_spi_write_256, };
+static const struct spi_programmer spi_programmer_ich_hwseq = {
- .type = SPI_CONTROLLER_ICH_HWSEQ,
- .max_data_read = 64,
- .max_data_write = 64,
- .command = ich_hwseq_send_command,
- .multicommand = default_spi_send_multicommand,
- .read = ich_hwseq_read,
- .write_256 = ich_hwseq_write_256,
+};
Move the hunk above to the hookup patch as well.
int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, int ich_generation) { @@ -1214,21 +1519,20 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, uint8_t old, new; uint16_t spibar_offset, tmp2; uint32_t tmp;
char *arg; int ichspi_desc = 0;
int hwseq_en = 0;
switch (ich_generation) { case 7:
register_spi_programmer(&spi_programmer_ich7);register_spi_programmer(&spi_programmer_ich9);register_spi_programmer(&spi_programmer_ich9);@@ -1239,8 +1543,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, /* Assign Virtual Address */ ich_spibar = rcrb + spibar_offset;
- switch (spi_programmer->type) {
- case SPI_CONTROLLER_ICH7:
- switch (ich_generation) {
- case 7:
Please make sure the VIA driver doesn't explode with this.
msg_pdbg("0x00: 0x%04x (SPIS)\n", mmio_readw(ich_spibar + 0)); msg_pdbg("0x02: 0x%04x (SPIC)\n",@@ -1277,9 +1581,21 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, ichspi_lock = 1; } ich_set_bbar(ich_generation, 0);
register_spi_programmer(&spi_programmer_ich7);
- case SPI_CONTROLLER_ICH9:
- case 8:
- case 9:
- case 10:
- default: /* Future version might behave the same */
OK until here.
arg = extract_programmer_param("hwseq");if (arg && !strcmp(arg, "yes"))hwseq_en = 1;else if (arg && !strlen(arg))msg_pinfo("Missing argument for hwseq.\n");else if (arg)msg_pinfo("Unknown argument for hwseq: %s\n", arg);free(arg);
Please move the programmer parameter evaluation to the hookup patch.
tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS); msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2); prettyprint_ich9_reg_hsfs(tmp2);@@ -1363,18 +1679,37 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
msg_pdbg("\n");#if BITFIELDS == 1
#if BITFIELDS needs to die.
{ /* to be able to declare desc here and avoid another* "#if BITFIELDS == 1" on top*/struct ich_descriptors desc = {{ 0 }};
Maybe rename ichspi_desc to ichspi_desc_active or ichspi_desc_enabled.
struct ich_descriptors desc = {{ 0 }}; if (read_ich_descriptors_via_fdo(ich_spibar, &desc) == RET_OK) prettyprint_ich_descriptors(CHIPSET_ICH_UNKNOWN, &desc);
/* If the descriptor is valid and indicates multiple* flash devices we need to use hwseq to be able to* access the second flash device.*/if (desc.content.NC != 0)hwseq_en = 1;} elsehwseq_en = 0; /* disable it even if the user said yes */
This case needs at least a msg_pinfo, maybe even msg_perr. We act contrary to the wishes of the user.
if (hwseq_en > 0) {hwseq.size_comp0 = getFCBA_component_density(0, &desc);hwseq.size_comp1 = getFCBA_component_density(1, &desc);register_spi_programmer(&spi_programmer_ich_hwseq);
Please move register_spi_programmer to the hookup patch. The rest of the code of this hunk should stay in this patch.
} else {register_spi_programmer(&spi_programmer_ich9);ich_init_opcodes();-#endif /* BITFIELDS == 1 */
}+#else /* BITFIELDS == 1 */
Once you remove the #ifdef, the hunk below will probably die completely.
register_spi_programmer(&spi_programmer_ich9);
break;- default:
/* Nothing */+#endif /* BITFIELDS == 1 */ break; }
diff --git a/programmer.h b/programmer.h index 6a28dbe..6e316d1 100644 --- a/programmer.h +++ b/programmer.h @@ -517,6 +517,7 @@ enum spi_controller { #if defined(__i386__) || defined(__x86_64__) SPI_CONTROLLER_ICH7, SPI_CONTROLLER_ICH9,
- SPI_CONTROLLER_ICH_HWSEQ,
Please move this to the hookup patch.
SPI_CONTROLLER_IT85XX, SPI_CONTROLLER_IT87XX, SPI_CONTROLLER_SB600,
Regards, Carl-Daniel