| Message ID | 1395401690-25221-4-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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@free-electrons.com> > --- > 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
Thanks for looking at this. On 12 May 11:01 AM, Brian Norris wrote: > 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? > Actually, on IRC discussions you enforced the idea that the kernel shouldn't allow a weaker ECC strength than the datasheet (ONFI or ID table) specified. Hence, following your request, the implementation considers such devicetree setting as illegal and tries to find another one. > If you'd rather just warn the user, it's possible we could move that to > common code in nand_base.c. > Now that you mention this, I think you're right: it's very stupid to try to match an ECC scheme, different from the requested. So, we either just warn the user (nosily) or we fail to probe the driver. If you ask me, I'd say let's just WARN() and let the user take the blame. > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > 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"). > Right. > > + > > + /* > > + * 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. So far so good. > 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?) > Ah... missed the reverse. > 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). > Great catch! I missed this detail entirely. > > 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? > Correct. I'll prepare new patchset. Thanks!
On Mon, May 12, 2014 at 04:18:41PM -0300, Ezequiel Garcia wrote: > On 12 May 11:01 AM, Brian Norris wrote: > > On Fri, Mar 21, 2014 at 08:34:49AM -0300, Ezequiel Garcia wrote: > > > 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? > > > > Actually, on IRC discussions you enforced the idea that the kernel shouldn't > allow a weaker ECC strength than the datasheet (ONFI or ID table) specified. > Hence, following your request, the implementation considers such devicetree > setting as illegal and tries to find another one. Hmm, I don't recall saying that exactly, although I might have said something similar that could have been misconstrued as such. I mostly recall discussing setting the ECC strength *without* device tree (as pxa3xx_nand currently does), which is a different case, since we don't have any outside input. But anyway... > > If you'd rather just warn the user, it's possible we could move that to > > common code in nand_base.c. > > > > Now that you mention this, I think you're right: it's very stupid to try to > match an ECC scheme, different from the requested. > > So, we either just warn the user (nosily) or we fail to probe the driver. > If you ask me, I'd say let's just WARN() and let the user take the blame. ...yes, I (regardless of what "IRC Brian" said!) think that is the correct approach when given an ECC scheme via device-tree. We should honor it, and blame the user loudly. Brian
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; + + /* + * 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; + 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) + pdata->ecc_strength = 0; + + pdata->ecc_step_size = of_get_nand_ecc_step_size(np); + if (pdata->ecc_step_size < 0) + 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];
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. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/mtd/nand/pxa3xx_nand.c | 47 +++++++++++++++++++-------- include/linux/platform_data/mtd-nand-pxa3xx.h | 3 ++ 2 files changed, 36 insertions(+), 14 deletions(-)