From patchwork Mon Apr 18 14:39:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roger Quadros X-Patchwork-Id: 66028 Delivered-To: patch@linaro.org Received: by 10.140.93.198 with SMTP id d64csp1323632qge; Mon, 18 Apr 2016 07:40:07 -0700 (PDT) X-Received: by 10.66.138.16 with SMTP id qm16mr50428044pab.28.1460990407692; Mon, 18 Apr 2016 07:40:07 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id yt2si3896896pab.188.2016.04.18.07.40.07; Mon, 18 Apr 2016 07:40:07 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752091AbcDROkF (ORCPT + 29 others); Mon, 18 Apr 2016 10:40:05 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:55770 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751363AbcDROkC (ORCPT ); Mon, 18 Apr 2016 10:40:02 -0400 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id u3IEd6EP021822; Mon, 18 Apr 2016 09:39:06 -0500 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id u3IEd5lK007777; Mon, 18 Apr 2016 09:39:05 -0500 Received: from dflp33.itg.ti.com (10.64.6.16) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.3.224.2; Mon, 18 Apr 2016 09:39:05 -0500 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp33.itg.ti.com (8.14.3/8.13.8) with ESMTP id u3IEd1DZ010824; Mon, 18 Apr 2016 09:39:02 -0500 Subject: Re: [PATCH v6 00/17] memory: omap-gpmc: mtd: nand: Support GPMC NAND on non-OMAP platforms To: Boris Brezillon References: <1460023715-19332-1-git-send-email-rogerq@ti.com> <20160413212500.GD5995@atomide.com> <5710B58C.7050108@ti.com> <20160415120942.3131eac1@bbrezillon> <5710C86A.9060108@ti.com> <20160415135108.63716f06@bbrezillon> <20160415154139.GS5995@atomide.com> <20160415180531.15d790d2@bbrezillon> <20160415161950.GT5995@atomide.com> <20160416105711.5310d2d0@bbrezillon> <5714D3A4.2050405@ti.com> <5714D8AA.9040304@ti.com> <20160418151305.17a42437@bbrezillon> <5714E5AA.5050200@ti.com> <20160418161001.2a1c0bed@bbrezillon> CC: Tony Lindgren , , , , , , , , , , From: Roger Quadros Message-ID: <5714F185.5080103@ti.com> Date: Mon, 18 Apr 2016 17:39:01 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160418161001.2a1c0bed@bbrezillon> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/04/16 17:10, Boris Brezillon wrote: > On Mon, 18 Apr 2016 16:48:26 +0300 > Roger Quadros wrote: > >> Boris, >> >> On 18/04/16 16:13, Boris Brezillon wrote: >>> Hi Roger, >>> >>> On Mon, 18 Apr 2016 15:52:58 +0300 >>> Roger Quadros 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 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 diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index e622a1b..eb85d6b 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 + BADBLOCK_MARKER_LENGTH, -1); + chip->read_buf(mtd, chip->oob_poi, chip->ecc.total); /* Calculate ecc bytes */ chip->ecc.calculate(mtd, buf, ecc_calc);