Re: [flashrom] [PATCH 2/5] Prepare wbsio_spi.c for check_trans().

[Carl-Daniel Hailfinger]

Am 03.10.2012 06:13 schrieb Stefan Tauner:
> Introduce determine_mode() which contains the extracted bits of the init
> function that do sanity checks but without altering the hardware state.
>
> Signed-off-by: Stefan Tauner

I see that you took great care to avoid the pitfalls in converting that
code. Good!

Side note: That hardware is just awful from a capability/interface
perspective. If I had a way to drive it by bitbanging, I'd do that
instantly. However, the two of the four essential SPI lines (MOSI and
MISO) are not usable as GPIO, so that way of working around hardware
limitations is not going to work.
> wbsio_spi.c |  136 +++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 77 insertions(+), 59 deletions(-)
>
> diff --git a/wbsio_spi.c b/wbsio_spi.c
> index 7d4bb2a..778b983 100644
> --- a/wbsio_spi.c
> +++ b/wbsio_spi.c
> @@ -61,6 +61,53 @@ done:
>    return flashport;
>  }
>
> +/* W83627DHG has 11 command modes:
> 1=1 command only
> 2=1 command+1 data write
> 3=1 command+2 data read
> 4=1 command+3 address
> 5=1 command+3 address+1 data write
> 6=1 command+3 address+4 data write
> 7=1 command+3 address+1 dummy address inserted by wbsio+4 data read
> 8=1 command+3 address+1 data read
> 9=1 command+3 address+2 data read
> a=1 command+3 address+3 data read
> b=1 command+3 address+4 data read
> mode[7:4] holds the command mode
> mode[3:0] holds SPI address bits [19:16]
> The Winbond SPI master only supports 20 bit addresses on the SPI bus. :\
> Would one more byte of RAM in the chip (to get all 24 bits) really make
> such a big difference?
> */
>
> +static uint8_t determine_mode(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt,
> 	      const unsigned char *writearr)
>
> +{
> if (1 == writecnt && 0 == readcnt) {
> return 0x10;
> } else if (2 == writecnt && 0 == readcnt) {
> return 0x20;
> } else if (1 == writecnt && 2 == readcnt) {
> return 0x30;
> } else if (4 == writecnt && 0 == readcnt) {
> return 0x40 | (writearr[1] & 0x0f);

Would it be possible to return only 0x40 here, and add the low nibble of
writearr[1] inside wbsio_send_command? That low nibble is not part of
the mode, it just happens to be stored in the same hw register for some
modes.
Same for other cases.
> } else if (5 == writecnt && 0 == readcnt) {
> return 0x50 | (writearr[1] & 0x0f);
> } else if (8 == writecnt && 0 == readcnt) {
> return 0x60 | (writearr[1] & 0x0f);
> } else if (5 == writecnt && 4 == readcnt) {
> /* XXX: TODO not supported by flashrom infrastructure!
>  * This mode, 7, discards the fifth byte in writecnt,
>  * but since we can not express that in flashrom, fail
>  * the operation for now.
>  */;
> } else if (4 == writecnt && readcnt >= 1 && readcnt <= 4) {
> return ((7 + readcnt) << 4) | (writearr[1] & 0x0f);
> }
> return 0;
>
> +}
>
> static int wbsio_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    			  unsigned int readcnt,
>    			  const unsigned char *writearr,
> @@ -95,52 +142,37 @@ int wbsio_check_for_spi(void)
>    return 0;
>  }
>
> -/* W83627DHG has 11 command modes:
> 1=1 command only
> 2=1 command+1 data write
> 3=1 command+2 data read
> 4=1 command+3 address
> 5=1 command+3 address+1 data write
> 6=1 command+3 address+4 data write
> 7=1 command+3 address+1 dummy address inserted by wbsio+4 data read
> 8=1 command+3 address+1 data read
> 9=1 command+3 address+2 data read
> a=1 command+3 address+3 data read
> b=1 command+3 address+4 data read
> mode[7:4] holds the command mode
> mode[3:0] holds SPI address bits [19:16]
> The Winbond SPI master only supports 20 bit addresses on the SPI bus. :\
> Would one more byte of RAM in the chip (to get all 24 bits) really make
> such a big difference?
> */
>
> static int wbsio_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    			  unsigned int readcnt,
>    			  const unsigned char *writearr,
>    			  unsigned char *readarr)
>  {
>    int i;
> uint8_t mode = 0;
> uint8_t mode = determine_mode(flash, writecnt, readcnt, writearr);
> msg_pspew("%s:", __func__);
> if (!mode) {
> msg_perr("%s: unsupported command type wr=%d rd=%d\n",
> 	__func__, writecnt, readcnt);
> /* Command type refers to the number of bytes read/written. */
> return SPI_INVALID_LENGTH;
> }
> if (1 == writecnt && 0 == readcnt) {
> mode = 0x10;
> } else if (2 == writecnt && 0 == readcnt) {
> msg_pspew(" cmd=%02x mode=%02x\n", writearr[0], mode);
> switch (mode & 0xF0) {

That masking would become unnecessary.
> case 0x10: break;
> case 0x20:
> OUTB(writearr[1], wbsio_spibase + 4);
> msg_pspew(" data=0x%02x", writearr[1]);
> mode = 0x20;
> } else if (1 == writecnt && 2 == readcnt) {
> mode = 0x30;
> } else if (4 == writecnt && 0 == readcnt) {
> break;
> case 0x30: break;
> case 0x40:
> msg_pspew(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < writecnt; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	msg_pspew("%02x", writearr[i]);
> }
> mode = 0x40 | (writearr[1] & 0x0f);

And here you could use
mode |= (writearr[1] & 0x0f);
Same for other cases where mode >= 0x40.
> } else if (5 == writecnt && 0 == readcnt) {
> break;
> case 0x50:
> msg_pspew(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < 4; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
>
> @@ -148,8 +180,8 @@ static int wbsio_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    	}
>    	OUTB(writearr[i], wbsio_spibase + i);
>    	msg_pspew(" data=0x%02x", writearr[i]);
> mode = 0x50 | (writearr[1] & 0x0f);
> } else if (8 == writecnt && 0 == readcnt) {
> break;
> case 0x60:
> msg_pspew(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < 4; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
>
> @@ -160,44 +192,30 @@ static int wbsio_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    		OUTB(writearr[i], wbsio_spibase + i);
>    		msg_pspew("%02x", writearr[i]);
>    	}
> mode = 0x60 | (writearr[1] & 0x0f);
> } else if (5 == writecnt && 4 == readcnt) {
> /* XXX: TODO not supported by flashrom infrastructure!
>  * This mode, 7, discards the fifth byte in writecnt,
>  * but since we can not express that in flashrom, fail
>  * the operation for now.
>  */
> ;
> } else if (4 == writecnt && readcnt >= 1 && readcnt <= 4) {
> break;
> case 0x70: /* unimplemented */
> break;
> default:
> msg_pspew(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < writecnt; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	msg_pspew("%02x", writearr[i]);
> }
> mode = ((7 + readcnt) << 4) | (writearr[1] & 0x0f);
> }
> msg_pspew(" cmd=%02x mode=%02x\n", writearr[0], mode);
> if (!mode) {
> msg_perr("%s: unsupported command type wr=%d rd=%d\n",
> 	__func__, writecnt, readcnt);
> /* Command type refers to the number of bytes read/written. */
> return SPI_INVALID_LENGTH;
> break;
>
> }
>
> OUTB(writearr[0], wbsio_spibase);
> OUTB(mode, wbsio_spibase + 1);
> programmer_delay(10);

Side note: I have absolutely no idea why this programmer_delay() is
here. It seems to be pointless... unless the hardware is even more
broken than I thought. No need to change it, I just wanted to point it out.
> if (!readcnt)
> return 0;
> msg_pspew("%s: returning data =", __func__);
> for (i = 0; i < readcnt; i++) {
> readarr[i] = INB(wbsio_spibase + 4 + i);
> msg_pspew(" 0x%02x", readarr[i]);
> if (readcnt > 0) {
> msg_pspew("%s: returning data =", __func__);
> for (i = 0; i < readcnt; i++) {
> 	readarr[i] = INB(wbsio_spibase + 4 + i);
> 	msg_pspew(" 0x%02x", readarr[i]);
> }
> msg_pspew("\n");
> }
> msg_pspew("\n");
> return 0;
>
> }

I don't see the point of this last hunk. Except for additional
indentation and an elimination of an early return, the code stays the
same. IMHO the early return is more readable, but I guess code
readability is in the eye of the beholder. I don't care that much either
way, it's your choice.

Regards,
Carl-Daniel