Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753906AbdHQWPq convert rfc822-to-8bit (ORCPT ); Thu, 17 Aug 2017 18:15:46 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:48997 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753326AbdHQWPp (ORCPT ); Thu, 17 Aug 2017 18:15:45 -0400 Date: Fri, 18 Aug 2017 00:15:42 +0200 From: Boris Brezillon To: Miquel Raynal Cc: Richard Weinberger , linux-mtd@lists.infradead.org, David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , linux-kernel@vger.kernel.org, KOBAYASHI Yoshitake Subject: Re: [PATCH v3] MTD: NAND: add OOB layout without ECC bytes Message-ID: <20170818001542.01d5aa95@bbrezillon> In-Reply-To: <20170816073906.5819-1-miquel.raynal@free-electrons.com> References: <20170816073906.5819-1-miquel.raynal@free-electrons.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5174 Lines: 163 +Yoshi Hi Miquel, Subject prefix should be "mtd: nand: " in lowercase. Le Wed, 16 Aug 2017 09:39:06 +0200, Miquel Raynal a écrit : > Add layout functions for small and large pages with mainly free bytes > plus reserved space for Bad Block Markers. This may be useful for > specific chip that use on-die ECC feature, ie. the TOSHIBA BENAND > (Built-in ECC NAND). The ECC bytes are stored in an isolated area > inaccessible by the driver and thus the OOB layout must only reserve > space for Bad Block Markers. Yes, that's true, but as said below, I expect the toshiba driver to explicitly set this layout. > > Bad Block Markers position was extracted from the existing OOB layouts > by assigning as free all the bytes marked as ECC. > > The layout functions added by this patch were supposed to be used with > ECC_NONE mode but it would break existing drivers that do not rely on > an implementation of this layout by the core, so set it as a fallthrough > when no other layout matches. Well, it's still supposed to be used when ECC_NONE is set, except we can only use it for cases that were not already falling in the supported large/small page with ECC cases. > > Signed-off-by: Miquel Raynal > --- > drivers/mtd/nand/nand_base.c | 65 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index c5221795a1e8..7c0ae674e50a 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -53,7 +53,38 @@ static int nand_get_device(struct mtd_info *mtd, int new_state); > static int nand_do_write_oob(struct mtd_info *mtd, loff_t to, > struct mtd_oob_ops *ops); > > -/* Define default oob placement schemes for large and small page devices */ > +/* > + * Define default OOB placement schemes for: > + * - no ECC or software ECC > + * - small or large page devices > + */ > +static int nand_ooblayout_free_sp_no_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + if (section > 1) > + return -ERANGE; > + > + if (!section) { > + if (mtd->oobsize == 16) { > + oobregion->offset = 0; > + oobregion->length = 4; > + } else { > + oobregion->offset = 0; > + oobregion->length = 5; > + } > + } else { > + oobregion->offset = 6; > + oobregion->length = mtd->oobsize - oobregion->offset; > + } > + > + return 0; > +} > + > +const struct mtd_ooblayout_ops nand_ooblayout_sp_no_ecc_ops = { > + .free = nand_ooblayout_free_sp_no_ecc, > +}; > +EXPORT_SYMBOL_GPL(nand_ooblayout_sp_no_ecc_ops); > + > static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section, > struct mtd_oob_region *oobregion) > { > @@ -109,6 +140,23 @@ const struct mtd_ooblayout_ops nand_ooblayout_sp_ops = { > }; > EXPORT_SYMBOL_GPL(nand_ooblayout_sp_ops); > > +static int nand_ooblayout_free_lp_no_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + if (section) > + return -ERANGE; > + > + oobregion->offset = 2; > + oobregion->length = mtd->oobsize - oobregion->offset; > + > + return 0; > +} > + > +const struct mtd_ooblayout_ops nand_ooblayout_lp_no_ecc_ops = { > + .free = nand_ooblayout_free_lp_no_ecc, > +}; > +EXPORT_SYMBOL_GPL(nand_ooblayout_lp_no_ecc_ops); No need to export those symbols until someone really needs them. BTW, they are exported but not declared in nand.h, so right now nobody can use them. > + > static int nand_ooblayout_ecc_lp(struct mtd_info *mtd, int section, > struct mtd_oob_region *oobregion) > { > @@ -4649,10 +4697,17 @@ int nand_scan_tail(struct mtd_info *mtd) > mtd_set_ooblayout(mtd, &nand_ooblayout_lp_hamming_ops); > break; > default: > - WARN(1, "No oob scheme defined for oobsize %d\n", > - mtd->oobsize); > - ret = -EINVAL; > - goto err_free; > + /* > + * In the case of on-die ECC (when ECC are stored in an > + * isolated area inaccessible by the driver), the OOB > + * layout must only reserve space for Bad Block Markers. > + */ This kind of on-die ECC is not yet supported and I'd expect NAND manufacturer drivers to set the appropriate layout before reaching this point, so that's not really relevant here. > + if (mtd->writesize <= 512) > + mtd_set_ooblayout(mtd, > + &nand_ooblayout_sp_no_ecc_ops); > + else > + mtd_set_ooblayout(mtd, > + &nand_ooblayout_lp_no_ecc_ops); That's not exactly what I was suggesting. What I had in mind was: if (ecc->mode == NAND_ECC_NONE) { /* * Expose the whole OOB area to users if ECC_NONE * is passed. We could do that for all kind of * ->oobsize, but we must keep the old * large/small page with ECC layout for * compatibility reasons. */ if (mtd->writesize <= 512) mtd_set_ooblayout(mtd, &nand_ooblayout_sp_no_ecc_ops); else mtd_set_ooblayout(mtd, &nand_ooblayout_lp_no_ecc_ops); break; } WARN(1, "No oob scheme defined for oobsize %d\n", mtd->oobsize); ret = -EINVAL; goto err_free; > } > } >