Re: [PATCH 3/5] mmc: card: Add eMMC command queuing support in mmc block layer

[Asutosh Das <asutoshd@codeaurora.org>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gbk; format=flowed, Size: 4516 bytes --]

Hi Ziji
On 12/9/2014 12:33 AM, Ziji Hu wrote:
> Hi Asutosh,
>
>          May I ask whether you have tested the code on hardware platform?
I have done some basic testing on an emulation platform that using an 
earlier kernel version.
>          If you have not tested it yet, do you have a schedule for testing?
>
> -----Original Message-----
> From: Asutosh Das [mailto:das.asutosh@gmail.com]
> Sent: 2014Äê12ÔÂ7ÈÕ 21:59
> To: Ziji Hu
> Cc: linux-arm-msm@vger.kernel.org; linux-mmc@vger.kernel.org
> Subject: Re: [PATCH 3/5] mmc: card: Add eMMC command queuing support in mmc block layer
>
> Hi Ziji
> Thanks for your comments.
>
> I'm adding linux-mmc to this as well.
>
> On 8 December 2014 at 07:47, Hu Ziji <huziji@marvell.com> wrote:
>>
>> Asutosh Das <asutoshd <at> codeaurora.org> writes:
>>
>>> +static inline bool mmc_cmdq_should_pull_reqs(struct mmc_host *host,
>>> +                                     struct mmc_cmdq_context_info
>>> +*ctx) {
>>> +     spin_lock_bh(&ctx->cmdq_ctx_lock);
>>> +     if (ctx->active_dcmd || ctx->rpmb_in_wait) {
>>> +             if ((ctx->curr_state != CMDQ_STATE_HALT) ||
>>> +                     (ctx->curr_state != CMDQ_STATE_ERR)) {
>>
>>                          Because CMDQ_STATE_HALT == 0 and
>> CMDQ_STATE_ERR == 1, it is impossible that condition ((ctx->curr_state
>> == CMDQ_STATE_HALT) && (ctx-
>>> curr_state != CMDQ_STATE_ERR)) is satisfied. Thus the above
>>> if-condition will
>> always be true.
>>
>
> Agree.
>
>>
>>> +                     pr_debug("%s: %s: skip pulling reqs: dcmd: %d rpmb: %d state:
>> %d\n",
>>> +                              mmc_hostname(host), __func__, ctx->active_dcmd,
>>> +                              ctx->rpmb_in_wait, ctx->curr_state);
>>> +                     spin_unlock_bh(&ctx->cmdq_ctx_lock);
>>> +                     return false;
>>> +             }
>>> +     } else {
>>> +             spin_unlock_bh(&ctx->cmdq_ctx_lock);
>>> +             return true;
>>> +     }
>>> +}
>>
>>> +/**
>>> + *   mmc_cmdq_halt - halt/un-halt the command queue engine
>>> + *    <at> host: host instance
>>> + *    <at> halt: true - halt, un-halt otherwise
>>> + *
>>> + *   Host halts the command queue engine. It should complete
>>> + *   the ongoing transfer and release the SD bus.
>>> + *   All legacy SD commands can be sent upon successful
>>> + *   completion of this function.
>>> + *   Returns 0 on success, negative otherwise
>>> + */
>>> +int mmc_cmdq_halt(struct mmc_host *host, bool halt) {
>>> +     int err = 0;
>>> +
>>> +     if ((halt && (host->cmdq_ctx.curr_state & CMDQ_STATE_HALT)) ||
>>> +         (!halt && !(host->cmdq_ctx.curr_state & CMDQ_STATE_HALT)))
>>> +             return 1;
>>> +
>>> +     if (host->cmdq_ops->halt) {
>>> +             err = host->cmdq_ops->halt(host, halt);
>>> +             if (!err && halt)
>>> +                     host->cmdq_ctx.curr_state |= CMDQ_STATE_HALT;
>>> +             else if (!err && !halt)
>>> +                     host->cmdq_ctx.curr_state &= ~CMDQ_STATE_HALT;
>>
>>                  Since CMDQ_STATE_HALE == 0, ORed with CMDQ_STATE_HALT
>> and ANDed with ~CMDQ_STATE_HALT will neither modify the value of
>> curr_state. Thus CMDQ_STATE_HALT as 0 cannot indicate the halt state of cmd queue.
>
> Agree.
>
>>
>>
>>> +     }
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL(mmc_cmdq_halt);
>>
>>> +enum cmdq_states {
>>> +     CMDQ_STATE_HALT,
>>> +     CMDQ_STATE_ERR,
>>> +};
>>
>>      According to above definition, CMDQ_STATE_HALT is 0x0 and
>> CMDQ_STATE_ERR is 0x1.
>>
>
> I will address these comments in the next patch.
>
>>
>>
>> Hi Asutosh,
>>
>>      I think that the definition of CMDQ_STATE_HALT might fail to
>> indicate halt status correctly.
>>      Could you check my comments please?
>>      If I misunderstand your source code, please correct me.
>>
>> Best regards,
>> Ziji
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-arm-msm" in the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> Thanks & Regards
> ~/Asutosh Das
> N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^\x7f)Þº{.n\x7f+‰·¥Š{±šg"žØ^n‡r¡ö¦z\x7f\x1aëh™¨è­\x7f&¢ø\x1e®G«éh\x7f\x03(­éšŽŠÝ¢j"ú\x1a\x7f^[m\x7f\x7fïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmml==
>


-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.