On 18/04/16 18:05, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 17:32:49 +0300
> Roger Quadros <rogerq@xxxxxx> wrote:
>
>> Boris,
>>
>> On 30/03/16 19:14, Boris Brezillon wrote:
>>> Implementing the mtd_ooblayout_ops interface is the new way of exposing
>>> ECC/OOB layout to MTD users.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/mtd/nand/omap2.c | 194 +++++++++++++++++++++++++++--------------------
>>> 1 file changed, 113 insertions(+), 81 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>>> index 4ebf16b..bca154a 100644
>>> --- a/drivers/mtd/nand/omap2.c
>>> +++ b/drivers/mtd/nand/omap2.c
>>> @@ -169,8 +169,6 @@ struct omap_nand_info {
>>> u_char *buf;
>>> int buf_len;
>>> struct gpmc_nand_regs reg;
>>> - /* generated at runtime depending on ECC algorithm and layout selected */
>>> - struct nand_ecclayout oobinfo;
>>> /* fields specific for BCHx_HW ECC scheme */
>>> struct device *elm_dev;
>>> struct device_node *of_node;
>>> @@ -1639,19 +1637,108 @@ static bool omap2_nand_ecc_check(struct omap_nand_info *info,
>>> return true;
>>> }
>>>
>>> +static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
>>> + struct mtd_oob_region *oobregion)
>>> +{
>>> + struct nand_chip *chip = mtd_to_nand(mtd);
>>> + int off = chip->options & NAND_BUSWIDTH_16 ?
>>> + BADBLOCK_MARKER_LENGTH : 1;
>>
>> IMO "off = 1" is valid only for OMAP_ECC_HAM1_CODE_HW and 8-bit NAND.
>> For all other layouts it is always set to BADBLOCK_MARKER_LENGTH.
>
> Indeed.
>
> [...]
>
>>> - /* all OOB bytes from oobfree->offset till end off OOB are free */
>>> - ecclayout->oobfree->length = mtd->oobsize - ecclayout->oobfree->offset;
>>> /* check if NAND device's OOB is enough to store ECC signatures */
>>> - if (mtd->oobsize < (ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH)) {
>>> + min_oobbytes = (oobbytes_per_step *
>>> + (mtd->writesize / nand_chip->ecc.size)) +
>>> + (nand_chip->options & NAND_BUSWIDTH_16 ?
>>> + BADBLOCK_MARKER_LENGTH : 1);
>>
>> would it affect this as well?
>
> And here as well.
>
> I've generated a patch (see below) fixing those problems.
>
>>
>>> + if (mtd->oobsize < min_oobbytes) {
>>> dev_err(&info->pdev->dev,
>>> "not enough OOB bytes required = %d, available=%d\n",
>>> - ecclayout->eccbytes, mtd->oobsize);
>>> + min_oobbytes, mtd->oobsize);
>>> err = -EINVAL;
>>> goto return_error;
>>> }
>>>
>>
>> I will need to test this change with all possible configurations.
>> The number of configurations on OMAP is a bit overwhelming :(.
>
> Sorry about that, but at least now I have someone who can test it :).
>
> Thanks,
>
> Boris
>
> --->8---
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 9b96f56..07d4039 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1640,9 +1640,13 @@ static bool omap2_nand_ecc_check(struct omap_nand_info *info,
> static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
> struct mtd_oob_region *oobregion)
> {
> - struct nand_chip *chip = mtd_to_nand(mtd);
> - int off = chip->options & NAND_BUSWIDTH_16 ?
> - BADBLOCK_MARKER_LENGTH : 1;
> + struct omap_nand_info *info = mtd_to_omap(mtd);
> + struct nand_chip *chip = &info->nand;
> + int off = BADBLOCK_MARKER_LENGTH;
> +
> + if (info->ecc_opt == OMAP_ECC_HAM1_CODE_HW &&
> + !(chip->options & NAND_BUSWIDTH_16))
> + off = 1;
>
> if (section)
> return -ERANGE;
> @@ -1656,9 +1660,13 @@ static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
> static int omap_ooblayout_free(struct mtd_info *mtd, int section,
> struct mtd_oob_region *oobregion)
> {
> - struct nand_chip *chip = mtd_to_nand(mtd);
> - int off = chip->options & NAND_BUSWIDTH_16 ?
> - BADBLOCK_MARKER_LENGTH : 1;
> + struct omap_nand_info *info = mtd_to_omap(mtd);
> + struct nand_chip *chip = &info->nand;
> + int off = BADBLOCK_MARKER_LENGTH;
> +
> + if (info->ecc_opt == OMAP_ECC_HAM1_CODE_HW &&
> + !(chip->options & NAND_BUSWIDTH_16))
> + off = 1;
>
> if (section)
> return -ERANGE;
> @@ -1682,8 +1690,7 @@ static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
> struct mtd_oob_region *oobregion)
> {
> struct nand_chip *chip = mtd_to_nand(mtd);
> - int off = chip->options & NAND_BUSWIDTH_16 ?
> - BADBLOCK_MARKER_LENGTH : 1;
> + int off = BADBLOCK_MARKER_LENGTH;
>
> if (section >= chip->ecc.steps)
> return -ERANGE;
> @@ -1702,8 +1709,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
> struct mtd_oob_region *oobregion)
> {
> struct nand_chip *chip = mtd_to_nand(mtd);
> - int off = chip->options & NAND_BUSWIDTH_16 ?
> - BADBLOCK_MARKER_LENGTH : 1;
> + int off = BADBLOCK_MARKER_LENGTH;
>
> if (section)
> return -ERANGE;
> @@ -1737,7 +1743,7 @@ static int omap_nand_probe(struct platform_device *pdev)
> dma_cap_mask_t mask;
> unsigned sig;
> struct resource *res;
> - int min_oobbytes;
> + int min_oobbytes = BADBLOCK_MARKER_LENGTH;
> int oobbytes_per_step;
>
> pdata = dev_get_platdata(&pdev->dev);
> @@ -1921,6 +1927,9 @@ static int omap_nand_probe(struct platform_device *pdev)
> nand_chip->ecc.correct = omap_correct_data;
> mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
> oobbytes_per_step = nand_chip->ecc.bytes;
> +
> + if (nand_chip->options & NAND_BUSWIDTH_16)
> + min_oobbytes = 1;
Shouldn't this have been
if (!(nand_chip->options & NAND_BUSWIDTH_16)
min_oobbytes = 1;
?
> break;
>
> case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
> @@ -2038,10 +2047,8 @@ static int omap_nand_probe(struct platform_device *pdev)
> }
>
> /* check if NAND device's OOB is enough to store ECC signatures */
> - min_oobbytes = (oobbytes_per_step *
> - (mtd->writesize / nand_chip->ecc.size)) +
> - (nand_chip->options & NAND_BUSWIDTH_16 ?
> - BADBLOCK_MARKER_LENGTH : 1);
> + min_oobbytes += (oobbytes_per_step *
> + (mtd->writesize / nand_chip->ecc.size));
> if (mtd->oobsize < min_oobbytes) {
> dev_err(&info->pdev->dev,
> "not enough OOB bytes required = %d, available=%d\n",
>
After the above changes BCH with HW ECC worked fine but BCH with SW ECC still failed.
I had to fix it up with the below patch. This is mainly because chip->ecc.steps wasn't
yet initialized before calling nand_bch_init().
After the below patch it worked fine with bch4 (hw & sw), bch8 (hw & sw) and ham1.
I couldn't yet verify bch16 though.
--cheers,
-roger
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index db22c01..1b1a804 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1692,7 +1692,7 @@ static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
struct nand_chip *chip = mtd_to_nand(mtd);
int off = BADBLOCK_MARKER_LENGTH;
- if (section >= chip->ecc.steps)
+ if (section >= mtd->writesize / chip->ecc.size) /* ecc steps */
return -ERANGE;
/*
@@ -1711,7 +1711,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
struct nand_chip *chip = mtd_to_nand(mtd);
int off = BADBLOCK_MARKER_LENGTH;
- if (section)
+ if (section >= mtd->writesize / chip->ecc.size) /* ecc steps */
return -ERANGE;
/*
--
2.5.0
