Re: [flashrom] [PATCH 2/2] ichspi: add support for Intel Hardware Sequencing
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 <stefan.tauner@student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> with a few small comments.
--- 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.
+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; + i++; + } + return 1; +} + /* * Try to set BBAR (BIOS Base Address Register), but read back the value in case * it didn't stick. @@ -1325,6 +1525,14 @@ static const struct spi_programmer spi_programmer_ich9 = { .write_256 = default_spi_write_256, };
+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.
+}; + int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, int ich_generation) { @@ -1332,7 +1540,14 @@ 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 desc_valid = 0; + struct ich_descriptors desc = {{ 0 }}; + enum ich_spi_mode { + ich_auto, + ich_hwseq, + ich_swseq + } ich_spi_mode = ich_auto;
switch (ich_generation) { case 7: @@ -1399,6 +1614,22 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, case 9: case 10: default: /* Future version might behave the same */ + 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. */
+ free(arg); + tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS); msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2); prettyprint_ich9_reg_hsfs(tmp2); @@ -1484,14 +1715,41 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
msg_pdbg("\n"); if (desc_valid) { - struct ich_descriptors desc = {{ 0 }}; if (read_ich_descriptors_via_fdo(ich_spibar, &desc) == ICH_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 (ich_spi_mode == ich_auto && desc.content.NC != 0) { + msg_pinfo("Enabling hardware sequencing due to " + "multiple flash chips detected.\n"); + ich_spi_mode = ich_hwseq; + } + } + + if (ich_spi_mode == ich_auto && ichspi_lock && + !ich_check_opcodes()) { + msg_pinfo("Enabling hardware sequencing because " + "some important opcodes are locked.\n"); + ich_spi_mode = ich_hwseq; + } + + 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.
+ } + hwseq_data.size_comp0 = getFCBA_component_density(&desc, 0); + hwseq_data.size_comp1 = getFCBA_component_density(&desc, 1); + register_opaque_programmer(&opaque_programmer_ich_hwseq); + } else { + register_spi_programmer(&spi_programmer_ich9); } - register_spi_programmer(&spi_programmer_ich9); - ich_init_opcodes(); break; }
Regards, Carl-Daniel -- http://www.hailfinger.org/
On Mon, 07 Nov 2011 00:15:53 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> 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 <stefan.tauner@student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> 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
Am 07.11.2011 23:51 schrieb Stefan Tauner:
On Mon, 07 Nov 2011 00:15:53 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> 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 <stefan.tauner@student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> thanks, but ignored for now.
Hm, so if I ack this patch as well, will you commit one combined patch?
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, ...?)
Nice.
[…] + 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.
This code was indeed suboptimal. Thanks for fixing it!
[…] + 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).
Nice.
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.
I have a fix handy, I just didn't want to push such a patch in before hwseq because that would feel like jumping the review queue.
From 9d85001930afc54ca724d75dc80a7ca7e26c1b6d Mon Sep 17 00:00:00 2001 From: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> 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 <stefan.tauner@student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> with comments.
diff --git a/ichspi.c b/ichspi.c index 85456cd..3c3b7f1 100644 --- a/ichspi.c +++ b/ichspi.c @@ -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];
Can you return 1 (or -1 or some other nonzero value) instead? Returning one missing opcode reduces redability and provides no gain (other missing opcodes are ignored). Unless I'm mistaken, we'll see the available opcodes anyway in the verbose log.
i++; } - return 1; + return 0; }
/* @@ -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) {
ich_check_opcodes()) {
msg_pinfo("Enabling hardware sequencing because " - "some important opcodes are locked.\n"); + "opcode 0x%02x is inaccessible.\n", i);
Please keep the old message or at least replace "opcode 0x%02x" with "some important opcode".
ich_spi_mode = ich_hwseq; }
Regards, Carl-Daniel -- http://www.hailfinger.org/
On Tue, 08 Nov 2011 01:05:01 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
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];
Can you return 1 (or -1 or some other nonzero value) instead? Returning one missing opcode reduces redability and provides no gain (other missing opcodes are ignored). Unless I'm mistaken, we'll see the available opcodes anyway in the verbose log.
IF there is a verbose log, you are right (the error mentioning the missing opcode was msg_pinfo iirc). if not and flashrom bricks the computer we wont get far without a verbose log anyway :) i have also renamed the function as discussed on IRC.
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
\o/ eventually committed in r1461. there might be some things in the commit that you frowned upon before, but my reply to your mails remained unanswered. one example is mixing hex with decimal output in error messages. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Am 08.11.2011 11:59 schrieb Stefan Tauner:
On Tue, 08 Nov 2011 01:05:01 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> \o/
eventually committed in r1461.
Great, thank you for your persistence!
there might be some things in the commit that you frowned upon before, but my reply to your mails remained unanswered. one example is mixing hex with decimal output in error messages.
Sorry, I tried to reply to all mails, but I seem to have missed quite a few. The code is now in, and any remaining issues can be handled afterwards. Regards, Carl-Daniel -- http://www.hailfinger.org/
participants (2)
-
Carl-Daniel Hailfinger -
Stefan Tauner