[v3,03/40] mtd: rawnand: Create a new enumeration to describe OOB placement
diff mbox series

Message ID 20190919193141.7865-4-miquel.raynal@bootlin.com
State New
Headers show
Series
  • Introduce the generic ECC engine abstraction
Related show

Commit Message

Miquel Raynal Sept. 19, 2019, 7:31 p.m. UTC
There is currently a confusion between the ECC type/mode/provider
(eg. hardware, software, on-die or none) and the in-bad/out-of-band
layout which is only described for hardware engines (OOB first,
syndrome).

Create a new enumeration to describe this placement.

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

Comments

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

> There is currently a confusion between the ECC type/mode/provider
> (eg. hardware, software, on-die or none) and the in-bad/out-of-band

						   ^in-band

> layout which is only described for hardware engines (OOB first,
> syndrome).

It's not really about in-band/out-of-band data placement (though it
also has an impact on it since free OOB bytes are sometimes protected
by ECC or placed next to it), more ECC bytes placement.

> 
> Create a new enumeration to describe this placement.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c |  5 +++++
>  include/linux/mtd/rawnand.h      | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index e6c483ec191a..74e9289e931c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4842,6 +4842,11 @@ static const char * const nand_ecc_modes[] = {
>  	[NAND_ECC_ON_DIE]	= "on-die",
>  };
>  
> +static const char * const nand_ecc_engine_oob_placement[] = {

			     ^nand_ecc_placement ?

> +	[NAND_ECC_SYNDROME_OOB_PLACEMENT] = "hw_syndrome",
> +	[NAND_ECC_OOB_FIRST_PLACEMENT] = "hw_oob_first",

Since this is something you introduce, I'd recommend to change the
naming here:

s/NAND_ECC_SYNDROME_OOB_PLACEMENT/NAND_ECC_PLACEMENT_INTERLEAVED/
s/hw_syndrome/ecc-interleaved/
s/NAND_ECC_OOB_FIRST_PLACEMENT/NAND_ECC_PLACEMENT_FIRST/
s/hw_oob_first/ecc-first/

> +};
> +
>  static int of_get_nand_ecc_mode(struct device_node *np)
>  {
>  	const char *pm;
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index ccdc0c314acc..89f964816f2c 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -93,6 +93,18 @@ enum nand_ecc_mode {
>  	NAND_ECC_ON_DIE,
>  };
>  
> +/**
> + * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
> + * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified

Maybe describe what the standard layout is (ECC placed at the end), and
mention that "not specified/default" means the driver can decide to put
the ECC/free-OOB bytes where he wants.

> + * @NAND_ECC_SYNDROME_OOB_PLACEMENT: Syndrome layout (interlaced)
> + * @NAND_ECC_OOB_FIRST_PLACEMENT: Free OOB bytes first
> + */
> +enum nand_ecc_engine_oob_placement {
> +	NAND_ECC_DEFAULT_OOB_PLACEMENT,
> +	NAND_ECC_SYNDROME_OOB_PLACEMENT,
> +	NAND_ECC_OOB_FIRST_PLACEMENT,
> +};
> +
>  enum nand_ecc_algo {
>  	NAND_ECC_UNKNOWN,
>  	NAND_ECC_HAMMING,
Boris Brezillon Oct. 12, 2019, 9:07 a.m. UTC | #2
On Sat, 12 Oct 2019 11:02:09 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 19 Sep 2019 21:31:03 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > There is currently a confusion between the ECC type/mode/provider
> > (eg. hardware, software, on-die or none) and the in-bad/out-of-band  
> 
> 						   ^in-band
> 
> > layout which is only described for hardware engines (OOB first,
> > syndrome).  
> 
> It's not really about in-band/out-of-band data placement (though it
> also has an impact on it since free OOB bytes are sometimes protected
> by ECC or placed next to it), more ECC bytes placement.
> 
> > 
> > Create a new enumeration to describe this placement.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c |  5 +++++
> >  include/linux/mtd/rawnand.h      | 12 ++++++++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index e6c483ec191a..74e9289e931c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -4842,6 +4842,11 @@ static const char * const nand_ecc_modes[] = {
> >  	[NAND_ECC_ON_DIE]	= "on-die",
> >  };
> >  
> > +static const char * const nand_ecc_engine_oob_placement[] = {  
> 
> 			     ^nand_ecc_placement ?
> 
> > +	[NAND_ECC_SYNDROME_OOB_PLACEMENT] = "hw_syndrome",
> > +	[NAND_ECC_OOB_FIRST_PLACEMENT] = "hw_oob_first",  
> 
> Since this is something you introduce, I'd recommend to change the
> naming here:
> 
> s/NAND_ECC_SYNDROME_OOB_PLACEMENT/NAND_ECC_PLACEMENT_INTERLEAVED/
> s/hw_syndrome/ecc-interleaved/

I realize the "ecc-" prefix is unneeded, so maybe just "interleaved"
and "first".

> s/NAND_ECC_OOB_FIRST_PLACEMENT/NAND_ECC_PLACEMENT_FIRST/
> s/hw_oob_first/ecc-first/
> 
> > +};
> > +
> >  static int of_get_nand_ecc_mode(struct device_node *np)
> >  {
> >  	const char *pm;
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index ccdc0c314acc..89f964816f2c 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -93,6 +93,18 @@ enum nand_ecc_mode {
> >  	NAND_ECC_ON_DIE,
> >  };
> >  
> > +/**
> > + * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
> > + * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified  
> 
> Maybe describe what the standard layout is (ECC placed at the end), and
> mention that "not specified/default" means the driver can decide to put
> the ECC/free-OOB bytes where he wants.
> 
> > + * @NAND_ECC_SYNDROME_OOB_PLACEMENT: Syndrome layout (interlaced)
> > + * @NAND_ECC_OOB_FIRST_PLACEMENT: Free OOB bytes first
> > + */
> > +enum nand_ecc_engine_oob_placement {
> > +	NAND_ECC_DEFAULT_OOB_PLACEMENT,
> > +	NAND_ECC_SYNDROME_OOB_PLACEMENT,
> > +	NAND_ECC_OOB_FIRST_PLACEMENT,
> > +};
> > +
> >  enum nand_ecc_algo {
> >  	NAND_ECC_UNKNOWN,
> >  	NAND_ECC_HAMMING,  
>

Patch
diff mbox series

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index e6c483ec191a..74e9289e931c 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4842,6 +4842,11 @@  static const char * const nand_ecc_modes[] = {
 	[NAND_ECC_ON_DIE]	= "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",
+};
+
 static int of_get_nand_ecc_mode(struct device_node *np)
 {
 	const char *pm;
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index ccdc0c314acc..89f964816f2c 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -93,6 +93,18 @@  enum nand_ecc_mode {
 	NAND_ECC_ON_DIE,
 };
 
+/**
+ * enum nand_ecc_engine_oob_placement - NAND ECC engine OOB placement
+ * @NAND_ECC_DEFAULT_OOB_PLACEMENT: Standard layout, or not specified
+ * @NAND_ECC_SYNDROME_OOB_PLACEMENT: Syndrome layout (interlaced)
+ * @NAND_ECC_OOB_FIRST_PLACEMENT: Free OOB bytes first
+ */
+enum nand_ecc_engine_oob_placement {
+	NAND_ECC_DEFAULT_OOB_PLACEMENT,
+	NAND_ECC_SYNDROME_OOB_PLACEMENT,
+	NAND_ECC_OOB_FIRST_PLACEMENT,
+};
+
 enum nand_ecc_algo {
 	NAND_ECC_UNKNOWN,
 	NAND_ECC_HAMMING,