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. > > Signed-off-by: Ezequiel Garcia <[email protected]> > --- > 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). > 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. > + 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
