On Thu, 28 May 2020 13:31:10 +0200
Miquel Raynal [off-list ref] wrote:

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?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel