[flashrom] Add support for unlock and write to Atmel AT26F040

[Stefan Tauner]

On Tue, 21 Jun 2011 00:38:55 +0100 (BST)
tim at buttersideup.com (Tim Small) wrote:

> 
> Add support for unlock and write to Atmel AT26F040 SPI 512k chips.
> 
> Tested using a bus pirate only.
> 
> I'm unavailble for a couple of weeks imminently, but will reply on
> return if pos...

since you are back now... and no one has beaten me to it, i'll give it
a quick look. on the whole it does look quite ok. and you will probably
change it a bit anyway according to your irc announcements... anyway
here is what i was thinking when i read over the code:

> Index: at25.c
> ===================================================================
> --- at25.c	(revision 1349)
> +++ at25.c	(working copy)
> @@ -215,6 +215,8 @@
>  	/* spi_disable_blockprotect_at25df is not really the right way to do
>  	 * this, but the side effects of said function work here as well.
>  	 */
> +	/* FIXME? This does a global unlock, but at25f don't have this command?
> +	 */
which "command"? the comment above yours explains it quite accurately i
think.

>  	return spi_disable_blockprotect_at25df(flash);
>  }
>  
> @@ -285,3 +287,71 @@
>  	}
>  	return 0;
>  }
> +
> +
> +int spi_disable_blockprotect_at26f040(struct flashchip *flash)
> +{
> +	uint8_t status;
> +	uint32_t i;
> +	int result;
> +
> +	/* See Page 19							 *
> +	 * http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf */
could be moved above the whole function

> +
> +       	/* Write Protect Pin status: 0 == cannot set SPRL to 0 (unlocked) */
wrong indention and spaces instead of tabs

> +	const uint8_t stat_bit_WPP = (1 << 4);
carldani has preferred shorter variables in the past. we usually don't
use such constants and verbose comments (although it would help newbies
to understand the code). i think anyone able to read a datasheet
would prefer these inline (if not they should be macros instead).
look at the other functions in at25.c for examples.

> +	
> +       	/* Software Protection Status: 00 == all sectors unprotected */
> +	const uint8_t stat_bit_SWP = (3 << 2);
> +
> +       	/* Sector Protection registers locked: 0 == unlocked. *
wrong indention and spaces instead of tabs

> +	 * Cannot be set to 0 if WPP is 1 (hardware lock).    */
> +	const uint8_t stat_bit_SPRL = (1 << 7);
> +
> +	/* Page 6 */
> +	const uint8_t at26f004_unprotect_sector = 0x39;
> +
> +	status = spi_read_status_register();
> +	/* If block protection is disabled, OK to go ahead, stop here. */
> +	if ((status & stat_bit_SWP) == 0)
> +		return 0;
> +
> +	msg_cdbg("Some block protection in effect, disabling\n");
> +	if (status & stat_bit_SPRL) {
> +		msg_cdbg("Need to disable Sector Protection Register Lock\n");
> +		if ((status & stat_bit_WPP) == 0) {
> +			msg_cerr("The chips write-protect pin is being "
> +				 "asserted (driven low) by external hardware."
> +				 "\nWriting is impossible :-(.\n");
that's a bit too verbose, and the smiley although fitting is not
appropriate imho.

> +			return 1;
> +		}
> +		/* All bits except bit 7 (SPRL) are readonly. */
> +		/* Unlock sector protection registers. */
> +		result = spi_write_status_register(flash, status & ~stat_bit_SPRL);
> +		if (result) {
> +			msg_cerr("spi_write_status_register failed\n");
> +			return result;
> +		}
> +		
> +	}
> +	/* Call unprotect sector on each sector.  We need to give an address */
> +        /* within each sector, and the smallest sector is 8KB. */
spaces instead of tabs

> +	for (i = 0; i < 0x80000; i += 0x1000) {
hm 0x1000 is 4kB?
the upper bound should be derived from flash->total_size

> +		const unsigned char cmd[4] = { at26f004_unprotect_sector,
> +		       			       (i >> 16) & 0xff,
> +					       (i >> 8) & 0xff,
> +					       i & 0xff};
i have not verified this, but it seems to be correct.

> +		/* set the chip's "WEL" write latch - WREN (Write Enable) */
> +		spi_write_enable();
> +		spi_send_command(sizeof(cmd), 0, cmd, NULL);
> +	}
> +	status = spi_read_status_register();
> +	if ((status & stat_bit_SWP) != 0) {
> +		msg_cerr("Block protection could not be disabled!\n");
> +		return 1;
> +	} else {
> +		msg_cerr("All write protection disabled!\n");
> +	}
could (and probably should) remove the curly braces in the else path

> +
> +	return 0;
> +}
> Index: spi25.c
> ===================================================================
> --- spi25.c	(revision 1349)
> +++ spi25.c	(working copy)
> @@ -1210,3 +1210,94 @@
>  
>  	return 0;
>  }
> +
> +
i am not entirely sure this hunk should be in spi25.c... but since the
sst chips may be compatible with this, there need to be more
investigation anyway and it may very well be the right place...

> +/* This is for Atmel chips only (AT26F004, AT26DF161A etc.) - the 25F004 only
> + * does either this mode, or byte-at-a-time. */
but then this comment should be generalized.

> +int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len)
> +{
> +	/* ref http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf
> +	 * page 9 etc. */
should be a function comment if at all

> +	const int AT26_SEQ_BYTE_WRITE_OUTSIZE = 5;
> +	const int AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE = 2;
> +	const int AT26_SEQ_BYTE_WRITE_CMD = 0xaf;
those should be macros in spi.h

> +	uint32_t pos = start;
> +	int result;
> +	unsigned char cmd[4] = {
> +		AT26_SEQ_BYTE_WRITE_CMD
> +	};
that init looks very odd when one reads it first.
i think not initializing it here and setting cmd[0] explicitly before
the loop would be better.

> +	struct spi_command cmds[] = {
> +	{
> +		.writecnt	= JEDEC_WREN_OUTSIZE,
> +		.writearr	= (const unsigned char[]){ JEDEC_WREN },
> +		.readcnt	= 0,
> +		.readarr	= NULL,
> +	}, {
> +		.writecnt	= AT26_SEQ_BYTE_WRITE_OUTSIZE,
> +		.writearr	= (const unsigned char[]){
> +					AT26_SEQ_BYTE_WRITE_CMD,
> +					(start >> 16) & 0xff,
> +					(start >> 8) & 0xff,
> +					(start & 0xff),
> +					buf[0]
> +				},
> +		.readcnt	= 0,
> +		.readarr	= NULL,
> +	}, {
> +		.writecnt	= 0,
> +		.writearr	= NULL,
> +		.readcnt	= 0,
> +		.readarr	= NULL,
> +	}};
i did not verify that.

> +	switch (spi_programmer->type) {
> +#if CONFIG_INTERNAL == 1
> +#if defined(__i386__) || defined(__x86_64__)
> +	case SPI_CONTROLLER_IT87XX:
> +	case SPI_CONTROLLER_WBSIO:
> +		msg_perr("%s: impossible with this SPI controller,"
> +				" degrading to byte program\n", __func__);
> +		return spi_chip_write_1(flash, buf, start, len);
> +#endif
> +#endif
> +	default:
> +		break;
> +	}
> +
> +	cmds[1].writearr = (const unsigned char[]){
> +		0xAF,
> +			(pos >> 16) & 0xff,
> +			(pos >> 8) & 0xff,
> +			(pos & 0xff),
> +			buf[pos - start]
> +	};
pos = start so this makes it buf[0]? did i miss something?
the whole assignment is equal to the initialization afaics...!? :)

> +	
> +
> +	result = spi_send_multicommand(cmds);
> +	if (result) {
> +		msg_cerr("%s failed during start command execution\n",
> +			 __func__);
> +		/* FIXME: Should we send WRDI here as well to make sure the chip
> +		 * is not in seuquential wirte mode?
> +		 */
> +		return result;
> +	}
> +	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> +		programmer_delay(5);
> +
> +	pos ++;
please remove the space

> +
> +	while (pos < start + len) {
> +		cmd[1] = buf[pos++ - start];
> +		spi_send_command(AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE, 0, cmd, NULL);
> +		while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> +			programmer_delay(5);
> +	}
hm... couldn't the first byte write be split from the
spi_send_multicommand above and be done in the loop instead?
no, it can't, because chip select would be different and it needs to be
done like described here (cs low, seq enable, start address, first
data, cs high), pity.

> +
> +	/* Use WRDI to exit sequential write mode. This needs to be done before
> +	 * issuing any other non-sequential-byte-write commands.
> +	 */
too verbose imho

> +	spi_write_disable();
> +
> +	return 0;
> +}
> Index: flashchips.c
> ===================================================================
> --- flashchips.c	(revision 1349)
> +++ flashchips.c	(working copy)
> @@ -1883,6 +1883,7 @@
>  		},
>  		.printlock	= spi_prettyprint_status_register_atmel_at26df081a,
>  		.unlock		= spi_disable_blockprotect,
> +		/* Should support spi_at26_write_sequential too, but it's untested */
would that buy us anything if used?

+		/* Supports spi_at26_write_sequential too, but it's untested */
>  		.write		= spi_chip_write_256,
>  		.read		= spi_chip_read,
>  		.voltage	= {2700, 3600},
> @@ -1914,6 +1915,7 @@
>  		.model_id	= ATMEL_AT26F004,
>  		.total_size	= 512,
>  		.page_size	= 256,
> +		.feature_bits   = FEATURE_WRSR_WREN,
>  		.tested		= TEST_UNTESTED,
>  		.probe		= probe_spi_rdid,
>  		.probe_timing	= TIMING_ZERO,
> @@ -1936,7 +1938,9 @@
>  				.block_erase = spi_block_erase_c7,
>  			}
>  		},
> -		.write		= NULL /* Incompatible Page write */,
> +		.unlock		= spi_disable_blockprotect_at26f040,
> +		/* also supports spi_chip_write_1, but nothing else */
have you tested spi_chip_write_1?

+		/* Supports spi_chip_write_1 too */
or something similar to the comment for the AT26DF161A

> +		.write		= spi_at26_write_sequential, 
>  		.read		= spi_chip_read,
>  		.voltage	= {2700, 3600},
>  	},
> Index: chipdrivers.h
> ===================================================================
> --- chipdrivers.h	(revision 1349)
> +++ chipdrivers.h	(working copy)
> @@ -55,6 +55,7 @@
>  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
>  int spi_write_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
>  int spi_aai_write(struct flashchip *flash, uint8_t *buf, int start, int len);
> +int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len);
>  
>  /* a25.c */
>  int spi_prettyprint_status_register_amic_a25l05p(struct flashchip *flash);
> @@ -74,6 +75,7 @@
>  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_disable_blockprotect_at26f040(struct flashchip *flash);
>  
>  /* 82802ab.c */
>  uint8_t wait_82802ab(struct flashchip *flash);

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner