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

[Stefan Agner]

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 <stefan at 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).

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?

> 
> I had seen that different FS seem to do 'write' and then immediately
> follow with a read.  If you believe the controller and the write status
> was ok, then I think it is fine to reuse the existing buffer and keep
> this caching.
> 
> For UBI, there were VID/EB header reads when sub-pages were enabled as
> they are in the same page; but these are now seperate pages.
> Especially, people may only care about code size in 'u-boot' and the
> typical use will only be to read an image (or config) from NAND so this
> optimization is probably not too helpful (execept maybe when flashing
> new kernels).  The 2nd patch set is not needed if we do not re-use
> existing data?

You mean "mtd: vf610_nfc: specify transfer size before each transfer"?
When removing the page cache here, it probably wouldn't be needed...
However, I think that patch would still make sense since it leads to
less data beeing copied between the NAND device and the host. Especially
the status command gets called quite often. I'm not 100% sure, but I
think the performance improvement between v3 and v4 of the Linux kernel
driver was due to that fix.


> 
> 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.

> 
> At least in the small, this is a minimal change that is correct.
> 
> Ack-by: Bill Pringlemeir <bpringlemeir at nbsps.com>

Thanks for the Ack. If still possible, it would be nice to see that in
2015.04... :-)

--
Stefan