Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling
diff --git a/m b/m
index 35d9102..1be76df 100644
--- a/m
+++ b/m
@@ -2,63 +2,91 @@
 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.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
-	DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,
-	SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no
-	version=3.4.0
+X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
+	DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,
+	SIGNED_OFF_BY,SPF_PASS autolearn=ham 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 A4583C43218
-	for <infradead-linux-arm-kernel@archiver.kernel.org>; Thu, 25 Apr 2019 14:01:03 +0000 (UTC)
+	by smtp.lore.kernel.org (Postfix) with ESMTP id D95C0C43219
+	for <infradead-linux-arm-kernel@archiver.kernel.org>; Thu, 25 Apr 2019 14:09:49 +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 8A030206BA
-	for <infradead-linux-arm-kernel@archiver.kernel.org>; Thu, 25 Apr 2019 14:01:02 +0000 (UTC)
+	by mail.kernel.org (Postfix) with ESMTPS id C11A920644
+	for <infradead-linux-arm-kernel@archiver.kernel.org>; Thu, 25 Apr 2019 14:09:49 +0000 (UTC)
 Authentication-Results: mail.kernel.org;
-	dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Qui/jiFN"
-DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8A030206BA
-Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com
+	dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="P2juJ3+8";
+	dkim=fail reason="signature verification failed" (2048-bit key) header.d=st.com header.i=@st.com header.b="Q0NOgg/W"
+DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C11A920644
+Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=st.com
 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=QX69CNRl4MaHyem+EJu5wZP95Yum6vBXHVOCgiW+FVc=; b=Qui/jiFNWWEsWj
-	RpM+C3rFQlCegl+1JHJIFw/+YK9ztF7BvoVtNjkN4Hhg3wz2vNOKaRapMEWc4ELryEfWmJ3jTxSQ2
-	8LQDHmJvoLl+/JFK22AzUzPjGVt/BlWil2kWQFpEHY+yGRYuAL1SGsvPGJUTOQf9x1EnlR+65tAh0
-	JctsAqQSYduhh7ea+Rn/sY2ojzjuOWfyQlOYSZX7IfKHINfEy8hd10A/VjiiBci5XXYxVOx56l8ce
-	outEE6xpmVowI5Vl3oFZlePRmUbDjRh4jX+DD08rB0WrZF61mUh2ck47RQhC4NbG/C28qkBOnWqN5
-	9fRvw2/+eXszjMTxkvPA==;
+	d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type:
+	Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive:
+	List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From:
+	References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date:
+	Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner;
+	 bh=su1w7GV6G2sWiCkguTBi2Vfn8UgMUCmysKzSGV4qmtA=; b=P2juJ3+8GOMjDODVtKmSoDeb5
+	rSP/7K27mEeO9BeUUSpfb6amqOEpwgPR9V+DhAdoPu2DlR1AxkoIP9mzStSXrzmyEdRWcOS/T9wGd
+	3CGGPcYZhRDDev7uZfMO/ai7fnl/i3/LNuKH6dIIjQeR2ws2YebHOxkP3n+BC9SZH5RWv3NhBGiVr
+	gdEcc07h7POgzcywsW+lK+cKkM8VdMjY6lEIaROIz/mMsar9AtuLyahgjjlVW6oUHOF58I/dYt+C3
+	sUzYbqndIUhwlsToo0IWjT2MdcsvFBcRhEQuqIjbGKaPT87MuuI1Z4mouZLpg2M4/uKtGDvq5fwGd
+	RUTeGF5NQ==;
 Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org)
 	by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux))
-	id 1hJevj-0002sS-Ql; Thu, 25 Apr 2019 14:00:51 +0000
-Received: from foss.arm.com ([217.140.101.70])
- by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux))
- id 1hJevg-0002rk-DV
- for linux-arm-kernel@lists.infradead.org; Thu, 25 Apr 2019 14:00:49 +0000
-Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])
- by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D360A78;
- Thu, 25 Apr 2019 07:00:45 -0700 (PDT)
-Received: from fuggles.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com
- [10.72.51.249])
- by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 42D263F246;
- Thu, 25 Apr 2019 07:00:43 -0700 (PDT)
-Date: Thu, 25 Apr 2019 15:00:40 +0100
-From: Will Deacon <will.deacon@arm.com>
-To: Kees Cook <keescook@chromium.org>
-Subject: Re: [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with
- Clang and LTO
-Message-ID: <20190425140040.GC23796@fuggles.cambridge.arm.com>
-References: <20190424165537.GA48378@beast>
+	id 1hJf4P-00051C-0g; Thu, 25 Apr 2019 14:09:49 +0000
+Received: from mx07-00178001.pphosted.com ([62.209.51.94])
+ by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux))
+ id 1hJf4K-0004zv-Qq
+ for linux-arm-kernel@lists.infradead.org; Thu, 25 Apr 2019 14:09:46 +0000
+Received: from pps.filterd (m0046037.ppops.net [127.0.0.1])
+ by mx07-00178001.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id
+ x3PE6pwi031045; Thu, 25 Apr 2019 16:09:37 +0200
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=st.com;
+ h=subject : to : cc :
+ references : from : message-id : date : mime-version : in-reply-to :
+ content-type : content-transfer-encoding; s=STMicroelectronics;
+ bh=5gU58BSfd/u6NaJSnx/oRSYTlpw/4kusuaMrKHoGSOc=;
+ b=Q0NOgg/WDhdp6G0dfkiY3OYqV0Xp9HiRvi2nAnG0Cahl9PA1C5jbvDqNPOv5QMePIfgB
+ 2MwjKmMPX2XiG7/FITQy3LVSiqn3Q2cV1KqjqeWii67mTlr4GWQTOqWtAn7v3xLp8J2c
+ vkAV15Gk5ljIQ74QG9LWWMWxFYb9LEbUgmQPmFJp97gHG1T4FAbVilqtDwAJPqM1FGnl
+ koHxk2bpjJsV6Bzjv9JZzRcSzhXsMongkBq/jQrtnDAjH+f0GojYNSBr2OeFsaet2fzM
+ F88qZnpNhCXPtkc4qWnc/wyt2x/VK+5jTG22miwH46akPmAduBAyiFXk2ueavKZAnreN Gg== 
+Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35])
+ by mx07-00178001.pphosted.com with ESMTP id 2s3bbv8yy8-1
+ (version=TLSv1 cipher=ECDHE-RSA-AES256-SHA bits=256 verify=NOT);
+ Thu, 25 Apr 2019 16:09:37 +0200
+Received: from zeta.dmz-eu.st.com (zeta.dmz-eu.st.com [164.129.230.9])
+ by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 03BC431;
+ Thu, 25 Apr 2019 14:09:37 +0000 (GMT)
+Received: from Webmail-eu.st.com (sfhdag6node1.st.com [10.75.127.16])
+ by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id C993C27DF;
+ Thu, 25 Apr 2019 14:09:36 +0000 (GMT)
+Received: from [10.48.0.237] (10.75.127.47) by SFHDAG6NODE1.st.com
+ (10.75.127.16) with Microsoft SMTP Server (TLS) id 15.0.1347.2; Thu, 25 Apr
+ 2019 16:09:36 +0200
+Subject: Re: [PATCH 1/4] mmc: mmci: avoid fake busy polling
+To: Ulf Hansson <ulf.hansson@linaro.org>
+References: <1551802205-32188-1-git-send-email-ludovic.Barre@st.com>
+ <1551802205-32188-2-git-send-email-ludovic.Barre@st.com>
+ <CAPDyKFoZuQ+hshZpj-qjchKf7enW4ChPd=r_QhK2xtuJcSvqxQ@mail.gmail.com>
+ <fd57fd63-093e-dd23-5ca4-6bd4f99ecda9@st.com>
+ <CAPDyKFrmTX6w1ZgwBkEmieCQ5swUQx_O864mHofhNsz3LFC32A@mail.gmail.com>
+From: Ludovic BARRE <ludovic.barre@st.com>
+Message-ID: <30eae958-fd66-96a2-52a2-661c0646a302@st.com>
+Date: Thu, 25 Apr 2019 16:09:35 +0200
+User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
+ Thunderbird/60.6.1
 MIME-Version: 1.0
-Content-Disposition: inline
-In-Reply-To: <20190424165537.GA48378@beast>
-User-Agent: Mutt/1.11.1+86 (6f28e57d73f2) ()
+In-Reply-To: <CAPDyKFrmTX6w1ZgwBkEmieCQ5swUQx_O864mHofhNsz3LFC32A@mail.gmail.com>
+Content-Language: en-GB
+X-Originating-IP: [10.75.127.47]
+X-ClientProxiedBy: SFHDAG7NODE3.st.com (10.75.127.21) To SFHDAG6NODE1.st.com
+ (10.75.127.16)
+X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, ,
+ definitions=2019-04-25_11:, , signatures=0
 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
-X-CRM114-CacheID: sfid-20190425_070048_462793_BE0A1CE3 
-X-CRM114-Status: GOOD (  14.96  )
+X-CRM114-CacheID: sfid-20190425_070945_342427_8BA5D2CE 
+X-CRM114-Status: GOOD (  27.71  )
 X-BeenThere: linux-arm-kernel@lists.infradead.org
 X-Mailman-Version: 2.1.21
 Precedence: list
@@ -70,64 +98,190 @@
 List-Help: <mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>
 List-Subscribe: <http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>, 
  <mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>
-Cc: Mark Rutland <mark.rutland@arm.com>, Arnd Bergmann <arnd@arndb.de>,
- Ard Biesheuvel <ard.biesheuvel@linaro.org>,
- Catalin Marinas <catalin.marinas@arm.com>,
- Nick Desaulniers <ndesaulniers@google.com>, linux-kernel@vger.kernel.org,
- Yury Norov <ynorov@caviumnetworks.com>,
- Sami Tolvanen <samitolvanen@google.com>, Alex Matveev <alxmtvv@gmail.com>,
- Matthias Kaehlcke <mka@chromium.org>, linux-arm-kernel@lists.infradead.org
-Content-Type: text/plain; charset="us-ascii"
+Cc: DTML <devicetree@vger.kernel.org>,
+ Alexandre Torgue <alexandre.torgue@st.com>,
+ "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
+ Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
+ Rob Herring <robh+dt@kernel.org>,
+ Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
+ Maxime Coquelin <mcoquelin.stm32@gmail.com>,
+ linux-stm32@st-md-mailman.stormreply.com,
+ Linux ARM <linux-arm-kernel@lists.infradead.org>
 Content-Transfer-Encoding: 7bit
+Content-Type: text/plain; charset="us-ascii"; Format="flowed"
 Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@lists.infradead.org>
 Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org
 
-On Wed, Apr 24, 2019 at 09:55:37AM -0700, Kees Cook wrote:
-> Clang's integrated assembler does not allow assembly macros defined
-> in one inline asm block using the .macro directive to be used across
-> separate asm blocks. LLVM developers consider this a feature and not a
-> bug, recommending code refactoring:
-> 
->   https://bugs.llvm.org/show_bug.cgi?id=19749
-> 
-> As binutils doesn't allow macros to be redefined, this change uses
-> UNDEFINE_MRS_S and UNDEFINE_MSR_S to define corresponding macros
-> in-place and workaround gcc and clang limitations on redefining macros
-> across different assembler blocks.
-> 
-> Specifically, the current state after preprocessing looks like this:
-> 
-> asm volatile(".macro mXX_s ... .endm");
-> void f()
-> {
-> 	asm volatile("mXX_s a, b");
-> }
-> 
-> With GCC, it gives macro redefinition error because sysreg.h is included
-> in multiple source files, and assembler code for all of them is later
-> combined for LTO (I've seen an intermediate file with hundreds of
-> identical definitions).
-> 
-> With clang, it gives macro undefined error because clang doesn't allow
-> sharing macros between inline asm statements.
-> 
-> I also seem to remember catching another sort of undefined error with
-> GCC due to reordering of macro definition asm statement and generated
-> asm code for function that uses the macro.
-> 
-> The solution with defining and undefining for each use, while certainly
-> not elegant, satisfies both GCC and clang, LTO and non-LTO.
-> 
-> Co-developed-by: Alex Matveev <alxmtvv@gmail.com>
-> Co-developed-by: Yury Norov <ynorov@caviumnetworks.com>
-> Co-developed-by: Sami Tolvanen <samitolvanen@google.com>
-> Signed-off-by: Kees Cook <keescook@chromium.org>
-> ---
-> v5: include register declaration in macro (rutland)
 
-Cheers all, I've picked this up for 5.2 with the two Reviewed-by tags.
+On 4/25/19 12:08 PM, Ulf Hansson wrote:
+> On Thu, 25 Apr 2019 at 11:22, Ludovic BARRE <ludovic.barre@st.com> wrote:
+>>
+>> hi Ulf
+>>
+>> On 4/23/19 3:39 PM, Ulf Hansson wrote:
+>>> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre <ludovic.Barre@st.com> wrote:
+>>>>
+>>>> From: Ludovic Barre <ludovic.barre@st.com>
+>>>>
+>>>> The busy status bit could occurred even if no busy response is
+>>>> expected (example cmd11). On sdmmc variant, the busy_detect_flag
+>>>> reflects inverted value of d0 state, it's sampled at the end of a
+>>>> CMD response and a second time 2 clk cycles after the CMD response.
+>>>> To avoid a fake busy, the busy status could be checked and polled
+>>>> only if the command has RSP_BUSY flag.
+>>>
+>>> I would appreciate a better explanation of what this patch really changes.
+>>>
+>>> The above is giving some background to the behavior of sdmmc variant,
+>>> but at this point that variant doesn't even have the
+>>> ->variant->busy_detect flag set.
+>>>
+>>
+>> Yes, I will try to explain more and focus on common behavior.
+>>
+>>>>
+>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
+>>>> ---
+>>>>    drivers/mmc/host/mmci.c | 19 +++++++++++++------
+>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
+>>>>
+>>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
+>>>> index 387ff14..4901b73 100644
+>>>> --- a/drivers/mmc/host/mmci.c
+>>>> +++ b/drivers/mmc/host/mmci.c
+>>>> @@ -1220,12 +1220,13 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
+>>>>                unsigned int status)
+>>>>    {
+>>>>           void __iomem *base = host->base;
+>>>> -       bool sbc;
+>>>> +       bool sbc, busy_resp;
+>>>>
+>>>>           if (!cmd)
+>>>>                   return;
+>>>>
+>>>>           sbc = (cmd == host->mrq->sbc);
+>>>> +       busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
+>>>>
+>>>>           /*
+>>>>            * We need to be one of these interrupts to be considered worth
+>>>> @@ -1239,8 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
+>>>>           /*
+>>>>            * ST Micro variant: handle busy detection.
+>>>>            */
+>>>> -       if (host->variant->busy_detect) {
+>>>> -               bool busy_resp = !!(cmd->flags & MMC_RSP_BUSY);
+>>>> +       if (busy_resp && host->variant->busy_detect) {
+>>>>
+>>>>                   /* We are busy with a command, return */
+>>>>                   if (host->busy_status &&
+>>>> @@ -1253,7 +1253,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
+>>>>                    * that the special busy status bit is still set before
+>>>>                    * proceeding.
+>>>>                    */
+>>>> -               if (!host->busy_status && busy_resp &&
+>>>> +               if (!host->busy_status &&
+>>>>                       !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
+>>>>                       (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
+>>>
+>>> All the changes above makes perfect sense to me, but looks more like a
+>>> cleanup of the code, rather than actually changing the behavior.
+>>
+>> yes, few changing (this just avoid to enter in
+>> "if (host->variant->busy_detect)") at each time.
+>> I could move this part in cleanup patch (before this patch)
+> 
+> Sounds good to me!
+> 
+>>
+>>>
+>>>>
+>>>> @@ -1508,6 +1508,7 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
+>>>>    {
+>>>>           struct mmci_host *host = dev_id;
+>>>>           u32 status;
+>>>> +       bool busy_resp;
+>>>>           int ret = 0;
+>>>>
+>>>>           spin_lock(&host->lock);
+>>>> @@ -1550,9 +1551,15 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
+>>>>                   }
+>>>>
+>>>>                   /*
+>>>> -                * Don't poll for busy completion in irq context.
+>>>> +                * Don't poll for:
+>>>> +                * -busy completion in irq context.
+>>>> +                * -no busy response expected.
+>>>>                    */
+>>>> -               if (host->variant->busy_detect && host->busy_status)
+>>>> +               busy_resp = host->cmd ?
+>>>> +                       !!(host->cmd->flags & MMC_RSP_BUSY) : false;
+>>>
+>>> This doesn't make sense to me, but I may be missing something.
+>>>
+>>> host->busy_status is being updated by mmci_cmd_irq() and only when
+>>> MMC_RSP_BUSY is set for the command in flight. In other words,
+>>> checking for MMC_RSP_BUSY here as well is redundant. No?
+>>
+>> In mmci_irq the "do while" loops until the status is totally cleared.
+>>
+>> Today (for variant with busy_detect option), the status busy_detect_flag
+>> is excluded only while busy_status period (command with MMC_RSP_BUSY and
+>> while busy line is low => "busy_status=1")
+>>
+>> On SDMMC variant I noticed that busy_detect_flag status could be enabled
+>> even if the command is not MMC_RSP_BUSY, for example sdmmc variant stay
+>> in loop while cmd11 voltage switch.
+> 
+> Right, I see.
+> 
+>>
+>> So I wish extend host->variant->busy_detect_flag exclusion for all
+>> commands which is not a MMC_RSP_BUSY. I suppose that other variants
+>> could have the same behavior, and else there is no impact, normally.
+> 
+> I am guessing this is because the variant->busy_dpsm_flag has been set
+> in the datactrl register, which is needed for mmci_card_busy().
+> 
+> That said, I am kind of wondering if we ever should need repeat the
+> while loop if 'status' contains the bit for
+> host->variant->busy_detect_flag. I mean we have already called
+> mmci_cmd_irq() to handle it.
+> 
+> So, couldn't we just always do:
+> 
+> if (host->variant->busy_detect_flag)
+>      status &= ~host->variant->busy_detect_flag;
+> 
+> No?
 
-Will
+yes that make sense, I launched tests on sdmmc and it's ok.
+I think, that we could take on this solution.
+
+If it's ok for you, I resend a series with all modifications.
+
+Regards
+Ludo
+
+> 
+>>
+>>>
+>>>> +
+>>>> +               if (host->variant->busy_detect &&
+>>>> +                   (!busy_resp || host->busy_status))
+>>>>                           status &= ~host->variant->busy_detect_flag;
+>>>>
+>>>>                   ret = 1;
+>>>> --
+>>>> 2.7.4
+>>>>
+>>>
+>>> Kind regards
+>>> Uffe
+>>>
+> 
+> Kind regards
+> Uffe
+> 
 
 _______________________________________________
 linux-arm-kernel mailing list