mtd: vf610_nfc: mark page as dirty on block erase

PATCH New Stefan Agner 3 participants 23 messages Mar 24, 2015 – Apr 7, 2015

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Stefan Agner

author

2015-03-24 16:54 UTC #0 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

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):

Quoted 3 lines

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 <[email protected]>

---
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(-)

drivers/mtd/nand/vf610_nfc.c +1 -2

index 928d58b..9de971c 100644
@@ -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);

Signature · 1 line

2.3.3

Stefan Agner

author

2015-03-24 16:54 UTC #1 lore raw

PATCH 2/2 mtd: vf610_nfc: specify transfer size before each transfer

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 <[email protected]>

---
 drivers/mtd/nand/vf610_nfc.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

drivers/mtd/nand/vf610_nfc.c +17 -8

index 9de971c..d98dd28 100644
@@ -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) {

Signature · 1 line

2.3.3

Bill Pringlemeir

2015-03-30 17:02 UTC #2 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

In reply to Stefan Agner

Quoted 33 lines · stefan at agner.ch

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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;

Quoted 4 lines

	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 <[email protected]>

Stefan Agner

author

2015-03-30 20:14 UTC #3 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 35 lines · Bill Pringlemeir

Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;

Quoted 9 lines

Quoted 4 lines
	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?

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

At least in the small, this is a minimal change that is correct.
Ack-by Bill Pringlemeir <[email protected]>

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

--
Stefan

Scott Wood

2015-03-30 20:46 UTC #4 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 46 lines · Stefan Agner

Quoted 35 lines · Bill Pringlemeir
Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;
Quoted 9 lines
Quoted 4 lines
	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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines
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?

Quoted 6 lines

Quoted 3 lines
At least in the small, this is a minimal change that is correct.
Ack-by Bill Pringlemeir <[email protected]>
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

Scott Wood

2015-03-30 20:34 UTC #5 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

In reply to Bill Pringlemeir

Quoted 35 lines · Bill Pringlemeir

Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;

Quoted 9 lines

Quoted 4 lines
	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.

Quoted 4 lines

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.

Quoted 2 lines

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

So fix Linux. :-)

-Scott

Stefan Agner

author

2015-03-30 20:40 UTC #6 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 49 lines · Scott Wood

Quoted 35 lines · Bill Pringlemeir
Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;
Quoted 9 lines
Quoted 4 lines
	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.

Quoted 7 lines

Quoted 4 lines
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...

Quoted 4 lines

Quoted 2 lines
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

Scott Wood

2015-03-30 20:48 UTC #7 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 55 lines · Stefan Agner

Quoted 54 lines · Scott Wood
Quoted 35 lines · Bill Pringlemeir
Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;
Quoted 9 lines
Quoted 4 lines
	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.
Quoted 4 lines
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.

Quoted 4 lines

Quoted 2 lines
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.

Quoted 3 lines

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.

Quoted 2 lines

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

Stefan Agner

author

2015-03-30 21:26 UTC #8 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 70 lines · Scott Wood

Quoted 55 lines · Stefan Agner
Quoted 54 lines · Scott Wood
Quoted 35 lines · Bill Pringlemeir
Quoted 33 lines · stefan at agner.ch
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):
Quoted 3 lines
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 <[email protected]>
---
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(-)
(quoted diff) +0 -0
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;
Quoted 9 lines
Quoted 4 lines
	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.
Quoted 4 lines
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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 3 lines
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.

Quoted 7 lines

Quoted 2 lines
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:

Quoted 10 lines

Quoted 9 lines
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?

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines
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...

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
At least in the small, this is a minimal change that is correct.
Ack-by Bill Pringlemeir <[email protected]>
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

Scott Wood

2015-03-30 22:15 UTC #9 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 6 lines · Stefan Agner

Quoted 4 lines · Scott Wood
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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
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?

Quoted 8 lines

Quoted 6 lines
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.

Quoted 4 lines

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.

Quoted 4 lines

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?

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
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?

Quoted 6 lines

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

Stefan Agner

author

2015-03-30 22:24 UTC #10 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 47 lines · Scott Wood

Quoted 6 lines · Stefan Agner
Quoted 4 lines · Scott Wood
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.
Quoted 6 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
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?
Quoted 8 lines
Quoted 6 lines
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.
Quoted 4 lines
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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 14 lines

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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?
Quoted 6 lines
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...

Quoted 4 lines

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

Scott Wood

2015-03-31 04:34 UTC #11 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 11 lines · Stefan Agner

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;

Quoted 5 lines

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

Bill Pringlemeir

2015-03-31 15:02 UTC #12 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 12 lines · scottwood at freescale.com

Quoted 11 lines · Stefan Agner
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;

Quoted 2 lines

Quoted 2 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 3 lines · Scott Wood

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.

Scott Wood

2015-04-02 23:48 UTC #13 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 16 lines · Bill Pringlemeir

Quoted 3 lines · Scott Wood
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

Quoted 8 lines

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

Stefan Agner

author

2015-04-03 18:09 UTC #14 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 22 lines · Scott Wood

Quoted 21 lines · Bill Pringlemeir
Quoted 3 lines · Scott Wood
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.

Quoted 5 lines

Quoted 3 lines
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

Scott Wood

2015-04-03 20:15 UTC #15 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 27 lines · Stefan Agner

Quoted 26 lines · Scott Wood
Quoted 24 lines · Bill Pringlemeir
Quoted 3 lines · Scott Wood
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.

Quoted 6 lines

Quoted 1 line
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.

Quoted 9 lines

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

Stefan Agner

author

2015-04-03 20:28 UTC #16 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 30 lines · Scott Wood

Quoted 29 lines · Stefan Agner
Quoted 27 lines · Scott Wood
Quoted 16 lines · Bill Pringlemeir
Quoted 3 lines · Scott Wood
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
Quoted 8 lines
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?

Quoted 7 lines

Quoted 4 lines
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").

Quoted 5 lines

Quoted 5 lines
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

Quoted 7 lines

Quoted 4 lines
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

Scott Wood

2015-04-03 21:03 UTC #17 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 36 lines · Stefan Agner

Quoted 35 lines · Scott Wood
Quoted 33 lines · Stefan Agner
Quoted 22 lines · Scott Wood
Quoted 21 lines · Bill Pringlemeir
Quoted 3 lines · Scott Wood
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.
Quoted 5 lines
Quoted 3 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 3 lines

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

Bill Pringlemeir

2015-04-07 14:06 UTC #18 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 5 lines · scottwood at freescale.com

Quoted 2 lines · Stefan Agner
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)...

Quoted 7 lines

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.

Quoted 3 lines

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.

Scott Wood

2015-04-07 16:02 UTC #19 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 24 lines · Bill Pringlemeir

Quoted 5 lines · scottwood at freescale.com
Quoted 2 lines · Stefan Agner
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)...
Quoted 7 lines
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?

Quoted 3 lines

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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 1 line

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

Bill Pringlemeir

2015-04-07 17:54 UTC #20 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 21 lines · scottwood at freescale.com

Quoted 7 lines · Bill Pringlemeir
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.
Quoted 5 lines
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.

Quoted 6 lines

What is the virtual buffer feature?
Quoted 2 lines
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].

Quoted 6 lines

Quoted 4 lines
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).

Quoted 1 line

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.

Quoted 1 line

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.

Scott Wood

2015-04-07 21:09 UTC #21 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

Quoted 47 lines · Bill Pringlemeir

Quoted 21 lines · scottwood at freescale.com
Quoted 7 lines · Bill Pringlemeir
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.
Quoted 5 lines
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.
Quoted 6 lines
What is the virtual buffer feature?
Quoted 2 lines
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].
Quoted 6 lines
Quoted 4 lines
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).
Quoted 1 line
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.

Quoted 4 lines

Quoted 1 line
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).

Quoted 6 lines

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.

Quoted 4 lines

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

Bill Pringlemeir

2015-03-30 21:35 UTC #22 lore raw

PATCH 1/2 mtd: vf610_nfc: mark page as dirty on block erase

In reply to Scott Wood

Quoted 8 lines · scottwood at freescale.com

Quoted 2 lines · Stefan Agner
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.