[PATCH] ICH SPI Preop handling

[Carl-Daniel Hailfinger]

The current ICH SPI preop handling is a hack which spews lots of
warnings, but still yields correct results.
With the multicommand infrastructure I introduced in r645, it became
possible to integrate ICH SPI preopcodes cleanly into the flashrom design.

The new code checks for every opcode in a multicommand array if it is a
preopcode. If yes, it checks if the next opcode is associated with that
preopcode and in that case it simply runs the opcode because the correct
preopcode will be run automatically before the opcode.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-ich9_multicommand_preop/ichspi.c
===================================================================
--- flashrom-ich9_multicommand_preop/ichspi.c	(Revision 725)
+++ flashrom-ich9_multicommand_preop/ichspi.c	(Arbeitskopie)
@@ -745,18 +745,25 @@
 int ich_spi_send_multicommand(struct spi_command *spicommands)
 {
    int ret = 0;
+	int oppos, preoppos;
    while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
+		/* Will the next loop iteration be taken? */
+		if ((spicommands + 1)->writecnt || (spicommands + 1)->readcnt) {
+			preoppos = find_preop(curopcodes, spicommands->writearr[0]);
+			oppos = find_opcode(curopcodes, (spicommands + 1)->writearr[0]);
+			/* Is the current op a preop for the next op? */
+			if ((oppos != -1) && (preoppos != -1) &&
+			    (curopcodes->opcode[oppos].atomic == preoppos)) {
+				printf_debug("opcode 0x%02x will be run as PREOP\n",
+					     spicommands->writearr[0]);
+				ret = 0;
+				spicommands++;
+				continue;
+			}
+		}	
+			
    	ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
    				   spicommands->writearr, spicommands->readarr);
-		/* This awful hack needs to be smarter.
-		 */
-		if ((ret == SPI_INVALID_OPCODE) &&
-		    ((spicommands->writearr[0] == JEDEC_WREN) ||
-		     (spicommands->writearr[0] == JEDEC_EWSR))) {
-			printf_debug(" due to SPI master limitation, ignoring"
-				     " and hoping it will be run as PREOP\n");
-			ret = 0;
-		}
    	spicommands++;
    }
    return ret;

[FENG Yu Ning]

Carl-Daniel Hailfinger wrote:
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-ich9_multicommand_preop/ichspi.c
>
> --- flashrom-ich9_multicommand_preop/ichspi.c   (Revision 725)
> +++ flashrom-ich9_multicommand_preop/ichspi.c   (Arbeitskopie)
> @@ -745,18 +745,25 @@
>  int ich_spi_send_multicommand(struct spi_command *spicommands)
>  {
>        int ret = 0;
> int oppos, preoppos;
>
> while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
> /* Will the next loop iteration be taken? */
> if ((spicommands + 1)->writecnt || (spicommands + 1)->readcnt) {

Are we sure there is a (spicommands + 1)?
> preoppos = find_preop(curopcodes, spicommands->writearr[0]);
> oppos = find_opcode(curopcodes, (spicommands + 1)->writearr[0]);
> /* Is the current op a preop for the next op? */

s/current op/current byte/
easier to parse?
> if ((oppos != -1) && (preoppos != -1) &&
> (curopcodes->opcode[oppos].atomic == preoppos)) {
> printf_debug("opcode 0x%02x will be run as PREOP\n",
> spicommands->writearr[0]);
> ret = 0;
> spicommands++;
> continue;
> }
> }
>
> ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
>                                           spicommands->writearr, spicommands->readarr);
> /* This awful hack needs to be smarter.
> */
> if ((ret == SPI_INVALID_OPCODE) &&
> ((spicommands->writearr[0] == JEDEC_WREN) ||
> (spicommands->writearr[0] == JEDEC_EWSR))) {
> printf_debug(" due to SPI master limitation, ignoring"
> " and hoping it will be run as PREOP\n");
> ret = 0;
> }
>
> spicommands++;
>        }
>        return ret;

I will write it this way:

int ich_spi_send_multicommand(struct spi_command *cmd)
{
        int ret, op, pre;
        /* better if I could write cmd->wcnt, cmd->rcnt */
        for (; cmd->writecnt || cmd->readcnt; cmd++) {
                /* if we are sure there is a (cmd + 1) */
                if ((cmd + 1)->writecnt || (cmd + 1)->readcnt) {
                        /* my bad to invent this eye-hurting 'curopcodes' */
                        pre = find_preop (curopcodes,  cmd->writearr[0]);
                        op  = find_opcode(curopcodes, (cmd + 1)->writearr[0]);
                        if ((op != -1) && (pre != -1) &&
                            /* see struct _OPCODE comments, atomic
value is error prone.
                               I am considering flattening struct
OPCODES, so that we can
                               write curop[i].atomic and curpre[i]
instead of this looong
                               expression */
                            (curopcodes->opcode[op].atomic-- == pre))
                                continue;
                }

ret = ich_spi_send_command(cmd->writecnt, cmd->readcnt,
                                           cmd->writearr, cmd->readarr);
                if (ret) break;
        }
        return ret;
}

do you think it is easier to read?

yu ning

[FENG Yu Ning]

FENG Yu Ning wrote:
> /* see struct _OPCODE comments, atomic
> value is error prone.
>                               I am considering flattening struct
> OPCODES, so that we can
>                               write curop[i].atomic and curpre[i]
> instead of this looong
>                               expression */

Gmail messed things up. sigh.
> do you think it is easier to read?

Well, after fixing those broken lines...

yu ning

[Carl-Daniel Hailfinger]

On 16.09.2009 17:49, FENG Yu Ning wrote:
> Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-ich9_multicommand_preop/ichspi.c
> >
> > --- flashrom-ich9_multicommand_preop/ichspi.c   (Revision 725)
> > +++ flashrom-ich9_multicommand_preop/ichspi.c   (Arbeitskopie)
> > @@ -745,18 +745,25 @@
> >  int ich_spi_send_multicommand(struct spi_command *spicommands)
> >  {
> >        int ret = 0;
> >   int oppos, preoppos;
> >  while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
> >           /* Will the next loop iteration be taken? */
> >           if ((spicommands + 1)->writecnt || (spicommands + 1)->readcnt) {
>
> Are we sure there is a (spicommands + 1)?

Yes. We use a struct spicommand with readcnt=writecnt=0 as last member
of the array, so as long as readcnt||writecnt we know there will be at
least one member after this one.
> >                   preoppos = find_preop(curopcodes, spicommands->writearr[0]);
> >                   oppos = find_opcode(curopcodes, (spicommands + 1)->writearr[0]);
> >                   /* Is the current op a preop for the next op? */
>
> s/current op/current byte/
> easier to parse?

Maybe s/op/opcode/ which should be a bit more obvious.
> >                   if ((oppos != -1) && (preoppos != -1) &&
> >                       (curopcodes->opcode[oppos].atomic == preoppos)) {
> >                           printf_debug("opcode 0x%02x will be run as PREOP\n",
> >                                        spicommands->writearr[0]);
> >                           ret = 0;
> >                           spicommands++;
> >                           continue;
> >                   }
> >           }
> >          ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
> >                                     spicommands->writearr, spicommands->readarr);
> >           /* This awful hack needs to be smarter.
> >            */
> >           if ((ret == SPI_INVALID_OPCODE) &&
> >               ((spicommands->writearr[0] == JEDEC_WREN) ||
> >                (spicommands->writearr[0] == JEDEC_EWSR))) {
> >                   printf_debug(" due to SPI master limitation, ignoring"
> >                                " and hoping it will be run as PREOP\n");
> >                   ret = 0;
> >           }
> >          spicommands++;
> >  }
> >  return ret;
>
> I will write it this way:
>
> int ich_spi_send_multicommand(struct spi_command *cmd)
> {
>         int ret, op, pre;

op and pre are positions, not opcodes/preopcodes. That's why I called
them oppos and preoppos. We could call them opindex and preopindex
instead. What do you think?
>     /* better if I could write cmd->wcnt, cmd->rcnt */

rcnt and wcnt are a bit short. We want to make this easy to
read+understand. Too short variable names mean new developers have to
search for the meaning.
>     for (; cmd->writecnt || cmd->readcnt; cmd++) {

I like the for loop. We should use it for all multicommand functions.
>             /* if we are sure there is a (cmd + 1) */

Yes.
>             if ((cmd + 1)->writecnt || (cmd + 1)->readcnt) {
>                     /* my bad to invent this eye-hurting 'curopcodes' */
>                     pre = find_preop (curopcodes,  cmd->writearr[0]);
>                     op  = find_opcode(curopcodes, (cmd + 1)->writearr[0]);
>                     if ((op != -1) && (pre != -1) &&
>                         /* see struct _OPCODE comments, atomic value is error prone.

I did not see any comments there.
>                            I am considering flattening struct OPCODES, so that we can
>                            write curop[i].atomic and curpre[i] instead of this looong
>                            expression */
>                         (curopcodes->opcode[op].atomic-- == pre))

The .atomic-- is a bug because it changes the atomic value.
>                             continue;
>             }
>
>             ret = ich_spi_send_command(cmd->writecnt, cmd->readcnt,
>                                        cmd->writearr, cmd->readarr);
>             if (ret) break;
>     }
>     return ret;
>
> }
>
> do you think it is easier to read?

Yes. I'll send a new patch in a few minutes.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 16.09.2009 15:33, Carl-Daniel Hailfinger wrote:
> The current ICH SPI preop handling is a hack which spews lots of
> warnings, but still yields correct results.
> With the multicommand infrastructure I introduced in r645, it became
> possible to integrate ICH SPI preopcodes cleanly into the flashrom design.
>
> The new code checks for every opcode in a multicommand array if it is a
> preopcode. If yes, it checks if the next opcode is associated with that
> preopcode and in that case it simply runs the opcode because the correct
> preopcode will be run automatically before the opcode.

Updated patch with comments from FENG Yu Ning incorporated. I also shortened
struct spi_command *spicommands
to
struct spi_command *cmds
in a few places. I think the code is still readable, and we don't break
the 80 char limit anymore.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-ich9_multicommand_preop/flash.h
===================================================================
--- flashrom-ich9_multicommand_preop/flash.h	(Revision 725)
+++ flashrom-ich9_multicommand_preop/flash.h	(Arbeitskopie)
@@ -498,7 +498,7 @@
 struct spi_programmer {
    int (*command)(unsigned int writecnt, unsigned int readcnt,
    	   const unsigned char *writearr, unsigned char *readarr);
-	int (*multicommand)(struct spi_command *spicommands);
+	int (*multicommand)(struct spi_command *cmds);

/* Optimized functions for this programmer */
    int (*read)(struct flashchip *flash, uint8_t *buf, int start, int len);
@@ -514,7 +514,7 @@
 int probe_spi_res(struct flashchip *flash);
 int spi_send_command(unsigned int writecnt, unsigned int readcnt,
    	const unsigned char *writearr, unsigned char *readarr);
-int spi_send_multicommand(struct spi_command *spicommands);
+int spi_send_multicommand(struct spi_command *cmds);
 int spi_write_enable(void);
 int spi_write_disable(void);
 int spi_chip_erase_60(struct flashchip *flash);
@@ -539,7 +539,7 @@
 uint32_t spi_get_valid_read_addr(void);
 int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
    		     const unsigned char *writearr, unsigned char *readarr);
-int default_spi_send_multicommand(struct spi_command *spicommands);
+int default_spi_send_multicommand(struct spi_command *cmds);

/* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
@@ -565,7 +565,7 @@
    	    const unsigned char *writearr, unsigned char *readarr);
 int ich_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
 int ich_spi_write_256(struct flashchip *flash, uint8_t * buf);
-int ich_spi_send_multicommand(struct spi_command *spicommands);
+int ich_spi_send_multicommand(struct spi_command *cmds);

/* it87spi.c */
 extern uint16_t it8716f_flashport;
Index: flashrom-ich9_multicommand_preop/spi.c
===================================================================
--- flashrom-ich9_multicommand_preop/spi.c	(Revision 725)
+++ flashrom-ich9_multicommand_preop/spi.c	(Arbeitskopie)
@@ -118,7 +118,7 @@
    					      writearr, readarr);
 }

-int spi_send_multicommand(struct spi_command *spicommands)
+int spi_send_multicommand(struct spi_command *cmds)
 {
    if (!spi_programmer[spi_controller].multicommand) {
    	fprintf(stderr, "%s called, but SPI is unsupported on this "
@@ -126,7 +126,7 @@
    	return 1;
    }

-	return spi_programmer[spi_controller].multicommand(spicommands);
+	return spi_programmer[spi_controller].multicommand(cmds);
 }

int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
@@ -148,13 +148,12 @@
    return spi_send_multicommand(cmd);
 }

-int default_spi_send_multicommand(struct spi_command *spicommands)
+int default_spi_send_multicommand(struct spi_command *cmds)
 {
    int result = 0;
-	while ((spicommands->writecnt || spicommands->readcnt) && !result) {
-		result = spi_send_command(spicommands->writecnt, spicommands->readcnt,
-					  spicommands->writearr, spicommands->readarr);
-		spicommands++;
+	for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
+		result = spi_send_command(cmds->writecnt, cmds->readcnt,
+					  cmds->writearr, cmds->readarr);
    }
    return result;
 }
@@ -494,7 +493,7 @@
 int spi_chip_erase_60(struct flashchip *flash)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -518,7 +517,7 @@
    	return result;
    }
    
-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -540,7 +539,7 @@
 int spi_chip_erase_c7(struct flashchip *flash)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -564,7 +563,7 @@
    	return result;
    }

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n", __func__);
    	return result;
@@ -596,7 +595,7 @@
 int spi_block_erase_52(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -614,7 +613,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -640,7 +639,7 @@
 int spi_block_erase_d8(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -658,7 +657,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n", __func__);
    	return result;
@@ -702,7 +701,7 @@
 int spi_block_erase_20(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -720,7 +719,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -779,7 +778,7 @@
 int spi_write_status_register(int status)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_EWSR_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_EWSR },
@@ -797,7 +796,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -808,7 +807,7 @@
 int spi_byte_program(int addr, uint8_t byte)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -826,7 +825,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -844,7 +843,7 @@
    	(address >> 8) & 0xff,
    	(address >> 0) & 0xff,
    };
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -873,7 +872,7 @@

memcpy(&cmd[4], bytes, len);

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
Index: flashrom-ich9_multicommand_preop/ichspi.c
===================================================================
--- flashrom-ich9_multicommand_preop/ichspi.c	(Revision 725)
+++ flashrom-ich9_multicommand_preop/ichspi.c	(Arbeitskopie)
@@ -742,22 +742,26 @@
    return result;
 }

-int ich_spi_send_multicommand(struct spi_command *spicommands)
+int ich_spi_send_multicommand(struct spi_command *cmds)
 {
    int ret = 0;
-	while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
-		ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
-					   spicommands->writearr, spicommands->readarr);
-		/* This awful hack needs to be smarter.
-		 */
-		if ((ret == SPI_INVALID_OPCODE) &&
-		    ((spicommands->writearr[0] == JEDEC_WREN) ||
-		     (spicommands->writearr[0] == JEDEC_EWSR))) {
-			printf_debug(" due to SPI master limitation, ignoring"
-				     " and hoping it will be run as PREOP\n");
-			ret = 0;
-		}
-		spicommands++;
+	int oppos, preoppos;
+	for (; (cmds->writecnt || cmds->readcnt) && !ret; cmds++) {
+		/* Is the next command valid or a terminator? */
+		if ((cmds + 1)->writecnt || (cmds + 1)->readcnt) {
+			preoppos = find_preop(curopcodes, cmds->writearr[0]);
+			oppos = find_opcode(curopcodes, (cmds + 1)->writearr[0]);
+			/* Is the current opcode a preopcode for the next opcode? */
+			if ((oppos != -1) && (preoppos != -1) &&
+			    (curopcodes->opcode[oppos].atomic == preoppos)) {
+				printf_debug("opcode 0x%02x will be run as PREOP\n",
+					     cmds->writearr[0]);
+				continue;
+			}
+		}	
+			
+		ret = ich_spi_send_command(cmds->writecnt, cmds->readcnt,
+					   cmds->writearr, cmds->readarr);
    }
    return ret;
 }

[FENG Yu Ning]

> I wrote:
> > Are we sure there is a (spicommands + 1)?

Carl-Daniel Hailfinger wrote:
> Yes. We use a struct spicommand with readcnt=writecnt=0 as last member
> of the array, so as long as readcnt||writecnt we know there will be at
> least one member after this one.

Thanks for the explanation.
> > > /* Is the current op a preop for the next op? */
> >
> > s/current op/current byte/
> > easier to parse?
>
> Maybe s/op/opcode/ which should be a bit more obvious.

Just make the two "op"s different. Not a big deal though.
> > int ret, op, pre;
>
> op and pre are positions, not opcodes/preopcodes. That's why I called
> them oppos and preoppos. We could call them opindex and preopindex
> instead. What do you think?

I admit that my naming is misleading. Both your namings are OK then.
> > /* better if I could write cmd->wcnt, cmd->rcnt */
>
> rcnt and wcnt are a bit short. We want to make this easy to
> read+understand. Too short variable names mean new developers have to
> search for the meaning.

I see.
> > for (; cmd->writecnt || cmd->readcnt; cmd++) {
>
> I like the for loop. We should use it for all multicommand functions.

great.
> > /* see struct _OPCODE comments, atomic value is error prone.
>
> I did not see any comments there.

http://www.flashrom.org/trac/flashrom/browser/trunk/ichspi.c#L105
atomic == 0 means no preop, while preop[0] is some op.
> > (curopcodes->opcode[op].atomic-- == pre))
>
> The .atomic-- is a bug because it changes the atomic value.

Oops. That was stupid. I should have written .atomic - 1

yu ning

[FENG Yu Ning]

Carl-Daniel Hailfinger wrote:
> > The new code checks for every opcode in a multicommand array if it is a
> > preopcode...
>
> Updated patch with comments from FENG Yu Ning incorporated. I also shortened
> struct spi_command *spicommands...
>
> Signed-off-by: Carl-Daniel Hailfinger

Except the "atomic" thing, the patch is

Acked-by: FENG Yu Ning

[Carl-Daniel Hailfinger]

Next try. I reworded the comment about ops/preops in the hope of making
it clearer. Thanks to FENG Yu Ning for the reviews.

The current ICH SPI preop handling is a hack which spews lots of
warnings, but still yields correct results.
With the multicommand infrastructure I introduced in r645, it became
possible to integrate ICH SPI preopcodes cleanly into the flashrom design.

The new code checks for every opcode in a multicommand array if it is a
preopcode. If yes, it checks if the next opcode is associated with that
preopcode and in that case it simply runs the opcode because the correct
preopcode will be run automatically before the opcode.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-ich9_multicommand_preop/flash.h
===================================================================
--- flashrom-ich9_multicommand_preop/flash.h	(Revision 725)
+++ flashrom-ich9_multicommand_preop/flash.h	(Arbeitskopie)
@@ -498,7 +498,7 @@
 struct spi_programmer {
    int (*command)(unsigned int writecnt, unsigned int readcnt,
    	   const unsigned char *writearr, unsigned char *readarr);
-	int (*multicommand)(struct spi_command *spicommands);
+	int (*multicommand)(struct spi_command *cmds);

/* Optimized functions for this programmer */
    int (*read)(struct flashchip *flash, uint8_t *buf, int start, int len);
@@ -514,7 +514,7 @@
 int probe_spi_res(struct flashchip *flash);
 int spi_send_command(unsigned int writecnt, unsigned int readcnt,
    	const unsigned char *writearr, unsigned char *readarr);
-int spi_send_multicommand(struct spi_command *spicommands);
+int spi_send_multicommand(struct spi_command *cmds);
 int spi_write_enable(void);
 int spi_write_disable(void);
 int spi_chip_erase_60(struct flashchip *flash);
@@ -539,7 +539,7 @@
 uint32_t spi_get_valid_read_addr(void);
 int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
    		     const unsigned char *writearr, unsigned char *readarr);
-int default_spi_send_multicommand(struct spi_command *spicommands);
+int default_spi_send_multicommand(struct spi_command *cmds);

/* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
@@ -565,7 +565,7 @@
    	    const unsigned char *writearr, unsigned char *readarr);
 int ich_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
 int ich_spi_write_256(struct flashchip *flash, uint8_t * buf);
-int ich_spi_send_multicommand(struct spi_command *spicommands);
+int ich_spi_send_multicommand(struct spi_command *cmds);

/* it87spi.c */
 extern uint16_t it8716f_flashport;
Index: flashrom-ich9_multicommand_preop/spi.c
===================================================================
--- flashrom-ich9_multicommand_preop/spi.c	(Revision 725)
+++ flashrom-ich9_multicommand_preop/spi.c	(Arbeitskopie)
@@ -118,7 +118,7 @@
    					      writearr, readarr);
 }

-int spi_send_multicommand(struct spi_command *spicommands)
+int spi_send_multicommand(struct spi_command *cmds)
 {
    if (!spi_programmer[spi_controller].multicommand) {
    	fprintf(stderr, "%s called, but SPI is unsupported on this "
@@ -126,7 +126,7 @@
    	return 1;
    }

-	return spi_programmer[spi_controller].multicommand(spicommands);
+	return spi_programmer[spi_controller].multicommand(cmds);
 }

int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
@@ -148,13 +148,12 @@
    return spi_send_multicommand(cmd);
 }

-int default_spi_send_multicommand(struct spi_command *spicommands)
+int default_spi_send_multicommand(struct spi_command *cmds)
 {
    int result = 0;
-	while ((spicommands->writecnt || spicommands->readcnt) && !result) {
-		result = spi_send_command(spicommands->writecnt, spicommands->readcnt,
-					  spicommands->writearr, spicommands->readarr);
-		spicommands++;
+	for (; (cmds->writecnt || cmds->readcnt) && !result; cmds++) {
+		result = spi_send_command(cmds->writecnt, cmds->readcnt,
+					  cmds->writearr, cmds->readarr);
    }
    return result;
 }
@@ -494,7 +493,7 @@
 int spi_chip_erase_60(struct flashchip *flash)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -518,7 +517,7 @@
    	return result;
    }
    
-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -540,7 +539,7 @@
 int spi_chip_erase_c7(struct flashchip *flash)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -564,7 +563,7 @@
    	return result;
    }

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n", __func__);
    	return result;
@@ -596,7 +595,7 @@
 int spi_block_erase_52(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -614,7 +613,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -640,7 +639,7 @@
 int spi_block_erase_d8(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -658,7 +657,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n", __func__);
    	return result;
@@ -702,7 +701,7 @@
 int spi_block_erase_20(struct flashchip *flash, unsigned int addr, unsigned int blocklen)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -720,7 +719,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -779,7 +778,7 @@
 int spi_write_status_register(int status)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_EWSR_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_EWSR },
@@ -797,7 +796,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -808,7 +807,7 @@
 int spi_byte_program(int addr, uint8_t byte)
 {
    int result;
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -826,7 +825,7 @@
    	.readarr	= NULL,
    }};

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
@@ -844,7 +843,7 @@
    	(address >> 8) & 0xff,
    	(address >> 0) & 0xff,
    };
-	struct spi_command spicommands[] = {
+	struct spi_command cmds[] = {
    {
    	.writecnt	= JEDEC_WREN_OUTSIZE,
    	.writearr	= (const unsigned char[]){ JEDEC_WREN },
@@ -873,7 +872,7 @@

memcpy(&cmd[4], bytes, len);

-	result = spi_send_multicommand(spicommands);
+	result = spi_send_multicommand(cmds);
    if (result) {
    	fprintf(stderr, "%s failed during command execution\n",
    		__func__);
Index: flashrom-ich9_multicommand_preop/ichspi.c
===================================================================
--- flashrom-ich9_multicommand_preop/ichspi.c	(Revision 725)
+++ flashrom-ich9_multicommand_preop/ichspi.c	(Arbeitskopie)
@@ -742,22 +742,29 @@
    return result;
 }

-int ich_spi_send_multicommand(struct spi_command *spicommands)
+int ich_spi_send_multicommand(struct spi_command *cmds)
 {
    int ret = 0;
-	while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
-		ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
-					   spicommands->writearr, spicommands->readarr);
-		/* This awful hack needs to be smarter.
-		 */
-		if ((ret == SPI_INVALID_OPCODE) &&
-		    ((spicommands->writearr[0] == JEDEC_WREN) ||
-		     (spicommands->writearr[0] == JEDEC_EWSR))) {
-			printf_debug(" due to SPI master limitation, ignoring"
-				     " and hoping it will be run as PREOP\n");
-			ret = 0;
-		}
-		spicommands++;
+	int oppos, preoppos;
+	for (; (cmds->writecnt || cmds->readcnt) && !ret; cmds++) {
+		/* Is the next command valid or a terminator? */
+		if ((cmds + 1)->writecnt || (cmds + 1)->readcnt) {
+			preoppos = find_preop(curopcodes, cmds->writearr[0]);
+			oppos = find_opcode(curopcodes, (cmds + 1)->writearr[0]);
+			/* Is the opcode of the current command listed in the
+			 * ICH struct OPCODES as associated preopcode for the
+			 * opcode of the next command?
+			 */
+			if ((oppos != -1) && (preoppos != -1) &&
+			    (curopcodes->opcode[oppos].atomic - 1 == preoppos)) {
+				printf_debug("opcode 0x%02x will be run as PREOP\n",
+					     cmds->writearr[0]);
+				continue;
+			}
+		}	
+			
+		ret = ich_spi_send_command(cmds->writecnt, cmds->readcnt,
+					   cmds->writearr, cmds->readarr);
    }
    return ret;
 }

[FENG Yu Ning]

Carl-Daniel Hailfinger wrote:
> Next try. I reworded the comment about ops/preops in the hope of making
> it clearer.
> Thanks to FENG Yu Ning for the reviews.

You are welcome.
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-ich9_multicommand_preop/flash.h
>
> --- flashrom-ich9_multicommand_preop/flash.h    (Revision 725)
> ...

Acked-by: FENG Yu Ning

[Carl-Daniel Hailfinger]

On 18.09.2009 17:16, FENG Yu Ning wrote:
> Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: FENG Yu Ning

Thanks, r727.

Regards,
Carl-Daniel