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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EA401C433F5 for ; Wed, 8 Dec 2021 22:01:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237768AbhLHWFS (ORCPT ); Wed, 8 Dec 2021 17:05:18 -0500 Received: from smtp2.axis.com ([195.60.68.18]:42852 "EHLO smtp2.axis.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229604AbhLHWFS (ORCPT ); Wed, 8 Dec 2021 17:05:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1639000905; x=1670536905; h=date:to:cc:subject:message-id:references:mime-version: content-transfer-encoding:in-reply-to:from; bh=VRt6/LjubwnNkpPYOgxI34z0OGVQF3Sie8iEQwlJ3so=; b=prnJ8F9BBJZO0uWpzTpMiGIc00SqYOUITLSLpPlni1aWrTGrf2PgcfaZ j79LQkxfHC9c5O41nJX3LPP9dW0Ort4V/DCtxm/NdvzEJP7kqfGhecqW/ LSP5FhIULtOrDcs3R9Lnjhs6qtqu9Rp89592iLiodqg8byJV9LC61MErj IksL77EagIeB2SMxhiNC8Cx3hEOi0bE6eTyRjKDC7uh5v28YyzJN7ldJL cQAt4C2G14VbilnFDiVHJuUs0IFVrVBUZ9yEcFm8JffXYqyHzDh9Ey4zi KUUZwpCQwJPwKaPZOKdMkZyOdah+uVSWXSVhDKqtU57MZGzUqvkyYI8N0 g==; Date: Wed, 8 Dec 2021 23:01:44 +0100 To: Ulf Hansson CC: =?iso-8859-1?Q?M=E5rten?= Lindahl , Rob Herring , Krzysztof Kozlowski , Jaehoon Chung , Doug Anderson , kernel , "linux-mmc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-samsung-soc@vger.kernel.org" Subject: Re: [PATCH v2 4/4] mmc: dw_mmc: Do not wait for DTO in case of error Message-ID: <20211208220144.GA10156@axis.com> References: <20211206142929.26729-1-marten.lindahl@axis.com> <20211206142929.26729-5-marten.lindahl@axis.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) From: Marten Lindahl Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org On Wed, Dec 08, 2021 at 03:53:54PM +0100, Ulf Hansson wrote: > On Mon, 6 Dec 2021 at 15:29, Mårten Lindahl wrote: > > > > When running the ARTPEC-8 DWMMC IP version, and a data error interrupt > > comes during a data read transfer, there is no guarantee for the data > > transfer over interrupt (DTO) to come within the specified data timeout. > > This case is handled by the dto_timer handler which will complete the > > request with the comment: > > > > /* > > * If DTO interrupt does NOT come in sending data state, > > * we should notify the driver to terminate current transfer > > * and report a data timeout to the core. > > */ > > > > But since the ARTPEC-8 DWMMC IP version, supports an extended TMOUT > > register which allows longer timeouts than the non ARTPEC-8 version > > does, waiting for the dto_timer to complete the request in error cases > > may cause the request to take significantly longer time than necessary. > > This is specifically true for the failing steps during tuning of a > > device. > > > > Fix this by completing the request when the error interrupt comes. > > > > Signed-off-by: Mårten Lindahl > Hi Ulf! > Okay, this change looks a bit inconvenient to move into variant > specific callbacks. So, maybe the "quirks" flag makes sense, after > all. However, I would still look at using callbacks and library > functions, for the part implemented in patch3. Yes, I don't see how this patch can be easily made with callbacks, but definitely for patch3. So then I move the definition of the quirk from patch3 to this patch. > > When it comes to the order of the patches in the series, I suggest > flipping things around and making patch2 the final piece. Otherwise > the support for the artpec variant will be broken between patch2 and > patch4, right? Ok, you mean there may be a risk that the ARTPEC-8 dw_mmc does not work if the support is enabled in patch2, but patch3 and patch4 is not in place? That is a good point, but it actually does work quite fine (most of the time) without the extended timeout function. But it does not use the full function of the data timeout, and the HW timeout is most often set to full timeout (0xFFFFFF => 587ms with 200MHz), but the SW timer is limited to a lower value (0xFFFFFF => 84 + 10 ms with 200MHz). My reasoning is: patch1 - dtbindings for ARTPEC-8 patch2 - adding ARTPEC-8 to dw_mmc-exynos patch3 - implement ARTPEC-8 specific function for data timeout patch4 - add quirk to abort the extended timeout in case of errors, used by ARTPEC-8 so, this means patch3 and patch4 depends on patch2, and patch4 depends on patch3. Kind regards Mårten > > Kind regards > Uffe > > > --- > > drivers/mmc/host/dw_mmc.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index 45ea9fd97a6a..d6b76f47b1a2 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -2777,11 +2777,19 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id) > > if (pending & DW_MCI_DATA_ERROR_FLAGS) { > > spin_lock(&host->irq_lock); > > > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > > + del_timer(&host->dto_timer); > > + > > /* if there is an error report DATA_ERROR */ > > mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS); > > host->data_status = pending; > > smp_wmb(); /* drain writebuffer */ > > set_bit(EVENT_DATA_ERROR, &host->pending_events); > > + > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > > + /* In case of error, we cannot expect a DTO */ > > + set_bit(EVENT_DATA_COMPLETE, &host->pending_events); > > + > > tasklet_schedule(&host->tasklet); > > > > spin_unlock(&host->irq_lock); > > -- > > 2.20.1 > > 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 87780C433EF for ; Wed, 8 Dec 2021 22:04:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:From:In-Reply-To:MIME-Version: References:Message-ID:Subject:CC:To:Date:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=LwieSr35RU0G19n2Ls17oSd3I1PTIisZV0ySLMZNF2o=; b=QJoXPzHoFt832803SuBIaefi9A 0DW6aqD6srC/p37FJAr+9lwEMfnp54VGJEUp/oOIjacDJ1T6/nmJojq9rhPNz7G0JJRPvxVKazOpQ eT6Oj/5SdHW9NJfp9b40hw1/+n/LfYfTLsvoIgtY63HSjmdttb5+wTiQAdE5A7VjyxS5bhGZ76g93 3JjqDNz/uJ25mCoeBNGQ+ii/JyJsCPaMruoHH78Rgxnh0t7IXahkfyxQUtnzqN3CyICCva0RGFwFX 2eDCZHWzvNam6vrLxhdBmBxvAc9x9WSAmM882NR2i8tf4BHT5UXzunGbErtc583LCbPLszApN5l+W GeevK5qg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mv50d-00EMYt-JB; Wed, 08 Dec 2021 22:01:55 +0000 Received: from smtp2.axis.com ([195.60.68.18]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mv50U-00EMXG-O7 for linux-arm-kernel@lists.infradead.org; Wed, 08 Dec 2021 22:01:48 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1639000906; x=1670536906; h=date:to:cc:subject:message-id:references:mime-version: content-transfer-encoding:in-reply-to:from; bh=VRt6/LjubwnNkpPYOgxI34z0OGVQF3Sie8iEQwlJ3so=; b=HtF69DprxCeOdLo8oJzdSvE8W1qjJN6R2Vv7bCHPq/K9L7imhGwk9JrB aVucTtdrN4lSOZf8j2CWpmi9ejyN+2d74FJ3L2opZrE2O5Dq9KWHxE81H rfuTBZn+8OltRObA+mGDadamKaWBnEi31claVMR1KR+K9Co8MPRSFI1D4 SNDGm6VXPXYSvtsGLo1/Y1MITVbJBmHBnsIxN0iiKgOVdAw7Pjt1LtSUH FTIf40/wMSN2FHxoTgDAq8z/l9CgdXGHTfcxDDFMcJWL72ef/S6hD6Y5R 9Aii87c8GJCi9is5Xa5BLuT/sYHJXnoWTS3eyg6pY2HCBDZ05Vo0WfNuy w==; Date: Wed, 8 Dec 2021 23:01:44 +0100 To: Ulf Hansson CC: =?iso-8859-1?Q?M=E5rten?= Lindahl , Rob Herring , Krzysztof Kozlowski , Jaehoon Chung , Doug Anderson , kernel , "linux-mmc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-samsung-soc@vger.kernel.org" Subject: Re: [PATCH v2 4/4] mmc: dw_mmc: Do not wait for DTO in case of error Message-ID: <20211208220144.GA10156@axis.com> References: <20211206142929.26729-1-marten.lindahl@axis.com> <20211206142929.26729-5-marten.lindahl@axis.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) From: Marten Lindahl X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211208_140147_179079_2B557596 X-CRM114-Status: GOOD ( 39.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Dec 08, 2021 at 03:53:54PM +0100, Ulf Hansson wrote: > On Mon, 6 Dec 2021 at 15:29, M=E5rten Lindahl w= rote: > > > > When running the ARTPEC-8 DWMMC IP version, and a data error interrupt > > comes during a data read transfer, there is no guarantee for the data > > transfer over interrupt (DTO) to come within the specified data timeout. > > This case is handled by the dto_timer handler which will complete the > > request with the comment: > > > > /* > > * If DTO interrupt does NOT come in sending data state, > > * we should notify the driver to terminate current transfer > > * and report a data timeout to the core. > > */ > > > > But since the ARTPEC-8 DWMMC IP version, supports an extended TMOUT > > register which allows longer timeouts than the non ARTPEC-8 version > > does, waiting for the dto_timer to complete the request in error cases > > may cause the request to take significantly longer time than necessary. > > This is specifically true for the failing steps during tuning of a > > device. > > > > Fix this by completing the request when the error interrupt comes. > > > > Signed-off-by: M=E5rten Lindahl > = Hi Ulf! > Okay, this change looks a bit inconvenient to move into variant > specific callbacks. So, maybe the "quirks" flag makes sense, after > all. However, I would still look at using callbacks and library > functions, for the part implemented in patch3. Yes, I don't see how this patch can be easily made with callbacks, but definitely for patch3. So then I move the definition of the quirk from patch3 to this patch. > = > When it comes to the order of the patches in the series, I suggest > flipping things around and making patch2 the final piece. Otherwise > the support for the artpec variant will be broken between patch2 and > patch4, right? Ok, you mean there may be a risk that the ARTPEC-8 dw_mmc does not work if the support is enabled in patch2, but patch3 and patch4 is not in place? That is a good point, but it actually does work quite fine (most of the time) without the extended timeout function. But it does not use the full function of the data timeout, and the HW timeout is most often set to full timeout (0xFFFFFF =3D> 587ms with 200MHz), but the SW timer is limited to a lower value (0xFFFFFF =3D> 84 + 10 ms with 200MHz). My reasoning is: patch1 - dtbindings for ARTPEC-8 patch2 - adding ARTPEC-8 to dw_mmc-exynos patch3 - implement ARTPEC-8 specific function for data timeout patch4 - add quirk to abort the extended timeout in case of errors, used by ARTPEC-8 so, this means patch3 and patch4 depends on patch2, and patch4 depends on patch3. Kind regards M=E5rten > = > Kind regards > Uffe > = > > --- > > drivers/mmc/host/dw_mmc.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > index 45ea9fd97a6a..d6b76f47b1a2 100644 > > --- a/drivers/mmc/host/dw_mmc.c > > +++ b/drivers/mmc/host/dw_mmc.c > > @@ -2777,11 +2777,19 @@ static irqreturn_t dw_mci_interrupt(int irq, vo= id *dev_id) > > if (pending & DW_MCI_DATA_ERROR_FLAGS) { > > spin_lock(&host->irq_lock); > > > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > > + del_timer(&host->dto_timer); > > + > > /* if there is an error report DATA_ERROR */ > > mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLA= GS); > > host->data_status =3D pending; > > smp_wmb(); /* drain writebuffer */ > > set_bit(EVENT_DATA_ERROR, &host->pending_events= ); > > + > > + if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT) > > + /* In case of error, we cannot expect a= DTO */ > > + set_bit(EVENT_DATA_COMPLETE, &host->pen= ding_events); > > + > > tasklet_schedule(&host->tasklet); > > > > spin_unlock(&host->irq_lock); > > -- > > 2.20.1 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel