From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756300AbdCXH76 (ORCPT ); Fri, 24 Mar 2017 03:59:58 -0400 Received: from mga14.intel.com ([192.55.52.115]:57452 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752339AbdCXH6g (ORCPT ); Fri, 24 Mar 2017 03:58:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,214,1486454400"; d="scan'208";a="1146400482" Subject: Re: [PATCH] mmc: core: Do not hold re-tuning during CMD6 commands To: Chaotian Jing , Ulf Hansson References: <1490336341-22292-1-git-send-email-chaotian.jing@mediatek.com> <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> Cc: Matthias Brugger , Jaehoon Chung , Shawn Lin , Masahiro Yamada , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream@mediatek.com From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <13a83728-0031-5683-c371-4b517df32299@intel.com> Date: Fri, 24 Mar 2017 09:52:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24/03/17 08:19, Chaotian Jing wrote: > this patch is refine for 'commit c6dbab9cb58f ("mmc: core: Hold re-tuning > during switch commands")' > Since it has 3 retries at max for CMD6, if the first CMD6 got CRC error, > then should do re-tune before the next CMD6 was sent. > > Signed-off-by: Chaotian Jing > --- > drivers/mmc/core/mmc_ops.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index fe80f26..6931927 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -534,8 +534,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > bool use_r1b_resp = use_busy_signal; > unsigned char old_timing = host->ios.timing; > > - mmc_retune_hold(host); > - > /* > * If the cmd timeout and the max_busy_timeout of the host are both > * specified, let's validate them. A failure means we need to prevent > @@ -567,6 +565,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > cmd.sanitize_busy = true; > > err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES); > + mmc_retune_hold(host); That is not how mmc_retune_hold() works, you need mmc_retune_hold_now() as it is here: https://marc.info/?l=linux-mmc&m=148940903816582 But using "retries" with commands that have busy-waiting on the data line doesn't make much sense anyway. Particularly with CRC errors, I would expect the card is actually busily doing the switch and we need only to wait for it. The same can be true for timeout errors. For some CMD6 we might need to send CMD12 if the card is busy after an error. I would prefer an explicit attempt at recovery from CMD6 errors. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrian Hunter Subject: Re: [PATCH] mmc: core: Do not hold re-tuning during CMD6 commands Date: Fri, 24 Mar 2017 09:52:18 +0200 Message-ID: <13a83728-0031-5683-c371-4b517df32299@intel.com> References: <1490336341-22292-1-git-send-email-chaotian.jing@mediatek.com> <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Chaotian Jing , Ulf Hansson Cc: srv_heupstream@mediatek.com, Shawn Lin , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, Jaehoon Chung , Masahiro Yamada , linux-mediatek@lists.infradead.org, Matthias Brugger , linux-arm-kernel@lists.infradead.org List-Id: linux-mmc@vger.kernel.org On 24/03/17 08:19, Chaotian Jing wrote: > this patch is refine for 'commit c6dbab9cb58f ("mmc: core: Hold re-tuning > during switch commands")' > Since it has 3 retries at max for CMD6, if the first CMD6 got CRC error, > then should do re-tune before the next CMD6 was sent. > > Signed-off-by: Chaotian Jing > --- > drivers/mmc/core/mmc_ops.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index fe80f26..6931927 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -534,8 +534,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > bool use_r1b_resp = use_busy_signal; > unsigned char old_timing = host->ios.timing; > > - mmc_retune_hold(host); > - > /* > * If the cmd timeout and the max_busy_timeout of the host are both > * specified, let's validate them. A failure means we need to prevent > @@ -567,6 +565,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > cmd.sanitize_busy = true; > > err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES); > + mmc_retune_hold(host); That is not how mmc_retune_hold() works, you need mmc_retune_hold_now() as it is here: https://marc.info/?l=linux-mmc&m=148940903816582 But using "retries" with commands that have busy-waiting on the data line doesn't make much sense anyway. Particularly with CRC errors, I would expect the card is actually busily doing the switch and we need only to wait for it. The same can be true for timeout errors. For some CMD6 we might need to send CMD12 if the card is busy after an error. I would prefer an explicit attempt at recovery from CMD6 errors. From mboxrd@z Thu Jan 1 00:00:00 1970 From: adrian.hunter@intel.com (Adrian Hunter) Date: Fri, 24 Mar 2017 09:52:18 +0200 Subject: [PATCH] mmc: core: Do not hold re-tuning during CMD6 commands In-Reply-To: <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> References: <1490336341-22292-1-git-send-email-chaotian.jing@mediatek.com> <1490336341-22292-2-git-send-email-chaotian.jing@mediatek.com> Message-ID: <13a83728-0031-5683-c371-4b517df32299@intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 24/03/17 08:19, Chaotian Jing wrote: > this patch is refine for 'commit c6dbab9cb58f ("mmc: core: Hold re-tuning > during switch commands")' > Since it has 3 retries at max for CMD6, if the first CMD6 got CRC error, > then should do re-tune before the next CMD6 was sent. > > Signed-off-by: Chaotian Jing > --- > drivers/mmc/core/mmc_ops.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index fe80f26..6931927 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -534,8 +534,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > bool use_r1b_resp = use_busy_signal; > unsigned char old_timing = host->ios.timing; > > - mmc_retune_hold(host); > - > /* > * If the cmd timeout and the max_busy_timeout of the host are both > * specified, let's validate them. A failure means we need to prevent > @@ -567,6 +565,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value, > cmd.sanitize_busy = true; > > err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES); > + mmc_retune_hold(host); That is not how mmc_retune_hold() works, you need mmc_retune_hold_now() as it is here: https://marc.info/?l=linux-mmc&m=148940903816582 But using "retries" with commands that have busy-waiting on the data line doesn't make much sense anyway. Particularly with CRC errors, I would expect the card is actually busily doing the switch and we need only to wait for it. The same can be true for timeout errors. For some CMD6 we might need to send CMD12 if the card is busy after an error. I would prefer an explicit attempt at recovery from CMD6 errors.