On Mon, 07 Nov 2011 00:15:53 +0100 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 26.10.2011 15:35 schrieb Stefan Tauner: > > Based on the new opaque programmer framework this patch adds support > > for Intel Hardware Sequencing on ICH8 and its successors. > > > > By default (or when setting the ich_spi_mode option to auto) > > the module tries to use swseq and only activates hwseq if need be: > > - if important opcodes are inaccessible due to lockdown > > - if more than one flash chip is attached. > > The other options (swseq, hwseq) select the respective mode (if possible). > > > > A general description of Hardware Sequencing can be found in this blog > > entry: > > http://blogs.coreboot.org/blog/2011/06/11/gsoc-2011-flashrom-part-1/ > > > > TODO: adding real documentation when we have a directory for it > > > > Signed-off-by: Stefan Tauner <[email protected]> > > Acked-by: Carl-Daniel Hailfinger <[email protected]> thanks, but ignored for now. > with a few small comments. in contrast to those of course ;) > > --- > > flashrom.8 | 20 +++++ > > ichspi.c | 268 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 283 insertions(+), 5 deletions(-) > > > > diff --git a/flashrom.8 b/flashrom.8 > > index a8f4660..66cde4f 100644 > > --- a/flashrom.8 > > +++ b/flashrom.8 > > @@ -303,6 +303,26 @@ is the I/O port number (must be a multiple of 8). In > > the unlikely case > > flashrom doesn't detect an active IT87 LPC<->SPI bridge, please send a bug > > report so we can diagnose the problem. > > .sp > > +If you have an Intel chipset with an ICH8 or later southbridge with SPI > > flash > > +attached, and if a valid descriptor was written to it (e.g. by the > > vendor), the > > +chipset provides an alternative way to access the flash chip(s) named > > +.BR "Hardware Sequencing" . > > +It is much simpler than the normal access method (called > > +.BR "Software Sequencing" ")," > > +but does not allow the software to choose the SPI commands to be sent. > > +You can use the > > +.sp > > +.B " flashrom \-p internal:ich_spi_mode=value" > > +.sp > > +syntax where value can be > > +.BR auto ", " swseq " or " hwseq . > > +By default > > +.RB "(or when setting " ich_spi_mode=auto ) > > +the module tries to use swseq and only activates hwseq if need be (e.g. if > > +important opcodes are inaccessible due to lockdown; or if more than one > > flash > > +chip is attached). The other options (swseq, hwseq) select the respective > > mode > > +(if possible). > > +.sp > > If you have an Intel chipset with an ICH6 or later southbridge and if you > > want > > to set specific IDSEL values for a non-default flash chip or an embedded > > controller (EC), you can use the > > diff --git a/ichspi.c b/ichspi.c > > index bc03554..1d5dd34 100644 > > --- a/ichspi.c > > +++ b/ichspi.c > > @@ -575,6 +575,25 @@ static int program_opcodes(int ich_generation, OPCODES > > *op, int enable_undo) > > return 0; > > } > > > > +/* Returns true if the most important opcodes are accessible. */ > > You assume that some erase opcode will be available if BYTE_PROGRAM is > available. I think that assumption is reasonable, but it could be > documented in this comment above the function. this was just a basis for discussion actually. sorry for not explaining that bit. there are more problems than just the missing check for *any* available erase opcode. i do not intend to fix this soon. OTOH it is not really a problem imho. if this trigger would enable hwseq the bios has probably not only locked down the opcodes, but also set up PR or FREG/FRAP protections, which we can not deal with correctly yet, not even with hwseq. so *not* selecting hwseq due to a too simple ich_check_opcodes method will not hurt us, because it would not have saved us from failure anyway. i have reduced the method to the absolute minimum (see patch) and added the following comment: * FIXME: this should also check for * - at least one probing opcode (RDID (incl. AT25F variants?), REMS, RES?) * - at least one erasing opcode (lots.) * - at least one program opcode (BYTE_PROGRAM, AAI_WORD_PROGRAM, ...?) * - necessary preops? (EWSR, WREN, ...?) > > […] > > +static const struct opaque_programmer opaque_programmer_ich_hwseq = { > > + .max_data_read = 64, > > + .max_data_write = 64, > > + .probe = ich_hwseq_probe, > > + .read = ich_hwseq_read, > > + .write = ich_hwseq_write, > > Please add ich_hwseq_block_erase here. done > > […] > > + arg = extract_programmer_param("ich_spi_mode"); > > + if (arg && !strcmp(arg, "hwseq")) { > > + ich_spi_mode = ich_hwseq; > > + msg_pspew("user selected hwseq\n"); > > + } else if (arg && !strcmp(arg, "swseq")) { > > + ich_spi_mode = ich_swseq; > > + msg_pspew("user selected swseq\n"); > > + } else if (arg && !strcmp(arg, "auto")) { > > + msg_pspew("user selected auto\n"); > > + ich_spi_mode = ich_auto; > > + } else if (arg && !strlen(arg)) > > + msg_pinfo("Missing argument for ich_spi_mode.\n"); > > + else if (arg) > > + msg_pinfo("Unknown argument for ich_spi_mode: %s\n", > > arg); > > We should return an error all the way up to programmer init for both > cases (and clean up everything). I know that this is a complicated code > path, so if you decide not to fail programmer init here, please add a > comment like the one below: > /* FIXME: Return an error to programmer_init. */ fixing this was not that easy, because the ich code in chipset_enable.c was not passing the fatal error further; see patch. > > […] > > + if (ich_spi_mode == ich_hwseq) { > > + if (!desc_valid) { > > + msg_perr("Hardware sequencing was requested " > > + "but the flash descriptor is not " > > + "valid. Aborting.\n"); > > + return 1; > > Can you check that this indeed causes flashrom to return an error from > programmer_init? Chipset init IIRC ignores most errors unless they are > somehow declared to be fatal. done. and also if a bogus ich_gen is passed to ichspi_init at the beginning of the function. i am not sure if it was due to the changes to the previous patches, but ich_init_opcodes() were missing from the ich8+ code path (again). besides that i have also added null-pointer guards to find_opcode and find_preop, because i think it matches the other opcode methods better (curopcodes == NULL has some meaning and is actively used/checked in other functions). ps: you like to abort when the user gets the command line wrong for safety. there is a case we do not handle in this manner. for example with this patch set one can do: flashrom -p internal:laptop=force_I_want_a_brick,ich_spi_mode it prints "Unhandled programmer parameters: ich_spi_mode" but continues, you may wanna look into this. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From 9d85001930afc54ca724d75dc80a7ca7e26c1b6d Mon Sep 17 00:00:00 2001 From: Stefan Tauner <[email protected]> Date: Mon, 7 Nov 2011 21:06:36 +0100 Subject: [PATCH] squash! ichspi: add support for Intel Hardware Sequencing - Fix enable_flash_ich_dc_spi to pass ERROR_FATAL from ich_init_spi. The whole error handling looks a bit odd to me, so this patch does change very little. Also, it does not touch the tunnelcreek method, which should be refactored anyway. - Add null-pointer guards to find_opcode and find_preop to matches the other opcode methods better: curopcodes == NULL has some meaning and is actively used/checked in other functions. Signed-off-by: Stefan Tauner <[email protected]> --- chipset_enable.c | 12 +++++++----- ichspi.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/chipset_enable.c b/chipset_enable.c index 15bd3eb..77e0862 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -491,7 +491,7 @@ static int enable_flash_vt8237s_spi(struct pci_dev *dev, const char *name) static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, enum ich_chipset ich_generation) { - int ret; + int ret, ret_spi; uint8_t bbs, buc; uint32_t tmp, gcs; void *rcrb; @@ -569,10 +569,12 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, } /* This adds BUS_SPI */ - if (ich_init_spi(dev, tmp, rcrb, ich_generation) != 0) { - if (!ret) - ret = ERROR_NONFATAL; - } + ret_spi = ich_init_spi(dev, tmp, rcrb, ich_generation); + if (ret_spi == ERROR_FATAL) + return ret_spi; + + if (ret || ret_spi) + ret = ERROR_NONFATAL; return ret; } diff --git a/ichspi.c b/ichspi.c index 85456cd..3c3b7f1 100644 --- a/ichspi.c +++ b/ichspi.c @@ -423,6 +423,11 @@ static int find_opcode(OPCODES *op, uint8_t opcode) { int a; + if (op == NULL) { + msg_perr("\n%s: null OPCODES pointer!\n", __func__); + return -1; + } + for (a = 0; a < 8; a++) { if (op->opcode[a].opcode == opcode) return a; @@ -435,6 +440,11 @@ static int find_preop(OPCODES *op, uint8_t preop) { int a; + if (op == NULL) { + msg_perr("\n%s: null OPCODES pointer!\n", __func__); + return -1; + } + for (a = 0; a < 2; a++) { if (op->preop[a] == preop) return a; @@ -560,23 +570,29 @@ static int program_opcodes(OPCODES *op, int enable_undo) return 0; } -/* Returns true if the most important opcodes are accessible. */ +/* + * Checks if all of the most important opcodes are accessible. + * Returns 0 if they are, or the first opcode to be found inaccessible. + * FIXME: this should also check for + * - at least one probing opcode (RDID (incl. AT25F variants?), REMS, RES?) + * - at least one erasing opcode (lots.) + * - at least one program opcode (BYTE_PROGRAM, AAI_WORD_PROGRAM, ...?) + * - necessary preops? (EWSR, WREN, ...?) + */ static int ich_check_opcodes() { uint8_t ops[] = { JEDEC_READ, - JEDEC_BYTE_PROGRAM, JEDEC_RDSR, 0 }; int i = 0; while (ops[i] != 0) { - msg_pspew("checking for opcode 0x%02x\n", ops[i]); if (find_opcode(curopcodes, ops[i]) == -1) - return 0; + return ops[i]; i++; } - return 1; + return 0; } /* @@ -1506,6 +1522,7 @@ static const struct opaque_programmer opaque_programmer_ich_hwseq = { .probe = ich_hwseq_probe, .read = ich_hwseq_read, .write = ich_hwseq_write, + .erase = ich_hwseq_block_erase, }; int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, @@ -1528,7 +1545,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, switch (ich_generation) { case CHIPSET_ICH_UNKNOWN: - return -1; + return ERROR_FATAL; case CHIPSET_ICH7: case CHIPSET_ICH8: spibar_offset = 0x3020; @@ -1545,6 +1562,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, /* Assign Virtual Address */ ich_spibar = rcrb + spibar_offset; + ich_init_opcodes(); + switch (ich_generation) { case CHIPSET_ICH7: msg_pdbg("0x00: 0x%04x (SPIS)\n", @@ -1584,7 +1603,6 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, } ich_set_bbar(0); register_spi_programmer(&spi_programmer_ich7); - ich_init_opcodes(); break; case CHIPSET_ICH8: default: /* Future version might behave the same */ @@ -1598,10 +1616,16 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, } else if (arg && !strcmp(arg, "auto")) { msg_pspew("user selected auto\n"); ich_spi_mode = ich_auto; - } else if (arg && !strlen(arg)) - msg_pinfo("Missing argument for ich_spi_mode.\n"); - else if (arg) - msg_pinfo("Unknown argument for ich_spi_mode: %s\n", arg); + } else if (arg && !strlen(arg)) { + msg_perr("Missing argument for ich_spi_mode.\n"); + free(arg); + return ERROR_FATAL; + } else if (arg) { + msg_perr("Unknown argument for ich_spi_mode: %s\n", + arg); + free(arg); + return ERROR_FATAL; + } free(arg); tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS); @@ -1705,9 +1729,9 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, } if (ich_spi_mode == ich_auto && ichspi_lock && - !ich_check_opcodes()) { + (i = ich_check_opcodes()) != 0) { msg_pinfo("Enabling hardware sequencing because " - "some important opcodes are locked.\n"); + "opcode 0x%02x is inaccessible.\n", i); ich_spi_mode = ich_hwseq; } @@ -1716,7 +1740,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, msg_perr("Hardware sequencing was requested " "but the flash descriptor is not " "valid. Aborting.\n"); - return 1; + return ERROR_FATAL; } hwseq_data.size_comp0 = getFCBA_component_density(&desc, 0); hwseq_data.size_comp1 = getFCBA_component_density(&desc, 1); -- 1.7.1
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
