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=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable 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 B923AC2D0F9 for ; Wed, 13 May 2020 09:20:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9E4FD20753 for ; Wed, 13 May 2020 09:20:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727918AbgEMJUa (ORCPT ); Wed, 13 May 2020 05:20:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44652 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727858AbgEMJU3 (ORCPT ); Wed, 13 May 2020 05:20:29 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6A4ADC061A0C for ; Wed, 13 May 2020 02:20:29 -0700 (PDT) Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jYnYq-0005Ol-6U; Wed, 13 May 2020 11:20:20 +0200 Received: from sha by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1jYnYn-0006w7-Sj; Wed, 13 May 2020 11:20:17 +0200 Date: Wed, 13 May 2020 11:20:17 +0200 From: Sascha Hauer To: Robin Gong Cc: "mark.rutland@arm.com" , "devicetree@vger.kernel.org" , "catalin.marinas@arm.com" , "kernel@pengutronix.de" , "shawnguo@kernel.org" , "will.deacon@arm.com" , "linux-kernel@vger.kernel.org" , "linux-spi@vger.kernel.org" , "vkoul@kernel.org" , "robh+dt@kernel.org" , dl-linux-imx , "martin.fuzzey@flowbird.group" , "u.kleine-koenig@pengutronix.de" , "dmaengine@vger.kernel.org" , "dan.j.williams@intel.com" , "festevam@gmail.com" , "linux-arm-kernel@lists.infradead.org" , "l.stach@pengutronix.de" Subject: Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165 Message-ID: <20200513092017.GQ5877@pengutronix.de> References: <1589218356-17475-1-git-send-email-yibin.gong@nxp.com> <1589218356-17475-8-git-send-email-yibin.gong@nxp.com> <20200513073359.GM5877@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 11:12:33 up 83 days, 16:43, 120 users, load average: 0.19, 0.21, 0.18 User-Agent: Mutt/1.10.1 (2018-07-13) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: dmaengine@vger.kernel.org Sender: dmaengine-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dmaengine@vger.kernel.org On Wed, May 13, 2020 at 09:05:33AM +0000, Robin Gong wrote: > On 2020/05/13 Sascha Hauer wrote:d > > > drivers/spi/spi-imx.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index > > > f4f28a4..70df8e6 100644 > > > --- a/drivers/spi/spi-imx.c > > > +++ b/drivers/spi/spi-imx.c > > > @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct > > spi_imx_data *spi_imx, > > > ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk); > > > spi_imx->spi_bus_clk = clk; > > > > > > - if (spi_imx->usedma) > > > - ctrl |= MX51_ECSPI_CTRL_SMC; > > > + /* ERR009165: work in XHC mode as PIO */ > > > + ctrl &= ~MX51_ECSPI_CTRL_SMC; > > > > > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); > > > > > > @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data > > *spi_imx) > > > * and enable DMA request. > > > */ > > > writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) | > > > - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) | > > > + MX51_ECSPI_DMA_TX_WML(0) | > > > MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) | > > > MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN | > > > MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA); > > @@ -1171,7 > > > +1171,11 @@ static int spi_imx_dma_configure(struct spi_master *master) > > > tx.direction = DMA_MEM_TO_DEV; > > > tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA; > > > tx.dst_addr_width = buswidth; > > > - tx.dst_maxburst = spi_imx->wml; > > > + /* > > > + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size > > > + * to speed up fifo filling as possible. > > > + */ > > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > In the next patch this is changed again to: > > > > + if (spi_imx->devtype_data->tx_glitch_fixed) > > + tx.dst_maxburst = spi_imx->wml; > > + else > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > So with tx_glitch_fixed we end up with tx.dst_maxburst being the same as two > > patches before which is rather confusing. Better introduce tx_glitch_fixed in > > this patch, or maybe even merge this patch and the next one. > Sorry confused you, I should repleace 'tx_wml=0' in the above comments > with ' TX_THRESHOLD=0', which means tx transfer dma have to wait all > the tx data in tx fifo transferred with ERR009165 rather than > generically 'tx_wml' (for example --half fifo size used as > TX_THRESHOLD). Obviously TX_THRESHOLD=0 would down performance, so > enlarge dst_maxburst to fifo size as PIO with ERR009165. After > ERR009165 fixed at HW level. TX_THRESHOLD could be used as common > 'spi_imx->wml' so change it back. Will add more detail information in > v8. I am not confused, I meant the patches are confusing. What you are doing is: No patch: tx.dst_maxburst = a; 1st patch tx.dst_maxburst = b; 2nd patch: if (foo) tx.dst_maxburst = a; else tx.dst_maxburst = b; It would be better readable and understandable if you did that in one patch, because that would directly say "Under certain conditions we have to choose a, otherwise b". That's much better than changing "a" to "b" and then to "a or b" Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | 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=-5.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 29059CA90AF for ; Wed, 13 May 2020 09:20:36 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EE2E820661 for ; Wed, 13 May 2020 09:20:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="EwWCTOSS" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EE2E820661 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Mc1MHb1Ty6KQhY/5zHWqOkk07HUTXr6uzQyaMnGrXXA=; b=EwWCTOSSIZr8uI yj95QbY3UR9DqGLb1s3Z4hI8IAwEnvNyzIsYbEosgXPD4qg1ar/5UeEGBQll14U6KiSTNLLEOuxS5 xSVn8ULGnIAfb9GzJqrzQXK/0+QLcqO/ddhupAHDYLUQZA0kZQvmzj7+D2aWRF67xO3eDXgBNtftF jJRyC4V420eXvmbB0eivOF4AIrEplQmBPgY11H3OI7TMTlh12rYcE5Ow3AmwNP/qI3jsY2rI4rVCX BRVKJ5idQvX/eebRSoC+cIxykXztDrsJnBVJW9MAVO2mYCVGbTTML81/Vrw5VE/OFgVwtY7ihjRlG Js0vwoiUYGU33qwW8bBA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jYnZ5-0000NY-9R; Wed, 13 May 2020 09:20:35 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jYnZ1-0000Mc-CO for linux-arm-kernel@lists.infradead.org; Wed, 13 May 2020 09:20:33 +0000 Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jYnYq-0005Ol-6U; Wed, 13 May 2020 11:20:20 +0200 Received: from sha by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1jYnYn-0006w7-Sj; Wed, 13 May 2020 11:20:17 +0200 Date: Wed, 13 May 2020 11:20:17 +0200 From: Sascha Hauer To: Robin Gong Subject: Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165 Message-ID: <20200513092017.GQ5877@pengutronix.de> References: <1589218356-17475-1-git-send-email-yibin.gong@nxp.com> <1589218356-17475-8-git-send-email-yibin.gong@nxp.com> <20200513073359.GM5877@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 11:12:33 up 83 days, 16:43, 120 users, load average: 0.19, 0.21, 0.18 User-Agent: Mutt/1.10.1 (2018-07-13) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200513_022031_418972_4DFD7E60 X-CRM114-Status: GOOD ( 23.66 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "mark.rutland@arm.com" , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "martin.fuzzey@flowbird.group" , "catalin.marinas@arm.com" , "will.deacon@arm.com" , "linux-kernel@vger.kernel.org" , "linux-spi@vger.kernel.org" , "vkoul@kernel.org" , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "u.kleine-koenig@pengutronix.de" , "dmaengine@vger.kernel.org" , "dan.j.williams@intel.com" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "l.stach@pengutronix.de" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, May 13, 2020 at 09:05:33AM +0000, Robin Gong wrote: > On 2020/05/13 Sascha Hauer wrote:d > > > drivers/spi/spi-imx.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index > > > f4f28a4..70df8e6 100644 > > > --- a/drivers/spi/spi-imx.c > > > +++ b/drivers/spi/spi-imx.c > > > @@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct > > spi_imx_data *spi_imx, > > > ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk); > > > spi_imx->spi_bus_clk = clk; > > > > > > - if (spi_imx->usedma) > > > - ctrl |= MX51_ECSPI_CTRL_SMC; > > > + /* ERR009165: work in XHC mode as PIO */ > > > + ctrl &= ~MX51_ECSPI_CTRL_SMC; > > > > > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); > > > > > > @@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data > > *spi_imx) > > > * and enable DMA request. > > > */ > > > writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) | > > > - MX51_ECSPI_DMA_TX_WML(spi_imx->wml) | > > > + MX51_ECSPI_DMA_TX_WML(0) | > > > MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) | > > > MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN | > > > MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA); > > @@ -1171,7 > > > +1171,11 @@ static int spi_imx_dma_configure(struct spi_master *master) > > > tx.direction = DMA_MEM_TO_DEV; > > > tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA; > > > tx.dst_addr_width = buswidth; > > > - tx.dst_maxburst = spi_imx->wml; > > > + /* > > > + * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size > > > + * to speed up fifo filling as possible. > > > + */ > > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > In the next patch this is changed again to: > > > > + if (spi_imx->devtype_data->tx_glitch_fixed) > > + tx.dst_maxburst = spi_imx->wml; > > + else > > + tx.dst_maxburst = spi_imx->devtype_data->fifo_size; > > > > So with tx_glitch_fixed we end up with tx.dst_maxburst being the same as two > > patches before which is rather confusing. Better introduce tx_glitch_fixed in > > this patch, or maybe even merge this patch and the next one. > Sorry confused you, I should repleace 'tx_wml=0' in the above comments > with ' TX_THRESHOLD=0', which means tx transfer dma have to wait all > the tx data in tx fifo transferred with ERR009165 rather than > generically 'tx_wml' (for example --half fifo size used as > TX_THRESHOLD). Obviously TX_THRESHOLD=0 would down performance, so > enlarge dst_maxburst to fifo size as PIO with ERR009165. After > ERR009165 fixed at HW level. TX_THRESHOLD could be used as common > 'spi_imx->wml' so change it back. Will add more detail information in > v8. I am not confused, I meant the patches are confusing. What you are doing is: No patch: tx.dst_maxburst = a; 1st patch tx.dst_maxburst = b; 2nd patch: if (foo) tx.dst_maxburst = a; else tx.dst_maxburst = b; It would be better readable and understandable if you did that in one patch, because that would directly say "Under certain conditions we have to choose a, otherwise b". That's much better than changing "a" to "b" and then to "a or b" Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel