Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751289AbdGQG0m (ORCPT ); Mon, 17 Jul 2017 02:26:42 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:50808 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751253AbdGQG0k (ORCPT ); Mon, 17 Jul 2017 02:26:40 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 17 Jul 2017 11:56:37 +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, devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, andy.gross@linaro.org, sricharan@codeaurora.org Subject: Re: [PATCH 02/14] qcom: mtd: nand: add and initialize QPIC DMA resources In-Reply-To: <9fa676a5-db6b-4792-dd86-7603c01cd12d@codeaurora.org> References: <1498720566-20782-1-git-send-email-absahu@codeaurora.org> <1498720566-20782-3-git-send-email-absahu@codeaurora.org> <9fa676a5-db6b-4792-dd86-7603c01cd12d@codeaurora.org> Message-ID: 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: 8245 Lines: 256 On 2017-07-03 10:47, Archit Taneja wrote: > On 06/29/2017 12:45 PM, Abhishek Sahu wrote: >> 1. The QPIC NAND uses 3 BAM channels: command, data tx and >> data rx while EBI2 NAND uses only single ADM channel. >> >> 2. The EBI2 NAND uses normal register read buffer since this >> buffer will be remapped with dma_map_sg. The QPIC NAND will give >> register read buffer in command descriptor and the command >> descriptor will be mapped with dma_map_sg so the register buffer >> should be DMA coherent. > > It isn't entirely clear from this commit message why we require > reg_read_buf to be DMA coherent for QPIC NAND. Could you please explain > this > better? I have used DMA coherent since we need to pass this memory in command descriptor where BAM will fill the register contents. Now for v2, I have planned to use streaming DMA API's and its working fine. > > Besides Marek's comment to splitting the patch, it looks okay to me. > > Thanks, > Archit > >> >> Signed-off-by: Abhishek Sahu >> --- >> .../devicetree/bindings/mtd/qcom_nandc.txt | 25 +++-- >> drivers/mtd/nand/qcom_nandc.c | 106 >> ++++++++++++++++----- >> 2 files changed, 99 insertions(+), 32 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> index 5d0f7ae..87b9a56 100644 >> --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt >> @@ -9,15 +9,17 @@ Required properties: >> - clock-names: must contain "core" for the core clock and "aon" for >> the >> always on clock >> - dmas: DMA specifier, consisting of a phandle to the ADM DMA >> - controller node and the channel number to be used for >> - NAND. Refer to dma.txt and qcom_adm.txt for more details >> -- dma-names: must be "rxtx" >> -- qcom,cmd-crci: must contain the ADM command type CRCI block >> instance >> - number specified for the NAND controller on the given >> - platform >> -- qcom,data-crci: must contain the ADM data type CRCI block instance >> - number specified for the NAND controller on the given >> - platform >> + or BAM DMA controller node and the channel number to >> + be used for NAND. Refer to dma.txt, qcom_adm.txt(ADM) >> + and qcom/bam_dma.txt(BAM) for more details >> +- dma-names: "rxtx" - ADM >> + "tx", "rx", "cmd" - BAM >> +- qcom,cmd-crci: Only required for ADM DMA. must contain the ADM >> command >> + type CRCI block instance number specified for the NAND >> + controller on the given platform. >> +- qcom,data-crci: Only required for ADM DMA. must contain the ADM >> data >> + type CRCI block instance number specified for the NAND >> + controller on the given platform. >> - #address-cells: <1> - subnodes give the chip-select number >> - #size-cells: <0> >> >> @@ -95,6 +97,11 @@ nand@79b0000 { >> <&gcc GCC_QPIC_AHB_CLK>; >> clock-names = "core", "aon"; >> >> + dmas = <&qpicbam 0>, >> + <&qpicbam 1>, >> + <&qpicbam 2>; >> + dma-names = "tx", "rx", "cmd"; >> + >> #address-cells = <1>; >> #size-cells = <0>; >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c >> index f55f728..520add9 100644 >> --- a/drivers/mtd/nand/qcom_nandc.c >> +++ b/drivers/mtd/nand/qcom_nandc.c >> @@ -226,6 +226,7 @@ struct nandc_regs { >> * by upper layers directly >> * @buf_size/count/start: markers for chip->read_buf/write_buf >> functions >> * @reg_read_buf: local buffer for reading back registers via DMA >> + * @reg_read_buf_phys: contains dma address for register read buffer >> * @reg_read_pos: marker for data read in reg_read_buf >> * >> * @regs: a contiguous chunk of memory for DMA register >> @@ -249,9 +250,19 @@ struct qcom_nand_controller { >> struct clk *core_clk; >> struct clk *aon_clk; >> >> - struct dma_chan *chan; >> - unsigned int cmd_crci; >> - unsigned int data_crci; >> + union { >> + struct { >> + struct dma_chan *tx_chan; >> + struct dma_chan *rx_chan; >> + struct dma_chan *cmd_chan; >> + }; >> + struct { >> + struct dma_chan *chan; >> + unsigned int cmd_crci; >> + unsigned int data_crci; >> + }; >> + }; >> + >> struct list_head desc_list; >> >> u8 *data_buffer; >> @@ -261,6 +272,7 @@ struct qcom_nand_controller { >> int buf_start; >> >> __le32 *reg_read_buf; >> + dma_addr_t reg_read_buf_phys; >> int reg_read_pos; >> >> struct nandc_regs *regs; >> @@ -1956,16 +1968,48 @@ static int qcom_nandc_alloc(struct >> qcom_nand_controller *nandc) >> if (!nandc->regs) >> return -ENOMEM; >> >> - nandc->reg_read_buf = devm_kzalloc(nandc->dev, >> - MAX_REG_RD * sizeof(*nandc->reg_read_buf), >> - GFP_KERNEL); >> - if (!nandc->reg_read_buf) >> - return -ENOMEM; >> >> - nandc->chan = dma_request_slave_channel(nandc->dev, "rxtx"); >> - if (!nandc->chan) { >> - dev_err(nandc->dev, "failed to request slave channel\n"); >> - return -ENODEV; >> + if (!nandc->dma_bam_enabled) { >> + nandc->reg_read_buf = >> + devm_kzalloc(nandc->dev, MAX_REG_RD * >> + sizeof(*nandc->reg_read_buf), GFP_KERNEL); >> + >> + if (!nandc->reg_read_buf) >> + return -ENOMEM; >> + >> + nandc->chan = dma_request_slave_channel(nandc->dev, "rxtx"); >> + if (!nandc->chan) { >> + dev_err(nandc->dev, >> + "failed to request slave channel\n"); >> + return -ENODEV; >> + } >> + } else { >> + nandc->reg_read_buf = >> + dmam_alloc_coherent(nandc->dev, MAX_REG_RD * >> + sizeof(*nandc->reg_read_buf), >> + &nandc->reg_read_buf_phys, >> + GFP_KERNEL); >> + >> + if (!nandc->reg_read_buf) >> + return -ENOMEM; >> + >> + nandc->tx_chan = dma_request_slave_channel(nandc->dev, "tx"); >> + if (!nandc->tx_chan) { >> + dev_err(nandc->dev, "failed to request tx channel\n"); >> + return -ENODEV; >> + } >> + >> + nandc->rx_chan = dma_request_slave_channel(nandc->dev, "rx"); >> + if (!nandc->rx_chan) { >> + dev_err(nandc->dev, "failed to request rx channel\n"); >> + return -ENODEV; >> + } >> + >> + nandc->cmd_chan = dma_request_slave_channel(nandc->dev, "cmd"); >> + if (!nandc->cmd_chan) { >> + dev_err(nandc->dev, "failed to request cmd channel\n"); >> + return -ENODEV; >> + } >> } >> >> INIT_LIST_HEAD(&nandc->desc_list); >> @@ -1978,7 +2022,19 @@ static int qcom_nandc_alloc(struct >> qcom_nand_controller *nandc) >> >> static void qcom_nandc_unalloc(struct qcom_nand_controller *nandc) >> { >> - dma_release_channel(nandc->chan); >> + if (nandc->dma_bam_enabled) { >> + if (nandc->tx_chan) >> + dma_release_channel(nandc->tx_chan); >> + >> + if (nandc->rx_chan) >> + dma_release_channel(nandc->rx_chan); >> + >> + if (nandc->cmd_chan) >> + dma_release_channel(nandc->cmd_chan); >> + } else { >> + if (nandc->chan) >> + dma_release_channel(nandc->chan); >> + } >> } >> >> /* one time setup of a few nand controller registers */ >> @@ -2063,16 +2119,20 @@ static int qcom_nandc_parse_dt(struct >> platform_device *pdev) >> struct device_node *np = nandc->dev->of_node; >> int ret; >> >> - ret = of_property_read_u32(np, "qcom,cmd-crci", &nandc->cmd_crci); >> - if (ret) { >> - dev_err(nandc->dev, "command CRCI unspecified\n"); >> - return ret; >> - } >> + if (!nandc->dma_bam_enabled) { >> + ret = of_property_read_u32(np, "qcom,cmd-crci", >> + &nandc->cmd_crci); >> + if (ret) { >> + dev_err(nandc->dev, "command CRCI unspecified\n"); >> + return ret; >> + } >> >> - ret = of_property_read_u32(np, "qcom,data-crci", &nandc->data_crci); >> - if (ret) { >> - dev_err(nandc->dev, "data CRCI unspecified\n"); >> - return ret; >> + ret = of_property_read_u32(np, "qcom,data-crci", >> + &nandc->data_crci); >> + if (ret) { >> + dev_err(nandc->dev, "data CRCI unspecified\n"); >> + return ret; >> + } >> } >> >> return 0; >> @@ -2128,7 +2188,7 @@ static int qcom_nandc_probe(struct >> platform_device *pdev) >> >> ret = qcom_nandc_alloc(nandc); >> if (ret) >> - return ret; >> + goto err_core_clk; >> >> ret = clk_prepare_enable(nandc->core_clk); >> if (ret) >> -- Abhishek Sahu