[1/2] mmc: core: enable CMD19 tuning for DDR50 mode

Commit Message

Barry Song Sept. 15, 2015, 7:13 a.m. UTC

2015-08-25 20:05 GMT+08:00 Ulf Hansson <[e-mail]>:
> On 20 August 2015 at 02:16, Barry Song <[e-mail]> wrote:
>> 2015-08-18 1:11 GMT+08:00 Ulf Hansson <[e-mail]>:
>>> On 11 August 2015 at 10:41, Barry Song <[e-mail]> wrote:
>>>> From: Weijun Yang <[e-mail]>
>>>>
>>>> As SD Specifications Part1 Physical Layer Specification Version
>>>> 3.01 says, CMD19 tuning is available for unlocked cards in transfer
>>>> state of 1.8V signaling mode. The small difference between v3.00
>>>> and 3.01 spec means that CMD19 tuning is also available for DDR50
>>>> mode.
>>>
>>> So what happens with cards following the 3.0 spec version, those
>>> doesn't need to support the tuning CMD right? Perhaps that needs to be
>>> addressed in this patch well!?
>>
>> from HW registers of the card, we cann't know whether the HW needs
>> tuning. it is said 3.0.x need tuning, but 3.0 doesn't need.  @weijun,
>> pls fix me if i am wrong.
>> if so, it seems we need a static flag somewhere to indicate whether
>> the tuning is needed. we can't detect to find the tuning requirement.
>
> Another way is to always try doing the tuning for DDR50, but in case
> of errors just ignore them and print a debug/info message!?

Uffe, do you mean something like below:

     case MMC_TIMING_UHS_SDR50:

>
> Kind regards
> Uffe

-barry

Comments

Ulf Hansson Sept. 17, 2015, 12:27 p.m. UTC | #1

On 15 September 2015 at 09:13, Barry Song <[e-mail]> wrote:
> 2015-08-25 20:05 GMT+08:00 Ulf Hansson <[e-mail]>:
>> On 20 August 2015 at 02:16, Barry Song <[e-mail]> wrote:
>>> 2015-08-18 1:11 GMT+08:00 Ulf Hansson <[e-mail]>:
>>>> On 11 August 2015 at 10:41, Barry Song <[e-mail]> wrote:
>>>>> From: Weijun Yang <[e-mail]>
>>>>>
>>>>> As SD Specifications Part1 Physical Layer Specification Version
>>>>> 3.01 says, CMD19 tuning is available for unlocked cards in transfer
>>>>> state of 1.8V signaling mode. The small difference between v3.00
>>>>> and 3.01 spec means that CMD19 tuning is also available for DDR50
>>>>> mode.
>>>>
>>>> So what happens with cards following the 3.0 spec version, those
>>>> doesn't need to support the tuning CMD right? Perhaps that needs to be
>>>> addressed in this patch well!?
>>>
>>> from HW registers of the card, we cann't know whether the HW needs
>>> tuning. it is said 3.0.x need tuning, but 3.0 doesn't need.  @weijun,
>>> pls fix me if i am wrong.
>>> if so, it seems we need a static flag somewhere to indicate whether
>>> the tuning is needed. we can't detect to find the tuning requirement.
>>
>> Another way is to always try doing the tuning for DDR50, but in case
>> of errors just ignore them and print a debug/info message!?
>
> Uffe, do you mean something like below:
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 4e7366a..d4df9f0 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -629,8 +629,23 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>       */
>      if (!mmc_host_is_spi(card->host) &&
>          (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
> -         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
> +         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
> +         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
>          err = mmc_execute_tuning(card);
> +
> +        /*
> +         * As SD Specifications Part1 Physical Layer Specification Version
> +         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
> +         * state of 1.8V signaling mode. The small difference between v3.00
> +         * and 3.01 spec means that CMD19 tuning is also available for DDR50
> +         * mode.
> +         */
> +        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
> +            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));

Perhaps pr_warn() instead.

Moreover, you may just assign err = 0 and leave the return/kfree to
the "out" label.

> +            kfree(status);
> +            return 0;
> +        }
> +    }
>  out:
>      kfree(status);
>

[...]

Also, I was giving this a second thought...

What happens with the SD 3.0 card when it *doesn't* support CMD19.
Will it still be functional afterwards or will it move from "transfer
state" to another not known state?

So maybe we should just do the tuning and return the error if it
fails, as you suggested in the original patch. :-)

Kind regards
Uffe

Barry Song Sept. 18, 2015, 9:40 a.m. UTC | #2

2015-09-17 20:27 GMT+08:00 Ulf Hansson <[e-mail]>:
> On 15 September 2015 at 09:13, Barry Song <[e-mail]> wrote:
>> 2015-08-25 20:05 GMT+08:00 Ulf Hansson <[e-mail]>:
>>> On 20 August 2015 at 02:16, Barry Song <[e-mail]> wrote:
>>>> 2015-08-18 1:11 GMT+08:00 Ulf Hansson <[e-mail]>:
>>>>> On 11 August 2015 at 10:41, Barry Song <[e-mail]> wrote:
>>>>>> From: Weijun Yang <[e-mail]>
>>>>>>
>>>>>> As SD Specifications Part1 Physical Layer Specification Version
>>>>>> 3.01 says, CMD19 tuning is available for unlocked cards in transfer
>>>>>> state of 1.8V signaling mode. The small difference between v3.00
>>>>>> and 3.01 spec means that CMD19 tuning is also available for DDR50
>>>>>> mode.
>>>>>
>>>>> So what happens with cards following the 3.0 spec version, those
>>>>> doesn't need to support the tuning CMD right? Perhaps that needs to be
>>>>> addressed in this patch well!?
>>>>
>>>> from HW registers of the card, we cann't know whether the HW needs
>>>> tuning. it is said 3.0.x need tuning, but 3.0 doesn't need.  @weijun,
>>>> pls fix me if i am wrong.
>>>> if so, it seems we need a static flag somewhere to indicate whether
>>>> the tuning is needed. we can't detect to find the tuning requirement.
>>>
>>> Another way is to always try doing the tuning for DDR50, but in case
>>> of errors just ignore them and print a debug/info message!?
>>
>> Uffe, do you mean something like below:
>>
>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>> index 4e7366a..d4df9f0 100644
>> --- a/drivers/mmc/core/sd.c
>> +++ b/drivers/mmc/core/sd.c
>> @@ -629,8 +629,23 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
>>       */
>>      if (!mmc_host_is_spi(card->host) &&
>>          (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
>> -         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
>> +         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
>> +         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
>>          err = mmc_execute_tuning(card);
>> +
>> +        /*
>> +         * As SD Specifications Part1 Physical Layer Specification Version
>> +         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
>> +         * state of 1.8V signaling mode. The small difference between v3.00
>> +         * and 3.01 spec means that CMD19 tuning is also available for DDR50
>> +         * mode.
>> +         */
>> +        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
>> +            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));
>
> Perhaps pr_warn() instead.
>
> Moreover, you may just assign err = 0 and leave the return/kfree to
> the "out" label.
>
>> +            kfree(status);
>> +            return 0;
>> +        }
>> +    }
>>  out:
>>      kfree(status);
>>
>
> [...]
>
> Also, I was giving this a second thought...
>
> What happens with the SD 3.0 card when it *doesn't* support CMD19.
> Will it still be functional afterwards or will it move from "transfer
> state" to another not known state?

uffe, i am not the expert of this issue but weijun is. weijun told that
for the cards who don't support cmd19, it will take it as illegal
command and not response it. and it also sets illegal command error in
registers.
but the card status is still functional and doesn't cause unknown state.

so maybe we can just ignore the err of tuning, and use the second
patch with the refine you mentioned?

>
> So maybe we should just do the tuning and return the error if it
> fails, as you suggested in the original patch. :-)
>
> Kind regards
> Uffe

-barry

Ulf Hansson Sept. 23, 2015, 7:25 p.m. UTC | #3

[...]

>>
>> Also, I was giving this a second thought...
>>
>> What happens with the SD 3.0 card when it *doesn't* support CMD19.
>> Will it still be functional afterwards or will it move from "transfer
>> state" to another not known state?
>
> uffe, i am not the expert of this issue but weijun is. weijun told that
> for the cards who don't support cmd19, it will take it as illegal
> command and not response it. and it also sets illegal command error in
> registers.
> but the card status is still functional and doesn't cause unknown state.
>
> so maybe we can just ignore the err of tuning, and use the second
> patch with the refine you mentioned?

Sure, let's do that. Please send a v2 of the patchset then.

Kind regards
Uffe

Carlo Caione Dec. 12, 2015, 11:51 p.m. UTC | #4

On Wed, Sep 23, 2015 at 9:25 PM, Ulf Hansson <[e-mail]> wrote:
> [...]
>
>>>
>>> Also, I was giving this a second thought...
>>>
>>> What happens with the SD 3.0 card when it *doesn't* support CMD19.
>>> Will it still be functional afterwards or will it move from "transfer
>>> state" to another not known state?
>>
>> uffe, i am not the expert of this issue but weijun is. weijun told that
>> for the cards who don't support cmd19, it will take it as illegal
>> command and not response it. and it also sets illegal command error in
>> registers.
>> but the card status is still functional and doesn't cause unknown state.
>>
>> so maybe we can just ignore the err of tuning, and use the second
>> patch with the refine you mentioned?
>
> Sure, let's do that. Please send a v2 of the patchset then.

Replying here since I'm not subscribed to linux-mmc.

Commit 9faac7b95 "mmc: sdhci: enable tuning for DDR50" actually breaks
WiFi on SDIO at least for the ASUS X205TA.

I tried to ignore the error from the tuning also for the SDIO card
patching mmc_sdio_init_uhs_card() in drivers/mmc/core/sdio.c but I end
up with the error "Controller never released inhibit bit(s)". At least
in my case CMD19 shouldn't be issued at all to have the SDIO card
working.

Any suggestion how to approach this problem?

Yang, York Dec. 14, 2015, 8:46 a.m. UTC | #5

Hi, Carlo
	In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.
	This is based on the SD spec version 3.01.
	mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Carlo Caione

Sent: 2015?12?13? 7:52
To: Ulf Hansson <[e-mail]>
Cc: Barry Song <[e-mail]>; Yang, York <[e-mail]>; linux-mmc <[e-mail]>; DL-SHA-WorkGroupLinux <[e-mail]>; [e-mail]; Song, Barry <[e-mail]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

On Wed, Sep 23, 2015 at 9:25 PM, Ulf Hansson <[e-mail]> wrote:
> [...]

>

>>>

>>> Also, I was giving this a second thought...

>>>

>>> What happens with the SD 3.0 card when it *doesn't* support CMD19.

>>> Will it still be functional afterwards or will it move from 

>>> "transfer state" to another not known state?

>>

>> uffe, i am not the expert of this issue but weijun is. weijun told 

>> that for the cards who don't support cmd19, it will take it as 

>> illegal command and not response it. and it also sets illegal command 

>> error in registers.

>> but the card status is still functional and doesn't cause unknown state.

>>

>> so maybe we can just ignore the err of tuning, and use the second 

>> patch with the refine you mentioned?

>

> Sure, let's do that. Please send a v2 of the patchset then.


Replying here since I'm not subscribed to linux-mmc.

Commit 9faac7b95 "mmc: sdhci: enable tuning for DDR50" actually breaks WiFi on SDIO at least for the ASUS X205TA.

I tried to ignore the error from the tuning also for the SDIO card patching mmc_sdio_init_uhs_card() in drivers/mmc/core/sdio.c but I end up with the error "Controller never released inhibit bit(s)". At least in my case CMD19 shouldn't be issued at all to have the SDIO card working.

Any suggestion how to approach this problem?

--
Carlo Caione


 To report this email as spam click https://www.mailcontrol.com/sr/t!EO05MqfQ7GX2PQPOmvUkWM85sEKD4+jZQCQMWcEF+Ktu75GnJjKIuILC13YpF6Ld90r0YXMUaNc!rJiVGaJg== .

Carlo Caione Dec. 14, 2015, 9:13 a.m. UTC | #6

On Mon, Dec 14, 2015 at 9:46 AM, Yang, York <[e-mail]> wrote:
> Hi, Carlo
>         In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.
>         This is based on the SD spec version 3.01.
>         mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.

Hi Yang,
sorry for not being clear. Modifying mmc_sdio_init_uhs_card() was only
a test I did to check whether that could fix the issue, but it didn't.

Right now the situation is:
- In the latest master with commit 9faac7b95 WiFi is broken on the
ASUS X205TA (bcm43341)
- Reverting 9faac7b95 makes it working fine again

My conclusion is that issuing the CMD19 is basically breaking WiFi on
this platform.

Yang, York Dec. 14, 2015, 9:24 a.m. UTC | #7

Hi, Carlo

	I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
	Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
	Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions.
	If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card().
	Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Carlo Caione

Sent: 2015?12?14? 17:14
To: Yang, York <[e-mail]>
Cc: Carlo Caione <[e-mail]>; Ulf Hansson <[e-mail]>; Barry Song <[e-mail]>; linux-mmc <[e-mail]>; DL-SHA-WorkGroupLinux <[e-mail]>; [e-mail]; Song, Barry <[e-mail]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

On Mon, Dec 14, 2015 at 9:46 AM, Yang, York <[e-mail]> wrote:
> Hi, Carlo

>         In 9faac7b95, tuning for DDR50 is enabled in mmc_sd_init_uhs_card(), so only SD card is affected.

>         This is based on the SD spec version 3.01.

>         mmc_sdio_init_uhs_card() is untouched. I don't know who add the code into mmc_sdio_init_uhs_card() and you may have a check on your side.


Hi Yang,
sorry for not being clear. Modifying mmc_sdio_init_uhs_card() was only a test I did to check whether that could fix the issue, but it didn't.

Right now the situation is:
- In the latest master with commit 9faac7b95 WiFi is broken on the ASUS X205TA (bcm43341)
- Reverting 9faac7b95 makes it working fine again

My conclusion is that issuing the CMD19 is basically breaking WiFi on this platform.

--
Carlo Caione

Carlo Caione Dec. 14, 2015, 10:56 a.m. UTC | #8

On Mon, Dec 14, 2015 at 10:24 AM, Yang, York <[e-mail]> wrote:
> Hi, Carlo
>
>         I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
>         Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
>         Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions.
>         If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card().
>         Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.

Hi Yang,

On the unmodified master what I see is:

- mmc_sdio_init_uhs_card() is called
- card->sw_caps.sd3_bus_mode is 0x14
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is actually verified
(even though the tuning for DDR50 is not enabled)
- mmc_execute_tuning() is actually called

Jaehoon Chung Dec. 15, 2015, 11:56 p.m. UTC | #9

Hi,

On 12/14/2015 07:56 PM, Carlo Caione wrote:
> On Mon, Dec 14, 2015 at 10:24 AM, Yang, York <[e-mail]> wrote:
>> Hi, Carlo
>>
>>         I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.
>>         Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.
>>         Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions.
>>         If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card().
>>         Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.
> 
> Hi Yang,
> 
> On the unmodified master what I see is:
> 
> - mmc_sdio_init_uhs_card() is called
> - card->sw_caps.sd3_bus_mode is 0x14
> - card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is actually verified
> (even though the tuning for DDR50 is not enabled)
> - mmc_execute_tuning() is actually called

As i know, When mode is UHS_SDR50, tuning is not mandatory.
Even though i need to check the spec file, i think that condition check is strange.

Best Regards,
Jaehoon Chung

>

Yang, York Dec. 17, 2015, 2 a.m. UTC | #10

Hi, Carlo

	I don't understand. We can see that 9faac7b95 is all about DDR50. It should not affect the behavior of SDR50.
	Since the commit is very simple, you may remove the change one by one by yourself. And see if any difference occurs.
	Besides, please check if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning enabled.

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Carlo Caione

Sent: 2015?12?14? 18:57
To: Yang, York <[e-mail]>
Cc: Carlo Caione <[e-mail]>; Ulf Hansson <[e-mail]>; Barry Song <[e-mail]>; linux-mmc <[e-mail]>; DL-SHA-WorkGroupLinux <[e-mail]>; [e-mail]; Song, Barry <[e-mail]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

On Mon, Dec 14, 2015 at 10:24 AM, Yang, York <[e-mail]> wrote:
> Hi, Carlo

>

>         I don't think so. You may check the 9faac7b95, there are two changes in sd.c and sdhc.c.

>         Surely the change in sdhc.c will not affect ASUS X205TA (bcm43341) since it is a SDIO device.

>         Actually the calling sequence is this: mmc_sd_init_uhs_card() calls sdhci_execute_tuning(). In 9faac7b95, I enabled tuning for DDR50 in these two functions.

>         If your function calling sequence is mmc_sdio_init_uhs_card()->sdhci_execute_tuning(), the tuning will not carried out, since you didn't enable tuning for DDR50 in mmc_sdio_init_uhs_card().

>         Unless you call sdhci_execute_tuning directly, the tuning command may go through be sent out.


Hi Yang,

On the unmodified master what I see is:

- mmc_sdio_init_uhs_card() is called
- card->sw_caps.sd3_bus_mode is 0x14
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is actually verified (even though the tuning for DDR50 is not enabled)
- mmc_execute_tuning() is actually called

--
Carlo Caione

Carlo Caione Dec. 17, 2015, 7:54 p.m. UTC | #11

On gio, dic 17, 2015 at 02:00:17 +0000, Yang, York wrote:
> Hi, Carlo

Hi Yang,

> I don't understand. We can see that 9faac7b95 is all about
> DDR50. It should not affect the behavior of SDR50.

What I'm saying is that this commit is negatively affecting some DDR50
SDIO card as in my case.

Ok, let's try to go through the code with and without 9faac7b95.

Without 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and
mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- since there is no case for that we end up in 'default', goto
out_unlock and CMD19 is never issued

With 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and
  mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- this time we have the the 'case MMC_TIMING_UHS_DDR50' and we break on that
- CMD19 is issued and the card stops working

> Since the commit is very simple, you may remove the change one by one
> by yourself. And see if any difference occurs.

As stated above the problem is in the new 'case MMC_TIMING_UHS_DDR50'
we have in sdhci_execute_tuning()

> Besides, please check
> if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning
> enabled.

It doesn't.

Yang, York Dec. 18, 2015, 5:52 a.m. UTC | #12

Hi, Carlo
	I understand now. So the problem comes out because the sw_caps.sd3_bus_mode marks the capability of a card, while the host->timing is the mode we actually do with.
	For your case, you are running DDR50 mode, what if we add more condition for coming into the tuning logic?
	struct sdhci_host *host = mmc_priv(card->host);
	if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
			((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
			 (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))&&
			((host->timing & MMC_TIMING_UHS_SDR50) ||
				(host->timing & MMC_TIMING_UHS_SDR104))) {
		mmc_host_clk_hold(card->host);
		err = card->host->ops->execute_tuning(card->host,
						      MMC_SEND_TUNING_BLOCK);
		mmc_host_clk_release(card->host);
	}

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Carlo Caione

Sent: 2015?12?18? 3:54
To: Yang, York <[e-mail]>
Cc: Ulf Hansson <[e-mail]>; Barry Song <[e-mail]>; linux-mmc <[e-mail]>; DL-SHA-WorkGroupLinux <[e-mail]>; [e-mail]; Song, Barry <[e-mail]>
Subject: Re: [PATCH 1/2] mmc: core: enable CMD19 tuning for DDR50 mode

On gio, dic 17, 2015 at 02:00:17 +0000, Yang, York wrote:
> Hi, Carlo


Hi Yang,

> I don't understand. We can see that 9faac7b95 is all about DDR50. It 

> should not affect the behavior of SDR50.


What I'm saying is that this commit is negatively affecting some DDR50 SDIO card as in my case.

Ok, let's try to go through the code with and without 9faac7b95.

Without 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and
mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- since there is no case for that we end up in 'default', goto out_unlock and CMD19 is never issued

With 9faac7b95:
- mmc_sdio_init_uhs_card() is called
- sw_caps.sd3_bus_mode is 0x14 (SD_MODE_UHS_SDR50 | SD_MODE_UHS_DDR50)
- card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50 is verified and
  mmc_execute_tuning() is called
- sdhci_execute_tuning() is called
- host->timing is MMC_TIMING_UHS_DDR50
- this time we have the the 'case MMC_TIMING_UHS_DDR50' and we break on that
- CMD19 is issued and the card stops working

> Since the commit is very simple, you may remove the change one by one 

> by yourself. And see if any difference occurs.


As stated above the problem is in the new 'case MMC_TIMING_UHS_DDR50'
we have in sdhci_execute_tuning()

> Besides, please check

> if the card->sw_caps.sd3_bus_mode changes with or without DDR50 tuning 

> enabled.


It doesn't.

--
Carlo Caione

Patch

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 4e7366a..d4df9f0 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -629,8 +629,23 @@  static int mmc_sd_init_uhs_card(struct mmc_card *card)
      */
     if (!mmc_host_is_spi(card->host) &&
         (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-         card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+         card->sd_bus_speed == UHS_DDR50_BUS_SPEED ||
+         card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
         err = mmc_execute_tuning(card);
+
+        /*
+         * As SD Specifications Part1 Physical Layer Specification Version
+         * 3.01 says, CMD19 tuning is available for unlocked cards in transfer
+         * state of 1.8V signaling mode. The small difference between v3.00
+         * and 3.01 spec means that CMD19 tuning is also available for DDR50
+         * mode.
+         */
+        if (err && (card->sd_bus_speed == UHS_DDR50_BUS_SPEED))
+            pr_err("%s: ddr50 tuning failed\n", mmc_hostname(card->host));
+            kfree(status);
+            return 0;
+        }
+    }
 out:
     kfree(status);

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 64b7fdb..382810d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1915,6 +1915,7 @@  static int sdhci_execute_tuning(struct mmc_host
*mmc, u32 opcode)
         break;

     case MMC_TIMING_UHS_SDR104:
+    case MMC_TIMING_UHS_DDR50:
         break;