Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751321AbdGQHRL (ORCPT ); Mon, 17 Jul 2017 03:17:11 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:39584 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751261AbdGQHRJ (ORCPT ); Mon, 17 Jul 2017 03:17:09 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 17 Jul 2017 12:47:07 +0530 From: Abhishek Sahu To: Archit Taneja Cc: dwmw2@infradead.org, computersforpeace@gmail.com, boris.brezillon@free-electrons.com, marek.vasut@gmail.com, richard@nod.at, cyrille.pitchen@wedev4u.fr, robh+dt@kernel.org, mark.rutland@arm.com, linux-mtd@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, andy.gross@linaro.org, sricharan@codeaurora.org Subject: Re: [PATCH 09/14] qcom: mtd: nand: BAM support for read page In-Reply-To: References: <1498720566-20782-1-git-send-email-absahu@codeaurora.org> <1498720566-20782-10-git-send-email-absahu@codeaurora.org> Message-ID: <53fd2482d3f8f6d965910fc1a711751a@codeaurora.org> User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5762 Lines: 177 On 2017-07-04 15:10, Archit Taneja wrote: > On 06/29/2017 12:46 PM, Abhishek Sahu wrote: >> 1. The BAM mode requires few registers configuration before each >> NAND page read and codeword read which is different from ADM >> so add the helper functions which will be called in BAM mode >> only. >> >> 2. The NAND page read handling of BAM is different from ADM so >> call the appropriate helper functions >> >> Signed-off-by: Abhishek Sahu >> --- >> drivers/mtd/nand/qcom_nandc.c | 63 >> ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 62 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c >> index 8e7dc9e..17766af 100644 >> --- a/drivers/mtd/nand/qcom_nandc.c >> +++ b/drivers/mtd/nand/qcom_nandc.c >> @@ -870,6 +870,35 @@ static void config_cw_read(struct >> qcom_nand_controller *nandc) >> } >> >> /* >> + * Helpers to prepare DMA descriptors for configuring registers >> + * before reading a NAND page with BAM. >> + */ >> +static void config_bam_page_read(struct qcom_nand_controller *nandc) >> +{ >> + write_reg_dma(nandc, NAND_FLASH_CMD, 3, 0); >> + write_reg_dma(nandc, NAND_DEV0_CFG0, 3, 0); >> + write_reg_dma(nandc, NAND_EBI2_ECC_BUF_CFG, 1, 0); >> + write_reg_dma(nandc, NAND_ERASED_CW_DETECT_CFG, 1, 0); >> + write_reg_dma(nandc, NAND_ERASED_CW_DETECT_CFG, 1, >> + NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL); >> +} >> + >> +/* >> + * Helpers to prepare DMA descriptors for configuring registers >> + * before reading each codeword in NAND page with BAM. >> + */ > > If I understood right, EBI2 nand required us to load all the registers > configured in config_cw_read() for every codeword, and for BAM, the > registers configured in config_bam_page_read() just needs to be done > once, > and the registers in config_bam_cw_read() need to be reloaded for > every > codeword? > > Could you please clarify this better in the commit message and > comments? Also, > I still see config_cw_read() being used for QPIC nand in nandc_param() > and > copy_last_cw()? > > Also, I think these should be called config_qpic_page_read() and > config_qpic_cw_read() since it seems more of a property of the NAND > controller > rather than the underlying DMA engine. If so, config_cw_read() can be > called > config_cw_ebi2_read(). Please correct me if I'm wrong somewhere. > I did some code reorganization in v2 in this area and now, we don't have do different things for EBI2 and QPIC for read. >> +static void config_bam_cw_read(struct qcom_nand_controller *nandc) >> +{ >> + write_reg_dma(nandc, NAND_READ_LOCATION_0, 2, 0); >> + write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL); >> + write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + read_reg_dma(nandc, NAND_FLASH_STATUS, 2, 0); >> + read_reg_dma(nandc, NAND_ERASED_CW_DETECT_STATUS, 1, >> + NAND_BAM_NEXT_SGL); >> +} >> + >> +/* >> * helpers to prepare dma descriptors used to configure registers >> needed for >> * writing a codeword/step in a page >> */ >> @@ -1398,6 +1427,9 @@ static int read_page_ecc(struct qcom_nand_host >> *host, u8 *data_buf, >> struct nand_ecc_ctrl *ecc = &chip->ecc; >> int i, ret; >> >> + if (nandc->dma_bam_enabled) >> + config_bam_page_read(nandc); >> + >> /* queue cmd descs for each codeword */ >> for (i = 0; i < ecc->steps; i++) { >> int data_size, oob_size; >> @@ -1411,7 +1443,36 @@ static int read_page_ecc(struct qcom_nand_host >> *host, u8 *data_buf, >> oob_size = host->ecc_bytes_hw + host->spare_bytes; >> } >> >> - config_cw_read(nandc); >> + if (nandc->dma_bam_enabled) { >> + if (data_buf && oob_buf) { >> + nandc_set_reg(nandc, NAND_READ_LOCATION_0, >> + (0 << READ_LOCATION_OFFSET) | >> + (data_size << >> + READ_LOCATION_SIZE) | >> + (0 << READ_LOCATION_LAST)); >> + nandc_set_reg(nandc, NAND_READ_LOCATION_1, >> + (data_size << >> + READ_LOCATION_OFFSET) | >> + (oob_size << READ_LOCATION_SIZE) | >> + (1 << READ_LOCATION_LAST)); >> + } else if (data_buf) { >> + nandc_set_reg(nandc, NAND_READ_LOCATION_0, >> + (0 << READ_LOCATION_OFFSET) | >> + (data_size << >> + READ_LOCATION_SIZE) | >> + (1 << READ_LOCATION_LAST)); >> + } else { >> + nandc_set_reg(nandc, NAND_READ_LOCATION_0, >> + (data_size << >> + READ_LOCATION_OFFSET) | >> + (oob_size << READ_LOCATION_SIZE) | >> + (1 << READ_LOCATION_LAST)); >> + } > > Could we put the READ_LOCATION_x register configuration into a small > helper? > This is probably a matter of taste, but you could consider configuring > like this. > Maybe something similar for patch #11 for raw page reads. > I will use macro for assigning READ LOCATION registers in v2, which makes the code cleaner. If required, I will use helpers also. > if (data_buf && oob_buf) { > r0_off = 0; > r0_size = r1_off = data_size; > r1_size = oob_size; > r0_last = 0; > r1_last = 1; > } else if (data_buf) { > rl0_off = 0; > rl0_size = data_size; > rl0_last = 1; > } else { > rl0_off = data_size; > rl0_size = oob_size; > rl0_last = 1; > } > > nandc_set_reg(nandc, NAND_READ_LOCATION_0, > (rl0_off << READ_LOCATION_OFFSET) | > (rl0_size << READ_LOCATION_SIZE) | > (rl0_last << READ_LOCATION_LAST)); > if (rl1_last) > /* program LOCATION_1 register */ > > Thanks, > Archit > >> + >> + config_bam_cw_read(nandc); >> + } else { >> + config_cw_read(nandc); >> + } >> >> if (data_buf) >> read_data_dma(nandc, FLASH_BUF_ACC, data_buf, >> -- Abhishek Sahu