Re: [PATCH v2] mmc: core: Fix the HPI execution sequence

[kdorfman@codeaurora.org]

Hello,

2 comments (see inside):
1. About card state before sending HPI
2. About waiting after sending HPI

> On Wed, May 9, 2012 at 7:36 PM,  <kdorfman@codeaurora.org> wrote:
>>> On Wed, May 9, 2012 at 6:47 PM,  <kdorfman@codeaurora.org> wrote:
>>
>>>>> @@ -403,6 +403,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>  {
>>>>>       int err;
>>>>>       u32 status;
>>>>> +     unsigned long starttime;
>>>>>
>>>>>       BUG_ON(!card);
>>>>>
>>>>> @@ -421,27 +422,31 @@ int mmc_interrupt_hpi(struct mmc_card *card)
>>>>>       /*
>>>>>        * If the card status is in PRG-state, we can send the HPI
>>>>> command.
>>>>>        */
>>>>> -     if (R1_CURRENT_STATE(status) == R1_STATE_PRG) {
>>>>> -             do {
>>>>> -                     /*
>>>>> -                      * We don't know when the HPI command will
>>>>> finish
>>>>> -                      * processing, so we need to resend HPI until
>>>>> out
>>>>> -                      * of prg-state, and keep checking the card
>>>>> status
>>>>> -                      * with SEND_STATUS.  If a timeout error occurs
>>>>> when
>>>>> -                      * sending the HPI command, we are already out
>>>>> of
>>>>> -                      * prg-state.
>>>>> -                      */
>>>>> -                     err = mmc_send_hpi_cmd(card, &status);
>>>>> -                     if (err)
>>>>> -                             pr_debug("%s: abort HPI (%d error)\n",
>>>>> -                                      mmc_hostname(card->host),
>>>>> err);
>>>>> +     if (R1_CURRENT_STATE(status) != R1_STATE_PRG) {
>>>>> +             pr_debug("%s: Can't send HPI: Card state=%d\n",
>>>>> +                     mmc_hostname(card->host),
>>>>> R1_CURRENT_STATE(status));
>>>>> +             err = -EINVAL;
>>>>> +             goto out;
>>>>> +     }
>>>> You can't return error here, because the card status may be:
>>>> 0 = Idle 1 = Ready 2 = Ident 3 = Stby 4 = Tran 5 = Data 6 = Rcv 7 =
>>>> Prg
>>>> 8
>>>> = Dis 9 = Btst 10
>>>> Not every value is error here: the card state may be Idle/Ready and
>>>> then
>>>> you do not need to return error.
>>>>
>>> Depends. When the intent is to send HPI command, and the current state
>>> is such that it can't be sent,
>>> then a error has to be returned, right ?
>>> OTOH, if the API was "mmc_get_to_transfer_state()", then it would make
>>> sense to not return an error.

It does't matter what caller flow is, when current operation finished, the
card may be in IDLE state
you do not need to send HPI and it is not error, because it is like to
have successful HPI done.
Probably not only IDLE state is like this (I think of standby state).

> It's a completely different issue that I had informed Saugata, who is
> doing the rework on
> power off notify and sleep command sequence.
> Essentially, the card shouldn't be put to sleep once BKOPS_START has
> been issued,
> and we should need to interrupt with HPI only when a higher layer
> request needs to be serviced.

Once BKOPS started, it will trigger IDLE timer for 5 sec, but BKOPS may
take more time.
This means that card will enter sleep again and CMD5 will fail.

> I don't know whether you are interested in handling only urgent BKOPS
> or periodic BKOPS,
> but we can discuss the design on a separate thread.

Let's do this discussion on newly published thread: "[PATCH v8] mmc:
support BKOPS feature for eMMC"

>
>>>
>>>
>>>>> +
>>>>> +     starttime = jiffies;
>>>>> +     err = mmc_send_hpi_cmd(card, &status);
>>>>> +     if (err) {
>>>>> +             pr_debug("%s:HPI could not be sent.err=%d)\n",
>>>>> +                     mmc_hostname(card->host), err);
>>>>> +             goto out;
>>>>> +     }
>>>>> +
>>>>> +     do {
>>>>> +             err = mmc_send_status(card, &status);
>>>>> +
>>>>> +             if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
>>>>> +                     goto out;
>>>>> +             if (jiffies_to_msecs(jiffies - starttime) >
>>>>> +                     card->ext_csd.out_of_int_time)
I think we need to add some margin time to the out_of_int_time here.
Because card clock is different from
jiffies.

Thanks,
Kostya
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum