Re: [flashrom] [PATCH 2/2] ichspi: add support for Intel Hardware Sequencing

[Carl-Daniel Hailfinger]

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

Acked-by: Carl-Daniel Hailfinger 
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