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@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
On Wed, 13 Jul 2011 01:09:01 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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
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).
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 stefan.tauner@student.tuwien.ac.at 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 stefan.tauner@student.tuwien.ac.at
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 c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel