This patch simplifies ich spi read/write operations, and changes naming from "page" to "block".
The impact of modification have been made minimal, other parts of flashrom should not be affected.
Not tested due to lack of facility. Very appreciated if anyone can help.
Signed-off-by: FENG yu ning fengyuning1984@gmail.com
Index: flashrom/ichspi.c =================================================================== --- flashrom/ichspi.c (revision 3767) +++ flashrom/ichspi.c (working copy) @@ -150,10 +150,10 @@ static int program_opcodes(OPCODES * op); static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data); -static int ich_spi_read_page(struct flashchip *flash, uint8_t * buf, - int offset, int maxdata); -static int ich_spi_write_page(struct flashchip *flash, uint8_t * bytes, - int offset, int maxdata); +static int ich_spi_read_block(struct flashchip *flash, uint8_t * buf, + int offset, int block_size, int maxdata); +static int ich_spi_write_block(struct flashchip *flash, uint8_t * bytes, + int offset, int block_size, int maxdata);
OPCODES O_ST_M25P = { { @@ -478,28 +478,25 @@ return -1; }
-static int ich_spi_read_page(struct flashchip *flash, uint8_t * buf, int offset, - int maxdata) +static int ich_spi_read_block(struct flashchip *flash, uint8_t * buf, + int offset, int block_size, int maxdata) { - int page_size = flash->page_size; - uint32_t remaining = flash->page_size; + uint32_t remaining = block_size; int a;
- printf_debug("ich_spi_read_page: offset=%d, number=%d, buf=%p\n", - offset, page_size, buf); + printf_debug("ich_spi_read_block: offset=%d, block size=%d, buf=%p\n", + offset, block_size, buf);
- for (a = 0; a < page_size; a += maxdata) { + for (a = 0; a < block_size; a += maxdata) { if (remaining < maxdata) {
- if (spi_nbyte_read(offset + (page_size - remaining), - &buf[page_size - remaining], remaining)) { + if (spi_nbyte_read(offset + a, &buf[a], remaining)) { printf_debug("Error reading"); return 1; } remaining = 0; } else { - if (spi_nbyte_read(offset + (page_size - remaining), - &buf[page_size - remaining], maxdata)) { + if (spi_nbyte_read(offset + a, &buf[a], maxdata)) { printf_debug("Error reading"); return 1; } @@ -510,22 +507,21 @@ return 0; }
-static int ich_spi_write_page(struct flashchip *flash, uint8_t * bytes, - int offset, int maxdata) +static int ich_spi_write_block(struct flashchip *flash, uint8_t * bytes, + int offset, int block_size, int maxdata) { - int page_size = flash->page_size; - uint32_t remaining = page_size; + uint32_t remaining = block_size; int a;
- printf_debug("ich_spi_write_page: offset=%d, number=%d, buf=%p\n", - offset, page_size, bytes); + printf_debug("ich_spi_write_block: offset=%d, block size=%d, buf=%p\n", + offset, block_size, bytes);
- for (a = 0; a < page_size; a += maxdata) { + for (a = 0; a < block_size; a += maxdata) { if (remaining < maxdata) { if (run_opcode (curopcodes->opcode[0], - offset + (page_size - remaining), remaining, - &bytes[page_size - remaining]) != 0) { + offset + a, remaining, + &bytes[a]) != 0) { printf_debug("Error writing"); return 1; } @@ -533,8 +529,8 @@ } else { if (run_opcode (curopcodes->opcode[0], - offset + (page_size - remaining), maxdata, - &bytes[page_size - remaining]) != 0) { + offset + a, maxdata, + &bytes[a]) != 0) { printf_debug("Error writing"); return 1; } @@ -547,34 +543,29 @@
int ich_spi_read(struct flashchip *flash, uint8_t * buf) { - int i, rc = 0; + int rc = 0; int total_size = flash->total_size * 1024; - int page_size = flash->page_size; int maxdata = 64;
if (flashbus == BUS_TYPE_VIA_SPI) { maxdata = 16; }
- for (i = 0; (i < total_size / page_size) && (rc == 0); i++) { - rc = ich_spi_read_page(flash, (void *)(buf + i * page_size), - i * page_size, maxdata); - } + rc = ich_spi_read_block(flash, buf, 0, total_size, maxdata);
return rc; }
int ich_spi_write(struct flashchip *flash, uint8_t * buf) { - int i, j, rc = 0; + int i, rc = 0; int total_size = flash->total_size * 1024; - int page_size = flash->page_size; int erase_size = 64 * 1024; int maxdata = 64;
spi_disable_blockprotect();
- printf("Programming page: \n"); + printf("Programming flashchip: \n");
for (i = 0; i < total_size / erase_size; i++) { /* FIMXE: call the chip-specific spi_block_erase_XX instead. @@ -590,11 +581,12 @@ if (flashbus == BUS_TYPE_VIA_SPI) maxdata = 16;
- for (j = 0; j < erase_size / page_size; j++) { - ich_spi_write_page(flash, - (void *)(buf + (i * erase_size) + (j * page_size)), - (i * erase_size) + (j * page_size), maxdata); - } + ich_spi_write_block(flash, + (void *)(buf + (i * erase_size)), + i * erase_size, + erase_size, + maxdata); + }
printf("\n");
FENG Yu Ning wrote:
This patch simplifies ich spi read/write operations, and changes naming from "page" to "block".
The impact of modification have been made minimal, other parts of flashrom should not be affected.
Not tested due to lack of facility. Very appreciated if anyone can help.
Signed-off-by: FENG yu ning fengyuning1984@gmail.com
I like this patch a lot, but I would like it to be tested before it is committed. Unfortunately I don't have any hardware to test on. :\
Could someone with an Intel SPI board please test?
//Peter
On Wed, Nov 26, 2008 at 8:14 AM, Peter Stuge peter@stuge.se wrote:
I like this patch a lot, but I would like it to be tested before it is committed. Unfortunately I don't have any hardware to test on. :\
It's encouraging to hear that :-) I understand. Better leaving the patch on the list than commiting it non-tested.
yu ning
On 25.11.2008 06:15, FENG Yu Ning wrote:
This patch simplifies ich spi read/write operations, and changes naming from "page" to "block".
Can you change the naming to "chunk"? "page" and "block" have meanings which do not match the code very well.
The impact of modification have been made minimal, other parts of flashrom should not be affected.
I think I saw a data sheet which stated that you may not read arbitrarily large chunks of the chip. Although the current ICH SPI code is not matching the design of the parallel flash chip drivers, your change will effectively change the chunk size for reads.
There are some other architectural changes which look good at first, but they are unneeded. Look at ich_spi_read_block and tell me why you want block_size as a parameter. The block size should be stored in struct flashchip and not be passed around separately.
Regards, Carl-Daniel
On Wed, Nov 26, 2008 at 9:17 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Can you change the naming to "chunk"? "page" and "block" have meanings which do not match the code very well.
Yes.
I think I saw a data sheet which stated that you may not read arbitrarily large chunks of the chip. Although the current ICH SPI code is not matching the design of the parallel flash chip drivers, your change will effectively change the chunk size for reads.
We need to document it then. I wondered about it when reading the code, but I just could not figure out the reason. The original design is, after all, a loop inside another. Atomic operations (to the programmers) are reads of not more than 64 bytes, which, I think, have seperated them from each other. If the difference is some time delay between two chunk reads, it is better to delay it explicitly with a delay function.
A (not so confident) suggestion: iff. we do not exactly know what is the very thing to seperate chunk reads, what about still patch it(with some additional comment) and wait for the bug to come out? At that time, we know which chips do not like this and can solve the problem effectively.
There are some other architectural changes which look good at first, but they are unneeded. Look at ich_spi_read_block and tell me why you want block_size as a parameter. The block size should be stored in struct flashchip and not be passed around separately.
You are right. I have considered that, too. But, That could be a temporary solution. Let me explain a little further. At present, the flashchip structure change and this simplification is related to each other. I choose to divide them. Changing the flashchip structure affects more, therefore I deal with simplfication first. And this non-optimal design change is necessary to keep the code work while the flashchip structure stays unchanged. Later, we can also change the structure, without worrying about its effect to the ich spi operations. At some time we feel good(or not) about the structure, we can improve ich spi operations, making the design optimal.
I am not sure but changing the flashchip structure could be quite some work, is it?
What do you think?
yu ning
On Wed, Nov 26, 2008 at 11:51 AM, FENG Yu Ning fengyuning1984@gmail.com wrote:
On Wed, Nov 26, 2008 at 9:17 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
I think I saw a data sheet which stated that you may not read arbitrarily large chunks of the chip. Although the current ICH SPI code is not matching the design of the parallel flash chip drivers, your change will effectively change the chunk size for reads.
We need to document it then. I wondered about it when reading the code, but I just could not figure out the reason. The original design is, after all, a loop inside another. Atomic operations (to the programmers) are reads of not more than 64 bytes, which, I think, have seperated them from each other. If the difference is some time delay between two chunk reads, it is better to delay it explicitly with a delay function.
I find some info which supports both your memory and my suggestion.
I read the data sheet of SST29{E,L,V}E020 (which we support) again, carefully. Here is some excerpt:
"The Write operation has three functional cycles: the Software Data Protection load sequence, the page-load cycle, and the internal Write cycle. ... "See Figures 5 and 6 for the Page-Write cycle timing diagrams. If after the completion of the three-byte SDP load sequence or the initial byte-load cycle, the host loads a second byte into the page buffer within a byte-load cycle time (TBLC) of 100 µs, the SST29EE/LE/VE020 will stay in the page-load cycle. Additional bytes are then loaded consecutively. The page-load cycle will be terminated if no additional byte is loaded into the page buffer within 200 µs (TBLCO) from the last byte-load cycle, i.e., no subsequent WE# or CE# high-to-low transition after the last rising edge of WE# or CE#. Data in the page buffer can be changed by a subsequent byte-load cycle. The page-load period can continue indefinitely, as long as the host continues to load the device within the byte-load cycle time of 100 µs. The page to be loaded is determined by the page address of the last byte loaded."
Here are the things I can think of now: 1. We need a "random-write-or-page-write" field in the flashchip struct, to indicate different types of chips. Again, a good name is needed. The erase-write function would have an if-branch to 2. 2. For page-write chips, we need page size and delay-between-page-write info in the flashchip struct.
There are some other architectural changes which look good at first, but they are unneeded. Look at ich_spi_read_block and tell me why you want block_size as a parameter. The block size should be stored in struct flashchip and not be passed around separately.
You are right. I have considered that, too. But, That could be a temporary solution. Let me explain a little further. At present, the flashchip structure change and this simplification is related to each other. I choose to divide them. Changing the flashchip structure affects more, therefore I deal with simplfication first. And this non-optimal design change is necessary to keep the code work while the flashchip structure stays unchanged. Later, we can also change the structure, without worrying about its effect to the ich spi operations. At some time we feel good(or not) about the structure, we can improve ich spi operations, making the design optimal.
I am not sure but changing the flashchip structure could be quite some work, is it?
What do you think?
yu ning
Sorry for the unfinished mail.
1. We need a "random-write-or-page-write" field in the flashchip struct, to indicate different types of chips. Again, a good name is needed. The erase-write function would have an if-branch to choose different writing mechanisms.
erase-write (...) { isVIA if random_write { ... } else { /* page write */ ... } }
2. For page-write chips, we need page size and delay-time-between-page-write info in the flashchip struct. The writing mechanism will look like:
loop { erase loop{ /* we need to fill the whole page whose size is larger than SPID's 64 bytes. */ write_chunk } delay }
3. For random-write chips, we will use the simplified writing mechanism.
yu ning
-
Carl-Daniel Hailfinger -
FENG Yu Ning -
Peter Stuge