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

[Roger Quadros]

Boris,

On 18/04/16 17:57, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 17:39:01 +0300
> Roger Quadros <rogerq@xxxxxx> wrote:
> 
>> On 18/04/16 17:10, Boris Brezillon wrote:
>>> On Mon, 18 Apr 2016 16:48:26 +0300
>>> Roger Quadros <rogerq@xxxxxx> wrote:
>>>
>>>> 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.
>>>
>>> Ok, I think I got it: the ECC correction is pipelined with data read,
>>> and the controller expect to have ECC bytes right after the in-band
>>> data, is that correct?
>>
>> That is correct.
>>
>>>
>>>>
>>>>>>
>>>>>> 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?
>>>
>>> For the BCH case, it seems that ECC bytes always start at offset
>>> BADBLOCK_MARKER_LENGTH, so you can just pass
>>> mtd->writesize + BADBLOCK_MARKER_LENGTH.
>>>
>>> Let me know if this works, and I'll squash those changes into the
>>> faulty commit (I know this implies a rebase + push -f, but IMO that's
>>> better than breaking bisectability).
>>>
>>>
>>
>> So, the below patch works as well. Please feel free to fold it with your patch.
> 
> Will do.
> 
> Thanks,
> 
> Boris

After all the changes we discussed in [1] I was able to test nand/next-with-gpmc-rework
and it worked fine.

[1] - http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/22596/focus=22936

I'd be happy to test the branch again after you've incorporated all changes.

Since you are gong to do a push -f anyways, I was wondering if you want to pull in my
gpmc branch first to avoid the merge conflict. But it is totally up to you.

--
cheers,
-roger