[PATCH -next] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)
Boris Brezillon
boris.brezillon at free-electrons.com
Fri May 26 09:22:54 PDT 2017
Hi KOBAYASHI,
Le Fri, 26 May 2017 10:15:35 +0900,
KOBAYASHI Yoshitake <yoshitake.kobayashi at toshiba.co.jp> a écrit :
> This patch enables support for Toshiba BENAND. It was originally posted on
> https://lkml.org/lkml/2015/7/24/571
>
> This patch is rewrote to adapt the recent mtd "separate vendor specific code
> from core" cleanup process.
>
> Signed-off-by: KOBAYASHI Yoshitake <yoshitake.kobayashi at toshiba.co.jp>
> ---
> drivers/mtd/nand/Kconfig | 17 ++++++
> drivers/mtd/nand/nand_base.c | 15 ++++++
> drivers/mtd/nand/nand_toshiba.c | 112 +++++++++++++++++++++++++++++++++++++++-
> include/linux/mtd/nand.h | 1 +
> 4 files changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 0bd2319..6634d4b 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -36,6 +36,23 @@ config MTD_NAND_ECC_BCH
> ECC codes. They are used with NAND devices requiring more than 1 bit
> of error correction.
>
> +config MTD_NAND_BENAND
> + bool "Support for Toshiba BENAND (Built-in ECC NAND)"
> + default n
> + help
> + This enables support for Toshiba BENAND.
> + Toshiba BENAND is a SLC NAND solution that automatically
> + generates ECC inside NAND chip.
> +
> +config MTD_NAND_BENAND_ECC_STATUS
> + bool "Enable ECC Status Read Command(0x7A)"
> + depends on MTD_NAND_BENAND
> + default n
> + help
> + This enables support for ECC Status Read Command(0x7A) of BENAND.
> + When this enables, report the real number of bitflips.
> + In other cases, report the assumud number.
> +
Please drop the Kconfig options. The amount of code added here is quite
small, and I don't think we need to compile it out. If the vendor code
continues to grow we'll see how we want to deal with that, but we're
not there yet.
> config MTD_SM_COMMON
> tristate
> default n
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 43722a8..ab8652e 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4186,6 +4186,7 @@ static const char * const nand_ecc_modes[] = {
> [NAND_ECC_HW_SYNDROME] = "hw_syndrome",
> [NAND_ECC_HW_OOB_FIRST] = "hw_oob_first",
> [NAND_ECC_ON_DIE] = "on-die",
> + [NAND_ECC_BENAND] = "benand",
No. BENAND and on-die ECC are the same thing (not the same
implementation, but the same feature). Please re-use the existing
ECC_ON_DIE definition.
> };
>
> static int of_get_nand_ecc_mode(struct device_node *np)
> @@ -4751,6 +4752,19 @@ int nand_scan_tail(struct mtd_info *mtd)
> ecc->write_oob = nand_write_oob_std;
> break;
>
> + case NAND_ECC_BENAND:
> + if (!ecc->read_page || !ecc->read_subpage) {
> + WARN(1, "No ECC functions supplied; benand ECC not possible\n");
> + ret = -EINVAL;
> + goto err_free;
> + }
> + ecc->write_page = nand_write_page_raw;
> + ecc->read_page_raw = nand_read_page_raw;
> + ecc->write_page_raw = nand_write_page_raw;
> + ecc->read_oob = nand_read_oob_std;
> + ecc->write_oob = nand_write_oob_std;
> + break;
Ditto.
> +
> case NAND_ECC_NONE:
> pr_warn("NAND_ECC_NONE selected by board driver. This is not recommended!\n");
> ecc->read_page = nand_read_page_raw;
> @@ -4831,6 +4845,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> /* Large page NAND with SOFT_ECC should support subpage reads */
> switch (ecc->mode) {
> case NAND_ECC_SOFT:
> + case NAND_ECC_BENAND:
And here as well.
> if (chip->page_shift > 9)
> chip->options |= NAND_SUBPAGE_READ;
> break;
> diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c
> index fa787ba..bb3c852 100644
> --- a/drivers/mtd/nand/nand_toshiba.c
> +++ b/drivers/mtd/nand/nand_toshiba.c
> @@ -17,6 +17,97 @@
>
> #include <linux/mtd/nand.h>
>
> +/* ECC Status Read Command for BENAND */
> +#define NAND_CMD_ECC_STATUS 0x7A
Can you prefix the name with TOSHIBA_:
#define TOSHIBA_NAND_CMD_ECC_STATUS 0x7a
> +
> +/* Recommended to rewrite for BENAND */
> +#define NAND_STATUS_RECOM_REWRT 0x08
Ditto, add a TOSHIBA somewhere:
#define TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED BIT(3)
But anyway, I'm not sure we want to use this metric since we have the
number of corrected bitflips thanks to the TOSHIBA_NAND_CMD_ECC_STATUS
command.
> +
> +
Drop the extra empty line.
> +static int toshiba_nand_benand_status_chk(struct mtd_info *mtd,
> + struct nand_chip *chip)
Try to align parameters on the open parenthesis when you have multiple
lines.
> +{
> + unsigned int max_bitflips = 0;
> + u8 status;
> +
> + /* Check Read Status */
> + chip->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> + status = chip->read_byte(mtd);
> +
> + /* timeout */
> + if (!(status & NAND_STATUS_READY)) {
> + pr_debug("BENAND : Time Out!\n");
> + return -EIO;
> + }
Hm, I think this one has already been checked by the core.
> +
> + /* uncorrectable */
> + else if (status & NAND_STATUS_FAIL)
> + mtd->ecc_stats.failed++;
You have all the information to add the exact number of failures (it's
a per-sector information, not per-page).
> +
> + /* correctable */
> + else if (status & NAND_STATUS_RECOM_REWRT) {
> + if (chip->cmd_ctrl &&
> + IS_ENABLED(CONFIG_MTD_NAND_BENAND_ECC_STATUS)) {
> +
> + int i;
> + u8 ecc_status;
> + unsigned int bitflips;
> +
> + /* Check Read ECC Status */
> + chip->cmd_ctrl(mtd, NAND_CMD_ECC_STATUS,
> + NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
> + /* Get bitflips info per 512Byte */
> + for (i = 0; i < mtd->writesize >> 9; i++) {
> + ecc_status = chip->read_byte(mtd);
> + bitflips = ecc_status & 0x0f;
> + max_bitflips = max_t(unsigned int,
> + max_bitflips, bitflips);
> + }
> + mtd->ecc_stats.corrected += max_bitflips;
> + } else {
> + /*
> + * If can't use chip->cmd_ctrl,
> + * we can't get real number of bitflips.
> + * So, we set max_bitflips mtd->bitflip_threshold.
> + */
And that's exactly the kind of situation I want to avoid. I hate the
fact that, depending on the NAND controller, we have this feature
working or not. Well, if it's a real limitation of the
controller, that's acceptable, but most of the time it's just a driver
limitation.
I'd like to have the ->exec_op() [1] infrastructure ready before we
start adding vendor specific commands. It probably needs to be extended
with an ->supports_op() hook to ask the controller whether it supports a
specific operation or not and let the core/vendor driver decide whether
this part can be attached to the controller based on the result.
> + max_bitflips = mtd->bitflip_threshold;
> + mtd->ecc_stats.corrected += max_bitflips;
> + }
> + }
For the if () ... else if () ... blocks please try to do:
if (...) {
/* comment here */
...
} else if (...) {
/* comment here */
...
} else {
/* comment here */
...
}
> +
> + return max_bitflips;
> +}
> +
> +static int toshiba_nand_read_page_benand(struct mtd_info *mtd,
> + struct nand_chip *chip, uint8_t *buf,
> + int oob_required, int page)
> +{
> + unsigned int max_bitflips = 0;
> +
> + chip->ecc.read_page_raw(mtd, chip, buf, oob_required, page);
> + max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);
> +
> + return max_bitflips;
> +}
> +
> +static int toshiba_nand_read_subpage_benand(struct mtd_info *mtd,
> + struct nand_chip *chip, uint32_t data_offs,
> + uint32_t readlen, uint8_t *bufpoi, int page)
> +{
> + uint8_t *p;
> + unsigned int max_bitflips = 0;
> +
> + if (data_offs != 0)
> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, data_offs, -1);
> +
> + p = bufpoi + data_offs;
> + chip->read_buf(mtd, p, readlen);
> +
> + max_bitflips = toshiba_nand_benand_status_chk(mtd, chip);
> +
> + return max_bitflips;
> +}
> +
> static void toshiba_nand_decode_id(struct nand_chip *chip)
> {
> struct mtd_info *mtd = nand_to_mtd(chip);
> @@ -33,15 +124,32 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> */
> if (chip->id.len >= 6 && nand_is_slc(chip) &&
> (chip->id.data[5] & 0x7) == 0x6 /* 24nm */ &&
> - !(chip->id.data[4] & 0x80) /* !BENAND */)
> - mtd->oobsize = 32 * mtd->writesize >> 9;
> + (chip->id.data[4] & 0x80) /* BENAND */) {
> + if (IS_ENABLED(CONFIG_MTD_NAND_BENAND))
> + chip->ecc.mode = NAND_ECC_BENAND;
No, you should not set that explicitly. This choice should be left to
the user. Now, since the internal ECC engine cannot be disabled here,
you should fail in toshiba_nand_init() if you are dealing with a BENAND
and chip->ecc.mode != NAND_ECC_ON_DIE.
> + } else {
> + mtd->oobsize = 32 * mtd->writesize >> 9; /* !BENAND */
> + }
You're changing the ID decoding logic here.
It should be:
if (chip->id.len >= 6 && nand_is_slc(chip) &&
(chip->id.data[5] & 0x7) == 0x6 /* 24nm */) {
if (chip->id.data[4] & 0x80)
chip->ecc.mode = NAND_ECC_BENAND;
else
mtd->oobsize = 32 * mtd->writesize >> 9;
}
> }
>
> static int toshiba_nand_init(struct nand_chip *chip)
> {
> + struct mtd_info *mtd = nand_to_mtd(chip);
> +
> if (nand_is_slc(chip))
> chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>
> + if (chip->ecc.mode == NAND_ECC_BENAND) {
> + chip->ecc.options = NAND_ECC_CUSTOM_PAGE_ACCESS;
> + chip->ecc.bytes = 0;
It should be set to 16 according to the datasheet. But I guess this is
not exactly true. I'm pretty sure we can use some of these bytes to
store real data. Assuming you're using BCH, only 13bytes are needed for
8bits/512bytes strength, and I guess the BBM region is also left
untouched (first 2 bytes of the OOB region).
> + chip->ecc.strength = 8;
> + chip->ecc.total = 0;
No need to explicitly set that one, but you should set chip->ecc.size
to 512.
> + chip->ecc.read_page = toshiba_nand_read_page_benand;
> + chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
> +
> + mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> + }
Should be:
if (chip->id.len >= 6 && nand_is_slc(chip) &&
(chip->id.data[5] & 0x7) == 0x6 &&
(chip->id.data[4] & 0x80)) {
/* BENAND */
/*
* We can't disable the internal ECC engine, the user
* has to use on-die ECC, there is no alternative.
*/
if (chip->ecc.mode != NAND_ECC_ON_DIE)
return -EINVAL;
....
}
> +
> return 0;
> }
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 3a6a77f..6821334 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -117,6 +117,7 @@ typedef enum {
> NAND_ECC_HW_SYNDROME,
> NAND_ECC_HW_OOB_FIRST,
> NAND_ECC_ON_DIE,
> + NAND_ECC_BENAND,
As explained above: this is unneeded.
> } nand_ecc_modes_t;
>
> enum nand_ecc_algo {
[1]https://github.com/bbrezillon/linux/commits/nand/exec_op1
More information about the linux-mtd
mailing list