Re: [flashrom] [PATCH 4/8] Add support for Atmel's AT25F series of SPI flash chips. This includes a new probing method (probe_spi_rdid_at25f), block erase method (spi_block_erase_62), spi_prettyprint_status_register_at25f and spi_disable_blockprotect_at25f.

[Carl-Daniel Hailfinger]

Hi Stefan,

thanks for your patch.

As you found out, we do have a few architectural changes in locking 
ahead of us, and the best we can do until then is to print the decoded 
status register (.printlock) and unlock the whole chip (.unlock). The 
longterm goal is to abuse .printlock (probably renamed to .detectlock) 
to gather locking info for some flashrom internal data structure, and to 
abuse .unlock (probably renamed to .changelock) to lock and unlock 
regions of the flash chip. A generic lockprinting function will then 
simply display user-readable output for the data stored internally once 
.printlock has run.

You might think that writing printlock functions is a waste of time 
considering the long-term design plans, but they actually make it a lot 
easier to write a proper detectlock and changelock routine, I'd even say 
that a conversion to detectlock/changelock becomes straightforward if 
you already have readable code for lock interpretation.

Comments on the patch are inline.

Am 15.03.2011 16:29 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index dc46fe1..09dd751 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -27,6 +27,7 @@
>
> /* spi.c, should probably be in spi_chip.c */
>   int probe_spi_rdid(struct flashchip *flash);
> +int probe_spi_rdid_at25f(struct flashchip *flash);

That name is a bit unfortunate, it's not really related to the command 
we know as RDID. That said, the vendor datasheet IIRC mentions the name 
RDID, so naming is not totally obvious. A name like probe_spi_at25f 
might be less misleading. Then again, my original naming choice for the 
command definitions in spi.h has the same problem.
Thoughts?
> int probe_spi_rdid4(struct flashchip *flash);
>   int probe_spi_rems(struct flashchip *flash);
>   int probe_spi_res1(struct flashchip *flash);
> @@ -37,6 +38,7 @@ int spi_block_erase_20(struct flashchip *flash, unsigned int addr, unsigned int
>   int spi_block_erase_52(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
>   int spi_block_erase_d7(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
>   int spi_block_erase_d8(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
> +int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
>   int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
>   int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen);
>   int spi_chip_write_1(struct flashchip *flash, uint8_t *buf, int start, int len);
> @@ -45,12 +47,14 @@ int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len);
>   uint8_t spi_read_status_register(void);
>   int spi_prettyprint_status_register_at25df(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash);
> +int spi_prettyprint_status_register_at25f(struct flashchip *flash);

AFAICS that hunk depends on an earlier patch of yours.
> int spi_prettyprint_status_register_at25f512b(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25fs010(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25fs040(struct flashchip *flash);
>   int spi_disable_blockprotect(struct flashchip *flash);
>   int spi_disable_blockprotect_at25df(struct flashchip *flash);
>   int spi_disable_blockprotect_at25df_sec(struct flashchip *flash);
> +int spi_disable_blockprotect_at25f(struct flashchip *flash);
>   int spi_disable_blockprotect_at25fs010(struct flashchip *flash);
>   int spi_disable_blockprotect_at25fs040(struct flashchip *flash);
>   int spi_byte_program(int addr, uint8_t databyte);
> diff --git a/flashchips.c b/flashchips.c
> index 29a4da0..7cb7d39 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -1583,6 +1583,63 @@ struct flashchip flashchips[] = {
>
> {
>    	.vendor		= "Atmel",
> .name		= "AT25F512",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
> .model_id	= ATMEL_AT25F512,
> .total_size	= 64,
> .page_size	= 256,

This may be correct, but it looks odd... does AT25F512 really have a 
bigger page_size for writes than AT25F512A?
> .feature_bits	= FEATURE_WRSR_WREN,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid_at25f,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { {32 * 1024, 2} },
> 		.block_erase = spi_block_erase_52,
> 	}, {
> 		.eraseblocks = { {64 * 1024, 1} },
> 		.block_erase = spi_block_erase_62,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_at25f,
> .unlock		= spi_disable_blockprotect_at25f,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read,
> },
> {
> .vendor		= "Atmel",
> .name		= "AT25F512A",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
> .model_id	= ATMEL_AT25F512A,
> .total_size	= 64,
> .page_size	= 128,
> .feature_bits	= FEATURE_WRSR_WREN,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid_at25f,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { {32 * 1024, 2} },
> 		.block_erase = spi_block_erase_52,
> 	}, {
> 		.eraseblocks = { {64 * 1024, 1} },
> 		.block_erase = spi_block_erase_62,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_at25f,
> /* Not correct to use this one, because the BP1 bit is N/A. */
> .unlock		= spi_disable_blockprotect_at25f,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read,
> },
> {
> .vendor		= "Atmel",
> .name		= "AT25F512B",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
>
> @@ -1622,6 +1679,36 @@ struct flashchip flashchips[] = {
>
> {
>    	.vendor		= "Atmel",
> /* The A suffix indicates 33MHz instead of 20MHz clock rate.
>  * All other properties seem to be the same.*/
> .name		= "AT25F1024(A)",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
> .model_id	= ATMEL_AT25F1024,
> .total_size	= 128,
> .page_size	= 256,
> .feature_bits	= FEATURE_WRSR_WREN,
> .tested		= TEST_OK_PREW,
> .probe		= probe_spi_rdid_at25f,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { {32 * 1024, 4} },
> 		.block_erase = spi_block_erase_52,
> 	}, {
> 		.eraseblocks = { {128 * 1024, 1} },
> 		.block_erase = spi_block_erase_62,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_at25f,
> .unlock		= spi_disable_blockprotect_at25f,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read,
> },
> {
> .vendor		= "Atmel",
> .name		= "AT25FS010",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
>
> diff --git a/flashchips.h b/flashchips.h
> index 3b2b94f..9b08d25 100644
> --- a/flashchips.h
> +++ b/flashchips.h
> @@ -134,13 +134,11 @@
>   #define ATMEL_AT25DF321A	0x4701
>   #define ATMEL_AT25DF641		0x4800
>   #define ATMEL_AT25DQ161		0x8600
> -#define ATMEL_AT25F512		/* No device ID found in datasheet. Vendor ID
> 		 * can be read with AT25F512A_RDID */
>
> -#define ATMEL_AT25F512A		0x65 /* Needs AT25F512A_RDID */
> +#define ATMEL_AT25F512		0x65	/* guessed, no device ID in datasheet.
> 			 * Vendor ID can be read with AT25F_RDID */
>
> +#define ATMEL_AT25F512A		0x65 /* Needs AT25F_RDID */
>   #define ATMEL_AT25F512B		0x6500
> -#define ATMEL_AT25F1024		/* No device ID found in datasheet. Vendor ID
> 		 * can be read with AT25F512A_RDID */
>
> -#define ATMEL_AT25F1024A		0x60 /* Needs AT25F512A_RDID */
> +#define ATMEL_AT25F1024		0x60 /* Needs AT25F_RDID */

That's a bit odd. You merge AT25F1024 and AT25F1024A into one 
definition, but you keep AT25F512 and AT25F512A separate.
> #define ATMEL_AT25FS010		0x6601
>   #define ATMEL_AT25FS040		0x6604
>   #define ATMEL_AT26DF041		0x4400
> diff --git a/spi.h b/spi.h
> index b908603..a5c3406 100644
> --- a/spi.h
> +++ b/spi.h
> @@ -30,10 +30,11 @@
>   /* INSIZE may be 0x04 for some chips*/
>   #define JEDEC_RDID_INSIZE	0x03
>
> -/* AT25F512A has bit 3 as don't care bit in commands */
> -#define AT25F512A_RDID		0x15	/* 0x15 or 0x1d */
> -#define AT25F512A_RDID_OUTSIZE	0x01
> -#define AT25F512A_RDID_INSIZE	0x02
> +/* Some Atmel AT25F* models have bit 3 as don't care bit in commands */
> +/* 0x15 or 0x1d */
> +#define AT25F_RDID		0x15
> +#define AT25F_RDID_OUTSIZE	0x01
> +#define AT25F_RDID_INSIZE	0x02

See the naming discussion near the beginning of my mail.
> /* Read Electronic Manufacturer Signature */
>   #define JEDEC_REMS		0x90
> @@ -61,6 +62,11 @@
>   #define JEDEC_CE_60_OUTSIZE	0x01
>   #define JEDEC_CE_60_INSIZE	0x00
>
> +/* Chip Erase 0x62 is supported by Atmel AT25F chips. */
> +#define JEDEC_CE_62		0x62
> +#define JEDEC_CE_62_OUTSIZE	0x01
> +#define JEDEC_CE_62_INSIZE	0x00
> /* Chip Erase 0xc7 is supported by SST/ST/EON/Macronix chips. */
> #define JEDEC_CE_C7		0xc7
> #define JEDEC_CE_C7_OUTSIZE	0x01
>
> diff --git a/spi25.c b/spi25.c
> index c4cd6b2..38037f1 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -141,7 +141,11 @@ static int probe_spi_rdid_generic(struct flashchip *flash, int bytes_in, unsigne
>    	}
>    } else {
>    	id1 = readarr[0];
> id2 = (readarr[1]<<  8) | readarr[2];
> /* Special case for AT25F chips. */
> if (bytes_in == 2)
> 	id2 = readarr[1];
> else
> 	id2 = (readarr[1]<<  8) | readarr[2];

Could you make AT25F probing a totally separate function which does not 
reuse probe_spi_rdid_generic? And move it to at25.c white you're at it?
> }
>
> msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2);
> @@ -173,6 +177,11 @@ int probe_spi_rdid(struct flashchip *flash)
>    return probe_spi_rdid_generic(flash, JEDEC_RDID_INSIZE, JEDEC_RDID);
>   }
>
> +int probe_spi_rdid_at25f(struct flashchip *flash)
> +{
> return probe_spi_rdid_generic(flash, AT25F_RDID_INSIZE, AT25F_RDID);

See above.
> +}
> int probe_spi_rdid4(struct flashchip *flash)
> {
>  /* Some SPI controllers do not support commands with writecnt=1 and
>
> @@ -394,6 +403,32 @@ int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash)
>    return spi_prettyprint_status_register_at25df(flash);
>   }
>
> +int spi_prettyprint_status_register_at25f(struct flashchip *flash)
> +{
> uint8_t status;
> status = spi_read_status_register();
> msg_cdbg("Chip status register is %02x\n", status);
> msg_cdbg("Chip status register: Status Register Write Protect (WPEN) "
>  "is %sset\n", (status&  (1<<  7)) ? "" : "not ");
> /* The following 3 bits are undefined in AT25F512(A), AT25F1024(A):
> msg_cdbg("Chip status register: Bit 6 is "
>  "%sset\n", (status&  (1<<  6)) ? "" : "not ");
> msg_cdbg("Chip status register: Bit 5 is "
>  "%sset\n", (status&  (1<<  5)) ? "" : "not ");
> msg_cdbg("Chip status register: Bit 4 is "
>  "%sset\n", (status&  (1<<  4)) ? "" : "not ");
> */
> /* This is undefined for AT25F512A; will be refactored soonish anyway */
> msg_cdbg("Chip status register: Block Protect 1 (BP1) is %sset\n",
>  (status&  (1<<  3)) ? "" : "not ");
> msg_cdbg("Chip status register: Block Protect 0 (BP0) is %sset\n",
>  (status&  (1<<  2)) ? "" : "not ");
> spi_prettyprint_status_register_welwip(status);
> return 0;
>
> +}

Status printing should already be handled just fine with the current 
code, could you please cross-check that?
> int spi_prettyprint_status_register_at25f512b(struct flashchip *flash)
> {
>  uint8_t status;
>
> @@ -592,6 +627,46 @@ int spi_chip_erase_60(struct flashchip *flash)
>    return 0;
>   }
>
> +int spi_chip_erase_62(struct flashchip *flash)
> +{
> int result;
> struct spi_command cmds[] = {
> {
> .writecnt	= JEDEC_WREN_OUTSIZE,
> .writearr	= (const unsigned char[]){ JEDEC_WREN },
> .readcnt	= 0,
> .readarr	= NULL,
> }, {
> .writecnt	= JEDEC_CE_62_OUTSIZE,
> .writearr	= (const unsigned char[]){ JEDEC_CE_62 },
> .readcnt	= 0,
> .readarr	= NULL,
> }, {
> .writecnt	= 0,
> .writearr	= NULL,
> .readcnt	= 0,
> .readarr	= NULL,
> }};
> result = spi_send_multicommand(cmds);
> if (result) {
> msg_cerr("%s failed during command execution\n",
> 	__func__);
> return result;
> }
> /* Wait until the Write-In-Progress bit is cleared.
> * This usually takes 2-5 s, so wait in 100 ms steps.
> */
> /* FIXME: We assume spi_read_status_register will never fail. */
> while (spi_read_status_register()&  JEDEC_RDSR_BIT_WIP)
> programmer_delay(100 * 1000);
> if (check_erased_range(flash, 0, flash->total_size * 1024)) {
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> +}
> int spi_chip_erase_c7(struct flashchip *flash)
> {
>  int result;
>
> @@ -826,6 +901,16 @@ int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int
>    return spi_chip_erase_60(flash);
>   }
>
> +int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
> +{
> if ((addr != 0) || (blocklen != flash->total_size * 1024)) {
> msg_cerr("%s called with incorrect arguments\n",
> 	__func__);
> return -1;
> }
> return spi_chip_erase_62(flash);
>
> +}

Should *_erase_62 be moved to at25.c or is it generic enogh to live in 
spi25.c?
> int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
> {
>  if ((addr != 0) || (blocklen != flash->total_size * 1024)) {
>
> @@ -1076,6 +1161,57 @@ int spi_disable_blockprotect(struct flashchip *flash)
>    return 0;
>   }
>
> +int spi_disable_blockprotect_at25f(struct flashchip *flash)
> +{
> uint8_t status;
> int result;
> status = spi_read_status_register();
> /* If block protection is disabled (BP0 and BP1 are 0), stop here. */
> if ((status&  (3<<  2)) == 0)
> return 0;
> msg_cdbg("Some block protection in effect, disabling\n");
> if (status&  (1<<  7)) {
> msg_cdbg("Need to disable Write Protect Enable (WPEN)\n");
> /* The following is used in spi_disable_blockprotect_at25df
>  * to check the state of the hardware lock pin. This is not
>  * possible with this chip, so we have to try.
> if ((status&  (1<<  4)) == 0) {
> 	msg_cerr("WP# pin is active, disabling "
> 		 "write protection is impossible.\n");
> 	return 1;
> }
> */
> /* All bits except bit 7 (WPEN) are readonly. If the WP pin is
>  * low, WPEN is readonly and this will fail.  */
> result = spi_write_status_register(flash, status&  ~(1<<  7));
> if (result) {
> 	msg_cerr("spi_write_status_register failed\n");
> 	return result;
> }
> status = spi_read_status_register();
> if (status&  (1<<  7)) {
> 	msg_cerr("WP# pin is probably active, disabling "
> 		 "write protection is impossible.\n");
> 	return 1;
> }
> }
> /* Global unprotect. Make sure to mask WPEN as well. */
> result = spi_write_status_register(flash, status&  ~0x8c);
> if (result) {
> msg_cerr("spi_write_status_register failed\n");
> return result;
> }
> status = spi_read_status_register();
> if ((status&  (3<<  2)) != 0) {
> msg_cerr("Block protection could not be disabled!\n");
> return 1;
> }
> return 0;
>
> +}

The function above should be in at25.c.
> int spi_disable_blockprotect_at25df(struct flashchip *flash)
> {
>  uint8_t status;

Feel free to only respond to my comments without posting a new patch if 
you think some of my comments are not applicable.

Regards,
Carl-Daniel