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

[Carl-Daniel Hailfinger]

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?


> + 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.


> +
> +	for (a = 0; a < len; a++) {
> +		if ((a % 4) == 0)
> +			temp32 = REGREAD32(reg0_off + (a));
>   

Are the extra parentheses around a really needed?


> +
> +		data[a] = (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))
> +			  >> ((a % 4) * 8);
>   

I think David had a suggestion on how to simplify this formula a bit.


> +	}
> +}
> +
> +/* 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.


> +static uint8_t ich_fill_data(const uint8_t *data, int len, int reg0_off)
> +{
> +	uint32_t temp32 = 0;
> +	int a;
>   

See ich_read_data.


> +
> +	if (len > spi_programmer->max_data_write)
> +		len = spi_programmer->max_data_write;
>   

See ich_read_data.


> +
> +	if (len <= 0)
> +		return 0;
>   

Superfluous check? len<0 should not happen, and len==0 won't execute the
for loop below.


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

Please kill superfluous {}.


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

Please kill superfluous {}.


> +	}
> +	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.


> +		REGWRITE32(reg0_off + ((a - 1) - ((a - 1) % 4)), temp32);
> +	}
>   

Please kill superfluous {}.


> +	return len;
> +}
> +
>  /* This function generates OPCODES from or programs OPCODES to ICH according to
>   * the chipset's SPI configuration lock.
>   *
> @@ -638,9 +689,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
>  {
>  	int write_cmd = 0;
>  	int timeout;
> -	uint32_t temp32 = 0;
> +	uint32_t temp32;
>  	uint16_t temp16;
> -	uint32_t a;
>  	uint64_t opmenu;
>  	int opcode_index;
>  
> @@ -664,26 +714,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset,
>  	REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF) | temp32);
>  
>  	/* Program data into SPID0 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(ICH7_REG_SPID0 + (a - (a % 4)),
> -					   temp32);
> -			}
> -		}
> -		if (((a - 1) % 4) != 3) {
> -			REGWRITE32(ICH7_REG_SPID0 +
> -				   ((a - 1) - ((a - 1) % 4)), temp32);
> -		}
> -
> -	}
> +	if (write_cmd && (datalength != 0))
> +		ich_fill_data(data, datalength, ICH7_REG_SPID0);
>   

Nice! Killed superfluous {}.


>  
>  	/* 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?


>  	}
>  
>  	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 {}


>  	}
>  
>  	return 0;
>   


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/