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

[Carl-Daniel Hailfinger]

Am 21.10.2011 00:39 schrieb Stefan Tauner:
> Based on the new opaque programmer framework this patch adds support
> for Intel Hardware Sequencing on ICH8 and its successors.
>
> If the flash descriptor is valid, one can activate Hardware
> Sequencing by using the hwseq option of the internal programmer,
> example: flashrom -p internal:hwseq=yes
>
> 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:
> man page
> adding real documentation when we have a directory for it
>
> Signed-off-by: Stefan Tauner

I really like the paranoid sanity checks everywhere in the code.
There is one small comment near the end of the patch, but it is
informational and not holding back your patch at all.
Acked-by: Carl-Daniel Hailfinger

I am so glad we can finally merge this code.
A big THANK YOU to Stefan Tauner. This is probably the most reworked
patch which ever hit our tree, and in the process flashrom got a lot
more extensible as well.
> ichspi.c |  231 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 226 insertions(+), 5 deletions(-)
>
> diff --git a/ichspi.c b/ichspi.c
> index afa420b..ce1fff9 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -1081,7 +1081,11 @@ static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    return result;
>  }
>
> -#if 0
> +static struct hwseq {
> uint32_t size_comp0;
> uint32_t size_comp1;
>
> +} hwseq;
>
> /* Sets FLA in FADDR to (addr & 0x01FFFFFF) without touching other bits. */
>  static void ich_hwseq_set_addr(uint32_t addr)
>  {
> @@ -1150,7 +1154,184 @@ static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout,
>    }
>    return 0;
>  }
> -#endif
>
> +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;
> 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);
> }
> flash->tested = TEST_OK_PREW;
> 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 */
> 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 (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, opaque_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(struct flashchip *flash, uint8_t *buf, int addr, int len)
> +{
> uint16_t hsfc;
> uint16_t timeout = 100 * 60;
> uint8_t block_len;
> 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, opaque_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)
>  {
> @@ -1315,6 +1496,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,
>
> +};
>
> int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    		int ich_generation)
>  {
> @@ -1322,7 +1511,10 @@ 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;
>
> int hwseq_en = 0;
>
> struct ich_descriptors desc = {{ 0 }};
>
> switch (ich_generation) {
> case 7:
>
> @@ -1389,6 +1581,14 @@ 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("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);
> tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS);
> msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2);
> prettyprint_ich9_reg_hsfs(tmp2);
>
> @@ -1474,14 +1674,35 @@ 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 (desc.content.NC != 0) {
> 		msg_pinfo("Enabling hardware sequencing due to "
> 			  "multiple flash chips detected.\n");
> 		hwseq_en = 1;
> 	}
> }
> register_spi_programmer(&spi_programmer_ich9);
> ich_init_opcodes();
> if (hwseq_en) {
> 	if (!desc_valid) {
> 		msg_perr("Hardware sequencing was requested "
> 			 "but the flash descriptor is not "
> 			 "valid. Aborting.\n");
> 		return 1;
> 	}
> 	hwseq.size_comp0 = getFCBA_component_density(&desc, 0);
> 	hwseq.size_comp1 = getFCBA_component_density(&desc, 1);
> 	register_opaque_programmer(&opaque_programmer_ich_hwseq);

This is a big question. Should we only register hwseq, or should we
register hwseq in addition to swseq? Both solutions make sense, and I'm
not yet sure which one is better. There's the user interface issue for
non-experts which would benefit from having only one (virtual) chip
visible, and there's the desire to access at least one flash chip with
swseq even if multiple chips are present. This question should not block
a merge of this patch, but we need to address it eventually.
> } else {
> 	register_spi_programmer(&spi_programmer_ich9);
> 	ich_init_opcodes();
> }
> break;
> }