[flashrom] [PATCH 1/3] ichspi.c: refactor filling and reading the fdata/spid registers

[Stefan Tauner]

On Wed, 13 Jul 2011 01:09:01 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 01.07.2011 05:38 schrieb Stefan Tauner:
> > - add ich_fill_data to fill the chipset registers from an array
> > - add ich_read_data to copy the data from the chipset register into an array
> > - replace the existing code with calls to those functions
> >
> > NB: this changes the semantic of the *run_opcode functions a bit:
> > previously they could trash the chipset registers following the data registers because there
> > was no (explicit) length check (it was and is checked by the dispatching run_opcode method).
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> >   
> 
> I know you're just moving code, but this is a chance to clean up the
> code and improve readability.
> 
> 
> > diff --git a/ichspi.c b/ichspi.c
> > index 19e52d2..905ed70 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -592,6 +592,57 @@ static void ich_set_bbar(uint32_t min_addr)
> >  		msg_perr("Setting BBAR failed!\n");
> >  }
> >  
> > +/* Reads up to len byte from the fdata/spid register into the data array.
> > + * The amount actually read is limited by the maximum read size of the
> > + * chipset. */
> >   
> 
> Can you put the terminating */ on its own line like the multiline
> comments in other files?

done.

> > + static void ich_read_data(uint8_t *data, int len, int reg0_off)
> > + {
> > +	int a;
> >   
> 
> int i would be a nicer name... although you just moved that code, so
> it's not your fault.
> 
> 
> > +	uint32_t temp32 = 0;
> > +
> > +	if (len > spi_programmer->max_data_read)
> > +		len = spi_programmer->max_data_read;
> >   
> 
> Can this really happen? If yes, does it depend on bugs elsewhere in the
> code? This needs at least a msg_perr("bug in blabla, please report to
> flashrom@"), we might even want to have the function return int to
> handle errors.

when i am done (i.e. with hwseq applied) there will be 3 calls to this.
one in ich_hwseq_read and the others in ich7_run_opcode and
ich9_run_opcode respectively.
for the latter there is a common check in run_opcode and ich_hwseq_read
i am using "block_len = min(len, spi_programmer->max_data_read);" so
basically no this can currently not happen.

the reason that i have added this check is that ich_fill_data has the
same semantic and my hwseq relies on that. the difference is the return
value. ich_fill_data returns the length actually written.

> 
> > +
> > +	for (a = 0; a < len; a++) {
> > +		if ((a % 4) == 0)
> > +			temp32 = REGREAD32(reg0_off + (a));
> >   
> 
> Are the extra parentheses around a really needed?

no of course not... i did not touch the actual code at all (besides
indent and maybe curly braces iirc)...

> 
> > +	}
> > +}
> > +
> > +/* Fills up to len bytes from the data array into the fdata/spid registers.
> > + * The amount actually written is limited by the maximum write size of the
> > + * chipset and is returned by the function. */
> >   
> 
> The result is discarded anyway, and we probably shouldn't call this
> function with a too big len in the first place, so a return type of void
> may be the best choice.
> That said, if you decide to care about the result of this function,
> please make sure that ich_fill_data and ich_read_data have identical
> return types and check that the comments above the functions match the
> prototypes.

as explained above i use the return value of ich_fill_data. i don't
foresee a possibility that there will be other functions added that use
those methods, so the return value is useless in the other case. OTOH
having similar semantics for both is better. maybe i should get rid of
the return value and the buffer size check in both and handle it on my
own...

let's see what i will come up with :)

> > +
> > +	if (len <= 0)
> > +		return 0;
> >   
> 
> Superfluous check? len<0 should not happen, and len==0 won't execute the
> for loop below.

it is quite obvious for the loop but the following if is not that clear
if you read through it quickly (and might be implementation specific
as you have noted below...). so it is somewhat superfluous, but i think
readability is enhanced and it also makes this case independent from
further optimizations too.

> 
> > +
> > +	for (a = 0; a < len; a++) {
> > +		if ((a % 4) == 0) {
> > +			temp32 = 0;
> > +		}
> >   
> 
> Please kill superfluous {}.

done

> > +
> > +		temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);
> > +
> > +		if ((a % 4) == 3) {
> > +			REGWRITE32(reg0_off + (a - (a % 4)), temp32);
> > +		}
> >   
> 
> Please kill superfluous {}.

done

> > +	}
> > +	if (((a - 1) % 4) != 3) {
> >   
> 
> Yeargh! AFAIK modulo for negative integers is implementation-defined. So
> your len<=0 check a few lines above may have been a good idea. That
> said, it is a good idea to add a comment so others won't fall into that
> trap.
> And all this (a-1) stuff in the line above and below would be a lot more
> readable if you inserted a--; before the if.

will optimize...

> > +		REGWRITE32(reg0_off + ((a - 1) - ((a - 1) % 4)), temp32);
> > +	}
> >   
> 
> Please kill superfluous {}.

done

> 
> >  
> >  	/* Assemble SPIS */
> >  	temp16 = REGREAD16(ICH7_REG_SPIS);
> > @@ -765,15 +797,7 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
> >  	}
> >  
> >  	if ((!write_cmd) && (datalength != 0)) {
> > -		for (a = 0; a < datalength; a++) {
> > -			if ((a % 4) == 0) {
> > -				temp32 = REGREAD32(ICH7_REG_SPID0 + (a));
> > -			}
> > -
> > -			data[a] =
> > -			    (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> > -			    >> ((a % 4) * 8);
> > -		}
> > +		ich_read_data(data, datalength, ICH7_REG_SPID0);
> >   
> 
> Can you kill {} here as well?

done

> >  	}
> >  
> >  	return 0;
> > @@ -785,7 +809,6 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> >  	int write_cmd = 0;
> >  	int timeout;
> >  	uint32_t temp32;
> > -	uint32_t a;
> >  	uint64_t opmenu;
> >  	int opcode_index;
> >  
> > @@ -810,25 +833,8 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> >  	REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF) | temp32);
> >  
> >  	/* Program data into FDATA0 to N */
> > -	if (write_cmd && (datalength != 0)) {
> > -		temp32 = 0;
> > -		for (a = 0; a < datalength; a++) {
> > -			if ((a % 4) == 0) {
> > -				temp32 = 0;
> > -			}
> > -
> > -			temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);
> > -
> > -			if ((a % 4) == 3) {
> > -				REGWRITE32(ICH9_REG_FDATA0 + (a - (a % 4)),
> > -					   temp32);
> > -			}
> > -		}
> > -		if (((a - 1) % 4) != 3) {
> > -			REGWRITE32(ICH9_REG_FDATA0 +
> > -				   ((a - 1) - ((a - 1) % 4)), temp32);
> > -		}
> > -	}
> > +	if (write_cmd && (datalength != 0))
> > +		ich_fill_data(data, datalength, ICH9_REG_FDATA0);
> >  
> >  	/* Assemble SSFS + SSFC */
> >  	temp32 = REGREAD32(ICH9_REG_SSFS);
> > @@ -917,15 +923,7 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> >  	}
> >  
> >  	if ((!write_cmd) && (datalength != 0)) {
> > -		for (a = 0; a < datalength; a++) {
> > -			if ((a % 4) == 0) {
> > -				temp32 = REGREAD32(ICH9_REG_FDATA0 + (a));
> > -			}
> > -
> > -			data[a] =
> > -			    (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> > -			    >> ((a % 4) * 8);
> > -		}
> > +		ich_read_data(data, datalength, ICH9_REG_FDATA0);
> >   
> 
> Kill superfluous {}

done

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner