From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Mon, 30 Mar 2015 15:46:35 -0500 Subject: [U-Boot] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase In-Reply-To: <1a5c9a187ce3db91d1222ed8df7bbc0d@agner.ch> References: <1427216060-29120-1-git-send-email-stefan@agner.ch> <87mw2uo1va.fsf@nbsps.com> <1a5c9a187ce3db91d1222ed8df7bbc0d@agner.ch> Message-ID: <1427748395.22867.185.camel@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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, stefan at 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 > >> --- > >> 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 > > 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