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

[Carl-Daniel Hailfinger]

On 10.07.2010 20:30, Michael Karcher wrote:
> 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?

Can you fix it up? Such a change is
> Acked-by: Carl-Daniel Hailfinger
> > -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.

OK, will change the code to error out.

Thanks for your review.

Regards,
Carl-Daniel