From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,T_DKIM_INVALID, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60578C1B0F2 for ; Wed, 20 Jun 2018 09:30:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3C0C720693 for ; Wed, 20 Jun 2018 09:30:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="o8c9Tk1D"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="mly4wUE8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3C0C720693 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754651AbeFTJaF (ORCPT ); Wed, 20 Jun 2018 05:30:05 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:42786 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753989AbeFTJaA (ORCPT ); Wed, 20 Jun 2018 05:30:00 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id B687E608C1; Wed, 20 Jun 2018 07:04:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1529478242; bh=v+Ua4n6aXmTntYWRdsuf5pKJl2Y0mPaPhSgiU0vyYvg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=o8c9Tk1DUr1o56AUtUfXWLQQVXkpLDQb8u8ChFZOlys++QmUZpcDA1X11CFTN2J2R Mu7V7XatfrV+4teD8p0ZT0r6r23joNRnl6tJnIevDIkaRGKqcbPNeXG1XrdlFUlzVV NuV+bv/46eTtcp2siGpf4z2NAIRPeYWImy7FYpnE= Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 6CE0F60714; Wed, 20 Jun 2018 07:04:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1529478241; bh=v+Ua4n6aXmTntYWRdsuf5pKJl2Y0mPaPhSgiU0vyYvg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=mly4wUE8yzMCqyHVgGSyLnW0np2B1pDw6H3cL1hxVnB7Vtru/ohCmZJdOq+5Q1vQC H8LLbGe9rAk4Rvpgb3u6SPOlf9d3tYZ8j9zCjSPNkdfXDLVlTKQo5twrspGrn2m5+J ZIeeVZqEORH784Tv5MKvP274iz0F1nt0xH9dwTPE= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 20 Jun 2018 12:34:01 +0530 From: Abhishek Sahu To: Miquel Raynal Cc: Boris Brezillon , David Woodhouse , Brian Norris , Marek Vasut , Richard Weinberger , Cyrille Pitchen , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, Andy Gross , Archit Taneja Subject: Re: [PATCH v3 13/16] mtd: rawnand: qcom: minor code reorganization for bad block check In-Reply-To: <20180618133556.06e9a16a@xps13> References: <1527250904-21988-1-git-send-email-absahu@codeaurora.org> <1527250904-21988-14-git-send-email-absahu@codeaurora.org> <20180526104629.74315561@xps13> <90ae248edf8a06a1d35e2da458f75af5@codeaurora.org> <20180528090352.254022ed@xps13> <53caff8799d30b6a81ac10f63a7c56c4@codeaurora.org> <20180607145326.339170fd@xps13> <7179eb382dbd3d49e2066178914636fd@codeaurora.org> <20180618133556.06e9a16a@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-06-18 17:05, Miquel Raynal wrote: > Hi Abhishek, > > Boris, one question for you below :) > >> >> >> >> So for last CW, the 464 is BBM (i.e 2048th byte) in >> >> >> full page. >> >> >> >> > >> >> clear_bam_transaction(nandc); >> >> >> >> - ret = copy_last_cw(host, page); >> >> >> >> - if (ret) >> >> >> >> + clear_read_regs(nandc); >> >> >> >> + >> >> >> >> + set_address(host, host->cw_size * (ecc->steps - 1), page); >> >> >> >> + update_rw_regs(host, 1, true); >> >> >> >> + >> >> >> >> + /* >> >> >> >> + * The last codeword data will be copied from NAND device to NAND >> >> >> >> + * controller internal HW buffer. Copy only required BBM size bytes >> >> >> >> + * from this HW buffer to bbm_bytes_buf which is present at >> >> >> >> + * bbpos offset. >> >> >> >> + */ >> >> >> >> + nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> >> + config_nand_single_cw_page_read(nandc); >> >> >> >> + read_data_dma(nandc, FLASH_BUF_ACC + bbpos, bbm_bytes_buf, >> >> >> >> + host->bbm_size, 0); >> >> >> >> + >> >> >> >> + ret = submit_descs(nandc); >> >> >> >> + free_descs(nandc); >> >> >> >> + if (ret) { >> >> >> >> + dev_err(nandc->dev, "failed to copy bad block bytes\n"); >> >> >> >> goto err; >> >> >> >> + } >> >> >> >> >> flash_status = le32_to_cpu(nandc->reg_read_buf[0]); >> >> >> >> >> @@ -2141,12 +2127,10 @@ static int qcom_nandc_block_bad(struct >> mtd_info *mtd, loff_t ofs) >> >> >> >> goto err; >> >> >> >> } >> >> >> >> >> - bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1); >> >> >> >> - >> >> >> >> - bad = nandc->data_buffer[bbpos] != 0xff; >> >> >> >> + bad = bbm_bytes_buf[0] != 0xff; >> >> >> > > This is suspect as it still points to the beginning of the data buffer. >> >> >> > Can you please check you did not meant bbm_bytes_buf[bbpos]? >> >> >> > >> >> >> The main thing here is >> >> >> nandc_set_read_loc(nandc, 0, bbpos, host->bbm_size, 1); >> >> >> >> After reading one complete CW from NAND, the data will be still >> >> >> in NAND HW buffer. >> >> >> >> The above register tells that we need to read data from >> >> >> bbpos of size host->bbm_size (which is 1 byte for 8 bus witdh >> >> >> and 2 byte for 16 bus width) in bbm_bytes_buf. >> >> > > I see: idx 0 in bbm_bytes_buf is the data at offset bbpos. Then >> >> > it's ok. >> >> > >> >> So bbm_bytes_buf[0] will contain the BBM first byte. >> >> >> and bbm_bytes_buf[1] will contain the BBM second byte. >> >> >> >> Regards, >> >> >> Abhishek >> >> >> >> >> >> if (chip->options & NAND_BUSWIDTH_16) >> >> >> >> - bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff); >> >> >> >> + bad = bad || (bbm_bytes_buf[1] != 0xff); >> >> > > Sorry, my mistake, I did not see the above line. >> >> > > However, technically, the BBM could be located in the first, second or >> >> > last page of the block. You should check the three of them are 0xFF >> >> > before declaring the block is not bad. >> >> > > The more I look at the function, the more I wonder if you actually need >> >> > it. Why does the generic nand_block_bad() implementation in the core >> >> > do not fit? >> >> >> The BBM bytes can be accessed in raw mode only for QCOM NAND >> >> Contoller. We started with following patch for initial patches >> >> >> http://patchwork.ozlabs.org/patch/508565/ >> >> >> I am also not very much sure, how can we go ahead now. >> >> Ideally we need to use generic function only which >> >> requires raw_read. >> >> > > I see, thanks for pointing this thread. >> > > Well for now then let's keep our driver-specific implementation. >> > > I will just ask you to do a consistent check as requested above (you >> > can copy code from the core) and add a comment above this function >> > explaining why it is needed (what you just told me). >> > >> Hi Miquel, >> >> I explored more regarding making custom bad block functions in this >> thread and it looks like, we can move to generic block_bad function >> by small changes in QCOM NAND driver >> only. The main problem was, in read page with ECC, the bad block >> byte was skipped. >> >> But controller is copying the bad block bytes in another register >> with following status bytes. >> >> BAD_BLOCK_STATUS : With every page read operation, when the >> controller >> reads a page with a bad block, it writes the bad block status data >> into >> this register. >> >> We can update the BBM bytes at start of OOB data in read_oob >> function >> with these status bytes. It will help in getting rid of >> driver-specific >> implementation for chip->block_bad. > > If think this is acceptable. > >> >> For chip->block_markbad, if we want to get rid of >> driver-specific implementation then we can have >> following logic >> >> in write_oob function check for bad block bytes in oob >> and do the raw write for updating BBM bytes alone in >> flash if BBM bytes are non 0xff. > > Ok but this will have to be properly explained in a descriptive > comment! > > Maybe Boris can give its point of view on the subject. Is it worth > adding the above 'hacks' in the qcom driver and get rid of the > driver-specific ->is_bad()/->mark_bad() impementations? Thanks Miquel. I will remove this patch from this patch series and will send separate patch series for all bad block handling related changes once things are finalized. Regards, Abhishek