| Message ID | 1402041728-12056-1-git-send-email-voice.shen@atmel.com |
|---|---|
| State | Accepted |
| Commit | b38576667cdcf7a646843b07666faae11cc63e9c |
| Headers | show |
Hi, Bo Thanks for the patch. On 6/6/2014 4:02 PM, Bo Shen wrote: > If the ecc parameter is not the same as definition, when the > mtd core check these parameters, it will give the unexpected > result. Could you be more specific to tell what kind of result will happened? Best Regards, Josh Wu > > Signed-off-by: Bo Shen <voice.shen@atmel.com> > --- > drivers/mtd/nand/atmel_nand.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > index 4ce181a..4f5348f 100644 > --- a/drivers/mtd/nand/atmel_nand.c > +++ b/drivers/mtd/nand/atmel_nand.c > @@ -861,12 +861,11 @@ static int pmecc_correction(struct mtd_info *mtd, u32 pmecc_stat, uint8_t *buf, > { > struct nand_chip *nand_chip = mtd->priv; > struct atmel_nand_host *host = nand_chip->priv; > - int i, err_nbr, eccbytes; > + int i, err_nbr; > uint8_t *buf_pos; > int total_err = 0; > > - eccbytes = nand_chip->ecc.bytes; > - for (i = 0; i < eccbytes; i++) > + for (i = 0; i < nand_chip->ecc.total; i++) > if (ecc[i] != 0xff) > goto normal_check; > /* Erased page, return OK */ > @@ -928,7 +927,7 @@ static int atmel_nand_pmecc_read_page(struct mtd_info *mtd, > struct nand_chip *chip, uint8_t *buf, int oob_required, int page) > { > struct atmel_nand_host *host = chip->priv; > - int eccsize = chip->ecc.size; > + int eccsize = chip->ecc.size * chip->ecc.steps; > uint8_t *oob = chip->oob_poi; > uint32_t *eccpos = chip->ecc.layout->eccpos; > uint32_t stat; > @@ -1169,8 +1168,7 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev, > goto err; > } > > - /* ECC is calculated for the whole page (1 step) */ > - nand_chip->ecc.size = mtd->writesize; > + nand_chip->ecc.size = sector_size; > > /* set ECC page size and oob layout */ > switch (mtd->writesize) { > @@ -1185,18 +1183,20 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev, > host->pmecc_index_of = host->pmecc_rom_base + > host->pmecc_lookup_table_offset; > > - nand_chip->ecc.steps = 1; > + nand_chip->ecc.steps = host->pmecc_sector_number; > nand_chip->ecc.strength = cap; > - nand_chip->ecc.bytes = host->pmecc_bytes_per_sector * > + nand_chip->ecc.bytes = host->pmecc_bytes_per_sector; > + nand_chip->ecc.total = host->pmecc_bytes_per_sector * > host->pmecc_sector_number; > - if (nand_chip->ecc.bytes > mtd->oobsize - 2) { > + if (nand_chip->ecc.total > mtd->oobsize - 2) { > dev_err(host->dev, "No room for ECC bytes\n"); > err_no = -EINVAL; > goto err; > } > pmecc_config_ecc_layout(&atmel_pmecc_oobinfo, > mtd->oobsize, > - nand_chip->ecc.bytes); > + nand_chip->ecc.total); > + > nand_chip->ecc.layout = &atmel_pmecc_oobinfo; > break; > case 512:
Hi Josh, On 06/09/2014 06:30 PM, Josh Wu wrote: > Hi, Bo > > Thanks for the patch. > > On 6/6/2014 4:02 PM, Bo Shen wrote: >> If the ecc parameter is not the same as definition, when the >> mtd core check these parameters, it will give the unexpected >> result. > Could you be more specific to tell what kind of result will happened? For example: When try to calculate how many bits can correct in one page according to the definition, it will give the wrong result. one page correct bits = (mtd->writesize * ecc->strength) / ecc->size; take the following use case as an example: mtd->writesize = 2048 bytes, ecc->strength = 4 bytes, (for 512 bytes) before this patch, the ecc->size = 2048, so the result is 4 bytes. after this patch, the ecc->size = 512, so the result is 16 bytes. The maximum correct bytes is 16 bytes while not 4 bytes. So, if not align Best Regards, Bo Shen
Hi, Bo On 6/11/2014 11:24 AM, Bo Shen wrote: > Hi Josh, > > On 06/09/2014 06:30 PM, Josh Wu wrote: >> Hi, Bo >> >> Thanks for the patch. >> >> On 6/6/2014 4:02 PM, Bo Shen wrote: >>> If the ecc parameter is not the same as definition, when the >>> mtd core check these parameters, it will give the unexpected >>> result. >> Could you be more specific to tell what kind of result will happened? > > For example: > When try to calculate how many bits can correct in one page according > to the definition, it will give the wrong result. > > one page correct bits = (mtd->writesize * ecc->strength) / ecc->size; > > take the following use case as an example: > mtd->writesize = 2048 bytes, > ecc->strength = 4 bytes, (for 512 bytes) > > before this patch, the ecc->size = 2048, so the result is 4 bytes. > > after this patch, the ecc->size = 512, so the result is 16 bytes. > > The maximum correct bytes is 16 bytes while not 4 bytes. So, if not align That is clear enough, thanks. The patch is fine to me. so Acked-by: Josh Wu <josh.wu@atmel.com> > > Best Regards, > Bo Shen Best Regards, Josh Wu
On 12/06/2014 09:20, Josh Wu : > Hi, Bo > > On 6/11/2014 11:24 AM, Bo Shen wrote: >> Hi Josh, >> >> On 06/09/2014 06:30 PM, Josh Wu wrote: >>> Hi, Bo >>> >>> Thanks for the patch. >>> >>> On 6/6/2014 4:02 PM, Bo Shen wrote: >>>> If the ecc parameter is not the same as definition, when the >>>> mtd core check these parameters, it will give the unexpected >>>> result. >>> Could you be more specific to tell what kind of result will happened? >> >> For example: >> When try to calculate how many bits can correct in one page according >> to the definition, it will give the wrong result. >> >> one page correct bits = (mtd->writesize * ecc->strength) / ecc->size; >> >> take the following use case as an example: >> mtd->writesize = 2048 bytes, >> ecc->strength = 4 bytes, (for 512 bytes) >> >> before this patch, the ecc->size = 2048, so the result is 4 bytes. >> >> after this patch, the ecc->size = 512, so the result is 16 bytes. >> >> The maximum correct bytes is 16 bytes while not 4 bytes. So, if not align > > That is clear enough, thanks. The patch is fine to me. so > Acked-by: Josh Wu <josh.wu@atmel.com> So I advice to add this additional information to the commit message: Can you send a v2 with enhanced commit message. Thanks, bye,
Hi Nicolas, On 06/12/2014 03:26 PM, Nicolas Ferre wrote: > On 12/06/2014 09:20, Josh Wu : >> Hi, Bo >> >> On 6/11/2014 11:24 AM, Bo Shen wrote: >>> Hi Josh, >>> >>> On 06/09/2014 06:30 PM, Josh Wu wrote: >>>> Hi, Bo >>>> >>>> Thanks for the patch. >>>> >>>> On 6/6/2014 4:02 PM, Bo Shen wrote: >>>>> If the ecc parameter is not the same as definition, when the >>>>> mtd core check these parameters, it will give the unexpected >>>>> result. >>>> Could you be more specific to tell what kind of result will happened? >>> >>> For example: >>> When try to calculate how many bits can correct in one page according >>> to the definition, it will give the wrong result. >>> >>> one page correct bits = (mtd->writesize * ecc->strength) / ecc->size; >>> >>> take the following use case as an example: >>> mtd->writesize = 2048 bytes, >>> ecc->strength = 4 bytes, (for 512 bytes) >>> >>> before this patch, the ecc->size = 2048, so the result is 4 bytes. >>> >>> after this patch, the ecc->size = 512, so the result is 16 bytes. >>> >>> The maximum correct bytes is 16 bytes while not 4 bytes. So, if not align >> >> That is clear enough, thanks. The patch is fine to me. so >> Acked-by: Josh Wu <josh.wu@atmel.com> > > So I advice to add this additional information to the commit message: > Can you send a v2 with enhanced commit message. OK, no problem. > Thanks, bye, > Best Regards, Bo Shen
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index 4ce181a..4f5348f 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -861,12 +861,11 @@ static int pmecc_correction(struct mtd_info *mtd, u32 pmecc_stat, uint8_t *buf, { struct nand_chip *nand_chip = mtd->priv; struct atmel_nand_host *host = nand_chip->priv; - int i, err_nbr, eccbytes; + int i, err_nbr; uint8_t *buf_pos; int total_err = 0; - eccbytes = nand_chip->ecc.bytes; - for (i = 0; i < eccbytes; i++) + for (i = 0; i < nand_chip->ecc.total; i++) if (ecc[i] != 0xff) goto normal_check; /* Erased page, return OK */ @@ -928,7 +927,7 @@ static int atmel_nand_pmecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { struct atmel_nand_host *host = chip->priv; - int eccsize = chip->ecc.size; + int eccsize = chip->ecc.size * chip->ecc.steps; uint8_t *oob = chip->oob_poi; uint32_t *eccpos = chip->ecc.layout->eccpos; uint32_t stat; @@ -1169,8 +1168,7 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev, goto err; } - /* ECC is calculated for the whole page (1 step) */ - nand_chip->ecc.size = mtd->writesize; + nand_chip->ecc.size = sector_size; /* set ECC page size and oob layout */ switch (mtd->writesize) { @@ -1185,18 +1183,20 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev, host->pmecc_index_of = host->pmecc_rom_base + host->pmecc_lookup_table_offset; - nand_chip->ecc.steps = 1; + nand_chip->ecc.steps = host->pmecc_sector_number; nand_chip->ecc.strength = cap; - nand_chip->ecc.bytes = host->pmecc_bytes_per_sector * + nand_chip->ecc.bytes = host->pmecc_bytes_per_sector; + nand_chip->ecc.total = host->pmecc_bytes_per_sector * host->pmecc_sector_number; - if (nand_chip->ecc.bytes > mtd->oobsize - 2) { + if (nand_chip->ecc.total > mtd->oobsize - 2) { dev_err(host->dev, "No room for ECC bytes\n"); err_no = -EINVAL; goto err; } pmecc_config_ecc_layout(&atmel_pmecc_oobinfo, mtd->oobsize, - nand_chip->ecc.bytes); + nand_chip->ecc.total); + nand_chip->ecc.layout = &atmel_pmecc_oobinfo; break; case 512:
If the ecc parameter is not the same as definition, when the mtd core check these parameters, it will give the unexpected result. Signed-off-by: Bo Shen <voice.shen@atmel.com> --- drivers/mtd/nand/atmel_nand.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)