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=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS 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 390ADC43219 for ; Thu, 25 Apr 2019 14:09:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2096120644 for ; Thu, 25 Apr 2019 14:09:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=st.com header.i=@st.com header.b="Q0NOgg/W" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728657AbfDYOJv (ORCPT ); Thu, 25 Apr 2019 10:09:51 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:53073 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725965AbfDYOJt (ORCPT ); Thu, 25 Apr 2019 10:09:49 -0400 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 CC: Rob Herring , Srinivas Kandagatla , Maxime Coquelin , Alexandre Torgue , Linux ARM , Linux Kernel Mailing List , DTML , "linux-mmc@vger.kernel.org" , References: <1551802205-32188-1-git-send-email-ludovic.Barre@st.com> <1551802205-32188-2-git-send-email-ludovic.Barre@st.com> From: Ludovic BARRE 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 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/25/19 12:08 PM, Ulf Hansson wrote: > On Thu, 25 Apr 2019 at 11:22, Ludovic BARRE wrote: >> >> hi Ulf >> >> On 4/23/19 3:39 PM, Ulf Hansson wrote: >>> On Tue, 5 Mar 2019 at 17:10, Ludovic Barre wrote: >>>> >>>> From: Ludovic Barre >>>> >>>> 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 >>>> --- >>>> 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? 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 >