On Thu, 28 May 2020 13:31:10 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> Create a generic ECC engine object.
>
> Later the ecc.c file will receive more generic code coming from
> the raw NAND specific part. This is a base to instantiate ECC engine
> objects.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
> drivers/mtd/nand/Kconfig | 7 ++
> drivers/mtd/nand/Makefile | 2 +
> drivers/mtd/nand/ecc.c | 138 ++++++++++++++++++++++++++++++++++++++
> include/linux/mtd/nand.h | 67 ++++++++++++++++++
> 4 files changed, 214 insertions(+)
> create mode 100644 drivers/mtd/nand/ecc.c
>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index c1a45b071165..a4478ffa279d 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -9,4 +9,11 @@ source "drivers/mtd/nand/onenand/Kconfig"
> source "drivers/mtd/nand/raw/Kconfig"
> source "drivers/mtd/nand/spi/Kconfig"
>
> +menu "ECC engine support"
> +
> +config MTD_NAND_ECC
> + bool
> +
> +endmenu
> +
> endmenu
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 7ecd80c0a66e..981372953b56 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -6,3 +6,5 @@ obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
> obj-y += onenand/
> obj-y += raw/
> obj-y += spi/
> +
> +nandcore-$(CONFIG_MTD_NAND_ECC) += ecc.o
> diff --git a/drivers/mtd/nand/ecc.c b/drivers/mtd/nand/ecc.c
> new file mode 100644
> index 000000000000..e4f2b6fcbb12
> --- /dev/null
> +++ b/drivers/mtd/nand/ecc.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Generic Error-Correcting Code (ECC) engine
> + *
> + * Copyright (C) 2019 Macronix
> + * Author:
> + * Miquèl RAYNAL <miquel.raynal@xxxxxxxxxxx>
> + *
> + *
> + * This file describes the abstraction of any NAND ECC engine. It has been
> + * designed to fit most cases, including parallel NANDs and SPI-NANDs.
> + *
> + * There are three main situations where instantiating this ECC engine makes
> + * sense:
> + * - "external": The ECC engine is outside the NAND pipeline, typically this
I'm not sure why you put quotes around those names.
> + * is a software ECC engine. One can also imagine a generic
^ or an hardware
engine that's outside the NAND controller pipeline.
You can the drop the "One can also imagine ..." since it's more than a
theoretical use case, we already have a few engines that fall in this
category.
> + * hardware ECC engine which would be an IP itself. Interacting
> + * with a SPI-NAND device without on-die ECC could be achieved
^can
> + * thanks to the use of such external engine.
But I think I would simply drop this last sentence.
> + * - "pipelined": The ECC engine is inside the NAND pipeline, ie. on the
> + * controller's side. This is the case of most of the raw NAND
> + * controllers. These controllers usually embed an hardware ECC
> + * engine which is managed thanks to the same register set as
> + * the controller's.
Again, I would drop the last sentence here. I think saying the ECC
bytes are generated/data corrected on the fly when a page is
written/read would be more useful.
> + * - "ondie": The ECC engine is inside the NAND pipeline, on the chip's side.
> + * Some NAND chips can correct themselves the data. The on-die
> + * correction can be enabled, disabled and the status of the
> + * correction after a read may be retrieved with a NAND command
> + * (may be vendor specific).
"The on-die correction can be enabled, disabled" -> this is true for
any kind of ECC engine :P.
> + *
> + * Besides the initial setup and final cleanups, the interfaces are rather
> + * simple:
> + * - "prepare": Prepare an I/O request, check the ECC engine is enabled or
^if/whether
> + * disabled as requested before the I/O. In case of software
How about "Enable/disable the ECC engine based on the I/O request type."
> + * correction, this step may involve to derive the ECC bytes and
> + * place them in the OOB area before a write.
This is also true for external hardware ECC engines.
> + * - "finish": Finish an I/O request, check the status of the operation ie.
> + * the data validity in case of a read (report to the upper layer
> + * any bitflip/errors).
It's all about correcting/reporting errors, right. Let's try to put
that into simple words: "Correct the data in case of a read request and
report the number of corrected bits/uncorrectable errors. Most likely
empty for write operations, unless you have hardware specific stuff to
do, like shutting down the engine to save some power"
> + *
> + * Both prepare/finish callbacks are supposed to enclose I/O request and will
"The I/O request should be enclosed in a prepare()/finish() pair of
calls" or "The prepare/finish call should surround the I/O request".
> + * behave differently depending on the desired correction:
^requested I/O type
> + * - "raw": Correction disabled
> + * - "ecc": Correction enabled
> + *
> + * The request direction is impacting the logic as well:
> + * - "read": Load data from the NAND chip
> + * - "write": Store data in the NAND chip
> + *
> + * Mixing all this combinations together gives the following behavior.
Mention that those are just examples, and drivers are free to add
custom steps in their prepare/finish hooks.
> + *
> + * ["external" ECC engine]
> + * - external + prepare + raw + read: do nothing
> + * - external + finish + raw + read: do nothing
> + * - external + prepare + raw + write: do nothing
> + * - external + finish + raw + write: do nothing
> + * - external + prepare + ecc + read: do nothing
> + * - external + finish + ecc + read: calculate expected ECC bytes, extract
> + * ECC bytes from OOB buffer, correct
> + * and report any bitflip/error
> + * - external + prepare + ecc + write: calculate ECC bytes and store them at
> + * the right place in the OOB buffer based
> + * on the OOB layout
> + * - external + finish + ecc + write: do nothing
> + *
> + * ["pipelined" ECC engine]
> + * - pipelined + prepare + raw + read: disable the controller's ECC engine if
> + * activated
> + * - pipelined + finish + raw + read: do nothing
> + * - pipelined + prepare + raw + write: disable the controller's ECC engine if
> + * activated
> + * - pipelined + finish + raw + write: do nothing
> + * - pipelined + prepare + ecc + read: enable the controller's ECC engine if
> + * deactivated
> + * - pipelined + finish + ecc + read: check the status, report any
> + * error/bitflip
> + * - pipelined + prepare + ecc + write: enable the controller's ECC engine if
> + * deactivated
> + * - pipelined + finish + ecc + write: do nothing
> + *
> + * ["ondie" ECC engine]
> + * - ondie + prepare + raw + read: send commands to disable the on-chip ECC
> + * engine if activated
> + * - ondie + finish + raw + read: do nothing
> + * - ondie + prepare + raw + write: send commands to disable the on-chip ECC
> + * engine if activated
> + * - ondie + finish + raw + write: do nothing
> + * - ondie + prepare + ecc + read: send commands to enable the on-chip ECC
> + * engine if deactivated
> + * - ondie + finish + ecc + read: send commands to check the status, report
> + * any error/bitflip
> + * - ondie + prepare + ecc + write: send commands to enable the on-chip ECC
> + * engine if deactivated
> + * - ondie + finish + ecc + write: do nothing
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/nand.h>
> +
Shouldn't we have kernel-docs for those functions?
> +int nand_ecc_init_ctx(struct nand_device *nand)
> +{
> + if (!nand->ecc.engine->ops->init_ctx)
> + return 0;
> +
> + return nand->ecc.engine->ops->init_ctx(nand);
> +}
> +EXPORT_SYMBOL(nand_ecc_init_ctx);
> +
> +void nand_ecc_cleanup_ctx(struct nand_device *nand)
> +{
> + if (nand->ecc.engine->ops->cleanup_ctx)
> + nand->ecc.engine->ops->cleanup_ctx(nand);
> +}
> +EXPORT_SYMBOL(nand_ecc_cleanup_ctx);
> +
> +int nand_ecc_prepare_io_req(struct nand_device *nand,
> + struct nand_page_io_req *req)
> +{
> + if (!nand->ecc.engine->ops->prepare_io_req)
> + return 0;
> +
> + return nand->ecc.engine->ops->prepare_io_req(nand, req);
> +}
> +EXPORT_SYMBOL(nand_ecc_prepare_io_req);
> +
> +int nand_ecc_finish_io_req(struct nand_device *nand,
> + struct nand_page_io_req *req)
> +{
> + if (!nand->ecc.engine->ops->finish_io_req)
> + return 0;
> +
> + return nand->ecc.engine->ops->finish_io_req(nand, req);
> +}
> +EXPORT_SYMBOL(nand_ecc_finish_io_req);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Miquel Raynal <miquel.raynal@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Generic ECC engine");
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 2e9af24936cd..0be260fd2f86 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -221,6 +221,73 @@ struct nand_ops {
> bool (*isbad)(struct nand_device *nand, const struct nand_pos *pos);
> };
>
> +/**
> + * struct nand_ecc_context - Context for the ECC engine
> + * @conf: basic ECC engine parameters
> + * @total: Total number of bytes used for storing ECC codes, this is used by
Sometimes you start your description with an uppercase, sometimes not.
> + * generic OOB layouts
> + * @priv: ECC engine driver private data
> + */
> +struct nand_ecc_context {
> + struct nand_ecc_props conf;
> + unsigned int total;
> + void *priv;
> +};
> +
> +/**
> + * struct nand_ecc_engine_ops - Generic ECC engine operations
^s/Generic//
> + * @init_ctx: given a desired user configuration for the pointed NAND device,
> + * requests the ECC engine driver to setup a configuration with
> + * values it supports.
> + * @cleanup_ctx: clean the context initialized by @init_ctx.
> + * @prepare_io_req: is called before reading/writing a page to prepare the I/O
> + * request to be performed with ECC correction.
> + * @finish_io_req: is called after reading/writing a page to terminate the I/O
> + * request and ensure proper ECC correction.
> + */
> +struct nand_ecc_engine_ops {
> + int (*init_ctx)(struct nand_device *nand);
> + void (*cleanup_ctx)(struct nand_device *nand);
> + int (*prepare_io_req)(struct nand_device *nand,
> + struct nand_page_io_req *req);
> + int (*finish_io_req)(struct nand_device *nand,
> + struct nand_page_io_req *req);
> +};
> +
> +/**
> + * struct nand_ecc_engine - Generic ECC engine abstraction for NAND devices
^s/Generic//
> + * @ops: ECC engine operations
> + */
> +struct nand_ecc_engine {
> + struct nand_ecc_engine_ops *ops;
> +};
> +
> +int nand_ecc_init_ctx(struct nand_device *nand);
> +void nand_ecc_cleanup_ctx(struct nand_device *nand);
> +int nand_ecc_prepare_io_req(struct nand_device *nand,
> + struct nand_page_io_req *req);
> +int nand_ecc_finish_io_req(struct nand_device *nand,
> + struct nand_page_io_req *req);
> +
> +/**
> + * struct nand_ecc - High-level ECC object
I think you can drop the "High-level" and just say "Information
relative to the ECC"
> + * @defaults: Default values, depend on the underlying subsystem
> + * @requirements: ECC requirements from the NAND chip perspective
> + * @user_conf: User desires in terms of ECC parameters
> + * @ctx: ECC context for the ECC engine, derived from the device @requirements
> + * the @user_conf and the @defaults
> + * @ondie_engine: On-die ECC engine reference, if any
> + * @engine: ECC engine actually bound
> + */
> +struct nand_ecc {
> + struct nand_ecc_props defaults;
> + struct nand_ecc_props requirements;
> + struct nand_ecc_props user_conf;
> + struct nand_ecc_context ctx;
> + struct nand_ecc_engine *ondie_engine;
> + struct nand_ecc_engine *engine;
> +};
> +
> /**
> * struct nand_device - NAND device
> * @mtd: MTD instance attached to the NAND device
