[PATCH v1 0/3] SDIO fixes

[PATCH v1 0/3] SDIO fixes

[Subhash Jadavani]

This patch-series is having 1 fix for SDIO resume and 2 fixes related
to SDIO3.0 card detection

Subhash Jadavani (2):
  mmc: sdio: fix resume failure
  mmc: sdio: print correct UHS mode during card detection

Sujit Reddy Thumma (1):
  mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

 drivers/mmc/core/sdio.c  |   33 ++++++++++++++++++++-------------
 include/linux/mmc/host.h |    8 ++++++++
 2 files changed, 28 insertions(+), 13 deletions(-)

-- 
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v1 1/3] mmc: sdio: fix resume failure

[Subhash Jadavani]

If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
be reinitialized during resume but as we are not resetting
(CMD52 reset) the SDIO card during this reinitialization, card may
fail to respond back to subsequent commands (CMD5 etc...).

This change resets the card before the reinitialization of card
during resume.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
---
 drivers/mmc/core/sdio.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 2273ce6..34ad4c8 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	mmc_claim_host(host);
 
 	/* No need to reinitialize powered-resumed nonremovable cards */
-	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
+	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
+		sdio_reset(host);
+		mmc_go_idle(host);
 		err = mmc_sdio_init_card(host, host->ocr, host->card,
 					mmc_card_keep_power(host));
-	else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
+	} else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
 		if (err > 0) {
-- 
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Ulf Hansson]

On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
> be reinitialized during resume but as we are not resetting
> (CMD52 reset) the SDIO card during this reinitialization, card may
> fail to respond back to subsequent commands (CMD5 etc...).
>
> This change resets the card before the reinitialization of card
> during resume.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> ---
>  drivers/mmc/core/sdio.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 2273ce6..34ad4c8 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>         mmc_claim_host(host);
>
>         /* No need to reinitialize powered-resumed nonremovable cards */
> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
> +               sdio_reset(host);
> +               mmc_go_idle(host);

If the card has lost power, why is this needed? Could it be that the
host is not capable of cutting the power to card?

It might be more safe to do this but I would like to understand more,
before you get my ack on this patch.

>                 err = mmc_sdio_init_card(host, host->ocr, host->card,
>                                         mmc_card_keep_power(host));
> -       else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
> +       } else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
>                 /* We may have switched to 1-bit mode during suspend */
>                 err = sdio_enable_4bit_bus(host->card);
>                 if (err > 0) {
> --
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> 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

Kind regards
Ulf Hansson

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Subhash Jadavani]

On 12/6/2012 4:03 PM, Ulf Hansson wrote:
> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org> wrote:
>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>> be reinitialized during resume but as we are not resetting
>> (CMD52 reset) the SDIO card during this reinitialization, card may
>> fail to respond back to subsequent commands (CMD5 etc...).
>>
>> This change resets the card before the reinitialization of card
>> during resume.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> ---
>>   drivers/mmc/core/sdio.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 2273ce6..34ad4c8 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>          mmc_claim_host(host);
>>
>>          /* No need to reinitialize powered-resumed nonremovable cards */
>> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
>> +               sdio_reset(host);
>> +               mmc_go_idle(host);
> If the card has lost power, why is this needed? Could it be that the
> host is not capable of cutting the power to card?

Yes, the regulator powering the card is always on in this case which 
means SDIO VDD is not powered off during the suspend.

>
> It might be more safe to do this but I would like to understand more,
> before you get my ack on this patch.
>
>>                  err = mmc_sdio_init_card(host, host->ocr, host->card,
>>                                          mmc_card_keep_power(host));
>> -       else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
>> +       } else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
>>                  /* We may have switched to 1-bit mode during suspend */
>>                  err = sdio_enable_4bit_bus(host->card);
>>                  if (err > 0) {
>> --
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> 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
> Kind regards
> Ulf Hansson

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Ulf Hansson]

On 6 December 2012 16:25, Subhash Jadavani <subhashj@codeaurora.org> wrote:
>
> On 12/6/2012 4:03 PM, Ulf Hansson wrote:
>>
>> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org>
>> wrote:
>>>
>>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>>> be reinitialized during resume but as we are not resetting
>>> (CMD52 reset) the SDIO card during this reinitialization, card may
>>> fail to respond back to subsequent commands (CMD5 etc...).
>>>
>>> This change resets the card before the reinitialization of card
>>> during resume.
>>>
>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>> ---
>>>   drivers/mmc/core/sdio.c |    6 ++++--
>>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index 2273ce6..34ad4c8 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>          mmc_claim_host(host);
>>>
>>>          /* No need to reinitialize powered-resumed nonremovable cards */
>>> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
>>> +               sdio_reset(host);
>>> +               mmc_go_idle(host);
>>
>> If the card has lost power, why is this needed? Could it be that the
>> host is not capable of cutting the power to card?
>
>
> Yes, the regulator powering the card is always on in this case which means
> SDIO VDD is not powered off during the suspend.
>

So in principle the host should be able to notify the protocol layer
about that mmc_card_keep_power will always be set somehow.
That could be a more proper way of solving this maybe? What do you think?

>
>>
>> It might be more safe to do this but I would like to understand more,
>> before you get my ack on this patch.
>>
>>>                  err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>                                          mmc_card_keep_power(host));
>>> -       else if (mmc_card_keep_power(host) &&
>>> mmc_card_wake_sdio_irq(host)) {
>>> +       } else if (mmc_card_keep_power(host) &&
>>> mmc_card_wake_sdio_irq(host)) {
>>>                  /* We may have switched to 1-bit mode during suspend */
>>>                  err = sdio_enable_4bit_bus(host->card);
>>>                  if (err > 0) {
>>> --
>>> --
>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>
>>> --
>>> 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
>>
>> Kind regards
>> Ulf Hansson
>
>

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Subhash Jadavani]

On 12/7/2012 3:40 AM, Ulf Hansson wrote:
> On 6 December 2012 16:25, Subhash Jadavani <subhashj@codeaurora.org> wrote:
>> On 12/6/2012 4:03 PM, Ulf Hansson wrote:
>>> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org>
>>> wrote:
>>>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>>>> be reinitialized during resume but as we are not resetting
>>>> (CMD52 reset) the SDIO card during this reinitialization, card may
>>>> fail to respond back to subsequent commands (CMD5 etc...).
>>>>
>>>> This change resets the card before the reinitialization of card
>>>> during resume.
>>>>
>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>> ---
>>>>    drivers/mmc/core/sdio.c |    6 ++++--
>>>>    1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>> index 2273ce6..34ad4c8 100644
>>>> --- a/drivers/mmc/core/sdio.c
>>>> +++ b/drivers/mmc/core/sdio.c
>>>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>>           mmc_claim_host(host);
>>>>
>>>>           /* No need to reinitialize powered-resumed nonremovable cards */
>>>> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>>> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
>>>> +               sdio_reset(host);
>>>> +               mmc_go_idle(host);
>>> If the card has lost power, why is this needed? Could it be that the
>>> host is not capable of cutting the power to card?
>>
>> Yes, the regulator powering the card is always on in this case which means
>> SDIO VDD is not powered off during the suspend.
>>
> So in principle the host should be able to notify the protocol layer
> about that mmc_card_keep_power will always be set somehow.
> That could be a more proper way of solving this maybe? What do you think?
Ok.
Basically this would be the suspend/resume sequence with Keep Power flag 
cleared.

Suspend:
1. Execute Function driver suspend
2. mmc_power_off()
     2.1 As this is regulator powering card SDIO VDD line is always on 
regulator, it will stay on.

Resume:
1. mmc_power_up()
      1.1 Regulator was anyway on so nothing change here.
2.  mmc_sdio_init_card() - reinitialize the card
      2.1 CMD5 gets the commands response timeout which means card 
doesn't respond back.

My understanding is that behaviour seen in 2.1 (resume) is because 
during suspend, function driver suspend might have done something with 
the SDIO card (as keep power flag was disabled) and that's the reason 
during resume, card fails to respond back to CMD5 (unless we send the 
CMD52 to reset the card). It's an athros SDIO wifi card here.

 >> So in principle the host should be able to notify the protocol layer 
about that mmc_card_keep_power will always be set somehow.
Given the above issue, i am not sure how is this suggestion going to 
help in this case?

Regards,
Subhash

>
>>> It might be more safe to do this but I would like to understand more,
>>> before you get my ack on this patch.
>>>
>>>>                   err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>>                                           mmc_card_keep_power(host));
>>>> -       else if (mmc_card_keep_power(host) &&
>>>> mmc_card_wake_sdio_irq(host)) {
>>>> +       } else if (mmc_card_keep_power(host) &&
>>>> mmc_card_wake_sdio_irq(host)) {
>>>>                   /* We may have switched to 1-bit mode during suspend */
>>>>                   err = sdio_enable_4bit_bus(host->card);
>>>>                   if (err > 0) {
>>>> --
>>>> --
>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>>
>>>> --
>>>> 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
>>> Kind regards
>>> Ulf Hansson
>>

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Ulf Hansson]

On 7 December 2012 13:15, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> On 12/7/2012 3:40 AM, Ulf Hansson wrote:
>>
>> On 6 December 2012 16:25, Subhash Jadavani <subhashj@codeaurora.org>
>> wrote:
>>>
>>> On 12/6/2012 4:03 PM, Ulf Hansson wrote:
>>>>
>>>> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org>
>>>> wrote:
>>>>>
>>>>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>>>>> be reinitialized during resume but as we are not resetting
>>>>> (CMD52 reset) the SDIO card during this reinitialization, card may
>>>>> fail to respond back to subsequent commands (CMD5 etc...).
>>>>>
>>>>> This change resets the card before the reinitialization of card
>>>>> during resume.
>>>>>
>>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>>> ---
>>>>>    drivers/mmc/core/sdio.c |    6 ++++--
>>>>>    1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>> index 2273ce6..34ad4c8 100644
>>>>> --- a/drivers/mmc/core/sdio.c
>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>>>           mmc_claim_host(host);
>>>>>
>>>>>           /* No need to reinitialize powered-resumed nonremovable cards
>>>>> */
>>>>> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>>>> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>>>> {
>>>>> +               sdio_reset(host);
>>>>> +               mmc_go_idle(host);
>>>>
>>>> If the card has lost power, why is this needed? Could it be that the
>>>> host is not capable of cutting the power to card?
>>>
>>>
>>> Yes, the regulator powering the card is always on in this case which
>>> means
>>> SDIO VDD is not powered off during the suspend.
>>>
>> So in principle the host should be able to notify the protocol layer
>> about that mmc_card_keep_power will always be set somehow.
>> That could be a more proper way of solving this maybe? What do you think?
>
> Ok.
> Basically this would be the suspend/resume sequence with Keep Power flag
> cleared.

I mean the exact opposite. :-)

mmc_keep_power should be _set_ during suspend/resume sequence.

That will mean at suspend; sdio_disable_4bit_bus will be called and
mmc_power_off will not.
At resume, sdio_enable_4bit_bus will be called, without doing
mmc_power_up and mmc_sdio_init_card.

Do note that there is no API for the setting mmc_keep_power from a
host. It exist only in the SDIO API. (sdio_set_host_pm_flags).
So I guess we should invent this API for the host to use.

>
> Suspend:
> 1. Execute Function driver suspend
> 2. mmc_power_off()
>     2.1 As this is regulator powering card SDIO VDD line is always on
> regulator, it will stay on.
>
> Resume:
> 1. mmc_power_up()
>      1.1 Regulator was anyway on so nothing change here.
> 2.  mmc_sdio_init_card() - reinitialize the card
>      2.1 CMD5 gets the commands response timeout which means card doesn't
> respond back.
>
> My understanding is that behaviour seen in 2.1 (resume) is because during
> suspend, function driver suspend might have done something with the SDIO
> card (as keep power flag was disabled) and that's the reason during resume,
> card fails to respond back to CMD5 (unless we send the CMD52 to reset the
> card). It's an athros SDIO wifi card here.
>
>
>>> So in principle the host should be able to notify the protocol layer
>>> about that mmc_card_keep_power will always be set somehow.
> Given the above issue, i am not sure how is this suggestion going to help in
> this case?
>
> Regards,
> Subhash
>
>
>>
>>>> It might be more safe to do this but I would like to understand more,
>>>> before you get my ack on this patch.
>>>>
>>>>>                   err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>>>                                           mmc_card_keep_power(host));
>>>>> -       else if (mmc_card_keep_power(host) &&
>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>> +       } else if (mmc_card_keep_power(host) &&
>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>>                   /* We may have switched to 1-bit mode during suspend
>>>>> */
>>>>>                   err = sdio_enable_4bit_bus(host->card);
>>>>>                   if (err > 0) {
>>>>> --
>>>>> --
>>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>>>> member
>>>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>>>
>>>>> --
>>>>> 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
>>>>
>>>> Kind regards
>>>> Ulf Hansson
>>>
>>>
>

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Subhash Jadavani]

On 12/7/2012 8:21 PM, Ulf Hansson wrote:
> On 7 December 2012 13:15, Subhash Jadavani <subhashj@codeaurora.org> wrote:
>> On 12/7/2012 3:40 AM, Ulf Hansson wrote:
>>> On 6 December 2012 16:25, Subhash Jadavani <subhashj@codeaurora.org>
>>> wrote:
>>>> On 12/6/2012 4:03 PM, Ulf Hansson wrote:
>>>>> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org>
>>>>> wrote:
>>>>>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>>>>>> be reinitialized during resume but as we are not resetting
>>>>>> (CMD52 reset) the SDIO card during this reinitialization, card may
>>>>>> fail to respond back to subsequent commands (CMD5 etc...).
>>>>>>
>>>>>> This change resets the card before the reinitialization of card
>>>>>> during resume.
>>>>>>
>>>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>>>> ---
>>>>>>     drivers/mmc/core/sdio.c |    6 ++++--
>>>>>>     1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>>> index 2273ce6..34ad4c8 100644
>>>>>> --- a/drivers/mmc/core/sdio.c
>>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>>>>            mmc_claim_host(host);
>>>>>>
>>>>>>            /* No need to reinitialize powered-resumed nonremovable cards
>>>>>> */
>>>>>> -       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>>>>> +       if (mmc_card_is_removable(host) || !mmc_card_keep_power(host))
>>>>>> {
>>>>>> +               sdio_reset(host);
>>>>>> +               mmc_go_idle(host);
>>>>> If the card has lost power, why is this needed? Could it be that the
>>>>> host is not capable of cutting the power to card?
>>>>
>>>> Yes, the regulator powering the card is always on in this case which
>>>> means
>>>> SDIO VDD is not powered off during the suspend.
>>>>
>>> So in principle the host should be able to notify the protocol layer
>>> about that mmc_card_keep_power will always be set somehow.
>>> That could be a more proper way of solving this maybe? What do you think?
>> Ok.
>> Basically this would be the suspend/resume sequence with Keep Power flag
>> cleared.
> I mean the exact opposite. :-)
>
> mmc_keep_power should be _set_ during suspend/resume sequence.
>
> That will mean at suspend; sdio_disable_4bit_bus will be called and
> mmc_power_off will not.
> At resume, sdio_enable_4bit_bus will be called, without doing
> mmc_power_up and mmc_sdio_init_card.
>
> Do note that there is no API for the setting mmc_keep_power from a
> host. It exist only in the SDIO API. (sdio_set_host_pm_flags).
> So I guess we should invent this API for the host to use.

Thanks Ulf. I got your point. Will try out your suggestion and will 
update (with patch & result) soon.
On the other hand,
Do you see it's worth to have this current patch as well? I guess it's 
safe to do sdio_reset() before SDIO card init and we are already playing 
safe even in mmc_rescan*() by calling sdio_reset() before card detection.

Regards,
Subhash

>
>> Suspend:
>> 1. Execute Function driver suspend
>> 2. mmc_power_off()
>>      2.1 As this is regulator powering card SDIO VDD line is always on
>> regulator, it will stay on.
>>
>> Resume:
>> 1. mmc_power_up()
>>       1.1 Regulator was anyway on so nothing change here.
>> 2.  mmc_sdio_init_card() - reinitialize the card
>>       2.1 CMD5 gets the commands response timeout which means card doesn't
>> respond back.
>>
>> My understanding is that behaviour seen in 2.1 (resume) is because during
>> suspend, function driver suspend might have done something with the SDIO
>> card (as keep power flag was disabled) and that's the reason during resume,
>> card fails to respond back to CMD5 (unless we send the CMD52 to reset the
>> card). It's an athros SDIO wifi card here.
>>
>>
>>>> So in principle the host should be able to notify the protocol layer
>>>> about that mmc_card_keep_power will always be set somehow.
>> Given the above issue, i am not sure how is this suggestion going to help in
>> this case?
>>
>> Regards,
>> Subhash
>>
>>
>>>>> It might be more safe to do this but I would like to understand more,
>>>>> before you get my ack on this patch.
>>>>>
>>>>>>                    err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>>>>                                            mmc_card_keep_power(host));
>>>>>> -       else if (mmc_card_keep_power(host) &&
>>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>>> +       } else if (mmc_card_keep_power(host) &&
>>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>>>                    /* We may have switched to 1-bit mode during suspend
>>>>>> */
>>>>>>                    err = sdio_enable_4bit_bus(host->card);
>>>>>>                    if (err > 0) {
>>>>>> --
>>>>>> --
>>>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>>>>> member
>>>>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>>>>
>>>>>> --
>>>>>> 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
>>>>> Kind regards
>>>>> Ulf Hansson
>>>>

Re: [PATCH v1 1/3] mmc: sdio: fix resume failure

[Ulf Hansson]

On 8 December 2012 06:55, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> On 12/7/2012 8:21 PM, Ulf Hansson wrote:
>>
>> On 7 December 2012 13:15, Subhash Jadavani <subhashj@codeaurora.org>
>> wrote:
>>>
>>> On 12/7/2012 3:40 AM, Ulf Hansson wrote:
>>>>
>>>> On 6 December 2012 16:25, Subhash Jadavani <subhashj@codeaurora.org>
>>>> wrote:
>>>>>
>>>>> On 12/6/2012 4:03 PM, Ulf Hansson wrote:
>>>>>>
>>>>>> On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> If SDIO keep power flag (MMC_PM_KEEP_POWER) is not set, card would
>>>>>>> be reinitialized during resume but as we are not resetting
>>>>>>> (CMD52 reset) the SDIO card during this reinitialization, card may
>>>>>>> fail to respond back to subsequent commands (CMD5 etc...).
>>>>>>>
>>>>>>> This change resets the card before the reinitialization of card
>>>>>>> during resume.
>>>>>>>
>>>>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>>>>> ---
>>>>>>>     drivers/mmc/core/sdio.c |    6 ++++--
>>>>>>>     1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>>>> index 2273ce6..34ad4c8 100644
>>>>>>> --- a/drivers/mmc/core/sdio.c
>>>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>>>> @@ -937,10 +937,12 @@ static int mmc_sdio_resume(struct mmc_host
>>>>>>> *host)
>>>>>>>            mmc_claim_host(host);
>>>>>>>
>>>>>>>            /* No need to reinitialize powered-resumed nonremovable
>>>>>>> cards
>>>>>>> */
>>>>>>> -       if (mmc_card_is_removable(host) ||
>>>>>>> !mmc_card_keep_power(host))
>>>>>>> +       if (mmc_card_is_removable(host) ||
>>>>>>> !mmc_card_keep_power(host))
>>>>>>> {
>>>>>>> +               sdio_reset(host);
>>>>>>> +               mmc_go_idle(host);
>>>>>>
>>>>>> If the card has lost power, why is this needed? Could it be that the
>>>>>> host is not capable of cutting the power to card?
>>>>>
>>>>>
>>>>> Yes, the regulator powering the card is always on in this case which
>>>>> means
>>>>> SDIO VDD is not powered off during the suspend.
>>>>>
>>>> So in principle the host should be able to notify the protocol layer
>>>> about that mmc_card_keep_power will always be set somehow.
>>>> That could be a more proper way of solving this maybe? What do you
>>>> think?
>>>
>>> Ok.
>>> Basically this would be the suspend/resume sequence with Keep Power flag
>>> cleared.
>>
>> I mean the exact opposite. :-)
>>
>> mmc_keep_power should be _set_ during suspend/resume sequence.
>>
>> That will mean at suspend; sdio_disable_4bit_bus will be called and
>> mmc_power_off will not.
>> At resume, sdio_enable_4bit_bus will be called, without doing
>> mmc_power_up and mmc_sdio_init_card.
>>
>> Do note that there is no API for the setting mmc_keep_power from a
>> host. It exist only in the SDIO API. (sdio_set_host_pm_flags).
>> So I guess we should invent this API for the host to use.
>
>
> Thanks Ulf. I got your point. Will try out your suggestion and will update
> (with patch & result) soon.

OK, great. You may keep me on cc.

> On the other hand,
> Do you see it's worth to have this current patch as well? I guess it's safe
> to do sdio_reset() before SDIO card init and we are already playing safe
> even in mmc_rescan*() by calling sdio_reset() before card detection.
>

That is a valid point. So you have my Ack for this patch!

> Regards,
> Subhash
>
>
>>
>>> Suspend:
>>> 1. Execute Function driver suspend
>>> 2. mmc_power_off()
>>>      2.1 As this is regulator powering card SDIO VDD line is always on
>>> regulator, it will stay on.
>>>
>>> Resume:
>>> 1. mmc_power_up()
>>>       1.1 Regulator was anyway on so nothing change here.
>>> 2.  mmc_sdio_init_card() - reinitialize the card
>>>       2.1 CMD5 gets the commands response timeout which means card
>>> doesn't
>>> respond back.
>>>
>>> My understanding is that behaviour seen in 2.1 (resume) is because during
>>> suspend, function driver suspend might have done something with the SDIO
>>> card (as keep power flag was disabled) and that's the reason during
>>> resume,
>>> card fails to respond back to CMD5 (unless we send the CMD52 to reset the
>>> card). It's an athros SDIO wifi card here.
>>>
>>>
>>>>> So in principle the host should be able to notify the protocol layer
>>>>> about that mmc_card_keep_power will always be set somehow.
>>>
>>> Given the above issue, i am not sure how is this suggestion going to help
>>> in
>>> this case?
>>>
>>> Regards,
>>> Subhash
>>>
>>>
>>>>>> It might be more safe to do this but I would like to understand more,
>>>>>> before you get my ack on this patch.
>>>>>>
>>>>>>>                    err = mmc_sdio_init_card(host, host->ocr,
>>>>>>> host->card,
>>>>>>>
>>>>>>> mmc_card_keep_power(host));
>>>>>>> -       else if (mmc_card_keep_power(host) &&
>>>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>>>> +       } else if (mmc_card_keep_power(host) &&
>>>>>>> mmc_card_wake_sdio_irq(host)) {
>>>>>>>                    /* We may have switched to 1-bit mode during
>>>>>>> suspend
>>>>>>> */
>>>>>>>                    err = sdio_enable_4bit_bus(host->card);
>>>>>>>                    if (err > 0) {
>>>>>>> --
>>>>>>> --
>>>>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>>>>>> member
>>>>>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>>>>>
>>>>>>> --
>>>>>>> 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
>>>>>>
>>>>>> Kind regards
>>>>>> Ulf Hansson
>>>>>
>>>>>
>

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

Kind regards
Ulf Hansson

[PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Subhash Jadavani]

From: Sujit Reddy Thumma <sthumma@codeaurora.org>

According to UHS-I initialization sequence for SDIO 3.0 cards,
the host must set bit[24] (S18R) of OCR register during OCR
handshake to know whether the SDIO card is capable of doing
1.8V I/O.

Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
---
 drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
 include/linux/mmc/host.h |    8 ++++++++
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 34ad4c8..9565d38 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -157,10 +157,7 @@ static int sdio_read_cccr(struct mmc_card *card, u32 ocr)
 			if (ret)
 				goto out;
 
-			if (card->host->caps &
-				(MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
-				 MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
-				 MMC_CAP_UHS_DDR50)) {
+			if (mmc_host_uhs(card->host)) {
 				if (data & SDIO_UHS_DDR50)
 					card->sw_caps.sd3_bus_mode
 						|= SD_MODE_UHS_DDR50;
@@ -478,8 +475,7 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
 	 * If the host doesn't support any of the UHS-I modes, fallback on
 	 * default speed.
 	 */
-	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
-	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
+	if (!mmc_host_uhs(card->host))
 		return 0;
 
 	bus_speed = SDIO_SPEED_SDR12;
@@ -645,11 +641,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 * systems that claim 1.8v signalling in fact do not support
 	 * it.
 	 */
-	if ((ocr & R4_18V_PRESENT) &&
-		(host->caps &
-			(MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
-			 MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
-			 MMC_CAP_UHS_DDR50))) {
+	if ((ocr & R4_18V_PRESENT) && mmc_host_uhs(host)) {
 		err = mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180,
 				true);
 		if (err) {
@@ -1022,6 +1014,10 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 		goto out;
 	}
 
+	if (mmc_host_uhs(host))
+		/* to query card if 1.8V signalling is supported */
+		host->ocr |= R4_18V_PRESENT;
+
 	ret = mmc_sdio_init_card(host, host->ocr, host->card,
 				mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
@@ -1087,6 +1083,10 @@ int mmc_attach_sdio(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
+	if (mmc_host_uhs(host))
+		/* to query card if 1.8V signalling is supported */
+		host->ocr |= R4_18V_PRESENT;
+
 	err = mmc_sdio_init_card(host, host->ocr, NULL, 0);
 	if (err) {
 		if (err == -EAGAIN) {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 23df21e..9be0440 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -433,6 +433,14 @@ static inline int mmc_boot_partition_access(struct mmc_host *host)
 	return !(host->caps2 & MMC_CAP2_BOOTPART_NOACC);
 }
 
+static inline int mmc_host_uhs(struct mmc_host *host)
+{
+	return host->caps &
+		(MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
+		 MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
+		 MMC_CAP_UHS_DDR50);
+}
+
 #ifdef CONFIG_MMC_CLKGATE
 void mmc_host_clk_hold(struct mmc_host *host);
 void mmc_host_clk_release(struct mmc_host *host);
-- 
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Bing Zhao]

Hi,

> From: Sujit Reddy Thumma <sthumma@codeaurora.org>
> 
> According to UHS-I initialization sequence for SDIO 3.0 cards,
> the host must set bit[24] (S18R) of OCR register during OCR
> handshake to know whether the SDIO card is capable of doing
> 1.8V I/O.

In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
Setting it to 1 might cause side effect to 2.0 cards.
Perhaps using a quirk for S18R is more suitable for all combinations.

Regards,
Bing

> 
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> ---
>  drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
>  include/linux/mmc/host.h |    8 ++++++++
>  2 files changed, 19 insertions(+), 11 deletions(-)

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Shen, Jackey]

Hi Bing,

The function mmc_host_uhs() will return false and never set bit[24] (S18R) for SDIO 2.0 cards.

Thanks,
Jackey

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Bing Zhao
Sent: Wednesday, December 05, 2012 5:14 AM
To: Subhash Jadavani; linux-mmc@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

Hi,

> From: Sujit Reddy Thumma <sthumma@codeaurora.org>
> 
> According to UHS-I initialization sequence for SDIO 3.0 cards, the 
> host must set bit[24] (S18R) of OCR register during OCR handshake to 
> know whether the SDIO card is capable of doing 1.8V I/O.

In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
Setting it to 1 might cause side effect to 2.0 cards.
Perhaps using a quirk for S18R is more suitable for all combinations.

Regards,
Bing

> 
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> ---
>  drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
>  include/linux/mmc/host.h |    8 ++++++++
>  2 files changed, 19 insertions(+), 11 deletions(-)

--
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

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Bing Zhao]

Hi Jackey,

> Hi Bing,
> 
> The function mmc_host_uhs() will return false and never set bit[24] (S18R) for SDIO 2.0 cards.

I thought mmc_host_uhs() will return host controller's capabilities, no?

Thanks,
Bing

> 
> Thanks,
> Jackey
> 
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Bing Zhao
> Sent: Wednesday, December 05, 2012 5:14 AM
> To: Subhash Jadavani; linux-mmc@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
> Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence
> 
> Hi,
> 
> > From: Sujit Reddy Thumma <sthumma@codeaurora.org>
> >
> > According to UHS-I initialization sequence for SDIO 3.0 cards, the
> > host must set bit[24] (S18R) of OCR register during OCR handshake to
> > know whether the SDIO card is capable of doing 1.8V I/O.
> 
> In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
> Setting it to 1 might cause side effect to 2.0 cards.
> Perhaps using a quirk for S18R is more suitable for all combinations.
> 
> Regards,
> Bing
> 
> >
> > Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > ---
> >  drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
> >  include/linux/mmc/host.h |    8 ++++++++
> >  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> --
> 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
> 

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Shen, Jackey]

Hi Bing,

sdhci_add_host() reads host controller's capabilities and initialize host->caps of function mmc_host_uhs(). So mmc_host_uhs() only return this cached values.

Thanks,
Jackey

-----Original Message-----
From: Bing Zhao [mailto:bzhao@marvell.com] 
Sent: Thursday, December 06, 2012 3:14 AM
To: Shen, Jackey; Subhash Jadavani; linux-mmc@vger.kernel.org
Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

Hi Jackey,

> Hi Bing,
> 
> The function mmc_host_uhs() will return false and never set bit[24] (S18R) for SDIO 2.0 cards.

I thought mmc_host_uhs() will return host controller's capabilities, no?

Thanks,
Bing

> 
> Thanks,
> Jackey
> 
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org 
> [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Bing Zhao
> Sent: Wednesday, December 05, 2012 5:14 AM
> To: Subhash Jadavani; linux-mmc@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
> Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I 
> initialization sequence
> 
> Hi,
> 
> > From: Sujit Reddy Thumma <sthumma@codeaurora.org>
> >
> > According to UHS-I initialization sequence for SDIO 3.0 cards, the 
> > host must set bit[24] (S18R) of OCR register during OCR handshake to 
> > know whether the SDIO card is capable of doing 1.8V I/O.
> 
> In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
> Setting it to 1 might cause side effect to 2.0 cards.
> Perhaps using a quirk for S18R is more suitable for all combinations.
> 
> Regards,
> Bing
> 
> >
> > Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > ---
> >  drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
> >  include/linux/mmc/host.h |    8 ++++++++
> >  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> --
> 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
> 

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Bing Zhao]

Hi Jackey,

> Hi Bing,
> 
> sdhci_add_host() reads host controller's capabilities and initialize host->caps of function
> mmc_host_uhs(). So mmc_host_uhs() only return this cached values.

So, it does represent host controller's capabilities, not the capabilities of the card.
What happens if I have a UHS capable controller and a 2.0 card?
The S18R bit will be set in this case, which might cause problem for the card?

Thanks,
Bing

> 
> Thanks,
> Jackey
> 
> -----Original Message-----
> From: Bing Zhao [mailto:bzhao@marvell.com]
> Sent: Thursday, December 06, 2012 3:14 AM
> To: Shen, Jackey; Subhash Jadavani; linux-mmc@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
> Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence
> 
> Hi Jackey,
> 
> > Hi Bing,
> >
> > The function mmc_host_uhs() will return false and never set bit[24] (S18R) for SDIO 2.0 cards.
> 
> I thought mmc_host_uhs() will return host controller's capabilities, no?
> 
> Thanks,
> Bing
> 
> >
> > Thanks,
> > Jackey
> >
> > -----Original Message-----
> > From: linux-mmc-owner@vger.kernel.org
> > [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Bing Zhao
> > Sent: Wednesday, December 05, 2012 5:14 AM
> > To: Subhash Jadavani; linux-mmc@vger.kernel.org
> > Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
> > Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I
> > initialization sequence
> >
> > Hi,
> >
> > > From: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > >
> > > According to UHS-I initialization sequence for SDIO 3.0 cards, the
> > > host must set bit[24] (S18R) of OCR register during OCR handshake to
> > > know whether the SDIO card is capable of doing 1.8V I/O.
> >
> > In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
> > Setting it to 1 might cause side effect to 2.0 cards.
> > Perhaps using a quirk for S18R is more suitable for all combinations.
> >
> > Regards,
> > Bing
> >
> > >
> > > Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> > > ---
> > >  drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
> > >  include/linux/mmc/host.h |    8 ++++++++
> > >  2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > --
> > 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
> >
> 
> 

Re: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Subhash Jadavani]

On 12/6/2012 9:03 AM, Bing Zhao wrote:
> Hi Jackey,
>
>> Hi Bing,
>>
>> sdhci_add_host() reads host controller's capabilities and initialize host->caps of function
>> mmc_host_uhs(). So mmc_host_uhs() only return this cached values.
> So, it does represent host controller's capabilities, not the capabilities of the card.
> What happens if I have a UHS capable controller and a 2.0 card?
> The S18R bit will be set in this case, which might cause problem for the card?
Hi Bing,

Why do you see an issue with setting S18R bit when sending CMD5 to 
SDIO2.0 compliant card? If SDIO2.0 card don't support it, return OCR 
won't have the S18A bit set.
We are already doing same thing for SD cards as well (check 
mmc_sd_get_cid function). There also even if we set the S18R bit which 
sending OCR to non-SD3.0 cards, they respond back with S18A bit cleared.

If non-SDIO3.0 card misbehaves after receiving the OCR with S18R bit set 
then it means card is not compliant to specification properly and in 
that case those cards can be handled with QUIRKs. But there is no need 
to add any QUIRKs as of now.

I hope this make sense.

Regards,
Subhash

>
> Thanks,
> Bing
>
>> Thanks,
>> Jackey
>>
>> -----Original Message-----
>> From: Bing Zhao [mailto:bzhao@marvell.com]
>> Sent: Thursday, December 06, 2012 3:14 AM
>> To: Shen, Jackey; Subhash Jadavani; linux-mmc@vger.kernel.org
>> Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
>> Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence
>>
>> Hi Jackey,
>>
>>> Hi Bing,
>>>
>>> The function mmc_host_uhs() will return false and never set bit[24] (S18R) for SDIO 2.0 cards.
>> I thought mmc_host_uhs() will return host controller's capabilities, no?
>>
>> Thanks,
>> Bing
>>
>>> Thanks,
>>> Jackey
>>>
>>> -----Original Message-----
>>> From: linux-mmc-owner@vger.kernel.org
>>> [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Bing Zhao
>>> Sent: Wednesday, December 05, 2012 5:14 AM
>>> To: Subhash Jadavani; linux-mmc@vger.kernel.org
>>> Cc: linux-arm-msm@vger.kernel.org; Sujit Reddy Thumma
>>> Subject: RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I
>>> initialization sequence
>>>
>>> Hi,
>>>
>>>> From: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>>>
>>>> According to UHS-I initialization sequence for SDIO 3.0 cards, the
>>>> host must set bit[24] (S18R) of OCR register during OCR handshake to
>>>> know whether the SDIO card is capable of doing 1.8V I/O.
>>> In SDIO 2.0 spec, bit[24] is reserved, "shall be set to 0".
>>> Setting it to 1 might cause side effect to 2.0 cards.
>>> Perhaps using a quirk for S18R is more suitable for all combinations.
>>>
>>> Regards,
>>> Bing
>>>
>>>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>> ---
>>>>   drivers/mmc/core/sdio.c  |   22 +++++++++++-----------
>>>>   include/linux/mmc/host.h |    8 ++++++++
>>>>   2 files changed, 19 insertions(+), 11 deletions(-)
>>> --
>>> 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
>>>
>>

RE: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Bing Zhao]

Hi Subhash,

> >> sdhci_add_host() reads host controller's capabilities and initialize host->caps of function
> >> mmc_host_uhs(). So mmc_host_uhs() only return this cached values.
> > So, it does represent host controller's capabilities, not the capabilities of the card.
> > What happens if I have a UHS capable controller and a 2.0 card?
> > The S18R bit will be set in this case, which might cause problem for the card?
> Hi Bing,
> 
> Why do you see an issue with setting S18R bit when sending CMD5 to
> SDIO2.0 compliant card? 
> If SDIO2.0 card don't support it, return OCR won't have the S18A bit set.

Some times ago I ever encountered a problem with CMD52 when I played with an SDIO driver and accidentally set one of the reserved bits to 1. The issue was resolved immediately after I realized it and cleared that bit to 0.

This experience makes me thinking that CMD5 S18R might confuse some 2.0 cards, because bit24 is also a reserved bit in 2.0 spec.

> We are already doing same thing for SD cards as well (check
> mmc_sd_get_cid function). There also even if we set the S18R bit which
> sending OCR to non-SD3.0 cards, they respond back with S18A bit cleared.

Then it's fair enough to handle SDIO the same way. Thanks for the elaboration.

> 
> If non-SDIO3.0 card misbehaves after receiving the OCR with S18R bit set
> then it means card is not compliant to specification properly and in
> that case those cards can be handled with QUIRKs. But there is no need
> to add any QUIRKs as of now.

That sounds reasonable. The quirk for non-compliant cards can be done later when they come up.

Regards,
Bing

> 
> I hope this make sense.
> 
> Regards,
> Subhash
> 

Re: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Johan Rudholm]

Hi,

2012/12/4 Subhash Jadavani <subhashj@codeaurora.org>:
> From: Sujit Reddy Thumma <sthumma@codeaurora.org>
>
> According to UHS-I initialization sequence for SDIO 3.0 cards,
> the host must set bit[24] (S18R) of OCR register during OCR
> handshake to know whether the SDIO card is capable of doing
> 1.8V I/O.
>

Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>

Re: [PATCH v1 2/3] mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence

[Ulf Hansson]

On 7 December 2012 09:42, Johan Rudholm <jrudholm@gmail.com> wrote:
> Hi,
>
> 2012/12/4 Subhash Jadavani <subhashj@codeaurora.org>:
>> From: Sujit Reddy Thumma <sthumma@codeaurora.org>
>>
>> According to UHS-I initialization sequence for SDIO 3.0 cards,
>> the host must set bit[24] (S18R) of OCR register during OCR
>> handshake to know whether the SDIO card is capable of doing
>> 1.8V I/O.
>>
>
> Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> --
> 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

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

[PATCH v1 3/3] mmc: sdio: print correct UHS mode during card detection

[Subhash Jadavani]

When SDIO3.0 card is detected, incorrect bus speed mode
is printed as part of card detection print in kernel logs.

This change fixes it so that user won't be confused by
looking at incorrect card detection message in logs.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
---
 drivers/mmc/core/sdio.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 9565d38..3a64933 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -485,23 +485,27 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
 			bus_speed = SDIO_SPEED_SDR104;
 			timing = MMC_TIMING_UHS_SDR104;
 			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
+			card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
 	} else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
 		   (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
 			bus_speed = SDIO_SPEED_DDR50;
 			timing = MMC_TIMING_UHS_DDR50;
 			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
+			card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
 	} else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
 		    MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
 		    SD_MODE_UHS_SDR50)) {
 			bus_speed = SDIO_SPEED_SDR50;
 			timing = MMC_TIMING_UHS_SDR50;
 			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
+			card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
 	} else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
 		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
 		   (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
 			bus_speed = SDIO_SPEED_SDR25;
 			timing = MMC_TIMING_UHS_SDR25;
 			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
+			card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
 	} else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
 		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
 		    MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode &
@@ -509,6 +513,7 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
 			bus_speed = SDIO_SPEED_SDR12;
 			timing = MMC_TIMING_UHS_SDR12;
 			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
+			card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
 	}
 
 	err = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_SPEED, 0, &speed);
-- 
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1 3/3] mmc: sdio: print correct UHS mode during card detection

[Ulf Hansson]

On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> When SDIO3.0 card is detected, incorrect bus speed mode
> is printed as part of card detection print in kernel logs.
>
> This change fixes it so that user won't be confused by
> looking at incorrect card detection message in logs.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> ---
>  drivers/mmc/core/sdio.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 9565d38..3a64933 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -485,23 +485,27 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
>                         bus_speed = SDIO_SPEED_SDR104;
>                         timing = MMC_TIMING_UHS_SDR104;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
>         } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
>                         bus_speed = SDIO_SPEED_DDR50;
>                         timing = MMC_TIMING_UHS_DDR50;
>                         card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> +                       card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR50)) {
>                         bus_speed = SDIO_SPEED_SDR50;
>                         timing = MMC_TIMING_UHS_SDR50;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
>                         bus_speed = SDIO_SPEED_SDR25;
>                         timing = MMC_TIMING_UHS_SDR25;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
>                     MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode &
> @@ -509,6 +513,7 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
>                         bus_speed = SDIO_SPEED_SDR12;
>                         timing = MMC_TIMING_UHS_SDR12;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
>         }
>
>         err = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_SPEED, 0, &speed);
> --
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> 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

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

RE: [PATCH v1 3/3] mmc: sdio: print correct UHS mode during card detection

[Shen, Jackey]

Hi,

Tested-by: Jackey Shen <Jackey.Shen@amd.com>

Thanks,
Jackey

-----Original Message-----
From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Ulf Hansson
Sent: Thursday, December 06, 2012 6:42 PM
To: Subhash Jadavani
Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v1 3/3] mmc: sdio: print correct UHS mode during card detection

On 4 December 2012 12:36, Subhash Jadavani <subhashj@codeaurora.org> wrote:
> When SDIO3.0 card is detected, incorrect bus speed mode is printed as 
> part of card detection print in kernel logs.
>
> This change fixes it so that user won't be confused by looking at 
> incorrect card detection message in logs.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> ---
>  drivers/mmc/core/sdio.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index 
> 9565d38..3a64933 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -485,23 +485,27 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
>                         bus_speed = SDIO_SPEED_SDR104;
>                         timing = MMC_TIMING_UHS_SDR104;
>                         card->sw_caps.uhs_max_dtr = 
> UHS_SDR104_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
>         } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
>                         bus_speed = SDIO_SPEED_DDR50;
>                         timing = MMC_TIMING_UHS_DDR50;
>                         card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> +                       card->sd_bus_speed = UHS_DDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50)) && (card->sw_caps.sd3_bus_mode &
>                     SD_MODE_UHS_SDR50)) {
>                         bus_speed = SDIO_SPEED_SDR50;
>                         timing = MMC_TIMING_UHS_SDR50;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR50_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
>                    (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25)) {
>                         bus_speed = SDIO_SPEED_SDR25;
>                         timing = MMC_TIMING_UHS_SDR25;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR25_BUS_SPEED;
>         } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>                     MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
>                     MMC_CAP_UHS_SDR12)) && (card->sw_caps.sd3_bus_mode 
> & @@ -509,6 +513,7 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
>                         bus_speed = SDIO_SPEED_SDR12;
>                         timing = MMC_TIMING_UHS_SDR12;
>                         card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> +                       card->sd_bus_speed = UHS_SDR12_BUS_SPEED;
>         }
>
>         err = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_SPEED, 0, 
> &speed);
> --
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
> member of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> 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

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
--
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

Re: FW: [PATCH v1 0/3] SDIO fixes

[Subhash Jadavani]

Hi Chris,

Can i request you to merge following 3 patches to mmc-next? Ulf have 
acked all 3 patches and Johan Rudholm also reviewed one of them.

Regards,
Subhash

>
>
>
>
>
> -------- Original Message --------
> Subject: [PATCH v1 0/3] SDIO fixes
> Date: Tue,  4 Dec 2012 17:06:17 +0530
> From: Subhash Jadavani <subhashj@codeaurora.org>
> To: linux-mmc@vger.kernel.org
> CC: linux-arm-msm@vger.kernel.org, Subhash Jadavani <subhashj@codeaurora.org>
>
> This patch-series is having 1 fix for SDIO resume and 2 fixes related to SDIO3.0 card detection
>
> Subhash Jadavani (2):
>     mmc: sdio: fix resume failure
>     mmc: sdio: print correct UHS mode during card detection
>
> Sujit Reddy Thumma (1):
>     mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence
>
>    drivers/mmc/core/sdio.c  |   33 ++++++++++++++++++++-------------
>    include/linux/mmc/host.h |    8 ++++++++
>    2 files changed, 28 insertions(+), 13 deletions(-)
>
> --
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> 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
>
>

Re: FW: [PATCH v1 0/3] SDIO fixes

[Chris Ball]

Hi Subhash,

On Tue, Dec 11 2012, Subhash Jadavani wrote:
> This patch-series is having 1 fix for SDIO resume and 2 fixes related to SDIO3.0 card detection
>
> Subhash Jadavani (2):
>     mmc: sdio: fix resume failure
>     mmc: sdio: print correct UHS mode during card detection
>
> Sujit Reddy Thumma (1):
>     mmc: sdio: Fix SDIO 3.0 UHS-I initialization sequence
>
>    drivers/mmc/core/sdio.c  |   33 ++++++++++++++++++++-------------
>    include/linux/mmc/host.h |    8 ++++++++
>    2 files changed, 28 insertions(+), 13 deletions(-)

Thanks, pushed all three patches to mmc-next.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child