Re: [flashrom] [PATCH] Convert SPI chips to partial write

[Michael Karcher]

Am Freitag, den 09.07.2010, 19:34 +0200 schrieb Carl-Daniel Hailfinger:
> -int spi_chip_write_1(struct flashchip *flash, uint8_t *buf)
> +/* real chunksize is 1, logical chunksize is 1 */
> +int spi_chip_write_1_new(struct flashchip *flash, uint8_t *buf, int start, int len)

I think "spi_chip_write_1_range" makes a better name than
"spi_chip_write_1_new". But if your plan is to remove the old
spi_chip_write_1 function and rename this function at the same time to
"spi_chip_write_1", I don't really mind the temporary name.
> -int it8716f_spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
> +int it8716f_spi_chip_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {

[...]
> +#ifdef TODO
> +#warning This function needs to be converted to partial write
> if ((programmer == PROGRAMMER_IT87SPI) || (flash->total_size * 1024 > 512 * 1024)) {
>
> +#endif
> spi_chip_write_1_new(flash, buf, start, len);
>
> +#ifdef TODO
> +#warning This function needs to be converted to partial write

[...]
> +#endif

So this function will fall back to single-byte writes every time now
until it is changed in a later patch to support ranges. Did I understand
the patch correctly? I think this is acceptable to keep the patch size
manageable.
> ===================================================================
> --- flashrom-partial_write_spi_intermediate/flashchips.c	(Revision 1072)
> +++ flashrom-partial_write_spi_intermediate/flashchips.c	(Arbeitskopie)
> @@ -1392,7 +1392,7 @@
>    			.block_erase = spi_block_erase_c7,
>    		}
>    	},
> .write		= spi_aai_write,
> .write		= spi_chip_write_1,
> .read		= read_memmapped,
> },

You do this change to accommodate for the changed signature of
spi_aai_write, I think. But you lose the information that this chip used
AAI writes this way. Would adding a comment like "/* AAI capable */" be
sensible?
> @@ -197,7 +197,8 @@
> Program chip using page (256 bytes) programming.
> Some SPI masters can't do this, they use single byte programming instead.
>
> */
> -int spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
> +/* real chunksize is up to 256, logical chunksize is 256 */
> +int spi_chip_write_256_new(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
>    if (!spi_programmer[spi_controller].write_256) {
>    	msg_perr("%s called, but SPI page write is unsupported on this "

Oops! The comment says that single-byte programming is used on
non-256-capable masters, but the code does not handle the fallback to
single bytes. I also am unable to find any other place in flashrom that
checks for the block write capability before calling this function. Is
that a bug?
> +/* Wrapper function until the generic code is converted to partial writes. */
> +int spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
> +{
> int ret;
> spi_disable_blockprotect();
> msg_pinfo("Erasing flash before programming... ");
> if (erase_flash(flash)) {
> msg_perr("ERASE FAILED!\n");
> return -1;
> }
> msg_pinfo("done.\n");
> msg_pinfo("Programming flash... ");
> ret = spi_chip_write_256_new(flash, buf, 0, flash->total_size * 1024);
> if (!ret)
> msg_pinfo("done.\n");
> else
> msg_pinfo("\n");
> return ret;
>
> +}

This reminds me of patch 1371. I'm all for getting that patch in before
this patch and not move around the erase-on-write logic just to kill it.
I will send a review soon.
> -int wbsio_spi_write_1(struct flashchip *flash, uint8_t *buf)
> +int wbsio_spi_write_1(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> int size = flash->total_size * 1024;
> if (size > 1024 * 1024) {
> if (flash->total_size * 1024 > 1024 * 1024) {
> msg_perr("%s: Winbond saved on 4 register bits so max chip size is 1024 KB!\n", __func__);
> return 1;
> }
> return spi_chip_write_1(flash, buf);
> return spi_chip_write_1_new(flash, buf, start, len);
>
> }

As soon as we have explicit erase, this check is too late in the write
function. Don't we even have the same problem even at read time?

Remaining stuff looks fine.

Regards,
  Michael Karcher