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

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

[Stefan Agner]

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@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);
-- 
2.3.3

[U-Boot] [PATCH 2/2] mtd: vf610_nfc: specify transfer size before each transfer

[Stefan Agner]

Testing showed, that commands like STATUS made the buffer dirty
when executed with NFC_SECSZ set to the page size. It looks
like the controller transfers bogus data when this register
is configured. When setting it to 0, the buffer does not get
altered while the status command still seems to work flawless.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/vf610_nfc.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 9de971c..d98dd28 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -146,6 +146,7 @@ struct vf610_nfc {
 	void __iomem	  *regs;
 	uint               column;
 	int                spareonly;
+	int		   page_sz;
 	int                page;
 	/* Status and ID are in alternate locations. */
 	int                alt_buf;
@@ -329,6 +330,11 @@ static void vf610_nfc_addr_cycle(struct mtd_info *mtd, int column, int page)
 				    ROW_ADDR_SHIFT, page);
 }
 
+static inline void vf610_nfc_transfer_size(void __iomem *regbase, int size)
+{
+	__raw_writel(size, regbase + NFC_SECTOR_SIZE);
+}
+
 /* Send command to NAND chip */
 static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 			      int column, int page)
@@ -342,12 +348,14 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 	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;
 
 	case NAND_CMD_RESET:
+		vf610_nfc_transfer_size(nfc->regs, 0);
 		vf610_nfc_send_command(nfc->regs, command, RESET_CMD_CODE);
 		break;
 	/*
@@ -363,6 +371,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 		if (nfc->page == page)
 			return;
 		nfc->page = page;
+		vf610_nfc_transfer_size(nfc->regs, nfc->page_sz);
 		vf610_nfc_send_commands(nfc->regs, NAND_CMD_READ0,
 					NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
 		vf610_nfc_addr_cycle(mtd, column, page);
@@ -370,6 +379,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 
 	case NAND_CMD_ERASE1:
 		nfc->page = -1;
+		vf610_nfc_transfer_size(nfc->regs, 0);
 		vf610_nfc_send_commands(nfc->regs, command,
 					NAND_CMD_ERASE2, ERASE_CMD_CODE);
 		vf610_nfc_addr_cycle(mtd, column, page);
@@ -377,11 +387,13 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 
 	case NAND_CMD_READID:
 		nfc->alt_buf = ALT_BUF_ID;
+		vf610_nfc_transfer_size(nfc->regs, 0);
 		vf610_nfc_send_command(nfc->regs, command, READ_ID_CMD_CODE);
 		break;
 
 	case NAND_CMD_STATUS:
 		nfc->alt_buf = ALT_BUF_STAT;
+		vf610_nfc_transfer_size(nfc->regs, 0);
 		vf610_nfc_send_command(nfc->regs, command,
 				       STATUS_READ_CMD_CODE);
 		break;
@@ -579,7 +591,6 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 	struct nand_chip *chip;
 	struct vf610_nfc *nfc;
 	int err = 0;
-	int page_sz;
 	struct vf610_nfc_config cfg = {
 		.hardware_ecc = 1,
 #ifdef CONFIG_SYS_NAND_BUSWIDTH_16BIT
@@ -633,9 +644,8 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 	chip->bbt_td = &bbt_main_descr;
 	chip->bbt_md = &bbt_mirror_descr;
 
-	page_sz = PAGE_2K + OOB_64;
-	page_sz += cfg.width == 16 ? 1 : 0;
-	vf610_nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+	nfc->page_sz = PAGE_2K + OOB_64;
+	nfc->page_sz += cfg.width == 16 ? 1 : 0;
 
 	/* Set configuration register. */
 	vf610_nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
@@ -664,16 +674,15 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 
 	chip->ecc.mode = NAND_ECC_SOFT; /* default */
 
-	page_sz = mtd->writesize + mtd->oobsize;
+	nfc->page_sz = mtd->writesize + mtd->oobsize;
 
 	/* Single buffer only, max 256 OOB minus ECC status */
-	if (page_sz > PAGE_2K + 256 - 8) {
+	if (nfc->page_sz > PAGE_2K + 256 - 8) {
 		dev_err(nfc->dev, "Unsupported flash size\n");
 		err = -ENXIO;
 		goto error;
 	}
-	page_sz += cfg.width == 16 ? 1 : 0;
-	vf610_nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+	nfc->page_sz += cfg.width == 16 ? 1 : 0;
 
 	if (cfg.hardware_ecc) {
 		if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
-- 
2.3.3

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

[Bill Pringlemeir]

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

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?

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

At least in the small, this is a minimal change that is correct.

Ack-by: Bill Pringlemeir <bpringlemeir@nbsps.com>

[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@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@nbsps.com>

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

--
Stefan

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

[Scott Wood]

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 <stefan@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 <bpringlemeir@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] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase

[Scott Wood]

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

> I guess we want to stay the same as the mainline Linux you are
> submitting.

So fix Linux. :-)

-Scott

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

[Stefan Agner]

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@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).
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...? However, if removing the caching in the
driver would lead to a performance drop, I would rather prefer to keep
it...

>> I guess we want to stay the same as the mainline Linux you are
>> submitting.
> 
> So fix Linux. :-)

The driver is actually not yet in Linux. That change however is included
in the latest revision. Hence, yes, we will :-)

--
Stefan

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

[Scott Wood]

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

-Scott

[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@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@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

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

[Scott Wood]

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?

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

-Scott

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

[Stefan Agner]

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] [PATCH 1/2] mtd: vf610_nfc: mark page as dirty on block erase

[Scott Wood]

On Tue, 2015-03-31 at 00:24 +0200, Stefan Agner wrote:
> 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.

OK.  In the meantime I'll apply this.

-Scott

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

[Bill Pringlemeir]

On 31 Mar 2015, scottwood at freescale.com wrote:

> On Tue, 2015-03-31 at 00:24 +0200, Stefan Agner wrote:
>> 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.

Yes, this is correct.  I should have looked at the code again.  It would
be interesting to see what it would do if we did cache.  I know I was
concerned about the upper layers and this caching; Ie, they may
explicitly want to check a physical re-read of a page just after
programming.

>> I will check the performance consequences when disabling cache
>> entirely, and whether it would be possible to implement a OOB only
>> read.

> OK.  In the meantime I'll apply this.

The patches are surely an improvement; maybe not perfect.  I think this
is a good decision.

On 2015-03-31 00:15, Scott Wood wrote:

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

The AHB bus that the NFC controller is on is relatively slow.  Here are
some numbers from 'AN4947-vybrid-bus-architechure',

Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),

   First read     Subsequent
   285            8              all caches on
   345            269            no cache, mmu
   437            371            no cache, no mmu

The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
DDR).  The AHB will be about four times slower.  Also the reads and
writes to the physical NAND must take place serially.  Here are the
program page steps.

  1. Issue controller Read full page to NFC buffer.
  2. Copy update partial page from DDR to NFC buffer.
  3. Issue write NAND page.

The alternate,

  1. Issue Read full page to NFC buffer.
  2. Copy full page to DDR.
  3. Update DDR with new data.
  4. Copy updated DDR page to NFC buffer.
  5. Issue write NAND page.

This involves a lot more bus transactions with the NFC AHB as either the
source and/or destination.  To disable the 'cache' (code in vf610_nfc),
we would actually need page program to only be called with full page
data (maybe that is the case?).  This is then much better,

  1. Copy full page to NFC buffer.
  2. Write NAND page.

Probably it would be beneficial to test this in the 'NAND_CMD_SEQIN' and
not issue the 'read'.  Unfortunately, I don't think there is a way to
know if a full page is being written from the driver with the current
NAND API?  Maybe the upper layer has already read the whole page, so the
NAND_CMD_SEQIN is always called with the page as current.  Maybe,

        case NAND_CMD_SEQIN: /* Pre-read for partial writes. */
+               /* Already read? */
+               if (nfc->page == page)   /* If this is always true, the */
+                       return;          /* NFC caching does nothing. */
        case NAND_CMD_READ0:
                column = 0;
-               /* Already read? */
-               if (nfc->page == page)
-                       return;
                nfc->page = page;

The AHB bus speed is a similar order to the NFC (33 MHz NFC clock versus
66MHz AHB clock) with both single beat after setup; you can actually
clock the NAND faster with out hw ECC with some NAND chips supporting
~50MHz.  Initially the NFC clock was under 10MHz; at this frequency NAND
chip timing is quite dominate.  After improving the NFC clock, the
actual transfer from/to the main DDR to the NFC buffers actually becomes
significant.

Well, that is my hypothesis.  I guess some measurements are always
beneficial.  I had measured with/without 'cache' and the results were not
insignificant.  I also put 'printk()' to see what pages were being
read/written and it seemed that both 'read followed by write' and 'write
followed by read' were issued quite frequently to the same page.  I am
also pretty sure that most systems will be structured like this.

     CPU -> L1 -> L2? - High speed bus -> DDR
     CPU              - Low speed bus  -> NFC

So, I don't think that this is Vybrid specific.  The PPC, ColdFire, etc
will probably have similar issues.  DMA has the same limitations as the
CPU, with setup overhead.  Of course, you can parallel the main CPU with
DMA but many systems want the NAND to complete synchronously; especially
u-boot.

Fwiw,
Bill Pringlemeir.

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

[Scott Wood]

On Tue, 2015-03-31 at 11:02 -0400, Bill Pringlemeir wrote:
> On 2015-03-31 00:15, Scott Wood wrote:
> 
> > 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?
> 
> The AHB bus that the NFC controller is on is relatively slow.  Here are
> some numbers from 'AN4947-vybrid-bus-architechure',
> 
> Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
> 
>    First read     Subsequent
>    285            8              all caches on
>    345            269            no cache, mmu
>    437            371            no cache, no mmu
> 
> The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
> DDR).  The AHB will be about four times slower.  Also the reads and
> writes to the physical NAND must take place serially.  Here are the
> program page steps.
> 
>   1. Issue controller Read full page to NFC buffer.
>   2. Copy update partial page from DDR to NFC buffer.
>   3. Issue write NAND page.

Why is any sort of read part of the write process?

If this controller can't handle subpage writes properly, then disable
them.

-Scott

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

[Stefan Agner]

On 2015-04-03 01:48, Scott Wood wrote:
> On Tue, 2015-03-31 at 11:02 -0400, Bill Pringlemeir wrote:
>> On 2015-03-31 00:15, Scott Wood wrote:
>>
>> > 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?
>>
>> The AHB bus that the NFC controller is on is relatively slow.  Here are
>> some numbers from 'AN4947-vybrid-bus-architechure',
>>
>> Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
>>
>>    First read     Subsequent
>>    285            8              all caches on
>>    345            269            no cache, mmu
>>    437            371            no cache, no mmu
>>
>> The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
>> DDR).  The AHB will be about four times slower.  Also the reads and
>> writes to the physical NAND must take place serially.  Here are the
>> program page steps.
>>
>>   1. Issue controller Read full page to NFC buffer.
>>   2. Copy update partial page from DDR to NFC buffer.
>>   3. Issue write NAND page.
> 
> Why is any sort of read part of the write process?

To recalculate the correct ECC, which is done in the controller, the
controller has to have the page in the SRAM. I will send out a patch
which implements vf610_nfc_write_subpage. And the read part is done when
the MTD subsystem calls NAND_CMD_SEQIN.

Actually, the Linux NAND driver supports subpage writes already by using
the generic nand_write_subpage_hwecc function. However, in U-Boot I
added driver specific page_read/page_write due to performance reasons:
http://lists.denx.de/pipermail/u-boot/2014-August/186293.html

However, I tried driver specific page_read/page_write functions for
Linux too, but I couldn't measure noticeable performance improvements. I
think the reason why it lead to noticeable improvements for U-Boot was
because U-Boot does not use caches so far. We could also switch to the
framework functions again, but those are more complex than necessary.
Given that U-Boot uses device specific binaries anyway, there is no size
advantage by using the (shared) MTD subsystems functions.

--
Stefan

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

[Scott Wood]

On Fri, 2015-04-03 at 20:09 +0200, Stefan Agner wrote:
> On 2015-04-03 01:48, Scott Wood wrote:
> > On Tue, 2015-03-31 at 11:02 -0400, Bill Pringlemeir wrote:
> >> On 2015-03-31 00:15, Scott Wood wrote:
> >>
> >> > 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?
> >>
> >> The AHB bus that the NFC controller is on is relatively slow.  Here are
> >> some numbers from 'AN4947-vybrid-bus-architechure',
> >>
> >> Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
> >>
> >>    First read     Subsequent
> >>    285            8              all caches on
> >>    345            269            no cache, mmu
> >>    437            371            no cache, no mmu
> >>
> >> The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
> >> DDR).  The AHB will be about four times slower.  Also the reads and
> >> writes to the physical NAND must take place serially.  Here are the
> >> program page steps.
> >>
> >>   1. Issue controller Read full page to NFC buffer.
> >>   2. Copy update partial page from DDR to NFC buffer.
> >>   3. Issue write NAND page.
> > 
> > Why is any sort of read part of the write process?
> 
> To recalculate the correct ECC, which is done in the controller, the
> controller has to have the page in the SRAM. I will send out a patch
> which implements vf610_nfc_write_subpage. And the read part is done when
> the MTD subsystem calls NAND_CMD_SEQIN.

Again, if this is the only way you can do subpage accesses then you
should not do them.

> Actually, the Linux NAND driver supports subpage writes already by using
> the generic nand_write_subpage_hwecc function. However, in U-Boot I
> added driver specific page_read/page_write due to performance reasons:
> http://lists.denx.de/pipermail/u-boot/2014-August/186293.html
> 
> However, I tried driver specific page_read/page_write functions for
> Linux too, but I couldn't measure noticeable performance improvements. I
> think the reason why it lead to noticeable improvements for U-Boot was
> because U-Boot does not use caches so far.

If you care about U-Boot's performance at all, enabling cache would be
the first thing I'd try...

-Scott

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

[Stefan Agner]

On 2015-04-03 22:15, Scott Wood wrote:
> On Fri, 2015-04-03 at 20:09 +0200, Stefan Agner wrote:
>> On 2015-04-03 01:48, Scott Wood wrote:
>> > On Tue, 2015-03-31 at 11:02 -0400, Bill Pringlemeir wrote:
>> >> On 2015-03-31 00:15, Scott Wood wrote:
>> >>
>> >> > 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?
>> >>
>> >> The AHB bus that the NFC controller is on is relatively slow.  Here are
>> >> some numbers from 'AN4947-vybrid-bus-architechure',
>> >>
>> >> Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
>> >>
>> >>    First read     Subsequent
>> >>    285            8              all caches on
>> >>    345            269            no cache, mmu
>> >>    437            371            no cache, no mmu
>> >>
>> >> The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
>> >> DDR).  The AHB will be about four times slower.  Also the reads and
>> >> writes to the physical NAND must take place serially.  Here are the
>> >> program page steps.
>> >>
>> >>   1. Issue controller Read full page to NFC buffer.
>> >>   2. Copy update partial page from DDR to NFC buffer.
>> >>   3. Issue write NAND page.
>> >
>> > Why is any sort of read part of the write process?
>>
>> To recalculate the correct ECC, which is done in the controller, the
>> controller has to have the page in the SRAM. I will send out a patch
>> which implements vf610_nfc_write_subpage. And the read part is done when
>> the MTD subsystem calls NAND_CMD_SEQIN.
> 
> Again, if this is the only way you can do subpage accesses then you
> should not do them.

Why not? IMHO, there are valid reason to do it, since we save coping
data over the bus (we save copying page size - write len of bytes)...
Also, I guess all NAND controller which do HW ECC need to read at least
ECC step size back to the controller... Maybe we can move the discussion
to the actual code (see "mtd: vf610_nfc: support subpage write").
 
>> Actually, the Linux NAND driver supports subpage writes already by using
>> the generic nand_write_subpage_hwecc function. However, in U-Boot I
>> added driver specific page_read/page_write due to performance reasons:
>> http://lists.denx.de/pipermail/u-boot/2014-August/186293.html
>>
>> However, I tried driver specific page_read/page_write functions for
>> Linux too, but I couldn't measure noticeable performance improvements. I
>> think the reason why it lead to noticeable improvements for U-Boot was
>> because U-Boot does not use caches so far.
> 
> If you care about U-Boot's performance at all, enabling cache would be
> the first thing I'd try...

Yes, we have that in our downstream since some months, and patch is
pending, see:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/215896

But it was not the first thing we did, we started with the functionality
needed to boot, such as reading from NAND... Hence we could now go back
and use the MTD subsystems generic functions. I do not have a strong
opinion on this...

--
Stefan

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

[Scott Wood]

On Fri, 2015-04-03 at 22:28 +0200, Stefan Agner wrote:
> On 2015-04-03 22:15, Scott Wood wrote:
> > On Fri, 2015-04-03 at 20:09 +0200, Stefan Agner wrote:
> >> On 2015-04-03 01:48, Scott Wood wrote:
> >> > On Tue, 2015-03-31 at 11:02 -0400, Bill Pringlemeir wrote:
> >> >> On 2015-03-31 00:15, Scott Wood wrote:
> >> >>
> >> >> > 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?
> >> >>
> >> >> The AHB bus that the NFC controller is on is relatively slow.  Here are
> >> >> some numbers from 'AN4947-vybrid-bus-architechure',
> >> >>
> >> >> Vybrid Cortex A5 to DDR (in CPU clocks 400/500MHz),
> >> >>
> >> >>    First read     Subsequent
> >> >>    285            8              all caches on
> >> >>    345            269            no cache, mmu
> >> >>    437            371            no cache, no mmu
> >> >>
> >> >> The NFC is on an AHB bus 32bit, 66MHz (not AXI 64bit, 133-166MHz like
> >> >> DDR).  The AHB will be about four times slower.  Also the reads and
> >> >> writes to the physical NAND must take place serially.  Here are the
> >> >> program page steps.
> >> >>
> >> >>   1. Issue controller Read full page to NFC buffer.
> >> >>   2. Copy update partial page from DDR to NFC buffer.
> >> >>   3. Issue write NAND page.
> >> >
> >> > Why is any sort of read part of the write process?
> >>
> >> To recalculate the correct ECC, which is done in the controller, the
> >> controller has to have the page in the SRAM. I will send out a patch
> >> which implements vf610_nfc_write_subpage. And the read part is done when
> >> the MTD subsystem calls NAND_CMD_SEQIN.
> > 
> > Again, if this is the only way you can do subpage accesses then you
> > should not do them.
> 
> Why not? IMHO, there are valid reason to do it, since we save coping
> data over the bus (we save copying page size - write len of bytes)...

According to http://www.linux-mtd.infradead.org/doc/ubi.html#L_subpage
the motivation for subpages is saving space, not time, and it's only
used for headers (specifically because using subpages may be slower). 
So it may not make a huge performance difference either way, even if
subpages are less efficient on this controller -- though it does have a
complexity cost that is higher than with most controllers.  It looks
like the space savings is around one page per block.

It'd be good to benchmark up front to be sure you're happy with the
speed/space/complexity tradeoff, though, since enabling/disabling
subpage writes breaks UBI compatibility.

> Also, I guess all NAND controller which do HW ECC need to read at least
> ECC step size back to the controller... Maybe we can move the discussion
> to the actual code (see "mtd: vf610_nfc: support subpage write").

No, most controller drivers do not read from the NAND chip during the
programming process.

-Scott

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

[Bill Pringlemeir]

On  3 Apr 2015, scottwood at freescale.com wrote:

> On Fri, 2015-04-03 at 22:28 +0200, Stefan Agner wrote:

>> Why not? IMHO, there are valid reason to do it, since we save coping
>> data over the bus (we save copying page size - write len of bytes)...

> According to http://www.linux-mtd.infradead.org/doc/ubi.html#L_subpage
> the motivation for subpages is saving space, not time, and it's only
> used for headers (specifically because using subpages may be slower).
> So it may not make a huge performance difference either way, even if
> subpages are less efficient on this controller -- though it does have
> a complexity cost that is higher than with most controllers.  It looks
> like the space savings is around one page per block.

I think that Artem wrote this.  I don't believe the document is
completely correct; I think he meant that sub-pages is not architected
as a speed improvement.  For instance, UBI will read both a VID (volume
ID) and EB (erase block).  As the are combined into one, then if a
driver needs to read a full page (like it is the only thing it supports)
then it is simple to read a full page with a VID/EB sub-pages.
Certainly for mounts with out 'fastmap', this will result in much faster
scans and initial UBI/UbiFS mounts?

It also saves one page overhead per erase block.  So in all cases,
sub-pages will optimize flash usage.  Probably performance depends on
the MTD driver and CPU.

> It'd be good to benchmark up front to be sure you're happy with the
> speed/space/complexity tradeoff, though, since enabling/disabling
> subpage writes breaks UBI compatibility.

I think that if you format the UbiFs without sub-pages and then update
code to handle sub-pages, you are fine.  If you have flash/UBI formatted
with sub-pages and then you disable it in the code, the disk is no
longer mountable.

In any case the document has,

  If the NAND flash supports sub-pages, then what can be done is ECC
  codes can be calculated on per-sub-page basis, instead of per-NAND
  page basis. In this case it becomes possible to read and write
  sub-pages independently.

Probably if we want sub-pages with this controller and hw-ecc, we need
to use the virtual buffer features of the chip.  The controller needs an
entire current buffer in the SRAM to calculate the hw-ecc to write.  So
even if it writes less than a full page, the entire page must be read to
calculate the hw-ecc to be written.  I am pretty sure that all
controllers that support hw-ecc will need to do this.

Fwiw,
Bill Pringlemeir.

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

[Scott Wood]

On Tue, 2015-04-07 at 10:06 -0400, Bill Pringlemeir wrote:
> On  3 Apr 2015, scottwood at freescale.com wrote:
> 
> > On Fri, 2015-04-03 at 22:28 +0200, Stefan Agner wrote:
> 
> >> Why not? IMHO, there are valid reason to do it, since we save coping
> >> data over the bus (we save copying page size - write len of bytes)...
> 
> > According to http://www.linux-mtd.infradead.org/doc/ubi.html#L_subpage
> > the motivation for subpages is saving space, not time, and it's only
> > used for headers (specifically because using subpages may be slower).
> > So it may not make a huge performance difference either way, even if
> > subpages are less efficient on this controller -- though it does have
> > a complexity cost that is higher than with most controllers.  It looks
> > like the space savings is around one page per block.
> 
> I think that Artem wrote this.  I don't believe the document is
> completely correct; I think he meant that sub-pages is not architected
> as a speed improvement.  For instance, UBI will read both a VID (volume
> ID) and EB (erase block).  As the are combined into one, then if a
> driver needs to read a full page (like it is the only thing it supports)
> then it is simple to read a full page with a VID/EB sub-pages.
> Certainly for mounts with out 'fastmap', this will result in much faster
> scans and initial UBI/UbiFS mounts?
> 
> It also saves one page overhead per erase block.  So in all cases,
> sub-pages will optimize flash usage.  Probably performance depends on
> the MTD driver and CPU.

Possibly.  Again, I recommend benchmarking before committing to it.

> > It'd be good to benchmark up front to be sure you're happy with the
> > speed/space/complexity tradeoff, though, since enabling/disabling
> > subpage writes breaks UBI compatibility.
> 
> I think that if you format the UbiFs without sub-pages and then update
> code to handle sub-pages, you are fine.  If you have flash/UBI formatted
> with sub-pages and then you disable it in the code, the disk is no
> longer mountable.

If that's the case, then that's even more reason to leave subpages
disabled until you're sure you want them.

> In any case the document has,
> 
>   If the NAND flash supports sub-pages, then what can be done is ECC
>   codes can be calculated on per-sub-page basis, instead of per-NAND
>   page basis. In this case it becomes possible to read and write
>   sub-pages independently.
> 
> Probably if we want sub-pages with this controller and hw-ecc, we need
> to use the virtual buffer features of the chip.  The controller needs an
> entire current buffer in the SRAM to calculate the hw-ecc to write.  So
> even if it writes less than a full page, the entire page must be read to
> calculate the hw-ecc to be written.

That limitation sounds similar to the Freescale NAND controllers that
I'm familiar with (eLBC and IFC).  For eLBC we do subpages by just
writing 0xff, because on that controller the ECC of all 0xff is all 0xff
so it doesn't disturb anything.  IFC has different ECC algorithms where
that isn't the case, so we disabled subpage write on IFC.

What is the virtual buffer feature?

> I am pretty sure that all controllers that support hw-ecc will need to do this.

Not if they can handle doing ECC on a single subpage.

-Scott

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

[Bill Pringlemeir]

On  7 Apr 2015, scottwood at freescale.com wrote:
> On Tue, 2015-04-07 at 10:06 -0400, Bill Pringlemeir wrote:

>> In any case the document has,

>> If the NAND flash supports sub-pages, then what can be done is ECC
>> codes can be calculated on per-sub-page basis, instead of per-NAND
>> page basis. In this case it becomes possible to read and write
>> sub-pages independently.

>> Probably if we want sub-pages with this controller and hw-ecc, we
>> need to use the virtual buffer features of the chip.  The controller
>> needs an entire current buffer in the SRAM to calculate the hw-ecc to
>> write.  So even if it writes less than a full page, the entire page
>> must be read to calculate the hw-ecc to be written.

> That limitation sounds similar to the Freescale NAND controllers that
> I'm familiar with (eLBC and IFC).  For eLBC we do subpages by just
> writing 0xff, because on that controller the ECC of all 0xff is all
> 0xff so it doesn't disturb anything.  IFC has different ECC algorithms
> where that isn't the case, so we disabled subpage write on IFC.

> What is the virtual buffer feature?

>> I am pretty sure that all controllers that support hw-ecc will need
>> to do this.

> Not if they can handle doing ECC on a single subpage.

[from another thread, but the same subject].

>> The other way to handle things would to be to investigate the
>> NFC_CFG[PAGE_CNT] and NFC_SECSZ to have the virtual pages support
>> sub-pages.  I think the OOB mapping would be non-standard in such
>> cases.

> Wouldn't that mess up factory bad block markers?

All the stuff above is related (afaik).

  > What is the virtual buffer feature?

It splits programming of a flash page into separate buffers.  The
BCH-HW-ECC is then applied separately to each 'virtual-page' with
reduced strength.  Section "31.4.6 Organization of the Data in the NAND
Flash" of the Vybrid Software RM has details.

 > Wouldn't that mess up factory bad block markers?

I am unsure; certainly they can be read but they might be a data portion
of the fourth sub-page depending on the ECC strength selected.  There is
also a 'NFC_SWAP' register to switch the position of one 16bit index
(move OOB-BBT 16bit from data to OOB).  I think this can be used.  By
non-standard, I meant not fitting the current drivers idea of OOB
layout.

However, it seems like your comment that the ECC must be different
per-subpage (and what Artem said in the sub-pages documentation) makes
'virtual buffers' the most promising path for this driver and sub-page
support with hw-ecc.  As the bit strength is reduced, the amount of
corrections/error detection also seems reduced.  I think the math would
make this the same for everyone?

Your question about factory BBT is a good point.  Using the 'virtual
buffers' feature will complicate the driver code by quite a bit at least
from what I could think of and what I see in the MTD tree where I
believe similar features are used.

Fwiw,
Bill Pringlemeir.

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

[Scott Wood]

On Tue, 2015-04-07 at 13:54 -0400, Bill Pringlemeir wrote:
> On  7 Apr 2015, scottwood at freescale.com wrote:
> > On Tue, 2015-04-07 at 10:06 -0400, Bill Pringlemeir wrote:
> 
> >> In any case the document has,
> 
> >> If the NAND flash supports sub-pages, then what can be done is ECC
> >> codes can be calculated on per-sub-page basis, instead of per-NAND
> >> page basis. In this case it becomes possible to read and write
> >> sub-pages independently.
> 
> >> Probably if we want sub-pages with this controller and hw-ecc, we
> >> need to use the virtual buffer features of the chip.  The controller
> >> needs an entire current buffer in the SRAM to calculate the hw-ecc to
> >> write.  So even if it writes less than a full page, the entire page
> >> must be read to calculate the hw-ecc to be written.
> 
> > That limitation sounds similar to the Freescale NAND controllers that
> > I'm familiar with (eLBC and IFC).  For eLBC we do subpages by just
> > writing 0xff, because on that controller the ECC of all 0xff is all
> > 0xff so it doesn't disturb anything.  IFC has different ECC algorithms
> > where that isn't the case, so we disabled subpage write on IFC.
> 
> > What is the virtual buffer feature?
> 
> >> I am pretty sure that all controllers that support hw-ecc will need
> >> to do this.
> 
> > Not if they can handle doing ECC on a single subpage.
> 
> [from another thread, but the same subject].
> 
> >> The other way to handle things would to be to investigate the
> >> NFC_CFG[PAGE_CNT] and NFC_SECSZ to have the virtual pages support
> >> sub-pages.  I think the OOB mapping would be non-standard in such
> >> cases.
> 
> > Wouldn't that mess up factory bad block markers?
> 
> All the stuff above is related (afaik).
> 
>   > What is the virtual buffer feature?
> 
> It splits programming of a flash page into separate buffers.  The
> BCH-HW-ECC is then applied separately to each 'virtual-page' with
> reduced strength.  Section "31.4.6 Organization of the Data in the NAND
> Flash" of the Vybrid Software RM has details.
> 
>  > Wouldn't that mess up factory bad block markers?
> 
> I am unsure; certainly they can be read but they might be a data portion
> of the fourth sub-page depending on the ECC strength selected.

That would be bad.  Even if you somehow get the MTD code to read markers
in the main area on first use, you wouldn't be able to reconstruct the
BBT after you start writing data.  If you must do this, I suggest
migrating the bad block markers to the new OOB before usage (see
http://patchwork.ozlabs.org/patch/168855/ for an example).

> However, it seems like your comment that the ECC must be different
> per-subpage (and what Artem said in the sub-pages documentation) makes
> 'virtual buffers' the most promising path for this driver and sub-page
> support with hw-ecc.  As the bit strength is reduced, the amount of
> corrections/error detection also seems reduced.  I think the math would
> make this the same for everyone?

Flash chips usually specify the required ECC on a subpage basis.

> Your question about factory BBT is a good point.  Using the 'virtual
> buffers' feature will complicate the driver code by quite a bit at least
> from what I could think of and what I see in the MTD tree where I
> believe similar features are used.

Which gets back to the question of whether subpages are worth it with
this controller.  Or, if subpage writes are infrequent enough, stick
with the read/modify/write approach but delay the read until you know
that it's going to be a subpage write.

-Scott

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

[Bill Pringlemeir]

On 30 Mar 2015, scottwood at freescale.com wrote:

> On Mon, 2015-03-30 at 22:40 +0200, Stefan Agner wrote:

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

While I observed some performance differences, but that is an aside.  I
think the mxc driver is similar in sub-page (or partial page) support.
Before doing a sub-page write, it reads a full page and then copies the
sub-page update to the buffer to be reprogrammed.  This works fine if
each and every sub-page has separate OOB data for software ECC.  The
hardware ECC of this vf610_nfc controller doesn't support this.

At least that is my understanding of this [mxc_nand] code,

        case NAND_CMD_SEQIN:
                if (column >= mtd->writesize) {
                        /*
                         * before sending SEQIN command for partial write,
                         * we need read one page out. FSL NFC does not support
                         * partial write. It always sends out 512+ecc+512+ecc
                         * for large page nand flash. But for small page nand
                         * flash, it does support SPARE ONLY operation.
                         */
                        if (host->pagesize_2k) {
                                /* call ourself to read a page */
                                mxc_nand_command(mtd, NAND_CMD_READ0, 0,
                                                page_addr);
                        }

in 'drivers/mtd/nand/mxc_nand.c'.  So, originally this was mainly for
sub-pages (an optimization but also functionality).

So the 'caching' is just to preserve read data before a partial write as
the vf610 requires a full page (like the mxc).  The same 'caching' was
keep for 'write followed by read' when the ECC is successful.

I realize that the upper layers may not like this so I mentioned it; I
guess that is proof that the patch is getting a good review and I am not
causing problems for Stefan ;)

I also agree with Stefan that setting the SECSZ in the 2nd patch will
result in less data transfer (and maybe less current/power and system
noise).  However, this will be inside the NFC controller.  Talking over
the AHB to the NFC controller is quite slow on the Vybrid.  Certainly it
seems a little more elegant to tell the controller to transfer nothing
(and that we [programmers] expect nothing as a sort of documentation)?

I think it would be worthwhile to benchmark without the cache.  Or maybe
Stefan already has some numbers?  Upper layers doing partial pages will
definitely benefit with the 'cache'; we would also need more DDR memory
as the NFC controller memory is being used as a scratch buffer.

Fwiw,
Bill Pringlemeir.