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

On 18/04/16 17:10, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 16:48:26 +0300

> Roger Quadros <rogerq@ti.com> 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@ti.com> 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@atomide.com> 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.

--
cheers,
-roger