[U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase

[Stefan Agner]

On 2015-03-31 00:15, Scott Wood wrote:
> On Mon, 2015-03-30 at 23:26 +0200, Stefan Agner wrote:
>> On 2015-03-30 22:48, Scott Wood wrote:
>> > What is special about this controller, that caching makes sense here but
>> > not on other controllers?  If it makes sense everywhere, then the upper
>> > layer is the place to do it.
>> >
>>
>> Well, its not a caching which could be handled by upper layers: The NFC
>> reads the data into SRAM, where it gets hardware ECC checked. After
>> that, the hardware would be capable of copy the data into main memory
>> using DMA, however, the driver currently is not using this
>> functionality. Instead, we use memcpy, and copy only the data which are
>> requested.
> 
> The upper layer could choose to read the whole page at once.
> 
>> >> However, in order to avoid a full page getting allocated and memcpy'ed
>> >> when only writing a part of a page, we actually could re-enable that
>> >> feature. But I'm not sure about the semantics of a subpage writes: Does
>> >> the stack makes sure that the rest of the bytes are in the cache (e.g.
>> >> read before write)? From what I understand, a subpage write would only
>> >> copy (via vf610_nfc_write_buf) the data required into the the page
>> >> cache, which then gets written to the device. Who makes sure that the
>> >> rest of the page contains sound data?
>> >
>> > If the rest of the page is all 0xff it shouldn't affect the existing
>> > data -- as long as the controller isn't writing some non-0xff ECC bytes
>> > as a result.
>> >
>> > It's moot if subpage writes are disabled, though.
>> >
>>
>> Hm, and if its not 0xff? Is a sub page write valid in that case?
> 
> It's also OK if it matches what's already there.  Otherwise, it will
> corrupt.
> 
>> If a subpage write means that the rest of the page is 0xff, we could
>> also memset the whole SRAM 0xff and copy only the subpage data into the
>> buffer. We would still need to write the whole page, but only read parts
>> of it from main memory.
> 
> The generic NAND code already does this -- see the memset() in
> nand_do_write_ops().  In Linux we rely on this in the eLBC driver
> because it worked by accident and disabling subpages now would break UBI
> compatibility.
> 
>>  OTH, when we disable subpage write, and the stack prepares the whole
>> page, the page is probably still in the CPU cache anyway, and hence the
>> copy operation would be nearly as fast as memset/and partly memcpy....
>> Probably not worth the whole effort.
> 
> Especially since you'd be doing one write rather than four full-page
> "partial" writes.  Surely the bottleneck here is the NAND chip itself,
> not copying data to the buffer?
> 

Of course, if sub-page writes are supported, this would be faster. But
that is not the case (the controller has something called virtual pages
for pages over 2K, but I guess that qualifies not as sub-page write).

Afaik, everything happens sequentially, so every operation is hurting
the overall performance.

>> >> I think it mainly makes sense when the higher layer reads OOB and then
>> >> the main data or visa-verse. I saw such access patterns at least in
>> >> U-Boot.
>> >
>> > Wouldn't it make more sense to not read a full page every time OOB is
>> > read?
>>
>> I think when ECC is enabled this is not possible. However, since OOB is
>> not ECC checked, we could also turn off ECC and read only the OOB.
>> However, I'm also not sure if that is supported by the controller, I
>> need to check that. If it both is not possible, I would argue that
>> remembering the page in SRAM is worth to effort to speed up page => OOB
>> or OOB => page access...
> 
> If the controller can't support reading OOB by itself, that would be an
> answer to "what is different about this controller"...  Though if
> caching is done to deal with that, it should be limited to only dealing
> with consecutive reads.  Don't cache writes.

Actually, I just realized that the driver is not caching writes.

     switch (command) {
     case NAND_CMD_PAGEPROG:
         nfc->page = -1;
+        vf610_nfc_transfer_size(nfc->regs, nfc->page_sz);
         vf610_nfc_send_commands(nfc->regs, NAND_CMD_SEQIN,
                     command, PROGRAM_PAGE_CMD_CODE);
         vf610_nfc_addr_cycle(mtd, column, page);
         break;

The nfc->page = -1 resets the page cache, so the next read triggers a
full page read.

I will check the performance consequences when disabling cache entirely,
and whether it would be possible to implement a OOB only read.

--
Stefan