| Message ID | 20190919193141.7865-6-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | Introduce the generic ECC engine abstraction | expand |
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
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
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 --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
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(+)