Message-ID: <20170819114159.77076761@xps13>
Date:   Sat, 19 Aug 2017 11:41:59 +0200
From:   Miquel RAYNAL <miquel.raynal@...e-electrons.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     Richard Weinberger <richard@....at>,
        KOBAYASHI Yoshitake <yoshitake.kobayashi@...hiba.co.jp>,
        linux-kernel@...r.kernel.org, Marek Vasut <marek.vasut@...il.com>,
        linux-mtd@...ts.infradead.org,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH v3] MTD: NAND: add OOB layout without ECC bytes

Hello Boris,

On Fri, 18 Aug 2017 00:15:42 +0200
Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:

> +Yoshi
> 
> Hi Miquel,
> 
> Subject prefix should be "mtd: nand: " in lowercase.

I was not aware of that kind of rule, next time I'll be careful.

> 
> Le Wed, 16 Aug 2017 09:39:06 +0200,
> Miquel Raynal <miquel.raynal@...e-electrons.com> 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.

I will change:
-for specific chips that use on-die ECC feature, ie. the [...].
+for configurations that does set ECC_NONE. Specific chips that use
on-die ECC feature, ie. the [...], might also take advantage of it.

> 
> > 
> > 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.

I'll edit the commit message, what about:

The layout functions added by this patch intend to be used when ECC_NONE
is set, except it is only usable for cases that were not already falling
in the supported large/small page with ECC cases. Not breaking
existing drivers needs these functions to be set as a fallthrough when
no other layout matches and ECC_NONE is actually set.

> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@...e-electrons.com>
> > ---
> >  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.

I though the EXPORT_* was expected as it was below every layout
function. I'll remove both of the exports, but as you said, while
nobody use them, I won't add the prototypes in nand.h neither, right ?

> 
> > +
> >  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.

To be removed then.

> 
> > +			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;

Ok, I clearly misunderstood your previous explanation, sorry about
that. I'll use your version as is.

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


Thank you,
Miquèl

Powered by blists - more mailing lists