Re: [flashrom] [PATCH] Locking infrastructure and locking for W39V080FA and PMC 49LF040

[Carl-Daniel Hailfinger]

On 04.02.2010 07:29, Sean Nelson wrote:
> Locking infrastructure
> Add lock-status command option.
> Winbond W39V080FA has 64k lock blocks.
> PMC Pm49FL004 has 64k lock blocks. Tested, but write/erase fails.

Hm. That's bad. Does the code work in an older revision (e.g. 0.9.1)?
> PMC Pm49FL002 has 32k lock blocks.
>
> Signed-off-by: Sean Nelson
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index c5062ca..a2bd5c5 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -79,26 +79,29 @@ int write_en29f002a(struct flashchip *flash, uint8_t *buf);
>  uint8_t oddparity(uint8_t val);
>  void toggle_ready_jedec(chipaddr dst);
>  void data_polling_jedec(chipaddr dst, uint8_t data);
>  int write_byte_program_jedec(chipaddr bios, uint8_t *src,
>    		     chipaddr dst);
>  int probe_jedec(struct flashchip *flash);
>  int erase_chip_jedec(struct flashchip *flash);
>  int write_jedec(struct flashchip *flash, uint8_t *buf);
>  int write_jedec_1(struct flashchip *flash, uint8_t *buf);
>  int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize);
>  int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
>  int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
>  int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, chipaddr dst, unsigned int page_size, unsigned int mask);
> +int lock_status_block_jedec(struct flashchip *flash, unsigned int block);
> +int unlock_block_jedec(struct flashchip *flash, unsigned int block);
> +int lock_block_jedec(struct flashchip *flash, unsigned int block);

Maybe s/block/blockaddr/ in all functions above.
> /* m29f002.c */
>  int erase_m29f002(struct flashchip *flash);
>  int write_m29f002t(struct flashchip *flash, uint8_t *buf);
>  int write_m29f002b(struct flashchip *flash, uint8_t *buf);
>
> /* m29f400bt.c */
>  int probe_m29f400bt(struct flashchip *flash);
>  int erase_m29f400bt(struct flashchip *flash);
>  int block_erase_m29f400bt(struct flashchip *flash, unsigned int start, unsigned int len);
>  int block_erase_chip_m29f400bt(struct flashchip *flash, unsigned int start, unsigned int len);
>  int write_m29f400bt(struct flashchip *flash, uint8_t *buf);
>  int write_coreboot_m29f400bt(struct flashchip *flash, uint8_t *buf);
> diff --git a/cli_classic.c b/cli_classic.c
> index 6cb0578..91beb5c 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -42,26 +42,27 @@ void cli_classic_usage(const char *name)
>
> printf("Please note that the command line interface for flashrom will "
>    	"change before\nflashrom 1.0. Do not use flashrom in scripts "
>    	"or other automated tools without\nchecking that your flashrom"
>    	" version won't interpret options in a different way.\n\n");
>
> printf
>        ("   -r | --read:                      read flash and save into file\n"
>         "   -w | --write:                     write file into flash\n"
>         "   -v | --verify:                    verify flash against file\n"
>         "   -n | --noverify:                  don't verify flash against file\n"
>         "   -E | --erase:                     erase flash device\n"
>         "   -V | --verbose:                   more verbose output\n"
>     "   -s | --lock-status:               print lock status on chip\n"

Shouldn't be an extra option, but be displayed on probe.
>     "   -c | --chip <chipname>:           probe only for specified flash chip\n"
>
> #if INTERNAL_SUPPORT == 1
>         "   -m | --mainboard <[vendor:]part>: override mainboard settings\n"
>  #endif
>         "   -f | --force:                     force write without checking image\n"
>         "   -l | --layout <file.layout>:      read ROM layout from file\n"
>         "   -i | --image <name>:              only flash image name from flash layout\n"
>         "   -L | --list-supported:            print supported devices\n"
>  #if PRINT_WIKI_SUPPORT == 1
>         "   -z | --list-supported-wiki:       print supported devices in wiki syntax\n"
>  #endif
>         "   -p | --programmer <name>:         specify the programmer device");
>
> @@ -99,56 +100,58 @@ void cli_classic_usage(const char *name)
>
> int cli_classic(int argc, char *argv[])
>  {
>    unsigned long size;
>    /* Probe for up to three flash chips. */
>    struct flashchip *flash, *flashes[3];
>    const char *name;
>    int namelen;
>    int opt;
>    int option_index = 0;
>    int force = 0;
>    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
>    int dont_verify_it = 0, list_supported = 0;
> int print_lock_status = 0;
>
> #if PRINT_WIKI_SUPPORT == 1
>    int list_supported_wiki = 0;
>  #endif
>    int operation_specified = 0;
>    int i;
>
> #if PRINT_WIKI_SUPPORT == 1
> const char *optstring = "rRwvnVEfc:m:l:i:p:Lzh";
> const char *optstring = "rRwvnVEfc:m:l:i:p:Lzhs";
>
> #else
> const char *optstring = "rRwvnVEfc:m:l:i:p:Lh";
> const char *optstring = "rRwvnVEfc:m:l:i:p:Lhs";
>
> #endif
>    static struct option long_options[] = {
>    	{"read", 0, 0, 'r'},
>    	{"write", 0, 0, 'w'},
>    	{"erase", 0, 0, 'E'},
>    	{"verify", 0, 0, 'v'},
>    	{"noverify", 0, 0, 'n'},
>    	{"chip", 1, 0, 'c'},
>    	{"mainboard", 1, 0, 'm'},
>    	{"verbose", 0, 0, 'V'},
>    	{"force", 0, 0, 'f'},
>    	{"layout", 1, 0, 'l'},
>    	{"image", 1, 0, 'i'},
>    	{"list-supported", 0, 0, 'L'},
>  #if PRINT_WIKI_SUPPORT == 1
>    	{"list-supported-wiki", 0, 0, 'z'},
>  #endif
>    	{"programmer", 1, 0, 'p'},
>    	{"help", 0, 0, 'h'},
>    	{"version", 0, 0, 'R'},
> {"lock-status", 0, 0, 's'},
>
> {0, 0, 0, 0}
> };
>
> char *filename = NULL;
>
> char *tempstr = NULL;
>
> print_version();
>
> if (argc > 1) {
> /* Yes, print them. */
> printf_debug("The arguments are:\n");
> for (i = 1; i < argc; ++i)
>
> @@ -261,26 +264,29 @@ int cli_classic(int argc, char *argv[])
>    				}
>    				break;
>    			}
>    		}
>    		if (programmer == PROGRAMMER_INVALID) {
>    			printf("Error: Unknown programmer %s.\n", optarg);
>    			exit(1);
>    		}
>    		break;
>    	case 'R':
>    		/* print_version() is always called during startup. */
>    		exit(0);
>    		break;
> case 's':
> 	print_lock_status = 1;
> 	break;
>
> case 'h':
> default:
> 	cli_classic_usage(argv[0]);
> 	break;
> }
> }
>
> if (list_supported) {
> print_supported();
> exit(0);
> }
>
> #if PRINT_WIKI_SUPPORT == 1
> @@ -354,33 +360,33 @@ int cli_classic(int argc, char *argv[])
>    flash = flashes[0];
>
> check_chip_supported(flash);
>
> size = flash->total_size * 1024;
>    if (check_max_decode((buses_supported & flash->bustype), size) &&
>        (!force)) {
>    	fprintf(stderr, "Chip is too big for this programmer "
>    		"(-V gives details). Use --force to override.\n");
>    	programmer_shutdown();
>    	return 1;
>    }
> if (!(read_it | write_it | verify_it | erase_it)) {
> if (!(read_it | write_it | verify_it | erase_it | print_lock_status)) {
> printf("No operations were specified.\n");
> // FIXME: flash writes stay enabled!
> programmer_shutdown();
> exit(1);
> }
> if (!filename && !erase_it) {
>
> if (!filename && !(erase_it | print_lock_status)) {
> printf("Error: No filename specified.\n");
> // FIXME: flash writes stay enabled!
> programmer_shutdown();
> exit(1);
> }
>
> /* Always verify write operations unless -n is used. */
> if (write_it && !dont_verify_it)
> verify_it = 1;
> return doit(flash, force, filename, read_it, write_it, erase_it, verify_it);
> return doit(flash, force, filename, read_it, write_it, erase_it, verify_it, print_lock_status);
>
> }

No extra option (applies to all of the code above).
> diff --git a/flash.h b/flash.h
> index d8f02bb..81aad35 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -142,26 +142,30 @@ enum chipbustype {
>  };
>
> /*
> How many different contiguous runs of erase blocks with one size each do
> we have for a given erase function?
>
> */
>  #define NUM_ERASEREGIONS 5
>
> /*
> How many different erase functions do we have per chip?
>
> */
>  #define NUM_ERASEFUNCTIONS 5
>
> +/* How many different lock segments can a chip have? */
> +#define NUM_LOCKREGIONS 3

I think you need 4 regions on some SPI flash chips, maybe more.
> +#define NUM_LOCKFUNCTIONS 3

Excessive? I'd say we restrict that to 2 (or maybe even 1) until there
is a need for more.
> #define FEATURE_REGISTERMAP	(1 << 0)
>  #define FEATURE_BYTEWRITES	(1 << 1)
>  #define FEATURE_LONG_RESET	(0 << 4)
>  #define FEATURE_SHORT_RESET	(1 << 4)
>  #define FEATURE_EITHER_RESET	FEATURE_LONG_RESET
>  #define FEATURE_ADDR_FULL	(0 << 2)
>  #define FEATURE_ADDR_MASK	(3 << 2)
>  #define FEATURE_ADDR_2AA	(1 << 2)
>  #define FEATURE_ADDR_AAA	(2 << 2)
>  #define FEATURE_ADDR_SHIFTED	0
>
> struct flashchip {
>    const char *vendor;
> @@ -195,26 +199,41 @@ struct flashchip {
>
> /*
>     * Erase blocks and associated erase function. Any chip erase function
>     * is stored as chip-sized virtual block together with said function.
>     */
>    struct block_eraser {
>    	struct eraseblock{
>    		unsigned int size; /* Eraseblock size */
>    		unsigned int count; /* Number of contiguous blocks with that size */
>    	} eraseblocks[NUM_ERASEREGIONS];
>    	int (*block_erase) (struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen);
>    } block_erasers[NUM_ERASEFUNCTIONS];
>
> /*
> * Lock blocks and associated locking related functions. Lock blocks
> * are usually uniform blocks with three functions for locking,
> * unlocking, and lock status.
> */
>
> struct block_locker {
> struct lockblock {
> 	unsigned int size; /* Lock block size; 64k or 32k */
> 	unsigned int count; /* Number of contiguous blocks */
> } lockblocks[NUM_LOCKREGIONS];
> int (*lock) (struct flashchip *flash, unsigned int block);
> int (*unlock) (struct flashchip *flash, unsigned int block);
> int (*lock_status) (struct flashchip *flash, unsigned int block);
>
> } block_lockers[NUM_LOCKFUNCTIONS];
>
> int (*write) (struct flashchip *flash, uint8_t *buf);
>  int (*read) (struct flashchip *flash, uint8_t *buf, int start, int len);
>
> /* Some flash devices have an additional register space. */
>  chipaddr virtual_memory;
>  chipaddr virtual_registers;
>
> };
>
> #define TEST_UNTESTED	0
>
> #define TEST_OK_PROBE	(1 << 0)
>  #define TEST_OK_READ	(1 << 1)
>  #define TEST_OK_ERASE	(1 << 2)
> @@ -515,27 +534,30 @@ int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len);
>  int erase_flash(struct flashchip *flash);
>  struct flashchip *probe_flash(struct flashchip *first_flash, int force);
>  int read_flash(struct flashchip *flash, char *filename);
>  void check_chip_supported(struct flashchip *flash);
>  int check_max_decode(enum chipbustype buses, uint32_t size);
>  int min(int a, int b);
>  int max(int a, int b);
>  char *extract_param(char **haystack, char *needle, char *delim);
>  int check_erased_range(struct flashchip *flash, int start, int len);
>  int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message);
>  char *strcat_realloc(char *dest, const char *src);
>  void print_version(void);
>  int selfcheck(void);
> -int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it);
> +int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it, int print_lock_status);

Kill print_lock_status parameter. Printing should be automatic.
> +int unlock_flash(struct flashchip *flash);
> +int lock_flash(struct flashchip *flash);
> +int print_lock_status_flash(struct flashchip *flash);
>
> #define OK 0
>  #define NT 1    /* Not tested */
>
> /* cli_output.c */
>  int print(int type, const char *fmt, ...);
>  #define MSG_ERROR	0
>  #define MSG_INFO	1
>  #define MSG_DEBUG	2
>  #define MSG_BARF	3
>  #define msg_gerr(...)	print(MSG_ERROR, __VA_ARGS__)	/* general errors */
>  #define msg_perr(...)	print(MSG_ERROR, __VA_ARGS__)	/* programmer errors */
>  #define msg_cerr(...)	print(MSG_ERROR, __VA_ARGS__)	/* chip errors */
> diff --git a/flashchips.c b/flashchips.c
> index f66b95a..0f7a62f 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -3425,27 +3425,36 @@ struct flashchip flashchips[] = {
>    	.block_erasers	=
>    	{
>    		{
>    			.eraseblocks = { {4 * 1024, 64} },
>    			.block_erase = erase_sector_jedec,
>    		}, {
>    			.eraseblocks = { {16 * 1024, 16} },
>    			.block_erase = erase_block_jedec,
>    		}, {
>    			.eraseblocks = { {256 * 1024, 1} },
>    			.block_erase = erase_chip_block_jedec,
>    		}
>    	},
> .write		= write_49fl00x,
> .block_lockers =
> {
> 	{
> 		.lockblocks = { {64 * 1024, 8} },
> 		.lock = lock_block_jedec,
> 		.unlock = unlock_block_jedec,
> 		.lock_status = lock_status_block_jedec,
> 	},
> },
> .write		= write_jedec_1,
>
> .read		= read_memmapped,
> },
>
> {
> .vendor		= "PMC",
> .name		= "Pm49FL004",
> .bustype	= CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH, /* A/A Mux*/
> .manufacture_id	= PMC_ID_NOPREFIX,
> .model_id	= PMC_49FL004,
> .total_size	= 512,
> .page_size	= 64 * 1024,
> .feature_bits	= FEATURE_REGISTERMAP | FEATURE_EITHER_RESET,
> .tested		= TEST_UNTESTED,
>
> @@ -3455,27 +3464,36 @@ struct flashchip flashchips[] = {
>    	.block_erasers	=
>    	{
>    		{
>    			.eraseblocks = { {4 * 1024, 128} },
>    			.block_erase = erase_sector_jedec,
>    		}, {
>    			.eraseblocks = { {64 * 1024, 8} },
>    			.block_erase = erase_block_jedec,
>    		}, {
>    			.eraseblocks = { {512 * 1024, 1} },
>    			.block_erase = erase_chip_block_jedec,
>    		}
>    	},
> .write		= write_49fl00x,
> .block_lockers =
> {
> 	{
> 		.lockblocks = { {64 * 1024, 8} },
> 		.lock = lock_block_jedec,
> 		.unlock = unlock_block_jedec,
> 		.lock_status = lock_status_block_jedec,
> 	},
> },
> .write		= write_jedec_1,
>
> .read		= read_memmapped,
> },
>
> {
> .vendor		= "Sanyo",
> .name		= "LF25FW203A",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= SANYO_ID,
> .model_id	= SANYO_LE25FW203A,
> .total_size	= 2048,
> .page_size	= 256,
> .tested		= TEST_UNTESTED,
> .probe		= probe_spi_rdid,
>
> @@ -6120,26 +6138,35 @@ struct flashchip flashchips[] = {
>    	.probe		= probe_jedec,
>    	.probe_timing	= TIMING_FIXME,
>    	.erase		= NULL, /* Was erase_winbond_fwhub */
>    	.block_erasers	=
>    	{
>    		{
>    			.eraseblocks = { {64 * 1024, 16}, },
>    			.block_erase = erase_sector_jedec,
>    		}, {
>    			.eraseblocks = { {1024 * 1024, 1} },
>    			.block_erase = erase_chip_block_jedec,
>    		}
>    	},
> .block_lockers =
> {
> 	{
> 		.lockblocks = { {64 * 1024, 16} },
> 		.lock = lock_block_jedec,
> 		.unlock = unlock_block_jedec,
> 		.lock_status = lock_status_block_jedec,
> 	},
> },
>
> .write		= write_jedec_1,
> .read		= read_memmapped,
> },
>
> {
> .vendor		= "Winbond",
> .name		= "W39V080FA (dual mode)",
> .bustype	= CHIP_BUSTYPE_FWH,
> .manufacture_id	= WINBOND_ID,
> .model_id	= W_39V080FA_DM,
> .total_size	= 512,
> .page_size	= 64 * 1024,
> .feature_bits	= FEATURE_REGISTERMAP | FEATURE_EITHER_RESET,
>
> @@ -6147,26 +6174,35 @@ struct flashchip flashchips[] = {
>    	.probe		= probe_jedec,
>    	.probe_timing	= TIMING_FIXME,
>    	.erase		= NULL, /* Was erase_winbond_fwhub */
>    	.block_erasers	=
>    	{
>    		{
>    			.eraseblocks = { {64 * 1024, 8}, },
>    			.block_erase = erase_sector_jedec,
>    		}, {
>    			.eraseblocks = { {512 * 1024, 1} },
>    			.block_erase = erase_chip_block_jedec,
>    		}
>    	},
> .block_lockers =
> {
> 	{
> 		.lockblocks = { {64 * 1024, 8} },
> 		.lock = lock_block_jedec,
> 		.unlock = unlock_block_jedec,
> 		.lock_status = lock_status_block_jedec,
> 	},
> },
>
> .write		= write_jedec_1,
> .read		= read_memmapped,
> },
>
> {
> .vendor		= "Atmel",
> .name		= "unknown Atmel SPI chip",
> .bustype	= CHIP_BUSTYPE_SPI,
> .manufacture_id	= ATMEL_ID,
> .model_id	= GENERIC_DEVICE_ID,
> .total_size	= 0,
> .page_size	= 256,
> .tested		= TEST_BAD_PREW,
>
> diff --git a/flashrom.c b/flashrom.c
> index 326f725..71c1b53 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -984,26 +984,206 @@ int erase_flash(struct flashchip *flash)
>    if (!found) {
>    	fprintf(stderr, "ERROR: flashrom has no erase function for this flash chip.\n");
>    	return 1;
>    }
>
> if (ret) {
>    	fprintf(stderr, "FAILED!\n");
>    } else {
>    	printf("SUCCESS.\n");
>    }
>    return ret;
>  }
>
> +int unlock_flash(struct flashchip *flash)
> +{
> int i, j, k, ret = 0, found = 0;
> unsigned int start;
> printf("Unlocking flash chip... ");
> for (k = 0; k < NUM_LOCKFUNCTIONS; k++) {
> unsigned int done = 0;
> struct block_locker locker = flash->block_lockers[k];
> printf_debug("Looking at blockwise unlock function %i... ", k);
> if (!locker.unlock && !locker.lockblocks[0].count) {
> 	printf_debug("not defined. "
> 		"Looking for another unlock function.\n");
> 	continue;
> }
> if (!locker.unlock && locker.lockblocks[0].count) {
> 	printf_debug("lockblock layout is known, but no "
> 		"matching block unlock function found. "
> 		"Looking for another unlock function.\n");
> 	continue;
> }
> if (locker.unlock && !locker.lockblocks[0].count) {
> 	printf_debug("block unlock function found, but "
> 		"lockblock layout is unknown. "
> 		"Looking for another unlock function.\n");
> 	continue;
> }
> found = 1;
> printf_debug("trying... ");
> for (i = 0; i < NUM_LOCKREGIONS; i++) {
> 	/* count==0 for all automatically initialized array
> 	 * members so the loop below won't be executed for them.
> 	 */
> 	for (j = 0; j < locker.lockblocks[i].count; j++) {
> 		start = done + locker.lockblocks[i].size * j;
> 		ret = locker.unlock(flash, start);
> 		printf_debug("0x%06x, ", start);
> 		if (ret)
> 			break;
> 	}
> 	if (ret)
> 		break;
> 	done += locker.lockblocks[i].count *
> 		locker.lockblocks[i].size;
> }
> printf_debug("\n");
> /* If everything is OK, don't try another erase function. */

s/erase/unlock/
> if (!ret)
> 	break;
> }
> if (!found) {
> fprintf(stderr, "ERROR: flashrom has no unlock function for this flash chip.\n");
> return 1;
> }
> if (ret) {
> fprintf(stderr, "FAILED!\n");
> } else {
> printf("SUCCESS.\n");
> }
> return ret;
>
> +}
>
> +int lock_flash(struct flashchip *flash)
> +{
> int i, j, k, ret = 0, found = 0;
> unsigned int start;
> printf("Locking flash chip... ");
> for (k = 0; k < NUM_LOCKFUNCTIONS; k++) {
> unsigned int done = 0;
> struct block_locker locker = flash->block_lockers[k];
> printf_debug("Looking at blockwise lock function %i... ", k);
> if (!locker.lock && !locker.lockblocks[0].count) {
> 	printf_debug("not defined. "
> 		"Looking for another lock function.\n");
> 	continue;
> }
> if (!locker.lock && locker.lockblocks[0].count) {
> 	printf_debug("lockblock layout is known, but no "
> 		"matching block lock function found. "
> 		"Looking for another lock function.\n");
> 	continue;
> }
> if (locker.lock && !locker.lockblocks[0].count) {
> 	printf_debug("block lock function found, but "
> 		"lockblock layout is unknown. "
> 		"Looking for another lock function.\n");
> 	continue;
> }
> found = 1;
> printf_debug("trying... ");
> for (i = 0; i < NUM_LOCKREGIONS; i++) {
> 	/* count==0 for all automatically initialized array
> 	 * members so the loop below won't be executed for them.
> 	 */
> 	for (j = 0; j < locker.lockblocks[i].count; j++) {
> 		start = done + locker.lockblocks[i].size * j;
> 		ret = locker.lock(flash, start);
> 		printf_debug("0x%06x, ", start);
> 		if (ret)
> 			break;
> 	}
> 	if (ret)
> 		break;
> 	done += locker.lockblocks[i].count *
> 		locker.lockblocks[i].size;
> }
> printf_debug("\n");
> /* If everything is OK, don't try another erase function. */

s/erase/lock/
> if (!ret)
> 	break;
> }
> if (!found) {
> fprintf(stderr, "ERROR: flashrom has no lock function for this flash chip.\n");
> return 1;
> }
> if (ret) {
> fprintf(stderr, "FAILED!\n");
> } else {
> printf("SUCCESS.\n");
> }
> return ret;
>
> +}
>
> +int print_lock_status_flash(struct flashchip *flash)
> +{
> int i, j, k, ret = 0, found = 0;
> unsigned int start, lock_stat = 0;
> printf("Printing Lock Status for flash chip... ");
> for (k = 0; k < NUM_LOCKFUNCTIONS; k++) {
> unsigned int done = 0;
> struct block_locker locker = flash->block_lockers[k];
> printf_debug("Looking at blockwise lock_status function %i... ", k);
> if (!locker.lock_status && !locker.lockblocks[0].count) {
> 	printf_debug("not defined. "
> 		"Looking for another lock_status function.\n");
> 	continue;
> }
> if (!locker.lock_status && locker.lockblocks[0].count) {
> 	printf_debug("lockblock layout is known, but no "
> 		"matching block lock_status function found. "
> 		"Looking for another lock_status function.\n");
> 	continue;
> }
> if (locker.lock_status && !locker.lockblocks[0].count) {
> 	printf_debug("block lock_status function found, but "
> 		"lockblock layout is unknown. "
> 		"Looking for another lock_status function.\n");
> 	continue;
> }
> found = 1;
> printf("trying... ");
> for (i = 0; i < NUM_LOCKREGIONS; i++) {
> 	/* count==0 for all automatically initialized array
> 	 * members so the loop below won't be executed for them.
> 	 */
> 	for (j = 0; j < locker.lockblocks[i].count; j++) {
> 		start = done + locker.lockblocks[i].size * j;
> 		lock_stat = locker.lock_status(flash, start);
> 		printf("0x%06x has status 0x%06x, ", start, lock_stat);
> 	}
> 	done += locker.lockblocks[i].count *
> 		locker.lockblocks[i].size;
> }
> printf("\n");
> }

Semantics... if we have multiple lock printing functions, which result
do we trust?
> if (!found) {
> fprintf(stderr, "ERROR: flashrom has no lock_status function for this flash chip.\n");
> return 1;
> }
> return ret;
>
> +}
>
> void emergency_help_message(void)
>  {
>    fprintf(stderr, "Your flash chip is in an unknown state.\n"
>    	"Get help on IRC at irc.freenode.net (channel #flashrom) or\n"
>    	"mail [e-mail]!\n--------------------"
>    	"-----------------------------------------------------------\n"
>    	"DO NOT REBOOT OR POWEROFF!\n");
>  }
>
> /* The way to go if you want a delimited list of programmers*/
>  void list_programmers(char *delim)
>  {
>    enum programmer p;
> @@ -1088,59 +1268,65 @@ void check_chip_supported(struct flashchip *flash)
>    	       "mainboard or programmer you tested. Thanks for your "
>    	       "help!\n===\n");
>    }
>  }
>
> int main(int argc, char *argv[])
>  {
>    return cli_classic(argc, argv);
>  }
>
> /* This function signature is horrible. We need to design a better interface,
> but right now it allows us to split off the CLI code.
>
> */
> -int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it)
> +int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it, int print_lock_status)

Kill that print_lock_status parameter.
> {
>    uint8_t *buf;
>    unsigned long numbytes;
>    FILE *image;
>    int ret = 0;
>    unsigned long size;
>
> size = flash->total_size * 1024;
>    buf = (uint8_t *) calloc(size, sizeof(char));
> unlock_flash(flash);

Bad idea. We don't want to unlock the flash so early, especially if
we're only planning to read.
> if (print_lock_status) {

Kill the if, this should run automatically (and probably from probe_flash).
> print_lock_status_flash(flash);
> }
> if (erase_it) {
>   if (flash->tested & TEST_BAD_ERASE) {
>   	fprintf(stderr, "Erase is not working on this chip. ");
>   	if (!force) {
>   		fprintf(stderr, "Aborting.\n");
>   		programmer_shutdown();
>   		return 1;
>   	} else {
>   		fprintf(stderr, "Continuing anyway.\n");
>   	}
>   }
>   if (erase_flash(flash)) {
>   	emergency_help_message();
>   	programmer_shutdown();
>   	return 1;
>   }
>  } else if (read_it) {
>   if (read_flash(flash, filename)) {
>   	programmer_shutdown();
>   	return 1;
>   }
> } else {
> } else if (write_it | verify_it) {

This chance can die as well.
> struct stat image_stat;
>
> if (flash->tested & TEST_BAD_ERASE) {
> 	fprintf(stderr, "Erase is not working on this chip "
> 		"and erase is needed for write. ");
> 	if (!force) {
> 		fprintf(stderr, "Aborting.\n");
> 		programmer_shutdown();
> 		return 1;
> 	} else {
> 		fprintf(stderr, "Continuing anyway.\n");
> 	}
> }
>
> @@ -1196,26 +1382,28 @@ int doit(struct flashchip *flash, int force, char *filename, int read_it, int wr
>    		return 1;
>    	}
>    	ret = flash->write(flash, buf);
>    	if (ret) {
>    		fprintf(stderr, "FAILED!\n");
>    		emergency_help_message();
>    		programmer_shutdown();
>    		return 1;
>    	} else {
>    		printf("COMPLETE.\n");
>    	}
>    }
> lock_flash(flash);

We probably don't want this either. For starters, it means that a
previously unlocked chip will now be unusable for all older flashrom
versions.
> if (verify_it) {
>   /* Work around chips which need some time to calm down. */
>   if (write_it)
>   	programmer_delay(1000*1000);
>   ret = verify_flash(flash, buf);
>   /* If we tried to write, and verification now fails, we
> might have an emergency situation.
>
> */
>   if (ret && write_it)
>   	emergency_help_message();
>  }
>
> programmer_shutdown();
>
> diff --git a/jedec.c b/jedec.c
> index 055e910..2002fd6 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -485,13 +485,33 @@ int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int s
>    int mask;
>
> mask = getaddrmask(flash);
>    return erase_block_jedec_common(flash, page, size, mask);
>  }
>
> int erase_chip_jedec(struct flashchip *flash)
>  {
>    int mask;
>
> mask = getaddrmask(flash);
>    return erase_chip_jedec_common(flash, mask);
>  }
>
> +int unlock_block_jedec(struct flashchip *flash, unsigned int block)
> +{
> chip_writeb(0, flash->virtual_registers + block + 2);
> return 0;
>
> +}
>
> +int lock_block_jedec(struct flashchip *flash, unsigned int block)
> +{
> chip_writeb(1, flash->virtual_registers + block + 2);
> return 0;
>
> +}
>
> +int lock_status_block_jedec(struct flashchip *flash, unsigned int block)
> +{
> int lock;
> lock = chip_readb(flash->virtual_registers + block + 2);
> return lock & 0x7;
>
> +}

Assuming there are multiple lock functions, which one is correct? If
they all have the same results anyway, there's no point in having more
than one set of lock functions. If they may have differing results,
either something is totally broken or there are different types of locks
(and then we'd have to lock and unlock _all_ of them, not just the first
one that is successful).

There is another conceptual problem. How do you differentiate between
- temporary reversible software locks
- temporary irreversible software locks (irreversible until next reset etc.)
- permanent software locks (write-once for the whole chip lifetime)
- unchangeable hardware locks (WP#, BPL#)
- locks which lock locks (once that "master" lock is set, other locks
may not be changed)
- read locks (i.e. chip region will return 0xff)
- write locks

All of the above exists in some flash chips supported by flashrom.

Regards,
Carl-Daniel