On 16 March 2012 10:43, Saugata Das <saugata.das@xxxxxxxxxx> wrote:
> On 16 March 2012 09:19, Girish K S <girish.shivananjappa@xxxxxxxxxx> wrote:
>> On 14 March 2012 20:53, Ulf Hansson <ulf.hansson@xxxxxxxxxxxxxx> wrote:
>>> Hi Girish and Chris,
>>>
>>> I noticed that this has been pushed for 3.3, I think we need to make a
>>> revert of it asap if possible.
>>>
>>> I were unfortunately not able to review this patch earlier but it has issues
>>> I believe. It will break suspend/resume for eMMC devices supporting the
>>> SLEEP command and not the "poweroff notify" from eMMC 4.5.
>>
>> By break do you mean compilation break or system crash. can you please
>> post the log that caused the break.
>> I re checked it on eMMC 4.5, 4.41, high speed mmc card and normal mmc
>> card. I can compile successfully and
>> could test the suspend/ resume functionality without carsh.
>> If you provide the log, i can check more.
>>
>>>
>>> Please see my comments below.
>>>
>>>
>>> On 01/31/2012 11:14 AM, Girish K S wrote:
>>>>
>>>> Modified the mmc_poweroff to resume before sending the
>>>> poweroff notification command. In sleep mode only AWAKE
>>>> and RESET commands are allowed, so before sending the
>>>> poweroff notification command resume from sleep mode and
>>>> then send the notification command.
>>>>
>>>> POwerOff Notify is tested on a Synopsis Designware Host
>>>> Controller(eMMC 4.5). The suspend to RAM and resume works fine.
>>>>
>>>> This patch is successfully applied on the Chris's mmc-next
>>>> branch
>>>>
>>>> cc: Chris Ball<cjb@xxxxxxxxxx>
>>>> Signed-off-by: Girish K S<girish.shivananjappa@xxxxxxxxxx>
>>>> Tested-by: Girish K S<girish.shivananjappa@xxxxxxxxxx>
>>>> ---
>>>> drivers/mmc/core/core.c | 28 ++++++++++++++++++++--------
>>>> drivers/mmc/core/mmc.c | 17 ++++++++++++-----
>>>> include/linux/mmc/card.h | 4 ++++
>>>> 3 files changed, 36 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index bec0bf2..14ec575 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -1239,7 +1239,8 @@ static void mmc_poweroff_notify(struct mmc_host
>>>> *host)
>>>> int err = 0;
>>>>
>>>> card = host->card;
>>>> -
>>>> + mmc_claim_host(host);
>>>> +
>>>> /*
>>>> * Send power notify command only if card
>>>> * is mmc and notify state is powered ON
>>>> @@ -1269,6 +1270,7 @@ static void mmc_poweroff_notify(struct mmc_host
>>>> *host)
>>>> /* Set the card state to no notification after the poweroff
>>>> */
>>>> card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
>>>> }
>>>> + mmc_release_host(host);
>>>> }
>>>>
>>>> /*
>>>> @@ -1327,12 +1329,28 @@ static void mmc_power_up(struct mmc_host *host)
>>>>
>>>> void mmc_power_off(struct mmc_host *host)
>>>> {
>>>> + int err = 0;
>>>> mmc_host_clk_hold(host);
>>>>
>>>> host->ios.clock = 0;
>>>> host->ios.vdd = 0;
>>>>
>>>> - mmc_poweroff_notify(host);
>>>> + /*
>>>> + * For eMMC 4.5 device send AWAKE command before
>>>> + * POWER_OFF_NOTIFY command, because in sleep state
>>>> + * eMMC 4.5 devices respond to only RESET and AWAKE cmd
>>>> + */
>>>> + if (host->card&& mmc_card_is_sleep(host->card)&&
>>>> + host->bus_ops->resume) {
>>>> + err = host->bus_ops->resume(host);
>>>
>>>
>>> This is just plain wrong. First we may have suspended the host from
>>> mmc_suspend_host then we enter this funtion (mmc_power_off) to cut the power
>>> to the card. Why do we even want to resume if we just did suspend?
>> for 4.5 case, cards wont respond to any command other than awake and
>> reset. so we resume before executing a switch
>> for poweroff notify. for non 4.5 card, it will resume and wont
>> continue in resume state because of the poweroff executed in the end.
>>>
>>> Moreover, this will actually mean that for mmc devices which are supporting
>>> SLEEP but not the new eMMC 4.5 feature poweroff_notify will leave this
>>> function in a resumed state (in other words, not in SLEEP state) which is
>>> not OK.
>> non 4.5 devices will not remain in resume state. Because mmc_set_ios
>> will power down the device.
>>>
>>>
>>>> +
>>>> + if (!err)
>>>> + mmc_poweroff_notify(host);
>>>> + else
>>>> + pr_warning("%s: error %d during resume "
>>>> + "(continue with poweroff sequence)\n",
>>>> + mmc_hostname(host), err);
>>>> + }
>>>>
>>>> /*
>>>> * Reset ocr mask to be the highest possible voltage supported for
>>>> @@ -2386,12 +2404,6 @@ int mmc_suspend_host(struct mmc_host *host)
>>>> */
>>>> if (mmc_try_claim_host(host)) {
>>>> if (host->bus_ops->suspend) {
>>>> - /*
>>>> - * For eMMC 4.5 device send notify command
>>>> - * before sleep, because in sleep state
>>>> eMMC 4.5
>>>> - * devices respond to only RESET and AWAKE
>>>> cmd
>>>> - */
>>>> - mmc_poweroff_notify(host);
>>>> err = host->bus_ops->suspend(host);
>>>> }
>>>> mmc_do_release_host(host);
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 2bc586b..c3f09a2 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1330,11 +1330,13 @@ static int mmc_suspend(struct mmc_host *host)
>>>> BUG_ON(!host->card);
>>>>
>>>> mmc_claim_host(host);
>>>> - if (mmc_card_can_sleep(host))
>>>> + if (mmc_card_can_sleep(host)) {
>>>> err = mmc_card_sleep(host);
>>>> - else if (!mmc_host_is_spi(host))
>>>> + if (!err)
>>>> + mmc_card_set_sleep(host->card);
>>>
>>>
>>> I suggest to move this inside the mmc_card_sleep function instead.
>> sure will do that as a additional patch
>>>
>>>
>>>> + } else if (!mmc_host_is_spi(host))
>>>> mmc_deselect_cards(host);
>>>> - host->card->state&= ~MMC_STATE_HIGHSPEED;
>>>> + host->card->state&= ~(MMC_STATE_HIGHSPEED |
>>>> MMC_STATE_HIGHSPEED_200);
>>>
>>>
>>> What has MMC_STATE_HIGHSPEED_200 with this patch to do?
>> same as high_speed. on suspend high speed state is reset.
>>>
>>>
>>>> mmc_release_host(host);
>>>>
>>>> return err;
>>>> @@ -1354,7 +1356,11 @@ static int mmc_resume(struct mmc_host *host)
>>>> BUG_ON(!host->card);
>>>>
>>>> mmc_claim_host(host);
>>>> - err = mmc_init_card(host, host->ocr, host->card);
>>>> + if (mmc_card_is_sleep(host->card)) {
>>>> + err = mmc_card_awake(host);
>>>> + mmc_card_clr_sleep(host->card);
>>>
>>>
>>> I suggest to move this inside the mmc_card_sleep function instead.
>> sure will do it.
>
> I guess the proposal is to move this inside mmc_card_awake ?
>
> But is there any specific reason for moving this code within
> mmc_card_awake or the above comment about moving some code from
> mmc_suspend to mmc_card_sleep ?
I understood it as moving the card state (set/clear) to be moved to
sleep and awake function.
>
>>>
>>>
>>>> + } else
>>>> + err = mmc_init_card(host, host->ocr, host->card);
>>>> mmc_release_host(host);
>>>>
>>>> return err;
>>>> @@ -1364,7 +1370,8 @@ static int mmc_power_restore(struct mmc_host *host)
>>>> {
>>>> int ret;
>>>>
>>>> - host->card->state&= ~MMC_STATE_HIGHSPEED;
>>>> + host->card->state&= ~(MMC_STATE_HIGHSPEED |
>>>> MMC_STATE_HIGHSPEED_200);
>>>
>>>
>>> What has MMC_STATE_HIGHSPEED_200 with this patch to do?
>>>
>>>> + mmc_card_clr_sleep(host->card);
>>>
>>>
>>> Why do we have clear the sleep state here?
>> currently it is of no use. coz init_card will initialize the card data
>> structure. If power_save / power_restore function is modified to put
>> the card in sleep state and only wake up (no complete card
>> initialization), then above setting is required.
>>>
>>>> mmc_claim_host(host);
>>>> ret = mmc_init_card(host, host->ocr, host->card);
>>>> mmc_release_host(host);
>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>>> index f9a0663..1a1ca71 100644
>>>> --- a/include/linux/mmc/card.h
>>>> +++ b/include/linux/mmc/card.h
>>>> @@ -219,6 +219,7 @@ struct mmc_card {
>>>> #define MMC_CARD_SDXC (1<<6) /* card is SDXC */
>>>> #define MMC_CARD_REMOVED (1<<7) /* card has been removed */
>>>> #define MMC_STATE_HIGHSPEED_200 (1<<8) /* card is in HS200
>>>> mode */
>>>> +#define MMC_STATE_SLEEP (1<<9) /* card is in
>>>> sleep state */
>>>> unsigned int quirks; /* card quirks */
>>>> #define MMC_QUIRK_LENIENT_FN0 (1<<0) /* allow SDIO FN0 writes
>>>> outside of the VS CCCR range */
>>>> #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1) /* use func->cur_blksize */
>>>> @@ -384,6 +385,7 @@ static inline void __maybe_unused remove_quirk(struct
>>>> mmc_card *card, int data)
>>>> #define mmc_sd_card_uhs(c) ((c)->state& MMC_STATE_ULTRAHIGHSPEED)
>>>> #define mmc_card_ext_capacity(c) ((c)->state& MMC_CARD_SDXC)
>>>> #define mmc_card_removed(c) ((c)&& ((c)->state& MMC_CARD_REMOVED))
>>>> +#define mmc_card_is_sleep(c) ((c)->state& MMC_STATE_SLEEP)
>>>>
>>>>
>>>> #define mmc_card_set_present(c) ((c)->state |= MMC_STATE_PRESENT)
>>>> #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
>>>> @@ -395,7 +397,9 @@ static inline void __maybe_unused remove_quirk(struct
>>>> mmc_card *card, int data)
>>>> #define mmc_sd_card_set_uhs(c) ((c)->state |= MMC_STATE_ULTRAHIGHSPEED)
>>>> #define mmc_card_set_ext_capacity(c) ((c)->state |= MMC_CARD_SDXC)
>>>> #define mmc_card_set_removed(c) ((c)->state |= MMC_CARD_REMOVED)
>>>> +#define mmc_card_set_sleep(c) ((c)->state |= MMC_STATE_SLEEP)
>>>>
>>>> +#define mmc_card_clr_sleep(c) ((c)->state&= ~MMC_STATE_SLEEP)
>>>>
>>>> /*
>>>> * Quirk add/remove for MMC products.
>>>> */
>>>
>>>
>>> Best regards
>>> Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
