[flashrom] [PATCH] Convert SPI chips to partial write

[Carl-Daniel Hailfinger]

On 10.07.2010 20:15, Michael Karcher wrote:
> Am Samstag, den 10.07.2010, 19:41 +0200 schrieb Carl-Daniel Hailfinger:
>   
>> On 10.07.2010 19:21, Michael Karcher wrote:
>>     
>>
>> AFAIK all write_1 chips can do AAI. Right now AAI is broken and I'm
>> waiting for an ack for patch 1548. Switching to write_1 instead of AAI
>> allows me to avoid creating a wrapper for AAI.
>>     
> What is missing for 1548? Looks like just the formal ack, as the patch
> seems to already be tested by den_m. In that case, I would do a review
> of that patch.
>   

Yes, it is tested, but den_m apparently had problems with the current
code as well. If someone aborts flashrom, the chip will stay in AAI
mode. That's suboptimal.


>>>> -int spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
>>>> +/* real chunksize is up to 256, logical chunksize is 256 */
>>>> +int spi_chip_write_256_new(struct flashchip *flash, uint8_t *buf, int start, int len)
>>>>  {
>>>>  	if (!spi_programmer[spi_controller].write_256) {
>>>>  		msg_perr("%s called, but SPI page write is unsupported on this "
>>>>     
>>>>         
>>> Oops! The comment says that single-byte programming is used on
>>> non-256-capable masters, but the code does not handle the fallback to
>>> single bytes. I also am unable to find any other place in flashrom that
>>> checks for the block write capability before calling this function. Is
>>> that a bug?
>>>       
>> Mh. That's very non-obvious. The spi_programmer array has a .write_256
>> member for every controller. In case the controller can't do anything
>> except 1-byte writes, we set
>> .write_256 = spi_chip_write_1_new
>>     
> Not every controller. SPI_CONTROLLER_DUMMY really has NULL in this
> field. Your patch does not change that. And please kill the test if we
> are sure that write_256 is always non-null.
>   

Right. I wanted to add a write_256 function to the dummy programmer
anyway. I'll send an updated patch.
Side note: If no SPI controller is selected, write_256 is empty as well,
but I'd say that's not a problem because it should never happen.


>>> This reminds me of patch 1371. I'm all for getting that patch in before
>>> this patch and not move around the erase-on-write logic just to kill it.
>>> I will send a review soon.
>>>       
>> This patch is basically a part of patch 1371 with less bugs, more
>> consistency and working backwards compatibility to make merging and
>> reviewing easier.
>>     
>
> I would consider it just the other way around. This patch is about
> partial write, and includes parts of the semantically unrelated patch
> about having erase outside of the write functions. Of course, one can
> also add the write area infrastructure before removing the erase code
> from the write code, but it seemed to make sense to me to first yank out
> erase and ad-hoc partial write from the chip driver's write functions,
> then convert them to a partial-write interface and finally put
> everything below the generalized erase-block walking.
>   

One of the reasons I basically abandoned the idea of just ripping out
erase and ad-hoc partial write was the missing progress printing. As
long as we don't have a good way to print progress, I'm a bit reluctant
to rip out erase, but hopefully we have some good progress printing code
very soon.


>> I don't like the introduction of two new wrappers either, but those are
>> a lot less of a problem than the existing per-controller wrappers and
>> removing them later will be a lot less invasive than 1371.
>>     
> If your plan is to base a new 1371 on top of this patch, I'm perfectly
> fine with these wrappers.
>   

Thanks. That's indeed the plan.


>>> As soon as we have explicit erase, this check is too late in the write
>>> function. Don't we even have the same problem even at read time?
>>>       
>> Yes, this should be handled with max_rom_decode instead. I will send a
>> followup patch for max_rom_decode.spi in wbsio and kill the
>> wbsio-specific read+write.
>>     
> Great!
>   

Thanks for your review.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/