diff mbox series

[v3,05/40] mtd: rawnand: Create a new enumeration to describe properly ECC types

Message ID 20190919193141.7865-6-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Introduce the generic ECC engine abstraction | expand

Commit Message

Miquel Raynal Sept. 19, 2019, 7:31 p.m. UTC
Now that the misleading mix between ECC engine type and OOB placement
has been addressed, add a new enumeration to properly define ECC types
(also called provider or mode).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c |  7 +++++++
 include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
 2 files changed, 23 insertions(+)

Comments

Boris Brezillon Oct. 12, 2019, 9:14 a.m. UTC | #1
On Thu, 19 Sep 2019 21:31:05 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Now that the misleading mix between ECC engine type and OOB placement
> has been addressed, add a new enumeration to properly define ECC types
> (also called provider or mode).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c |  7 +++++++
>  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 9a05ebfc44d1..00a261284aad 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4842,6 +4842,13 @@ static const char * const nand_ecc_modes[] = {
>  	[NAND_ECC_ON_DIE]	= "on-die",
>  };
>  
> +static const char * const nand_ecc_engine_providers[] = {
> +	[NAND_NO_ECC_ENGINE] = "none",
> +	[NAND_SOFT_ECC_ENGINE] = "soft",
> +	[NAND_HW_ECC_ENGINE] = "hw",
> +	[NAND_ON_DIE_ECC_ENGINE] = "on-die",
> +};
> +
>  static const char * const nand_ecc_engine_oob_placement[] = {
>  	[NAND_ECC_SYNDROME_OOB_PLACEMENT] = "hw_syndrome",
>  	[NAND_ECC_OOB_FIRST_PLACEMENT] = "hw_oob_first",
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 1b462fb2ab77..23feab162bc2 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -93,6 +93,22 @@ enum nand_ecc_mode {
>  	NAND_ECC_ON_DIE,
>  };
>  
> +/**
> + * enum nand_ecc_engine_type - NAND ECC engine type/provider
> + * @NAND_INVALID_ECC_ENGINE: Invalid value
> + * @NAND_NO_ECC_ENGINE: No ECC correction
> + * @NAND_SOFT_ECC_ENGINE: Software ECC correction
> + * @NAND_HW_ECC_ENGINE: Hardware (controller side) ECC correction
> + * @NAND_ON_DIE_ECC_ENGINE: Hardware (chip side) ECC correction
> + */
> +enum nand_ecc_engine_type {
> +	NAND_INVALID_ECC_ENGINE,

Same comment as for the NAND_ECC_INVALID addition: if you don't have an
entry in nand_ecc_engine_providers for this INVALID case, it's probably
better to define it to -1.

> +	NAND_NO_ECC_ENGINE,
> +	NAND_SOFT_ECC_ENGINE,
> +	NAND_HW_ECC_ENGINE,

I'd rename that one into

	NAND_CONTROLLER_ECC_ENGINE,

HW is a bit too vague.

> +	NAND_ON_DIE_ECC_ENGINE,

I also find it clearer when the same prefix is used:

	NAND_ECC_ENGINE_INVALID = -1,
	NAND_ECC_ENGINE_NONE = 0,
	NAND_ECC_ENGINE_SOFT,
	NAND_ECC_ENGINE_CONTROLLER,
	NAND_ECC_ENGINE_ON_DIE,

Looks good otherwise. Feel free to ignore my comments if you disagree.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> +};
> +
>  /**
>   * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
>   * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified
Boris Brezillon Oct. 12, 2019, 9:55 a.m. UTC | #2
On Thu, 19 Sep 2019 21:31:05 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Now that the misleading mix between ECC engine type and OOB placement
> has been addressed, add a new enumeration to properly define ECC types
> (also called provider or mode).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c |  7 +++++++
>  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 9a05ebfc44d1..00a261284aad 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4842,6 +4842,13 @@ static const char * const nand_ecc_modes[] = {
>  	[NAND_ECC_ON_DIE]	= "on-die",
>  };
>  
> +static const char * const nand_ecc_engine_providers[] = {
> +	[NAND_NO_ECC_ENGINE] = "none",
> +	[NAND_SOFT_ECC_ENGINE] = "soft",
> +	[NAND_HW_ECC_ENGINE] = "hw",
> +	[NAND_ON_DIE_ECC_ENGINE] = "on-die",
> +};
> +
>  static const char * const nand_ecc_engine_oob_placement[] = {
>  	[NAND_ECC_SYNDROME_OOB_PLACEMENT] = "hw_syndrome",
>  	[NAND_ECC_OOB_FIRST_PLACEMENT] = "hw_oob_first",
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 1b462fb2ab77..23feab162bc2 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -93,6 +93,22 @@ enum nand_ecc_mode {
>  	NAND_ECC_ON_DIE,
>  };
>  
> +/**
> + * enum nand_ecc_engine_type - NAND ECC engine type/provider
> + * @NAND_INVALID_ECC_ENGINE: Invalid value
> + * @NAND_NO_ECC_ENGINE: No ECC correction
> + * @NAND_SOFT_ECC_ENGINE: Software ECC correction
> + * @NAND_HW_ECC_ENGINE: Hardware (controller side) ECC correction
> + * @NAND_ON_DIE_ECC_ENGINE: Hardware (chip side) ECC correction
> + */
> +enum nand_ecc_engine_type {
> +	NAND_INVALID_ECC_ENGINE,
> +	NAND_NO_ECC_ENGINE,
> +	NAND_SOFT_ECC_ENGINE,
> +	NAND_HW_ECC_ENGINE,
> +	NAND_ON_DIE_ECC_ENGINE,
> +};
> +

No that I know where you're going, I'd recommend moving those
definitions to the generic NAND layer (or the generic ECC layer you're
about to introduce). Actually, re-ordering patches to put rawnand
changes after the introduction of the generic ECC layer might even be
simpler.

>  /**
>   * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
>   * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified
Miquel Raynal Jan. 15, 2020, 8:58 p.m. UTC | #3
Hi Boris,

> > +/**
> > + * enum nand_ecc_engine_type - NAND ECC engine type/provider
> > + * @NAND_INVALID_ECC_ENGINE: Invalid value
> > + * @NAND_NO_ECC_ENGINE: No ECC correction
> > + * @NAND_SOFT_ECC_ENGINE: Software ECC correction
> > + * @NAND_HW_ECC_ENGINE: Hardware (controller side) ECC correction
> > + * @NAND_ON_DIE_ECC_ENGINE: Hardware (chip side) ECC correction
> > + */
> > +enum nand_ecc_engine_type {
> > +	NAND_INVALID_ECC_ENGINE,  
> 
> Same comment as for the NAND_ECC_INVALID addition: if you don't have an
> entry in nand_ecc_engine_providers for this INVALID case, it's probably
> better to define it to -1.
> 
> > +	NAND_NO_ECC_ENGINE,
> > +	NAND_SOFT_ECC_ENGINE,
> > +	NAND_HW_ECC_ENGINE,  
> 
> I'd rename that one into
> 
> 	NAND_CONTROLLER_ECC_ENGINE,
> 
> HW is a bit too vague.
> 
> > +	NAND_ON_DIE_ECC_ENGINE,  
> 
> I also find it clearer when the same prefix is used:
> 
> 	NAND_ECC_ENGINE_INVALID = -1,
> 	NAND_ECC_ENGINE_NONE = 0,
> 	NAND_ECC_ENGINE_SOFT,
> 	NAND_ECC_ENGINE_CONTROLLER,
> 	NAND_ECC_ENGINE_ON_DIE,
> 
> Looks good otherwise. Feel free to ignore my comments if you disagree.

Same prefix it is!

However, I don't want to start as -1 as otherwise using a for-loop to
loop over each ECC engine would be less straightforward.

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 9a05ebfc44d1..00a261284aad 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4842,6 +4842,13 @@  static const char * const nand_ecc_modes[] = {
 	[NAND_ECC_ON_DIE]	= "on-die",
 };
 
+static const char * const nand_ecc_engine_providers[] = {
+	[NAND_NO_ECC_ENGINE] = "none",
+	[NAND_SOFT_ECC_ENGINE] = "soft",
+	[NAND_HW_ECC_ENGINE] = "hw",
+	[NAND_ON_DIE_ECC_ENGINE] = "on-die",
+};
+
 static const char * const nand_ecc_engine_oob_placement[] = {
 	[NAND_ECC_SYNDROME_OOB_PLACEMENT] = "hw_syndrome",
 	[NAND_ECC_OOB_FIRST_PLACEMENT] = "hw_oob_first",
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 1b462fb2ab77..23feab162bc2 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -93,6 +93,22 @@  enum nand_ecc_mode {
 	NAND_ECC_ON_DIE,
 };
 
+/**
+ * enum nand_ecc_engine_type - NAND ECC engine type/provider
+ * @NAND_INVALID_ECC_ENGINE: Invalid value
+ * @NAND_NO_ECC_ENGINE: No ECC correction
+ * @NAND_SOFT_ECC_ENGINE: Software ECC correction
+ * @NAND_HW_ECC_ENGINE: Hardware (controller side) ECC correction
+ * @NAND_ON_DIE_ECC_ENGINE: Hardware (chip side) ECC correction
+ */
+enum nand_ecc_engine_type {
+	NAND_INVALID_ECC_ENGINE,
+	NAND_NO_ECC_ENGINE,
+	NAND_SOFT_ECC_ENGINE,
+	NAND_HW_ECC_ENGINE,
+	NAND_ON_DIE_ECC_ENGINE,
+};
+
 /**
  * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
  * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified