Re: [flashrom] [PATCH] SPI bitbanging core fixups

[Carl-Daniel Hailfinger]

On 17.07.2010 09:47, Michael Karcher wrote:
> Am Samstag, den 17.07.2010, 00:59 +0200 schrieb Carl-Daniel Hailfinger:
> > > Why so slow? SPI was designed to be fast (if you want a slow system, use
> > > I2C instead ;) ). It might be quite sensible on programmers at the
> > > parallel port having some cable length and no driver chip at the flash
> > > piece (maybe that applies to SPIPGM), but that is specific to this
> > > programmer. nVidia on-board stuff should run slow enough without any
> > > programmer_delay calls.
> >
> > Not sure. I figured that 1 usec would be a sane default, and could be
> > reduced to zero by individual programmers depending on their knowledge
> > about timing.
>
> My idea was that everything that is not "broken" in hardware doesn't
> need a delay, so 0 is a sane default and programmers can set this to a
> bigger value in their code if they think that this programmer is so
> slow, but I don't really care, 1 as default is OK too.

I based the delay requirement on the possibility of really fast
hardware. Once your hardware can do bitbanging at speeds above 16 MHz,
it is too fast for some SPI chips. If someone doesn't know the maximum
hardware speed for a new driver, he/she can simply keep the default
delay and be confident that it won't be too fast. For all I know, the
Nvidia SPI stuff might reach a few MHz.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> I'd rather kill the whole buffer stuff (see attached patch) before
> applying your patch (and of course remove the buffer stuff from your
> patch, too). No other reason not to ack.
> > From 883e73a446a0fa2614fe4b6309a43ecfeeedea24 Mon Sep 17 00:00:00 2001
>
> From: Michael Karcher 
> Date: Sat, 17 Jul 2010 09:43:58 +0200
> Subject: [PATCH] remove temporary buffers from bitbanging
>
> There is no point in memcpying data around in/out temporary buffers if
> the original buffer does as good as the temporary one.
>
> Compile tested, no functional testing.
>
> Signed-off-by: Michael Karcher

Acked-by: Carl-Daniel Hailfinger 
with one small change.
> diff --git a/bitbang_spi.c b/bitbang_spi.c
> index 446be11..583db32 100644
> --- a/bitbang_spi.c
> +++ b/bitbang_spi.c
> @@ -85,58 +85,24 @@ uint8_t bitbang_spi_readwrite_byte(uint8_t val)
>  int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	const unsigned char *writearr, unsigned char *readarr)
>  {
>
> static unsigned char *bufout = NULL;
>
> static unsigned char *bufin = NULL;
>
> static int oldbufsize = 0;
>
> int bufsize;
> int i;
>
> /* Arbitrary size limitation here. We're only constrained by memory. */
>
> if (writecnt > 65536 || readcnt > 65536)
> return SPI_INVALID_LENGTH;
>
> bufsize = max(writecnt + readcnt, 260);
>
> /* Never shrink. realloc() calls are expensive. */
>
> if (bufsize > oldbufsize) {
> bufout = realloc(bufout, bufsize);
> if (!bufout) {
> 	msg_perr("Out of memory!\n");
> 	if (bufin)
> 		free(bufin);
> 	exit(1);
> }
> bufin = realloc(bufout, bufsize);
> if (!bufin) {
> 	msg_perr("Out of memory!\n");
> 	if (bufout)
> 		free(bufout);
> 	exit(1);
> }
> oldbufsize = bufsize;
>
> }
>
> memcpy(bufout, writearr, writecnt);
>
> /* Shift out 0x00 while reading data. */
>
> memset(bufout + writecnt, 0x00, readcnt);
>
> /* Make sure any non-read data is 0xff. */
>
> memset(bufin + writecnt, 0xff, readcnt);
>
> bitbang_spi_set_cs(0);
>
> for (i = 0; i < readcnt + writecnt; i++) {
> bufin[i] = bitbang_spi_readwrite_byte(bufout[i]);
>
> }
> for (i = 0; i < writecnt; i++)
> bitbang_spi_readwrite_byte(writearr[i]);
> for (i = 0; i < readcnt; i++)
> readarr[i] = bitbang_spi_readwrite_byte(0);
> programmer_delay(bitbang_spi_half_period);
>  bitbang_spi_set_cs(1);
>  programmer_delay(bitbang_spi_half_period);
>
> memcpy(readarr, bufin + writecnt, readcnt);
>
> return 0;
>
> }

I have to admit that your no-extra-buffer solution is elegant and should
work fine. It makes printing the readback during write impossible, but
such a feature is only needed for debugging of new drivers anyway.
> int bitbang_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> /* Maximum read length is unlimited, use 64k bytes. */
> return spi_read_chunked(flash, buf, start, len, 64 * 1024);
> return spi_nbyte_read(start, buf, len);

Please undo that hunk. It breaks a few semi-supported SPI chips with odd
sector sizes and is inconsistent with how the other SPI drivers do it.
> }
>
> int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)

Regards,
Carl-Daniel