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 <ezequiel.garcia@xxxxxxxxxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
