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

[Stefan Agner]

On 2015-03-30 22:48, Scott Wood wrote:
> On Mon, 2015-03-30 at 22:40 +0200, Stefan Agner wrote:
>> On 2015-03-30 22:34, Scott Wood wrote:
>> > On Mon, 2015-03-30 at 13:02 -0400, 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.
>> >
>> > It's more than possible -- Peter Tyser posted patches to do exactly that
>> > for command-line NAND writes.
>> >
>> >> 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.
>> >
>> > If the upper layers want to cache then let them cache.
>> >
>>
>> In this case, the lower layer is doing the caching (the driver).
> 
> It shouldn't.
> 
>> Depending on what the idea was behind the reread (e.g. check the amount
>> of ECC erros just after write?), this caching inside the driver might
>> miss lead the upper layers...?
> 
> Yes.  It's also extra complexity that has already led to problems, as
> this patchset demonstrates.
> 
>>  However, if removing the caching in the driver would lead to a
>> performance drop, I would rather prefer to keep it...
> 
> 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.

As far as I remember, Bill once mentioned that memcpy using the CPU is
actually faster than DMA (while of course keeping the CPU busy, but that
is not really a problem for U-Boot since we anyway do not use interrupt
mode).

The driver currently stores the page number which was read the last
time. In case the upper layer request a read command for the same page,
we ignore that request.

I unify the discussion of the other thread, since its related:

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

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

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

>> > 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... :-)
> 
> I'd rather see the caching removed entirely.
> 

It would at least band aid the problem for now... :-)

--
Stefan