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


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

  Powered by Linux