Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
To: Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	linux-mtd@lists.infradead.org
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH v13 17/20] mtd: rawnand: Use the ECC framework OOB layouts
Date: Sun, 20 Sep 2020 14:21:44 +0200
Message-ID: <3339465.iIbC2pHGDl@z50> (raw)
In-Reply-To: <20200827085208.16276-18-miquel.raynal@bootlin.com>

On Thursday, August 27, 2020 10:52:05 A.M. CEST Miquel Raynal wrote:
> No need to have our own in the raw NAND core.

Hi Miquel,

This patch introduced a regression for me, at least on ams-delta which sets 
nand_chip->ecc.engine_type to NAND_ECC_ENGINE_TYPE_SOFT and 
nand_chip.ecc->algo to NAND_ECC_ALGO_HAMMING.  No related kernel messages 
could be observed, but nanddump fails with "pread: Numerical result out of 
range" for me.

The ECC framework OOB layouts refer to nand_device->ecc.ctx.total which no 
driver nor ECC engine initializes so far, while the old layouts used to refer 
to nand_chip->ecc.total, still initialized by nand_scan_tail().  Is there any 
work in progress which will address the issue (initialization of 
nand_device->ecc.ctx.total at least)?

Thanks,
Janusz

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/arasan-nand-controller.c |   2 +-
>  drivers/mtd/nand/raw/atmel/nand-controller.c  |   2 +-
>  drivers/mtd/nand/raw/davinci_nand.c           |   3 +-
>  .../mtd/nand/raw/ingenic/ingenic_nand_drv.c   |   6 +-
>  drivers/mtd/nand/raw/nand_base.c              | 171 +-----------------
>  drivers/mtd/nand/raw/nand_toshiba.c           |   2 +-
>  drivers/mtd/nand/raw/vf610_nfc.c              |   2 +-
>  include/linux/mtd/rawnand.h                   |   4 +-
>  8 files changed, 16 insertions(+), 176 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/arasan-nand-controller.c b/drivers/mtd/
nand/raw/arasan-nand-controller.c
> index 006bc3d3ce20..fbb4ea751be8 100644
> --- a/drivers/mtd/nand/raw/arasan-nand-controller.c
> +++ b/drivers/mtd/nand/raw/arasan-nand-controller.c
> @@ -980,7 +980,7 @@ static int anfc_init_hw_ecc_controller(struct arasan_nfc 
*nfc,
>  		return -EINVAL;
>  	}
>  
> -	mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +	mtd_set_ooblayout(mtd, nand_get_large_page_ooblayout());
>  
>  	ecc->steps = mtd->writesize / ecc->size;
>  	ecc->algo = NAND_ECC_ALGO_BCH;
> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/
raw/atmel/nand-controller.c
> index 3d162fdc9ce2..7fcc5e67163e 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> @@ -1108,7 +1108,7 @@ static int atmel_nand_pmecc_init(struct nand_chip 
*chip)
>  
>  	chip->options |= NAND_NO_SUBPAGE_WRITE;
>  
> -	mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +	mtd_set_ooblayout(mtd, nand_get_large_page_ooblayout());
>  
>  	return 0;
>  }
> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/
davinci_nand.c
> index 58966a9706b1..427f320fb79b 100644
> --- a/drivers/mtd/nand/raw/davinci_nand.c
> +++ b/drivers/mtd/nand/raw/davinci_nand.c
> @@ -645,7 +645,8 @@ static int davinci_nand_attach_chip(struct nand_chip 
*chip)
>  				mtd_set_ooblayout(mtd,
>  						  
&hwecc4_small_ooblayout_ops);
>  			} else if (chunks == 4 || chunks == 8) {
> -				mtd_set_ooblayout(mtd, 
&nand_ooblayout_lp_ops);
> +				mtd_set_ooblayout(mtd,
> +						  
nand_get_large_page_ooblayout());
>  				info->chip.ecc.read_page = 
nand_davinci_read_page_hwecc_oob_first;
>  			} else {
>  				return -EIO;
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/
nand/raw/ingenic/ingenic_nand_drv.c
> index 70309f18124c..0e9d426fe4f2 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -243,8 +243,10 @@ static int ingenic_nand_attach_chip(struct nand_chip 
*chip)
>  	/* For legacy reasons we use a different layout on the qi,lb60 
board. */
>  	if (of_machine_is_compatible("qi,lb60"))
>  		mtd_set_ooblayout(mtd, &qi_lb60_ooblayout_ops);
> -	else
> +	else if (nfc->soc_info->oob_layout)
>  		mtd_set_ooblayout(mtd, nfc->soc_info->oob_layout);
> +	else
> +		mtd_set_ooblayout(mtd, nand_get_large_page_ooblayout());
>  
>  	return 0;
>  }
> @@ -532,7 +534,6 @@ static const struct jz_soc_info jz4740_soc_info = {
>  	.data_offset = 0x00000000,
>  	.cmd_offset = 0x00008000,
>  	.addr_offset = 0x00010000,
> -	.oob_layout = &nand_ooblayout_lp_ops,
>  };
>  
>  static const struct jz_soc_info jz4725b_soc_info = {
> @@ -546,7 +547,6 @@ static const struct jz_soc_info jz4780_soc_info = {
>  	.data_offset = 0x00000000,
>  	.cmd_offset = 0x00400000,
>  	.addr_offset = 0x00800000,
> -	.oob_layout = &nand_ooblayout_lp_ops,
>  };
>  
>  static const struct of_device_id ingenic_nand_dt_match[] = {
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/
nand_base.c
> index b4ea831ecd85..77f60849dfb4 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -46,166 +46,6 @@
>  
>  #include "internals.h"
>  
> -/* Define default oob placement schemes for large and small page devices */
> -static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section,
> -				 struct mtd_oob_region 
*oobregion)
> -{
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
> -
> -	if (section > 1)
> -		return -ERANGE;
> -
> -	if (!section) {
> -		oobregion->offset = 0;
> -		if (mtd->oobsize == 16)
> -			oobregion->length = 4;
> -		else
> -			oobregion->length = 3;
> -	} else {
> -		if (mtd->oobsize == 8)
> -			return -ERANGE;
> -
> -		oobregion->offset = 6;
> -		oobregion->length = ecc->total - 4;
> -	}
> -
> -	return 0;
> -}
> -
> -static int nand_ooblayout_free_sp(struct mtd_info *mtd, int section,
> -				  struct mtd_oob_region 
*oobregion)
> -{
> -	if (section > 1)
> -		return -ERANGE;
> -
> -	if (mtd->oobsize == 16) {
> -		if (section)
> -			return -ERANGE;
> -
> -		oobregion->length = 8;
> -		oobregion->offset = 8;
> -	} else {
> -		oobregion->length = 2;
> -		if (!section)
> -			oobregion->offset = 3;
> -		else
> -			oobregion->offset = 6;
> -	}
> -
> -	return 0;
> -}
> -
> -const struct mtd_ooblayout_ops nand_ooblayout_sp_ops = {
> -	.ecc = nand_ooblayout_ecc_sp,
> -	.free = nand_ooblayout_free_sp,
> -};
> -EXPORT_SYMBOL_GPL(nand_ooblayout_sp_ops);
> -
> -static int nand_ooblayout_ecc_lp(struct mtd_info *mtd, int section,
> -				 struct mtd_oob_region 
*oobregion)
> -{
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
> -
> -	if (section || !ecc->total)
> -		return -ERANGE;
> -
> -	oobregion->length = ecc->total;
> -	oobregion->offset = mtd->oobsize - oobregion->length;
> -
> -	return 0;
> -}
> -
> -static int nand_ooblayout_free_lp(struct mtd_info *mtd, int section,
> -				  struct mtd_oob_region 
*oobregion)
> -{
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
> -
> -	if (section)
> -		return -ERANGE;
> -
> -	oobregion->length = mtd->oobsize - ecc->total - 2;
> -	oobregion->offset = 2;
> -
> -	return 0;
> -}
> -
> -const struct mtd_ooblayout_ops nand_ooblayout_lp_ops = {
> -	.ecc = nand_ooblayout_ecc_lp,
> -	.free = nand_ooblayout_free_lp,
> -};
> -EXPORT_SYMBOL_GPL(nand_ooblayout_lp_ops);
> -
> -/*
> - * Support the old "large page" layout used for 1-bit Hamming ECC where ECC
> - * are placed at a fixed offset.
> - */
> -static int nand_ooblayout_ecc_lp_hamming(struct mtd_info *mtd, int section,
> -					 struct mtd_oob_region 
*oobregion)
> -{
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
> -
> -	if (section)
> -		return -ERANGE;
> -
> -	switch (mtd->oobsize) {
> -	case 64:
> -		oobregion->offset = 40;
> -		break;
> -	case 128:
> -		oobregion->offset = 80;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	oobregion->length = ecc->total;
> -	if (oobregion->offset + oobregion->length > mtd->oobsize)
> -		return -ERANGE;
> -
> -	return 0;
> -}
> -
> -static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int 
section,
> -					  struct 
mtd_oob_region *oobregion)
> -{
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> -	struct nand_ecc_ctrl *ecc = &chip->ecc;
> -	int ecc_offset = 0;
> -
> -	if (section < 0 || section > 1)
> -		return -ERANGE;
> -
> -	switch (mtd->oobsize) {
> -	case 64:
> -		ecc_offset = 40;
> -		break;
> -	case 128:
> -		ecc_offset = 80;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	if (section == 0) {
> -		oobregion->offset = 2;
> -		oobregion->length = ecc_offset - 2;
> -	} else {
> -		oobregion->offset = ecc_offset + ecc->total;
> -		oobregion->length = mtd->oobsize - oobregion->offset;
> -	}
> -
> -	return 0;
> -}
> -
> -static const struct mtd_ooblayout_ops nand_ooblayout_lp_hamming_ops = {
> -	.ecc = nand_ooblayout_ecc_lp_hamming,
> -	.free = nand_ooblayout_free_lp_hamming,
> -};
> -
>  static int nand_pairing_dist3_get_info(struct mtd_info *mtd, int page,
>  				       struct mtd_pairing_info 
*info)
>  {
> @@ -5377,7 +5217,7 @@ static int nand_set_ecc_soft_ops(struct nand_chip 
*chip)
>  				return -EINVAL;
>  			}
>  
> -			mtd_set_ooblayout(mtd, 
&nand_ooblayout_lp_ops);
> +			mtd_set_ooblayout(mtd, 
nand_get_large_page_ooblayout());
>  
>  		}
>  
> @@ -5386,7 +5226,7 @@ static int nand_set_ecc_soft_ops(struct nand_chip 
*chip)
>  		 * used, otherwise we don't know how many bytes can 
really be
>  		 * used.
>  		 */
> -		if (mtd->ooblayout == &nand_ooblayout_lp_ops &&
> +		if (mtd->ooblayout == nand_get_large_page_ooblayout() &&
>  		    ecc->options & NAND_ECC_MAXIMIZE) {
>  			int steps, bytes;
>  
> @@ -5788,11 +5628,12 @@ static int nand_scan_tail(struct nand_chip *chip)
>  		switch (mtd->oobsize) {
>  		case 8:
>  		case 16:
> -			mtd_set_ooblayout(mtd, 
&nand_ooblayout_sp_ops);
> +			mtd_set_ooblayout(mtd, 
nand_get_small_page_ooblayout());
>  			break;
>  		case 64:
>  		case 128:
> -			mtd_set_ooblayout(mtd, 
&nand_ooblayout_lp_hamming_ops);
> +			mtd_set_ooblayout(mtd,
> +					  
nand_get_large_page_hamming_ooblayout());
>  			break;
>  		default:
>  			/*
> @@ -5804,7 +5645,7 @@ static int nand_scan_tail(struct nand_chip *chip)
>  			 */
>  			if (ecc->engine_type == 
NAND_ECC_ENGINE_TYPE_NONE) {
>  				mtd_set_ooblayout(mtd,
> -						
&nand_ooblayout_lp_ops);
> +						  
nand_get_large_page_ooblayout());
>  				break;
>  			}
>  
> diff --git a/drivers/mtd/nand/raw/nand_toshiba.c b/drivers/mtd/nand/raw/
nand_toshiba.c
> index fb6662337b90..cf4f37959421 100644
> --- a/drivers/mtd/nand/raw/nand_toshiba.c
> +++ b/drivers/mtd/nand/raw/nand_toshiba.c
> @@ -140,7 +140,7 @@ static void toshiba_nand_benand_init(struct nand_chip 
*chip)
>  
>  	chip->options |= NAND_SUBPAGE_READ;
>  
> -	mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +	mtd_set_ooblayout(mtd, nand_get_large_page_ooblayout());
>  }
>  
>  static void toshiba_nand_decode_id(struct nand_chip *chip)
> diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/
vf610_nfc.c
> index 8ee2c1f539c4..50dc0c93140c 100644
> --- a/drivers/mtd/nand/raw/vf610_nfc.c
> +++ b/drivers/mtd/nand/raw/vf610_nfc.c
> @@ -779,7 +779,7 @@ static int vf610_nfc_attach_chip(struct nand_chip *chip)
>  		mtd->oobsize = 64;
>  
>  	/* Use default large page ECC layout defined in NAND core */
> -	mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> +	mtd_set_ooblayout(mtd, nand_get_large_page_ooblayout());
>  	if (chip->ecc.strength == 32) {
>  		nfc->ecc_mode = ECC_60_BYTE;
>  		chip->ecc.bytes = 60;
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 967b616c50df..1bbce6fa1b08 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -14,6 +14,7 @@
>  #define __LINUX_MTD_RAWNAND_H
>  
>  #include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
>  #include <linux/mtd/flashchip.h>
>  #include <linux/mtd/bbm.h>
>  #include <linux/mtd/jedec.h>
> @@ -1156,9 +1157,6 @@ struct nand_chip {
>  	void *priv;
>  };
>  
> -extern const struct mtd_ooblayout_ops nand_ooblayout_sp_ops;
> -extern const struct mtd_ooblayout_ops nand_ooblayout_lp_ops;
> -
>  static inline struct nand_chip *mtd_to_nand(struct mtd_info *mtd)
>  {
>  	return container_of(mtd, struct nand_chip, base.mtd);
> 





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

  parent reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27  8:51 [PATCH v13 00/20] Introduction of the generic ECC framework Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 01/20] mtd: rawnand: Add a kernel doc to the ECC algorithm enumeration Miquel Raynal
2020-09-07  7:25   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 02/20] mtd: rawnand: Rename the ECC algorithm enumeration items Miquel Raynal
2020-09-07  7:25   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 03/20] mtd: rawnand: Move the nand_ecc_algo enum to the generic NAND layer Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 04/20] mtd: nand: Add a NAND page I/O request type Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 05/20] dt-bindings: mtd: Document nand-ecc-placement Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 06/20] dt-bindings: mtd: Document nand-ecc-engine Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 07/20] dt-bindings: mtd: Document boolean NAND ECC properties Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 08/20] mtd: nand: Introduce the ECC engine framework Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 09/20] mtd: rawnand: Separate the ECC engine type and the ECC byte placement Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-10-14 21:36   ` Han Xu
2020-10-15  1:35     ` Fabio Estevam
2020-10-15  3:09       ` Han Xu
2020-10-15  7:25         ` Miquel Raynal
2020-10-16 17:09           ` Han Xu
2020-10-16 17:14             ` Miquel Raynal
2020-10-16 17:51               ` Han Xu
2020-10-16 18:00                 ` Miquel Raynal
2020-10-16 21:03                 ` Fabio Estevam
2020-10-19 15:51                   ` Han Xu
2020-10-19 16:52                     ` Fabio Estevam
2020-08-27  8:51 ` [PATCH v13 10/20] mtd: rawnand: Use the new ECC engine type enumeration Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:51 ` [PATCH v13 11/20] mtd: nand: Create a helper to extract the ECC configuration Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 12/20] mtd: spinand: Use nanddev_get_ecc_conf() when relevant Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 13/20] mtd: nand: Create helpers to set/extract the ECC requirements Miquel Raynal
2020-09-07  7:24   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 14/20] mtd: rawnand: Use nanddev_get/set_ecc_requirements() when relevant Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 15/20] mtd: nand: Use the new generic ECC object Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 16/20] mtd: rawnand: Make use of the ECC framework Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 17/20] mtd: rawnand: Use the ECC framework OOB layouts Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-09-20 12:21   ` Janusz Krzysztofik [this message]
2020-09-29  8:15     ` Miquel Raynal
2020-09-29 21:26       ` Janusz Krzysztofik
2020-08-27  8:52 ` [PATCH v13 18/20] mtd: rawnand: Use the ECC framework nand_ecc_is_strong_enough() helper Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 19/20] mtd: rawnand: Use the ECC framework user input parsing bits Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal
2020-09-21  9:36   ` Sascha Hauer
2020-09-28 15:18     ` Miquel Raynal
2020-08-27  8:52 ` [PATCH v13 20/20] mtd: rawnand: Use the NAND framework user_conf object for ECC flags Miquel Raynal
2020-09-07  7:23   ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3339465.iIbC2pHGDl@z50 \
    --to=jmkrzyszt@gmail.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=richard@nod.at \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git