Re: [PATCH v2 3/4] mtd: nand: pxa3xx: Use ECC strength and step size devicetree binding
From: Brian Norris <hidden>
Date: 2014-05-12 18:01:04
Hi Ezequiel, On Fri, Mar 21, 2014 at 08:34:49AM -0300, Ezequiel Garcia wrote:
This commit adds support for the user to specify the ECC strength and step size through the devicetree. We keep the previous behavior, when there is no DT parameter provided. Also, if there is an ONFI-specified minimum ECC strength requirement, and the DT-specified ECC strength is weaker, print a warning and try to select a strength compatible with the ONFI requirement.
Are you sure you want to override? That seems contrary to the idea of a DT property for specifying ECC. But maybe you have a good reason? If you'd rather just warn the user, it's possible we could move that to common code in nand_base.c.
quoted hunk
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --- drivers/mtd/nand/pxa3xx_nand.c | 47 +++++++++++++++++++-------- include/linux/platform_data/mtd-nand-pxa3xx.h | 3 ++ 2 files changed, 36 insertions(+), 14 deletions(-)diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index cf7d757..cc13896 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c@@ -1344,8 +1344,30 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info) } static int pxa_ecc_init(struct pxa3xx_nand_info *info, - struct nand_ecc_ctrl *ecc, int strength, int page_size) + struct nand_chip *chip, + struct pxa3xx_nand_platform_data *pdata, + unsigned int page_size) { + struct nand_ecc_ctrl *ecc = &chip->ecc; + unsigned int strength, onfi_strength; + + /* We need to normalize the ECC strengths, in order to compare them. */ + strength = (pdata->ecc_strength * 512) / pdata->ecc_step_size; + onfi_strength = (chip->ecc_strength_ds * 512) / chip->ecc_step_ds;
This is not necessarily an "ONFI" property; we also have the full-ID table in which a minimum ECC strength can be specified. Maybe you can match the existing naming with "strength" and "strength_ds" (for "datasheet").
+
+ /*
+ * The user requested ECC strength cannot be weaker than the ONFI
+ * minimum specified ECC strength.
+ */
+ if (strength && strength < onfi_strength) {
+ dev_warn(&info->pdev->dev,
+ "requested ECC strength is too weak\n");
+ strength = onfi_strength;
+ }
+
+ if (!strength)
+ strength = onfi_strength ? onfi_strength : 1;
+
I'm also not sure your "normalization" is generally correct. You can't
really normalize correction strengths to get a fully valid comparison.
Remember, 4-bit/2048B correction is not the same as 1-bit/512B
correction; it is at least as strong, yes. But then, the reverse is
*not* a good comparison. So your code might say that a flash which
requires 4-bit/2KB can be satisfied by 1-bit/512B. (These numbers may
not be seen in practice, but you get my point, right?)
By my understanding, a correct comparison requires two parts, which I've
summarized like this in my own code:
/*
* Does the given configuration meet the requirements?
*
* If our configuration corrects A bits per B bytes and the minimum
* required correction level is X bits per Y bytes, then we must ensure
* both of the following are true:
*
* (1) A / B >= X / Y
* (2) A >= X
*
* Requirement (1) ensures we can correct for the required bitflip
* density.
* Requirement (2) ensures we can correct even when all bitflips are
* clumped in the same sector.
*/
I think you are doing (1), but not (2).
quoted hunk
if (strength == 1 && page_size == 2048) { info->chunk_size = 2048; info->spare_size = 40;@@ -1375,7 +1397,6 @@ static int pxa_ecc_init(struct pxa3xx_nand_info *info, ecc->size = info->chunk_size; ecc->layout = &ecc_layout_2KB_bch4bit; ecc->strength = 16; - return 1; } else if (strength == 4 && page_size == 4096) { info->ecc_bch = 1;@@ -1424,7 +1445,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd) uint32_t id = -1; uint64_t chipsize; int i, ret, num; - uint16_t ecc_strength, ecc_step; if (pdata->keep_config && !pxa3xx_nand_detect_config(info)) goto KEEP_CONFIG;@@ -1519,17 +1539,8 @@ KEEP_CONFIG: } } - ecc_strength = chip->ecc_strength_ds; - ecc_step = chip->ecc_step_ds; - - /* Set default ECC strength requirements on non-ONFI devices */ - if (ecc_strength < 1 && ecc_step < 1) { - ecc_strength = 1; - ecc_step = 512; - } - - ret = pxa_ecc_init(info, &chip->ecc, (ecc_strength * 512) / ecc_step, - mtd->writesize); + /* Selects an ECC and fills pxa3xx_nand_info and nand_chip */ + ret = pxa_ecc_init(info, chip, pdata, mtd->writesize); if (ret) return ret;@@ -1729,6 +1740,14 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev) of_property_read_u32(np, "num-cs", &pdata->num_cs); pdata->flash_bbt = of_get_nand_on_flash_bbt(np); + pdata->ecc_strength = of_get_nand_ecc_strength(np); + if (pdata->ecc_strength < 0)
ecc_strength is unsigned, so this error check doesn't work. Maybe you want a local 'int ret' to temporarily stash the return value?
+ pdata->ecc_strength = 0; + + pdata->ecc_step_size = of_get_nand_ecc_step_size(np); + if (pdata->ecc_step_size < 0)
Ditto for ecc_step_size.
quoted hunk
+ pdata->ecc_step_size = 0; + pdev->dev.platform_data = pdata; return 0;diff --git a/include/linux/platform_data/mtd-nand-pxa3xx.h b/include/linux/platform_data/mtd-nand-pxa3xx.h index a941471..5c0f2e3 100644 --- a/include/linux/platform_data/mtd-nand-pxa3xx.h +++ b/include/linux/platform_data/mtd-nand-pxa3xx.h@@ -58,6 +58,9 @@ struct pxa3xx_nand_platform_data { /* use an flash-based bad block table */ bool flash_bbt; + /* requested ECC strength and ECC step size */ + unsigned int ecc_strength, ecc_step_size; + const struct mtd_partition *parts[NUM_CHIP_SELECT]; unsigned int nr_parts[NUM_CHIP_SELECT];
Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html