[U-Boot] [PATCH 1/4] mtd: vf610_nfc: use in-band bad block table

[U-Boot] [PATCH 1/4] mtd: vf610_nfc: use in-band bad block table

[Stefan Agner]

Use in-band bad block table (NAND_BBT_NO_OOB) which allows to
use the full OOB for hardare ECC purposes. Since there is no
ECC correction on the OOB it is also safer to use in-band area
to store the bad block table marker.

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

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 75c2493..8608ac3 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -155,29 +155,6 @@ struct vf610_nfc {
 #define mtd_to_nfc(_mtd) \
 	(struct vf610_nfc *)((struct nand_chip *)_mtd->priv)->priv
 
-static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
-static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
-
-static struct nand_bbt_descr bbt_main_descr = {
-	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
-		   NAND_BBT_2BIT | NAND_BBT_VERSION,
-	.offs =	11,
-	.len = 4,
-	.veroffs = 15,
-	.maxblocks = 4,
-	.pattern = bbt_pattern,
-};
-
-static struct nand_bbt_descr bbt_mirror_descr = {
-	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
-		   NAND_BBT_2BIT | NAND_BBT_VERSION,
-	.offs =	11,
-	.len = 4,
-	.veroffs = 15,
-	.maxblocks = 4,
-	.pattern = mirror_pattern,
-};
-
 static struct nand_ecclayout vf610_nfc_ecc45 = {
 	.eccbytes = 45,
 	.eccpos = {19, 20, 21, 22, 23,
@@ -622,10 +599,8 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 
 	/* Bad block options. */
 	if (cfg.flash_bbt)
-		chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_CREATE;
-
-	chip->bbt_td = &bbt_main_descr;
-	chip->bbt_md = &bbt_mirror_descr;
+		chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB |
+				    NAND_BBT_CREATE;
 
 	/* Set configuration register. */
 	vf610_nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
-- 
2.3.5

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Stefan Agner]

This commit allows users to enable/disable the Freescale NFC
controller found in systems like Vybrid (VF610), MPC5125, MCF54418
or Kinetis K70 via Kconfig with more detailed help docs.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 configs/vf610twr_defconfig |  2 ++
 drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
 include/configs/vf610twr.h |  3 ---
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
index 7de374a..5e0ac9f 100644
--- a/configs/vf610twr_defconfig
+++ b/configs/vf610twr_defconfig
@@ -1,3 +1,5 @@
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
 CONFIG_ARM=y
 CONFIG_TARGET_VF610TWR=y
+CONFIG_NAND_VF610_NFC=y
+CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 72825c3..8056c06 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
 	  of OOB area before last ECC sector data starts.  This is potentially
 	  used to preserve the bad block marker in the OOB area.
 
+config NAND_VF610_NFC
+	bool "Support for Freescale NFC for VF610/MPC5125"
+	select SYS_NAND_SELF_INIT
+	help
+	  Enables support for NAND Flash Controller on some Freescale
+	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
+	  The driver supports a maximum 2k page size. The driver
+	  currently does not support hardware ECC.
+
+config SYS_NAND_BUSWIDTH_16BIT
+	bool "Use 16-bit NAND interface"
+	depends on NAND_VF610_NFC
+	help
+	  Use 16-bit wide NAND flash interface.
+
 if SPL
 
 config SPL_NAND_DENALI
diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h
index 05bc7d0..621aa13 100644
--- a/include/configs/vf610twr.h
+++ b/include/configs/vf610twr.h
@@ -50,10 +50,7 @@
 #define CONFIG_CMD_NAND_TRIMFFS
 
 #ifdef CONFIG_CMD_NAND
-#define CONFIG_NAND_VF610_NFC
-#define CONFIG_SYS_NAND_SELF_INIT
 #define CONFIG_USE_ARCH_MEMCPY
-#define CONFIG_SYS_NAND_BUSWIDTH_16BIT
 #define CONFIG_SYS_MAX_NAND_DEVICE	1
 #define CONFIG_SYS_NAND_BASE		NFC_BASE_ADDR
 
-- 
2.3.5

[U-Boot] [PATCH 3/4] mtd: vf610_nfc: add 32-error correction option for HW ECC

[Stefan Agner]

Add option to choose between current 24-error correction and 32-error
correction through Kconfig. 32-error correction allow to use NAND
chips which require up to 8-bit error correction per 512 byte (when
using 2K pages).

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/Kconfig     | 15 +++++++++++++++
 drivers/mtd/nand/vf610_nfc.c | 38 ++++++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 8056c06..ca14cc2 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -47,6 +47,21 @@ config SYS_NAND_BUSWIDTH_16BIT
 	help
 	  Use 16-bit wide NAND flash interface.
 
+choice
+	prompt "Hardware ECC strength"
+	depends on NAND_VF610_NFC
+	default SYS_NAND_VF610_NFC_45_ECC_BYTES
+	help
+	  Select the ECC strength used in the hardware BCH ECC block.
+
+config SYS_NAND_VF610_NFC_45_ECC_BYTES
+	bool "24-error correction (45 ECC bytes)"
+
+config SYS_NAND_VF610_NFC_60_ECC_BYTES
+	bool "32-error correction (60 ECC bytes)"
+
+endchoice
+
 if SPL
 
 config SPL_NAND_DENALI
diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 8608ac3..a8af00e 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -71,6 +71,7 @@
 /* NFC ECC mode define */
 #define ECC_BYPASS			0
 #define ECC_45_BYTE			6
+#define ECC_60_BYTE			7
 
 /*** Register Mask and bit definitions */
 
@@ -155,7 +156,10 @@ struct vf610_nfc {
 #define mtd_to_nfc(_mtd) \
 	(struct vf610_nfc *)((struct nand_chip *)_mtd->priv)->priv
 
-static struct nand_ecclayout vf610_nfc_ecc45 = {
+#if defined (CONFIG_SYS_NAND_VF610_NFC_45_ECC_BYTES)
+#define ECC_HW_MODE ECC_45_BYTE
+
+static struct nand_ecclayout vf610_nfc_ecc = {
 	.eccbytes = 45,
 	.eccpos = {19, 20, 21, 22, 23,
 		   24, 25, 26, 27, 28, 29, 30, 31,
@@ -167,6 +171,24 @@ static struct nand_ecclayout vf610_nfc_ecc45 = {
 		{.offset = 8,
 		 .length = 11} }
 };
+#elif defined (CONFIG_SYS_NAND_VF610_NFC_60_ECC_BYTES)
+#define ECC_HW_MODE ECC_60_BYTE
+
+static struct nand_ecclayout vf610_nfc_ecc = {
+	.eccbytes = 60,
+	.eccpos = { 4,  5,  6,  7,  8,  9, 10, 11,
+		   12, 13, 14, 15, 16, 17, 18, 19,
+		   20, 21, 22, 23, 24, 25, 26, 27,
+		   28, 29, 30, 31, 32, 33, 34, 35,
+		   36, 37, 38, 39, 40, 41, 42, 43,
+		   44, 45, 46, 47, 48, 49, 50, 51,
+		   52, 53, 54, 55, 56, 57, 58, 59,
+		   60, 61, 62, 63 },
+	.oobfree = {
+		{.offset = 2,
+		 .length = 2} }
+};
+#endif
 
 static inline u32 vf610_nfc_read(struct mtd_info *mtd, uint reg)
 {
@@ -333,7 +355,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 		vf610_nfc_send_commands(nfc->regs, NAND_CMD_SEQIN,
 					command, PROGRAM_PAGE_CMD_CODE);
 		vf610_nfc_addr_cycle(mtd, column, page);
-		vf610_nfc_ecc_mode(mtd, ECC_45_BYTE);
+		vf610_nfc_ecc_mode(mtd, ECC_HW_MODE);
 		break;
 
 	case NAND_CMD_RESET:
@@ -362,7 +384,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
 		vf610_nfc_send_commands(nfc->regs, NAND_CMD_READ0,
 					NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
 		vf610_nfc_addr_cycle(mtd, column, page);
-		vf610_nfc_ecc_mode(mtd, ECC_45_BYTE);
+		vf610_nfc_ecc_mode(mtd, ECC_HW_MODE);
 		break;
 
 	case NAND_CMD_ERASE1:
@@ -643,17 +665,21 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 			goto error;
 		}
 
-		chip->ecc.layout = &vf610_nfc_ecc45;
-
 		/* propagate ecc.layout to mtd_info */
 		mtd->ecclayout = chip->ecc.layout;
 		chip->ecc.read_page = vf610_nfc_read_page;
 		chip->ecc.write_page = vf610_nfc_write_page;
 		chip->ecc.mode = NAND_ECC_HW;
 
-		chip->ecc.bytes = 45;
 		chip->ecc.size = PAGE_2K;
+		chip->ecc.layout = &vf610_nfc_ecc;
+#if defined(CONFIG_SYS_NAND_VF610_NFC_45_ECC_BYTES)
 		chip->ecc.strength = 24;
+		chip->ecc.bytes = 45;
+#elif defined(CONFIG_SYS_NAND_VF610_NFC_60_ECC_BYTES)
+		chip->ecc.strength = 32;
+		chip->ecc.bytes = 60;
+#endif
 
 		/* Enable ECC_STATUS */
 		vf610_nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
-- 
2.3.5

[U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write

[Stefan Agner]

Support subpage writes using a custom implementation of write_subpage.
The driver loads the page into SRAM buffer using NAND_CMD_READ0, when
the framework requests the NAND_CMD_SEQIN command. Then, the buffer is
updated by the custom write_subpage implementation. Upon write, the
controller calculates the hardware ECC across the whole page before
programming the page.

This method saves transferring the whole page over the bus to the NFC
IP, which would happen when using NAND_NO_SUBPAGE_WRITE.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This implements the procedure as discussed on the ML:
http://lists.denx.de/pipermail/u-boot/2015-March/209567.html
http://lists.denx.de/pipermail/u-boot/2015-March/209671.html

The drivers mxc_nand and mpc5121_nfc implement a similar subpage write.

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

diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index a8af00e..d552bed 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -566,6 +566,19 @@ static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 	return 0;
 }
 
+static int vf610_nfc_write_subpage(struct mtd_info *mtd, struct nand_chip *chip,
+				uint32_t offset, uint32_t data_len,
+				const uint8_t *buf, int oob_required)
+{
+	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+	nfc->column = offset;
+	vf610_nfc_write_buf(mtd, buf, data_len);
+	if (oob_required)
+		vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return 0;
+}
+
 struct vf610_nfc_config {
 	int hardware_ecc;
 	int width;
@@ -608,9 +621,6 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 		vf610_nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_16BIT);
 	}
 
-	/* Disable subpage writes as we do not provide ecc->hwctl */
-	chip->options |= NAND_NO_SUBPAGE_WRITE;
-
 	chip->dev_ready = vf610_nfc_dev_ready;
 	chip->cmdfunc = vf610_nfc_command;
 	chip->read_byte = vf610_nfc_read_byte;
@@ -669,6 +679,7 @@ static int vf610_nfc_nand_init(int devnum, void __iomem *addr)
 		mtd->ecclayout = chip->ecc.layout;
 		chip->ecc.read_page = vf610_nfc_read_page;
 		chip->ecc.write_page = vf610_nfc_write_page;
+		chip->ecc.write_subpage = vf610_nfc_write_subpage;
 		chip->ecc.mode = NAND_ECC_HW;
 
 		chip->ecc.size = PAGE_2K;
-- 
2.3.5

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Scott Wood]

On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
> This commit allows users to enable/disable the Freescale NFC
> controller found in systems like Vybrid (VF610), MPC5125, MCF54418
> or Kinetis K70 via Kconfig with more detailed help docs.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  configs/vf610twr_defconfig |  2 ++
>  drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
>  include/configs/vf610twr.h |  3 ---
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
> index 7de374a..5e0ac9f 100644
> --- a/configs/vf610twr_defconfig
> +++ b/configs/vf610twr_defconfig
> @@ -1,3 +1,5 @@
>  CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
>  CONFIG_ARM=y
>  CONFIG_TARGET_VF610TWR=y
> +CONFIG_NAND_VF610_NFC=y
> +CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 72825c3..8056c06 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
>  	  of OOB area before last ECC sector data starts.  This is potentially
>  	  used to preserve the bad block marker in the OOB area.
>  
> +config NAND_VF610_NFC
> +	bool "Support for Freescale NFC for VF610/MPC5125"
> +	select SYS_NAND_SELF_INIT
> +	help
> +	  Enables support for NAND Flash Controller on some Freescale
> +	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
> +	  The driver supports a maximum 2k page size. The driver
> +	  currently does not support hardware ECC.
> +
> +config SYS_NAND_BUSWIDTH_16BIT
> +	bool "Use 16-bit NAND interface"
> +	depends on NAND_VF610_NFC
> +	help
> +	  Use 16-bit wide NAND flash interface.

Why does a generic-sounding config name depend on VF610?  Especially
when README already lists three other drivers as using this option...

Also, the help text makes it sound like it's at the user's discretion,
rather than a description of hardware.  I'd phrase it as something like
"NAND has 16-bit interface"

-Scott

[U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write

[Scott Wood]

On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
> Support subpage writes using a custom implementation of write_subpage.
> The driver loads the page into SRAM buffer using NAND_CMD_READ0, when
> the framework requests the NAND_CMD_SEQIN command. Then, the buffer is
> updated by the custom write_subpage implementation. Upon write, the
> controller calculates the hardware ECC across the whole page before
> programming the page.
> 
> This method saves transferring the whole page over the bus to the NFC
> IP, which would happen when using NAND_NO_SUBPAGE_WRITE.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

As previously discussed, please explain why subpage writes make sense
with this controller.

-Scott

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Stefan Agner]

On 2015-04-03 22:30, Scott Wood wrote:
> On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
>> This commit allows users to enable/disable the Freescale NFC
>> controller found in systems like Vybrid (VF610), MPC5125, MCF54418
>> or Kinetis K70 via Kconfig with more detailed help docs.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  configs/vf610twr_defconfig |  2 ++
>>  drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
>>  include/configs/vf610twr.h |  3 ---
>>  3 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
>> index 7de374a..5e0ac9f 100644
>> --- a/configs/vf610twr_defconfig
>> +++ b/configs/vf610twr_defconfig
>> @@ -1,3 +1,5 @@
>>  CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
>>  CONFIG_ARM=y
>>  CONFIG_TARGET_VF610TWR=y
>> +CONFIG_NAND_VF610_NFC=y
>> +CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 72825c3..8056c06 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
>>  	  of OOB area before last ECC sector data starts.  This is potentially
>>  	  used to preserve the bad block marker in the OOB area.
>>
>> +config NAND_VF610_NFC
>> +	bool "Support for Freescale NFC for VF610/MPC5125"
>> +	select SYS_NAND_SELF_INIT
>> +	help
>> +	  Enables support for NAND Flash Controller on some Freescale
>> +	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
>> +	  The driver supports a maximum 2k page size. The driver
>> +	  currently does not support hardware ECC.
>> +
>> +config SYS_NAND_BUSWIDTH_16BIT
>> +	bool "Use 16-bit NAND interface"
>> +	depends on NAND_VF610_NFC
>> +	help
>> +	  Use 16-bit wide NAND flash interface.
> 
> Why does a generic-sounding config name depend on VF610?  Especially
> when README already lists three other drivers as using this option...

That option is _not_ meant as being VF610 specific.

Since we have the ability to specify dependencies with Kconfig, I think
it is nice to have options only available if a driver supports it, hence
the depends. So far the VF610 NAND driver is the only one which is in
Kconfig and supports it... I would expect that when another driver which
supports that option gets migrated, depends will be extended
accordingly.

However, I just realized that the option end up between Vybrid specific
configs because of Patch 3. I will move the option at the very bottom in
next revision.
 
> Also, the help text makes it sound like it's at the user's discretion,
> rather than a description of hardware.  I'd phrase it as something like
> "NAND has 16-bit interface"

Agreed, will change that.

--
Stefan

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Scott Wood]

On Fri, 2015-04-03 at 22:42 +0200, Stefan Agner wrote:
> On 2015-04-03 22:30, Scott Wood wrote:
> > On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
> >> This commit allows users to enable/disable the Freescale NFC
> >> controller found in systems like Vybrid (VF610), MPC5125, MCF54418
> >> or Kinetis K70 via Kconfig with more detailed help docs.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  configs/vf610twr_defconfig |  2 ++
> >>  drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
> >>  include/configs/vf610twr.h |  3 ---
> >>  3 files changed, 17 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
> >> index 7de374a..5e0ac9f 100644
> >> --- a/configs/vf610twr_defconfig
> >> +++ b/configs/vf610twr_defconfig
> >> @@ -1,3 +1,5 @@
> >>  CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
> >>  CONFIG_ARM=y
> >>  CONFIG_TARGET_VF610TWR=y
> >> +CONFIG_NAND_VF610_NFC=y
> >> +CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
> >> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> >> index 72825c3..8056c06 100644
> >> --- a/drivers/mtd/nand/Kconfig
> >> +++ b/drivers/mtd/nand/Kconfig
> >> @@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
> >>  	  of OOB area before last ECC sector data starts.  This is potentially
> >>  	  used to preserve the bad block marker in the OOB area.
> >>
> >> +config NAND_VF610_NFC
> >> +	bool "Support for Freescale NFC for VF610/MPC5125"
> >> +	select SYS_NAND_SELF_INIT
> >> +	help
> >> +	  Enables support for NAND Flash Controller on some Freescale
> >> +	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
> >> +	  The driver supports a maximum 2k page size. The driver
> >> +	  currently does not support hardware ECC.
> >> +
> >> +config SYS_NAND_BUSWIDTH_16BIT
> >> +	bool "Use 16-bit NAND interface"
> >> +	depends on NAND_VF610_NFC
> >> +	help
> >> +	  Use 16-bit wide NAND flash interface.
> > 
> > Why does a generic-sounding config name depend on VF610?  Especially
> > when README already lists three other drivers as using this option...
> 
> That option is _not_ meant as being VF610 specific.
> 
> Since we have the ability to specify dependencies with Kconfig, I think
> it is nice to have options only available if a driver supports it, hence
> the depends. So far the VF610 NAND driver is the only one which is in
> Kconfig and supports it... I would expect that when another driver which
> supports that option gets migrated, depends will be extended
> accordingly.
> 
> However, I just realized that the option end up between Vybrid specific
> configs because of Patch 3. I will move the option at the very bottom in
> next revision.

Could you also add a comment mentioning the other drivers that use it,
which aren't yet kconfiged?  And then remove the old text from the
README.

-Scott

[U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write

[Stefan Agner]

On 2015-04-03 22:36, Scott Wood wrote:
> On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
>> Support subpage writes using a custom implementation of write_subpage.
>> The driver loads the page into SRAM buffer using NAND_CMD_READ0, when
>> the framework requests the NAND_CMD_SEQIN command. Then, the buffer is
>> updated by the custom write_subpage implementation. Upon write, the
>> controller calculates the hardware ECC across the whole page before
>> programming the page.
>>
>> This method saves transferring the whole page over the bus to the NFC
>> IP, which would happen when using NAND_NO_SUBPAGE_WRITE.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> 
> As previously discussed, please explain why subpage writes make sense
> with this controller.

I thought the subpage callback is just about writing an arbitrary amount
of data (e.g. 32 bytes) into a page.

I quickly checked, when doing
# nand write ${loadaddr} 0x10000 0x10

vf610_nfc_write_subpage gets actually called with a data_len of 16.

This is how I understand it:
Currently, subpage writes are supported by the MTD subsystem due to the
option NAND_NO_SUBPAGE_WRITE. Due to that option, nand_write_page in
nand_base.c calls write_page which copies always the whole page into the
SRAM buffer over the AHB bus to the NFC IP (vf610_nfc_write_page uses
memcpy uses mtd->writesize).

This patch supports it naively, which means that only the updated data
(according to offset and data_len parameter of write_subpage) get copied
over the AHB bus to the NFC IP (vf610_nfc_write_subpage uses memcpy to
only copy data_len). To have reasonable data for the rest of the page,
the page gets read back when calling NAND_CMD_SEQIN.

Since the whole page get programmed, this assumes that none of the page
has been programmed before (erased page). Hence, we essentially just
read 0xff. When I think about it now, reading the page back in SEQIN is
then probably just an expensive memset 0xff replacement.

I guess when having real subpages (e.g. 4x512bytes, each subpage with
its own ECC), I would have to make sure only the affected subpage gets
ECC'ed and written.

Even without having real subpages, if somebody only writes part of a
page, memset directly into SRAM & memcpy the subpage data is
theoretically faster then memset the whole page in DDR RAM and memcpy
the whole page....

Just checked what higher up the stack is actually happening:
nand_write_page anyway receives a whole page buffer as argument, which
is memset'ed with 0xff and the data to be written memcpy'ed.

I see, that this patch tries to improve something which anyway is taken
care of by the stack.

I will remove the page read on NAND_CMD_SEQIN, since we memcpy the full
page anyway. I also just realized that the page read actually happens
always and hence slows down even full page writes...

--
Stefan

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Stefan Agner]

On 2015-04-03 22:46, Scott Wood wrote:
> On Fri, 2015-04-03 at 22:42 +0200, Stefan Agner wrote:
>> On 2015-04-03 22:30, Scott Wood wrote:
>> > On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
>> >> This commit allows users to enable/disable the Freescale NFC
>> >> controller found in systems like Vybrid (VF610), MPC5125, MCF54418
>> >> or Kinetis K70 via Kconfig with more detailed help docs.
>> >>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >>  configs/vf610twr_defconfig |  2 ++
>> >>  drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
>> >>  include/configs/vf610twr.h |  3 ---
>> >>  3 files changed, 17 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
>> >> index 7de374a..5e0ac9f 100644
>> >> --- a/configs/vf610twr_defconfig
>> >> +++ b/configs/vf610twr_defconfig
>> >> @@ -1,3 +1,5 @@
>> >>  CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
>> >>  CONFIG_ARM=y
>> >>  CONFIG_TARGET_VF610TWR=y
>> >> +CONFIG_NAND_VF610_NFC=y
>> >> +CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
>> >> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> >> index 72825c3..8056c06 100644
>> >> --- a/drivers/mtd/nand/Kconfig
>> >> +++ b/drivers/mtd/nand/Kconfig
>> >> @@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
>> >>  	  of OOB area before last ECC sector data starts.  This is potentially
>> >>  	  used to preserve the bad block marker in the OOB area.
>> >>
>> >> +config NAND_VF610_NFC
>> >> +	bool "Support for Freescale NFC for VF610/MPC5125"
>> >> +	select SYS_NAND_SELF_INIT
>> >> +	help
>> >> +	  Enables support for NAND Flash Controller on some Freescale
>> >> +	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
>> >> +	  The driver supports a maximum 2k page size. The driver
>> >> +	  currently does not support hardware ECC.
>> >> +
>> >> +config SYS_NAND_BUSWIDTH_16BIT
>> >> +	bool "Use 16-bit NAND interface"
>> >> +	depends on NAND_VF610_NFC
>> >> +	help
>> >> +	  Use 16-bit wide NAND flash interface.
>> >
>> > Why does a generic-sounding config name depend on VF610?  Especially
>> > when README already lists three other drivers as using this option...
>>
>> That option is _not_ meant as being VF610 specific.
>>
>> Since we have the ability to specify dependencies with Kconfig, I think
>> it is nice to have options only available if a driver supports it, hence
>> the depends. So far the VF610 NAND driver is the only one which is in
>> Kconfig and supports it... I would expect that when another driver which
>> supports that option gets migrated, depends will be extended
>> accordingly.
>>
>> However, I just realized that the option end up between Vybrid specific
>> configs because of Patch 3. I will move the option at the very bottom in
>> next revision.
> 
> Could you also add a comment mentioning the other drivers that use it,
> which aren't yet kconfiged?  And then remove the old text from the
> README.

By comment, you mean a Kconfig comment at that option, so the next
stumbles upon it? So I can keep that single depends NAND_VF610_NFC for
now?

Removing CONFIG_SYS_NAND_BUSWIDTH_16BIT from doc/README.nand right? But
with that, the options for the other drivers would be "undocumented" for
the time being... 

--
Stefan

[U-Boot] [PATCH 2/4] mtd: vf610_nfc: add Freescale NFC controller configs to Kconfig

[Scott Wood]

On Sat, 2015-04-04 at 00:30 +0200, Stefan Agner wrote:
> On 2015-04-03 22:46, Scott Wood wrote:
> > On Fri, 2015-04-03 at 22:42 +0200, Stefan Agner wrote:
> >> On 2015-04-03 22:30, Scott Wood wrote:
> >> > On Fri, 2015-04-03 at 20:40 +0200, Stefan Agner wrote:
> >> >> This commit allows users to enable/disable the Freescale NFC
> >> >> controller found in systems like Vybrid (VF610), MPC5125, MCF54418
> >> >> or Kinetis K70 via Kconfig with more detailed help docs.
> >> >>
> >> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> >> ---
> >> >>  configs/vf610twr_defconfig |  2 ++
> >> >>  drivers/mtd/nand/Kconfig   | 15 +++++++++++++++
> >> >>  include/configs/vf610twr.h |  3 ---
> >> >>  3 files changed, 17 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
> >> >> index 7de374a..5e0ac9f 100644
> >> >> --- a/configs/vf610twr_defconfig
> >> >> +++ b/configs/vf610twr_defconfig
> >> >> @@ -1,3 +1,5 @@
> >> >>  CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
> >> >>  CONFIG_ARM=y
> >> >>  CONFIG_TARGET_VF610TWR=y
> >> >> +CONFIG_NAND_VF610_NFC=y
> >> >> +CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
> >> >> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> >> >> index 72825c3..8056c06 100644
> >> >> --- a/drivers/mtd/nand/Kconfig
> >> >> +++ b/drivers/mtd/nand/Kconfig
> >> >> @@ -32,6 +32,21 @@ config NAND_DENALI_SPARE_AREA_SKIP_BYTES
> >> >>  	  of OOB area before last ECC sector data starts.  This is potentially
> >> >>  	  used to preserve the bad block marker in the OOB area.
> >> >>
> >> >> +config NAND_VF610_NFC
> >> >> +	bool "Support for Freescale NFC for VF610/MPC5125"
> >> >> +	select SYS_NAND_SELF_INIT
> >> >> +	help
> >> >> +	  Enables support for NAND Flash Controller on some Freescale
> >> >> +	  processors like the VF610, MPC5125, MCF54418 or Kinetis K70.
> >> >> +	  The driver supports a maximum 2k page size. The driver
> >> >> +	  currently does not support hardware ECC.
> >> >> +
> >> >> +config SYS_NAND_BUSWIDTH_16BIT
> >> >> +	bool "Use 16-bit NAND interface"
> >> >> +	depends on NAND_VF610_NFC
> >> >> +	help
> >> >> +	  Use 16-bit wide NAND flash interface.
> >> >
> >> > Why does a generic-sounding config name depend on VF610?  Especially
> >> > when README already lists three other drivers as using this option...
> >>
> >> That option is _not_ meant as being VF610 specific.
> >>
> >> Since we have the ability to specify dependencies with Kconfig, I think
> >> it is nice to have options only available if a driver supports it, hence
> >> the depends. So far the VF610 NAND driver is the only one which is in
> >> Kconfig and supports it... I would expect that when another driver which
> >> supports that option gets migrated, depends will be extended
> >> accordingly.
> >>
> >> However, I just realized that the option end up between Vybrid specific
> >> configs because of Patch 3. I will move the option at the very bottom in
> >> next revision.
> > 
> > Could you also add a comment mentioning the other drivers that use it,
> > which aren't yet kconfiged?  And then remove the old text from the
> > README.
> 
> By comment, you mean a Kconfig comment at that option, so the next
> stumbles upon it? So I can keep that single depends NAND_VF610_NFC for
> now?

Yes.

> Removing CONFIG_SYS_NAND_BUSWIDTH_16BIT from doc/README.nand right? But
> with that, the options for the other drivers would be "undocumented" for
> the time being... 

No, it'd be documented in the kconfig file, even if it's not presented
via the kconfig tool.  It seems bad to have the option description
duplicated -- and then potentially getting changed one place and not the
other.

-Scott

[U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write

[Bill Pringlemeir]

On  3 Apr 2015, stefan at agner.ch wrote:

> I will remove the page read on NAND_CMD_SEQIN, since we memcpy the
> full page anyway. I also just realized that the page read actually
> happens always and hence slows down even full page writes...

Yes, I remove this in Linux (4.0) and it corrupted things when writing.
I think your previous conclusion about we never use 'write caching' was
wrong.

This one is for writes,

	case NAND_CMD_SEQIN: /* Pre-read for partial writes. */

This one is for reads,

	case NAND_CMD_READ0:

The interface between 'nand_base' and the MTD driver is hard to
decipher.  Does Scott (or anyone) know if there is any documentation on
this?

Stefan is completely correct that if a full page is being written, then
the 'SEQIN' should not read a page.  However, I only see 'column' being
passed.  How is 'SEQIN' and 'PAGEPROG' to detect if a full page is being
written or not?

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.
The buffer management in the driver is most simple in it's current form.
The other versions that I found seemed to be buggy to me.  However, the
current driver doesn't use all of the NFC SRAM buffer space.

Btw, the READ_OOB is very nice for Linux as well.  It is a much faster
mount of UBI/UbiFs as well.

Fwiw,
Bill Pringlemeir.

[U-Boot] [PATCH 4/4] mtd: vf610_nfc: support subpage write

[Scott Wood]

On Tue, 2015-04-07 at 09:48 -0400, Bill Pringlemeir wrote:
> On  3 Apr 2015, stefan at agner.ch wrote:
> 
> > I will remove the page read on NAND_CMD_SEQIN, since we memcpy the
> > full page anyway. I also just realized that the page read actually
> > happens always and hence slows down even full page writes...
> 
> Yes, I remove this in Linux (4.0) and it corrupted things when writing.
> I think your previous conclusion about we never use 'write caching' was
> wrong.
> 
> This one is for writes,
> 
> 	case NAND_CMD_SEQIN: /* Pre-read for partial writes. */
> 
> This one is for reads,
> 
> 	case NAND_CMD_READ0:
> 
> The interface between 'nand_base' and the MTD driver is hard to
> decipher.  Does Scott (or anyone) know if there is any documentation on
> this?

It's an awkward interface for drivers that expose a higher-level
programming model.  Basically you have to behave as if nand_base could
send commands directly to the chip.

> Stefan is completely correct that if a full page is being written, then
> the 'SEQIN' should not read a page.  However, I only see 'column' being
> passed.  How is 'SEQIN' and 'PAGEPROG' to detect if a full page is being
> written or not?

At SEQIN time, you can't.  You'll know based on how much data is written
into the buffer.  Or, you can skip this low-level interface and replace
nand_write_page (which is what I wish we'd done in the eLBC/IFC
drivers).

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

-Scott