Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops
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
> ---
> 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.
> +
> + if (section)
> + return -ERANGE;
> +
> + oobregion->offset = off;
> + oobregion->length = chip->ecc.total;
> +
> + return 0;
> +}
> +
> +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;
ditto.
> +
> + if (section)
> + return -ERANGE;
> +
> + off += chip->ecc.total;
> + if (off >= mtd->oobsize)
> + return -ERANGE;
> +
> + oobregion->offset = off;
> + oobregion->length = mtd->oobsize - off;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops omap_ooblayout_ops = {
> + .ecc = omap_ooblayout_ecc,
> + .free = omap_ooblayout_free,
> +};
> +
> +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;
> +
> + if (section >= chip->ecc.steps)
> + return -ERANGE;
> +
> + /*
> + * When SW correction is employed, one OMAP specific marker byte is
> + * reserved after each ECC step.
> + */
> + oobregion->offset = off + (section * (chip->ecc.bytes + 1));
> + oobregion->length = chip->ecc.bytes;
> +
> + return 0;
> +}
> +
> +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;
> +
> + if (section)
> + return -ERANGE;
> +
> + /*
> + * When SW correction is employed, one OMAP specific marker byte is
> + * reserved after each ECC step.
> + */
> + off += ((chip->ecc.bytes + 1) * chip->ecc.steps);
> + if (off >= mtd->oobsize)
> + return -ERANGE;
> +
> + oobregion->offset = off;
> + oobregion->length = mtd->oobsize - off;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops omap_sw_ooblayout_ops = {
> + .ecc = omap_sw_ooblayout_ecc,
> + .free = omap_sw_ooblayout_free,
> +};
> +
> static int omap_nand_probe(struct platform_device *pdev)
> {
> struct omap_nand_info *info;
> struct omap_nand_platform_data *pdata;
> struct mtd_info *mtd;
> struct nand_chip*nand_chip;
> - struct nand_ecclayout *ecclayout;
> int err;
> - int i;
> dma_cap_mask_t mask;
> unsignedsig;
> - unsignedoob_index;
> struct resource *res;
> + int min_oobbytes;
> + int oobbytes_per_step;
>
> pdata = dev_get_platdata(&pdev->dev);
> if (pdata == NULL) {
> @@ -1810,7 +1897,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>
> /*
>* Bail out earlier to let NAND_ECC_SOFT code create its own
> - * ecclayout instead of using ours.
> + * ooblayout instead of using ours.
Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops
On 18/04/16 18:05, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 17:32:49 +0300
> Roger Quadros 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
>>> ---
>>> 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
Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops
On 19/04/16 14:22, Boris Brezillon wrote:
> Hi Roger,
>
> On Tue, 19 Apr 2016 13:28:50 +0300
> Roger Quadros wrote:
>
>>> @@ -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;
>> ?
>
> Yep.
>
>>
>>> 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.
>
I just verified that bch16 works as well.
> Thanks for the fix, but I'd prefer fixing the bug for all soft BCH
> users.
>
> Could you try this patch?
I tried your patch and it worked fine.
You will still need the below change to omap2.c
--
cheers,
-roger
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0abfba6..33c8fde 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1715,7 +1715,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 >= chip->ecc.steps)
return -ERANGE;
/*
--
2.5.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops
On 19/04/16 15:41, Boris Brezillon wrote:
> On Tue, 19 Apr 2016 15:30:39 +0300
> Roger Quadros wrote:
>
>> On 19/04/16 14:22, Boris Brezillon wrote:
>>> Hi Roger,
>>>
>>> On Tue, 19 Apr 2016 13:28:50 +0300
>>> Roger Quadros wrote:
>>>
>>>>> @@ -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;
>>>> ?
>>>
>>> Yep.
>>>
>>>>
>>>>> 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.
>>>
>>
>> I just verified that bch16 works as well.
>>
>>> Thanks for the fix, but I'd prefer fixing the bug for all soft BCH
>>> users.
>>>
>>> Could you try this patch?
>>
>> I tried your patch and it worked fine.
>
> Thanks, I'll provide a reworked nand/next branch soon.
> BTW, is there anything to fix in my merge commit (the commit merging
> your GPMC/OMAP changes in nand/next)?
>
I just replied in the other thread that the conflict resolution is fine.
>> You will still need the below change to omap2.c
>>
>> --
>> cheers,
>> -roger
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 0abfba6..33c8fde 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -1715,7 +1715,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 >= chip->ecc.steps)
>> return -ERANGE;
>
> Sorry but I don't get why we need that one. Don't we have a single
> oobfree section starting at the end of the ECC sections?
>
>
You are right. Nothing needs to be changed there then. Thanks :)
cheers,
-roger
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
