From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Tudor Ambarus <Tudor.Ambarus@microchip.com>,
<linux-mtd@lists.infradead.org>, Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>, <devicetree@vger.kernel.org>
Subject: Re: [RESEND v5 03/21] mtd: rawnand: Drop OOB_FIRST placement scheme
Date: Wed, 27 May 2020 00:46:14 +0200 [thread overview]
Message-ID: <20200527004614.126ddc99@collabora.com> (raw)
In-Reply-To: <20200526195633.11543-4-miquel.raynal@bootlin.com>
On Tue, 26 May 2020 21:56:15 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> This scheme has been introduced for the Davinci controller and means
> that the OOB area must be read *before* the rest of the data. This has
> nothing to do with the ECC in OOB placement as it could be understood
> and most importantly, there is no point in having this function out of
> the Davinci NAND controller driver. A DT property for this scheme has
> been added but never used, even by the Davinci driver which only uses
> this scheme to change the default nand_read_page().
>
> Move the main read_page() helper into the Davinci driver and remove
> the remaining boilerplate.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> drivers/mtd/nand/raw/davinci_nand.c | 126 +++++++++++++++++++++-------
> drivers/mtd/nand/raw/nand_base.c | 81 ------------------
> include/linux/mtd/rawnand.h | 1 -
> 3 files changed, 98 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/davinci_nand.c b/drivers/mtd/nand/raw/davinci_nand.c
> index 52b87304954b..d975a62caaa5 100644
> --- a/drivers/mtd/nand/raw/davinci_nand.c
> +++ b/drivers/mtd/nand/raw/davinci_nand.c
> @@ -371,6 +371,77 @@ static int nand_davinci_correct_4bit(struct nand_chip *chip, u_char *data,
> return corrected;
> }
>
> +/**
> + * nand_read_page_hwecc_oob_first - hw ecc, read oob first
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + *
> + * Hardware ECC for large page chips, require OOB to be read first. For this
> + * ECC mode, the write_page method is re-used from ECC_HW. These methods
> + * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
> + * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
> + * the data area, by overwriting the NAND manufacturer bad block markings.
> + */
> +static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip *chip,
> + uint8_t *buf,
> + int oob_required, int page)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + int i, eccsize = chip->ecc.size, ret;
> + int eccbytes = chip->ecc.bytes;
> + int eccsteps = chip->ecc.steps;
> + uint8_t *p = buf;
> + uint8_t *ecc_code = chip->ecc.code_buf;
> + uint8_t *ecc_calc = chip->ecc.calc_buf;
> + unsigned int max_bitflips = 0;
> +
> + /* Read the OOB area first */
> + ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
> + if (ret)
> + return ret;
> +
> + ret = nand_read_page_op(chip, page, 0, NULL, 0);
> + if (ret)
> + return ret;
> +
> + ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> + chip->ecc.total);
> + if (ret)
> + return ret;
> +
> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> + int stat;
> +
> + chip->ecc.hwctl(chip, NAND_ECC_READ);
> +
> + ret = nand_read_data_op(chip, p, eccsize, false, false);
> + if (ret)
> + return ret;
> +
> + chip->ecc.calculate(chip, p, &ecc_calc[i]);
> +
> + stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> + if (stat == -EBADMSG &&
> + (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> + /* check for empty pages with bitflips */
> + stat = nand_check_erased_ecc_chunk(p, eccsize,
> + &ecc_code[i],
> + eccbytes, NULL, 0,
> + chip->ecc.strength);
> + }
> +
> + if (stat < 0) {
> + mtd->ecc_stats.failed++;
> + } else {
> + mtd->ecc_stats.corrected += stat;
> + max_bitflips = max_t(unsigned int, max_bitflips, stat);
> + }
> + }
> + return max_bitflips;
> +}
> +
> /*----------------------------------------------------------------------*/
>
> /* An ECC layout for using 4-bit ECC with small-page flash, storing
> @@ -530,6 +601,13 @@ static int davinci_nand_attach_chip(struct nand_chip *chip)
> break;
> case NAND_ECC_HW:
> if (pdata->ecc_bits == 4) {
> + int chunks = mtd->writesize / 512;
> +
> + if (!chunks || mtd->oobsize < 16) {
> + dev_dbg(&info->pdev->dev, "too small\n");
> + return -EINVAL;
> + }
> +
> /*
> * No sanity checks: CPUs must support this,
> * and the chips may not use NAND_BUSWIDTH_16.
> @@ -552,6 +630,26 @@ static int davinci_nand_attach_chip(struct nand_chip *chip)
> info->chip.ecc.bytes = 10;
> info->chip.ecc.options = NAND_ECC_GENERIC_ERASED_CHECK;
> info->chip.ecc.algo = NAND_ECC_BCH;
> +
> + /*
> + * Update ECC layout if needed ... for 1-bit HW ECC, the
> + * default is OK, but it allocates 6 bytes when only 3
> + * are needed (for each 512 bytes). For 4-bit HW ECC,
> + * the default is not usable: 10 bytes needed, not 6.
> + *
> + * For small page chips, preserve the manufacturer's
> + * badblock marking data ... and make sure a flash BBT
> + * table marker fits in the free bytes.
> + */
> + if (chunks == 1) {
> + mtd_set_ooblayout(mtd,
> + &hwecc4_small_ooblayout_ops);
> + } else if (chunks == 4 || chunks == 8) {
> + mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> + info->chip.ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
> + } else {
> + return -EIO;
> + }
> } else {
> /* 1bit ecc hamming */
> info->chip.ecc.calculate = nand_davinci_calculate_1bit;
> @@ -567,34 +665,6 @@ static int davinci_nand_attach_chip(struct nand_chip *chip)
> return -EINVAL;
> }
>
> - /*
> - * Update ECC layout if needed ... for 1-bit HW ECC, the default
> - * is OK, but it allocates 6 bytes when only 3 are needed (for
> - * each 512 bytes). For the 4-bit HW ECC, that default is not
> - * usable: 10 bytes are needed, not 6.
> - */
> - if (pdata->ecc_bits == 4) {
> - int chunks = mtd->writesize / 512;
> -
> - if (!chunks || mtd->oobsize < 16) {
> - dev_dbg(&info->pdev->dev, "too small\n");
> - return -EINVAL;
> - }
> -
> - /* For small page chips, preserve the manufacturer's
> - * badblock marking data ... and make sure a flash BBT
> - * table marker fits in the free bytes.
> - */
> - if (chunks == 1) {
> - mtd_set_ooblayout(mtd, &hwecc4_small_ooblayout_ops);
> - } else if (chunks == 4 || chunks == 8) {
> - mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> - info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST;
> - } else {
> - return -EIO;
> - }
> - }
> -
> return ret;
> }
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index e2cdfbb45923..7176e513a0bb 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3075,76 +3075,6 @@ static int nand_read_page_hwecc(struct nand_chip *chip, uint8_t *buf,
> return max_bitflips;
> }
>
> -/**
> - * nand_read_page_hwecc_oob_first - [REPLACEABLE] hw ecc, read oob first
> - * @chip: nand chip info structure
> - * @buf: buffer to store read data
> - * @oob_required: caller requires OOB data read to chip->oob_poi
> - * @page: page number to read
> - *
> - * Hardware ECC for large page chips, require OOB to be read first. For this
> - * ECC mode, the write_page method is re-used from ECC_HW. These methods
> - * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME support with
> - * multiple ECC steps, follows the "infix ECC" scheme and reads/writes ECC from
> - * the data area, by overwriting the NAND manufacturer bad block markings.
> - */
> -static int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t *buf,
> - int oob_required, int page)
> -{
> - struct mtd_info *mtd = nand_to_mtd(chip);
> - int i, eccsize = chip->ecc.size, ret;
> - int eccbytes = chip->ecc.bytes;
> - int eccsteps = chip->ecc.steps;
> - uint8_t *p = buf;
> - uint8_t *ecc_code = chip->ecc.code_buf;
> - uint8_t *ecc_calc = chip->ecc.calc_buf;
> - unsigned int max_bitflips = 0;
> -
> - /* Read the OOB area first */
> - ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
> - if (ret)
> - return ret;
> -
> - ret = nand_read_page_op(chip, page, 0, NULL, 0);
> - if (ret)
> - return ret;
> -
> - ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> - chip->ecc.total);
> - if (ret)
> - return ret;
> -
> - for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> - int stat;
> -
> - chip->ecc.hwctl(chip, NAND_ECC_READ);
> -
> - ret = nand_read_data_op(chip, p, eccsize, false, false);
> - if (ret)
> - return ret;
> -
> - chip->ecc.calculate(chip, p, &ecc_calc[i]);
> -
> - stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> - if (stat == -EBADMSG &&
> - (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> - /* check for empty pages with bitflips */
> - stat = nand_check_erased_ecc_chunk(p, eccsize,
> - &ecc_code[i], eccbytes,
> - NULL, 0,
> - chip->ecc.strength);
> - }
> -
> - if (stat < 0) {
> - mtd->ecc_stats.failed++;
> - } else {
> - mtd->ecc_stats.corrected += stat;
> - max_bitflips = max_t(unsigned int, max_bitflips, stat);
> - }
> - }
> - return max_bitflips;
> -}
> -
> /**
> * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome based page read
> * @chip: nand chip info structure
> @@ -5085,7 +5015,6 @@ static const char * const nand_ecc_modes[] = {
> [NAND_ECC_SOFT] = "soft",
> [NAND_ECC_HW] = "hw",
> [NAND_ECC_HW_SYNDROME] = "hw_syndrome",
> - [NAND_ECC_HW_OOB_FIRST] = "hw_oob_first",
> [NAND_ECC_ON_DIE] = "on-die",
> };
>
> @@ -5834,16 +5763,6 @@ static int nand_scan_tail(struct nand_chip *chip)
> */
>
> switch (ecc->mode) {
> - case NAND_ECC_HW_OOB_FIRST:
> - /* Similar to NAND_ECC_HW, but a separate read_page handle */
> - if (!ecc->calculate || !ecc->correct || !ecc->hwctl) {
> - WARN(1, "No ECC functions supplied; hardware ECC not possible\n");
> - ret = -EINVAL;
> - goto err_nand_manuf_cleanup;
> - }
> - if (!ecc->read_page)
> - ecc->read_page = nand_read_page_hwecc_oob_first;
> - fallthrough;
> case NAND_ECC_HW:
> /* Use standard hwecc read page function? */
> if (!ecc->read_page)
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 0911e845cced..7b87c5dc89bd 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -88,7 +88,6 @@ enum nand_ecc_mode {
> NAND_ECC_SOFT,
> NAND_ECC_HW,
> NAND_ECC_HW_SYNDROME,
> - NAND_ECC_HW_OOB_FIRST,
> NAND_ECC_ON_DIE,
> };
>
next prev parent reply other threads:[~2020-05-26 22:46 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-26 19:56 [RESEND v5 00/21] Introduce the generic ECC engine abstraction Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 01/21] mtd: Fix typo in mtd_ooblayout_set_databytes() description Miquel Raynal
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 02/21] mtd: rawnand: Avoid a typedef Miquel Raynal
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 03/21] mtd: rawnand: Drop OOB_FIRST placement scheme Miquel Raynal
2020-05-26 22:46 ` Boris Brezillon [this message]
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 04/21] dt-bindings: mtd: Deprecate OOB_FIRST mode Miquel Raynal
2020-05-26 22:42 ` Boris Brezillon
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 05/21] mtd: rawnand: Return an enum from of_get_nand_ecc_algo() Miquel Raynal
2020-05-26 22:41 ` Boris Brezillon
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 06/21] mtd: rawnand: Add an invalid ECC mode to discriminate with valid ones Miquel Raynal
2020-05-27 14:11 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 07/21] mtd: rawnand: Create a new enumeration to describe OOB placement Miquel Raynal
2020-05-26 22:39 ` Boris Brezillon
2020-05-27 8:00 ` Miquel Raynal
2020-05-27 8:22 ` Boris Brezillon
2020-05-26 19:56 ` [RESEND v5 08/21] mtd: rawnand: Separate the ECC engine type and the " Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 09/21] mtd: rawnand: Create a new enumeration to describe properly ECC types Miquel Raynal
2020-05-26 22:55 ` Boris Brezillon
2020-05-26 23:21 ` Boris Brezillon
2020-05-27 8:21 ` Miquel Raynal
2020-05-26 22:59 ` Boris Brezillon
2020-05-27 8:33 ` Miquel Raynal
2020-05-27 8:44 ` Boris Brezillon
2020-05-27 8:50 ` Boris Brezillon
2020-05-26 19:56 ` [RESEND v5 10/21] mtd: rawnand: Create a helper to retrieve the ECC placement Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 11/21] mtd: rawnand: Use the new ECC engine type enumeration Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 12/21] mtd: rawnand: Deprecate nand-ecc-mode in favor of nand-ecc-provider Miquel Raynal
2020-05-26 23:03 ` Boris Brezillon
2020-05-27 8:42 ` Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 13/21] mtd: rawnand: Drop the legacy ECC type enumeration Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 14/21] dt-bindings: mtd: Add the nand-ecc-placement property Miquel Raynal
2020-05-26 23:08 ` Boris Brezillon
2020-05-26 23:10 ` Rob Herring
2020-05-26 19:56 ` [RESEND v5 15/21] dt-bindings: mtd: Deprecate hw_syndrome from the ECC modes Miquel Raynal
2020-05-26 23:09 ` Boris Brezillon
2020-05-26 19:56 ` [RESEND v5 16/21] dt-bindings: mtd: Deprecate the nand-ecc-mode property Miquel Raynal
2020-05-26 23:12 ` Boris Brezillon
2020-05-26 19:56 ` [RESEND v5 17/21] mtd: nand: Move nand_device forward declaration to the top Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 18/21] mtd: nand: Add an extra level in the Kconfig hierarchy Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 19/21] mtd: nand: Drop useless 'depends on' in Kconfig Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 20/21] mtd: nand: Add a NAND page I/O request type Miquel Raynal
2020-05-26 19:56 ` [RESEND v5 21/21] mtd: nand: Rename a core structure 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=20200527004614.126ddc99@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=Tudor.Ambarus@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).