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

[Carl-Daniel Hailfinger]

On 10.07.2010 19:21, Michael Karcher wrote:
> 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.

Yes, the plan is to kill the wrapper ASAP once the other chips are
converted as well.
> > -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.

My updated patch fixes that, but I'd really like someone to check all
corner cases of the code.
> > ===================================================================
> > --- 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?

AFAIK all write_1 chips can do AAI. Right now AAI is broken and I'm
waiting for an ack for patch 1548. Switching to write_1 instead of AAI
allows me to avoid creating a wrapper for AAI.
> > @@ -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?

Mh. That's very non-obvious. The spi_programmer array has a .write_256
member for every controller. In case the controller can't do anything
except 1-byte writes, we set
.write_256 = spi_chip_write_1_new
> > +/* 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.

This patch is basically a part of patch 1371 with less bugs, more
consistency and working backwards compatibility to make merging and
reviewing easier.
IMHO patch 1371 is very difficult to review, whereas this patch (and
patch 1603) will allow me to post an improved version of patch 1371.
I don't like the introduction of two new wrappers either, but those are
a lot less of a problem than the existing per-controller wrappers and
removing them later will be a lot less invasive than 1371.
> > -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?

Yes, this should be handled with max_rom_decode instead. I will send a
followup patch for max_rom_decode.spi in wbsio and kill the
wbsio-specific read+write.
> Remaining stuff looks fine.

Thanks.

Regards,
Carl-Daniel