| Submitter | Michael Karcher |
|---|---|
| Date | 2010-07-11 16:42:57 |
| Message ID | <1278866577-9243-1-git-send-email-flashrom@mkarcher.dialup.fu-berlin.de> |
| Download | mbox | patch |
| Permalink | /patch/1609/ |
| State | New |
| Headers | show |
Comments
On 11.07.2010 18:42, Michael Karcher wrote: > Read function tested for chunk size 62 and with start offset 3, works > as expected. > > Signed-off-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> > --- > spi25.c | 80 +++++++++++++++++++++++++------------------------------------- > 1 files changed, 32 insertions(+), 48 deletions(-) > > diff --git a/spi25.c b/spi25.c > index 51be397..260dd1c 100644 > --- a/spi25.c > +++ b/spi25.c > @@ -879,35 +879,27 @@ int spi_nbyte_read(int address, uint8_t *bytes, int len) > int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) > { > int rc = 0; > - int i, j, starthere, lenhere; > + int ofs, startofs, endofs; /* offsets relative to page begin */ > int page_size = flash->page_size; > - int toread; > - > - /* Warning: This loop has a very unusual condition and body. > - * The loop needs to go through each page with at least one affected > - * byte. The lowest page number is (start / page_size) since that > - * division rounds down. The highest page number we want is the page > - * where the last byte of the range lives. That last byte has the > - * address (start + len - 1), thus the highest page number is > - * (start + len - 1) / page_size. Since we want to include that last > - * page as well, the loop condition uses <=. > - */ > - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { > - /* Byte position of the first byte in the range in this page. */ > - /* starthere is an offset to the base address of the chip. */ > - starthere = max(start, i * page_size); > - /* Length of bytes in the range in this page. */ > - lenhere = min(start + len, (i + 1) * page_size) - starthere; > - for (j = 0; j < lenhere; j += chunksize) { > - toread = min(chunksize, lenhere - j); > - rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread); > + int pageaddr, toread; > + > /* startofs is an offset to current page address. */ > + startofs = start % page_size; > + /* iterate over all affected pages */ > + for (pageaddr = start - startofs; pageaddr < start + len; > + pageaddr += page_size) { > Please use tabs here, at least for the first 8-space blocks. > + endofs = min(page_size, start + len - pageaddr); > endofs is a misnomer. The correct name would be one_after_endofs, but that name is ugly. What about /* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs; and using (startofs + lenhere) in place of endofs everywhere? > + /* iterate over all chunks in the current page */ > + for (ofs = startofs; ofs < endofs; ofs += chunksize) { > + toread = min(chunksize, endofs - ofs); > + rc = spi_nbyte_read(pageaddr + ofs, buf, toread); > + buf += toread; > if (rc) > - break; > + goto leave_page_loop; > return rc; > } > - if (rc) > - break; > + startofs = 0; /* all further pages processed from start */ > } > > +leave_page_loop: > Not needed if you return rc immediately in the error case. > return rc; > } > Here is my variant of your code. Feel free to ignore it completely, I just wanted to send it for reference. int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0; int j, lenhere; int page_size = flash->page_size; int pageaddr, toread, startofs; /* This loop needs to go through each page with at least one affected * byte. */ startofs = start % page_size; for (pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) { /* startofs is an offset to current page address. */ /* Number of bytes to write in this page. */ lenhere = min(page_size, start + len - pageaddr) - startofs; for (j = startofs; j < startofs + lenhere; j += chunksize) { toread = min(chunksize, startofs + lenhere - j); rc = spi_nbyte_read(pageaddr + j, buf, toread); if (rc) return rc; buf += toread; } startofs = 0; } return rc; } Regards, Carl-Daniel
Am Dienstag, den 13.07.2010, 16:04 +0200 schrieb Carl-Daniel Hailfinger: > > - int i, j, starthere, lenhere; > > + int ofs, startofs, endofs; /* offsets relative to page begin */ > > int page_size = flash->page_size; > > - int toread; > > - > > - /* Warning: This loop has a very unusual condition and body. > > - * The loop needs to go through each page with at least one affected > > - * byte. The lowest page number is (start / page_size) since that > > - * division rounds down. The highest page number we want is the page > > - * where the last byte of the range lives. That last byte has the > > - * address (start + len - 1), thus the highest page number is > > - * (start + len - 1) / page_size. Since we want to include that last > > - * page as well, the loop condition uses <=. > > - */ > > - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { > > - /* Byte position of the first byte in the range in this page. */ > > - /* starthere is an offset to the base address of the chip. */ > > - starthere = max(start, i * page_size); > > - /* Length of bytes in the range in this page. */ > > - lenhere = min(start + len, (i + 1) * page_size) - starthere; > > - for (j = 0; j < lenhere; j += chunksize) { > > - toread = min(chunksize, lenhere - j); > > - rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread); > > + int pageaddr, toread; > > + > /* startofs is an offset to current page address. */ See the comment at the variable declaration (which is quite near to here if the long comment has been snipped. I don't think I have to repeat that here. > Please use tabs here, at least for the first 8-space blocks. Right, fixed. Checked with an editor that shows spaces. > > + endofs = min(page_size, start + len - pageaddr); > endofs is a misnomer. The correct name would be one_after_endofs, but > that name is ugly. What about > > /* Number of bytes to write in this page. */ > lenhere = min(page_size, start + len - pageaddr) - startofs; > > and using (startofs + lenhere) in place of endofs everywhere? One could do that, but as we are not going to use the length as is, but always (lenhere + startofs) I consider subtracting startofs here just to add it on all uses of the variable overly contrieved. I think it would be better to spend energy in finding a better name than to circumvent the use of a value that would be needed just because we can't really name it. What about "past_end_ofs", together, accompanied by introducing an underscore in start_ofs too? > int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) > { > int rc = 0; > int j, lenhere; > int page_size = flash->page_size; > int pageaddr, toread, startofs; > > /* This loop needs to go through each page with at least one affected > * byte. > */ > startofs = start % page_size; > for (pageaddr = start - startofs; pageaddr < start + len; pageaddr += page_size) { > /* startofs is an offset to current page address. */ > /* Number of bytes to write in this page. */ > lenhere = min(page_size, start + len - pageaddr) - startofs; > for (j = startofs; j < startofs + lenhere; j += chunksize) { As this double-loop construction is a bit contrieved, I really prefer having loop variables as meaningfull as possible. This is why I renamed "j" to "ofs", so that we know it contains an offset which has to be added to a the page address to produce an eeprom address. Another consideration would be to split the chunking logic from the read/write calls, just like you just proposed for the eraseblock walker. This prevents having the chunking logic twice. But as I know that I am prone to adding too many layers of abstraction, I want to hear a second oppinion on that. Regards, Michael Karcher
Patch
diff --git a/spi25.c b/spi25.c index 51be397..260dd1c 100644 --- a/spi25.c +++ b/spi25.c @@ -879,35 +879,27 @@ int spi_nbyte_read(int address, uint8_t *bytes, int len) int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0; - int i, j, starthere, lenhere; + int ofs, startofs, endofs; /* offsets relative to page begin */ int page_size = flash->page_size; - int toread; - - /* Warning: This loop has a very unusual condition and body. - * The loop needs to go through each page with at least one affected - * byte. The lowest page number is (start / page_size) since that - * division rounds down. The highest page number we want is the page - * where the last byte of the range lives. That last byte has the - * address (start + len - 1), thus the highest page number is - * (start + len - 1) / page_size. Since we want to include that last - * page as well, the loop condition uses <=. - */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { - /* Byte position of the first byte in the range in this page. */ - /* starthere is an offset to the base address of the chip. */ - starthere = max(start, i * page_size); - /* Length of bytes in the range in this page. */ - lenhere = min(start + len, (i + 1) * page_size) - starthere; - for (j = 0; j < lenhere; j += chunksize) { - toread = min(chunksize, lenhere - j); - rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread); + int pageaddr, toread; + + startofs = start % page_size; + /* iterate over all affected pages */ + for (pageaddr = start - startofs; pageaddr < start + len; + pageaddr += page_size) { + endofs = min(page_size, start + len - pageaddr); + /* iterate over all chunks in the current page */ + for (ofs = startofs; ofs < endofs; ofs += chunksize) { + toread = min(chunksize, endofs - ofs); + rc = spi_nbyte_read(pageaddr + ofs, buf, toread); + buf += toread; if (rc) - break; + goto leave_page_loop; } - if (rc) - break; + startofs = 0; /* all further pages processed from start */ } +leave_page_loop: return rc; } @@ -918,42 +910,34 @@ int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int spi_write_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize) { int rc = 0; - int i, j, starthere, lenhere; + int ofs, startofs, endofs; /* offsets relative to page begin */ /* FIXME: page_size is the wrong variable. We need max_writechunk_size * in struct flashchip to do this properly. All chips using * spi_chip_write_256 have page_size set to max_writechunk_size, so * we're OK for now. */ int page_size = flash->page_size; - int towrite; - - /* Warning: This loop has a very unusual condition and body. - * The loop needs to go through each page with at least one affected - * byte. The lowest page number is (start / page_size) since that - * division rounds down. The highest page number we want is the page - * where the last byte of the range lives. That last byte has the - * address (start + len - 1), thus the highest page number is - * (start + len - 1) / page_size. Since we want to include that last - * page as well, the loop condition uses <=. - */ - for (i = start / page_size; i <= (start + len - 1) / page_size; i++) { - /* Byte position of the first byte in the range in this page. */ - /* starthere is an offset to the base address of the chip. */ - starthere = max(start, i * page_size); - /* Length of bytes in the range in this page. */ - lenhere = min(start + len, (i + 1) * page_size) - starthere; - for (j = 0; j < lenhere; j += chunksize) { - towrite = min(chunksize, lenhere - j); - rc = spi_nbyte_program(starthere + j, buf + starthere - start + j, towrite); + int pageaddr, towrite; + + startofs = start % page_size; + /* iterate over all affected pages */ + for (pageaddr = start - startofs; pageaddr < start + len; + pageaddr += page_size) { + endofs = min(page_size, start + len - pageaddr); + /* iterate over all chunks in the current page */ + for (ofs = startofs; ofs < endofs; ofs += chunksize) { + towrite = min(chunksize, endofs - ofs); + rc = spi_nbyte_program(pageaddr + ofs, buf, towrite); + buf += towrite; if (rc) - break; + goto leave_page_loop; while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) programmer_delay(10); } - if (rc) - break; + startofs = 0; /* all further pages processed from start */ } +leave_page_loop: return rc; }
Read function tested for chunk size 62 and with start offset 3, works as expected. Signed-off-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> --- spi25.c | 80 +++++++++++++++++++++++++------------------------------------- 1 files changed, 32 insertions(+), 48 deletions(-)