From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH v8 2/3] mtd: nand: Qualcomm NAND controller driver Date: Tue, 16 Feb 2016 12:20:10 +0530 Message-ID: <56C2C6A2.8030200@codeaurora.org> References: <1453360399-32029-1-git-send-email-architt@codeaurora.org> <1454489991-14717-1-git-send-email-architt@codeaurora.org> <1454489991-14717-3-git-send-email-architt@codeaurora.org> <20160204113938.1317095d@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:53728 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753640AbcBPGuR (ORCPT ); Tue, 16 Feb 2016 01:50:17 -0500 In-Reply-To: <20160204113938.1317095d@bbrezillon> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: computersforpeace@gmail.com Cc: Boris Brezillon , dehrenberg@google.com, cernekee@gmail.com, sboyd@codeaurora.org, linux-mtd@lists.infradead.org, linux-arm-msm@vger.kernel.org, andy.gross@linaro.org Hi Brian, On 02/04/2016 04:09 PM, Boris Brezillon wrote: > Hi Archit, > > On Wed, 3 Feb 2016 14:29:50 +0530 > Archit Taneja wrote: > >> The Qualcomm NAND controller is found in SoCs like IPQ806x, MSM7xx, >> MDM9x15 series. >> >> It exists as a sub block inside the IPs EBI2 (External Bus Interface 2) >> and QPIC (Qualcomm Parallel Interface Controller). These IPs provide a >> broader interface for external slow peripheral devices such as LCD and >> NAND/NOR flash memory or SRAM like interfaces. >> >> We add support for the NAND controller found within EBI2. For the SoCs >> of our interest, we only use the NAND controller within EBI2. Therefore, >> it's safe for us to assume that the NAND controller is a standalone block >> within the SoC. >> >> The controller supports 512B, 2kB, 4kB and 8kB page 8-bit and 16-bit NAND >> flash devices. It contains a HW ECC block that supports BCH ECC (4, 8 and >> 16 bit correction/step) and RS ECC(4 bit correction/step) that covers main >> and spare data. The controller contains an internal 512 byte page buffer >> to which we read/write via DMA. The EBI2 type NAND controller uses ADM DMA >> for register read/write and data transfers. The controller performs page >> reads and writes at a codeword/step level of 512 bytes. It can support up >> to 2 external chips of different configurations. >> >> The driver prepares register read and write configuration descriptors for >> each codeword, followed by data descriptors to read or write data from the >> controller's internal buffer. It uses a single ADM DMA channel that we get >> via dmaengine API. The controller requires 2 ADM CRCIs for command and >> data flow control. These are passed via DT. >> >> The ecc layout used by the controller is syndrome like, but we can't use >> the standard syndrome ecc ops because of several reasons. First, the amount >> of data bytes covered by ecc isn't same in each step. Second, writing to >> free oob space requires us writing to the entire step in which the oob >> lies. This forces us to create our own ecc ops. >> >> One more difference is how the controller accesses the bad block marker. >> The controller ignores reading the marker when ECC is enabled. ECC needs >> to be explicity disabled to read or write to the bad block marker. The >> nand_bbt helpers library hence can't access BBMs for the controller. >> For now, we skip the creation of BBT and populate chip->block_bad and >> chip->block_markbad helpers instead. >> >> Reviewed-by: Andy Gross >> Signed-off-by: Stephen Boyd >> Signed-off-by: Archit Taneja >> --- >> v8: >> - Use nand_check_erased_ecc_chunk in the right manner as >> suggested by Boris (place the check when we see uncorrectable >> errors). >> - Rewrite the empty_page_fixup code such that we check for a >> codeword being erased rather than the entire page. This simplifies >> the code and we can now place it in parse_read_errors(). >> - Introduce raw page access functions. This results in making some >> modifications in the existing ECC page access ops too, since the >> layout now also considers the real/dummy bad block markers. Explained >> in comments. >> >> v7: >> - Incorporated missing/new comments by Boris >> - Cleaned up some strict checkpatch warnings >> >> v6: >> - Fix up erased page parsing. Use nand_check_erased_ecc_chunk to >> return corrected bitflips in an erased page. >> - Fix whitespace issues >> - Update compatible tring to something more specific >> >> v5: >> - split chip/controller structs >> - simplify layout by considering reserved bytes as part of ECC >> - create ecc layouts automatically >> - implement block_bad and block_markbad chip ops instead of >> - read_oob_raw/write_oob_raw ecc ops to access BBMs. >> - Add NAND_SKIP_BBTSCAN flag until we get badblockbits support. >> - misc clean ups >> >> v4: >> - Shrink submit_descs >> - add desc list node at the end of dma_prep_desc >> - Endianness and warning fixes >> - Add Stephen's Signed-off since he provided a patch to fix >> endianness problems >> >> v3: >> - Refactor dma functions for maximum reuse >> - Use dma_slave_confing on stack >> - optimize and clean upempty_page_fixup using memchr_inv >> - ensure portability with dma register reads using le32_* funcs >> - use NAND_USE_BOUNCE_BUFFER instead of doing it ourselves >> - fix handling of return values of dmaengine funcs >> - constify wherever possible >> - Remove dependency on ADM DMA in Kconfig >> - Misc fixes and clean ups >> >> v2: >> - Use new BBT flag that allows us to read BBM in raw mode >> - reduce memcpy-s in the driver >> - some refactor and clean ups because of above changes >> >> drivers/mtd/nand/Kconfig | 7 + >> drivers/mtd/nand/Makefile | 1 + >> drivers/mtd/nand/qcom_nandc.c | 2224 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 2232 insertions(+) >> create mode 100644 drivers/mtd/nand/qcom_nandc.c >> > > [...] > >> +static int qcom_nand_host_setup(struct qcom_nand_host *host) >> +{ >> + struct nand_chip *chip = &host->chip; >> + struct mtd_info *mtd = nand_to_mtd(chip); >> + struct nand_ecc_ctrl *ecc = &chip->ecc; >> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); >> + int cwperpage, bad_block_byte; >> + bool wide_bus; >> + int ecc_mode = 1; >> + >> + /* >> + * the controller requires each step consists of 512 bytes of data. >> + * bail out if DT has populated a wrong step size. >> + */ >> + if (ecc->size != NANDC_STEP_SIZE) { >> + dev_err(nandc->dev, "invalid ecc size\n"); >> + return -EINVAL; >> + } >> + >> + wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false; >> + >> + if (ecc->strength >= 8) { >> + /* 8 bit ECC defaults to BCH ECC on all platforms */ >> + host->bch_enabled = true; >> + ecc_mode = 1; >> + >> + if (wide_bus) { >> + host->ecc_bytes_hw = 14; >> + host->spare_bytes = 0; >> + host->bbm_size = 2; >> + } else { >> + host->ecc_bytes_hw = 13; >> + host->spare_bytes = 2; >> + host->bbm_size = 1; >> + } >> + } else { >> + /* >> + * if the controller supports BCH for 4 bit ECC, the controller >> + * uses lesser bytes for ECC. If RS is used, the ECC bytes is >> + * always 10 bytes >> + */ >> + if (nandc->ecc_modes & ECC_BCH_4BIT) { >> + /* BCH */ >> + host->bch_enabled = true; >> + ecc_mode = 0; >> + >> + if (wide_bus) { >> + host->ecc_bytes_hw = 8; >> + host->spare_bytes = 2; >> + host->bbm_size = 2; >> + } else { >> + host->ecc_bytes_hw = 7; >> + host->spare_bytes = 4; >> + host->bbm_size = 1; >> + } >> + } else { >> + /* RS */ >> + host->ecc_bytes_hw = 10; >> + >> + if (wide_bus) { >> + host->spare_bytes = 0; >> + host->bbm_size = 2; >> + } else { >> + host->spare_bytes = 1; >> + host->bbm_size = 1; >> + } >> + } >> + } >> + >> + /* >> + * we consider ecc->bytes as the sum of all the non-data content in a >> + * step. It gives us a clean representation of the oob area (even if >> + * all the bytes aren't used for ECC).It is always 16 bytes for 8 bit >> + * ECC and 12 bytes for 4 bit ECC >> + */ >> + ecc->bytes = host->ecc_bytes_hw + host->spare_bytes + host->bbm_size; >> + >> + ecc->read_page = qcom_nandc_read_page; >> + ecc->read_page_raw = qcom_nandc_read_page_raw; >> + ecc->read_oob = qcom_nandc_read_oob; >> + ecc->write_page = qcom_nandc_write_page; >> + ecc->write_page_raw = qcom_nandc_write_page_raw; >> + ecc->write_oob = qcom_nandc_write_oob; > > You should probably also implement ->{read, write}_oob_raw(), otherwise > the core set them to ecc->{read, write}_oob(), which is not exactly the > same. Anyway, let's keep that as things that as future improvements. > The rest looks good to me. > > Reviewed-by: Boris Brezillon If the patchset looks okay to you, could you please take this for the 4.6 merge window? Thanks, Archit > > Thanks for your patience, and all the reworks you've done. > > Best Regards, > > Boris > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation