mmc: core: Restore parts of the polling policy when switch to HS/HS DDR

Message ID 1484305503-30723-1-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Jan. 13, 2017, 11:05 a.m.
Regressions for not being able to detect an eMMC HS DDR mode card has been
reported for the sdhci-esdhc-imx driver, but potentially other sdhci
variants may suffer from the similar problem.

The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when
switch to HS DDR mode"), is causing the problem. It seems that change moved
one step to far, regarding changing the host's timing before polling for a
busy card.

To fix this, let's move back to the behaviour when the host's timing is
updated after the polling, but before the switch status is fetched and
validated.

In cases when polling with CMD13, we keep validating the switch status at
each attempt. However, to align with the other card busy detections
mechanism, let's fetch and validate the switch status also after the host's
timing is updated.

Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>
Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")
Cc: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Dong Aisheng <aisheng.dong@nxp.com>
Cc: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
1.9.1

Comments

Clemens Gruber Jan. 13, 2017, 12:25 p.m. | #1
On Fri, Jan 13, 2017 at 12:05:03PM +0100, Ulf Hansson wrote:
> Regressions for not being able to detect an eMMC HS DDR mode card has been

> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

> variants may suffer from the similar problem.

> 

> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

> switch to HS DDR mode"), is causing the problem. It seems that change moved

> one step to far, regarding changing the host's timing before polling for a

> busy card.

> 

> To fix this, let's move back to the behaviour when the host's timing is

> updated after the polling, but before the switch status is fetched and

> validated.

> 

> In cases when polling with CMD13, we keep validating the switch status at

> each attempt. However, to align with the other card busy detections

> mechanism, let's fetch and validate the switch status also after the host's

> timing is updated.

> 

> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

> Cc: Shawn Lin <shawn.lin@rock-chips.com>

> Cc: Dong Aisheng <aisheng.dong@nxp.com>

> Cc: Haibo Chen <haibo.chen@nxp.com>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)

> 

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

> index db2969f..fe80f26 100644

> --- a/drivers/mmc/core/mmc_ops.c

> +++ b/drivers/mmc/core/mmc_ops.c

> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

>  		}

>  	} while (busy);

>  

> -	if (host->ops->card_busy && send_status)

> -		return mmc_switch_status(card);

> -

>  	return 0;

>  }

>  

> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

>  	if (!use_busy_signal)

>  		goto out;

>  

> -	/* Switch to new timing before poll and check switch status. */

> -	if (timing)

> -		mmc_set_timing(host, timing);

> -

>  	/*If SPI or used HW busy detection above, then we don't need to poll. */

>  	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||

> -		mmc_host_is_spi(host)) {

> -		if (send_status)

> -			err = mmc_switch_status(card);

> +		mmc_host_is_spi(host))

>  		goto out_tim;

> -	}

>  

>  	/* Let's try to poll to find out when the command is completed. */

>  	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);

> +	if (err)

> +		goto out;

>  

>  out_tim:

> -	if (err && timing)

> -		mmc_set_timing(host, old_timing);

> +	/* Switch to new timing before check switch status. */

> +	if (timing)

> +		mmc_set_timing(host, timing);

> +

> +	if (send_status) {

> +		err = mmc_switch_status(card);

> +		if (err && timing)

> +			mmc_set_timing(host, old_timing);

> +	}

>  out:

>  	mmc_retune_release(host);

>  

> -- 

> 1.9.1

> 


Hi,

this patch fixes the boot problem on my i.MX6Q board!

Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>


Thanks,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jagan Teki Jan. 15, 2017, 9:48 p.m. | #2
On Fri, Jan 13, 2017 at 1:25 PM, Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
> On Fri, Jan 13, 2017 at 12:05:03PM +0100, Ulf Hansson wrote:

>> Regressions for not being able to detect an eMMC HS DDR mode card has been

>> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

>> variants may suffer from the similar problem.

>>

>> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

>> switch to HS DDR mode"), is causing the problem. It seems that change moved

>> one step to far, regarding changing the host's timing before polling for a

>> busy card.

>>

>> To fix this, let's move back to the behaviour when the host's timing is

>> updated after the polling, but before the switch status is fetched and

>> validated.

>>

>> In cases when polling with CMD13, we keep validating the switch status at

>> each attempt. However, to align with the other card busy detections

>> mechanism, let's fetch and validate the switch status also after the host's

>> timing is updated.

>>

>> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

>> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

>> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

>> Cc: Shawn Lin <shawn.lin@rock-chips.com>

>> Cc: Dong Aisheng <aisheng.dong@nxp.com>

>> Cc: Haibo Chen <haibo.chen@nxp.com>

>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

>> ---

>>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>>  1 file changed, 12 insertions(+), 13 deletions(-)

>>

>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

>> index db2969f..fe80f26 100644

>> --- a/drivers/mmc/core/mmc_ops.c

>> +++ b/drivers/mmc/core/mmc_ops.c

>> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

>>               }

>>       } while (busy);

>>

>> -     if (host->ops->card_busy && send_status)

>> -             return mmc_switch_status(card);

>> -

>>       return 0;

>>  }

>>

>> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

>>       if (!use_busy_signal)

>>               goto out;

>>

>> -     /* Switch to new timing before poll and check switch status. */

>> -     if (timing)

>> -             mmc_set_timing(host, timing);

>> -

>>       /*If SPI or used HW busy detection above, then we don't need to poll. */

>>       if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||

>> -             mmc_host_is_spi(host)) {

>> -             if (send_status)

>> -                     err = mmc_switch_status(card);

>> +             mmc_host_is_spi(host))

>>               goto out_tim;

>> -     }

>>

>>       /* Let's try to poll to find out when the command is completed. */

>>       err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);

>> +     if (err)

>> +             goto out;

>>

>>  out_tim:

>> -     if (err && timing)

>> -             mmc_set_timing(host, old_timing);

>> +     /* Switch to new timing before check switch status. */

>> +     if (timing)

>> +             mmc_set_timing(host, timing);

>> +

>> +     if (send_status) {

>> +             err = mmc_switch_status(card);

>> +             if (err && timing)

>> +                     mmc_set_timing(host, old_timing);

>> +     }

>>  out:

>>       mmc_retune_release(host);

>>

>> --

>> 1.9.1

>>

>

> Hi,

>

> this patch fixes the boot problem on my i.MX6Q board!

>

> Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>


Tested on imx6q and imx6ul boards, with format, write and read.

Tested-by: Jagan Teki <jagan@amarulasolutions.com>


thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shawn Lin Jan. 16, 2017, 2:38 a.m. | #3
On 2017/1/13 19:05, Ulf Hansson wrote:
> Regressions for not being able to detect an eMMC HS DDR mode card has been

> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

> variants may suffer from the similar problem.

>

> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

> switch to HS DDR mode"), is causing the problem. It seems that change moved

> one step to far, regarding changing the host's timing before polling for a

> busy card.

>

> To fix this, let's move back to the behaviour when the host's timing is

> updated after the polling, but before the switch status is fetched and

> validated.

>

> In cases when polling with CMD13, we keep validating the switch status at

> each attempt. However, to align with the other card busy detections

> mechanism, let's fetch and validate the switch status also after the host's

> timing is updated.

>


Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>


> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

> Cc: Shawn Lin <shawn.lin@rock-chips.com>

> Cc: Dong Aisheng <aisheng.dong@nxp.com>

> Cc: Haibo Chen <haibo.chen@nxp.com>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)

>

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

> index db2969f..fe80f26 100644

> --- a/drivers/mmc/core/mmc_ops.c

> +++ b/drivers/mmc/core/mmc_ops.c

> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

>  		}

>  	} while (busy);

>

> -	if (host->ops->card_busy && send_status)

> -		return mmc_switch_status(card);

> -

>  	return 0;

>  }

>

> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

>  	if (!use_busy_signal)

>  		goto out;

>

> -	/* Switch to new timing before poll and check switch status. */

> -	if (timing)

> -		mmc_set_timing(host, timing);

> -

>  	/*If SPI or used HW busy detection above, then we don't need to poll. */

>  	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||

> -		mmc_host_is_spi(host)) {

> -		if (send_status)

> -			err = mmc_switch_status(card);

> +		mmc_host_is_spi(host))

>  		goto out_tim;

> -	}

>

>  	/* Let's try to poll to find out when the command is completed. */

>  	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);

> +	if (err)

> +		goto out;

>

>  out_tim:

> -	if (err && timing)

> -		mmc_set_timing(host, old_timing);

> +	/* Switch to new timing before check switch status. */

> +	if (timing)

> +		mmc_set_timing(host, timing);

> +

> +	if (send_status) {

> +		err = mmc_switch_status(card);

> +		if (err && timing)

> +			mmc_set_timing(host, old_timing);

> +	}

>  out:

>  	mmc_retune_release(host);

>

>



-- 
Best Regards
Shawn Lin

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bough Chen Jan. 16, 2017, 3:09 a.m. | #4
> -----Original Message-----

> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]

> Sent: Friday, January 13, 2017 7:05 PM

> To: linux-mmc@vger.kernel.org; Ulf Hansson <ulf.hansson@linaro.org>

> Cc: Jaehoon Chung <jh80.chung@samsung.com>; Adrian Hunter

> <adrian.hunter@intel.com>; Linus Walleij <linus.walleij@linaro.org>; Chaotian

> Jing <chaotian.jing@mediatek.com>; Yong Mao <yong.mao@mediatek.com>;

> Shawn Lin <shawn.lin@rock-chips.com>; Clemens Gruber

> <clemens.gruber@pqgruber.com>; Gary Bisson

> <gary.bisson@boundarydevices.com>; Dong Aisheng <dongas86@gmail.com>;

> Fabio Estevam <festevam@gmail.com>; A.S. Dong <aisheng.dong@nxp.com>;

> Bough Chen <haibo.chen@nxp.com>

> Subject: [PATCH] mmc: core: Restore parts of the polling policy when switch to

> HS/HS DDR

> 

> Regressions for not being able to detect an eMMC HS DDR mode card has been

> reported for the sdhci-esdhc-imx driver, but potentially other sdhci variants

> may suffer from the similar problem.

> 

> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

> switch to HS DDR mode"), is causing the problem. It seems that change moved

> one step to far, regarding changing the host's timing before polling for a busy

> card.

> 

> To fix this, let's move back to the behaviour when the host's timing is updated

> after the polling, but before the switch status is fetched and validated.

> 

> In cases when polling with CMD13, we keep validating the switch status at each

> attempt. However, to align with the other card busy detections mechanism,

> let's fetch and validate the switch status also after the host's timing is updated.

> 

> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

> Cc: Shawn Lin <shawn.lin@rock-chips.com>

> Cc: Dong Aisheng <aisheng.dong@nxp.com>

> Cc: Haibo Chen <haibo.chen@nxp.com>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)

> 

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

> index db2969f..fe80f26 100644

> --- a/drivers/mmc/core/mmc_ops.c

> +++ b/drivers/mmc/core/mmc_ops.c

> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card,

> unsigned int timeout_ms,

>  		}

>  	} while (busy);

> 

> -	if (host->ops->card_busy && send_status)

> -		return mmc_switch_status(card);

> -

>  	return 0;

>  }

> 

> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8

> index, u8 value,

>  	if (!use_busy_signal)

>  		goto out;

> 

> -	/* Switch to new timing before poll and check switch status. */

> -	if (timing)

> -		mmc_set_timing(host, timing);

> -

>  	/*If SPI or used HW busy detection above, then we don't need to poll.

> */

>  	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)

> ||

> -		mmc_host_is_spi(host)) {

> -		if (send_status)

> -			err = mmc_switch_status(card);

> +		mmc_host_is_spi(host))

>  		goto out_tim;

> -	}

> 

>  	/* Let's try to poll to find out when the command is completed. */

>  	err = mmc_poll_for_busy(card, timeout_ms, send_status,

> retry_crc_err);

> +	if (err)

> +		goto out;

> 

>  out_tim:

> -	if (err && timing)

> -		mmc_set_timing(host, old_timing);

> +	/* Switch to new timing before check switch status. */

> +	if (timing)

> +		mmc_set_timing(host, timing);

> +

> +	if (send_status) {

> +		err = mmc_switch_status(card);

> +		if (err && timing)

> +			mmc_set_timing(host, old_timing);

> +	}

>  out:

>  	mmc_retune_release(host);

> 

> --

> 1.9.1


Test this patch on i.MX6Q-SABRESD board, it can fix the issue.

Tested-by: Haibo Chen <haibo.chen@nxp.com>


Best Regards,
Hiabo Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng Jan. 16, 2017, 3:48 a.m. | #5
Hi Ulf,

On Fri, Jan 13, 2017 at 7:05 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Regressions for not being able to detect an eMMC HS DDR mode card has been

> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

> variants may suffer from the similar problem.

>

> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

> switch to HS DDR mode"), is causing the problem. It seems that change moved

> one step to far, regarding changing the host's timing before polling for a

> busy card.

>

> To fix this, let's move back to the behaviour when the host's timing is

> updated after the polling, but before the switch status is fetched and

> validated.

>

> In cases when polling with CMD13, we keep validating the switch status at

> each attempt. However, to align with the other card busy detections

> mechanism, let's fetch and validate the switch status also after the host's

> timing is updated.

>

> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

> Cc: Shawn Lin <shawn.lin@rock-chips.com>

> Cc: Dong Aisheng <aisheng.dong@nxp.com>

> Cc: Haibo Chen <haibo.chen@nxp.com>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Thanks for the fix.

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>


BTW, i wonder if we could then remove the CMD13 Polling method or give a
temporarily WARN_ONCE to indicate OBSOLETED using and force all hosts
to provide card_busy() callback.
Then we can permanently fix the potential timing mismatch issue.

Regards
Dong Aisheng

> ---

>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)

>

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

> index db2969f..fe80f26 100644

> --- a/drivers/mmc/core/mmc_ops.c

> +++ b/drivers/mmc/core/mmc_ops.c

> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

>                 }

>         } while (busy);

>

> -       if (host->ops->card_busy && send_status)

> -               return mmc_switch_status(card);

> -

>         return 0;

>  }

>

> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

>         if (!use_busy_signal)

>                 goto out;

>

> -       /* Switch to new timing before poll and check switch status. */

> -       if (timing)

> -               mmc_set_timing(host, timing);

> -

>         /*If SPI or used HW busy detection above, then we don't need to poll. */

>         if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||

> -               mmc_host_is_spi(host)) {

> -               if (send_status)

> -                       err = mmc_switch_status(card);

> +               mmc_host_is_spi(host))

>                 goto out_tim;

> -       }

>

>         /* Let's try to poll to find out when the command is completed. */

>         err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);

> +       if (err)

> +               goto out;

>

>  out_tim:

> -       if (err && timing)

> -               mmc_set_timing(host, old_timing);

> +       /* Switch to new timing before check switch status. */

> +       if (timing)

> +               mmc_set_timing(host, timing);

> +

> +       if (send_status) {

> +               err = mmc_switch_status(card);

> +               if (err && timing)

> +                       mmc_set_timing(host, old_timing);

> +       }

>  out:

>         mmc_retune_release(host);

>

> --

> 1.9.1

>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Jan. 16, 2017, 3:13 p.m. | #6
On 16 January 2017 at 04:48, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Ulf,

>

> On Fri, Jan 13, 2017 at 7:05 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> Regressions for not being able to detect an eMMC HS DDR mode card has been

>> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

>> variants may suffer from the similar problem.

>>

>> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

>> switch to HS DDR mode"), is causing the problem. It seems that change moved

>> one step to far, regarding changing the host's timing before polling for a

>> busy card.

>>

>> To fix this, let's move back to the behaviour when the host's timing is

>> updated after the polling, but before the switch status is fetched and

>> validated.

>>

>> In cases when polling with CMD13, we keep validating the switch status at

>> each attempt. However, to align with the other card busy detections

>> mechanism, let's fetch and validate the switch status also after the host's

>> timing is updated.

>>

>> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

>> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

>> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

>> Cc: Shawn Lin <shawn.lin@rock-chips.com>

>> Cc: Dong Aisheng <aisheng.dong@nxp.com>

>> Cc: Haibo Chen <haibo.chen@nxp.com>

>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

>

> Thanks for the fix.

>

> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>


Thanks for reviewing!

>

> BTW, i wonder if we could then remove the CMD13 Polling method or give a

> temporarily WARN_ONCE to indicate OBSOLETED using and force all hosts

> to provide card_busy() callback.

> Then we can permanently fix the potential timing mismatch issue.


In the end it seems like the issue is host specific. I have
successfully used CM13 polling on my ux500 board (mmci.c, with some
hacks to force CMD13).

If we see any further issues, perhaps we should invent a new host cap,
which forbids the CMD13 polling and then those host drivers that has
issues, can set it. Wouldn't that be okay for you?

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Jan. 16, 2017, 3:14 p.m. | #7
On 13 January 2017 at 12:05, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Regressions for not being able to detect an eMMC HS DDR mode card has been

> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

> variants may suffer from the similar problem.

>

> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

> switch to HS DDR mode"), is causing the problem. It seems that change moved

> one step to far, regarding changing the host's timing before polling for a

> busy card.

>

> To fix this, let's move back to the behaviour when the host's timing is

> updated after the polling, but before the switch status is fetched and

> validated.

>

> In cases when polling with CMD13, we keep validating the switch status at

> each attempt. However, to align with the other card busy detections

> mechanism, let's fetch and validate the switch status also after the host's

> timing is updated.

>

> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

> Cc: Shawn Lin <shawn.lin@rock-chips.com>

> Cc: Dong Aisheng <aisheng.dong@nxp.com>

> Cc: Haibo Chen <haibo.chen@nxp.com>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Applied for fixes! Thanks everybody for reviews and tests!

Kind regards
Uffe

> ---

>  drivers/mmc/core/mmc_ops.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)

>

> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

> index db2969f..fe80f26 100644

> --- a/drivers/mmc/core/mmc_ops.c

> +++ b/drivers/mmc/core/mmc_ops.c

> @@ -506,9 +506,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,

>                 }

>         } while (busy);

>

> -       if (host->ops->card_busy && send_status)

> -               return mmc_switch_status(card);

> -

>         return 0;

>  }

>

> @@ -577,24 +574,26 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,

>         if (!use_busy_signal)

>                 goto out;

>

> -       /* Switch to new timing before poll and check switch status. */

> -       if (timing)

> -               mmc_set_timing(host, timing);

> -

>         /*If SPI or used HW busy detection above, then we don't need to poll. */

>         if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||

> -               mmc_host_is_spi(host)) {

> -               if (send_status)

> -                       err = mmc_switch_status(card);

> +               mmc_host_is_spi(host))

>                 goto out_tim;

> -       }

>

>         /* Let's try to poll to find out when the command is completed. */

>         err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);

> +       if (err)

> +               goto out;

>

>  out_tim:

> -       if (err && timing)

> -               mmc_set_timing(host, old_timing);

> +       /* Switch to new timing before check switch status. */

> +       if (timing)

> +               mmc_set_timing(host, timing);

> +

> +       if (send_status) {

> +               err = mmc_switch_status(card);

> +               if (err && timing)

> +                       mmc_set_timing(host, old_timing);

> +       }

>  out:

>         mmc_retune_release(host);

>

> --

> 1.9.1

>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dong Aisheng Jan. 17, 2017, 1:43 a.m. | #8
Hi Ulf,

On Mon, Jan 16, 2017 at 11:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 16 January 2017 at 04:48, Dong Aisheng <dongas86@gmail.com> wrote:

>> Hi Ulf,

>>

>> On Fri, Jan 13, 2017 at 7:05 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>>> Regressions for not being able to detect an eMMC HS DDR mode card has been

>>> reported for the sdhci-esdhc-imx driver, but potentially other sdhci

>>> variants may suffer from the similar problem.

>>>

>>> The commit e173f8911f09 ("mmc: core: Update CMD13 polling policy when

>>> switch to HS DDR mode"), is causing the problem. It seems that change moved

>>> one step to far, regarding changing the host's timing before polling for a

>>> busy card.

>>>

>>> To fix this, let's move back to the behaviour when the host's timing is

>>> updated after the polling, but before the switch status is fetched and

>>> validated.

>>>

>>> In cases when polling with CMD13, we keep validating the switch status at

>>> each attempt. However, to align with the other card busy detections

>>> mechanism, let's fetch and validate the switch status also after the host's

>>> timing is updated.

>>>

>>> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>

>>> Reported-by: Gary Bisson <gary.bisson@boundarydevices.com>

>>> Fixes: e173f8911f09 ("mmc: core: Update CMD13 polling policy when switch..")

>>> Cc: Shawn Lin <shawn.lin@rock-chips.com>

>>> Cc: Dong Aisheng <aisheng.dong@nxp.com>

>>> Cc: Haibo Chen <haibo.chen@nxp.com>

>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

>>

>> Thanks for the fix.

>>

>> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

>

> Thanks for reviewing!

>

>>

>> BTW, i wonder if we could then remove the CMD13 Polling method or give a

>> temporarily WARN_ONCE to indicate OBSOLETED using and force all hosts

>> to provide card_busy() callback.

>> Then we can permanently fix the potential timing mismatch issue.

>

> In the end it seems like the issue is host specific. I have

> successfully used CM13 polling on my ux500 board (mmci.c, with some

> hacks to force CMD13).

>

> If we see any further issues, perhaps we should invent a new host cap,

> which forbids the CMD13 polling and then those host drivers that has

> issues, can set it. Wouldn't that be okay for you?

>


I'm fine with it.
Thanks

Regards
Dong Aisheng

> [...]

>

> Kind regards

> Uffe

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index db2969f..fe80f26 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -506,9 +506,6 @@  static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		}
 	} while (busy);
 
-	if (host->ops->card_busy && send_status)
-		return mmc_switch_status(card);
-
 	return 0;
 }
 
@@ -577,24 +574,26 @@  int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (!use_busy_signal)
 		goto out;
 
-	/* Switch to new timing before poll and check switch status. */
-	if (timing)
-		mmc_set_timing(host, timing);
-
 	/*If SPI or used HW busy detection above, then we don't need to poll. */
 	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||
-		mmc_host_is_spi(host)) {
-		if (send_status)
-			err = mmc_switch_status(card);
+		mmc_host_is_spi(host))
 		goto out_tim;
-	}
 
 	/* Let's try to poll to find out when the command is completed. */
 	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
+	if (err)
+		goto out;
 
 out_tim:
-	if (err && timing)
-		mmc_set_timing(host, old_timing);
+	/* Switch to new timing before check switch status. */
+	if (timing)
+		mmc_set_timing(host, timing);
+
+	if (send_status) {
+		err = mmc_switch_status(card);
+		if (err && timing)
+			mmc_set_timing(host, old_timing);
+	}
 out:
 	mmc_retune_release(host);