Re: [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

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

[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  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
>
> 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

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

[Stefan Tauner]

i have added a variable to hold (i % 4) in ich_fill_data:
unsigned int bite; /* offset of the current byte within a 32b word */

dunno if that makes it more or less readable? should i drop it again?
besides that i think there is not much room left for improvement.

and i got rid of the return values (hwseq needs a fixup to work with
this).

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

[Carl-Daniel Hailfinger]

Am 13.07.2011 15:33 schrieb Stefan Tauner:
> i have added a variable to hold (i % 4) in ich_fill_data:
> unsigned int bite; /* offset of the current byte within a 32b word */
>
> dunno if that makes it more or less readable? should i drop it again?

Hm. Maybe "octet" would be a better name than "bite". Or maybe "offs" or
"offset". Then again, the readability improvements are not really huge,
so keeping (i % 4) is also an option.
> besides that i think there is not much room left for improvement.
>
> and i got rid of the return values (hwseq needs a fixup to work with
> this).

Thanks!
> From: Stefan Tauner 
> Date: Tue, 28 Jun 2011 05:15:17 +0200
> Subject: [PATCH 1/3] ichspi.c: refactor filling and reading the fdata/spid registers
> 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
> Signed-off-by: Stefan Tauner
>
> ichspi.c |  121 +++++++++++++++++++++++++++++---------------------------------
>  1 files changed, 57 insertions(+), 64 deletions(-)
>
> diff --git a/ichspi.c b/ichspi.c
> index 99c4613..fb4184c 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -592,6 +592,54 @@ static void ich_set_bbar(uint32_t min_addr)
>    	msg_perr("Setting BBAR failed!\n");
>  }
>
> +/* Reads len bytes from the fdata/spid register into the data array.

s/Reads/Read/
> Note that using len > spi_programmer->max_data_read will return garbage or
> may even crash.
> */
> static void ich_read_data(uint8_t *data, int len, int reg0_off)
> {
> int i;
> uint32_t temp32 = 0;
> for (i = 0; i < len; i++) {
> if ((i % 4) == 0)
> 	temp32 = REGREAD32(reg0_off + i);
> data[i] = (temp32 >> ((i % 4) * 8)) & 0xff;
> }
>
> +}
>
> +/* Fills len bytes from the data array into the fdata/spid registers.

s/Fills/Fill/
> Note that using len > spi_programmer->max_data_write will trash the registers
> following the data registers.
> */
>
> +static void ich_fill_data(const uint8_t *data, int len, int reg0_off)
> +{
> uint32_t temp32 = 0;
> int i;
> unsigned int bite; /* offset of the current byte within a 32b word */

If you indeed keep the variable, please use this comment (or something
similar) instead:
/* Byte offset in the current little-endian 32bit register. */
> if (len <= 0)
> return;
> for (i = 0; i < len; i++) {
> bite = (i % 4);
> if (bite == 0)
> 	temp32 = 0;
> temp32 |= ((uint32_t) data[i]) << (bite * 8);
> if (bite == 3) /* last byte in this 32b word */

Can you use the following comment instead?
/* 32 bits are full, write them to registers. */
> 	REGWRITE32(reg0_off + (i - bite), temp32);
> }
> i--;
> bite = (i % 4);
> if (bite != 3) /* if last byte is not on a 32b boundary write it here */

Can you use the following comment instead?
/* Write remaining data to registers. */
> REGWRITE32(reg0_off + (i - bite), temp32);
>
> +}
>
> /* This function generates OPCODES from or programs OPCODES to ICH according to
> the chipset's SPI configuration lock.

Thanks for the cleanup!

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel