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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/