Re: [PATCH] mmc: core: Fix PowerOff Notify suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?

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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux