diff mbox series

[v13,19/20] mtd: rawnand: Use the ECC framework user input parsing bits

Message ID 20200827085208.16276-20-miquel.raynal@bootlin.com
State Accepted
Delegated to: Miquel Raynal
Headers show
Series Introduction of the generic ECC framework | expand

Commit Message

Miquel Raynal Aug. 27, 2020, 8:52 a.m. UTC 
Many helpers are generic to all NAND chips, they should not be
raw-NAND specific, so use the generic ones.

To avoid moving all the raw NAND core "history" into the generic NAND
layer, we keep a part of this parsing in the raw NAND core to ensure
backward compatibility.

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

Comments

Miquel Raynal Sept. 7, 2020, 7:23 a.m. UTC | #1 
On Thu, 2020-08-27 at 08:52:07 UTC, Miquel Raynal wrote:
> Many helpers are generic to all NAND chips, they should not be
> raw-NAND specific, so use the generic ones.
> 
> To avoid moving all the raw NAND core "history" into the generic NAND
> layer, we keep a part of this parsing in the raw NAND core to ensure
> backward compatibility.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.

Miquel
Sascha Hauer Sept. 21, 2020, 9:36 a.m. UTC | #2 
On Thu, Aug 27, 2020 at 10:52:07AM +0200, Miquel Raynal wrote:
> Many helpers are generic to all NAND chips, they should not be
> raw-NAND specific, so use the generic ones.
> 
> To avoid moving all the raw NAND core "history" into the generic NAND
> layer, we keep a part of this parsing in the raw NAND core to ensure
> backward compatibility.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

...

> -static int of_get_nand_ecc_step_size(struct device_node *np)
> +static void of_get_nand_ecc_legacy_user_config(struct nand_chip *chip)
>  {
> -	int ret;
> -	u32 val;
> +	struct device_node *dn = nand_get_flash_node(chip);
> +	struct nand_ecc_props *user_conf = &chip->base.ecc.user_conf;
>  
> -	ret = of_property_read_u32(np, "nand-ecc-step-size", &val);
> -	return ret ? ret : val;
> -}
> +	if (user_conf->engine_type != NAND_ECC_ENGINE_TYPE_INVALID)
> +		user_conf->engine_type = of_get_rawnand_ecc_engine_type_legacy(dn);

I think the intention is to set user_conf->engine_type when it's not set already,
so this should be == instead of !=

>  
> -static int of_get_nand_ecc_strength(struct device_node *np)
> -{
> -	int ret;
> -	u32 val;
> +	if (user_conf->algo != NAND_ECC_ALGO_UNKNOWN)
> +		user_conf->algo = of_get_rawnand_ecc_algo_legacy(dn);

ditto

>  
> -	ret = of_property_read_u32(np, "nand-ecc-strength", &val);
> -	return ret ? ret : val;
> +	if (user_conf->placement != NAND_ECC_PLACEMENT_UNKNOWN)
> +		user_conf->placement = of_get_rawnand_ecc_placement_legacy(dn);

ditto

Sascha
Miquel Raynal Sept. 28, 2020, 3:18 p.m. UTC | #3 
Hi Sascha,

Sascha Hauer <s.hauer@pengutronix.de> wrote on Mon, 21 Sep 2020
11:36:03 +0200:

> On Thu, Aug 27, 2020 at 10:52:07AM +0200, Miquel Raynal wrote:
> > Many helpers are generic to all NAND chips, they should not be
> > raw-NAND specific, so use the generic ones.
> > 
> > To avoid moving all the raw NAND core "history" into the generic NAND
> > layer, we keep a part of this parsing in the raw NAND core to ensure
> > backward compatibility.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---  
> 
> ...
> 
> > -static int of_get_nand_ecc_step_size(struct device_node *np)
> > +static void of_get_nand_ecc_legacy_user_config(struct nand_chip *chip)
> >  {
> > -	int ret;
> > -	u32 val;
> > +	struct device_node *dn = nand_get_flash_node(chip);
> > +	struct nand_ecc_props *user_conf = &chip->base.ecc.user_conf;
> >  
> > -	ret = of_property_read_u32(np, "nand-ecc-step-size", &val);
> > -	return ret ? ret : val;
> > -}
> > +	if (user_conf->engine_type != NAND_ECC_ENGINE_TYPE_INVALID)
> > +		user_conf->engine_type = of_get_rawnand_ecc_engine_type_legacy(dn);  
> 
> I think the intention is to set user_conf->engine_type when it's not set already,
> so this should be == instead of !=
> 
> >  
> > -static int of_get_nand_ecc_strength(struct device_node *np)
> > -{
> > -	int ret;
> > -	u32 val;
> > +	if (user_conf->algo != NAND_ECC_ALGO_UNKNOWN)
> > +		user_conf->algo = of_get_rawnand_ecc_algo_legacy(dn);  
> 
> ditto
> 
> >  
> > -	ret = of_property_read_u32(np, "nand-ecc-strength", &val);
> > -	return ret ? ret : val;
> > +	if (user_conf->placement != NAND_ECC_PLACEMENT_UNKNOWN)
> > +		user_conf->placement = of_get_rawnand_ecc_placement_legacy(dn);  
> 
> ditto
> 
> Sascha
> 

Sorry for the delay I was away.

You are completely right, I messed up with the logic here... This is
now fixed, 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 66443986d80f..6a5f5699514f 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4882,18 +4882,27 @@  static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
 	return ret;
 }
 
-static const char * const nand_ecc_modes[] = {
-	[NAND_ECC_NONE]		= "none",
-	[NAND_ECC_SOFT]		= "soft",
-	[NAND_ECC_HW]		= "hw",
-	[NAND_ECC_HW_SYNDROME]	= "hw_syndrome",
-	[NAND_ECC_ON_DIE]	= "on-die",
-};
-
 static enum nand_ecc_engine_type
-of_get_nand_ecc_engine_type(struct device_node *np)
+of_get_rawnand_ecc_engine_type_legacy(struct device_node *np)
 {
-	enum nand_ecc_mode eng_type;
+	enum nand_ecc_legacy_mode {
+		NAND_ECC_INVALID,
+		NAND_ECC_NONE,
+		NAND_ECC_SOFT,
+		NAND_ECC_SOFT_BCH,
+		NAND_ECC_HW,
+		NAND_ECC_HW_SYNDROME,
+		NAND_ECC_ON_DIE,
+	};
+	const char * const nand_ecc_legacy_modes[] = {
+		[NAND_ECC_NONE]		= "none",
+		[NAND_ECC_SOFT]		= "soft",
+		[NAND_ECC_SOFT_BCH]	= "soft_bch",
+		[NAND_ECC_HW]		= "hw",
+		[NAND_ECC_HW_SYNDROME]	= "hw_syndrome",
+		[NAND_ECC_ON_DIE]	= "on-die",
+	};
+	enum nand_ecc_legacy_mode eng_type;
 	const char *pm;
 	int err;
 
@@ -4902,12 +4911,13 @@  of_get_nand_ecc_engine_type(struct device_node *np)
 		return NAND_ECC_ENGINE_TYPE_INVALID;
 
 	for (eng_type = NAND_ECC_NONE;
-	     eng_type < ARRAY_SIZE(nand_ecc_modes); eng_type++) {
-		if (!strcasecmp(pm, nand_ecc_modes[eng_type])) {
+	     eng_type < ARRAY_SIZE(nand_ecc_legacy_modes); eng_type++) {
+		if (!strcasecmp(pm, nand_ecc_legacy_modes[eng_type])) {
 			switch (eng_type) {
 			case NAND_ECC_NONE:
 				return NAND_ECC_ENGINE_TYPE_NONE;
 			case NAND_ECC_SOFT:
+			case NAND_ECC_SOFT_BCH:
 				return NAND_ECC_ENGINE_TYPE_SOFT;
 			case NAND_ECC_HW:
 			case NAND_ECC_HW_SYNDROME:
@@ -4920,43 +4930,29 @@  of_get_nand_ecc_engine_type(struct device_node *np)
 		}
 	}
 
-	/*
-	 * For backward compatibility we support few obsoleted values that don't
-	 * have their mappings into the nand_ecc_engine_providers enum anymore
-	 * (they were merged with other enums).
-	 */
-	if (!strcasecmp(pm, "soft_bch"))
-		return NAND_ECC_ENGINE_TYPE_SOFT;
-
 	return NAND_ECC_ENGINE_TYPE_INVALID;
 }
 
-static const char * const nand_ecc_algos[] = {
-	[NAND_ECC_ALGO_HAMMING]	= "hamming",
-	[NAND_ECC_ALGO_BCH]	= "bch",
-	[NAND_ECC_ALGO_RS]	= "rs",
-};
-
-static enum nand_ecc_algo of_get_nand_ecc_algo(struct device_node *np)
+enum nand_ecc_placement
+of_get_rawnand_ecc_placement_legacy(struct device_node *np)
 {
-	enum nand_ecc_algo ecc_algo;
 	const char *pm;
 	int err;
 
-	err = of_property_read_string(np, "nand-ecc-algo", &pm);
+	err = of_property_read_string(np, "nand-ecc-mode", &pm);
 	if (!err) {
-		for (ecc_algo = NAND_ECC_ALGO_HAMMING;
-		     ecc_algo < ARRAY_SIZE(nand_ecc_algos);
-		     ecc_algo++) {
-			if (!strcasecmp(pm, nand_ecc_algos[ecc_algo]))
-				return ecc_algo;
-		}
+		if (!strcasecmp(pm, "hw_syndrome"))
+			return NAND_ECC_PLACEMENT_INTERLEAVED;
 	}
 
-	/*
-	 * For backward compatibility we also read "nand-ecc-mode" checking
-	 * for some obsoleted values that were specifying ECC algorithm.
-	 */
+	return NAND_ECC_PLACEMENT_UNKNOWN;
+}
+
+static enum nand_ecc_algo of_get_rawnand_ecc_algo_legacy(struct device_node *np)
+{
+	const char *pm;
+	int err;
+
 	err = of_property_read_string(np, "nand-ecc-mode", &pm);
 	if (!err) {
 		if (!strcasecmp(pm, "soft"))
@@ -4968,22 +4964,19 @@  static enum nand_ecc_algo of_get_nand_ecc_algo(struct device_node *np)
 	return NAND_ECC_ALGO_UNKNOWN;
 }
 
-static int of_get_nand_ecc_step_size(struct device_node *np)
+static void of_get_nand_ecc_legacy_user_config(struct nand_chip *chip)
 {
-	int ret;
-	u32 val;
+	struct device_node *dn = nand_get_flash_node(chip);
+	struct nand_ecc_props *user_conf = &chip->base.ecc.user_conf;
 
-	ret = of_property_read_u32(np, "nand-ecc-step-size", &val);
-	return ret ? ret : val;
-}
+	if (user_conf->engine_type != NAND_ECC_ENGINE_TYPE_INVALID)
+		user_conf->engine_type = of_get_rawnand_ecc_engine_type_legacy(dn);
 
-static int of_get_nand_ecc_strength(struct device_node *np)
-{
-	int ret;
-	u32 val;
+	if (user_conf->algo != NAND_ECC_ALGO_UNKNOWN)
+		user_conf->algo = of_get_rawnand_ecc_algo_legacy(dn);
 
-	ret = of_property_read_u32(np, "nand-ecc-strength", &val);
-	return ret ? ret : val;
+	if (user_conf->placement != NAND_ECC_PLACEMENT_UNKNOWN)
+		user_conf->placement = of_get_rawnand_ecc_placement_legacy(dn);
 }
 
 static int of_get_nand_bus_width(struct device_node *np)
@@ -5007,12 +5000,10 @@  static bool of_get_nand_on_flash_bbt(struct device_node *np)
 	return of_property_read_bool(np, "nand-on-flash-bbt");
 }
 
-static int nand_dt_init(struct nand_chip *chip)
+static int rawnand_dt_init(struct nand_chip *chip)
 {
+	struct nand_device *nand = mtd_to_nanddev(nand_to_mtd(chip));
 	struct device_node *dn = nand_get_flash_node(chip);
-	enum nand_ecc_engine_type ecc_type;
-	enum nand_ecc_algo ecc_algo;
-	int ecc_strength, ecc_step;
 
 	if (!dn)
 		return 0;
@@ -5026,26 +5017,33 @@  static int nand_dt_init(struct nand_chip *chip)
 	if (of_get_nand_on_flash_bbt(dn))
 		chip->bbt_options |= NAND_BBT_USE_FLASH;
 
-	ecc_type = of_get_nand_ecc_engine_type(dn);
-	ecc_algo = of_get_nand_ecc_algo(dn);
-	ecc_strength = of_get_nand_ecc_strength(dn);
-	ecc_step = of_get_nand_ecc_step_size(dn);
-
-	if (ecc_type != NAND_ECC_ENGINE_TYPE_INVALID)
-		chip->ecc.engine_type = ecc_type;
-
-	if (ecc_algo != NAND_ECC_ALGO_UNKNOWN)
-		chip->ecc.algo = ecc_algo;
-
-	if (ecc_strength >= 0)
-		chip->ecc.strength = ecc_strength;
-
-	if (ecc_step > 0)
-		chip->ecc.size = ecc_step;
-
 	if (of_property_read_bool(dn, "nand-ecc-maximize"))
 		chip->ecc.options |= NAND_ECC_MAXIMIZE;
 
+	of_get_nand_ecc_user_config(nand);
+	of_get_nand_ecc_legacy_user_config(chip);
+
+	/*
+	 * If neither the user nor the NAND controller have requested a specific
+	 * ECC engine type, we will default to NAND_ECC_ENGINE_TYPE_ON_HOST.
+	 */
+	nand->ecc.defaults.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
+
+	/*
+	 * Use the user requested engine type, unless there is none, in this
+	 * case default to the NAND controller choice, otherwise fallback to
+	 * the raw NAND default one.
+	 */
+	if (nand->ecc.user_conf.engine_type != NAND_ECC_ENGINE_TYPE_INVALID)
+		chip->ecc.engine_type = nand->ecc.user_conf.engine_type;
+	if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_INVALID)
+		chip->ecc.engine_type = nand->ecc.defaults.engine_type;
+
+	chip->ecc.placement = nand->ecc.user_conf.placement;
+	chip->ecc.algo = nand->ecc.user_conf.algo;
+	chip->ecc.strength = nand->ecc.user_conf.strength;
+	chip->ecc.size = nand->ecc.user_conf.step_size;
+
 	return 0;
 }
 
@@ -5082,7 +5080,7 @@  static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
 	/* Enforce the right timings for reset/detection */
 	chip->current_interface_config = nand_get_reset_interface_config();
 
-	ret = nand_dt_init(chip);
+	ret = rawnand_dt_init(chip);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 1bbce6fa1b08..671e60948deb 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -81,18 +81,6 @@  struct nand_chip;
 
 #define NAND_DATA_IFACE_CHECK_ONLY	-1
 
-/*
- * Constants for ECC_MODES
- */
-enum nand_ecc_mode {
-	NAND_ECC_INVALID,
-	NAND_ECC_NONE,
-	NAND_ECC_SOFT,
-	NAND_ECC_HW,
-	NAND_ECC_HW_SYNDROME,
-	NAND_ECC_ON_DIE,
-};
-
 /*
  * Constants for Hardware ECC
  */