On Mon, 2015-03-30 at 22:14 +0200, Stefan Agner wrote: > On 2015-03-30 19:02, Bill Pringlemeir wrote: > > On 24 Mar 2015, ste...@agner.ch wrote: > > > >> The driver tries to re-use the page buffer by storing the page > >> number of the current page in the buffer. The page is only read > >> if the requested page number is not currently in the buffer. When > >> a block is erased, the page number is marked as invalid if the > >> erased page equals the one currently in the cache. However, since > >> a erase block consists of multiple pages, also other page numbers > >> could be affected. > >> > >> The commands to reproduce this issue (on a written page): > >>> nand dump 0x800 > >>> nand erase 0x0 0x20000 > >>> nand dump 0x800 > >> > >> The second nand dump command returns the data from the buffer, > >> while in fact the page is erased (0xff). > >> > >> Avoid the hassle to calculate whether the page is affected or not, > >> but set the page buffer unconditionally to invalid instead. > >> > >> Signed-off-by: Stefan Agner <ste...@agner.ch> > >> --- > >> This are two bug fixes which would be nice if they would still > >> make it into 2015.04... > >> > >> drivers/mtd/nand/vf610_nfc.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/vf610_nfc.c > >> b/drivers/mtd/nand/vf610_nfc.c index 928d58b..9de971c 100644 --- > >> a/drivers/mtd/nand/vf610_nfc.c +++ b/drivers/mtd/nand/vf610_nfc.c @@ > >> -369,8 +369,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, > >> unsigned command, break; > >> > >> case NAND_CMD_ERASE1: - if (nfc->page == page) - nfc->page = > >> -1; + nfc->page = -1; vf610_nfc_send_commands(nfc->regs, command, > >> NAND_CMD_ERASE2, ERASE_CMD_CODE); vf610_nfc_addr_cycle(mtd, column, > >> page); > > > > This change looks sensible. It is also possible that because sub-pages > > were removed that we could just remove the caching all together. It is > > possible that a higher layer may intentionally want to program and then > > do a read to verify. > > Hm, I now realize that this actually happened somewhat by accident. Back > when I migrated the driver to U-Boot, I removed the hwctl function since > it was an empty function. This probably lead to the problem with sub > page writes, which is why sub-page writes has been removed (by adding > NAND_NO_SUBPAGE_WRITE).
Subpages can be supported without hwctl, by implementing the appropriate commands -- if the hardware supports it (e.g. some controllers only want to do ECC on full pages). There's a comment in the driver saying that "NFC does not support sub-page reads and writes". If hwctl was empty it probably means that this controller does not expose an interface that matches well with hwctl. > 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. > > I guess we want to stay the same as the mainline Linux you are > > submitting. I haven't benchmarked the updates since sub-pages were > > removed to see if this is worth it. I think it was only ~10-20% in some > > benchmark I was doing with the 'caching'. > > 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? > > At least in the small, this is a minimal change that is correct. > > > > Ack-by: Bill Pringlemeir <bpringlem...@nbsps.com> > > Thanks for the Ack. If still possible, it would be nice to see that in > 2015.04... :-) I'd rather see the caching removed entirely. -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot