[PATCH v2 0/5] Fixes for Rockchip NAND controller driver

[PATCH v2 0/5] Fixes for Rockchip NAND controller driver

[Johan Jonker]

This serie contains various fixes for the Rockchip NAND controller
driver that showed up while testing boot block writing.

Fixed are:
  Always copy hwecc PA data to/from oob_poi buffer in order to be able
  to read/write the various boot block layouts.
  Add option to safely probe the driver on a NAND with unknown data layout.
  Fix default timing.
  Fix oobfree layout.
  Add missing chip ID.

Changed V2:
  Add tag
  Add manufacturer ops
  Reword

Johan Jonker (4):
  mtd: nand: raw: rockchip-nand-controller: copy hwecc PA data to
    oob_poi buffer
  mtd: nand: raw: rockchip-nand-controller: add skipbbt option
  mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and
    description
  mtd: nand: raw: add basic sandisk manufacturer ops

Paweł Jarosz (1):
  mtd: nand: add support for the Sandisk SDTNQGAMA chip

 drivers/mtd/nand/raw/Makefile                 |  1 +
 drivers/mtd/nand/raw/internals.h              |  1 +
 drivers/mtd/nand/raw/nand_ids.c               |  5 +-
 drivers/mtd/nand/raw/nand_sandisk.c           | 26 ++++++++++
 .../mtd/nand/raw/rockchip-nand-controller.c   | 52 ++++++++++++-------
 5 files changed, 64 insertions(+), 21 deletions(-)
 create mode 100644 drivers/mtd/nand/raw/nand_sandisk.c

--
2.30.2

[PATCH v2 1/5] mtd: nand: raw: rockchip-nand-controller: copy hwecc PA data to oob_poi buffer

[Johan Jonker]

Rockchip boot blocks are written per 4 x 512 byte sectors per page.
Each page must have a page address (PA) pointer in OOB to the next page.
Pages are written in a pattern depending on the NAND chip ID.
This logic used to build a page pattern table is not fully disclosed and
is not easy to fit in the MTD framework.
The formula in rk_nfc_write_page_hwecc() function is not correct.
Make hwecc and raw behavior identical.
Generate boot block page address and pattern for hwecc in user space
and copy PA data to/from the last 4 bytes in the
chip->oob_poi data layout.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 .../mtd/nand/raw/rockchip-nand-controller.c   | 34 ++++++++++++-------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index 2312e2736..cafccc324 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -597,7 +597,7 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
 	int pages_per_blk = mtd->erasesize / mtd->writesize;
 	int ret = 0, i, boot_rom_mode = 0;
 	dma_addr_t dma_data, dma_oob;
-	u32 reg;
+	u32 tmp;
 	u8 *oob;

 	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
@@ -624,6 +624,13 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
 	 *
 	 *   0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
 	 *
+	 * The code here just swaps the first 4 bytes with the last
+	 * 4 bytes without losing any data.
+	 *
+	 * The chip->oob_poi data layout:
+	 *
+	 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
+	 *
 	 * Configure the ECC algorithm supported by the boot ROM.
 	 */
 	if ((page < (pages_per_blk * rknand->boot_blks)) &&
@@ -634,21 +641,17 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
 	}

 	for (i = 0; i < ecc->steps; i++) {
-		if (!i) {
-			reg = 0xFFFFFFFF;
-		} else {
+		if (!i)
+			oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
+		else
 			oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
-			reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
-			      oob[3] << 24;
-		}

-		if (!i && boot_rom_mode)
-			reg = (page & (pages_per_blk - 1)) * 4;
+		tmp = oob[0] | oob[1] << 8 | oob[2] << 16 | oob[3] << 24;

 		if (nfc->cfg->type == NFC_V9)
-			nfc->oob_buf[i] = reg;
+			nfc->oob_buf[i] = tmp;
 		else
-			nfc->oob_buf[i * (oob_step / 4)] = reg;
+			nfc->oob_buf[i * (oob_step / 4)] = tmp;
 	}

 	dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
@@ -811,12 +814,17 @@ static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *buf, int oob_on,
 		goto timeout_err;
 	}

-	for (i = 1; i < ecc->steps; i++) {
-		oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
+	for (i = 0; i < ecc->steps; i++) {
+		if (!i)
+			oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
+		else
+			oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
+
 		if (nfc->cfg->type == NFC_V9)
 			tmp = nfc->oob_buf[i];
 		else
 			tmp = nfc->oob_buf[i * (oob_step / 4)];
+
 		*oob++ = (u8)tmp;
 		*oob++ = (u8)(tmp >> 8);
 		*oob++ = (u8)(tmp >> 16);
--
2.30.2

[PATCH v2 2/5] mtd: nand: raw: rockchip-nand-controller: add skipbbt option

[Johan Jonker]

On Rockchip SoCs the first boot stages are written on NAND
with help of manufacturer software that uses a different format
then the MTD framework. Skip the automatic BBT scan with the
NAND_SKIP_BBTSCAN option so that the original content is unchanged
during the driver probe.
The NAND_NO_BBM_QUIRK option allows us to erase bad blocks with
the nand_erase_nand() function and the flash_erase command.
With these options the user has the "freedom of choice" by neutral
access mode to read and write in whatever format is needed.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---

Changed V2:
  reword
---
 drivers/mtd/nand/raw/rockchip-nand-controller.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index cafccc324..31d8c7a87 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -188,6 +188,10 @@ struct rk_nfc {
 	unsigned long assigned_cs;
 };

+static int skipbbt;
+module_param(skipbbt, int, 0644);
+MODULE_PARM_DESC(skipbbt, "Skip BBT scan if data on the NAND chip is not in MTD format.");
+
 static inline struct rk_nfc_nand_chip *rk_nfc_to_rknand(struct nand_chip *chip)
 {
 	return container_of(chip, struct rk_nfc_nand_chip, chip);
@@ -1156,6 +1160,9 @@ static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,

 	nand_set_controller_data(chip, nfc);

+	if (skipbbt)
+		chip->options |= NAND_SKIP_BBTSCAN | NAND_NO_BBM_QUIRK;
+
 	chip->options |= NAND_USES_DMA | NAND_NO_SUBPAGE_WRITE;
 	chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;

--
2.30.2

[PATCH v2 3/5] mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and description

[Johan Jonker]

The MTD framework reserves 1 or 2 bytes for the bad block marker
depending on the bus size. The rockchip-nand-controller driver
currently only supports a 8 bit bus, but reserves standard 2 bytes
for the BBM. The first free OOB byte is therefore OOB2 at offset 2.
Page address(PA) bytes are moved to the last 4 positions before
ECC. Update the description for Linux.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index 31d8c7a87..fcda4c760 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
 		 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
 		 *
 		 * The rk_nfc_ooblayout_free() function already has reserved
-		 * these 4 bytes with:
+		 * these 4 bytes together with 2 bytes for BBM
+		 * by reducing it's length:
 		 *
-		 * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
+		 * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
 		 */
 		if (!i)
 			memcpy(rk_nfc_oob_ptr(chip, i),
@@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
 	if (section)
 		return -ERANGE;

-	/*
-	 * The beginning of the OOB area stores the reserved data for the NFC,
-	 * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
-	 */
 	oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
-	oob_region->offset = NFC_SYS_DATA_SIZE + 2;
+	oob_region->offset = 2;

 	return 0;
 }
--
2.30.2

[PATCH v2 4/5] mtd: nand: raw: add basic sandisk manufacturer ops

[Johan Jonker]

Add basic Sandisk manufacturer ops support to get
SDTNQGAMA timing data with the nand_get_sdr_timings()
function.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---

Limited to one chip model for now.
In need for someone with better knowledge of the Sandisk product
range to expand and improve. Documentation is rare to find.
---
 drivers/mtd/nand/raw/Makefile       |  1 +
 drivers/mtd/nand/raw/internals.h    |  1 +
 drivers/mtd/nand/raw/nand_ids.c     |  2 +-
 drivers/mtd/nand/raw/nand_sandisk.c | 26 ++++++++++++++++++++++++++
 4 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/raw/nand_sandisk.c

diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 917cdfb81..d93e861d8 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -67,5 +67,6 @@ nand-objs += nand_esmt.o
 nand-objs += nand_hynix.o
 nand-objs += nand_macronix.o
 nand-objs += nand_micron.o
+nand-objs += nand_sandisk.o
 nand-objs += nand_samsung.o
 nand-objs += nand_toshiba.o
diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
index 7016e0f38..e9932da18 100644
--- a/drivers/mtd/nand/raw/internals.h
+++ b/drivers/mtd/nand/raw/internals.h
@@ -73,6 +73,7 @@ extern const struct nand_manufacturer_ops hynix_nand_manuf_ops;
 extern const struct nand_manufacturer_ops macronix_nand_manuf_ops;
 extern const struct nand_manufacturer_ops micron_nand_manuf_ops;
 extern const struct nand_manufacturer_ops samsung_nand_manuf_ops;
+extern const struct nand_manufacturer_ops sandisk_nand_manuf_ops;
 extern const struct nand_manufacturer_ops toshiba_nand_manuf_ops;

 /* MLC pairing schemes */
diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
index dacc5529b..1a89ed796 100644
--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -188,7 +188,7 @@ static const struct nand_manufacturer_desc nand_manufacturer_descs[] = {
 	{NAND_MFR_NATIONAL, "National"},
 	{NAND_MFR_RENESAS, "Renesas"},
 	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
-	{NAND_MFR_SANDISK, "SanDisk"},
+	{NAND_MFR_SANDISK, "SanDisk", &sandisk_nand_manuf_ops},
 	{NAND_MFR_STMICRO, "ST Micro"},
 	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
 	{NAND_MFR_WINBOND, "Winbond"},
diff --git a/drivers/mtd/nand/raw/nand_sandisk.c b/drivers/mtd/nand/raw/nand_sandisk.c
new file mode 100644
index 000000000..7c66e4187
--- /dev/null
+++ b/drivers/mtd/nand/raw/nand_sandisk.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include "internals.h"
+
+static int
+sdtnqgama_choose_interface_config(struct nand_chip *chip,
+				  struct nand_interface_config *iface)
+{
+	onfi_fill_interface_config(chip, iface, NAND_SDR_IFACE, 0);
+
+	return nand_choose_best_sdr_timings(chip, iface, NULL);
+}
+
+static int sandisk_nand_init(struct nand_chip *chip)
+{
+	if (!strncmp("SDTNQGAMA", chip->parameters.model,
+		     sizeof("SDTNQGAMA") - 1))
+		chip->ops.choose_interface_config =
+			&sdtnqgama_choose_interface_config;
+
+	return 0;
+}
+
+const struct nand_manufacturer_ops sandisk_nand_manuf_ops = {
+	.init = sandisk_nand_init,
+};
--
2.30.2

[PATCH v2 5/5] mtd: nand: add support for the Sandisk SDTNQGAMA chip

[Johan Jonker]

Sandisk SDTNQGAMA is a 8GB size, 3.3V 8 bit chip with 16KB page size,
1KB write size and 40 bit ecc support

Co-developed-by: Paweł Jarosz <paweljarosz3691@gmail.com>
Signed-off-by: Paweł Jarosz <paweljarosz3691@gmail.com>
Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 drivers/mtd/nand/raw/nand_ids.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
index 1a89ed796..650351c62 100644
--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -44,6 +44,9 @@ struct nand_flash_dev nand_flash_ids[] = {
 	{"TC58NVG6D2 64G 3.3V 8-bit",
 		{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20} },
 		  SZ_8K, SZ_8K, SZ_2M, 0, 8, 640, NAND_ECC_INFO(40, SZ_1K) },
+	{"SDTNQGAMA 64G 3.3V 8-bit",
+		{ .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x57} },
+		  SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
 	{"SDTNRGAMA 64G 3.3V 8-bit",
 		{ .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x50} },
 		  SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
--
2.30.2

Re: [PATCH v2 3/5] mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and description

[Miquel Raynal]

Hi Johan,

jbx6244@gmail.com wrote on Mon, 12 Jun 2023 17:03:18 +0200:

> The MTD framework reserves 1 or 2 bytes for the bad block marker
> depending on the bus size. The rockchip-nand-controller driver
> currently only supports a 8 bit bus, but reserves standard 2 bytes
> for the BBM.

We always reserve 2 bytes, no?

> The first free OOB byte is therefore OOB2 at offset 2.
> Page address(PA) bytes are moved to the last 4 positions before
> ECC. Update the description for Linux.

The description should just be:

Move Page Address (PA) bytes to the last 4 positions before ECC.

And then you should justify why this is needed. Also, this would break
all existing jffs2 users, right?

> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> index 31d8c7a87..fcda4c760 100644
> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
>  		 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
>  		 *
>  		 * The rk_nfc_ooblayout_free() function already has reserved
> -		 * these 4 bytes with:
> +		 * these 4 bytes together with 2 bytes for BBM
> +		 * by reducing it's length:
>  		 *
> -		 * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> +		 * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>  		 */
>  		if (!i)
>  			memcpy(rk_nfc_oob_ptr(chip, i),
> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>  	if (section)
>  		return -ERANGE;
> 
> -	/*
> -	 * The beginning of the OOB area stores the reserved data for the NFC,
> -	 * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
> -	 */
>  	oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
> -	oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> +	oob_region->offset = 2;
> 
>  	return 0;
>  }
> --
> 2.30.2
> 


Thanks,
Miquèl

Re: [PATCH v2 1/5] mtd: nand: raw: rockchip-nand-controller: copy hwecc PA data to oob_poi buffer

[Miquel Raynal]

Hi Johan,

jbx6244@gmail.com wrote on Mon, 12 Jun 2023 17:02:40 +0200:

> Rockchip boot blocks are written per 4 x 512 byte sectors per page.
> Each page must have a page address (PA) pointer in OOB to the next page.
> Pages are written in a pattern depending on the NAND chip ID.
> This logic used to build a page pattern table is not fully disclosed and
> is not easy to fit in the MTD framework.
> The formula in rk_nfc_write_page_hwecc() function is not correct.
> Make hwecc and raw behavior identical.
> Generate boot block page address and pattern for hwecc in user space
> and copy PA data to/from the last 4 bytes in the
> chip->oob_poi data layout.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  .../mtd/nand/raw/rockchip-nand-controller.c   | 34 ++++++++++++-------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> index 2312e2736..cafccc324 100644
> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -597,7 +597,7 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>  	int pages_per_blk = mtd->erasesize / mtd->writesize;
>  	int ret = 0, i, boot_rom_mode = 0;
>  	dma_addr_t dma_data, dma_oob;
> -	u32 reg;
> +	u32 tmp;
>  	u8 *oob;
> 
>  	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> @@ -624,6 +624,13 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>  	 *
>  	 *   0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
>  	 *
> +	 * The code here just swaps the first 4 bytes with the last
> +	 * 4 bytes without losing any data.

Maybe you don't loose any data, but you basically break all existing
jffs2 users, right? Is this page address only useful on your rk SoC or
are all the SoCs using the same logic?

I think it would be best to flag where this is required and avoid a
massive incompatible change like this one (and the previous one). BTW,
any reason not to merge the two first patches? It seems like the series
would not be bisectable between the two first commits.

Patches 4 and 5 look good as they are not directly related, I'll queue
them, you can avoid re-sending them.

> +	 *
> +	 * The chip->oob_poi data layout:
> +	 *
> +	 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
> +	 *
>  	 * Configure the ECC algorithm supported by the boot ROM.
>  	 */
>  	if ((page < (pages_per_blk * rknand->boot_blks)) &&
> @@ -634,21 +641,17 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>  	}
> 
>  	for (i = 0; i < ecc->steps; i++) {
> -		if (!i) {
> -			reg = 0xFFFFFFFF;
> -		} else {
> +		if (!i)
> +			oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
> +		else
>  			oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> -			reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
> -			      oob[3] << 24;
> -		}
> 
> -		if (!i && boot_rom_mode)
> -			reg = (page & (pages_per_blk - 1)) * 4;
> +		tmp = oob[0] | oob[1] << 8 | oob[2] << 16 | oob[3] << 24;
> 
>  		if (nfc->cfg->type == NFC_V9)
> -			nfc->oob_buf[i] = reg;
> +			nfc->oob_buf[i] = tmp;
>  		else
> -			nfc->oob_buf[i * (oob_step / 4)] = reg;
> +			nfc->oob_buf[i * (oob_step / 4)] = tmp;
>  	}
> 
>  	dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
> @@ -811,12 +814,17 @@ static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *buf, int oob_on,
>  		goto timeout_err;
>  	}
> 
> -	for (i = 1; i < ecc->steps; i++) {
> -		oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> +	for (i = 0; i < ecc->steps; i++) {
> +		if (!i)
> +			oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
> +		else
> +			oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> +
>  		if (nfc->cfg->type == NFC_V9)
>  			tmp = nfc->oob_buf[i];
>  		else
>  			tmp = nfc->oob_buf[i * (oob_step / 4)];
> +
>  		*oob++ = (u8)tmp;
>  		*oob++ = (u8)(tmp >> 8);
>  		*oob++ = (u8)(tmp >> 16);
> --
> 2.30.2
> 


Thanks,
Miquèl

Re: [PATCH v2 4/5] mtd: nand: raw: add basic sandisk manufacturer ops

[Miquel Raynal]

Hi Johan,

jbx6244@gmail.com wrote on Mon, 12 Jun 2023 17:03:35 +0200:

> Add basic Sandisk manufacturer ops support to get
> SDTNQGAMA timing data with the nand_get_sdr_timings()
> function.

Actually you can send patches 4 and 5 aside, in a dedicated series,
with the prefixes changed to "mtd: rawnand: ".

Thanks,
Miquèl

Re: [PATCH v2 3/5] mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and description

[Johan Jonker]

On 6/12/23 19:26, Miquel Raynal wrote:
> Hi Johan,
> 
> jbx6244@gmail.com wrote on Mon, 12 Jun 2023 17:03:18 +0200:
> 
>> The MTD framework reserves 1 or 2 bytes for the bad block marker
>> depending on the bus size. The rockchip-nand-controller driver
>> currently only supports a 8 bit bus, but reserves standard 2 bytes
>> for the BBM.
> 
> We always reserve 2 bytes, no?

Not always used, but for consistency/simplicity the author assumes/reserves 2 bytes. 

> 
>> The first free OOB byte is therefore OOB2 at offset 2.
>> Page address(PA) bytes are moved to the last 4 positions before
>> ECC. Update the description for Linux.
> 
> The description should just be:
> 

> Move Page Address (PA) bytes to the last 4 positions before ECC.

Space is already reserved, but overwritten.

> 
> And then you should justify why this is needed. Also, this would break
> all existing jffs2 users, right?

Hi Miquel,

From your comments it seems that the chip->oob_poi buffer layout is still not clear to you.
Hope that this text below helps.
If existing jffs2 users of free OOB are writing they are corrupting our PA data in RAW mode.
So that must be fixed. Please advise how we split pre and post change users.
(With a Module parameter like skipbbt renamed to "user_mode" = 0 offset 6, "user_mode" = 1 offset 2)
Copying PA data in both RAW and HW mode has already reserved space in the layout.
Let me know if I can help to get forward here.

Johan

===

Given:

Rockchip rk3066 MK808 with NAND:
nand: Hynix H27UCG8T2ATR-BC 64G 3.3V 8-bit
nand: 8192 MiB, MLC, erase size: 2048 KiB, page size: 8192, OOB size: 640

===

Calulations:

#define NFC_SYS_DATA_SIZE		(4) /* 4 bytes sys data in oob pre 1024 data.*/

So per step only 4 bytes of OOB can be read.

===

The NFC can read/write in 1024 data bytes per step.
To read/write a full page it needs 8 steps.

chip->ecc.size = 1024;
chip->ecc.steps = mtd->writesize / chip->ecc.size;
                = 8192 / 1024
                = 8 steps
===

The total size of usefull OOB before ECC:

rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
                      = 4 * 8
                      = 32
===

Wrong free OOB offset starts at OOB6:
oob_region->offset = NFC_SYS_DATA_SIZE + 2;
                   = 4 + 2
                   = 6

With a free OOB offset of 6 and a length of 26 ==> 6 + 26 = 32 we corrupt the PA address starting at offset 28.

New offset OOB2:
oob_region->offset = 2;

The full range of free runs from OOB2 till/including OOB27.
===

The last 4 bytes of metadata are reserved for this Page Address(PA) for the bootrom.
Currently only in use in RAW mode.
The current PA calculation needed to write boot blocks for all Rockchip SoCs is however useless.
The pattern of where the next page is written depends on the chip ID.
As the MTD framework doesn't pass this chip ID in it's data structures,
we must calculate that in userspace.

Therefore both RAW and HW mode must pass the PA bytes.

===

The NFC hardware is capable for a 16 bit bus, but not implemented yet.
Reserved are standard 2 bits for the BBM for a consistantency by the original author.

===

chip->oob_poi buffer layout for 8 steps:

BBM0   BBM1  OOB2  OOB3  | OOB4  OOB5  OOB6  OOB7

OOB8   OOB9  OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15
OOB16  OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23

OOB24  OOB25 OOB26 OOB27 | PA0   PA1   PA2   PA3

ECC0   ECC1  ECC2  ECC3  | ...   ...   ...   ...

===

rk_nfc_ooblayout_free:
oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
                   = 32 - 4 - 2
                   = 26

oob_region->offset = 2;

Free OOB should start at OOB2 to not overwrite PA data.

===

rk_nfc_ooblayout_ecc:
	oob_region->length = mtd->oobsize - rknand->metadata_size;
	                   = 640 - 32
	                   = 608
	oob_region->offset = rknand->metadata_size;
	                   = 32

ECC data starts at offset 32.

===


> 
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> index 31d8c7a87..fcda4c760 100644
>> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> @@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
>>  		 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2  PA3
>>  		 *
>>  		 * The rk_nfc_ooblayout_free() function already has reserved
>> -		 * these 4 bytes with:
>> +		 * these 4 bytes together with 2 bytes for BBM
>> +		 * by reducing it's length:
>>  		 *
>> -		 * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> +		 * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>>  		 */
>>  		if (!i)
>>  			memcpy(rk_nfc_oob_ptr(chip, i),
>> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>>  	if (section)
>>  		return -ERANGE;
>>
>> -	/*
>> -	 * The beginning of the OOB area stores the reserved data for the NFC,
>> -	 * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
>> -	 */
>>  	oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>> -	oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> +	oob_region->offset = 2;
>>
>>  	return 0;
>>  }
>> --
>> 2.30.2
>>
> 
> 
> Thanks,
> Miquèl

Re: [PATCH v2 3/5] mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and description

[Miquel Raynal]

Hi Johan,

jbx6244@gmail.com wrote on Wed, 14 Jun 2023 11:23:44 +0200:

> On 6/12/23 19:26, Miquel Raynal wrote:
> > Hi Johan,
> > 
> > jbx6244@gmail.com wrote on Mon, 12 Jun 2023 17:03:18 +0200:
> >   
> >> The MTD framework reserves 1 or 2 bytes for the bad block marker
> >> depending on the bus size. The rockchip-nand-controller driver
> >> currently only supports a 8 bit bus, but reserves standard 2 bytes
> >> for the BBM.  
> > 
> > We always reserve 2 bytes, no?  
> 
> Not always used, but for consistency/simplicity the author assumes/reserves 2 bytes. 

It's kind of an implicit rule in the raw NAND subsystem. It's not an
author choice.

> >> The first free OOB byte is therefore OOB2 at offset 2.
> >> Page address(PA) bytes are moved to the last 4 positions before
> >> ECC. Update the description for Linux.  
> > 
> > The description should just be:
> >   
> 
> > Move Page Address (PA) bytes to the last 4 positions before ECC.  
> 
> Space is already reserved, but overwritten.

Well, I don't know, but I'm quoting your commit log "Page address(PA)
bytes are moved to the last 4 positions before ECC" and if this
sentence is right, I am proposing another way to say this which sounds
more declarative.

> 
> > 
> > And then you should justify why this is needed. Also, this would
> > break all existing jffs2 users, right?  
> 
> Hi Miquel,
> 
> From your comments it seems that the chip->oob_poi buffer layout is
> still not clear to you. Hope that this text below helps.
> If existing jffs2 users of free OOB are writing

They are, it's the first thing that jjfs2 does: writing cleanmarkers in
the free area.

> they are corrupting
> our PA data in RAW mode. So that must be fixed. 

I did not yet understand whether corrupting the PA data was an absolute
mistake or if it was specific to a given range of ROM codes. But let's
assume it must be fixed.

> Please advise how we
> split pre and post change users.

If you change the layout, you break users. There is no question here.
But if you do that, we need:
- a crystal clear explanation of why this is needed
- to say it clearly: this change breaks existing jffs2 users

> (With a Module parameter like
> skipbbt renamed to "user_mode" = 0 offset 6, "user_mode" = 1 offset

I know the cafe driver does that, it is awful IMHO.

> 2) Copying PA data in both RAW and HW mode has already reserved space
> in the layout. Let me know if I can help to get forward here.
> 
> Johan
> 
> ===
> 
> Given:
> 
> Rockchip rk3066 MK808 with NAND:
> nand: Hynix H27UCG8T2ATR-BC 64G 3.3V 8-bit
> nand: 8192 MiB, MLC, erase size: 2048 KiB, page size: 8192, OOB size:
> 640
> 
> ===
> 
> Calulations:
> 
> #define NFC_SYS_DATA_SIZE		(4) /* 4 bytes sys data in
> oob pre 1024 data.*/
> 
> So per step only 4 bytes of OOB can be read.

I think I get what you mean but the above sentence is wrong. You can
always read the full OOB in raw mode. And in general you can as well in
host ECC mode. Then what users do with the OOB information is
orthogonal. However, if they don't want their data to be smashed, they
can request the information about which bytes are free to be used
(typically what jffs2 does, while ubi does not care about OOB). The oob
layout helpers can then restrain the advertised free area to only share
bytes which are not used by the PA.

> 
> ===
> 
> The NFC can read/write in 1024 data bytes per step.
> To read/write a full page it needs 8 steps.
> 
> chip->ecc.size = 1024;
> chip->ecc.steps = mtd->writesize / chip->ecc.size;
>                 = 8192 / 1024
>                 = 8 steps
> ===
> 
> The total size of usefull OOB before ECC:
> 
> rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
>                       = 4 * 8
>                       = 32
> ===
> 
> Wrong free OOB offset starts at OOB6:
> oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>                    = 4 + 2
>                    = 6
> 
> With a free OOB offset of 6 and a length of 26 ==> 6 + 26 = 32 we
> corrupt the PA address starting at offset 28.
> 
> New offset OOB2:
> oob_region->offset = 2;
> 
> The full range of free runs from OOB2 till/including OOB27.
> ===
> 
> The last 4 bytes of metadata are reserved for this Page Address(PA)
> for the bootrom. Currently only in use in RAW mode.

I'm not sure to understand what "currently on ly in use in raw mode".

In raw mode, the user can overwrite the whole OOB area, it is the user
input what should be written in each and every byte.

In ECC mode the ECC engine will smash some of this data to write its
own ECC bytes.

> The current PA calculation needed to write boot blocks for all
> Rockchip SoCs is however useless. The pattern of where the next page
> is written depends on the chip ID. As the MTD framework doesn't pass
> this chip ID in it's data structures, we must calculate that in
> userspace.

yes, I agree the right approach if you need to write these is to
perform raw OOB writes with values calculated manually.

> Therefore both RAW and HW mode must pass the PA bytes.

Yes, no problem with that.

> ===
> 
> The NFC hardware is capable for a 16 bit bus, but not implemented yet.
> Reserved are standard 2 bits for the BBM for a consistantency by the
> original author.
> 
> ===
> 
> chip->oob_poi buffer layout for 8 steps:
> 
> BBM0   BBM1  OOB2  OOB3  | OOB4  OOB5  OOB6  OOB7
> 
> OOB8   OOB9  OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15
> OOB16  OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23
> 
> OOB24  OOB25 OOB26 OOB27 | PA0   PA1   PA2   PA3
> 
> ECC0   ECC1  ECC2  ECC3  | ...   ...   ...   ...

Yes.

> 
> ===
> 
> rk_nfc_ooblayout_free:
> oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>                    = 32 - 4 - 2
>                    = 26
> 
> oob_region->offset = 2;
> 
> Free OOB should start at OOB2 to not overwrite PA data.

Yes.

> 
> ===
> 
> rk_nfc_ooblayout_ecc:
> 	oob_region->length = mtd->oobsize - rknand->metadata_size;
> 	                   = 640 - 32
> 	                   = 608
> 	oob_region->offset = rknand->metadata_size;
> 	                   = 32
> 
> ECC data starts at offset 32.

Yes.

> 
> ===
> 
> 
> >   
> >>
> >> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> >> ---
> >>  drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
> >>  1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> >> b/drivers/mtd/nand/raw/rockchip-nand-controller.c index
> >> 31d8c7a87..fcda4c760 100644 ---
> >> a/drivers/mtd/nand/raw/rockchip-nand-controller.c +++
> >> b/drivers/mtd/nand/raw/rockchip-nand-controller.c @@ -566,9
> >> +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip
> >> *chip, const u8 *buf,
> >>  		 *    BBM  OOB1 OOB2 OOB3 |......|  PA0  PA1  PA2
> >>  PA3 *
> >>  		 * The rk_nfc_ooblayout_free() function already
> >> has reserved
> >> -		 * these 4 bytes with:
> >> +		 * these 4 bytes together with 2 bytes for BBM
> >> +		 * by reducing it's length:
> >>  		 *
> >> -		 * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> >> +		 * oob_region->length = rknand->metadata_size -
> >> NFC_SYS_DATA_SIZE - 2; */
> >>  		if (!i)
> >>  			memcpy(rk_nfc_oob_ptr(chip, i),
> >> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct
> >> mtd_info *mtd, int section, if (section)
> >>  		return -ERANGE;
> >>
> >> -	/*
> >> -	 * The beginning of the OOB area stores the reserved data
> >> for the NFC,
> >> -	 * the size of the reserved data is NFC_SYS_DATA_SIZE
> >> bytes.
> >> -	 */
> >>  	oob_region->length = rknand->metadata_size -
> >> NFC_SYS_DATA_SIZE - 2;
> >> -	oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> >> +	oob_region->offset = 2;
> >>
> >>  	return 0;
> >>  }
> >> --
> >> 2.30.2
> >>  
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl