Re: [PATCH v6 00/17] memory: omap-gpmc: mtd: nand: Support GPMC NAND on non-OMAP platforms

[Roger Quadros]

Boris,

On 18/04/16 16:13, Boris Brezillon wrote:
> Hi Roger,
> 
> On Mon, 18 Apr 2016 15:52:58 +0300
> Roger Quadros <rogerq@xxxxxx> wrote:
> 
>> On 18/04/16 15:31, Roger Quadros wrote:
>>> On 16/04/16 11:57, Boris Brezillon wrote:
>>>> On Fri, 15 Apr 2016 09:19:51 -0700
>>>> Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>>>>
>>>>>
>>>>>> Or should I just pull this immutable branch in my current nand/next and
>>>>>> let you pull the same immutable branch in omap-soc. I mean, would this
>>>>>> prevent conflicts when our branches are merged into linux-next, no
>>>>>> matter the order.
>>>>>
>>>>> Ideally just one or more branches with just minimal changes in
>>>>> them against -rc1. But you may have other dependencies in
>>>>> your NAND tree so that may no longer be doable :) Usually if
>>>>> I merge something that may need to get merged into other
>>>>> branches, I just apply them into a separate branch against -rc1
>>>>> to start with, then merge that branch in.
>>>>
>>>> Okay, in this case, that's pretty much what I did from the beginning,
>>>> except the immutable branch was provided by Roger (based on 4.6-rc1).
>>>> Thanks for this detailed explanation, I'll try to remember that when
>>>> I'll need to provide an immutable branch for another subsystem.
>>>>
>>>> Roger, my request remains, could you check/test my conflict resolution
>>>> (branch nand/next-with-gpmc-rework)?
>>>
>>> I couldn't test that branch yet as nand/next is broken on omap platforms
>>> (at least on dra7-evm).
>>>
>>> The commit where it breaks is:
>>> a662ef4 mtd: nand: omap2: use mtd_ooblayout_xxx() helpers where appropriate
>>>
>>> I'm trying to figure out what went wrong there. Failure log below.
>>
>> OK. I was able to fix it when at commit a662ef4 with the below patch.
> 
> Thanks for debugging that.
> 
>>
>> Looks like we need to read exactly the ECC bytes through the ECC engine and not
>> the entire OOB region.
> 
> Hm, it looks like there's a bug somewhere else, because I don't see any
> reason why the controller wouldn't be able to read the full OOB region.

The controller can read the full OOB region but we only want it to read just
the ECC bytes because that is the way the ELM ECC engine works.

>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index e622a1b..46b61d2 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -1547,8 +1547,8 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
>>  	chip->read_buf(mtd, buf, mtd->writesize);
>>  
>>  	/* Read oob bytes */
>> -	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize, -1);
>> -	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +	chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize + chip->ecc.layout->eccpos[0], -1);
> 
> The whole point of this series is to get rid of chip->ecc.layout, so
> we'd rather use the mtd_ooblayout_find_eccregion() instead of
> chip->ecc.layout->eccpos[0].

We just need the position of the first ECC byte offset.
Is that the most optimal way to get it?

> 
>> +	chip->read_buf(mtd, chip->oob_poi, chip->ecc.total);
> 
> Can you print the ->oobsize, ->writesize, chip->ecc.layout->eccpos[0]
> and chip->ecc.total values here. I'll also need your NAND page layout
> (page size and OOB size provided in the datasheet).

eccpos[0]: 2, oobsize 64, ecctotal 56, writesize 2048

Nand part is MT29F2G16ABAEAWP
This has page size 2048 bytes and OOB size 64 bytes.

cheers,
-roger