On 25.03.2010 10:46, Michael Karcher wrote:
> Am Donnerstag, den 25.03.2010, 03:10 +0100 schrieb Carl-Daniel
> Hailfinger:
>
>> + /* 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);
>> + if (rc)
>> + break;
>> + while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>> + programmer_delay(10);
>> + }
>> + if (rc)
>> + break;
>> + }
>>
>
> i would write this as (carful, untested):
>
> startofs = start % pagesize; /* offset relative to page begin */
> /* iterate over all pages to write */
> for(pageaddr = start - startofs; pageaddr < start + len;
> pageaddr += page_size) {
> endofs = min(pagesize, start + len - pageaddr);
> /* iterate over chunks in this page */
> for(j = startofs; j < endofs; j += chunksize) {
> towrite = min(chunksize, endofs - j);
> rc = spi_nbyte_program(pageaddr + j, buf, towrite);
> buf += towrite;
> if (rc)
> goto leave_page_loop;
> /* Copy/paste wait here */
> }
> startofs = 0; /* all pages but the first need to be programmed
> from the beginning */
> }
> leave_page_loop:
>
I prefer your logic over mine, but my logic has been tested by David
Hendricks, and that's why I committed mine.
If you could send your code as patch for spi_read_chunked and
spi_write_chunked, I'd be really happy.
> Your logic seem right, too, so this is a matter of taste. I think my
> version needs less explanation, that's why I would prefer it that way.
>
Fully agreed. We want your logic.
> We should aim to remove all the foo_spi_write_n functions and put the
> max write size into the spi controller structure. This probably means to
> write one entry for Intel (64) and one for VIA (16). Currently, some of
> the write functions do erase, others don't. Some of the write functions
> do unprotect, others don't. But that's for a later patch.
>
Will send that patch in a few minutes.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
