Received: by 10.192.165.148 with SMTP id m20csp1708841imm; Thu, 26 Apr 2018 00:44:29 -0700 (PDT) X-Google-Smtp-Source: AIpwx4+Cl4TQLSjmkm/Jl/dnsqpKizkzVixaW5iInM5lRAUQJrsHBd0GUZq+CzSjyTUjhw/BNygZ X-Received: by 10.101.74.132 with SMTP id b4mr12787145pgu.36.1524728669612; Thu, 26 Apr 2018 00:44:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524728669; cv=none; d=google.com; s=arc-20160816; b=Q5+dxBWwlH6tNiLpGmYWC5YX5ORiz+ChjvCTdgVqBsaN9OejcvIA/kCNYX62CV8rSP QZg6q49xfskfY7RjF0kVTkPePajkdrCIX09PaWCTDFMxWJj1Bftp/I23s3Mao2sKH+Tr F2+wDSS6gXlZ9c8aBMQTN3ayNKREcfUooEi/kduDuFEyC0Bhmx9lqMtO67qY+xJcYird +TlNEtdfBRn4MmNf4y9GaZ+4V88C5a6WTtJmaLBjiJahw6Z//L75QLDafH58KaNfg8Oa UjNmCXAdhelQKDPXkYwG0jjqvBQp4bZtmc9l+jbecxmH5Jv8pi4CshJ4GjAUfUNpZaCo LwnA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:message-id:references :in-reply-to:subject:cc:to:from:date:content-transfer-encoding :mime-version:dkim-signature:dkim-signature :arc-authentication-results; bh=C/vyxxSbdTrwsxa1KRFGkZhs87dGxRP8JQuXO39qOtk=; b=RdHZWuSCaKkSWn35BxoIB8U1tjCQFzyKlvplEgj0Sgg4lf6YMuYtmKM+/eBNlb4muh ua+AU8nQt3DI8vCE2BNPtMkHCVlILHyzQJ92nhWJwtWmvC+z/lb1Ept3Vn/IEHaR15Sf z4tiWuxQkF404XuHcRhFaN+kaGkeXC7CeJ2LiImVFkL+cjr2hXsysqmOD7osaMDXDyi2 E2amxRAowTXhiWRV2iTbogWOE9WDhvJZhZsWPceQk35bcvYqzEDZkrE8TGdcEjGf3YLX +DV/JPxPnKnak5MKD3fdcvbCSIGQyQj/6ANG4bHCu38SH9oIEcgpiDq1Iz9HbjkaFYal X9pA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=LFafU2QM; dkim=pass header.i=@codeaurora.org header.s=default header.b=i4+5OrfJ; 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 Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id a81si11647168pfg.200.2018.04.26.00.44.15; Thu, 26 Apr 2018 00:44:29 -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; dkim=pass header.i=@codeaurora.org header.s=default header.b=LFafU2QM; dkim=pass header.i=@codeaurora.org header.s=default header.b=i4+5OrfJ; 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 S1753949AbeDZHm6 (ORCPT + 99 others); Thu, 26 Apr 2018 03:42:58 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:42668 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753487AbeDZHm4 (ORCPT ); Thu, 26 Apr 2018 03:42:56 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id A7EBF60C55; Thu, 26 Apr 2018 07:42:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1524728575; bh=bhvxFmoN/YgtQVpmL5Iuwds9Pntj+ZTI2Qd+Z8tGlqo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=LFafU2QMptFUkCgU+DvMZiyVD7BT7C2I5CRx/fOAjQW/+Stywp5/TeB9Z1o1yXMib S3WojR7l7FHCedbxlMUtQyXEp4jFR7pF50nV6WDgNjrC5whhyXDHwviw/idwQMVkYf fSSJ0zSjCFGmw5oPhGMbUsW2BCJsbofczsFqPz3g= X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 1950460117; Thu, 26 Apr 2018 07:42:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1524728573; bh=bhvxFmoN/YgtQVpmL5Iuwds9Pntj+ZTI2Qd+Z8tGlqo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=i4+5OrfJlmT73RJ0xbE9cDVDOJ589aFkp+8Bu0hROC4yaUjtxBegq0XCnbEWXAITH bWTf1am3zkePDmapMbs43SAQ9XHdl9Owktdix+sRRptEMvpYqTUpsI7WQbVFVSGbgG 15AbnZF2yaCXO2j1CH//HZDGsGqi8TYHls0y/8L0= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Thu, 26 Apr 2018 13:12:53 +0530 From: Abhishek Sahu To: Miquel Raynal Cc: Boris Brezillon , Archit Taneja , Richard Weinberger , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Marek Vasut , linux-mtd@lists.infradead.org, Cyrille Pitchen , Andy Gross , Brian Norris , David Woodhouse Subject: Re: [PATCH 8/9] mtd: nand: qcom: helper function for raw read In-Reply-To: <20180426091149.10cee27c@xps13> References: <1522845745-6624-1-git-send-email-absahu@codeaurora.org> <1522845745-6624-9-git-send-email-absahu@codeaurora.org> <20180410114428.68d59c8d@xps13> <20180422181929.65d42ca8@xps13> <484485e160af41cf062549938c53d078@codeaurora.org> <20180423085817.74e56091@xps13> <892130678772dca7116bd8dcdcb3cc44@codeaurora.org> <20180425145905.219b6faf@xps13> <798994bd2cf24e919403f568cb1ff3ea@codeaurora.org> <20180426091149.10cee27c@xps13> Message-ID: X-Sender: absahu@codeaurora.org User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018-04-26 12:41, Miquel Raynal wrote: > Hi Abhishek, > > On Thu, 26 Apr 2018 11:23:19 +0530, Abhishek Sahu > wrote: > >> On 2018-04-25 18:29, Miquel Raynal wrote: >> > Hi Abhishek, >> > > On Wed, 25 Apr 2018 12:02:29 +0530, Abhishek Sahu >> > wrote: >> > >> On 2018-04-23 12:28, Miquel Raynal wrote: >> >> > Hi Abhishek, >> >> > > On Mon, 23 Apr 2018 11:58:42 +0530, Abhishek Sahu >> >> > wrote: >> >> > >> On 2018-04-22 21:49, Miquel Raynal wrote: >> >> >> > Hi Abhishek, >> >> >> > > On Thu, 12 Apr 2018 12:36:42 +0530, Abhishek Sahu >> >> >> > wrote: >> >> >> > >> On 2018-04-10 15:14, Miquel Raynal wrote: >> >> >> >> > Hi Abhishek, >> >> >> >> > > On Wed, 4 Apr 2018 18:12:24 +0530, Abhishek Sahu >> >> >> >> > wrote: >> >> >> >> > >> This patch does minor code reorganization for raw reads. >> >> >> >> >> Currently the raw read is required for complete page but for >> >> >> >> >> subsequent patches related with erased codeword bit flips >> >> >> >> >> detection, only few CW should be read. So, this patch adds >> >> >> >> >> helper function and introduces the read CW bitmask which >> >> >> >> >> specifies which CW reads are required in complete page. >> >> >> >> > > I am not sure this is the right approach for subpage reads. If the >> >> >> >> > controller supports it, you should just enable it in chip->options. >> >> >> >> > >> >> >> >> Thanks Miquel. >> >> >> >> >> It is internal to this file only. This patch makes one static helper >> >> >> >> function which provides the support to read subpages. >> >> >> > > Drivers should expose raw helpers, why keeping this helper static then? >> >> >> > >> >> >> >> Signed-off-by: Abhishek Sahu >> >> >> >> >> --- >> >> >> >> >> drivers/mtd/nand/qcom_nandc.c | 186 >> +++++++++++++++++++++++++----------------- >> >> >> >> >> 1 file changed, 110 insertions(+), 76 deletions(-) >> >> >> >> >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c >> >> >> >> >> index 40c790e..f5d1fa4 100644 >> >> >> >> >> --- a/drivers/mtd/nand/qcom_nandc.c >> >> >> >> >> +++ b/drivers/mtd/nand/qcom_nandc.c >> >> >> >> >> @@ -1590,6 +1590,114 @@ static int check_flash_errors(struct >> qcom_nand_host *host, int cw_cnt) >> >> >> >> >> } >> >> >> >> >> >> /* >> >> >> >> >> + * Helper to perform the page raw read operation. The read_cw_mask >> will be >> >> >> >> >> + * used to specify the codewords for which the data should be read. >> The >> >> >> >> >> + * single page contains multiple CW. Sometime, only few CW data is >> required >> >> >> >> >> + * in complete page. Also, start address will be determined with >> >> >> >> >> + * this CW mask to skip unnecessary data copy from NAND flash device. >> Then, >> >> >> >> >> + * actual data copy from NAND controller to data buffer will be done >> only >> >> >> >> >> + * for the CWs which have the mask set. >> >> >> >> >> + */ >> >> >> >> >> +static int >> >> >> >> >> +nandc_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> >> >> >> >> + u8 *data_buf, u8 *oob_buf, >> >> >> >> >> + int page, unsigned long read_cw_mask) >> >> >> >> >> +{ >> >> >> >> >> >> >> >> >> - free_descs(nandc); >> >> >> >> >> - >> >> >> >> >> - if (!ret) >> >> >> >> >> - ret = check_flash_errors(host, ecc->steps); >> >> >> >> >> - >> >> >> >> >> - return 0; >> >> >> >> >> + return nandc_read_page_raw(mtd, chip, buf, chip->oob_poi, page, >> >> >> >> >> + BIT(chip->ecc.steps) - 1); >> >> >> >> > > I don't understand this. chip->ecc.steps is constant, right? So you >> >> >> >> > always ask for the same subpage? >> >> >> >> >> We need to do raw read for subpages in which we got uncorrectable >> >> >> >> error in next patch for erased page bitflip detection. This patch >> does >> >> >> >> reorganization of raw read and moves common code in helper function >> >> >> >> nandc_read_page_raw. >> >> >> >> >> For nomral raw read, all the subpages will be read so >> >> >> >> BIT(chip->ecc.steps) - 1 is used for qcom_nandc_read_page_raw. >> >> >> >> >> While for erased page raw read, only those sub pages will be >> >> >> >> read for which the controller gives the uncorrectable error. >> >> >> > > Still not okay: the driver should expose a way to do raw reads no >> >> >> > matter the length and the start and you should use that in a generic >> >> >> > way. >> >> >> > >> >> >> Thanks Miquel. >> >> >> I will explore regarding that. >> >> >> Looks like, we can some helper like read_subpage. >> >> > > Of course, when you implement raw accessors you can have static helpers >> >> > to clarify the code. >> >> > >> >> Hi Miquel, >> >> >> I checked regarding generic function. >> >> >> Normally the other NAND controller stores the data in main area >> >> and ecc bytes in oob area. So if page size is 2048+64 then 2048 >> >> data bytes will go in main area followed by ECC bytes >> >> >> Main | OOB >> >> ------------------------------------ >> >> D1 D2...........D2048|E1..........E64 >> >> >> The QCOM nand contoller follows different layout in which this >> >> 2048+64 is internally divided in 528 bytes codeword so it will >> >> come to 4 codewords. Each codeword contains 516 bytes followed >> >> by ECC parity bytes. Also in at 0x1f0 offset in each CW, the bad >> >> Sorry Miquel. >> Its 0x1d0 instead of 0x1f0. >> >> >> block marker is stored. so each CW contains badblock marker also. >> >> The avilable OOB bytes are 16 which is also protected by ECC. >> >> >> 516 * 4 = 2048 data bytes + 16 available oob bytes. >> >> >> So for QCOM layout, it will be something lile >> >> >> >> 0 D1.........D495 B1 D496....D516 E1...E7 U1..U4 >> >> 0x210 D517......D1012 B2 D1016..D1032 E8...E14 U5..U8 >> >> 0x420 D1033.....D1528 B3 D1529..D1548 E15..E21 U9..U12 >> >> 0x630 D1549.....D2044 B4 D2045..D2048 O1...O16 E22 E28 U13..U16 >> >> >> Where D - Data bytes >> >> B - BBM bytes >> >> E - ECC parity bytes >> >> U - Unused bytes >> >> O - Avilable OOB bytes >> >> >> Now, the raw read expect data to be returned in buffer in the >> >> form of >> >> >> D1.....D2048 B1 E1...E7 U1..U4 B2 E8...E14 U5..U8 B3 E15..E21 >> >> U9..U12 B4 O1...O16 E22 E28 U13..U16 >> >> >> Now, the puporose of that helper function is read selected >> >> codewords. This codeword is QCOM specific and other controller >> >> normally doesn't follow this kind of layout. >> >> >> Now If we make generic fuction for subpgage raw read with any >> >> start and length then that function we can't use for erased page >> >> bitflip detection. >> >> >> If we give the start as 0 with length 528 then our requirement is >> >> to get 516 data bytes in data buffer and 12 bytes in OOB buffer >> >> but according to standard operations, it will be interpreted as >> >> all data bytes from D1..D528 >> >> >> so that helper function is to read selected Codewords from >> >> QCOM NAND controller. This Codeword is different from subpages >> > > Thanks for the clarification, I understand now. Maybe you can add a >> > comment about this particular layout and why you need this helper to >> > check for erased pages. >> > >> Sure Miquel. Will add the same in comment. >> >> > BTW are these BBM preserved somehow? >> >> The QCOM nand layout is such that, the bad block byte for last >> codeowrd >> will come to first byte in spare area. If we have 2048+64 bytes >> device, >> then it will have 4 codewords of size 528. For the last codeword, >> the >> B4 will come at >> >> 528 * 3 + 0x1d0 = 0x800 which is first byte in OOB area. > > Great, so maybe that's why the kernel don't mess up with bad blocks. As > long as this byte is !0xff it should not be touched. > > For good pages however, I guess you just don't care about the fact that > three bytes are supposedly "markers" and use them as data? They won't > be checked by the kernel anyway. > Correct Miquql. The 3 bytes (B1, B2, B3) for first 3 codewords will be ignored. No data will be written over that. During read/write with ECC, NAND controller itself ignore that byte at 0x1d0 offset. Only B4 in last codeword (first byte in spare area) will be used for bad block marker. Thanks, Abhishek >> >> Normally vendor make this byte as non 0xff for factory bad block. >> For bad blocks created after that this byte will be marked >> as non 0xff in >> >> https://elixir.bootlin.com/linux/v4.16/source/drivers/mtd/nand/qcom_nandc.c#L2657 >> >> This bad block byte won't be touched during read/write with ECC. >> >> Thanks, >> Abhishek >> > > Thanks for the clarification, > Miquèl