All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
To: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Jaehoon Chung
	<jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	"linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Douglas Anderson
	<dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Subject: Re: [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request()【请注意,邮件由linux-mmc-owner@vger.kernel.org代发】
Date: Wed, 20 Mar 2019 10:06:39 +0800	[thread overview]
Message-ID: <cd9d8614-87fb-8016-baf6-5bfd98fcf135@rock-chips.com> (raw)
In-Reply-To: <CAPDyKFqMjEbM1L5=AjDv46xZt5KxOk8GLMGbWsdq2Xcxoi3gTg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>


On 2019/3/19 22:41, Ulf Hansson wrote:
> On Tue, 19 Mar 2019 at 10:14, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>
>> Move it from dw_mci_start_command() to dw_mci_request().
>> Then dw_mci_wait_while_busy() isn't called with host's
>> lock hold.
> 
> So, I decided to have a closer look at this.
> 
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Tested-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mmc/host/dw_mmc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 80dc2fd..703dedf 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -426,7 +426,6 @@ static void dw_mci_start_command(struct dw_mci *host,
>>
>>          mci_writel(host, CMDARG, cmd->arg);
>>          wmb(); /* drain writebuffer */
>> -       dw_mci_wait_while_busy(host, cmd_flags);
>>
>>          mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
>>
>> @@ -1419,6 +1418,10 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>                  return;
>>          }
>>
>> +       if ((mrq->cmd->opcode != MMC_SEND_STATUS && mrq->cmd->data) &&
>> +           !(mrq->cmd->opcode == SD_SWITCH_VOLTAGE))
>> +               dw_mci_wait_while_busy(host, SDMMC_CMD_PRV_DAT_WAIT);
>> +
> 
> This looks weird, because according to the change-log it sounds like
> you are moving things around to just avoid having the "lock" held.
> 
> To me, there seems to be more changes because of the new checks for
> the "opcode" above, no?

yes, that's what original dw_mci_wait_while_busy() did internally.

> 
>   Moreover, dw_mci_wait_while_busy() is also called from
> mci_send_cmd(), which means it may still be called with the "lock" is
> held. That is really confusing and needs more explanation.

dwmmc need to send a *NULL* cmd with SDMMC_CMD_UPD_CLK set in its CMD
register, in order to update bus clock, which is quite differnet from
others hosts. So mci_send_cmd()->dw_mci_wait_while_busy() is used for 
updating bus clock but not in the hot path. So the main concern is to
move dw_mci_wait_while_busy() out from normal data requst.


> 
>>          spin_lock_bh(&host->lock);
>>
>>          dw_mci_queue_request(host, slot, mrq);
>> --
>> 1.9.1
>>
>>
>>
> 
> Finally, I am not sure I understand why dw_mci_wait_while_busy() is
> called before starting a command, at all. That seems wrong to me.
> Instead, shouldn't we look at cmd->flags & MMC_RSP_BUSY, for the
> request in question and don't call mmc_request_done() for it, before
> the card have stop signaled busy. At least that the behavior the mmc
> core expects by the driver.
> 

yes, that was from commit 0bdbd0e88cf6b603("mmc: dw_mmc: Don't start 
commands while busy"), and it says "We'll leverage the existing dw_mmc
knowledge about whether it should wait for the previous command to
finish to know whether we should check for busy before sending the
command". I think the word, "leverage", explains the whole point, seems
a little tricky though.

> Kind regards
> Uffe
> 
> 

  parent reply	other threads:[~2019-03-20  2:06 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-19  9:12 [PATCH v3 0/3] Add hardware unbusy interrupt support for dw_mmc Shawn Lin
     [not found] ` <1552986778-33904-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2019-03-19  9:12   ` [PATCH v3 1/3] mmc: dw_mmc: Check busy state in dw_mci_request() Shawn Lin
     [not found]     ` <1552986778-33904-2-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2019-03-19 14:41       ` Ulf Hansson
     [not found]         ` <CAPDyKFqMjEbM1L5=AjDv46xZt5KxOk8GLMGbWsdq2Xcxoi3gTg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-20  2:06           ` Shawn Lin [this message]
2019-03-19  9:12   ` [PATCH v3 2/3] mmc: dw_mmc: Add hardware unbusy interrupt support Shawn Lin
2019-03-19  9:12   ` [PATCH v3 3/3] mmc: dw_mmc-rockchip: Enable " Shawn Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cd9d8614-87fb-8016-baf6-5bfd98fcf135@rock-chips.com \
    --to=shawn.lin-tnx95d0mmh7dzftrwevzcw@public.gmane.org \
    --cc=dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
    --cc=linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.