Re: [RFC PATCH 24/27] mtd: spinand: Instantiate a SPI-NAND on-die ECC engine

From: Boris Brezillon <bbrezillon@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
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 24/27] mtd: spinand: Instantiate a SPI-NAND on-die ECC engine
Date: Fri, 22 Feb 2019 15:38:53 +0100	[thread overview]
Message-ID: <20190222153853.3347b253@kernel.org> (raw)
In-Reply-To: <20190221125806.28875-12-miquel.raynal@bootlin.com>

On Thu, 21 Feb 2019 13:58:03 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Make use of the existing functions taken from the SPI-NAND core to
> instantiate an on-die ECC engine specific to the SPI-NAND core. The
> next step will be to tweak the core to use this object instead of
> calling the helpers directly.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/spi/on-die-ecc-engine.c   | 105 +++++++++++++++++++++

Maybe on-die-ecc.c is enough. To be honest, given the amount of code,
I'd even prefer to have this code directly in core.c.

>  include/linux/mtd/nand-spi-on-die-engine.h |  35 +++++++

Hm, let's make those defs part of the spinand.h file instead of
creating a new one.

>  include/linux/mtd/nand.h                   |   1 +
>  3 files changed, 141 insertions(+)
>  create mode 100644 include/linux/mtd/nand-spi-on-die-engine.h
> 
> diff --git a/drivers/mtd/nand/spi/on-die-ecc-engine.c b/drivers/mtd/nand/spi/on-die-ecc-engine.c
> index 3d5b83238e14..515b9e3efc93 100644
> --- a/drivers/mtd/nand/spi/on-die-ecc-engine.c
> +++ b/drivers/mtd/nand/spi/on-die-ecc-engine.c
> @@ -45,3 +45,108 @@ int spinand_check_ecc_status(struct spinand_device *spinand, u8 status)
>  
>  	return -EINVAL;
>  }
> +
> +static int spinand_noecc_ooblayout_ecc(struct mtd_info *mtd, int section,
> +				       struct mtd_oob_region *region)
> +{
> +	return -ERANGE;
> +}
> +
> +static int spinand_noecc_ooblayout_free(struct mtd_info *mtd, int section,
> +					struct mtd_oob_region *region)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	/* Reserve 2 bytes for the BBM. */
> +	region->offset = 2;
> +	region->length = 62;
> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops spinand_noecc_ooblayout = {
> +	.ecc = spinand_noecc_ooblayout_ecc,
> +	.free = spinand_noecc_ooblayout_free,
> +};
> +
> +static int ecc_spinand_ondie_init_ctx(struct nand_device *nand)
> +{
> +	struct spinand_device *spinand = nand_to_spinand(nand);
> +	struct mtd_info *mtd = nanddev_to_mtd(nand);
> +	struct ecc_sw_hamming_conf *engine_conf;
> +
> +	nand->ecc.ctx.conf.step_size = nand->ecc.requirements.step_size;
> +	nand->ecc.ctx.conf.strength = nand->ecc.requirements.strength;
> +
> +	engine_conf = kzalloc(sizeof(*engine_conf), GFP_KERNEL);
> +	if (!engine_conf)
> +		return -ENOMEM;
> +
> +	nand->ecc.ctx.priv = engine_conf;
> +
> +	if (spinand->eccinfo.ooblayout)
> +		mtd_set_ooblayout(mtd, spinand->eccinfo.ooblayout);
> +	else
> +		mtd_set_ooblayout(mtd, &spinand_noecc_ooblayout);
> +
> +	return 0;
> +}
> +
> +static void ecc_spinand_ondie_cleanup_ctx(struct nand_device *nand)
> +{
> +	kfree(nand->ecc.ctx.priv);
> +}
> +
> +static int ecc_spinand_ondie_prepare_io_req(struct nand_device *nand,
> +					    struct nand_page_io_req *req,
> +					    void *oobbuf)
> +{
> +	struct spinand_device *spinand = nand_to_spinand(nand);
> +	bool enable = (req->mode != MTD_OPS_RAW);
> +
> +	/* Only enable or disable the engine */
> +	return spinand_ecc_enable(spinand, enable);
> +}
> +
> +static int ecc_spinand_ondie_finish_io_req(struct nand_device *nand,
> +					   struct nand_page_io_req *req,
> +					   void *oobbuf)
> +{
> +	struct ecc_spinand_ondie_conf *engine_conf = nand->ecc.ctx.priv;
> +	struct spinand_device *spinand = nand_to_spinand(nand);
> +
> +	if (req->mode == MTD_OPS_RAW)
> +		return 0;
> +
> +	/* Nothing to do when finishing a page write */
> +	if (req->type == NAND_PAGE_WRITE)
> +		return 0;
> +
> +	/* Finish a page write: check the status, report errors/bitflips */
> +	return spinand_check_ecc_status(spinand, engine_conf->status);
> +}
> +
> +static struct nand_ecc_engine_ops spinand_ondie_engine_ops = {
> +	.init_ctx = ecc_spinand_ondie_init_ctx,
> +	.cleanup_ctx = ecc_spinand_ondie_cleanup_ctx,
> +	.prepare_io_req = ecc_spinand_ondie_prepare_io_req,
> +	.finish_io_req = ecc_spinand_ondie_finish_io_req,
> +};
> +
> +static struct nand_ecc_engine spinand_ondie_engine = {
> +	.ops = &spinand_ondie_engine_ops,
> +};
> +
> +void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
> +{
> +	struct ecc_spinand_ondie_conf *engine_conf = nand->ecc.ctx.priv;
> +
> +	if (nand->ecc.engine == &spinand_ondie_engine && engine_conf)
> +		engine_conf->status = status;
> +}
> +
> +struct nand_ecc_engine *spinand_ondie_ecc_get_engine(void)
> +{
> +	return &spinand_ondie_engine;

Hm, I was expecting to have one engine instance directly embedded in
spinand_device. The ECC engine is not a global thing here, it's
actually tied to the SPI NAND device itself.

> +}

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