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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot