[PATCH v2 3/3] mtd: nand: mxc_nand: support software ECC

Sascha Hauer posted 3 patches 1 year, 7 months ago
There is a newer version of this series
[PATCH v2 3/3] mtd: nand: mxc_nand: support software ECC
Posted by Sascha Hauer 1 year, 7 months ago
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 | 84 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.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;
 
 	struct completion	op_completion;
 
@@ -1397,10 +1398,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;
@@ -1465,6 +1466,54 @@ 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)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(this);
+	void *buf = host->data_buf;
+	unsigned int n_subpages = mtd->writesize / 512;
+	int oob_per_subpage, i;
+
+	/* mtd->writesize is not set during ident scanning */
+	if (!n_subpages)
+		n_subpages = 1;
+
+	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
+
+	for (i = 0; i < n_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;
+	}
+}
+
+static void copy_page_from_sram(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(this);
+	void *buf = host->data_buf;
+	unsigned int n_subpages = mtd->writesize / 512;
+	int oob_per_subpage, i;
+
+	/* mtd->writesize is not set during ident scanning */
+	if (!n_subpages)
+		n_subpages = 1;
+
+	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
+
+	for (i = 0; i < n_subpages; i++) {
+		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
+		buf += 512;
+
+		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
+				oob_per_subpage);
+		buf += 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 == NAND_CMD_PAGEPROG)
+			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
+				if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
+					copy_page_to_sram(mtd);
 				host->devtype_data->send_page(mtd, NFC_INPUT);
+			}
 
 			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
 
@@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
 				statusreq = true;
 
+			host->column = 0;
+
 			break;
 		case NAND_OP_ADDR_INSTR:
 			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
@@ -1517,9 +1571,14 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 			buf_write = instr->ctx.data.buf.out;
 			buf_len = instr->ctx.data.len;
 
-			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 == 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 += buf_len;
+			}
 
 			break;
 		case NAND_OP_DATA_IN_INSTR:
@@ -1552,11 +1611,18 @@ static int mxcnd_exec_op(struct nand_chip *chip,
 
 			host->devtype_data->read_page(chip);
 
-			if (IS_ALIGNED(buf_len, 4)) {
-				memcpy32_fromio(buf_read, host->main_area0, buf_len);
+			if (chip->ecc.engine_type == 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 += buf_len;
 			}
 
 			break;

-- 
2.39.2
Re: [PATCH v2 3/3] mtd: nand: mxc_nand: support software ECC
Posted by Miquel Raynal 1 year, 7 months ago
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.
> 
> 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 | 84 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.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;
>  
>  	struct completion	op_completion;
>  
> @@ -1397,10 +1398,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;
> @@ -1465,6 +1466,54 @@ 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)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(this);
> +	void *buf = host->data_buf;
> +	unsigned int n_subpages = mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages = 1;
> +
> +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i = 0; i < n_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;
> +	}
> +}
> +
> +static void copy_page_from_sram(struct mtd_info *mtd)
> +{
> +	struct nand_chip *this = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(this);
> +	void *buf = host->data_buf;
> +	unsigned int n_subpages = mtd->writesize / 512;
> +	int oob_per_subpage, i;
> +
> +	/* mtd->writesize is not set during ident scanning */
> +	if (!n_subpages)
> +		n_subpages = 1;
> +
> +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> +
> +	for (i = 0; i < n_subpages; i++) {
> +		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
> +		buf += 512;
> +
> +		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
> +				oob_per_subpage);
> +		buf += 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 == NAND_CMD_PAGEPROG)
> +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
> +				if (chip->ecc.engine_type != 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.

>  
>  			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
>  
> @@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
>  				statusreq = true;
>  
> +			host->column = 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 = 0; j < instr->ctx.addr.naddrs; j++) {
> @@ -1517,9 +1571,14 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  			buf_write = instr->ctx.data.buf.out;
>  			buf_len = instr->ctx.data.len;
>  
> -			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 == 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 += buf_len;
> +			}
>  
>  			break;
>  		case NAND_OP_DATA_IN_INSTR:
> @@ -1552,11 +1611,18 @@ static int mxcnd_exec_op(struct nand_chip *chip,
>  
>  			host->devtype_data->read_page(chip);
>  
> -			if (IS_ALIGNED(buf_len, 4)) {
> -				memcpy32_fromio(buf_read, host->main_area0, buf_len);
> +			if (chip->ecc.engine_type == 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 += buf_len;
>  			}
>  
>  			break;
> 


Thanks,
Miquèl
Re: [PATCH v2 3/3] mtd: nand: mxc_nand: support software ECC
Posted by Sascha Hauer 1 year, 7 months ago
On Mon, May 13, 2024 at 09:42:17AM +0200, Miquel Raynal wrote:
> 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.
> > 
> > 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 | 84 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.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;
> >  
> >  	struct completion	op_completion;
> >  
> > @@ -1397,10 +1398,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;
> > @@ -1465,6 +1466,54 @@ 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)
> > +{
> > +	struct nand_chip *this = mtd_to_nand(mtd);
> > +	struct mxc_nand_host *host = nand_get_controller_data(this);
> > +	void *buf = host->data_buf;
> > +	unsigned int n_subpages = mtd->writesize / 512;
> > +	int oob_per_subpage, i;
> > +
> > +	/* mtd->writesize is not set during ident scanning */
> > +	if (!n_subpages)
> > +		n_subpages = 1;
> > +
> > +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> > +
> > +	for (i = 0; i < n_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;
> > +	}
> > +}
> > +
> > +static void copy_page_from_sram(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *this = mtd_to_nand(mtd);
> > +	struct mxc_nand_host *host = nand_get_controller_data(this);
> > +	void *buf = host->data_buf;
> > +	unsigned int n_subpages = mtd->writesize / 512;
> > +	int oob_per_subpage, i;
> > +
> > +	/* mtd->writesize is not set during ident scanning */
> > +	if (!n_subpages)
> > +		n_subpages = 1;
> > +
> > +	oob_per_subpage = (mtd->oobsize / n_subpages) & ~1;
> > +
> > +	for (i = 0; i < n_subpages; i++) {
> > +		memcpy16_fromio(buf, host->main_area0 + i * 512, 512);
> > +		buf += 512;
> > +
> > +		memcpy16_fromio(buf, host->spare0 + i * host->devtype_data->spare_len,
> > +				oob_per_subpage);
> > +		buf += 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 == NAND_CMD_PAGEPROG)
> > +			if (instr->ctx.cmd.opcode == NAND_CMD_PAGEPROG) {
> > +				if (chip->ecc.engine_type != 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.
> 
> >  
> >  			host->devtype_data->send_cmd(host, instr->ctx.cmd.opcode, true);
> >  
> > @@ -1506,6 +1558,8 @@ static int mxcnd_exec_op(struct nand_chip *chip,
> >  			if (instr->ctx.cmd.opcode == NAND_CMD_STATUS)
> >  				statusreq = true;
> >  
> > +			host->column = 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?).

I don't think it's possible to separate introducing the host->column
variable to a separate patch, because this variable is only used with
NAND_ECC_ENGINE_TYPE_SOFT and on the other hand NAND_ECC_ENGINE_TYPE_SOFT
only works with this variable.

Anyway, adding a description what this is all about is a good idea and
I'll do this. In fact I already tried to add one when I found out that
it's really hard to put it into words, so I sent without it ;)

Sascha


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