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.

[Stefan Tauner]

On Fri, 20 May 2011 01:11:23 +0200
Carl-Daniel Hailfinger  wrote:
> 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.

it makes sense. else someone would have to document it in a similar way
without the benefit of having understandable messages. that's also why
we should share as much code as possible (and even single lines such as
spi_prettyprint_status_register_amic_a25_srwd might be worth it) and
the stuff you said (in an earlier mail) about naming flags in a
consistent non-datasheet/vendor-specific manner makes sense.
> 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?

NB: that stuff was one of the first things i did in flashrom - without
much knowledge about data flashes and even less about flashrom's
(naming) conventions).
it is certainly quite similar to the_rdid command, but if we take the
generic rdid function i have introduced apart then we can also rename
it. atmel will probably/hopefully not use that scheme in the future
anyway.
> > 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.

jep. the renaming of the at25df512b stuff.
"The AT25F512B is quite different from the other (yet unsupported)
chips in the AT25F* familiy, so rename 512B-specific stuff." says it all :)
> > 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?

guess what i thought when i read those datasheets - about my first
data flash datasheets ... WTF after WTF :)
those three chips are very different in some details. i cant remember
exactly what those differences are, but drop all expectations when
dealing with them (and to some extent also when dealing with
at25f1024* iirc).

to answer your question:
AT25F512A ds (doc3345.pdf) page 9: "A single PROGRAM instruction
programs 1 to 128 consecutive bytes within a page if it is not write
protected."
AT25F512/AT25F1024 ds (doc1440.pdf) page 10: "A single PROGRAM
instruction programs 1 to 256 consecutive bytes within a page if it is
not write protected."
> > .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.

i had (now fried :) a AT25F1024 on an intel nic. that's the reason why
i started to do this stuff in the first place.
i am not sure anymore but i think i merged them because i could confirm
the id of 0x65 with real hardware and left the other one as is because
i could not due to lack of hw.

also look at my comment for the AT25F1024(A) in flashchips.c:
/* The A suffix indicates 33MHz instead of 20MHz clock rate.
 * All other properties seem to be the same.*/
in the 512(A) case we have different page sizes as you already noted.
> > #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?

i expected this but i wanted to hear it. i am not sure it is the right
decision. have you looked at the old version of this function? it is
already quite convoluted and the special case does not make it (much)
worse. i am not arguing for my solution at all. i just want to be sure
you saw the whole picture.
> > }
> >
> > 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?

will do
> > 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?

i have no idea, i just mimicked the functions with similar semantics
namely spi_*_erase_60 and spi_*_erase_c7.
if the only reason to move those to at25.c is that there are no other
users yet, then i would say leave them in spi25.c. in their current
form they are potentially usable by other chips, they are similar to
the other erase functions named above and we dont get much back by
moving them.
> > 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.

jup
> > 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.

feel free to just query me the final decisions in irc :)