Received-SPF: pass (google.com: domain of linux-kernel+bounces-179656-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 15 May 2024 10:29:01 +0200
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] mtd: nand: mxc_nand: support software ECC
Message-ID: <ZkRyTT7FcDQ-5UeV@pengutronix.de>
References: <20240514-mtd-nand-mxc-nand-exec-op-v3-0-3e6f45fd3d82@pengutronix.de>
 <20240514-mtd-nand-mxc-nand-exec-op-v3-3-3e6f45fd3d82@pengutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240514-mtd-nand-mxc-nand-exec-op-v3-3-3e6f45fd3d82@pengutronix.de>

On Tue, May 14, 2024 at 04:25:19PM +0200, Sascha Hauer wrote:
> With these changes the driver can be used with software BCH ECC which
> is useful for NAND chips that require a stronger ECC than the i.MX
> hardware supports.
> 
> The controller normally interleaves user data with OOB data when
> accessing the NAND chip. With Software BCH ECC we write the data
> to the NAND in a way that the raw data on the NAND chip matches the
> way the NAND layer sees it. This way commands like NAND_CMD_RNDOUT
> work as expected.
> 
> This was tested on i.MX27 but should work on the other SoCs supported
> by this driver as well.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 73 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index 6f8b8f4b118ec..ae4cff2584a2d 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1404,10 +1404,10 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
>  	chip->ecc.bytes = host->devtype_data->eccbytes;
>  	host->eccsize = host->devtype_data->eccsize;
>  	chip->ecc.size = 512;
> -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
>  
>  	switch (chip->ecc.engine_type) {
>  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
>  		chip->ecc.read_page = mxc_nand_read_page;
>  		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
>  		chip->ecc.read_oob = mxc_nand_read_oob;
> @@ -1417,6 +1417,8 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
>  		break;
>  
>  	case NAND_ECC_ENGINE_TYPE_SOFT:
> +		chip->ecc.write_page_raw = nand_monolithic_write_page_raw;
> +		chip->ecc.read_page_raw = nand_monolithic_read_page_raw;
>  		break;
>  
>  	default:
> @@ -1472,6 +1474,59 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
>  	return host->devtype_data->setup_interface(chip, chipnr, conf);
>  }
>  
> +static void copy_page_to_sram(struct mtd_info *mtd, const void *buf, int buf_len)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(this);
> +	unsigned int no_subpages = mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	oob_per_subpage = (mtd->oobsize / no_subpages) & ~1;
> +
> +	/*
> +	 * During a page write the i.MX NAND controller will read 512b from
> +	 * main_area0 SRAM, then oob_per_subpage bytes from spare0 SRAM, then
> +	 * 512b from main_area1 SRAM and so on until the full page is written.
> +	 * For software ECC we want to have a 1:1 mapping between the raw page
> +	 * data on the NAND chip and the view of the NAND core. This is
> +	 * necessary to make the NAND_CMD_RNDOUT read the data it expects.
> +	 * To accomplish this we have to write the data in the order the controller
> +	 * reads it. This is reversed in copy_page_from_sram() below.
> +	 */
> +	for (i = 0; i < no_subpages; i++) {
> +		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
> +		buf += 512;
> +
> +		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
> +			      oob_per_subpage);
> +		buf += oob_per_subpage;
> +	}
> +}

I noticed the nandbiterr test won't work with this. It needs the following
fixup. The problem is that the core wants to write only user data
without OOB, so we have to make sure the remaining SRAM is filled up
with 0xff.

I'll wait some time for other comments before sending a v4 with this
included.

Sascha

-----------------------------8<--------------------------------

From 8287fc3b44c22811350467f00b5652c94a816a36 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Wed, 15 May 2024 10:25:29 +0200
Subject: [PATCH] fixup! mtd: nand: mxc_nand: support software ECC

---
 drivers/mtd/nand/raw/mxc_nand.c | 35 ++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index ae4cff2584a2d..9d50f41b3d5a1 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1474,6 +1474,15 @@ static int mxcnd_setup_interface(struct nand_chip *chip, int chipnr,
 	return host->devtype_data->setup_interface(chip, chipnr, conf);
 }
 
+static void memff16_toio(void *buf, int n)
+{
+	__iomem u16 *t = buf;
+	int i;
+
+	for (i = 0; i < (n >> 1); i++)
+		__raw_writew(0xffff, t++);
+}
+
 static void copy_page_to_sram(struct mtd_info *mtd, const void *buf, int buf_len)
 {
 	struct nand_chip *this = mtd_to_nand(mtd);
@@ -1492,14 +1501,34 @@ static void copy_page_to_sram(struct mtd_info *mtd, const void *buf, int buf_len
 	 * necessary to make the NAND_CMD_RNDOUT read the data it expects.
 	 * To accomplish this we have to write the data in the order the controller
 	 * reads it. This is reversed in copy_page_from_sram() below.
+	 *
+	 * buf_len can either be the full page including the OOB or user data only.
+	 * When it's user data only make sure that we fill up the rest of the
+	 * SRAM with 0xff.
 	 */
 	for (i = 0; i < no_subpages; i++) {
-		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
+		int now = min(buf_len, 512);
+
+		if (now)
+			memcpy16_toio(host->main_area0 + i * 512, buf, now);
+
+		if (now < 512)
+			memff16_toio(host->main_area0 + i * 512 + now, 512 - now);
+
 		buf += 512;
+		buf_len -= now;
+
+		now = min(buf_len, oob_per_subpage);
+		if (now)
+			memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len,
+				      buf, now);
+
+		if (now < oob_per_subpage)
+			memff16_toio(host->spare0 + i * host->devtype_data->spare_len + now,
+				     oob_per_subpage - now);
 
-		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
-			      oob_per_subpage);
 		buf += oob_per_subpage;
+		buf_len -= now;
 	}
 }
 
-- 
2.39.2


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |