Re: [flashrom] [PATCH] Hook up SPI unlock to infrastructure

[Michael Karcher]

Am Samstag, den 10.07.2010, 03:05 +0200 schrieb Carl-Daniel Hailfinger:
> -int spi_disable_blockprotect(void);
> +int spi_disable_blockprotect(struct flashchip *flash);

This change makes sense, but you don't use the flash parameter yet.
> @@ -1392,6 +1408,7 @@
>    			.block_erase = spi_block_erase_c7,
>    		}
>    	},
> .unlock		= spi_disable_blockprotect,
> .write		= spi_chip_write_1,
> .read		= read_memmapped,
> },

OUCH! "blame" me for committing that! read must not be "read_memmapped".
Should I submit a fixup patch for that or do you want to fix that in one
of your patches?
> -int spi_disable_blockprotect(void)
> +int spi_disable_blockprotect(struct flashchip *flash)
>  {
>    uint8_t status;
>    int result;
> @@ -855,6 +843,11 @@
>    		msg_cerr("spi_write_status_register failed\n");
>    		return result;
>    	}
> status = spi_read_status_register();
> if ((status & 0x3c) != 0) {
> 	msg_cerr("Block protection could not be disabled!\n");
> 	/* Should we error out here? */

Good question. As long as we have no partial write, we really should
error out here. But when we get partial writes, it would be great to
have flashrom being able to flash the lower part of the chip even if the
upper part is write protected. This would need major code changes,
though (unlocking would need to get the range to be unlocked), so for
now erroring out seems like the best option and as soon as not erroring
out might be sensible, we need to touch the code anyway.

Regards,
  Michael Karcher