[flashrom] [PATCH 2/2] ichspi: add support for Intel Hardware Sequencing
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Nov 7 00:15:53 CET 2011
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 at student.tuwien.ac.at>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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/
More information about the flashrom
mailing list