lkml.org 
[lkml]   [2017]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -next v2] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)
On Tue, 26 Sep 2017 18:18:04 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:

> On 2017/09/21 16:28, Boris Brezillon wrote:
> > On Thu, 21 Sep 2017 14:32:02 +0900
> > KOBAYASHI Yoshitake <yoshitake.kobayashi@toshiba.co.jp> wrote:
> >
> >> This patch enables support for Toshiba BENAND.
> >> The current implementation does not support vondor specific command
> >
> > ^ vendor
> >
> >> TOSHIBA_NAND_CMD_ECC_STATUS. I would like to add the command, when
> >> the exec_op() [1] infrastructure is implemented.
> >
> > It's not a good idea to reference a branch that is likely to disappear
> > in a commit message. Just say that you can't properly support the
> > TOSHIBA_NAND_CMD_ECC_STATUS operation right and that it might be
> > addressed in the future.
>
> Thanks. I'll change the comment.
>
> >> + */
> >> + if (status & NAND_STATUS_FAIL) {
> >> + /* uncorrectable */
> >> + mtd->ecc_stats.failed++;
> >> + } else if (status & TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED) {
> >> + /* correctable */
> >> + max_bitflips = mtd->bitflip_threshold;
> >> + mtd->ecc_stats.corrected += max_bitflips;
> >> + }
> >
> > Is this working correctly when you read more than one ECC chunk? The
> > ECC step size is 512 bytes and the page is bigger than that, which means
> > you have more than one ECC chunk per page. What happens to the
> > NAND_STATUS_FAIL flag if the first chunk is uncorrectable but
> > following ones are correctable (or do not contain bitflips at all)?
>
> As you mentioned, the ECC step size is 512 byte. But when 0x70 command is used
> a simplified ECC status per page is generated.

I'm fine with that as long as uncorrectable errors are detected
correctly and TOSHIBA_NAND_STATUS_REWRITE_RECOMMENDED is set as soon as
one of the ECC chunk exposes too many bitflips.

> In case the first chunk is uncorrectable but the following ones are correctable,
> the 0x70 command can only check the status of the uncorrectable one.

Sounds good.

> Each ECC chunk status can be checked by using the 0x7A command.
>
> >> @@ -39,9 +105,43 @@ static void toshiba_nand_decode_id(struct nand_chip *chip)
> >>
> >> 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 (nand_is_slc(chip) && (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) {
> >> + pr_err("On-die ECC should be selected.\n");
> >> + return -EINVAL;
> >> + }
> >
> > According to your previous explanation that's not exactly true. Since
> > ECC bytes are stored in a separate area, the user can decide to use
> > another mode without trouble. Just skip the BENAND initialization when
> > mode != NAND_ECC_ON_DIE and we should be good, or am I missing something?
>
> I am asking to product department to confirm it.

I'm almost sure this is the case ;-).

>
> >> +
> >> + /*
> >> + * On BENAND, all OOB reginon can be used by user (driver).
> >
> > the entire OOB region can be used by the
> > MTD user.
> >
> > I'd drop the '(driver)' part since it's not really clear what the
> > driver is. If you're talking about the NAND controller driver then it's
> > wrong (at least most of the time), the real users of free OOB bytes are
> > upper layers (like JFFS2).
> >
> >> + * The calculated ECC bytes are stored into other isolated
> >> + * area which is ubable to access from user.
> >
> > which is not accessible to users.
> >
> >> + * This is why chip->ecc.bytes = 0.
> >> + */
> >> + chip->ecc.bytes = 0;
> >> + chip->ecc.size = 512;
> >> + chip->ecc.strength = 8;
> >> + chip->ecc.read_page = toshiba_nand_read_page_benand;
> >> + chip->ecc.read_subpage = toshiba_nand_read_subpage_benand;
> >> + chip->ecc.write_page = nand_write_page_raw;
> >> + chip->ecc.read_page_raw = nand_read_page_raw;
> >> + chip->ecc.write_page_raw = nand_write_page_raw;
> >> +
> >> + chip->options |= NAND_SUBPAGE_READ;
> >> +
> >> + mtd_set_ooblayout(mtd, &nand_ooblayout_lp_ops);
> >
> > Can you please move this code block in a separate toshiba_benand_init()
> > function in order to keep the toshiba_nand_init() small/readable.
>
> Sure. I will resend.
>
> -- Yoshi
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

\
 
 \ /
  Last update: 2017-09-26 23:16    [W:0.287 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site