Re: [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <bbrezillon@kernel.org>
Cc: Mason Yang <masonccyang@mxic.com.tw>, Vignesh R <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Julien Su <juliensu@mxic.com.tw>,
	Richard Weinberger <richard@nod.at>,
	Schrempf Frieder <frieder.schrempf@kontron.de>,
	Marek Vasut <marek.vasut@gmail.com>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations
Date: Mon, 25 Feb 2019 17:01:18 +0100	[thread overview]
Message-ID: <20190225170118.62739fc4@xps13> (raw)
In-Reply-To: <20190222154431.66c6911f@kernel.org>

Hi Boris,

Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
15:44:31 +0100:

> On Thu, 21 Feb 2019 13:58:04 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Add the logic in the NAND core to find the right ECC engine depending
> > on the NAND chip requirements and the user desires. Right now, the
> > choice may be made between (more will come):
> > * software Hamming
> > * software BCH
> > * on-die (SPI-NAND devices only)
> > 
> > Once the ECC engine has been found, the ECC engine must be
> > configured.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/core.c  | 107 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/nand.h |   4 ++
> >  2 files changed, 111 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> > index 872d46b5fc0f..9feb118c9f68 100644
> > --- a/drivers/mtd/nand/core.c
> > +++ b/drivers/mtd/nand/core.c
> > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len)
> >  }
> >  EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks);
> >  
> > +/**
> > + * nanddev_find_ecc_engine() - Find a suitable ECC engine
> > + * @nand: NAND device
> > + */
> > +static int nanddev_find_ecc_engine(struct nand_device *nand)  
> 
> Can we pass the conf in argument instead of reading it from
> nand->ecc.user_conf?
> 
> > +{
> > +	bool is_spinand = mtd_type_is_spinand(&nand->mtd);  
> 
> And here is the reason for the SPINAND type.
> 
> > +
> > +	/* Read the user desires in terms of ECC engine/configuration */
> > +	nand_ecc_read_user_conf(nand);
> > +
> > +	/* No ECC engine requestedn, let's return without error */
> > +	if (nand->ecc.user_conf.mode == NAND_ECC_NONE)
> > +		return 0;
> > +
> > +	/* Raw NAND default mode is hardware */
> > +	if (!is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_HW;  
> 
> We should let the raw NAND layer take this decision (actually, it's
> even a raw NAND controller driver decision). Please complain if
> user_conf.mode is invalid.
> This way you won't need the SPINAND type you added in one of your
> previous patch.
> 
> > +
> > +	/* SPI-NAND default mode is on-die */
> > +	if (is_spinand && nand->ecc.user_conf.mode < 0)
> > +		nand->ecc.user_conf.mode = NAND_ECC_ON_DIE;
> > +
> > +	switch (nand->ecc.user_conf.mode) {
> > +	case NAND_ECC_SOFT:
> > +		nand->ecc.engine = nand_ecc_sw_get_engine(nand);
> > +		break;
> > +	case NAND_ECC_ON_DIE:
> > +		if (is_spinand)
> > +			nand->ecc.engine = spinand_ondie_ecc_get_engine();  
> 
> So, maybe it's worth having the ondie ECC engine instance directly
> embedded in nand_device instead of spinand, or maybe just a pointer, so
> that you don't reserve extra space when the NAND device does not have
> on-die ECC.
> 
> > +		else
> > +			pr_err("On-die ECC engines for non SPI devices not supported yet\n");
> > +		break;
> > +	case NAND_ECC_HW:
> > +		pr_err("Hardware ECC engines not supported yet\n");
> > +		break;
> > +	default:
> > +		pr_err("Missing ECC engine property\n");
> > +	}
> > +
> > +	if (!nand->ecc.engine)
> > +		return  -EINVAL;
> > +
> > +	return 0;
> > +}  
> 

I think this is the root patch were our ideas diverge. For me, each
'ECC engine' has a _get() helper and the NAND core decides which one to
call to retrieve the right engine. Can you please explain what was your
idea if this one does not fit?

Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the
user ECC mode and algo, so I cannot let the raw NAND core (or a raw
NAND controller driver) or the SPI NAND core decide which mode is the
default if not provided by the user.

Thanks,
Miquèl

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