Message ID | 20221103052843.2025-2-bayi.cheng@mediatek.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp332853wru; Wed, 2 Nov 2022 22:34:14 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7TS6Yivsqpj/x0yC/whaNG7DaJ8dblnzNmu6DyxI8d4FTtgYFxRlSVy1uaLwFVdDHRBFz6 X-Received: by 2002:a17:907:728a:b0:78d:2b4b:e7f7 with SMTP id dt10-20020a170907728a00b0078d2b4be7f7mr28125776ejc.269.1667453654520; Wed, 02 Nov 2022 22:34:14 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667453654; cv=none; d=google.com; s=arc-20160816; b=cBrDXKPCv3uLXmlXXlBJBYxaLu5RfMOWdvgNIdi4koVneWnmLI8/S9ie2QOHqDEm5C tbX7cYCkxtz05K8MeHIuJFdWXA89FDw7pLgjThB/tNcbJCK8Vkhj68VPEJezmjUqeexL YqORFJw2xML9O95nMzqB2IjvZhaiPswWpWhdUGZOF94HWhov/0w8d9kjz58T+FrPSTdj RUP8u/0c0QiETDxeds0VkHVrjm40TKoDoOcnBvk+DXwUvAcRkBaPGs6E7lBrdXyf9GEi uO9hpkjwt0p2JX2Ncqp3RDnnSc11anFQU96ZFy5IjJ3SisHv2bwfKwRz3JYLWQkGf0HJ y3eA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=AWuhXliJf4LNGvUCz31n1TTeINGwZ1aIEvAL7BSapmI=; b=PWsCSx7Q0MaoCFMfMh7aYurQ9LvV9edSeLQ3hNRfLyq6j3OdKyVQNtaEGvoNlPFKtH KJvQBtXuLmrzWeBZcqMirM4m7OpnPWlcL0kjctdAuKhVS7ww7d7UAz4ORYV1byMam7Xd 9tPR3HuRN/br6gvTITMEWjbmWngWhl90Ve9LN3IlxPq6XBJRdn92HOYQQ2QMfRq9ab++ pWQNG0XeeM0nWA3VSRrfs9Ta/QAnKUEisiZ2uKSdozPAa10AGZ/0/L8PDr86SYrZzvR9 k/yqBo57Se7b01YaiMOmoyrP0gOrr/k9hcI1zg4kiFqLHtPNuZGwDeK1pxvhrGZl4dwb HBrw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@mediatek.com header.s=dk header.b=NjF0mb3a; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=mediatek.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d2-20020a50fe82000000b004602df03e36si32617edt.486.2022.11.02.22.33.51; Wed, 02 Nov 2022 22:34:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@mediatek.com header.s=dk header.b=NjF0mb3a; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=mediatek.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230194AbiKCF33 (ORCPT <rfc822;yves.mi.zy@gmail.com> + 99 others); Thu, 3 Nov 2022 01:29:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56046 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229595AbiKCF31 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 3 Nov 2022 01:29:27 -0400 Received: from mailgw02.mediatek.com (unknown [210.61.82.184]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DD2145FAA; Wed, 2 Nov 2022 22:29:21 -0700 (PDT) X-UUID: e50e325deb9249638069d2e84d51f692-20221103 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mediatek.com; s=dk; h=Content-Type:Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:CC:To:From; bh=AWuhXliJf4LNGvUCz31n1TTeINGwZ1aIEvAL7BSapmI=; b=NjF0mb3a6LWmCRZmAQfsFlHSezhnmi4EJ40c6rjHBW3+yrDFg2u7A+sCia/qclvpyNBlllXlnOo067wdJFi+tVdoeEm/6z2kUnTnm52qGh86OWnxl7/l0wlwmNCbteTfY3u1q0ClQPw5h3/iRZTxpu5HOXDgL5nMsibSTenSZlg=; X-CID-P-RULE: Release_Ham X-CID-O-INFO: VERSION:1.1.12,REQID:61d56722-8b60-46cf-bd20-1738581aee07,IP:0,U RL:0,TC:0,Content:-25,EDM:0,RT:0,SF:0,FILE:0,BULK:0,RULE:Release_Ham,ACTIO N:release,TS:-25 X-CID-META: VersionHash:62cd327,CLOUDID:ea904aeb-84ac-4628-a416-bc50d5503da6,B ulkID:nil,BulkQuantity:0,Recheck:0,SF:102,TC:nil,Content:0,EDM:-3,IP:nil,U RL:0,File:nil,Bulk:nil,QS:nil,BEC:nil,COL:0 X-UUID: e50e325deb9249638069d2e84d51f692-20221103 Received: from mtkmbs11n1.mediatek.inc [(172.21.101.185)] by mailgw02.mediatek.com (envelope-from <bayi.cheng@mediatek.com>) (Generic MTA with TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384 256/256) with ESMTP id 1318099482; Thu, 03 Nov 2022 13:29:15 +0800 Received: from mtkmbs11n2.mediatek.inc (172.21.101.187) by mtkmbs13n1.mediatek.inc (172.21.101.193) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.792.15; Thu, 3 Nov 2022 13:29:13 +0800 Received: from localhost.localdomain (10.17.3.154) by mtkmbs11n2.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.2.792.15 via Frontend Transport; Thu, 3 Nov 2022 13:29:12 +0800 From: Bayi Cheng <bayi.cheng@mediatek.com> To: Mark Brown <broonie@kernel.org> CC: Matthias Brugger <matthias.bgg@gmail.com>, <linux-spi@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-mediatek@lists.infradead.org>, Chuanhong Guo <gch981213@gmail.com>, <linux-kernel@vger.kernel.org>, <srv_heupstream@mediatek.com>, Project_Global_Chrome_Upstream_Group <Project_Global_Chrome_Upstream_Group@mediatek.com>, bayi cheng <bayi.cheng@mediatek.com> Subject: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read Date: Thu, 3 Nov 2022 13:28:43 +0800 Message-ID: <20221103052843.2025-2-bayi.cheng@mediatek.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20221103052843.2025-1-bayi.cheng@mediatek.com> References: <20221103052843.2025-1-bayi.cheng@mediatek.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-MTK: N X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS, T_SPF_TEMPERROR,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748451882990527960?= X-GMAIL-MSGID: =?utf-8?q?1748451882990527960?= |
Series |
[v1] spi: spi-mtk-nor: Optimize timeout for dma read
|
|
Commit Message
Bayi Cheng
Nov. 3, 2022, 5:28 a.m. UTC
From: bayi cheng <bayi.cheng@mediatek.com> The timeout value of the current dma read is unreasonable. For example, If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a 4KB data in spi mode. But the actual measurement exceeds 50s when a dma read timeout is encountered. In order to be more accurately, It is necessary to use msecs_to_jiffies, After modification, the measured timeout value is about 130ms. Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> --- drivers/spi/spi-mtk-nor.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Comments
Il 03/11/22 06:28, Bayi Cheng ha scritto: > From: bayi cheng <bayi.cheng@mediatek.com> > > The timeout value of the current dma read is unreasonable. For example, > If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a > 4KB data in spi mode. But the actual measurement exceeds 50s when a > dma read timeout is encountered. > > In order to be more accurately, It is necessary to use msecs_to_jiffies, > After modification, the measured timeout value is about 130ms. > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> > --- > drivers/spi/spi-mtk-nor.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c > index d167699a1a96..3d989db80ee9 100644 > --- a/drivers/spi/spi-mtk-nor.c > +++ b/drivers/spi/spi-mtk-nor.c > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, > dma_addr_t dma_addr) > { > int ret = 0; > - ulong delay; > + ulong delay, timeout; > u32 reg; > > writel(from, sp->base + MTK_NOR_REG_DMA_FADR); > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, > mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0); > > delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); > + timeout = (delay + 1) * 100; > > if (sp->has_irq) { > if (!wait_for_completion_timeout(&sp->op_done, > - (delay + 1) * 100)) > + msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)))) You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`, this function takes a `const unsigned int` param. Please change the type to match that. Aside from that, your `timeout` variable contains a timeout in microseconds and this means that actually using msecs_to_jiffies() is suboptimal here. Please use usecs_to_jiffies() instead. Regards, Angelo
From: AngeloGioacchino Del Regno > Sent: 03 November 2022 09:44 > > Il 03/11/22 06:28, Bayi Cheng ha scritto: > > From: bayi cheng <bayi.cheng@mediatek.com> > > > > The timeout value of the current dma read is unreasonable. For example, > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a > > 4KB data in spi mode. But the actual measurement exceeds 50s when a > > dma read timeout is encountered. > > > > In order to be more accurately, It is necessary to use msecs_to_jiffies, > > After modification, the measured timeout value is about 130ms. > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> > > --- > > drivers/spi/spi-mtk-nor.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c > > index d167699a1a96..3d989db80ee9 100644 > > --- a/drivers/spi/spi-mtk-nor.c > > +++ b/drivers/spi/spi-mtk-nor.c > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, > > dma_addr_t dma_addr) > > { > > int ret = 0; > > - ulong delay; > > + ulong delay, timeout; > > u32 reg; > > > > writel(from, sp->base + MTK_NOR_REG_DMA_FADR); > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, > > mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0); > > > > delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); > > + timeout = (delay + 1) * 100; > > > > if (sp->has_irq) { > > if (!wait_for_completion_timeout(&sp->op_done, > > - (delay + 1) * 100)) > > + msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)))) > > You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`, > this function takes a `const unsigned int` param. > Please change the type to match that. The type shouldn't matter at all. What matters is the domain of the value. Quite why you need to use max_t(size_t, ...) is another matter. timeout is ulong so max(timeout/1000, 10ul) should be fine. But is ulong even right? The domain of the value is almost certainly the same on 32bit and 64bit. So you almost certainly need u32 or u64. David > > Aside from that, your `timeout` variable contains a timeout in microseconds and > this means that actually using msecs_to_jiffies() is suboptimal here. > > Please use usecs_to_jiffies() instead. > > Regards, > Angelo - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote: > From: AngeloGioacchino Del Regno > > Sent: 03 November 2022 09:44 > > > > Il 03/11/22 06:28, Bayi Cheng ha scritto: > > > From: bayi cheng <bayi.cheng@mediatek.com> > > > > > > The timeout value of the current dma read is unreasonable. For > > > example, > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to > > > read a > > > 4KB data in spi mode. But the actual measurement exceeds 50s when > > > a > > > dma read timeout is encountered. > > > > > > In order to be more accurately, It is necessary to use > > > msecs_to_jiffies, > > > After modification, the measured timeout value is about 130ms. > > > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> > > > --- > > > drivers/spi/spi-mtk-nor.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk- > > > nor.c > > > index d167699a1a96..3d989db80ee9 100644 > > > --- a/drivers/spi/spi-mtk-nor.c > > > +++ b/drivers/spi/spi-mtk-nor.c > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor > > > *sp, u32 from, unsigned int length, > > > dma_addr_t dma_addr) > > > { > > > int ret = 0; > > > - ulong delay; > > > + ulong delay, timeout; > > > u32 reg; > > > > > > writel(from, sp->base + MTK_NOR_REG_DMA_FADR); > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor > > > *sp, u32 from, unsigned int length, > > > mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, > > > 0); > > > > > > delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); > > > + timeout = (delay + 1) * 100; > > > > > > if (sp->has_irq) { > > > if (!wait_for_completion_timeout(&sp->op_done, > > > - (delay + 1) * 100)) > > > + msecs_to_jiffies(max_t(size_t, timeout / 1000, > > > 10)))) > > > > You're giving a `size_t` variable to msecs_to_jiffies(), but > > checking `jiffies.h`, > > this function takes a `const unsigned int` param. > > Please change the type to match that. > > The type shouldn't matter at all. > What matters is the domain of the value. > > Quite why you need to use max_t(size_t, ...) is another matter. > timeout is ulong so max(timeout/1000, 10ul) should be fine. > > But is ulong even right? > The domain of the value is almost certainly the same on 32bit and > 64bit. > So you almost certainly need u32 or u64. > > David > Hi David & Angelo Thank you for your comments! To sum up, I think the next version will make the following two changes: 1, The timeout value will not exceed u32, so the type of timeout will be changed from ulong to u32. 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be msecs_to_jiffies(max(timeout/1000, 10ul)). If you think these changes are not enough, please let me know, Thanks! Best Regards, Bayi > > > > Aside from that, your `timeout` variable contains a timeout in > > microseconds and > > this means that actually using msecs_to_jiffies() is suboptimal > > here. > > > > Please use usecs_to_jiffies() instead. > > > > Regards, > > Angelo > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK > Registration No: 1397386 (Wales)
On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote: > On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote: > > From: AngeloGioacchino Del Regno > > > Sent: 03 November 2022 09:44 > > > > > > Il 03/11/22 06:28, Bayi Cheng ha scritto: > > > > From: bayi cheng <bayi.cheng@mediatek.com> > > > > > > > > The timeout value of the current dma read is unreasonable. For > > > > example, > > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to > > > > read a > > > > 4KB data in spi mode. But the actual measurement exceeds 50s > > > > when > > > > a > > > > dma read timeout is encountered. > > > > > > > > In order to be more accurately, It is necessary to use > > > > msecs_to_jiffies, > > > > After modification, the measured timeout value is about 130ms. > > > > > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> > > > > --- > > > > drivers/spi/spi-mtk-nor.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk- > > > > nor.c > > > > index d167699a1a96..3d989db80ee9 100644 > > > > --- a/drivers/spi/spi-mtk-nor.c > > > > +++ b/drivers/spi/spi-mtk-nor.c > > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor > > > > *sp, u32 from, unsigned int length, > > > > dma_addr_t dma_addr) > > > > { > > > > int ret = 0; > > > > - ulong delay; > > > > + ulong delay, timeout; > > > > u32 reg; > > > > > > > > writel(from, sp->base + MTK_NOR_REG_DMA_FADR); > > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct > > > > mtk_nor > > > > *sp, u32 from, unsigned int length, > > > > mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, > > > > 0); > > > > > > > > delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); > > > > + timeout = (delay + 1) * 100; > > > > > > > > if (sp->has_irq) { > > > > if (!wait_for_completion_timeout(&sp->op_done, > > > > - (delay + 1) * > > > > 100)) > > > > + msecs_to_jiffies(max_t(size_t, timeout / > > > > 1000, > > > > 10)))) > > > > > > You're giving a `size_t` variable to msecs_to_jiffies(), but > > > checking `jiffies.h`, > > > this function takes a `const unsigned int` param. > > > Please change the type to match that. > > > > The type shouldn't matter at all. > > What matters is the domain of the value. > > > > Quite why you need to use max_t(size_t, ...) is another matter. > > timeout is ulong so max(timeout/1000, 10ul) should be fine. > > > > But is ulong even right? > > The domain of the value is almost certainly the same on 32bit and > > 64bit. > > So you almost certainly need u32 or u64. > > > > David > > > > Hi David & Angelo > > Thank you for your comments! > To sum up, I think the next version will make the following two > changes: > 1, The timeout value will not exceed u32, so the type of timeout will > be changed from ulong to u32. > 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be > msecs_to_jiffies(max(timeout / 1000, 10U)). > > If you think these changes are not enough, please let me know, > Thanks! > > Best Regards, > Bayi > Hi Angelo, Hi David, Just a gentle ping on this. Could you please review this patch and give us some suggestion? PS: With your permission, I will make the following changes in the next version: Change in v2: -Change the type of "timeout" from ulong to u32. -Replace max_t with max. Thanks. BRs, Bayi Cheng > > > > > > Aside from that, your `timeout` variable contains a timeout in > > > microseconds and > > > this means that actually using msecs_to_jiffies() is suboptimal > > > here. > > > > > > Please use usecs_to_jiffies() instead. > > > > > > Regards, > > > Angelo > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton > > Keynes, > > MK1 1PT, UK > > Registration No: 1397386 (Wales)
Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto: > On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote: >> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote: >>> From: AngeloGioacchino Del Regno >>>> Sent: 03 November 2022 09:44 >>>> >>>> Il 03/11/22 06:28, Bayi Cheng ha scritto: >>>>> From: bayi cheng <bayi.cheng@mediatek.com> >>>>> >>>>> The timeout value of the current dma read is unreasonable. For >>>>> example, >>>>> If the spi flash clock is 26Mhz, It will takes about 1.3ms to >>>>> read a >>>>> 4KB data in spi mode. But the actual measurement exceeds 50s >>>>> when >>>>> a >>>>> dma read timeout is encountered. >>>>> >>>>> In order to be more accurately, It is necessary to use >>>>> msecs_to_jiffies, >>>>> After modification, the measured timeout value is about 130ms. >>>>> >>>>> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> >>>>> --- >>>>> drivers/spi/spi-mtk-nor.c | 7 ++++--- >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk- >>>>> nor.c >>>>> index d167699a1a96..3d989db80ee9 100644 >>>>> --- a/drivers/spi/spi-mtk-nor.c >>>>> +++ b/drivers/spi/spi-mtk-nor.c >>>>> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor >>>>> *sp, u32 from, unsigned int length, >>>>> dma_addr_t dma_addr) >>>>> { >>>>> int ret = 0; >>>>> - ulong delay; >>>>> + ulong delay, timeout; >>>>> u32 reg; >>>>> >>>>> writel(from, sp->base + MTK_NOR_REG_DMA_FADR); >>>>> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct >>>>> mtk_nor >>>>> *sp, u32 from, unsigned int length, >>>>> mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, >>>>> 0); >>>>> >>>>> delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); >>>>> + timeout = (delay + 1) * 100; >>>>> >>>>> if (sp->has_irq) { >>>>> if (!wait_for_completion_timeout(&sp->op_done, >>>>> - (delay + 1) * >>>>> 100)) >>>>> + msecs_to_jiffies(max_t(size_t, timeout / >>>>> 1000, >>>>> 10)))) >>>> >>>> You're giving a `size_t` variable to msecs_to_jiffies(), but >>>> checking `jiffies.h`, >>>> this function takes a `const unsigned int` param. >>>> Please change the type to match that. >>> >>> The type shouldn't matter at all. >>> What matters is the domain of the value. >>> >>> Quite why you need to use max_t(size_t, ...) is another matter. >>> timeout is ulong so max(timeout/1000, 10ul) should be fine. >>> >>> But is ulong even right? >>> The domain of the value is almost certainly the same on 32bit and >>> 64bit. >>> So you almost certainly need u32 or u64. >>> >>> David >>> >> >> Hi David & Angelo >> >> Thank you for your comments! >> To sum up, I think the next version will make the following two >> changes: >> 1, The timeout value will not exceed u32, so the type of timeout will >> be changed from ulong to u32. >> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be >> msecs_to_jiffies(max(timeout / 1000, 10U)). >> >> If you think these changes are not enough, please let me know, >> Thanks! >> >> Best Regards, >> Bayi >> > > Hi Angelo, Hi David, > > Just a gentle ping on this. > Could you please review this patch and give us some suggestion? > > PS: With your permission, I will make the following changes in the next > version: > > Change in v2: > -Change the type of "timeout" from ulong to u32. > -Replace max_t with max. > I still recommend to use usecs_to_jiffies() when appropriate, instead of converting usecs to msecs and using msecs_to_jiffies(). As for the rest in your list: yes, please. Regards, Angelo > > Thanks. > > BRs, > Bayi Cheng > >>>> >>>> Aside from that, your `timeout` variable contains a timeout in >>>> microseconds and >>>> this means that actually using msecs_to_jiffies() is suboptimal >>>> here. >>>> >>>> Please use usecs_to_jiffies() instead. >>>> >>>> Regards, >>>> Angelo >>> >>> - >>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton >>> Keynes, >>> MK1 1PT, UK >>> Registration No: 1397386 (Wales)
On Fri, 2022-11-11 at 10:16 +0100, AngeloGioacchino Del Regno wrote: > Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto: > > On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote: > > > On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote: > > > > From: AngeloGioacchino Del Regno > > > > > Sent: 03 November 2022 09:44 > > > > > > > > > > Il 03/11/22 06:28, Bayi Cheng ha scritto: > > > > > > From: bayi cheng <bayi.cheng@mediatek.com> > > > > > > > > > > > > The timeout value of the current dma read is unreasonable. > > > > > > For > > > > > > example, > > > > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms > > > > > > to > > > > > > read a > > > > > > 4KB data in spi mode. But the actual measurement exceeds > > > > > > 50s > > > > > > when > > > > > > a > > > > > > dma read timeout is encountered. > > > > > > > > > > > > In order to be more accurately, It is necessary to use > > > > > > msecs_to_jiffies, > > > > > > After modification, the measured timeout value is about > > > > > > 130ms. > > > > > > > > > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com> > > > > > > --- > > > > > > drivers/spi/spi-mtk-nor.c | 7 ++++--- > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi- > > > > > > mtk- > > > > > > nor.c > > > > > > index d167699a1a96..3d989db80ee9 100644 > > > > > > --- a/drivers/spi/spi-mtk-nor.c > > > > > > +++ b/drivers/spi/spi-mtk-nor.c > > > > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct > > > > > > mtk_nor > > > > > > *sp, u32 from, unsigned int length, > > > > > > dma_addr_t dma_addr) > > > > > > { > > > > > > int ret = 0; > > > > > > - ulong delay; > > > > > > + ulong delay, timeout; > > > > > > u32 reg; > > > > > > > > > > > > writel(from, sp->base + MTK_NOR_REG_DMA_FADR); > > > > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct > > > > > > mtk_nor > > > > > > *sp, u32 from, unsigned int length, > > > > > > mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, > > > > > > 0); > > > > > > > > > > > > delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); > > > > > > + timeout = (delay + 1) * 100; > > > > > > > > > > > > if (sp->has_irq) { > > > > > > if (!wait_for_completion_timeout(&sp->op_done, > > > > > > - (delay + 1) * > > > > > > 100)) > > > > > > + msecs_to_jiffies(max_t(size_t, timeout / > > > > > > 1000, > > > > > > 10)))) > > > > > > > > > > You're giving a `size_t` variable to msecs_to_jiffies(), but > > > > > checking `jiffies.h`, > > > > > this function takes a `const unsigned int` param. > > > > > Please change the type to match that. > > > > > > > > The type shouldn't matter at all. > > > > What matters is the domain of the value. > > > > > > > > Quite why you need to use max_t(size_t, ...) is another matter. > > > > timeout is ulong so max(timeout/1000, 10ul) should be fine. > > > > > > > > But is ulong even right? > > > > The domain of the value is almost certainly the same on 32bit > > > > and > > > > 64bit. > > > > So you almost certainly need u32 or u64. > > > > > > > > David > > > > > > > > > > Hi David & Angelo > > > > > > Thank you for your comments! > > > To sum up, I think the next version will make the following two > > > changes: > > > 1, The timeout value will not exceed u32, so the type of timeout > > > will > > > be changed from ulong to u32. > > > 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to > > > be > > > msecs_to_jiffies(max(timeout / 1000, 10U)). > > > > > > If you think these changes are not enough, please let me know, > > > Thanks! > > > > > > Best Regards, > > > Bayi > > > > > > > Hi Angelo, Hi David, > > > > Just a gentle ping on this. > > Could you please review this patch and give us some suggestion? > > > > PS: With your permission, I will make the following changes in the > > next > > version: > > > > Change in v2: > > -Change the type of "timeout" from ulong to u32. > > -Replace max_t with max. > > > > I still recommend to use usecs_to_jiffies() when appropriate, instead > of > converting usecs to msecs and using msecs_to_jiffies(). > > As for the rest in your list: yes, please. > > Regards, > Angelo > Hi Angelo, Thanks for your comments! I will change it to be usecs_to_jiffies(max(timeout, 10000U)) in the next patch. BRs, Bayi Cheng > > > > Thanks. > > > > BRs, > > Bayi Cheng > > > > > > > > > > > > Aside from that, your `timeout` variable contains a timeout > > > > > in > > > > > microseconds and > > > > > this means that actually using msecs_to_jiffies() is > > > > > suboptimal > > > > > here. > > > > > > > > > > Please use usecs_to_jiffies() instead. > > > > > > > > > > Regards, > > > > > Angelo > > > > > > > > - > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton > > > > Keynes, > > > > MK1 1PT, UK > > > > Registration No: 1397386 (Wales) > >
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c index d167699a1a96..3d989db80ee9 100644 --- a/drivers/spi/spi-mtk-nor.c +++ b/drivers/spi/spi-mtk-nor.c @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, dma_addr_t dma_addr) { int ret = 0; - ulong delay; + ulong delay, timeout; u32 reg; writel(from, sp->base + MTK_NOR_REG_DMA_FADR); @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length, mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0); delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE); + timeout = (delay + 1) * 100; if (sp->has_irq) { if (!wait_for_completion_timeout(&sp->op_done, - (delay + 1) * 100)) + msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)))) ret = -ETIMEDOUT; } else { ret = readl_poll_timeout(sp->base + MTK_NOR_REG_DMA_CTL, reg, !(reg & MTK_NOR_DMA_START), delay / 3, - (delay + 1) * 100); + timeout); } if (ret < 0)