[flashrom] [PATCH] Partial writes for SPI

[Carl-Daniel Hailfinger]

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/