Received-SPF: pass (google.com: domain of linux-kernel+bounces-177269-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 13 May 2024 09:42:17 +0200
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Richard Weinberger <richard@nod.at>, Vignesh Raghavendra
 <vigneshr@ti.com>, linux-mtd@lists.infradead.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] mtd: nand: mxc_nand: support software ECC
Message-ID: <20240513094217.4c1a0fc2@xps-13>
In-Reply-To: <20240508-mtd-nand-mxc-nand-exec-op-v2-3-6b7366b7831f@pengutronix.de>
References: <20240508-mtd-nand-mxc-nand-exec-op-v2-0-6b7366b7831f@pengutronix.de>
	<20240508-mtd-nand-mxc-nand-exec-op-v2-3-6b7366b7831f@pengutronix.de>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Sascha,

s.hauer@pengutronix.de wrote on Wed, 08 May 2024 13:08:29 +0200:

> 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.
>=20
> 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.
>=20
> This was tested on i.MX27 but should work on the other SoCs supported
> by this driver as well.
>=20
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/raw/mxc_nand.c | 84 +++++++++++++++++++++++++++++++++++=
+-----
>  1 file changed, 75 insertions(+), 9 deletions(-)
>=20
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_n=
and.c
> index 1e8b4553e03ba..7004fd57c80f7 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -178,6 +178,7 @@ struct mxc_nand_host {
>  	int			used_oobsize;
>  	int			active_cs;
>  	unsigned int		ecc_stats_v1;
> +	unsigned int		column;
> =20
>  	struct completion	op_completion;
> =20
> @@ -1397,10 +1398,10 @@ static int mxcnd_attach_chip(struct nand_chip *ch=
ip)
>  	chip->ecc.bytes =3D host->devtype_data->eccbytes;
>  	host->eccsize =3D host->devtype_data->eccsize;
>  	chip->ecc.size =3D 512;
> -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> =20
>  	switch (chip->ecc.engine_type) {
>  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
>  		chip->ecc.read_page =3D mxc_nand_read_page;
>  		chip->ecc.read_page_raw =3D mxc_nand_read_page_raw;
>  		chip->ecc.read_oob =3D mxc_nand_read_oob;
> @@ -1465,6 +1466,54 @@ static int mxcnd_setup_interface(struct nand_chip =
*chip, int chipnr,
>  	return host->devtype_data->setup_interface(chip, chipnr, conf);
>  }
> =20
> +static void copy_page_to_sram(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this =3D mtd_to_nand(mtd);
> +	struct mxc_nand_host *host =3D nand_get_controller_data(this);
> +	void *buf =3D host->data_buf;
> +	unsigned int n_subpages =3D mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages =3D 1;
> +
> +	oob_per_subpage =3D (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i =3D 0; i < n_subpages; i++) {
> +		memcpy16_toio(host->main_area0 + i * 512, buf, 512);
> +		buf +=3D 512;
> +
> +		memcpy16_toio(host->spare0 + i * host->devtype_data->spare_len, buf,
> +			      oob_per_subpage);
> +		buf +=3D oob_per_subpage;
> +	}
> +}
> +
> +static void copy_page_from_sram(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this =3D mtd_to_nand(mtd);
> +	struct mxc_nand_host *host =3D nand_get_controller_data(this);
> +	void *buf =3D host->data_buf;
> +	unsigned int n_subpages =3D mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages =3D 1;
> +
> +	oob_per_subpage =3D (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i =3D 0; i < n_subpages; i++) {
> +		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
> +		buf +=3D 512;
> +
> +		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
> +				oob_per_subpage);
> +		buf +=3D oob_per_subpage;
> +	}
> +}
> +
>  static int mxcnd_exec_op(struct nand_chip *chip,
>  			 const struct nand_operation *op,
>  			 bool check_only)
> @@ -1496,8 +1545,11 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			 */
>  			break;
>  		case NAND_OP_CMD_INSTR:
> -			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_PAGEPROG)
> +			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_PAGEPROG) {
> +				if (chip->ecc.engine_type !=3D NAND_ECC_ENGINE_TYPE_ON_HOST)
> +					copy_page_to_sram(mtd);
>  				host->devtype_data->send_page(mtd, NFC_INPUT);
> +			}

Same as before: data moves should not happen here.

> =20
>  			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> =20
> @@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			if (instr->ctx.cmd.opcode =3D=3D NAND_CMD_STATUS)
>  				statusreq =3D true;
> =20
> +			host->column =3D 0;
> +

This is risky but maybe this is the trick to get NAND_CMD_RNDOUT
working? If yes maybe it is worth separating from this patchset, if
possible, with a comment explaining what this is all about (the
controller's SRAM being some kind of 1:1 mapping with the NAND SRAM,
thus we need to write the data at the correct offset in the controller
SRAM, I guess?).

Please write the comment above when defining the column field, and
also, if my understanding is correct, can we rename it to sram_column
or something like that?

>  			break;
>  		case NAND_OP_ADDR_INSTR:
>  			for (j =3D 0; j < instr->ctx.addr.naddrs; j++) {
> @@ -1517,9 +1571,14 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			buf_write =3D instr->ctx.data.buf.out;
>  			buf_len =3D instr->ctx.data.len;
> =20
> -			memcpy32_toio(host->main_area0, buf_write, buf_len);
> -			if (chip->oob_poi)
> -				copy_spare(mtd, false, chip->oob_poi);
> +			if (chip->ecc.engine_type =3D=3D NAND_ECC_ENGINE_TYPE_ON_HOST) {
> +				memcpy32_toio(host->main_area0, buf_write, buf_len);
> +				if (chip->oob_poi)
> +					copy_spare(mtd, false, chip->oob_poi);
> +			} else {
> +				memcpy(host->data_buf + host->column, buf_write, buf_len);
> +				host->column +=3D buf_len;
> +			}
> =20
>  			break;
>  		case NAND_OP_DATA_IN_INSTR:
> @@ -1552,11 +1611,18 @@ static int mxcnd_exec_op(struct nand_chip *chip,
> =20
>  			host->devtype_data->read_page(chip);
> =20
> -			if (IS_ALIGNED(buf_len, 4)) {
> -				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +			if (chip->ecc.engine_type =3D=3D NAND_ECC_ENGINE_TYPE_ON_HOST) {
> +				if (IS_ALIGNED(buf_len, 4)) {
> +					memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +				} else {
> +					memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> +					memcpy(buf_read, host->data_buf, buf_len);
> +				}
>  			} else {
> -				memcpy32_fromio(host->data_buf, host->main_area0, mtd->writesize);
> -				memcpy(buf_read, host->data_buf, buf_len);
> +				if (!host->column)
> +					copy_page_from_sram(mtd);
> +				memcpy(buf_read, host->data_buf + host->column, buf_len);
> +				host->column +=3D buf_len;
>  			}
> =20
>  			break;
>=20


Thanks,
Miqu=C3=A8l