Re: [flashrom] [PATCH 04/12] use the max_data_read field of the new spi_programmer struct to simplify run_opcode.
Am 08.06.2011 04:55 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
ichspi.c | 41 +++++++++++++++++------------------------ 1 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/ichspi.c b/ichspi.c index 12727d1..dff6f32 100644 --- a/ichspi.c +++ b/ichspi.c @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data) {
- uint8_t maxlength = spi_programmer->max_data_read;
Mh. I see what you're doing here, and it makes sense, but a comment would be appreciated in the code: /* The maximum data length is identical on ICH/VIA for the maximum read length and for the maximum write length without opcode and address. Opcode and address are stored in separate registers, not in the data registers. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa. */
Any suggestions on how to improve that comment?
- if (spi_programmer->type == SPI_CONTROLLER_NONE) {
msg_perr("%s: unsupported chipset\n", __func__);return -1;- }
- if (datalength > maxlength) {
msg_perr("%s: Internal command size error for ""opcode 0x%02x, got datalength=%i, want <=%i\n",__func__, op.opcode, datalength, maxlength);return SPI_INVALID_LENGTH;- }
- switch (spi_programmer->type) { case SPI_CONTROLLER_VIA:
if (datalength > 16) {msg_perr("%s: Internal command size error for ""opcode 0x%02x, got datalength=%i, want <=16\n",__func__, op.opcode, datalength);return SPI_INVALID_LENGTH;}return ich7_run_opcode(op, offset, datalength, data, 16);if (datalength > 64) {msg_perr("%s: Internal command size error for ""opcode 0x%02x, got datalength=%i, want <=16\n",__func__, op.opcode, datalength);return SPI_INVALID_LENGTH;}return ich7_run_opcode(op, offset, datalength, data, 64);
return ich7_run_opcode(op, offset, datalength, data, maxlength);
if (datalength > 64) {msg_perr("%s: Internal command size error for ""opcode 0x%02x, got datalength=%i, want <=16\n",__func__, op.opcode, datalength);return SPI_INVALID_LENGTH;}msg_perr("%s: unsupported chipset\n", __func__);
/* If we ever get here, something really weird happened */return -1;
- /* If we ever get here, something really weird happened */
- return -1;
}
static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
Apart from the comment issue (which can be fixed without resubmitting), this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
On Thu, 09 Jun 2011 00:56:08 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 08.06.2011 04:55 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
ichspi.c | 41 +++++++++++++++++------------------------ 1 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/ichspi.c b/ichspi.c index 12727d1..dff6f32 100644 --- a/ichspi.c +++ b/ichspi.c @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data) {
- uint8_t maxlength = spi_programmer->max_data_read;
Mh. I see what you're doing here, and it makes sense, but a comment would be appreciated in the code: /* The maximum data length is identical on ICH/VIA for the maximum read length and for the maximum write length without opcode and address. Opcode and address are stored in separate registers, not in the data registers. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa. */
Any suggestions on how to improve that comment?
i am not sure if this is the right place to put the second half of that comment, because i dont really see the relevance inside that function: it just delegates its inputs. OTOH it makes sense to have it at a place where both versions share code. what about putting "The maximum payload lengths for read and write operations without opcode and address are identical on ICH/VIA - using max_data_read arbitrarily here." there and the rest inside ich_spi_send_command in the "/* translate read/write array/count */" comment-block?
Am 09.06.2011 06:44 schrieb Stefan Tauner:
On Thu, 09 Jun 2011 00:56:08 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 08.06.2011 04:55 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
ichspi.c | 41 +++++++++++++++++------------------------ 1 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/ichspi.c b/ichspi.c index 12727d1..dff6f32 100644 --- a/ichspi.c +++ b/ichspi.c @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data) {
- uint8_t maxlength = spi_programmer->max_data_read;
Mh. I see what you're doing here, and it makes sense, but a comment would be appreciated in the code: /* The maximum data length is identical on ICH/VIA for the maximum read length and for the maximum write length without opcode and address. Opcode and address are stored in separate registers, not in the data registers. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa. */
Any suggestions on how to improve that comment?
i am not sure if this is the right place to put the second half of that comment, because i dont really see the relevance inside that function: it just delegates its inputs. OTOH it makes sense to have it at a place where both versions share code. what about putting "The maximum payload
I don't really like the word "payload". It has a totally different meaning in the coreboot world.
lengths for read and write operations without opcode and address are identical on ICH/VIA - using max_data_read arbitrarily here."
That description is incomplete and (for me) slightly confusing.
there and the rest inside ich_spi_send_command in the "/* translate read/write array/count */" comment-block?
Excellent idea, that location is where the _whole_ comment should live. It doesn't belong in run_opcode() anyway.
Regards, Carl-Daniel
Am 11.06.2011 21:04 schrieb Carl-Daniel Hailfinger:
Am 09.06.2011 06:44 schrieb Stefan Tauner:
On Thu, 09 Jun 2011 00:56:08 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 08.06.2011 04:55 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
ichspi.c | 41 +++++++++++++++++------------------------ 1 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/ichspi.c b/ichspi.c index 12727d1..dff6f32 100644 --- a/ichspi.c +++ b/ichspi.c @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data) {
- uint8_t maxlength = spi_programmer->max_data_read;
Mh. I see what you're doing here, and it makes sense, but a comment would be appreciated in the code: /* The maximum data length is identical on ICH/VIA for the maximum read length and for the maximum write length without opcode and address. Opcode and address are stored in separate registers, not in the data registers. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa. */
Any suggestions on how to improve that comment?
i am not sure if this is the right place to put the second half of that comment, because i dont really see the relevance inside that function: it just delegates its inputs. OTOH it makes sense to have it at a place where both versions share code. what about putting "The maximum payload
I don't really like the word "payload". It has a totally different meaning in the coreboot world.
lengths for read and write operations without opcode and address are identical on ICH/VIA - using max_data_read arbitrarily here."
That description is incomplete and (for me) slightly confusing.
Admittedly that's my fault because my text wasn't good either and you tried to stay close to it.
New suggested text for ich_spi_send_command (your location):
The maximum data length is identical for the maximum read length and for the maximum write length excluding opcode and address. Opcode and address are stored in separate registers, not in the data registers and are thus not counted towards data length. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa.
there and the rest inside ich_spi_send_command in the "/* translate read/write array/count */" comment-block?
Excellent idea, that location is where the _whole_ comment should live. It doesn't belong in run_opcode() anyway.
Followup, pasted from IRC: Maybe add some short blurb like "max_data_read == max_data_write for all Intel/VIA SPI masters" to run_opcode(). That would be readable and short enough.
Regards, Carl-Daniel
committed with the proposed comments in r1333, thanks!
-
Carl-Daniel Hailfinger -
Stefan Tauner