RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

On Thu, Nov 10, 2011 Maya Erez wrote:
> S, Venkatraman <svenkatr@ti.com> wrote:
>> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
wrote:
>> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
>> request *req)

The function prepares the checkable list and not only checks if packing is
possible, therefore I think its name should change to reflect its real
action.

>> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
>> >> > +                       !card->ext_csd.packed_event_en)
>> >> > +               goto no_packed;

Having the condition with a && can lead to cases where CMD23 is not 
supported and we send packed commands. Therfore the condition should be
changed to || or be splitted to 2 separate checks.
Also, according to the standard the generic error flag in
PACKED_COMMAND_STATUS is set in case of any error and having
PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
the packed_event_en should be a mandatory condition for using packed
coammnds.

>> >> > +       if (mmc_req_rel_wr(cur) &&
>> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
>> >> > +                       !en_rel_wr) {
>> >> > +               goto no_packed;
>> >> > +       }

Can you please explain this condition and its purpose?

>> >> > +               phys_segments +=  next->nr_phys_segments;
>> >> > +               if (phys_segments > max_phys_segs) {
>> >> > +                       blk_requeue_request(q, next);
>> >> > +                       break;
>> >> > +               }
>> >> I mentioned this before - if the next request is not packable and
>> requeued,
>> >> blk_fetch_request will retrieve it again and this while loop will
never terminate.
>> >>
>> > If next request is not packable, it is requeued and 'break'
terminates
>> this loop.
>> > So it not infinite.
>> Right !! But that doesn't help finding the commands that are packable.
Ideally, you'd need to pack all neighbouring requests into one packed
command.
>> The way CFQ works, it is not necessary that the fetch would return all
outstanding
>> requests that are packable (unless you invoke a forced dispatch) It
would be good to see some numbers on the number of pack hits /
misses
>> that
>> you would encounter with this logic, on a typical usecase.
> Is it considered only for CFQ scheduler? How about other I/O scheduler?
If all requests are drained from scheduler queue forcedly,
> the number of candidate to be packed can be increased.
> However we may lose the unique CFQ's strength and MMC D/D may take the
CFQ's duty.
> Basically, this patch accommodates the origin order requests from I/O
scheduler.
>

In order to better utilize the packed commands feature and achieve the
best performance improvements I think that the command packing should be
done in the block layer, according to the scheduler policy.
That is, the scheduler should be aware of the capability of the device to
receive a request list and its constrains (such as maximum number of
requests, max number of sectors etc) and use this information as a  factor
to its algorithm.
This way you keep the decision making in the hands of the scheduler while
the MMC layer will only have to send this list as a packed command.

>> >> > +       if (rqc)
>> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);

It would be best to keep all the calls to blk_fetch_request in the same
location. Therefore, I suggest to move the call to mmc_blk_chk_packable to
mmc/card/queue.c after the first request is fetched.

>> >> >  cmd_abort:
>> >> > -       spin_lock_irq(&md->lock);
>> >> > -       while (ret)
>> >> > -               ret = __blk_end_request(req, -EIO,
>> blk_rq_cur_bytes(req));
>> >> > -       spin_unlock_irq(&md->lock);
>> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {

This should be the case for MMC_PACKED_NONE.

>> >> > +               spin_lock_irq(&md->lock);
>> >> > +               while (ret)
>> >> > +                       ret = __blk_end_request(req, -EIO,
>> blk_rq_cur_bytes(req));

Do we need the while or should it be an if? In other cases where
__blk_end_request is called there is no such while.

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

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> On Thu, Nov 10, 2011 Maya Erez wrote:
> > S, Venkatraman <svenkatr@ti.com> wrote:
> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
> wrote:
> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
> >> request *req)
> 
> The function prepares the checkable list and not only checks if packing is
> possible, therefore I think its name should change to reflect its real
> action
I labored at naming. Isn't it proper? :)
Do you have any recommendation?
group_pack_req?

> 
> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> >> >> > +                       !card->ext_csd.packed_event_en)
> >> >> > +               goto no_packed;
> 
> Having the condition with a && can lead to cases where CMD23 is not
> supported and we send packed commands. Therfore the condition should be
> changed to || or be splitted to 2 separate checks.
> Also, according to the standard the generic error flag in
> PACKED_COMMAND_STATUS is set in case of any error and having
> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
> the packed_event_en should be a mandatory condition for using packed
> coammnds.
... cases where CMD23 is not supported and we send packed commands?
Packed command must not be allowed in such a case.
It works only with predefined mode which is essential fator.
And spec doesn't mentioned PACKED_EVENT_EN must be set.
So Packed command can be sent regardless PACKED_EVENT_EN,
but it's not complete without reporting of error.
Then host driver may suffer error recovery.
Why packed command is used without error reporting?

> 
> >> >> > +       if (mmc_req_rel_wr(cur) &&
> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
> >> >> > +                       !en_rel_wr) {
> >> >> > +               goto no_packed;
> >> >> > +       }
> 
> Can you please explain this condition and its purpose?
> 
In the case where reliable write is request but enhanced reliable write
is not supported, write access must be partial according to
reliable write sector count. Because even a single request can be split,
packed command is not allowed in this case.

> >> >> > +               phys_segments +=  next->nr_phys_segments;
> >> >> > +               if (phys_segments > max_phys_segs) {
> >> >> > +                       blk_requeue_request(q, next);
> >> >> > +                       break;
> >> >> > +               }
> >> >> I mentioned this before - if the next request is not packable and
> >> requeued,
> >> >> blk_fetch_request will retrieve it again and this while loop will
> never terminate.
> >> >>
> >> > If next request is not packable, it is requeued and 'break'
> terminates
> >> this loop.
> >> > So it not infinite.
> >> Right !! But that doesn't help finding the commands that are packable.
> Ideally, you'd need to pack all neighbouring requests into one packed
> command.
> >> The way CFQ works, it is not necessary that the fetch would return all
> outstanding
> >> requests that are packable (unless you invoke a forced dispatch) It
> would be good to see some numbers on the number of pack hits /
> misses
> >> that
> >> you would encounter with this logic, on a typical usecase.
> > Is it considered only for CFQ scheduler? How about other I/O scheduler?
> If all requests are drained from scheduler queue forcedly,
> > the number of candidate to be packed can be increased.
> > However we may lose the unique CFQ's strength and MMC D/D may take the
> CFQ's duty.
> > Basically, this patch accommodates the origin order requests from I/O
> scheduler.
> >
> 
> In order to better utilize the packed commands feature and achieve the
> best performance improvements I think that the command packing should be
> done in the block layer, according to the scheduler policy.
> That is, the scheduler should be aware of the capability of the device to
> receive a request list and its constrains (such as maximum number of
> requests, max number of sectors etc) and use this information as a  factor
> to its algorithm.
> This way you keep the decision making in the hands of the scheduler while
> the MMC layer will only have to send this list as a packed command.
> 
Yes, it would be another interesting approach.
Command packing you mentioned means gathering request among same direction(read/write)?
Currently I/O scheduler may know device constrains which MMC driver informs
with the exception of order information for packed command.
But I think the dependency of I/O scheduler may be able to come up.
How can MMC layer treat packed command with I/O scheduler which doesn't support this?

> >> >> > +       if (rqc)
> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> 
> It would be best to keep all the calls to blk_fetch_request in the same
> location. Therefore, I suggest to move the call to mmc_blk_chk_packable to
> mmc/card/queue.c after the first request is fetched.

At the first time, I considered that way.
I'll do more, if possible.
> 
> >> >> >  cmd_abort:
> >> >> > -       spin_lock_irq(&md->lock);
> >> >> > -       while (ret)
> >> >> > -               ret = __blk_end_request(req, -EIO,
> >> blk_rq_cur_bytes(req));
> >> >> > -       spin_unlock_irq(&md->lock);
> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> 
> This should be the case for MMC_PACKED_NONE.
Right, I missed it.
> 
> >> >> > +               spin_lock_irq(&md->lock);
> >> >> > +               while (ret)
> >> >> > +                       ret = __blk_end_request(req, -EIO,
> >> blk_rq_cur_bytes(req));
> 
> Do we need the while or should it be an if? In other cases where
> __blk_end_request is called there is no such while.
This part is not only the new but also origin code which is moved in this patch.
Maybe...,'If' case is used  for a whole of remained bytes and
'while' case is used for partial report of remained bytes.

Thank you for review.

Best regards,
Seugwon Jeon.
> 
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[S, Venkatraman]

On Fri, Nov 11, 2011 at 12:56 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> Maya Erez wrote:
>> On Thu, Nov 10, 2011 Maya Erez wrote:
>> > S, Venkatraman <svenkatr@ti.com> wrote:
>> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
>> wrote:
>> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
>> >> request *req)
>>
>> The function prepares the checkable list and not only checks if packing is
>> possible, therefore I think its name should change to reflect its real
>> action
> I labored at naming. Isn't it proper? :)
> Do you have any recommendation?
> group_pack_req?
>
>>
>> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
>> >> >> > +                       !card->ext_csd.packed_event_en)
>> >> >> > +               goto no_packed;
>>
>> Having the condition with a && can lead to cases where CMD23 is not
>> supported and we send packed commands. Therfore the condition should be
>> changed to || or be splitted to 2 separate checks.
>> Also, according to the standard the generic error flag in
>> PACKED_COMMAND_STATUS is set in case of any error and having
>> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
>> the packed_event_en should be a mandatory condition for using packed
>> coammnds.
> ... cases where CMD23 is not supported and we send packed commands?
> Packed command must not be allowed in such a case.
> It works only with predefined mode which is essential fator.
> And spec doesn't mentioned PACKED_EVENT_EN must be set.
> So Packed command can be sent regardless PACKED_EVENT_EN,
> but it's not complete without reporting of error.
> Then host driver may suffer error recovery.
> Why packed command is used without error reporting?
>
>>
>> >> >> > +       if (mmc_req_rel_wr(cur) &&
>> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
>> >> >> > +                       !en_rel_wr) {
>> >> >> > +               goto no_packed;
>> >> >> > +       }
>>
>> Can you please explain this condition and its purpose?
>>
> In the case where reliable write is request but enhanced reliable write
> is not supported, write access must be partial according to
> reliable write sector count. Because even a single request can be split,
> packed command is not allowed in this case.
>
>> >> >> > +               phys_segments +=  next->nr_phys_segments;
>> >> >> > +               if (phys_segments > max_phys_segs) {
>> >> >> > +                       blk_requeue_request(q, next);
>> >> >> > +                       break;
>> >> >> > +               }
>> >> >> I mentioned this before - if the next request is not packable and
>> >> requeued,
>> >> >> blk_fetch_request will retrieve it again and this while loop will
>> never terminate.
>> >> >>
>> >> > If next request is not packable, it is requeued and 'break'
>> terminates
>> >> this loop.
>> >> > So it not infinite.
>> >> Right !! But that doesn't help finding the commands that are packable.
>> Ideally, you'd need to pack all neighbouring requests into one packed
>> command.
>> >> The way CFQ works, it is not necessary that the fetch would return all
>> outstanding
>> >> requests that are packable (unless you invoke a forced dispatch) It
>> would be good to see some numbers on the number of pack hits /
>> misses
>> >> that
>> >> you would encounter with this logic, on a typical usecase.
>> > Is it considered only for CFQ scheduler? How about other I/O scheduler?
>> If all requests are drained from scheduler queue forcedly,
>> > the number of candidate to be packed can be increased.
>> > However we may lose the unique CFQ's strength and MMC D/D may take the
>> CFQ's duty.
>> > Basically, this patch accommodates the origin order requests from I/O
>> scheduler.
>> >
>>
>> In order to better utilize the packed commands feature and achieve the
>> best performance improvements I think that the command packing should be
>> done in the block layer, according to the scheduler policy.
>> That is, the scheduler should be aware of the capability of the device to
>> receive a request list and its constrains (such as maximum number of
>> requests, max number of sectors etc) and use this information as a  factor
>> to its algorithm.
>> This way you keep the decision making in the hands of the scheduler while
>> the MMC layer will only have to send this list as a packed command.
>>
> Yes, it would be another interesting approach.
> Command packing you mentioned means gathering request among same direction(read/write)?
> Currently I/O scheduler may know device constrains which MMC driver informs
> with the exception of order information for packed command.
> But I think the dependency of I/O scheduler may be able to come up.
> How can MMC layer treat packed command with I/O scheduler which doesn't support this?

The very act of packing presumes some sorting and re-ordering at the
I/O scheduler level.
When no such sorting is done (ex. noop), MMC should resort to
non-packed execution, respecting the system configuration choice.

Looking deeper into this, I think a better approach would be to set
the prep_rq_fn of the request_queue, with a custom mmc function that
decides if the requests are packable or not, and return a
BLKPREP_DEFER for those that can't be packed.

>
>> >> >> > +       if (rqc)
>> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
>>
>> It would be best to keep all the calls to blk_fetch_request in the same
>> location. Therefore, I suggest to move the call to mmc_blk_chk_packable to
>> mmc/card/queue.c after the first request is fetched.
>
> At the first time, I considered that way.
> I'll do more, if possible.
>>
>> >> >> >  cmd_abort:
>> >> >> > -       spin_lock_irq(&md->lock);
>> >> >> > -       while (ret)
>> >> >> > -               ret = __blk_end_request(req, -EIO,
>> >> blk_rq_cur_bytes(req));
>> >> >> > -       spin_unlock_irq(&md->lock);
>> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>>
>> This should be the case for MMC_PACKED_NONE.
> Right, I missed it.
>>
>> >> >> > +               spin_lock_irq(&md->lock);
>> >> >> > +               while (ret)
>> >> >> > +                       ret = __blk_end_request(req, -EIO,
>> >> blk_rq_cur_bytes(req));
>>
>> Do we need the while or should it be an if? In other cases where
>> __blk_end_request is called there is no such while.
> This part is not only the new but also origin code which is moved in this patch.
> Maybe...,'If' case is used  for a whole of remained bytes and
> 'while' case is used for partial report of remained bytes.
>
> Thank you for review.
>
> Best regards,
> Seugwon Jeon.
>>
>> Thanks,
>> Maya Erez
>> Consultant for Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>>
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

> On Fri, Nov 11, 2011 at 12:56 PM, Seungwon Jeon <tgih.jun@samsung.com>
> wrote:
>> Maya Erez wrote:
>>> On Thu, Nov 10, 2011 Maya Erez wrote:
>>> > S, Venkatraman <svenkatr@ti.com> wrote:
>>> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
>>> wrote:
>>> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
>>> >> request *req)
>>>
>>> The function prepares the checkable list and not only checks if packing
>>> is
>>> possible, therefore I think its name should change to reflect its real
>>> action
>> I labored at naming. Isn't it proper? :)
>> Do you have any recommendation?
>> group_pack_req?
>>
>>>
>>> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
>>> >> >> > +                       !card->ext_csd.packed_event_en)
>>> >> >> > +               goto no_packed;
>>>
>>> Having the condition with a && can lead to cases where CMD23 is not
>>> supported and we send packed commands. Therfore the condition should be
>>> changed to || or be splitted to 2 separate checks.
>>> Also, according to the standard the generic error flag in
>>> PACKED_COMMAND_STATUS is set in case of any error and having
>>> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
>>> the packed_event_en should be a mandatory condition for using packed
>>> coammnds.
>> ... cases where CMD23 is not supported and we send packed commands?
>> Packed command must not be allowed in such a case.
>> It works only with predefined mode which is essential fator.
>> And spec doesn't mentioned PACKED_EVENT_EN must be set.
>> So Packed command can be sent regardless PACKED_EVENT_EN,
>> but it's not complete without reporting of error.
>> Then host driver may suffer error recovery.
>> Why packed command is used without error reporting?
Let me better explain my comment:
If the first condition (!(md->flags & MMC_BLK_CMD23) is 1 (meaning
MMC_BLK_CMD23 flag is not set), then in case card->ext_csd.packed_event_en
is 1 the second condition will be 0 and we won't "goto no_packed;". In
this case, CMD_23 is not supported but we don't exit the function.
If you want both conditions to be mandatory you need to use here an ||.
>>
>>>
>>> >> >> > +       if (mmc_req_rel_wr(cur) &&
>>> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
>>> >> >> > +                       !en_rel_wr) {
>>> >> >> > +               goto no_packed;
>>> >> >> > +       }
>>>
>>> Can you please explain this condition and its purpose?
>>>
>> In the case where reliable write is request but enhanced reliable write
>> is not supported, write access must be partial according to
>> reliable write sector count. Because even a single request can be split,
>> packed command is not allowed in this case.
>>
>>> >> >> > +               phys_segments +=  next->nr_phys_segments;
>>> >> >> > +               if (phys_segments > max_phys_segs) {
>>> >> >> > +                       blk_requeue_request(q, next);
>>> >> >> > +                       break;
>>> >> >> > +               }
>>> >> >> I mentioned this before - if the next request is not packable and
>>> >> requeued,
>>> >> >> blk_fetch_request will retrieve it again and this while loop will
>>> never terminate.
>>> >> >>
>>> >> > If next request is not packable, it is requeued and 'break'
>>> terminates
>>> >> this loop.
>>> >> > So it not infinite.
>>> >> Right !! But that doesn't help finding the commands that are
>>> packable.
>>> Ideally, you'd need to pack all neighbouring requests into one packed
>>> command.
>>> >> The way CFQ works, it is not necessary that the fetch would return
>>> all
>>> outstanding
>>> >> requests that are packable (unless you invoke a forced dispatch) It
>>> would be good to see some numbers on the number of pack hits /
>>> misses
>>> >> that
>>> >> you would encounter with this logic, on a typical usecase.
>>> > Is it considered only for CFQ scheduler? How about other I/O
>>> scheduler?
>>> If all requests are drained from scheduler queue forcedly,
>>> > the number of candidate to be packed can be increased.
>>> > However we may lose the unique CFQ's strength and MMC D/D may take
>>> the
>>> CFQ's duty.
>>> > Basically, this patch accommodates the origin order requests from I/O
>>> scheduler.
>>> >
>>>
>>> In order to better utilize the packed commands feature and achieve the
>>> best performance improvements I think that the command packing should
>>> be
>>> done in the block layer, according to the scheduler policy.
>>> That is, the scheduler should be aware of the capability of the device
>>> to
>>> receive a request list and its constrains (such as maximum number of
>>> requests, max number of sectors etc) and use this information as a
>>>  factor
>>> to its algorithm.
>>> This way you keep the decision making in the hands of the scheduler
>>> while
>>> the MMC layer will only have to send this list as a packed command.
>>>
>> Yes, it would be another interesting approach.
>> Command packing you mentioned means gathering request among same
>> direction(read/write)?
>> Currently I/O scheduler may know device constrains which MMC driver
>> informs
>> with the exception of order information for packed command.
>> But I think the dependency of I/O scheduler may be able to come up.
>> How can MMC layer treat packed command with I/O scheduler which doesn't
>> support this?
>
> The very act of packing presumes some sorting and re-ordering at the
> I/O scheduler level.
> When no such sorting is done (ex. noop), MMC should resort to
> non-packed execution, respecting the system configuration choice.
>
> Looking deeper into this, I think a better approach would be to set
> the prep_rq_fn of the request_queue, with a custom mmc function that
> decides if the requests are packable or not, and return a
> BLKPREP_DEFER for those that can't be packed.
>
>>
>>> >> >> > +       if (rqc)
>>> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
>>>
>>> It would be best to keep all the calls to blk_fetch_request in the same
>>> location. Therefore, I suggest to move the call to mmc_blk_chk_packable
>>> to
>>> mmc/card/queue.c after the first request is fetched.
>>
>> At the first time, I considered that way.
>> I'll do more, if possible.
>>>
>>> >> >> >  cmd_abort:
>>> >> >> > -       spin_lock_irq(&md->lock);
>>> >> >> > -       while (ret)
>>> >> >> > -               ret = __blk_end_request(req, -EIO,
>>> >> blk_rq_cur_bytes(req));
>>> >> >> > -       spin_unlock_irq(&md->lock);
>>> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>>>
>>> This should be the case for MMC_PACKED_NONE.
>> Right, I missed it.
>>>
>>> >> >> > +               spin_lock_irq(&md->lock);
>>> >> >> > +               while (ret)
>>> >> >> > +                       ret = __blk_end_request(req, -EIO,
>>> >> blk_rq_cur_bytes(req));
>>>
>>> Do we need the while or should it be an if? In other cases where
>>> __blk_end_request is called there is no such while.
>> This part is not only the new but also origin code which is moved in
>> this patch.
>> Maybe...,'If' case is used  for a whole of remained bytes and
>> 'while' case is used for partial report of remained bytes.
>>
>> Thank you for review.
>>
>> Best regards,
>> Seugwon Jeon.
>>>
>>> Thanks,
>>> Maya Erez
>>> Consultant for Qualcomm Innovation Center, Inc.
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>>>
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>
Thanks,
Maya Erez
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

>> On Fri, Nov 11, 2011 at 12:56 PM, Seungwon Jeon <tgih.jun@samsung.com>
>> wrote:
>>> Maya Erez wrote:
>>>> On Thu, Nov 10, 2011 Maya Erez wrote:
>>>> > S, Venkatraman <svenkatr@ti.com> wrote:
>>>> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon
>>>> <tgih.jun@samsung.com>
>>>> wrote:
>>>> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
>>>> >> request *req)
>>>>
>>>> The function prepares the checkable list and not only checks if
>>>> packing
>>>> is
>>>> possible, therefore I think its name should change to reflect its real
>>>> action
>>> I labored at naming. Isn't it proper? :)
>>> Do you have any recommendation?
>>> group_pack_req?
>>>
How about mmc_blk_prep_packed_list?

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

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

S, Venkatraman <svenkatr@ti.com> wrote:
> On Fri, Nov 11, 2011 at 12:56 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > Maya Erez wrote:
> >> On Thu, Nov 10, 2011 Maya Erez wrote:
> >> > S, Venkatraman <svenkatr@ti.com> wrote:
> >> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
> >> wrote:
> >> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
> >> >> request *req)
> >>
> >> The function prepares the checkable list and not only checks if packing is
> >> possible, therefore I think its name should change to reflect its real
> >> action
> > I labored at naming. Isn't it proper? :)
> > Do you have any recommendation?
> > group_pack_req?
> >
> >>
> >> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> >> >> >> > +                       !card->ext_csd.packed_event_en)
> >> >> >> > +               goto no_packed;
> >>
> >> Having the condition with a && can lead to cases where CMD23 is not
> >> supported and we send packed commands. Therfore the condition should be
> >> changed to || or be splitted to 2 separate checks.
> >> Also, according to the standard the generic error flag in
> >> PACKED_COMMAND_STATUS is set in case of any error and having
> >> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
> >> the packed_event_en should be a mandatory condition for using packed
> >> coammnds.
> > ... cases where CMD23 is not supported and we send packed commands?
> > Packed command must not be allowed in such a case.
> > It works only with predefined mode which is essential fator.
> > And spec doesn't mentioned PACKED_EVENT_EN must be set.
> > So Packed command can be sent regardless PACKED_EVENT_EN,
> > but it's not complete without reporting of error.
> > Then host driver may suffer error recovery.
> > Why packed command is used without error reporting?
> >
> >>
> >> >> >> > +       if (mmc_req_rel_wr(cur) &&
> >> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
> >> >> >> > +                       !en_rel_wr) {
> >> >> >> > +               goto no_packed;
> >> >> >> > +       }
> >>
> >> Can you please explain this condition and its purpose?
> >>
> > In the case where reliable write is request but enhanced reliable write
> > is not supported, write access must be partial according to
> > reliable write sector count. Because even a single request can be split,
> > packed command is not allowed in this case.
> >
> >> >> >> > +               phys_segments +=  next->nr_phys_segments;
> >> >> >> > +               if (phys_segments > max_phys_segs) {
> >> >> >> > +                       blk_requeue_request(q, next);
> >> >> >> > +                       break;
> >> >> >> > +               }
> >> >> >> I mentioned this before - if the next request is not packable and
> >> >> requeued,
> >> >> >> blk_fetch_request will retrieve it again and this while loop will
> >> never terminate.
> >> >> >>
> >> >> > If next request is not packable, it is requeued and 'break'
> >> terminates
> >> >> this loop.
> >> >> > So it not infinite.
> >> >> Right !! But that doesn't help finding the commands that are packable.
> >> Ideally, you'd need to pack all neighbouring requests into one packed
> >> command.
> >> >> The way CFQ works, it is not necessary that the fetch would return all
> >> outstanding
> >> >> requests that are packable (unless you invoke a forced dispatch) It
> >> would be good to see some numbers on the number of pack hits /
> >> misses
> >> >> that
> >> >> you would encounter with this logic, on a typical usecase.
> >> > Is it considered only for CFQ scheduler? How about other I/O scheduler?
> >> If all requests are drained from scheduler queue forcedly,
> >> > the number of candidate to be packed can be increased.
> >> > However we may lose the unique CFQ's strength and MMC D/D may take the
> >> CFQ's duty.
> >> > Basically, this patch accommodates the origin order requests from I/O
> >> scheduler.
> >> >
> >>
> >> In order to better utilize the packed commands feature and achieve the
> >> best performance improvements I think that the command packing should be
> >> done in the block layer, according to the scheduler policy.
> >> That is, the scheduler should be aware of the capability of the device to
> >> receive a request list and its constrains (such as maximum number of
> >> requests, max number of sectors etc) and use this information as a  factor
> >> to its algorithm.
> >> This way you keep the decision making in the hands of the scheduler while
> >> the MMC layer will only have to send this list as a packed command.
> >>
> > Yes, it would be another interesting approach.
> > Command packing you mentioned means gathering request among same direction(read/write)?
> > Currently I/O scheduler may know device constrains which MMC driver informs
> > with the exception of order information for packed command.
> > But I think the dependency of I/O scheduler may be able to come up.
> > How can MMC layer treat packed command with I/O scheduler which doesn't support this?
> 
> The very act of packing presumes some sorting and re-ordering at the
> I/O scheduler level.
> When no such sorting is done (ex. noop), MMC should resort to
> non-packed execution, respecting the system configuration choice.
> 
> Looking deeper into this, I think a better approach would be to set
> the prep_rq_fn of the request_queue, with a custom mmc function that
> decides if the requests are packable or not, and return a
> BLKPREP_DEFER for those that can't be packed.

MMC layer anticipates the favorable requests for packing from I/O scheduler.
Obviously request order from I/O scheduler might be poor for packing and requests can't be packed.
But that doesn't mean mmc layer need to wait a better pack-case.
BLKPREP_DEFER may give rise to I/O latency. Top of request will be deferred next time. 
If request can't be packed, it'd rather be sent at once than delayed
by returning a BLKPREP_DEFER for better responsiveness.

Thanks.
Seungwon Jeon.
> 
> >
> >> >> >> > +       if (rqc)
> >> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> >>
> >> It would be best to keep all the calls to blk_fetch_request in the same
> >> location. Therefore, I suggest to move the call to mmc_blk_chk_packable to
> >> mmc/card/queue.c after the first request is fetched.
> >
> > At the first time, I considered that way.
> > I'll do more, if possible.
> >>
> >> >> >> >  cmd_abort:
> >> >> >> > -       spin_lock_irq(&md->lock);
> >> >> >> > -       while (ret)
> >> >> >> > -               ret = __blk_end_request(req, -EIO,
> >> >> blk_rq_cur_bytes(req));
> >> >> >> > -       spin_unlock_irq(&md->lock);
> >> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> >>
> >> This should be the case for MMC_PACKED_NONE.
> > Right, I missed it.
> >>
> >> >> >> > +               spin_lock_irq(&md->lock);
> >> >> >> > +               while (ret)
> >> >> >> > +                       ret = __blk_end_request(req, -EIO,
> >> >> blk_rq_cur_bytes(req));
> >>
> >> Do we need the while or should it be an if? In other cases where
> >> __blk_end_request is called there is no such while.
> > This part is not only the new but also origin code which is moved in this patch.
> > Maybe...,'If' case is used  for a whole of remained bytes and
> > 'while' case is used for partial report of remained bytes.
> >
> > Thank you for review.
> >
> > Best regards,
> > Seugwon Jeon.
> >>
> >> Thanks,
> >> Maya Erez
> >> Consultant for Qualcomm Innovation Center, Inc.
> >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> >>
> >>
> >>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> > On Fri, Nov 11, 2011 at 12:56 PM, Seungwon Jeon <tgih.jun@samsung.com>
> > wrote:
> >> Maya Erez wrote:
> >>> On Thu, Nov 10, 2011 Maya Erez wrote:
> >>> > S, Venkatraman <svenkatr@ti.com> wrote:
> >>> >> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com>
> >>> wrote:
> >>> >> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct
> >>> >> request *req)
> >>>
> >>> The function prepares the checkable list and not only checks if packing
> >>> is
> >>> possible, therefore I think its name should change to reflect its real
> >>> action
> >> I labored at naming. Isn't it proper? :)
> >> Do you have any recommendation?
> >> group_pack_req?
> >>
> >>>
> >>> >> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> >>> >> >> > +                       !card->ext_csd.packed_event_en)
> >>> >> >> > +               goto no_packed;
> >>>
> >>> Having the condition with a && can lead to cases where CMD23 is not
> >>> supported and we send packed commands. Therfore the condition should be
> >>> changed to || or be splitted to 2 separate checks.
> >>> Also, according to the standard the generic error flag in
> >>> PACKED_COMMAND_STATUS is set in case of any error and having
> >>> PACKED_EVENT_EN is only optional. Therefore, I don't think that setting
> >>> the packed_event_en should be a mandatory condition for using packed
> >>> coammnds.
> >> ... cases where CMD23 is not supported and we send packed commands?
> >> Packed command must not be allowed in such a case.
> >> It works only with predefined mode which is essential fator.
> >> And spec doesn't mentioned PACKED_EVENT_EN must be set.
> >> So Packed command can be sent regardless PACKED_EVENT_EN,
> >> but it's not complete without reporting of error.
> >> Then host driver may suffer error recovery.
> >> Why packed command is used without error reporting?
> Let me better explain my comment:
> If the first condition (!(md->flags & MMC_BLK_CMD23) is 1 (meaning
> MMC_BLK_CMD23 flag is not set), then in case card->ext_csd.packed_event_en
> is 1 the second condition will be 0 and we won't "goto no_packed;". In
> this case, CMD_23 is not supported but we don't exit the function.
> If you want both conditions to be mandatory you need to use here an ||.
Thank you for clearing comment.
This condition will be fixed.

> >>
> >>>
> >>> >> >> > +       if (mmc_req_rel_wr(cur) &&
> >>> >> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
> >>> >> >> > +                       !en_rel_wr) {
> >>> >> >> > +               goto no_packed;
> >>> >> >> > +       }
> >>>
> >>> Can you please explain this condition and its purpose?
> >>>
> >> In the case where reliable write is request but enhanced reliable write
> >> is not supported, write access must be partial according to
> >> reliable write sector count. Because even a single request can be split,
> >> packed command is not allowed in this case.
> >>
> >>> >> >> > +               phys_segments +=  next->nr_phys_segments;
> >>> >> >> > +               if (phys_segments > max_phys_segs) {
> >>> >> >> > +                       blk_requeue_request(q, next);
> >>> >> >> > +                       break;
> >>> >> >> > +               }
> >>> >> >> I mentioned this before - if the next request is not packable and
> >>> >> requeued,
> >>> >> >> blk_fetch_request will retrieve it again and this while loop will
> >>> never terminate.
> >>> >> >>
> >>> >> > If next request is not packable, it is requeued and 'break'
> >>> terminates
> >>> >> this loop.
> >>> >> > So it not infinite.
> >>> >> Right !! But that doesn't help finding the commands that are
> >>> packable.
> >>> Ideally, you'd need to pack all neighbouring requests into one packed
> >>> command.
> >>> >> The way CFQ works, it is not necessary that the fetch would return
> >>> all
> >>> outstanding
> >>> >> requests that are packable (unless you invoke a forced dispatch) It
> >>> would be good to see some numbers on the number of pack hits /
> >>> misses
> >>> >> that
> >>> >> you would encounter with this logic, on a typical usecase.
> >>> > Is it considered only for CFQ scheduler? How about other I/O
> >>> scheduler?
> >>> If all requests are drained from scheduler queue forcedly,
> >>> > the number of candidate to be packed can be increased.
> >>> > However we may lose the unique CFQ's strength and MMC D/D may take
> >>> the
> >>> CFQ's duty.
> >>> > Basically, this patch accommodates the origin order requests from I/O
> >>> scheduler.
> >>> >
> >>>
> >>> In order to better utilize the packed commands feature and achieve the
> >>> best performance improvements I think that the command packing should
> >>> be
> >>> done in the block layer, according to the scheduler policy.
> >>> That is, the scheduler should be aware of the capability of the device
> >>> to
> >>> receive a request list and its constrains (such as maximum number of
> >>> requests, max number of sectors etc) and use this information as a
> >>>  factor
> >>> to its algorithm.
> >>> This way you keep the decision making in the hands of the scheduler
> >>> while
> >>> the MMC layer will only have to send this list as a packed command.
> >>>
> >> Yes, it would be another interesting approach.
> >> Command packing you mentioned means gathering request among same
> >> direction(read/write)?
> >> Currently I/O scheduler may know device constrains which MMC driver
> >> informs
> >> with the exception of order information for packed command.
> >> But I think the dependency of I/O scheduler may be able to come up.
> >> How can MMC layer treat packed command with I/O scheduler which doesn't
> >> support this?
> >
> > The very act of packing presumes some sorting and re-ordering at the
> > I/O scheduler level.
> > When no such sorting is done (ex. noop), MMC should resort to
> > non-packed execution, respecting the system configuration choice.
> >
> > Looking deeper into this, I think a better approach would be to set
> > the prep_rq_fn of the request_queue, with a custom mmc function that
> > decides if the requests are packable or not, and return a
> > BLKPREP_DEFER for those that can't be packed.
> >
> >>
> >>> >> >> > +       if (rqc)
> >>> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> >>>
> >>> It would be best to keep all the calls to blk_fetch_request in the same
> >>> location. Therefore, I suggest to move the call to mmc_blk_chk_packable
> >>> to
> >>> mmc/card/queue.c after the first request is fetched.
> >>
> >> At the first time, I considered that way.
> >> I'll do more, if possible.
I considered more.
I think that mmc_blk_chk_packable would rather be called only for r/w type
than all request type(e.g. discard, flush).

> >>>
> >>> >> >> >  cmd_abort:
> >>> >> >> > -       spin_lock_irq(&md->lock);
> >>> >> >> > -       while (ret)
> >>> >> >> > -               ret = __blk_end_request(req, -EIO,
> >>> >> blk_rq_cur_bytes(req));
> >>> >> >> > -       spin_unlock_irq(&md->lock);
> >>> >> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> >>>
> >>> This should be the case for MMC_PACKED_NONE.
> >> Right, I missed it.
> >>>
> >>> >> >> > +               spin_lock_irq(&md->lock);
> >>> >> >> > +               while (ret)
> >>> >> >> > +                       ret = __blk_end_request(req, -EIO,
> >>> >> blk_rq_cur_bytes(req));
> >>>
> >>> Do we need the while or should it be an if? In other cases where
> >>> __blk_end_request is called there is no such while.
> >> This part is not only the new but also origin code which is moved in
> >> this patch.
> >> Maybe...,'If' case is used  for a whole of remained bytes and
> >> 'while' case is used for partial report of remained bytes.
> >>
> >> Thank you for review.
> >>
> >> Best regards,
> >> Seugwon Jeon.
> >>>
> >>> Thanks,
> >>> Maya Erez
> >>> Consultant for Qualcomm Innovation Center, Inc.
> >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >>
> >
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

>> >>> >> >> > +       if (rqc)
>> >>> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
>> >>>
>> >>> It would be best to keep all the calls to blk_fetch_request in the
>> same
>> >>> location. Therefore, I suggest to move the call to
>> mmc_blk_chk_packable
>> >>> to
>> >>> mmc/card/queue.c after the first request is fetched.
>> >>
>> >> At the first time, I considered that way.
>> >> I'll do more, if possible.
> I considered more.
> I think that mmc_blk_chk_packable would rather be called only for r/w type
> than all request type(e.g. discard, flush).
>
mmc_blk_chk_packable can check the cmd_flags of the request to verify it's
not a flush/disacrad etc. In such cases will not pack.

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

>> >> >> >> > +               phys_segments +=  next->nr_phys_segments;
>> >> >> >> > +               if (phys_segments > max_phys_segs) {
>> >> >> >> > +                       blk_requeue_request(q, next);
>> >> >> >> > +                       break;
>> >> >> >> > +               }
>> >> >> >> I mentioned this before - if the next request is not packable
>> and
>> >> >> requeued,
>> >> >> >> blk_fetch_request will retrieve it again and this while loop
>> will
>> >> never terminate.
>> >> >> >>
>> >> >> > If next request is not packable, it is requeued and 'break'
>> >> terminates
>> >> >> this loop.
>> >> >> > So it not infinite.
>> >> >> Right !! But that doesn't help finding the commands that are
>> packable.
>> >> Ideally, you'd need to pack all neighbouring requests into one packed
>> >> command.
>> >> >> The way CFQ works, it is not necessary that the fetch would return
>> all
>> >> outstanding
>> >> >> requests that are packable (unless you invoke a forced dispatch)
>> It
>> >> would be good to see some numbers on the number of pack hits /
>> >> misses
>> >> >> that
>> >> >> you would encounter with this logic, on a typical usecase.
>> >> > Is it considered only for CFQ scheduler? How about other I/O
>> scheduler?
>> >> If all requests are drained from scheduler queue forcedly,
>> >> > the number of candidate to be packed can be increased.
>> >> > However we may lose the unique CFQ's strength and MMC D/D may take
>> the
>> >> CFQ's duty.
>> >> > Basically, this patch accommodates the origin order requests from
>> I/O
>> >> scheduler.
>> >> >
>> >>
>> >> In order to better utilize the packed commands feature and achieve
>> the
>> >> best performance improvements I think that the command packing should
>> be
>> >> done in the block layer, according to the scheduler policy.
>> >> That is, the scheduler should be aware of the capability of the
>> device to
>> >> receive a request list and its constrains (such as maximum number of
>> >> requests, max number of sectors etc) and use this information as a
>>  factor
>> >> to its algorithm.
>> >> This way you keep the decision making in the hands of the scheduler
>> while
>> >> the MMC layer will only have to send this list as a packed command.
>> >>
>> > Yes, it would be another interesting approach.
>> > Command packing you mentioned means gathering request among same
>> direction(read/write)?
>> > Currently I/O scheduler may know device constrains which MMC driver
>> informs
>> > with the exception of order information for packed command.
>> > But I think the dependency of I/O scheduler may be able to come up.
>> > How can MMC layer treat packed command with I/O scheduler which
>> doesn't support this?
>>
>> The very act of packing presumes some sorting and re-ordering at the
>> I/O scheduler level.
>> When no such sorting is done (ex. noop), MMC should resort to
>> non-packed execution, respecting the system configuration choice.
>>
>> Looking deeper into this, I think a better approach would be to set
>> the prep_rq_fn of the request_queue, with a custom mmc function that
>> decides if the requests are packable or not, and return a
>> BLKPREP_DEFER for those that can't be packed.
>
> MMC layer anticipates the favorable requests for packing from I/O
> scheduler.
> Obviously request order from I/O scheduler might be poor for packing and
> requests can't be packed.
> But that doesn't mean mmc layer need to wait a better pack-case.
> BLKPREP_DEFER may give rise to I/O latency. Top of request will be
> deferred next time.
> If request can't be packed, it'd rather be sent at once than delayed
> by returning a BLKPREP_DEFER for better responsiveness.
>
> Thanks.
> Seungwon Jeon.
The decision whether a request is packable or not should not be made per
request, therefore I don't see the need for using req_prep_fn. The MMC
layer can peek each request and decide if to pack it or not when preparing
the packed list (as suggested in this patch). The scheduler changes will
improve the probability of getting a list that can be packed.
My suggestion for the scheduler change is:
The block layer will be notified of the packing limits via new queue
limits (in blk-settings). We can make it generic to allow all kinds of
requests lists. Example for the new queue limits can be:
Is_reqs_list_supported
Max_num_of_read_reqs_in_list
Max_num_of_write_reqs_in_list
max_blk_cnt_in_list
Is_list_interleaved (optional - to support read and write requests to be
linked together, not the case for eMMC4.5)
The scheduler, that will have all the required info in the queue limits,
will be able to decide which requests can be in the same list and move all
of them to the dispatch queue (in one elevator_dispatch_fn call).

I see 2 options for preparing the list:

Option 1. blk_fetch_request will prepare the list and return a list of
requests (will require a change in struct request to include the list but
can be more generic).

Option 2. The MMC layer will prepare the list. After fetching one request
the MMC layer can call blk_peek_request and check if the next request is
packable or not. By keeping all the calls to blk_peek_request under the
same queue lock we can guarantee to get the requests that the scheduler
pushed to the dispatch queue (this requires mmc_blk_chk_packable to move
to block.c). If the request is packable the MMC layer will call
blk_start_request to dequeue the request. This way there is no need to
re-queue the requests that cannot be packed.

Going back to this patch - the usage of blk_peek_request+blk_start_request
instead of blk_fetch_request can be done in mmc_blk_chk_packable in order
to eliminate the need to requeue commands and loose performance.

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

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> >> >>> >> >> > +       if (rqc)
> >> >>> >> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> >> >>>
> >> >>> It would be best to keep all the calls to blk_fetch_request in the
> >> same
> >> >>> location. Therefore, I suggest to move the call to
> >> mmc_blk_chk_packable
> >> >>> to
> >> >>> mmc/card/queue.c after the first request is fetched.
> >> >>
> >> >> At the first time, I considered that way.
> >> >> I'll do more, if possible.
> > I considered more.
> > I think that mmc_blk_chk_packable would rather be called only for r/w type
> > than all request type(e.g. discard, flush).
> >
> mmc_blk_chk_packable can check the cmd_flags of the request to verify it's
> not a flush/disacrad etc. In such cases will not pack.
Yes. It must be checked, but omitted.
I already have added this check. It will be applied next.

Thanks,
Seungwon Jeon.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> >> >> >> >> > +               phys_segments +=  next->nr_phys_segments;
> >> >> >> >> > +               if (phys_segments > max_phys_segs) {
> >> >> >> >> > +                       blk_requeue_request(q, next);
> >> >> >> >> > +                       break;
> >> >> >> >> > +               }
> >> >> >> >> I mentioned this before - if the next request is not packable
> >> and
> >> >> >> requeued,
> >> >> >> >> blk_fetch_request will retrieve it again and this while loop
> >> will
> >> >> never terminate.
> >> >> >> >>
> >> >> >> > If next request is not packable, it is requeued and 'break'
> >> >> terminates
> >> >> >> this loop.
> >> >> >> > So it not infinite.
> >> >> >> Right !! But that doesn't help finding the commands that are
> >> packable.
> >> >> Ideally, you'd need to pack all neighbouring requests into one packed
> >> >> command.
> >> >> >> The way CFQ works, it is not necessary that the fetch would return
> >> all
> >> >> outstanding
> >> >> >> requests that are packable (unless you invoke a forced dispatch)
> >> It
> >> >> would be good to see some numbers on the number of pack hits /
> >> >> misses
> >> >> >> that
> >> >> >> you would encounter with this logic, on a typical usecase.
> >> >> > Is it considered only for CFQ scheduler? How about other I/O
> >> scheduler?
> >> >> If all requests are drained from scheduler queue forcedly,
> >> >> > the number of candidate to be packed can be increased.
> >> >> > However we may lose the unique CFQ's strength and MMC D/D may take
> >> the
> >> >> CFQ's duty.
> >> >> > Basically, this patch accommodates the origin order requests from
> >> I/O
> >> >> scheduler.
> >> >> >
> >> >>
> >> >> In order to better utilize the packed commands feature and achieve
> >> the
> >> >> best performance improvements I think that the command packing should
> >> be
> >> >> done in the block layer, according to the scheduler policy.
> >> >> That is, the scheduler should be aware of the capability of the
> >> device to
> >> >> receive a request list and its constrains (such as maximum number of
> >> >> requests, max number of sectors etc) and use this information as a
> >>  factor
> >> >> to its algorithm.
> >> >> This way you keep the decision making in the hands of the scheduler
> >> while
> >> >> the MMC layer will only have to send this list as a packed command.
> >> >>
> >> > Yes, it would be another interesting approach.
> >> > Command packing you mentioned means gathering request among same
> >> direction(read/write)?
> >> > Currently I/O scheduler may know device constrains which MMC driver
> >> informs
> >> > with the exception of order information for packed command.
> >> > But I think the dependency of I/O scheduler may be able to come up.
> >> > How can MMC layer treat packed command with I/O scheduler which
> >> doesn't support this?
> >>
> >> The very act of packing presumes some sorting and re-ordering at the
> >> I/O scheduler level.
> >> When no such sorting is done (ex. noop), MMC should resort to
> >> non-packed execution, respecting the system configuration choice.
> >>
> >> Looking deeper into this, I think a better approach would be to set
> >> the prep_rq_fn of the request_queue, with a custom mmc function that
> >> decides if the requests are packable or not, and return a
> >> BLKPREP_DEFER for those that can't be packed.
> >
> > MMC layer anticipates the favorable requests for packing from I/O
> > scheduler.
> > Obviously request order from I/O scheduler might be poor for packing and
> > requests can't be packed.
> > But that doesn't mean mmc layer need to wait a better pack-case.
> > BLKPREP_DEFER may give rise to I/O latency. Top of request will be
> > deferred next time.
> > If request can't be packed, it'd rather be sent at once than delayed
> > by returning a BLKPREP_DEFER for better responsiveness.
> >
> > Thanks.
> > Seungwon Jeon.
> The decision whether a request is packable or not should not be made per
> request, therefore I don't see the need for using req_prep_fn. The MMC
> layer can peek each request and decide if to pack it or not when preparing
> the packed list (as suggested in this patch). The scheduler changes will
> improve the probability of getting a list that can be packed.
> My suggestion for the scheduler change is:
> The block layer will be notified of the packing limits via new queue
> limits (in blk-settings). We can make it generic to allow all kinds of
> requests lists. Example for the new queue limits can be:
> Is_reqs_list_supported
> Max_num_of_read_reqs_in_list
> Max_num_of_write_reqs_in_list
> max_blk_cnt_in_list
> Is_list_interleaved (optional - to support read and write requests to be
> linked together, not the case for eMMC4.5)
> The scheduler, that will have all the required info in the queue limits,
> will be able to decide which requests can be in the same list and move all
> of them to the dispatch queue (in one elevator_dispatch_fn call).

If probability of packing gets higher, it would be nice.
We need to think more.
> 
> I see 2 options for preparing the list:
> 
> Option 1. blk_fetch_request will prepare the list and return a list of
> requests (will require a change in struct request to include the list but
> can be more generic).
> 
> Option 2. The MMC layer will prepare the list. After fetching one request
> the MMC layer can call blk_peek_request and check if the next request is
> packable or not. By keeping all the calls to blk_peek_request under the
> same queue lock we can guarantee to get the requests that the scheduler
> pushed to the dispatch queue (this requires mmc_blk_chk_packable to move
> to block.c). If the request is packable the MMC layer will call
> blk_start_request to dequeue the request. This way there is no need to
> re-queue the requests that cannot be packed.
> 
> Going back to this patch - the usage of blk_peek_request+blk_start_request
> instead of blk_fetch_request can be done in mmc_blk_chk_packable in order
> to eliminate the need to requeue commands and loose performance.

Do you think blk_requeue_request() is heavy work?
Currently queue_lock was missed using blk_fetch_request. It will be fixed.
Anyway, if we use a set of blk_peek_request+blk_start_request,
there is no necessity for requeuing the request.
But during preparing several request for the list, queue_lock will be held in mmc layer.
Then queue_lock time of mmc layer might be more increased than before.

Thank you for suggestion.
Seungwon Jeon.
> 
> Thanks,
> Maya Erez
> Consultant for Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

> Maya Erez wrote:
>> >> >> >> >> > +               phys_segments +=  next->nr_phys_segments;
>> >> >> >> >> > +               if (phys_segments > max_phys_segs) {
>> >> >> >> >> > +                       blk_requeue_request(q, next);
>> >> >> >> >> > +                       break;
>> >> >> >> >> > +               }
>> >> >> >> >> I mentioned this before - if the next request is not
>> packable
>> >> and
>> >> >> >> requeued,
>> >> >> >> >> blk_fetch_request will retrieve it again and this while loop
>> >> will
>> >> >> never terminate.
>> >> >> >> >>
>> >> >> >> > If next request is not packable, it is requeued and 'break'
>> >> >> terminates
>> >> >> >> this loop.
>> >> >> >> > So it not infinite.
>> >> >> >> Right !! But that doesn't help finding the commands that are
>> >> packable.
>> >> >> Ideally, you'd need to pack all neighbouring requests into one
>> packed
>> >> >> command.
>> >> >> >> The way CFQ works, it is not necessary that the fetch would
>> return
>> >> all
>> >> >> outstanding
>> >> >> >> requests that are packable (unless you invoke a forced
>> dispatch)
>> >> It
>> >> >> would be good to see some numbers on the number of pack hits /
>> >> >> misses
>> >> >> >> that
>> >> >> >> you would encounter with this logic, on a typical usecase.
>> >> >> > Is it considered only for CFQ scheduler? How about other I/O
>> >> scheduler?
>> >> >> If all requests are drained from scheduler queue forcedly,
>> >> >> > the number of candidate to be packed can be increased.
>> >> >> > However we may lose the unique CFQ's strength and MMC D/D may
>> take
>> >> the
>> >> >> CFQ's duty.
>> >> >> > Basically, this patch accommodates the origin order requests
>> from
>> >> I/O
>> >> >> scheduler.
>> >> >> >
>> >> >>
>> >> >> In order to better utilize the packed commands feature and achieve
>> >> the
>> >> >> best performance improvements I think that the command packing
>> should
>> >> be
>> >> >> done in the block layer, according to the scheduler policy.
>> >> >> That is, the scheduler should be aware of the capability of the
>> >> device to
>> >> >> receive a request list and its constrains (such as maximum number
>> of
>> >> >> requests, max number of sectors etc) and use this information as a
>> >>  factor
>> >> >> to its algorithm.
>> >> >> This way you keep the decision making in the hands of the
>> scheduler
>> >> while
>> >> >> the MMC layer will only have to send this list as a packed
>> command.
>> >> >>
>> >> > Yes, it would be another interesting approach.
>> >> > Command packing you mentioned means gathering request among same
>> >> direction(read/write)?
>> >> > Currently I/O scheduler may know device constrains which MMC driver
>> >> informs
>> >> > with the exception of order information for packed command.
>> >> > But I think the dependency of I/O scheduler may be able to come up.
>> >> > How can MMC layer treat packed command with I/O scheduler which
>> >> doesn't support this?
>> >>
>> >> The very act of packing presumes some sorting and re-ordering at the
>> >> I/O scheduler level.
>> >> When no such sorting is done (ex. noop), MMC should resort to
>> >> non-packed execution, respecting the system configuration choice.
>> >>
>> >> Looking deeper into this, I think a better approach would be to set
>> >> the prep_rq_fn of the request_queue, with a custom mmc function that
>> >> decides if the requests are packable or not, and return a
>> >> BLKPREP_DEFER for those that can't be packed.
>> >
>> > MMC layer anticipates the favorable requests for packing from I/O
>> > scheduler.
>> > Obviously request order from I/O scheduler might be poor for packing
>> and
>> > requests can't be packed.
>> > But that doesn't mean mmc layer need to wait a better pack-case.
>> > BLKPREP_DEFER may give rise to I/O latency. Top of request will be
>> > deferred next time.
>> > If request can't be packed, it'd rather be sent at once than delayed
>> > by returning a BLKPREP_DEFER for better responsiveness.
>> >
>> > Thanks.
>> > Seungwon Jeon.
>> The decision whether a request is packable or not should not be made per
>> request, therefore I don't see the need for using req_prep_fn. The MMC
>> layer can peek each request and decide if to pack it or not when
>> preparing
>> the packed list (as suggested in this patch). The scheduler changes will
>> improve the probability of getting a list that can be packed.
>> My suggestion for the scheduler change is:
>> The block layer will be notified of the packing limits via new queue
>> limits (in blk-settings). We can make it generic to allow all kinds of
>> requests lists. Example for the new queue limits can be:
>> Is_reqs_list_supported
>> Max_num_of_read_reqs_in_list
>> Max_num_of_write_reqs_in_list
>> max_blk_cnt_in_list
>> Is_list_interleaved (optional - to support read and write requests to be
>> linked together, not the case for eMMC4.5)
>> The scheduler, that will have all the required info in the queue limits,
>> will be able to decide which requests can be in the same list and move
>> all
>> of them to the dispatch queue (in one elevator_dispatch_fn call).
>
> If probability of packing gets higher, it would be nice.
> We need to think more.
>>
>> I see 2 options for preparing the list:
>>
>> Option 1. blk_fetch_request will prepare the list and return a list of
>> requests (will require a change in struct request to include the list
>> but
>> can be more generic).
>>
>> Option 2. The MMC layer will prepare the list. After fetching one
>> request
>> the MMC layer can call blk_peek_request and check if the next request is
>> packable or not. By keeping all the calls to blk_peek_request under the
>> same queue lock we can guarantee to get the requests that the scheduler
>> pushed to the dispatch queue (this requires mmc_blk_chk_packable to move
>> to block.c). If the request is packable the MMC layer will call
>> blk_start_request to dequeue the request. This way there is no need to
>> re-queue the requests that cannot be packed.
>>
>> Going back to this patch - the usage of
>> blk_peek_request+blk_start_request
>> instead of blk_fetch_request can be done in mmc_blk_chk_packable in
>> order
>> to eliminate the need to requeue commands and loose performance.
>
> Do you think blk_requeue_request() is heavy work?
> Currently queue_lock was missed using blk_fetch_request. It will be fixed.
> Anyway, if we use a set of blk_peek_request+blk_start_request,
> there is no necessity for requeuing the request.
> But during preparing several request for the list, queue_lock will be held
> in mmc layer.
> Then queue_lock time of mmc layer might be more increased than before.
>
> Thank you for suggestion.
> Seungwon Jeon.
>>
Yes, I think that if we can avoid redundant dequeue and requeue it would
be better. Please note that the re-queue also requires locking the
queue_lock.
I agree that in case of preparing the packed list the queue_lock will be
taken for a longer time but I don't see any way to avoid it.

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

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> > Maya Erez wrote:
> >> >> >> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon
> >> <tgih.jun@samsung.com>
> >> >> wrote:
> >> >> >> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct
> >> mmc_card
> >> >> *card,
> >> >> >> >        if (!brq->data.bytes_xfered)
> >> >> >> >                return MMC_BLK_RETRY;
> >> >> >> >
> >> >> >> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> >> >> >> > +               if (unlikely(brq->data.blocks << 9 !=
> >> >> brq->data.bytes_xfered))
> >> >> >> > +                       return MMC_BLK_PARTIAL;
> >> >> >> > +               else
> >> >> >> > +                       return MMC_BLK_SUCCESS;
> >> >> >> > +       }
> >> >> >> > +
> >> >> >> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
> >> >> >> >                return MMC_BLK_PARTIAL;
> >> >> >> >
> >> >> >> >        return MMC_BLK_SUCCESS;
> >> >> >> >  }
> >> >> >> >
> >> >> >> > +static int mmc_blk_packed_err_check(struct mmc_card *card, +
> >>                        struct mmc_async_req *areq)
> >> >> >> > +{
> >> >> >> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct
> >> >> mmc_queue_req,
> >> >> >> > +                                                   mmc_active);
> >> +
> >>       int err, check, status;
> >> >> >> > +       u8 ext_csd[512];
> >> >> >> > +
> >> >> >> > +       check = mmc_blk_err_check(card, areq);
> >> >> >> > +
> >> >> >> > +       if (check == MMC_BLK_SUCCESS)
> >> >> >> > +               return check;
> >> I think we need to check the status for all cases and not only in case
> >> of
> >> MMC_BLK_PARTIAL. For example, in cases where the header was traferred
> >> successfully but had logic errors (wrong number of sectors etc.)
> >> mmc_blk_err_check will return MMC_BLK_SUCCESS although the packed
> >> commands
> >> failed.
> > Similarly, Sahitya Tummala is already mentioned this.
> > Other error case will be checked in next version.
> > The case you suggested is about read or write?
> > Device may detect error and stop transferring the data.
> Sahitya suggested to also check other error cases that mmc_blk_err_check
> returns (such as MMC_BLK_CMD_ERR, MMC_BLK_ECC_ERR and MMC_BLK_DATA_ERR).
> I suggest to also check the exception bit in the status even if
> mmc_blk_err_check returned success, since mmc_blk_err_check might not
> catch all the packed commands failures. One example for such a failure is
> when the header of read packed commands will have logical error.
This part is modified in next version.

Thanks,
Seungwon Jeon.

> Thanks,
> Maya
> --
> Seny by a Consultant for Qualcomm innovation center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

> Maya Erez wrote:
>> >> >> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon
>> <tgih.jun@samsung.com>
>> >> wrote:
>> >> >> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct
>> mmc_card
>> >> *card,
>> >> >> >        if (!brq->data.bytes_xfered)
>> >> >> >                return MMC_BLK_RETRY;
>> >> >> >
>> >> >> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
>> >> >> > +               if (unlikely(brq->data.blocks << 9 !=
>> >> brq->data.bytes_xfered))
>> >> >> > +                       return MMC_BLK_PARTIAL;
>> >> >> > +               else
>> >> >> > +                       return MMC_BLK_SUCCESS;
>> >> >> > +       }
>> >> >> > +
>> >> >> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
>> >> >> >                return MMC_BLK_PARTIAL;
>> >> >> >
>> >> >> >        return MMC_BLK_SUCCESS;
>> >> >> >  }
>> >> >> >
>> >> >> > +static int mmc_blk_packed_err_check(struct mmc_card *card, +
>>                        struct mmc_async_req *areq)
>> >> >> > +{
>> >> >> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct
>> >> mmc_queue_req,
>> >> >> > +                                                   mmc_active);
>> +
>>       int err, check, status;
>> >> >> > +       u8 ext_csd[512];
>> >> >> > +
>> >> >> > +       check = mmc_blk_err_check(card, areq);
>> >> >> > +
>> >> >> > +       if (check == MMC_BLK_SUCCESS)
>> >> >> > +               return check;
>> I think we need to check the status for all cases and not only in case
>> of
>> MMC_BLK_PARTIAL. For example, in cases where the header was traferred
>> successfully but had logic errors (wrong number of sectors etc.)
>> mmc_blk_err_check will return MMC_BLK_SUCCESS although the packed
>> commands
>> failed.
> Similarly, Sahitya Tummala is already mentioned this.
> Other error case will be checked in next version.
> The case you suggested is about read or write?
> Device may detect error and stop transferring the data.
Sahitya suggested to also check other error cases that mmc_blk_err_check
returns (such as MMC_BLK_CMD_ERR, MMC_BLK_ECC_ERR and MMC_BLK_DATA_ERR).
I suggest to also check the exception bit in the status even if
mmc_blk_err_check returned success, since mmc_blk_err_check might not
catch all the packed commands failures. One example for such a failure is
when the header of read packed commands will have logical error.

Thanks,
Maya
--
Seny by a Consultant for Qualcomm innovation center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

Maya Erez wrote:
> >> >> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com>
> >> wrote:
> >> >> > @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card
> >> *card,
> >> >> >         * kind.  If it was a write, we may have transitioned to
>       * program mode, which we have to wait for it to complete.
>       */
> >> >> > -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) !=
> >> READ) {
> >> >> > +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) !=
> >> READ) ||
> >> >> > +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR))
> Since the header's direction is WRITE I don't think we also need to check
> if mq_mrq->packed_cmd == MMC_PACKED_WR_HDR since it will be covered by the
> original condition.
The header is written. But origin request is about read.
That means rq_data_dir(req) is READ. So new condition is needed.
> >> {
> >> >> >                u32 status;
> >> >> >                do {
> >> >> >                        int err = get_card_status(card, &status,
> 5);
> A general question, not related specifically to packed commands - Do you
> know why the status is not checked to see which error we got?
This status is for checking whether the card is ready for new data.
> >> >> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card
> >> *card,
> >> >> >        if (!brq->data.bytes_xfered)
> >> >> >                return MMC_BLK_RETRY;
> >> >> >
> >> >> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> >> >> > +               if (unlikely(brq->data.blocks << 9 !=
> >> brq->data.bytes_xfered))
> >> >> > +                       return MMC_BLK_PARTIAL;
> >> >> > +               else
> >> >> > +                       return MMC_BLK_SUCCESS;
> >> >> > +       }
> >> >> > +
> >> >> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
> >> >> >                return MMC_BLK_PARTIAL;
> >> >> >
> >> >> >        return MMC_BLK_SUCCESS;
> >> >> >  }
> >> >> >
> >> >> > +static int mmc_blk_packed_err_check(struct mmc_card *card, +
>                        struct mmc_async_req *areq)
> >> >> > +{
> >> >> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct
> >> mmc_queue_req,
> >> >> > +                                                   mmc_active); +
>       int err, check, status;
> >> >> > +       u8 ext_csd[512];
> >> >> > +
> >> >> > +       check = mmc_blk_err_check(card, areq);
> >> >> > +
> >> >> > +       if (check == MMC_BLK_SUCCESS)
> >> >> > +               return check;
> I think we need to check the status for all cases and not only in case of
> MMC_BLK_PARTIAL. For example, in cases where the header was traferred
> successfully but had logic errors (wrong number of sectors etc.)
> mmc_blk_err_check will return MMC_BLK_SUCCESS although the packed commands
> failed.
Similarly, Sahitya Tummala is already mentioned this.
Other error case will be checked in next version.
The case you suggested is about read or write?
Device may detect error and stop transferring the data.
> >> >> > +
> >> >> > +       if (check == MMC_BLK_PARTIAL) {
> >> >> > +               err = get_card_status(card, &status, 0);
> >> >> > +               if (err)
> >> >> > +                       return MMC_BLK_ABORT;
> >> >> > +
> >> >> > +               if (status & R1_EXP_EVENT) {
> >> >> > +                       err = mmc_send_ext_csd(card, ext_csd); +
>                     if (err)
> >> >> > +                               return MMC_BLK_ABORT;
> >> >> > +
> >> >> > +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS +
> 0]
> why do we need the + 0?
It is explicit expression, no more, no less.
Actually, there is no need.
> >> &
> >> >> > +
> >> EXT_CSD_PACKED_INDEXED_ERROR) {
> >> >> > +                                       /* Make be 0-based */
> The comment is not understood
PACKED_FAILURE_INDEX starts from '1'
It is converted to 0-base for use.
> >> >> > +                                       mq_mrq->packed_fail_idx =
> +
> Thanks,
> Maya Erez
> --
> Seny by a Consultant for Qualcomm innovation center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[merez]

>> >> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com>
>> wrote:
>> >> > @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card
>> *card,
>> >> >         * kind.  If it was a write, we may have transitioned to  
      * program mode, which we have to wait for it to complete.  
      */
>> >> > -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) !=
>> READ) {
>> >> > +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) !=
>> READ) ||
>> >> > +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR))
Since the header's direction is WRITE I don't think we also need to check
if mq_mrq->packed_cmd == MMC_PACKED_WR_HDR since it will be covered by the
original condition.
>> {
>> >> >                u32 status;
>> >> >                do {
>> >> >                        int err = get_card_status(card, &status,
5);
A general question, not related specifically to packed commands - Do you
know why the status is not checked to see which error we got?
>> >> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card
>> *card,
>> >> >        if (!brq->data.bytes_xfered)
>> >> >                return MMC_BLK_RETRY;
>> >> >
>> >> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
>> >> > +               if (unlikely(brq->data.blocks << 9 !=
>> brq->data.bytes_xfered))
>> >> > +                       return MMC_BLK_PARTIAL;
>> >> > +               else
>> >> > +                       return MMC_BLK_SUCCESS;
>> >> > +       }
>> >> > +
>> >> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
>> >> >                return MMC_BLK_PARTIAL;
>> >> >
>> >> >        return MMC_BLK_SUCCESS;
>> >> >  }
>> >> >
>> >> > +static int mmc_blk_packed_err_check(struct mmc_card *card, +    
                       struct mmc_async_req *areq)
>> >> > +{
>> >> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct
>> mmc_queue_req,
>> >> > +                                                   mmc_active); +
      int err, check, status;
>> >> > +       u8 ext_csd[512];
>> >> > +
>> >> > +       check = mmc_blk_err_check(card, areq);
>> >> > +
>> >> > +       if (check == MMC_BLK_SUCCESS)
>> >> > +               return check;
I think we need to check the status for all cases and not only in case of
MMC_BLK_PARTIAL. For example, in cases where the header was traferred
successfully but had logic errors (wrong number of sectors etc.)
mmc_blk_err_check will return MMC_BLK_SUCCESS although the packed commands
failed.
>> >> > +
>> >> > +       if (check == MMC_BLK_PARTIAL) {
>> >> > +               err = get_card_status(card, &status, 0);
>> >> > +               if (err)
>> >> > +                       return MMC_BLK_ABORT;
>> >> > +
>> >> > +               if (status & R1_EXP_EVENT) {
>> >> > +                       err = mmc_send_ext_csd(card, ext_csd); +  
                    if (err)
>> >> > +                               return MMC_BLK_ABORT;
>> >> > +
>> >> > +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS +
0]
why do we need the + 0?
>> &
>> >> > +                                              
>> EXT_CSD_PACKED_INDEXED_ERROR) {
>> >> > +                                       /* Make be 0-based */
The comment is not understood
>> >> > +                                       mq_mrq->packed_fail_idx =
+                                              
Thanks,
Maya Erez
--
Seny by a Consultant for Qualcomm innovation center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

S, Venkatraman <svenkatr@ti.com> wrote: 
> On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > S, Venkatraman <svenkatr@ti.com> wrote:
> >> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> >> > This patch supports packed command of eMMC4.5 device.
> >> > Several reads(or writes) can be grouped in packed command
> >> > and all data of the individual commands can be sent in a
> >> > single transfer on the bus.
> >> >
> >> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> >> > ---
> >> >  drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
> >> >  drivers/mmc/card/queue.c |   48 ++++++-
> >> >  drivers/mmc/card/queue.h |   12 ++
> >> >  include/linux/mmc/core.h |    3 +
> >> >  4 files changed, 404 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> >> > index a1cb21f..6c49656 100644
> >> > --- a/drivers/mmc/card/block.c
> >> > +++ b/drivers/mmc/card/block.c
> >> > @@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
> >> >  #define INAND_CMD38_ARG_SECTRIM1 0x81
> >> >  #define INAND_CMD38_ARG_SECTRIM2 0x88
> >> >
> >> > +#define mmc_req_rel_wr(req)    (((req->cmd_flags & REQ_FUA) || \
> >> > +                       (req->cmd_flags & REQ_META)) && \
> >> > +                       (rq_data_dir(req) == WRITE))
> >> > +#define PACKED_CMD_VER         0x01
> >> > +#define PACKED_CMD_RD          0x01
> >> > +#define PACKED_CMD_WR          0x02
> >> > +
> >> >  static DEFINE_MUTEX(block_mutex);
> >> >
> >> >  /*
> >> > @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
> >> >         * kind.  If it was a write, we may have transitioned to
> >> >         * program mode, which we have to wait for it to complete.
> >> >         */
> >> > -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
> >> > +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
> >> > +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
> >> >                u32 status;
> >> >                do {
> >> >                        int err = get_card_status(card, &status, 5);
> >> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
> >> >        if (!brq->data.bytes_xfered)
> >> >                return MMC_BLK_RETRY;
> >> >
> >> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> >> > +               if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
> >> > +                       return MMC_BLK_PARTIAL;
> >> > +               else
> >> > +                       return MMC_BLK_SUCCESS;
> >> > +       }
> >> > +
> >> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
> >> >                return MMC_BLK_PARTIAL;
> >> >
> >> >        return MMC_BLK_SUCCESS;
> >> >  }
> >> >
> >> > +static int mmc_blk_packed_err_check(struct mmc_card *card,
> >> > +                            struct mmc_async_req *areq)
> >> > +{
> >> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
> >> > +                                                   mmc_active);
> >> > +       int err, check, status;
> >> > +       u8 ext_csd[512];
> >> > +
> >> > +       check = mmc_blk_err_check(card, areq);
> >> > +
> >> > +       if (check == MMC_BLK_SUCCESS)
> >> > +               return check;
> >> > +
> >> > +       if (check == MMC_BLK_PARTIAL) {
> >> > +               err = get_card_status(card, &status, 0);
> >> > +               if (err)
> >> > +                       return MMC_BLK_ABORT;
> >> > +
> >> > +               if (status & R1_EXP_EVENT) {
> >> > +                       err = mmc_send_ext_csd(card, ext_csd);
> >> > +                       if (err)
> >> > +                               return MMC_BLK_ABORT;
> >> > +
> >> > +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
> >> > +                                               EXT_CSD_PACKED_FAILURE) &&
> >> > +                                       (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> >> > +                                        EXT_CSD_PACKED_GENERIC_ERROR)) {
> >> > +                               if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> >> > +                                               EXT_CSD_PACKED_INDEXED_ERROR) {
> >> > +                                       /* Make be 0-based */
> >> > +                                       mq_mrq->packed_fail_idx =
> >> > +                                               ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
> >> > +                                       return MMC_BLK_PARTIAL;
> >> > +                               } else {
> >> > +                                       return MMC_BLK_RETRY;
> >> > +                               }
> >> > +                       }
> >> > +               } else {
> >> > +                       return MMC_BLK_RETRY;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       if (check != MMC_BLK_ABORT)
> >> > +               return MMC_BLK_RETRY;
> >> > +       else
> >> > +               return MMC_BLK_ABORT;
> >> > +}
> >> > +
> >> >  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> >> >                               struct mmc_card *card,
> >> >                               int disable_multi,
> >> > @@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> >> >        mmc_queue_bounce_pre(mqrq);
> >> >  }
> >> >
> >> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
> >> > +{
> >> > +       struct request_queue *q = mq->queue;
> >> > +       struct mmc_card *card = mq->card;
> >> > +       struct request *cur = req, *next = NULL;
> >> > +       struct mmc_blk_data *md = mq->data;
> >> > +       bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> >> > +       unsigned int req_sectors = 0, phys_segments = 0;
> >> > +       unsigned int max_blk_count, max_phys_segs;
> >> > +       u8 max_packed_rw = 0;
> >> > +       u8 reqs = 0;
> >> > +
> >> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> >> > +                       !card->ext_csd.packed_event_en)
> >> > +               goto no_packed;
> >> > +
> >> > +       if (rq_data_dir(cur) == READ)
> >> > +               max_packed_rw = card->ext_csd.max_packed_reads;
> >> > +       else
> >> > +               max_packed_rw = card->ext_csd.max_packed_writes;
> >> > +
> >> > +       if (max_packed_rw == 0)
> >> > +               goto no_packed;
> >> > +
> >> > +       if (mmc_req_rel_wr(cur) &&
> >> > +                       (md->flags & MMC_BLK_REL_WR) &&
> >> > +                       !en_rel_wr) {
> >> > +               goto no_packed;
> >> > +       }
> >> > +
> >> > +       max_blk_count = min(card->host->max_blk_count,
> >> > +                       card->host->max_req_size >> 9);
> >> > +       if (unlikely(max_blk_count > 0xffff))
> >> > +               max_blk_count = 0xffff;
> >> > +
> >> > +       max_phys_segs = queue_max_segments(q);
> >> > +       req_sectors += blk_rq_sectors(cur);
> >> > +       phys_segments += req->nr_phys_segments;
> >> > +
> >> > +       if (rq_data_dir(cur) == WRITE) {
> >> > +               req_sectors++;
> >> > +               phys_segments++;
> >> > +       }
> >> > +
> >> > +       while (reqs < max_packed_rw - 1) {
> >> > +               next = blk_fetch_request(q);
> >> > +               if (!next)
> >> > +                       break;
> >> > +
> >> > +               if (rq_data_dir(cur) != rq_data_dir(next)) {
> >> > +                       blk_requeue_request(q, next);
> >> > +                       break;
> >> > +               }
> >> > +
> >> > +               if (mmc_req_rel_wr(next) &&
> >> > +                               (md->flags & MMC_BLK_REL_WR) &&
> >> > +                               !en_rel_wr) {
> >> > +                       blk_requeue_request(q, next);
> >> > +                       break;
> >> > +               }
> >> > +
> >> > +               req_sectors += blk_rq_sectors(next);
> >> > +               if (req_sectors > max_blk_count) {
> >> > +                       blk_requeue_request(q, next);
> >> > +                       break;
> >> > +               }
> >> > +
> >> > +               phys_segments +=  next->nr_phys_segments;
> >> > +               if (phys_segments > max_phys_segs) {
> >> > +                       blk_requeue_request(q, next);
> >> > +                       break;
> >> > +               }
> >> I mentioned this before - if the next request is not packable and requeued,
> >> blk_fetch_request will retrieve it again and this while loop will
> >> never terminate.
> >>
> > If next request is not packable, it is requeued and 'break' terminates this loop.
> > So it not infinite.
> 
> Right !! But that doesn't help finding the commands that are packable.
> Ideally, you'd need to pack all neighbouring requests into one packed command.
> The way CFQ works, it is not necessary that the fetch would return all
> outstanding
> requests that are packable (unless you invoke a forced dispatch)
> 
> It would be good to see some numbers on the number of pack hits / misses that
> you would encounter with this logic, on a typical usecase.
> 
Is it considered only for CFQ scheduler? How about other I/O scheduler?
If all requests are drained from scheduler queue forcedly,
the number of candidate to be packed can be increased.
However we may lose the unique CFQ's strength and MMC D/D may take the CFQ's duty.
Basically, this patch accommodates the origin order requests from I/O scheduler.

> >> > +
> >> > +               list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
> >> > +               cur = next;
> >> > +               reqs++;
> >> > +       }
> >> > +
> >> > +       if (reqs > 0) {
> >> > +               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
> >> > +               return (reqs + 1);
> >> > +       }
> >> > +
> >> > +no_packed:
> >> > +       mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
> >> > +       return reqs;
> >> > +}
> >> > +
> >> > +static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
> >> > +                              struct mmc_card *card,
> >> > +                              struct mmc_queue *mq,
> >> > +                              u8 reqs)
> >> > +{
> >> > +       struct mmc_blk_request *brq = &mqrq->brq;
> >> > +       struct request *req = mqrq->req;
> >> > +       struct request *prq;
> >> > +       struct mmc_blk_data *md = mq->data;
> >> > +       bool do_rel_wr;
> >> > +       u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
> >> > +       u8 i =1;
> >> > +
> >> > +       mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
> >> > +               MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
> >> > +       mqrq->packed_blocks = 0;
> >> > +       mqrq->packed_fail_idx = -1;
> >> > +
> >> > +       memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
> >> > +       packed_cmd_hdr[0] = (reqs << 16) |
> >> > +               (((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
> >> > +               PACKED_CMD_VER;
> >> > +
> >> > +       /*
> >> > +        * Argument for each entry of packed group
> >> > +        */
> >> > +       list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
> >> > +               do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
> >> > +               /* Argument of CMD23*/
> >> > +               packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
> >> > +                       blk_rq_sectors(prq);
> >> > +               /* Argument of CMD18 or CMD25 */
> >> > +               packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
> >> > +                       blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
> >> > +               mqrq->packed_blocks += blk_rq_sectors(prq);
> >> > +               i++;
> >> > +       }
> >> > +
> >> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
> >> > +       brq->mrq.cmd = &brq->cmd;
> >> > +       brq->mrq.data = &brq->data;
> >> > +       brq->mrq.sbc = &brq->sbc;
> >> > +       brq->mrq.stop = &brq->stop;
> >> > +
> >> > +       brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> >> > +       brq->sbc.arg = MMC_CMD23_ARG_PACKED |
> >> > +               ((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
> >> > +       brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> >> > +
> >> > +       brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
> >> > +       brq->cmd.arg = blk_rq_pos(req);
> >> > +       if (!mmc_card_blockaddr(card))
> >> > +               brq->cmd.arg <<= 9;
> >> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> >> > +
> >> > +       brq->data.blksz = 512;
> >> > +       /*
> >> > +        * Write separately the packd command header only for packed read.
> >> > +        * In case of packed write, header is sent with blocks of data.
> >> > +        */
> >> > +       brq->data.blocks = (rq_data_dir(req) == READ) ?
> >> > +               1 : mqrq->packed_blocks + 1;
> >> > +       brq->data.flags |= MMC_DATA_WRITE;
> >> > +
> >> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> >> > +       brq->stop.arg = 0;
> >> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> >> > +
> >> > +       mmc_set_data_timeout(&brq->data, card);
> >> > +
> >> > +       brq->data.sg = mqrq->sg;
> >> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> >> > +
> >> > +       mqrq->mmc_active.mrq = &brq->mrq;
> >> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> >> > +
> >> > +       mmc_queue_bounce_pre(mqrq);
> >> > +}
> >> > +
> >> > +static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
> >> > +                              struct mmc_card *card,
> >> > +                              struct mmc_queue *mq)
> >> > +{
> >> > +       struct mmc_blk_request *brq = &mqrq->brq;
> >> > +       struct request *req = mqrq->req;
> >> > +
> >> > +       mqrq->packed_cmd = MMC_PACKED_READ;
> >> > +
> >> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
> >> > +       brq->mrq.cmd = &brq->cmd;
> >> > +       brq->mrq.data = &brq->data;
> >> > +       brq->mrq.stop = &brq->stop;
> >> > +
> >> > +       brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> >> > +       brq->cmd.arg = blk_rq_pos(req);
> >> > +       if (!mmc_card_blockaddr(card))
> >> > +               brq->cmd.arg <<= 9;
> >> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> >> > +       brq->data.blksz = 512;
> >> > +       brq->data.blocks = mqrq->packed_blocks;
> >> > +       brq->data.flags |= MMC_DATA_READ;
> >> > +
> >> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> >> > +       brq->stop.arg = 0;
> >> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> >> > +
> >> > +       mmc_set_data_timeout(&brq->data, card);
> >> > +
> >> > +       brq->data.sg = mqrq->sg;
> >> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> >> > +
> >> > +       mqrq->mmc_active.mrq = &brq->mrq;
> >> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> >> > +
> >> > +       mmc_queue_bounce_pre(mqrq);
> >> > +}
> >> > +
> >> >  static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
> >> >                           struct mmc_blk_request *brq, struct request *req,
> >> >                           int ret)
> >> > @@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >> >        int ret = 1, disable_multi = 0, retry = 0, type;
> >> >        enum mmc_blk_status status;
> >> >        struct mmc_queue_req *mq_rq;
> >> > -       struct request *req;
> >> > +       struct request *req, *prq;
> >> >        struct mmc_async_req *areq;
> >> > +       u8 reqs = 0;
> >> >
> >> >        if (!rqc && !mq->mqrq_prev->req)
> >> >                return 0;
> >> >
> >> > +       if (rqc)
> >> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> >> > +
> >> >        do {
> >> >                if (rqc) {
> >> > -                       mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> >> > +                       if (reqs >= 2) {
> >> > +                               mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
> >> > +                               if (rq_data_dir(rqc) == READ) {
> >> > +                                       areq = &mq->mqrq_cur->mmc_active;
> >> > +                                       mmc_wait_for_req(card->host, areq->mrq);
> >> > +                                       status = mmc_blk_packed_err_check(card, areq);
> >> > +                                       if (status == MMC_BLK_SUCCESS) {
> >> > +                                               mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
> >> > +                                       } else {
> >> > +                                               goto check_status;
> >> > +                                       }
> >> > +                               }
> >> > +                       } else {
> >>
> >> IIUC, the code in mmc_blk_chk_packable
> >> <snip>
> >>        if (reqs > 0) {
> >>                list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
> >>               return (reqs + 1);
> >> </snip>
> >> adds the request to the packed list, and needs to be reversed if it's
> >> decided to not to do a packed request ?
> > Any condition or reason  not to do packed request after several requests
> > is added in packed list? I didn't catch your comment exactly.
> >
> What I meant was that the request should be removed from queuelist
> when you decide not to pack.
> 
After request is add to packed list, it needs to decide not to pack?
Could you explain that case?
mmc_blk_chk_packable decides whether packing is possible or not.
If possible, it returns non-zero value.
If not, it return zero. Of course, packed_list is empty in this time.
I don't know what you concern.
> >>
> >>
> >> > +                               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> >> > +                       }
> >> >                        areq = &mq->mqrq_cur->mmc_active;
> >> >                } else
> >> >                        areq = NULL;
> >> > @@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >> >                if (!areq)
> >> >                        return 0;
> >> >
> >> > +check_status:
> >> >                mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
> >> >                brq = &mq_rq->brq;
> >> >                req = mq_rq->req;
> >> > @@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >> >                         * A block was successfully transferred.
> >> >                         */
> >> >                        mmc_blk_reset_success(md, type);
> >> > -                       spin_lock_irq(&md->lock);
> >> > -                       ret = __blk_end_request(req, 0,
> >> > +
> >> > +                       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> >> > +                               int idx = mq_rq->packed_fail_idx, i = 0;
> >> > +                               while (!list_empty(&mq_rq->packed_list)) {
> >> > +                                       prq = list_entry_rq(mq_rq->packed_list.next);
> >> > +                                       if (idx == i) {
> >> > +                                               /* retry from error index */
> >> > +                                               reqs -= idx;
> >> > +                                               mq_rq->req = prq;
> >> > +                                               ret = 1;
> >> > +                                               break;
> >> > +                                       }
> >> > +                                       list_del_init(&prq->queuelist);
> >> > +                                       spin_lock_irq(&md->lock);
> >> > +                                       ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
> >> > +                                       spin_unlock_irq(&md->lock);
> >> > +                                       i++;
> >> > +                               }
> >> > +                               break;
> >> > +                       } else {
> >> > +                               spin_lock_irq(&md->lock);
> >> > +                               ret = __blk_end_request(req, 0,
> >> >                                                brq->data.bytes_xfered);
> >> > -                       spin_unlock_irq(&md->lock);
> >> > +                               spin_unlock_irq(&md->lock);
> >> > +                       }
> >> > +
> >> >                        /*
> >> >                         * If the blk_end_request function returns non-zero even
> >> >                         * though all data has been transferred and no errors
> >> > @@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >> >                        break;
> >> >                }
> >> >
> >> > -               if (ret) {
> >> > +               if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
> >> >                        /*
> >> >                         * In case of a incomplete request
> >> >                         * prepare it again and resend.
> >> > @@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >> >        return 1;
> >> >
> >> >  cmd_abort:
> >> > -       spin_lock_irq(&md->lock);
> >> > -       while (ret)
> >> > -               ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> >> > -       spin_unlock_irq(&md->lock);
> >> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> >> > +               spin_lock_irq(&md->lock);
> >> > +               while (ret)
> >> > +                       ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> >> > +               spin_unlock_irq(&md->lock);
> >> > +       } else {
> >> > +               while (!list_empty(&mq_rq->packed_list)) {
> >> > +                       prq = list_entry_rq(mq_rq->packed_list.next);
> >> > +                       list_del_init(&prq->queuelist);
> >> > +                       spin_lock_irq(&md->lock);
> >> > +                       __blk_end_request(prq, -EIO, blk_rq_bytes(prq));
> >> > +                       spin_unlock_irq(&md->lock);
> >> > +               }
> >> > +       }
> >> >
> >> >  start_new_req:
> >> >        if (rqc) {
> >> > +               /*
> >> > +                * If current request is packed, it need to put back.
> >> > +                */
> >> > +               if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
> >> > +                       while (!list_empty(&mq->mqrq_cur->packed_list)) {
> >> > +                               prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
> >> > +                               if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
> >> > +                                       list_del_init(&prq->queuelist);
> >> > +                                       blk_requeue_request(mq->queue, prq);
> >> > +                               } else {
> >> > +                                       list_del_init(&prq->queuelist);
> >> > +                               }
> >> > +                       }
> >> > +               }
> >> >                mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> >> >                mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
> >> >        }
> >> > diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> >> > index dcad59c..3a4542e 100644
> >> > --- a/drivers/mmc/card/queue.c
> >> > +++ b/drivers/mmc/card/queue.c
> >> > @@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
> >> >
> >> >        memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
> >> >        memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
> >> > +       INIT_LIST_HEAD(&mqrq_cur->packed_list);
> >> > +       INIT_LIST_HEAD(&mqrq_prev->packed_list);
> >> >        mq->mqrq_cur = mqrq_cur;
> >> >        mq->mqrq_prev = mqrq_prev;
> >> >        mq->queue->queuedata = mq;
> >> > @@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
> >> >        }
> >> >  }
> >> >
> >> > +static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
> >> > +                               struct mmc_queue_req *mqrq,
> >> > +                               struct scatterlist *sg)
> >> > +{
> >> > +       struct scatterlist *__sg;
> >> > +       unsigned int sg_len = 0;
> >> > +       struct request *req;
> >> > +       enum mmc_packed_cmd cmd;
> >> > +
> >> > +       cmd = mqrq->packed_cmd;
> >> > +
> >> > +       if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
> >> > +               __sg =sg;
> >> > +               sg_set_buf(__sg, mqrq->packed_cmd_hdr,
> >> > +                               sizeof(mqrq->packed_cmd_hdr));
> >> > +               sg_len++;
> >> > +               if (cmd == MMC_PACKED_WR_HDR) {
> >> > +                       sg_mark_end(__sg);
> >> > +                       return sg_len;
> >> > +               }
> >> > +               __sg->page_link &= ~0x02;
> >> > +       }
> >> > +
> >> > +       __sg = sg + sg_len;
> >> > +       list_for_each_entry(req, &mqrq->packed_list, queuelist) {
> >> > +               sg_len += blk_rq_map_sg(mq->queue, req, __sg);
> >> > +               __sg = sg + (sg_len - 1);
> >> > +               (__sg++)->page_link &= ~0x02;
> >> > +       }
> >> > +       sg_mark_end(sg + (sg_len - 1));
> >> > +       return sg_len;
> >> > +}
> >> > +
> >> >  /*
> >> >  * Prepare the sg list(s) to be handed of to the host driver
> >> >  */
> >> > @@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req
> *mqrq)
> >> >        struct scatterlist *sg;
> >> >        int i;
> >> >
> >> > -       if (!mqrq->bounce_buf)
> >> > -               return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> >> > +       if (!mqrq->bounce_buf) {
> >> > +               if (!list_empty(&mqrq->packed_list))
> >> > +                       return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
> >> > +               else
> >> > +                       return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> >> > +       }
> >> >
> >> >        BUG_ON(!mqrq->bounce_sg);
> >> >
> >> > -       sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> >> > +       if (!list_empty(&mqrq->packed_list))
> >> > +               sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
> >> > +       else
> >> > +               sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> >> >
> >> >        mqrq->bounce_sg_len = sg_len;
> >> >
> >> > diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> >> > index d2a1eb4..5d0131e 100644
> >> > --- a/drivers/mmc/card/queue.h
> >> > +++ b/drivers/mmc/card/queue.h
> >> > @@ -12,6 +12,13 @@ struct mmc_blk_request {
> >> >        struct mmc_data         data;
> >> >  };
> >> >
> >> > +enum mmc_packed_cmd {
> >> > +       MMC_PACKED_NONE = 0,
> >> > +       MMC_PACKED_WR_HDR,
> >> > +       MMC_PACKED_WRITE,
> >> > +       MMC_PACKED_READ,
> >> > +};
> >> > +
> >> >  struct mmc_queue_req {
> >> >        struct request          *req;
> >> >        struct mmc_blk_request  brq;
> >> > @@ -20,6 +27,11 @@ struct mmc_queue_req {
> >> >        struct scatterlist      *bounce_sg;
> >> >        unsigned int            bounce_sg_len;
> >> >        struct mmc_async_req    mmc_active;
> >> > +       struct list_head        packed_list;
> >> > +       u32                     packed_cmd_hdr[128];
> >> > +       unsigned int            packed_blocks;
> >> > +       enum mmc_packed_cmd     packed_cmd;
> >> > +       int             packed_fail_idx;
> >> >  };
> >> >
> >> >  struct mmc_queue {
> >> > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> >> > index 174a844..3c61d5a 100644
> >> > --- a/include/linux/mmc/core.h
> >> > +++ b/include/linux/mmc/core.h
> >> > @@ -18,6 +18,8 @@ struct mmc_request;
> >> >  struct mmc_command {
> >> >        u32                     opcode;
> >> >        u32                     arg;
> >> > +#define MMC_CMD23_ARG_REL_WR   (1 << 31)
> >> > +#define MMC_CMD23_ARG_PACKED   ((0 << 31) | (1 << 30))
> >> >        u32                     resp[4];
> >> >        unsigned int            flags;          /* expected response type */
> >> >  #define MMC_RSP_PRESENT        (1 << 0)
> >> > @@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
> >> >  extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
> >> >        struct mmc_command *, int);
> >> >  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
> >> > +extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
> >>
> >> Stray change / doesn't belong to this patch.
> > Do you mean to split this in another patch?
> >
> Yes. In fact all the changes to the header files are independent of
> the implementation, so they belong to a separate patch.

I considered your opinion.
But it seems not big trouble even though being kept.

Thanks.
Seungwon Jeon.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[S, Venkatraman]

On Thu, Nov 3, 2011 at 7:23 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> S, Venkatraman <svenkatr@ti.com> wrote:
>> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> > This patch supports packed command of eMMC4.5 device.
>> > Several reads(or writes) can be grouped in packed command
>> > and all data of the individual commands can be sent in a
>> > single transfer on the bus.
>> >
>> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>> > ---
>> >  drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
>> >  drivers/mmc/card/queue.c |   48 ++++++-
>> >  drivers/mmc/card/queue.h |   12 ++
>> >  include/linux/mmc/core.h |    3 +
>> >  4 files changed, 404 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> > index a1cb21f..6c49656 100644
>> > --- a/drivers/mmc/card/block.c
>> > +++ b/drivers/mmc/card/block.c
>> > @@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
>> >  #define INAND_CMD38_ARG_SECTRIM1 0x81
>> >  #define INAND_CMD38_ARG_SECTRIM2 0x88
>> >
>> > +#define mmc_req_rel_wr(req)    (((req->cmd_flags & REQ_FUA) || \
>> > +                       (req->cmd_flags & REQ_META)) && \
>> > +                       (rq_data_dir(req) == WRITE))
>> > +#define PACKED_CMD_VER         0x01
>> > +#define PACKED_CMD_RD          0x01
>> > +#define PACKED_CMD_WR          0x02
>> > +
>> >  static DEFINE_MUTEX(block_mutex);
>> >
>> >  /*
>> > @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
>> >         * kind.  If it was a write, we may have transitioned to
>> >         * program mode, which we have to wait for it to complete.
>> >         */
>> > -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
>> > +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
>> > +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
>> >                u32 status;
>> >                do {
>> >                        int err = get_card_status(card, &status, 5);
>> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
>> >        if (!brq->data.bytes_xfered)
>> >                return MMC_BLK_RETRY;
>> >
>> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
>> > +               if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
>> > +                       return MMC_BLK_PARTIAL;
>> > +               else
>> > +                       return MMC_BLK_SUCCESS;
>> > +       }
>> > +
>> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
>> >                return MMC_BLK_PARTIAL;
>> >
>> >        return MMC_BLK_SUCCESS;
>> >  }
>> >
>> > +static int mmc_blk_packed_err_check(struct mmc_card *card,
>> > +                            struct mmc_async_req *areq)
>> > +{
>> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
>> > +                                                   mmc_active);
>> > +       int err, check, status;
>> > +       u8 ext_csd[512];
>> > +
>> > +       check = mmc_blk_err_check(card, areq);
>> > +
>> > +       if (check == MMC_BLK_SUCCESS)
>> > +               return check;
>> > +
>> > +       if (check == MMC_BLK_PARTIAL) {
>> > +               err = get_card_status(card, &status, 0);
>> > +               if (err)
>> > +                       return MMC_BLK_ABORT;
>> > +
>> > +               if (status & R1_EXP_EVENT) {
>> > +                       err = mmc_send_ext_csd(card, ext_csd);
>> > +                       if (err)
>> > +                               return MMC_BLK_ABORT;
>> > +
>> > +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
>> > +                                               EXT_CSD_PACKED_FAILURE) &&
>> > +                                       (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
>> > +                                        EXT_CSD_PACKED_GENERIC_ERROR)) {
>> > +                               if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
>> > +                                               EXT_CSD_PACKED_INDEXED_ERROR) {
>> > +                                       /* Make be 0-based */
>> > +                                       mq_mrq->packed_fail_idx =
>> > +                                               ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
>> > +                                       return MMC_BLK_PARTIAL;
>> > +                               } else {
>> > +                                       return MMC_BLK_RETRY;
>> > +                               }
>> > +                       }
>> > +               } else {
>> > +                       return MMC_BLK_RETRY;
>> > +               }
>> > +       }
>> > +
>> > +       if (check != MMC_BLK_ABORT)
>> > +               return MMC_BLK_RETRY;
>> > +       else
>> > +               return MMC_BLK_ABORT;
>> > +}
>> > +
>> >  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>> >                               struct mmc_card *card,
>> >                               int disable_multi,
>> > @@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>> >        mmc_queue_bounce_pre(mqrq);
>> >  }
>> >
>> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
>> > +{
>> > +       struct request_queue *q = mq->queue;
>> > +       struct mmc_card *card = mq->card;
>> > +       struct request *cur = req, *next = NULL;
>> > +       struct mmc_blk_data *md = mq->data;
>> > +       bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
>> > +       unsigned int req_sectors = 0, phys_segments = 0;
>> > +       unsigned int max_blk_count, max_phys_segs;
>> > +       u8 max_packed_rw = 0;
>> > +       u8 reqs = 0;
>> > +
>> > +       if (!(md->flags & MMC_BLK_CMD23) &&
>> > +                       !card->ext_csd.packed_event_en)
>> > +               goto no_packed;
>> > +
>> > +       if (rq_data_dir(cur) == READ)
>> > +               max_packed_rw = card->ext_csd.max_packed_reads;
>> > +       else
>> > +               max_packed_rw = card->ext_csd.max_packed_writes;
>> > +
>> > +       if (max_packed_rw == 0)
>> > +               goto no_packed;
>> > +
>> > +       if (mmc_req_rel_wr(cur) &&
>> > +                       (md->flags & MMC_BLK_REL_WR) &&
>> > +                       !en_rel_wr) {
>> > +               goto no_packed;
>> > +       }
>> > +
>> > +       max_blk_count = min(card->host->max_blk_count,
>> > +                       card->host->max_req_size >> 9);
>> > +       if (unlikely(max_blk_count > 0xffff))
>> > +               max_blk_count = 0xffff;
>> > +
>> > +       max_phys_segs = queue_max_segments(q);
>> > +       req_sectors += blk_rq_sectors(cur);
>> > +       phys_segments += req->nr_phys_segments;
>> > +
>> > +       if (rq_data_dir(cur) == WRITE) {
>> > +               req_sectors++;
>> > +               phys_segments++;
>> > +       }
>> > +
>> > +       while (reqs < max_packed_rw - 1) {
>> > +               next = blk_fetch_request(q);
>> > +               if (!next)
>> > +                       break;
>> > +
>> > +               if (rq_data_dir(cur) != rq_data_dir(next)) {
>> > +                       blk_requeue_request(q, next);
>> > +                       break;
>> > +               }
>> > +
>> > +               if (mmc_req_rel_wr(next) &&
>> > +                               (md->flags & MMC_BLK_REL_WR) &&
>> > +                               !en_rel_wr) {
>> > +                       blk_requeue_request(q, next);
>> > +                       break;
>> > +               }
>> > +
>> > +               req_sectors += blk_rq_sectors(next);
>> > +               if (req_sectors > max_blk_count) {
>> > +                       blk_requeue_request(q, next);
>> > +                       break;
>> > +               }
>> > +
>> > +               phys_segments +=  next->nr_phys_segments;
>> > +               if (phys_segments > max_phys_segs) {
>> > +                       blk_requeue_request(q, next);
>> > +                       break;
>> > +               }
>> I mentioned this before - if the next request is not packable and requeued,
>> blk_fetch_request will retrieve it again and this while loop will
>> never terminate.
>>
> If next request is not packable, it is requeued and 'break' terminates this loop.
> So it not infinite.

Right !! But that doesn't help finding the commands that are packable.
Ideally, you'd need to pack all neighbouring requests into one packed command.
The way CFQ works, it is not necessary that the fetch would return all
outstanding
requests that are packable (unless you invoke a forced dispatch)

It would be good to see some numbers on the number of pack hits / misses that
you would encounter with this logic, on a typical usecase.

>> > +
>> > +               list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
>> > +               cur = next;
>> > +               reqs++;
>> > +       }
>> > +
>> > +       if (reqs > 0) {
>> > +               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
>> > +               return (reqs + 1);
>> > +       }
>> > +
>> > +no_packed:
>> > +       mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
>> > +       return reqs;
>> > +}
>> > +
>> > +static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
>> > +                              struct mmc_card *card,
>> > +                              struct mmc_queue *mq,
>> > +                              u8 reqs)
>> > +{
>> > +       struct mmc_blk_request *brq = &mqrq->brq;
>> > +       struct request *req = mqrq->req;
>> > +       struct request *prq;
>> > +       struct mmc_blk_data *md = mq->data;
>> > +       bool do_rel_wr;
>> > +       u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
>> > +       u8 i =1;
>> > +
>> > +       mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
>> > +               MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
>> > +       mqrq->packed_blocks = 0;
>> > +       mqrq->packed_fail_idx = -1;
>> > +
>> > +       memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
>> > +       packed_cmd_hdr[0] = (reqs << 16) |
>> > +               (((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
>> > +               PACKED_CMD_VER;
>> > +
>> > +       /*
>> > +        * Argument for each entry of packed group
>> > +        */
>> > +       list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
>> > +               do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
>> > +               /* Argument of CMD23*/
>> > +               packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
>> > +                       blk_rq_sectors(prq);
>> > +               /* Argument of CMD18 or CMD25 */
>> > +               packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
>> > +                       blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
>> > +               mqrq->packed_blocks += blk_rq_sectors(prq);
>> > +               i++;
>> > +       }
>> > +
>> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
>> > +       brq->mrq.cmd = &brq->cmd;
>> > +       brq->mrq.data = &brq->data;
>> > +       brq->mrq.sbc = &brq->sbc;
>> > +       brq->mrq.stop = &brq->stop;
>> > +
>> > +       brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
>> > +       brq->sbc.arg = MMC_CMD23_ARG_PACKED |
>> > +               ((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
>> > +       brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> > +
>> > +       brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
>> > +       brq->cmd.arg = blk_rq_pos(req);
>> > +       if (!mmc_card_blockaddr(card))
>> > +               brq->cmd.arg <<= 9;
>> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
>> > +
>> > +       brq->data.blksz = 512;
>> > +       /*
>> > +        * Write separately the packd command header only for packed read.
>> > +        * In case of packed write, header is sent with blocks of data.
>> > +        */
>> > +       brq->data.blocks = (rq_data_dir(req) == READ) ?
>> > +               1 : mqrq->packed_blocks + 1;
>> > +       brq->data.flags |= MMC_DATA_WRITE;
>> > +
>> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
>> > +       brq->stop.arg = 0;
>> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>> > +
>> > +       mmc_set_data_timeout(&brq->data, card);
>> > +
>> > +       brq->data.sg = mqrq->sg;
>> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
>> > +
>> > +       mqrq->mmc_active.mrq = &brq->mrq;
>> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
>> > +
>> > +       mmc_queue_bounce_pre(mqrq);
>> > +}
>> > +
>> > +static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
>> > +                              struct mmc_card *card,
>> > +                              struct mmc_queue *mq)
>> > +{
>> > +       struct mmc_blk_request *brq = &mqrq->brq;
>> > +       struct request *req = mqrq->req;
>> > +
>> > +       mqrq->packed_cmd = MMC_PACKED_READ;
>> > +
>> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
>> > +       brq->mrq.cmd = &brq->cmd;
>> > +       brq->mrq.data = &brq->data;
>> > +       brq->mrq.stop = &brq->stop;
>> > +
>> > +       brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
>> > +       brq->cmd.arg = blk_rq_pos(req);
>> > +       if (!mmc_card_blockaddr(card))
>> > +               brq->cmd.arg <<= 9;
>> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
>> > +       brq->data.blksz = 512;
>> > +       brq->data.blocks = mqrq->packed_blocks;
>> > +       brq->data.flags |= MMC_DATA_READ;
>> > +
>> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
>> > +       brq->stop.arg = 0;
>> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>> > +
>> > +       mmc_set_data_timeout(&brq->data, card);
>> > +
>> > +       brq->data.sg = mqrq->sg;
>> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
>> > +
>> > +       mqrq->mmc_active.mrq = &brq->mrq;
>> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
>> > +
>> > +       mmc_queue_bounce_pre(mqrq);
>> > +}
>> > +
>> >  static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
>> >                           struct mmc_blk_request *brq, struct request *req,
>> >                           int ret)
>> > @@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>> >        int ret = 1, disable_multi = 0, retry = 0, type;
>> >        enum mmc_blk_status status;
>> >        struct mmc_queue_req *mq_rq;
>> > -       struct request *req;
>> > +       struct request *req, *prq;
>> >        struct mmc_async_req *areq;
>> > +       u8 reqs = 0;
>> >
>> >        if (!rqc && !mq->mqrq_prev->req)
>> >                return 0;
>> >
>> > +       if (rqc)
>> > +               reqs = mmc_blk_chk_packable(mq, rqc);
>> > +
>> >        do {
>> >                if (rqc) {
>> > -                       mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>> > +                       if (reqs >= 2) {
>> > +                               mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
>> > +                               if (rq_data_dir(rqc) == READ) {
>> > +                                       areq = &mq->mqrq_cur->mmc_active;
>> > +                                       mmc_wait_for_req(card->host, areq->mrq);
>> > +                                       status = mmc_blk_packed_err_check(card, areq);
>> > +                                       if (status == MMC_BLK_SUCCESS) {
>> > +                                               mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
>> > +                                       } else {
>> > +                                               goto check_status;
>> > +                                       }
>> > +                               }
>> > +                       } else {
>>
>> IIUC, the code in mmc_blk_chk_packable
>> <snip>
>>        if (reqs > 0) {
>>                list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
>>               return (reqs + 1);
>> </snip>
>> adds the request to the packed list, and needs to be reversed if it's
>> decided to not to do a packed request ?
> Any condition or reason  not to do packed request after several requests
> is added in packed list? I didn't catch your comment exactly.
>
What I meant was that the request should be removed from queuelist
when you decide not to pack.

>>
>>
>> > +                               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>> > +                       }
>> >                        areq = &mq->mqrq_cur->mmc_active;
>> >                } else
>> >                        areq = NULL;
>> > @@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>> >                if (!areq)
>> >                        return 0;
>> >
>> > +check_status:
>> >                mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
>> >                brq = &mq_rq->brq;
>> >                req = mq_rq->req;
>> > @@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>> >                         * A block was successfully transferred.
>> >                         */
>> >                        mmc_blk_reset_success(md, type);
>> > -                       spin_lock_irq(&md->lock);
>> > -                       ret = __blk_end_request(req, 0,
>> > +
>> > +                       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>> > +                               int idx = mq_rq->packed_fail_idx, i = 0;
>> > +                               while (!list_empty(&mq_rq->packed_list)) {
>> > +                                       prq = list_entry_rq(mq_rq->packed_list.next);
>> > +                                       if (idx == i) {
>> > +                                               /* retry from error index */
>> > +                                               reqs -= idx;
>> > +                                               mq_rq->req = prq;
>> > +                                               ret = 1;
>> > +                                               break;
>> > +                                       }
>> > +                                       list_del_init(&prq->queuelist);
>> > +                                       spin_lock_irq(&md->lock);
>> > +                                       ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
>> > +                                       spin_unlock_irq(&md->lock);
>> > +                                       i++;
>> > +                               }
>> > +                               break;
>> > +                       } else {
>> > +                               spin_lock_irq(&md->lock);
>> > +                               ret = __blk_end_request(req, 0,
>> >                                                brq->data.bytes_xfered);
>> > -                       spin_unlock_irq(&md->lock);
>> > +                               spin_unlock_irq(&md->lock);
>> > +                       }
>> > +
>> >                        /*
>> >                         * If the blk_end_request function returns non-zero even
>> >                         * though all data has been transferred and no errors
>> > @@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>> >                        break;
>> >                }
>> >
>> > -               if (ret) {
>> > +               if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
>> >                        /*
>> >                         * In case of a incomplete request
>> >                         * prepare it again and resend.
>> > @@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>> >        return 1;
>> >
>> >  cmd_abort:
>> > -       spin_lock_irq(&md->lock);
>> > -       while (ret)
>> > -               ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
>> > -       spin_unlock_irq(&md->lock);
>> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
>> > +               spin_lock_irq(&md->lock);
>> > +               while (ret)
>> > +                       ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
>> > +               spin_unlock_irq(&md->lock);
>> > +       } else {
>> > +               while (!list_empty(&mq_rq->packed_list)) {
>> > +                       prq = list_entry_rq(mq_rq->packed_list.next);
>> > +                       list_del_init(&prq->queuelist);
>> > +                       spin_lock_irq(&md->lock);
>> > +                       __blk_end_request(prq, -EIO, blk_rq_bytes(prq));
>> > +                       spin_unlock_irq(&md->lock);
>> > +               }
>> > +       }
>> >
>> >  start_new_req:
>> >        if (rqc) {
>> > +               /*
>> > +                * If current request is packed, it need to put back.
>> > +                */
>> > +               if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
>> > +                       while (!list_empty(&mq->mqrq_cur->packed_list)) {
>> > +                               prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
>> > +                               if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
>> > +                                       list_del_init(&prq->queuelist);
>> > +                                       blk_requeue_request(mq->queue, prq);
>> > +                               } else {
>> > +                                       list_del_init(&prq->queuelist);
>> > +                               }
>> > +                       }
>> > +               }
>> >                mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>> >                mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
>> >        }
>> > diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> > index dcad59c..3a4542e 100644
>> > --- a/drivers/mmc/card/queue.c
>> > +++ b/drivers/mmc/card/queue.c
>> > @@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>> >
>> >        memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
>> >        memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
>> > +       INIT_LIST_HEAD(&mqrq_cur->packed_list);
>> > +       INIT_LIST_HEAD(&mqrq_prev->packed_list);
>> >        mq->mqrq_cur = mqrq_cur;
>> >        mq->mqrq_prev = mqrq_prev;
>> >        mq->queue->queuedata = mq;
>> > @@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
>> >        }
>> >  }
>> >
>> > +static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
>> > +                               struct mmc_queue_req *mqrq,
>> > +                               struct scatterlist *sg)
>> > +{
>> > +       struct scatterlist *__sg;
>> > +       unsigned int sg_len = 0;
>> > +       struct request *req;
>> > +       enum mmc_packed_cmd cmd;
>> > +
>> > +       cmd = mqrq->packed_cmd;
>> > +
>> > +       if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
>> > +               __sg =sg;
>> > +               sg_set_buf(__sg, mqrq->packed_cmd_hdr,
>> > +                               sizeof(mqrq->packed_cmd_hdr));
>> > +               sg_len++;
>> > +               if (cmd == MMC_PACKED_WR_HDR) {
>> > +                       sg_mark_end(__sg);
>> > +                       return sg_len;
>> > +               }
>> > +               __sg->page_link &= ~0x02;
>> > +       }
>> > +
>> > +       __sg = sg + sg_len;
>> > +       list_for_each_entry(req, &mqrq->packed_list, queuelist) {
>> > +               sg_len += blk_rq_map_sg(mq->queue, req, __sg);
>> > +               __sg = sg + (sg_len - 1);
>> > +               (__sg++)->page_link &= ~0x02;
>> > +       }
>> > +       sg_mark_end(sg + (sg_len - 1));
>> > +       return sg_len;
>> > +}
>> > +
>> >  /*
>> >  * Prepare the sg list(s) to be handed of to the host driver
>> >  */
>> > @@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
>> >        struct scatterlist *sg;
>> >        int i;
>> >
>> > -       if (!mqrq->bounce_buf)
>> > -               return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
>> > +       if (!mqrq->bounce_buf) {
>> > +               if (!list_empty(&mqrq->packed_list))
>> > +                       return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
>> > +               else
>> > +                       return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
>> > +       }
>> >
>> >        BUG_ON(!mqrq->bounce_sg);
>> >
>> > -       sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
>> > +       if (!list_empty(&mqrq->packed_list))
>> > +               sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
>> > +       else
>> > +               sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
>> >
>> >        mqrq->bounce_sg_len = sg_len;
>> >
>> > diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
>> > index d2a1eb4..5d0131e 100644
>> > --- a/drivers/mmc/card/queue.h
>> > +++ b/drivers/mmc/card/queue.h
>> > @@ -12,6 +12,13 @@ struct mmc_blk_request {
>> >        struct mmc_data         data;
>> >  };
>> >
>> > +enum mmc_packed_cmd {
>> > +       MMC_PACKED_NONE = 0,
>> > +       MMC_PACKED_WR_HDR,
>> > +       MMC_PACKED_WRITE,
>> > +       MMC_PACKED_READ,
>> > +};
>> > +
>> >  struct mmc_queue_req {
>> >        struct request          *req;
>> >        struct mmc_blk_request  brq;
>> > @@ -20,6 +27,11 @@ struct mmc_queue_req {
>> >        struct scatterlist      *bounce_sg;
>> >        unsigned int            bounce_sg_len;
>> >        struct mmc_async_req    mmc_active;
>> > +       struct list_head        packed_list;
>> > +       u32                     packed_cmd_hdr[128];
>> > +       unsigned int            packed_blocks;
>> > +       enum mmc_packed_cmd     packed_cmd;
>> > +       int             packed_fail_idx;
>> >  };
>> >
>> >  struct mmc_queue {
>> > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> > index 174a844..3c61d5a 100644
>> > --- a/include/linux/mmc/core.h
>> > +++ b/include/linux/mmc/core.h
>> > @@ -18,6 +18,8 @@ struct mmc_request;
>> >  struct mmc_command {
>> >        u32                     opcode;
>> >        u32                     arg;
>> > +#define MMC_CMD23_ARG_REL_WR   (1 << 31)
>> > +#define MMC_CMD23_ARG_PACKED   ((0 << 31) | (1 << 30))
>> >        u32                     resp[4];
>> >        unsigned int            flags;          /* expected response type */
>> >  #define MMC_RSP_PRESENT        (1 << 0)
>> > @@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
>> >  extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
>> >        struct mmc_command *, int);
>> >  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
>> > +extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
>>
>> Stray change / doesn't belong to this patch.
> Do you mean to split this in another patch?
>
Yes. In fact all the changes to the header files are independent of
the implementation, so they belong to a separate patch.

RE: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

S, Venkatraman <svenkatr@ti.com> wrote:
> On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > This patch supports packed command of eMMC4.5 device.
> > Several reads(or writes) can be grouped in packed command
> > and all data of the individual commands can be sent in a
> > single transfer on the bus.
> >
> > Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> > ---
> >  drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
> >  drivers/mmc/card/queue.c |   48 ++++++-
> >  drivers/mmc/card/queue.h |   12 ++
> >  include/linux/mmc/core.h |    3 +
> >  4 files changed, 404 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index a1cb21f..6c49656 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
> >  #define INAND_CMD38_ARG_SECTRIM1 0x81
> >  #define INAND_CMD38_ARG_SECTRIM2 0x88
> >
> > +#define mmc_req_rel_wr(req)    (((req->cmd_flags & REQ_FUA) || \
> > +                       (req->cmd_flags & REQ_META)) && \
> > +                       (rq_data_dir(req) == WRITE))
> > +#define PACKED_CMD_VER         0x01
> > +#define PACKED_CMD_RD          0x01
> > +#define PACKED_CMD_WR          0x02
> > +
> >  static DEFINE_MUTEX(block_mutex);
> >
> >  /*
> > @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
> >         * kind.  If it was a write, we may have transitioned to
> >         * program mode, which we have to wait for it to complete.
> >         */
> > -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
> > +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
> > +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
> >                u32 status;
> >                do {
> >                        int err = get_card_status(card, &status, 5);
> > @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
> >        if (!brq->data.bytes_xfered)
> >                return MMC_BLK_RETRY;
> >
> > +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> > +               if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
> > +                       return MMC_BLK_PARTIAL;
> > +               else
> > +                       return MMC_BLK_SUCCESS;
> > +       }
> > +
> >        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
> >                return MMC_BLK_PARTIAL;
> >
> >        return MMC_BLK_SUCCESS;
> >  }
> >
> > +static int mmc_blk_packed_err_check(struct mmc_card *card,
> > +                            struct mmc_async_req *areq)
> > +{
> > +       struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
> > +                                                   mmc_active);
> > +       int err, check, status;
> > +       u8 ext_csd[512];
> > +
> > +       check = mmc_blk_err_check(card, areq);
> > +
> > +       if (check == MMC_BLK_SUCCESS)
> > +               return check;
> > +
> > +       if (check == MMC_BLK_PARTIAL) {
> > +               err = get_card_status(card, &status, 0);
> > +               if (err)
> > +                       return MMC_BLK_ABORT;
> > +
> > +               if (status & R1_EXP_EVENT) {
> > +                       err = mmc_send_ext_csd(card, ext_csd);
> > +                       if (err)
> > +                               return MMC_BLK_ABORT;
> > +
> > +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
> > +                                               EXT_CSD_PACKED_FAILURE) &&
> > +                                       (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> > +                                        EXT_CSD_PACKED_GENERIC_ERROR)) {
> > +                               if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> > +                                               EXT_CSD_PACKED_INDEXED_ERROR) {
> > +                                       /* Make be 0-based */
> > +                                       mq_mrq->packed_fail_idx =
> > +                                               ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
> > +                                       return MMC_BLK_PARTIAL;
> > +                               } else {
> > +                                       return MMC_BLK_RETRY;
> > +                               }
> > +                       }
> > +               } else {
> > +                       return MMC_BLK_RETRY;
> > +               }
> > +       }
> > +
> > +       if (check != MMC_BLK_ABORT)
> > +               return MMC_BLK_RETRY;
> > +       else
> > +               return MMC_BLK_ABORT;
> > +}
> > +
> >  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> >                               struct mmc_card *card,
> >                               int disable_multi,
> > @@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> >        mmc_queue_bounce_pre(mqrq);
> >  }
> >
> > +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
> > +{
> > +       struct request_queue *q = mq->queue;
> > +       struct mmc_card *card = mq->card;
> > +       struct request *cur = req, *next = NULL;
> > +       struct mmc_blk_data *md = mq->data;
> > +       bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> > +       unsigned int req_sectors = 0, phys_segments = 0;
> > +       unsigned int max_blk_count, max_phys_segs;
> > +       u8 max_packed_rw = 0;
> > +       u8 reqs = 0;
> > +
> > +       if (!(md->flags & MMC_BLK_CMD23) &&
> > +                       !card->ext_csd.packed_event_en)
> > +               goto no_packed;
> > +
> > +       if (rq_data_dir(cur) == READ)
> > +               max_packed_rw = card->ext_csd.max_packed_reads;
> > +       else
> > +               max_packed_rw = card->ext_csd.max_packed_writes;
> > +
> > +       if (max_packed_rw == 0)
> > +               goto no_packed;
> > +
> > +       if (mmc_req_rel_wr(cur) &&
> > +                       (md->flags & MMC_BLK_REL_WR) &&
> > +                       !en_rel_wr) {
> > +               goto no_packed;
> > +       }
> > +
> > +       max_blk_count = min(card->host->max_blk_count,
> > +                       card->host->max_req_size >> 9);
> > +       if (unlikely(max_blk_count > 0xffff))
> > +               max_blk_count = 0xffff;
> > +
> > +       max_phys_segs = queue_max_segments(q);
> > +       req_sectors += blk_rq_sectors(cur);
> > +       phys_segments += req->nr_phys_segments;
> > +
> > +       if (rq_data_dir(cur) == WRITE) {
> > +               req_sectors++;
> > +               phys_segments++;
> > +       }
> > +
> > +       while (reqs < max_packed_rw - 1) {
> > +               next = blk_fetch_request(q);
> > +               if (!next)
> > +                       break;
> > +
> > +               if (rq_data_dir(cur) != rq_data_dir(next)) {
> > +                       blk_requeue_request(q, next);
> > +                       break;
> > +               }
> > +
> > +               if (mmc_req_rel_wr(next) &&
> > +                               (md->flags & MMC_BLK_REL_WR) &&
> > +                               !en_rel_wr) {
> > +                       blk_requeue_request(q, next);
> > +                       break;
> > +               }
> > +
> > +               req_sectors += blk_rq_sectors(next);
> > +               if (req_sectors > max_blk_count) {
> > +                       blk_requeue_request(q, next);
> > +                       break;
> > +               }
> > +
> > +               phys_segments +=  next->nr_phys_segments;
> > +               if (phys_segments > max_phys_segs) {
> > +                       blk_requeue_request(q, next);
> > +                       break;
> > +               }
> I mentioned this before - if the next request is not packable and requeued,
> blk_fetch_request will retrieve it again and this while loop will
> never terminate.
> 
If next request is not packable, it is requeued and 'break' terminates this loop.
So it not infinite.
> > +
> > +               list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
> > +               cur = next;
> > +               reqs++;
> > +       }
> > +
> > +       if (reqs > 0) {
> > +               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
> > +               return (reqs + 1);
> > +       }
> > +
> > +no_packed:
> > +       mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
> > +       return reqs;
> > +}
> > +
> > +static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
> > +                              struct mmc_card *card,
> > +                              struct mmc_queue *mq,
> > +                              u8 reqs)
> > +{
> > +       struct mmc_blk_request *brq = &mqrq->brq;
> > +       struct request *req = mqrq->req;
> > +       struct request *prq;
> > +       struct mmc_blk_data *md = mq->data;
> > +       bool do_rel_wr;
> > +       u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
> > +       u8 i =1;
> > +
> > +       mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
> > +               MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
> > +       mqrq->packed_blocks = 0;
> > +       mqrq->packed_fail_idx = -1;
> > +
> > +       memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
> > +       packed_cmd_hdr[0] = (reqs << 16) |
> > +               (((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
> > +               PACKED_CMD_VER;
> > +
> > +       /*
> > +        * Argument for each entry of packed group
> > +        */
> > +       list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
> > +               do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
> > +               /* Argument of CMD23*/
> > +               packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
> > +                       blk_rq_sectors(prq);
> > +               /* Argument of CMD18 or CMD25 */
> > +               packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
> > +                       blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
> > +               mqrq->packed_blocks += blk_rq_sectors(prq);
> > +               i++;
> > +       }
> > +
> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
> > +       brq->mrq.cmd = &brq->cmd;
> > +       brq->mrq.data = &brq->data;
> > +       brq->mrq.sbc = &brq->sbc;
> > +       brq->mrq.stop = &brq->stop;
> > +
> > +       brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> > +       brq->sbc.arg = MMC_CMD23_ARG_PACKED |
> > +               ((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
> > +       brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> > +
> > +       brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
> > +       brq->cmd.arg = blk_rq_pos(req);
> > +       if (!mmc_card_blockaddr(card))
> > +               brq->cmd.arg <<= 9;
> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > +
> > +       brq->data.blksz = 512;
> > +       /*
> > +        * Write separately the packd command header only for packed read.
> > +        * In case of packed write, header is sent with blocks of data.
> > +        */
> > +       brq->data.blocks = (rq_data_dir(req) == READ) ?
> > +               1 : mqrq->packed_blocks + 1;
> > +       brq->data.flags |= MMC_DATA_WRITE;
> > +
> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> > +       brq->stop.arg = 0;
> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> > +
> > +       mmc_set_data_timeout(&brq->data, card);
> > +
> > +       brq->data.sg = mqrq->sg;
> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> > +
> > +       mqrq->mmc_active.mrq = &brq->mrq;
> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> > +
> > +       mmc_queue_bounce_pre(mqrq);
> > +}
> > +
> > +static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
> > +                              struct mmc_card *card,
> > +                              struct mmc_queue *mq)
> > +{
> > +       struct mmc_blk_request *brq = &mqrq->brq;
> > +       struct request *req = mqrq->req;
> > +
> > +       mqrq->packed_cmd = MMC_PACKED_READ;
> > +
> > +       memset(brq, 0, sizeof(struct mmc_blk_request));
> > +       brq->mrq.cmd = &brq->cmd;
> > +       brq->mrq.data = &brq->data;
> > +       brq->mrq.stop = &brq->stop;
> > +
> > +       brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> > +       brq->cmd.arg = blk_rq_pos(req);
> > +       if (!mmc_card_blockaddr(card))
> > +               brq->cmd.arg <<= 9;
> > +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> > +       brq->data.blksz = 512;
> > +       brq->data.blocks = mqrq->packed_blocks;
> > +       brq->data.flags |= MMC_DATA_READ;
> > +
> > +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> > +       brq->stop.arg = 0;
> > +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> > +
> > +       mmc_set_data_timeout(&brq->data, card);
> > +
> > +       brq->data.sg = mqrq->sg;
> > +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> > +
> > +       mqrq->mmc_active.mrq = &brq->mrq;
> > +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> > +
> > +       mmc_queue_bounce_pre(mqrq);
> > +}
> > +
> >  static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
> >                           struct mmc_blk_request *brq, struct request *req,
> >                           int ret)
> > @@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >        int ret = 1, disable_multi = 0, retry = 0, type;
> >        enum mmc_blk_status status;
> >        struct mmc_queue_req *mq_rq;
> > -       struct request *req;
> > +       struct request *req, *prq;
> >        struct mmc_async_req *areq;
> > +       u8 reqs = 0;
> >
> >        if (!rqc && !mq->mqrq_prev->req)
> >                return 0;
> >
> > +       if (rqc)
> > +               reqs = mmc_blk_chk_packable(mq, rqc);
> > +
> >        do {
> >                if (rqc) {
> > -                       mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> > +                       if (reqs >= 2) {
> > +                               mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
> > +                               if (rq_data_dir(rqc) == READ) {
> > +                                       areq = &mq->mqrq_cur->mmc_active;
> > +                                       mmc_wait_for_req(card->host, areq->mrq);
> > +                                       status = mmc_blk_packed_err_check(card, areq);
> > +                                       if (status == MMC_BLK_SUCCESS) {
> > +                                               mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
> > +                                       } else {
> > +                                               goto check_status;
> > +                                       }
> > +                               }
> > +                       } else {
> 
> IIUC, the code in mmc_blk_chk_packable
> <snip>
>        if (reqs > 0) {
>                list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
>               return (reqs + 1);
> </snip>
> adds the request to the packed list, and needs to be reversed if it's
> decided to not to do a packed request ?
Any condition or reason  not to do packed request after several requests
is added in packed list? I didn't catch your comment exactly.

> 
> 
> > +                               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> > +                       }
> >                        areq = &mq->mqrq_cur->mmc_active;
> >                } else
> >                        areq = NULL;
> > @@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >                if (!areq)
> >                        return 0;
> >
> > +check_status:
> >                mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
> >                brq = &mq_rq->brq;
> >                req = mq_rq->req;
> > @@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >                         * A block was successfully transferred.
> >                         */
> >                        mmc_blk_reset_success(md, type);
> > -                       spin_lock_irq(&md->lock);
> > -                       ret = __blk_end_request(req, 0,
> > +
> > +                       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> > +                               int idx = mq_rq->packed_fail_idx, i = 0;
> > +                               while (!list_empty(&mq_rq->packed_list)) {
> > +                                       prq = list_entry_rq(mq_rq->packed_list.next);
> > +                                       if (idx == i) {
> > +                                               /* retry from error index */
> > +                                               reqs -= idx;
> > +                                               mq_rq->req = prq;
> > +                                               ret = 1;
> > +                                               break;
> > +                                       }
> > +                                       list_del_init(&prq->queuelist);
> > +                                       spin_lock_irq(&md->lock);
> > +                                       ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
> > +                                       spin_unlock_irq(&md->lock);
> > +                                       i++;
> > +                               }
> > +                               break;
> > +                       } else {
> > +                               spin_lock_irq(&md->lock);
> > +                               ret = __blk_end_request(req, 0,
> >                                                brq->data.bytes_xfered);
> > -                       spin_unlock_irq(&md->lock);
> > +                               spin_unlock_irq(&md->lock);
> > +                       }
> > +
> >                        /*
> >                         * If the blk_end_request function returns non-zero even
> >                         * though all data has been transferred and no errors
> > @@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >                        break;
> >                }
> >
> > -               if (ret) {
> > +               if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
> >                        /*
> >                         * In case of a incomplete request
> >                         * prepare it again and resend.
> > @@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> >        return 1;
> >
> >  cmd_abort:
> > -       spin_lock_irq(&md->lock);
> > -       while (ret)
> > -               ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> > -       spin_unlock_irq(&md->lock);
> > +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> > +               spin_lock_irq(&md->lock);
> > +               while (ret)
> > +                       ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> > +               spin_unlock_irq(&md->lock);
> > +       } else {
> > +               while (!list_empty(&mq_rq->packed_list)) {
> > +                       prq = list_entry_rq(mq_rq->packed_list.next);
> > +                       list_del_init(&prq->queuelist);
> > +                       spin_lock_irq(&md->lock);
> > +                       __blk_end_request(prq, -EIO, blk_rq_bytes(prq));
> > +                       spin_unlock_irq(&md->lock);
> > +               }
> > +       }
> >
> >  start_new_req:
> >        if (rqc) {
> > +               /*
> > +                * If current request is packed, it need to put back.
> > +                */
> > +               if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
> > +                       while (!list_empty(&mq->mqrq_cur->packed_list)) {
> > +                               prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
> > +                               if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
> > +                                       list_del_init(&prq->queuelist);
> > +                                       blk_requeue_request(mq->queue, prq);
> > +                               } else {
> > +                                       list_del_init(&prq->queuelist);
> > +                               }
> > +                       }
> > +               }
> >                mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> >                mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
> >        }
> > diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> > index dcad59c..3a4542e 100644
> > --- a/drivers/mmc/card/queue.c
> > +++ b/drivers/mmc/card/queue.c
> > @@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
> >
> >        memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
> >        memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
> > +       INIT_LIST_HEAD(&mqrq_cur->packed_list);
> > +       INIT_LIST_HEAD(&mqrq_prev->packed_list);
> >        mq->mqrq_cur = mqrq_cur;
> >        mq->mqrq_prev = mqrq_prev;
> >        mq->queue->queuedata = mq;
> > @@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
> >        }
> >  }
> >
> > +static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
> > +                               struct mmc_queue_req *mqrq,
> > +                               struct scatterlist *sg)
> > +{
> > +       struct scatterlist *__sg;
> > +       unsigned int sg_len = 0;
> > +       struct request *req;
> > +       enum mmc_packed_cmd cmd;
> > +
> > +       cmd = mqrq->packed_cmd;
> > +
> > +       if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
> > +               __sg =sg;
> > +               sg_set_buf(__sg, mqrq->packed_cmd_hdr,
> > +                               sizeof(mqrq->packed_cmd_hdr));
> > +               sg_len++;
> > +               if (cmd == MMC_PACKED_WR_HDR) {
> > +                       sg_mark_end(__sg);
> > +                       return sg_len;
> > +               }
> > +               __sg->page_link &= ~0x02;
> > +       }
> > +
> > +       __sg = sg + sg_len;
> > +       list_for_each_entry(req, &mqrq->packed_list, queuelist) {
> > +               sg_len += blk_rq_map_sg(mq->queue, req, __sg);
> > +               __sg = sg + (sg_len - 1);
> > +               (__sg++)->page_link &= ~0x02;
> > +       }
> > +       sg_mark_end(sg + (sg_len - 1));
> > +       return sg_len;
> > +}
> > +
> >  /*
> >  * Prepare the sg list(s) to be handed of to the host driver
> >  */
> > @@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
> >        struct scatterlist *sg;
> >        int i;
> >
> > -       if (!mqrq->bounce_buf)
> > -               return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> > +       if (!mqrq->bounce_buf) {
> > +               if (!list_empty(&mqrq->packed_list))
> > +                       return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
> > +               else
> > +                       return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> > +       }
> >
> >        BUG_ON(!mqrq->bounce_sg);
> >
> > -       sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> > +       if (!list_empty(&mqrq->packed_list))
> > +               sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
> > +       else
> > +               sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> >
> >        mqrq->bounce_sg_len = sg_len;
> >
> > diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> > index d2a1eb4..5d0131e 100644
> > --- a/drivers/mmc/card/queue.h
> > +++ b/drivers/mmc/card/queue.h
> > @@ -12,6 +12,13 @@ struct mmc_blk_request {
> >        struct mmc_data         data;
> >  };
> >
> > +enum mmc_packed_cmd {
> > +       MMC_PACKED_NONE = 0,
> > +       MMC_PACKED_WR_HDR,
> > +       MMC_PACKED_WRITE,
> > +       MMC_PACKED_READ,
> > +};
> > +
> >  struct mmc_queue_req {
> >        struct request          *req;
> >        struct mmc_blk_request  brq;
> > @@ -20,6 +27,11 @@ struct mmc_queue_req {
> >        struct scatterlist      *bounce_sg;
> >        unsigned int            bounce_sg_len;
> >        struct mmc_async_req    mmc_active;
> > +       struct list_head        packed_list;
> > +       u32                     packed_cmd_hdr[128];
> > +       unsigned int            packed_blocks;
> > +       enum mmc_packed_cmd     packed_cmd;
> > +       int             packed_fail_idx;
> >  };
> >
> >  struct mmc_queue {
> > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> > index 174a844..3c61d5a 100644
> > --- a/include/linux/mmc/core.h
> > +++ b/include/linux/mmc/core.h
> > @@ -18,6 +18,8 @@ struct mmc_request;
> >  struct mmc_command {
> >        u32                     opcode;
> >        u32                     arg;
> > +#define MMC_CMD23_ARG_REL_WR   (1 << 31)
> > +#define MMC_CMD23_ARG_PACKED   ((0 << 31) | (1 << 30))
> >        u32                     resp[4];
> >        unsigned int            flags;          /* expected response type */
> >  #define MMC_RSP_PRESENT        (1 << 0)
> > @@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
> >  extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
> >        struct mmc_command *, int);
> >  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
> > +extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
> 
> Stray change / doesn't belong to this patch.
Do you mean to split this in another patch?

Thanks,
Seungwon Jeon.
> 
> >
> >  #define MMC_ERASE_ARG          0x00000000
> >  #define MMC_SECURE_ERASE_ARG   0x80000000
> > --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[S, Venkatraman]

On Wed, Nov 2, 2011 at 1:33 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> This patch supports packed command of eMMC4.5 device.
> Several reads(or writes) can be grouped in packed command
> and all data of the individual commands can be sent in a
> single transfer on the bus.
>
> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> ---
>  drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/mmc/card/queue.c |   48 ++++++-
>  drivers/mmc/card/queue.h |   12 ++
>  include/linux/mmc/core.h |    3 +
>  4 files changed, 404 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index a1cb21f..6c49656 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
>  #define INAND_CMD38_ARG_SECTRIM1 0x81
>  #define INAND_CMD38_ARG_SECTRIM2 0x88
>
> +#define mmc_req_rel_wr(req)    (((req->cmd_flags & REQ_FUA) || \
> +                       (req->cmd_flags & REQ_META)) && \
> +                       (rq_data_dir(req) == WRITE))
> +#define PACKED_CMD_VER         0x01
> +#define PACKED_CMD_RD          0x01
> +#define PACKED_CMD_WR          0x02
> +
>  static DEFINE_MUTEX(block_mutex);
>
>  /*
> @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
>         * kind.  If it was a write, we may have transitioned to
>         * program mode, which we have to wait for it to complete.
>         */
> -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
> +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
> +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
>                u32 status;
>                do {
>                        int err = get_card_status(card, &status, 5);
> @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
>        if (!brq->data.bytes_xfered)
>                return MMC_BLK_RETRY;
>
> +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> +               if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
> +                       return MMC_BLK_PARTIAL;
> +               else
> +                       return MMC_BLK_SUCCESS;
> +       }
> +
>        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
>                return MMC_BLK_PARTIAL;
>
>        return MMC_BLK_SUCCESS;
>  }
>
> +static int mmc_blk_packed_err_check(struct mmc_card *card,
> +                            struct mmc_async_req *areq)
> +{
> +       struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
> +                                                   mmc_active);
> +       int err, check, status;
> +       u8 ext_csd[512];
> +
> +       check = mmc_blk_err_check(card, areq);
> +
> +       if (check == MMC_BLK_SUCCESS)
> +               return check;
> +
> +       if (check == MMC_BLK_PARTIAL) {
> +               err = get_card_status(card, &status, 0);
> +               if (err)
> +                       return MMC_BLK_ABORT;
> +
> +               if (status & R1_EXP_EVENT) {
> +                       err = mmc_send_ext_csd(card, ext_csd);
> +                       if (err)
> +                               return MMC_BLK_ABORT;
> +
> +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
> +                                               EXT_CSD_PACKED_FAILURE) &&
> +                                       (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> +                                        EXT_CSD_PACKED_GENERIC_ERROR)) {
> +                               if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> +                                               EXT_CSD_PACKED_INDEXED_ERROR) {
> +                                       /* Make be 0-based */
> +                                       mq_mrq->packed_fail_idx =
> +                                               ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
> +                                       return MMC_BLK_PARTIAL;
> +                               } else {
> +                                       return MMC_BLK_RETRY;
> +                               }
> +                       }
> +               } else {
> +                       return MMC_BLK_RETRY;
> +               }
> +       }
> +
> +       if (check != MMC_BLK_ABORT)
> +               return MMC_BLK_RETRY;
> +       else
> +               return MMC_BLK_ABORT;
> +}
> +
>  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>                               struct mmc_card *card,
>                               int disable_multi,
> @@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>        mmc_queue_bounce_pre(mqrq);
>  }
>
> +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
> +{
> +       struct request_queue *q = mq->queue;
> +       struct mmc_card *card = mq->card;
> +       struct request *cur = req, *next = NULL;
> +       struct mmc_blk_data *md = mq->data;
> +       bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> +       unsigned int req_sectors = 0, phys_segments = 0;
> +       unsigned int max_blk_count, max_phys_segs;
> +       u8 max_packed_rw = 0;
> +       u8 reqs = 0;
> +
> +       if (!(md->flags & MMC_BLK_CMD23) &&
> +                       !card->ext_csd.packed_event_en)
> +               goto no_packed;
> +
> +       if (rq_data_dir(cur) == READ)
> +               max_packed_rw = card->ext_csd.max_packed_reads;
> +       else
> +               max_packed_rw = card->ext_csd.max_packed_writes;
> +
> +       if (max_packed_rw == 0)
> +               goto no_packed;
> +
> +       if (mmc_req_rel_wr(cur) &&
> +                       (md->flags & MMC_BLK_REL_WR) &&
> +                       !en_rel_wr) {
> +               goto no_packed;
> +       }
> +
> +       max_blk_count = min(card->host->max_blk_count,
> +                       card->host->max_req_size >> 9);
> +       if (unlikely(max_blk_count > 0xffff))
> +               max_blk_count = 0xffff;
> +
> +       max_phys_segs = queue_max_segments(q);
> +       req_sectors += blk_rq_sectors(cur);
> +       phys_segments += req->nr_phys_segments;
> +
> +       if (rq_data_dir(cur) == WRITE) {
> +               req_sectors++;
> +               phys_segments++;
> +       }
> +
> +       while (reqs < max_packed_rw - 1) {
> +               next = blk_fetch_request(q);
> +               if (!next)
> +                       break;
> +
> +               if (rq_data_dir(cur) != rq_data_dir(next)) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               if (mmc_req_rel_wr(next) &&
> +                               (md->flags & MMC_BLK_REL_WR) &&
> +                               !en_rel_wr) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               req_sectors += blk_rq_sectors(next);
> +               if (req_sectors > max_blk_count) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               phys_segments +=  next->nr_phys_segments;
> +               if (phys_segments > max_phys_segs) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
I mentioned this before - if the next request is not packable and requeued,
blk_fetch_request will retrieve it again and this while loop will
never terminate.

> +
> +               list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
> +               cur = next;
> +               reqs++;
> +       }
> +
> +       if (reqs > 0) {
> +               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
> +               return (reqs + 1);
> +       }
> +
> +no_packed:
> +       mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
> +       return reqs;
> +}
> +
> +static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
> +                              struct mmc_card *card,
> +                              struct mmc_queue *mq,
> +                              u8 reqs)
> +{
> +       struct mmc_blk_request *brq = &mqrq->brq;
> +       struct request *req = mqrq->req;
> +       struct request *prq;
> +       struct mmc_blk_data *md = mq->data;
> +       bool do_rel_wr;
> +       u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
> +       u8 i =1;
> +
> +       mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
> +               MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
> +       mqrq->packed_blocks = 0;
> +       mqrq->packed_fail_idx = -1;
> +
> +       memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
> +       packed_cmd_hdr[0] = (reqs << 16) |
> +               (((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
> +               PACKED_CMD_VER;
> +
> +       /*
> +        * Argument for each entry of packed group
> +        */
> +       list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
> +               do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
> +               /* Argument of CMD23*/
> +               packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
> +                       blk_rq_sectors(prq);
> +               /* Argument of CMD18 or CMD25 */
> +               packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
> +                       blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
> +               mqrq->packed_blocks += blk_rq_sectors(prq);
> +               i++;
> +       }
> +
> +       memset(brq, 0, sizeof(struct mmc_blk_request));
> +       brq->mrq.cmd = &brq->cmd;
> +       brq->mrq.data = &brq->data;
> +       brq->mrq.sbc = &brq->sbc;
> +       brq->mrq.stop = &brq->stop;
> +
> +       brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> +       brq->sbc.arg = MMC_CMD23_ARG_PACKED |
> +               ((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
> +       brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> +       brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
> +       brq->cmd.arg = blk_rq_pos(req);
> +       if (!mmc_card_blockaddr(card))
> +               brq->cmd.arg <<= 9;
> +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       brq->data.blksz = 512;
> +       /*
> +        * Write separately the packd command header only for packed read.
> +        * In case of packed write, header is sent with blocks of data.
> +        */
> +       brq->data.blocks = (rq_data_dir(req) == READ) ?
> +               1 : mqrq->packed_blocks + 1;
> +       brq->data.flags |= MMC_DATA_WRITE;
> +
> +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> +       brq->stop.arg = 0;
> +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +
> +       mmc_set_data_timeout(&brq->data, card);
> +
> +       brq->data.sg = mqrq->sg;
> +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> +
> +       mqrq->mmc_active.mrq = &brq->mrq;
> +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> +
> +       mmc_queue_bounce_pre(mqrq);
> +}
> +
> +static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
> +                              struct mmc_card *card,
> +                              struct mmc_queue *mq)
> +{
> +       struct mmc_blk_request *brq = &mqrq->brq;
> +       struct request *req = mqrq->req;
> +
> +       mqrq->packed_cmd = MMC_PACKED_READ;
> +
> +       memset(brq, 0, sizeof(struct mmc_blk_request));
> +       brq->mrq.cmd = &brq->cmd;
> +       brq->mrq.data = &brq->data;
> +       brq->mrq.stop = &brq->stop;
> +
> +       brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> +       brq->cmd.arg = blk_rq_pos(req);
> +       if (!mmc_card_blockaddr(card))
> +               brq->cmd.arg <<= 9;
> +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +       brq->data.blksz = 512;
> +       brq->data.blocks = mqrq->packed_blocks;
> +       brq->data.flags |= MMC_DATA_READ;
> +
> +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> +       brq->stop.arg = 0;
> +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +
> +       mmc_set_data_timeout(&brq->data, card);
> +
> +       brq->data.sg = mqrq->sg;
> +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> +
> +       mqrq->mmc_active.mrq = &brq->mrq;
> +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> +
> +       mmc_queue_bounce_pre(mqrq);
> +}
> +
>  static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
>                           struct mmc_blk_request *brq, struct request *req,
>                           int ret)
> @@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>        int ret = 1, disable_multi = 0, retry = 0, type;
>        enum mmc_blk_status status;
>        struct mmc_queue_req *mq_rq;
> -       struct request *req;
> +       struct request *req, *prq;
>        struct mmc_async_req *areq;
> +       u8 reqs = 0;
>
>        if (!rqc && !mq->mqrq_prev->req)
>                return 0;
>
> +       if (rqc)
> +               reqs = mmc_blk_chk_packable(mq, rqc);
> +
>        do {
>                if (rqc) {
> -                       mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> +                       if (reqs >= 2) {
> +                               mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
> +                               if (rq_data_dir(rqc) == READ) {
> +                                       areq = &mq->mqrq_cur->mmc_active;
> +                                       mmc_wait_for_req(card->host, areq->mrq);
> +                                       status = mmc_blk_packed_err_check(card, areq);
> +                                       if (status == MMC_BLK_SUCCESS) {
> +                                               mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
> +                                       } else {
> +                                               goto check_status;
> +                                       }
> +                               }
> +                       } else {

IIUC, the code in mmc_blk_chk_packable
<snip>
       if (reqs > 0) {
               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
              return (reqs + 1);
</snip>
adds the request to the packed list, and needs to be reversed if it's
decided to not to do a packed request ?


> +                               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> +                       }
>                        areq = &mq->mqrq_cur->mmc_active;
>                } else
>                        areq = NULL;
> @@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                if (!areq)
>                        return 0;
>
> +check_status:
>                mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
>                brq = &mq_rq->brq;
>                req = mq_rq->req;
> @@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                         * A block was successfully transferred.
>                         */
>                        mmc_blk_reset_success(md, type);
> -                       spin_lock_irq(&md->lock);
> -                       ret = __blk_end_request(req, 0,
> +
> +                       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> +                               int idx = mq_rq->packed_fail_idx, i = 0;
> +                               while (!list_empty(&mq_rq->packed_list)) {
> +                                       prq = list_entry_rq(mq_rq->packed_list.next);
> +                                       if (idx == i) {
> +                                               /* retry from error index */
> +                                               reqs -= idx;
> +                                               mq_rq->req = prq;
> +                                               ret = 1;
> +                                               break;
> +                                       }
> +                                       list_del_init(&prq->queuelist);
> +                                       spin_lock_irq(&md->lock);
> +                                       ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
> +                                       spin_unlock_irq(&md->lock);
> +                                       i++;
> +                               }
> +                               break;
> +                       } else {
> +                               spin_lock_irq(&md->lock);
> +                               ret = __blk_end_request(req, 0,
>                                                brq->data.bytes_xfered);
> -                       spin_unlock_irq(&md->lock);
> +                               spin_unlock_irq(&md->lock);
> +                       }
> +
>                        /*
>                         * If the blk_end_request function returns non-zero even
>                         * though all data has been transferred and no errors
> @@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                        break;
>                }
>
> -               if (ret) {
> +               if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
>                        /*
>                         * In case of a incomplete request
>                         * prepare it again and resend.
> @@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>        return 1;
>
>  cmd_abort:
> -       spin_lock_irq(&md->lock);
> -       while (ret)
> -               ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> -       spin_unlock_irq(&md->lock);
> +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> +               spin_lock_irq(&md->lock);
> +               while (ret)
> +                       ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> +               spin_unlock_irq(&md->lock);
> +       } else {
> +               while (!list_empty(&mq_rq->packed_list)) {
> +                       prq = list_entry_rq(mq_rq->packed_list.next);
> +                       list_del_init(&prq->queuelist);
> +                       spin_lock_irq(&md->lock);
> +                       __blk_end_request(prq, -EIO, blk_rq_bytes(prq));
> +                       spin_unlock_irq(&md->lock);
> +               }
> +       }
>
>  start_new_req:
>        if (rqc) {
> +               /*
> +                * If current request is packed, it need to put back.
> +                */
> +               if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
> +                       while (!list_empty(&mq->mqrq_cur->packed_list)) {
> +                               prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
> +                               if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
> +                                       list_del_init(&prq->queuelist);
> +                                       blk_requeue_request(mq->queue, prq);
> +                               } else {
> +                                       list_del_init(&prq->queuelist);
> +                               }
> +                       }
> +               }
>                mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>                mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
>        }
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index dcad59c..3a4542e 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>
>        memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
>        memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
> +       INIT_LIST_HEAD(&mqrq_cur->packed_list);
> +       INIT_LIST_HEAD(&mqrq_prev->packed_list);
>        mq->mqrq_cur = mqrq_cur;
>        mq->mqrq_prev = mqrq_prev;
>        mq->queue->queuedata = mq;
> @@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
>        }
>  }
>
> +static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
> +                               struct mmc_queue_req *mqrq,
> +                               struct scatterlist *sg)
> +{
> +       struct scatterlist *__sg;
> +       unsigned int sg_len = 0;
> +       struct request *req;
> +       enum mmc_packed_cmd cmd;
> +
> +       cmd = mqrq->packed_cmd;
> +
> +       if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
> +               __sg =sg;
> +               sg_set_buf(__sg, mqrq->packed_cmd_hdr,
> +                               sizeof(mqrq->packed_cmd_hdr));
> +               sg_len++;
> +               if (cmd == MMC_PACKED_WR_HDR) {
> +                       sg_mark_end(__sg);
> +                       return sg_len;
> +               }
> +               __sg->page_link &= ~0x02;
> +       }
> +
> +       __sg = sg + sg_len;
> +       list_for_each_entry(req, &mqrq->packed_list, queuelist) {
> +               sg_len += blk_rq_map_sg(mq->queue, req, __sg);
> +               __sg = sg + (sg_len - 1);
> +               (__sg++)->page_link &= ~0x02;
> +       }
> +       sg_mark_end(sg + (sg_len - 1));
> +       return sg_len;
> +}
> +
>  /*
>  * Prepare the sg list(s) to be handed of to the host driver
>  */
> @@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
>        struct scatterlist *sg;
>        int i;
>
> -       if (!mqrq->bounce_buf)
> -               return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> +       if (!mqrq->bounce_buf) {
> +               if (!list_empty(&mqrq->packed_list))
> +                       return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
> +               else
> +                       return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> +       }
>
>        BUG_ON(!mqrq->bounce_sg);
>
> -       sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> +       if (!list_empty(&mqrq->packed_list))
> +               sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
> +       else
> +               sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
>
>        mqrq->bounce_sg_len = sg_len;
>
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index d2a1eb4..5d0131e 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -12,6 +12,13 @@ struct mmc_blk_request {
>        struct mmc_data         data;
>  };
>
> +enum mmc_packed_cmd {
> +       MMC_PACKED_NONE = 0,
> +       MMC_PACKED_WR_HDR,
> +       MMC_PACKED_WRITE,
> +       MMC_PACKED_READ,
> +};
> +
>  struct mmc_queue_req {
>        struct request          *req;
>        struct mmc_blk_request  brq;
> @@ -20,6 +27,11 @@ struct mmc_queue_req {
>        struct scatterlist      *bounce_sg;
>        unsigned int            bounce_sg_len;
>        struct mmc_async_req    mmc_active;
> +       struct list_head        packed_list;
> +       u32                     packed_cmd_hdr[128];
> +       unsigned int            packed_blocks;
> +       enum mmc_packed_cmd     packed_cmd;
> +       int             packed_fail_idx;
>  };
>
>  struct mmc_queue {
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 174a844..3c61d5a 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -18,6 +18,8 @@ struct mmc_request;
>  struct mmc_command {
>        u32                     opcode;
>        u32                     arg;
> +#define MMC_CMD23_ARG_REL_WR   (1 << 31)
> +#define MMC_CMD23_ARG_PACKED   ((0 << 31) | (1 << 30))
>        u32                     resp[4];
>        unsigned int            flags;          /* expected response type */
>  #define MMC_RSP_PRESENT        (1 << 0)
> @@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
>  extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
>        struct mmc_command *, int);
>  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
> +extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);

Stray change / doesn't belong to this patch.

>
>  #define MMC_ERASE_ARG          0x00000000
>  #define MMC_SECURE_ERASE_ARG   0x80000000
> --

Re: [PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Girish K S]

On 2 November 2011 04:03, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> This patch supports packed command of eMMC4.5 device.
> Several reads(or writes) can be grouped in packed command
> and all data of the individual commands can be sent in a
> single transfer on the bus.
>
> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> ---
>  drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/mmc/card/queue.c |   48 ++++++-
>  drivers/mmc/card/queue.h |   12 ++
>  include/linux/mmc/core.h |    3 +
>  4 files changed, 404 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index a1cb21f..6c49656 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
>  #define INAND_CMD38_ARG_SECTRIM1 0x81
>  #define INAND_CMD38_ARG_SECTRIM2 0x88
>
> +#define mmc_req_rel_wr(req)    (((req->cmd_flags & REQ_FUA) || \
> +                       (req->cmd_flags & REQ_META)) && \
> +                       (rq_data_dir(req) == WRITE))
> +#define PACKED_CMD_VER         0x01
> +#define PACKED_CMD_RD          0x01
> +#define PACKED_CMD_WR          0x02
> +
>  static DEFINE_MUTEX(block_mutex);
>
>  /*
> @@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
>         * kind.  If it was a write, we may have transitioned to
>         * program mode, which we have to wait for it to complete.
>         */
> -       if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
> +       if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
> +                       (mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
>                u32 status;
>                do {
>                        int err = get_card_status(card, &status, 5);
> @@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
>        if (!brq->data.bytes_xfered)
>                return MMC_BLK_RETRY;
>
> +       if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
> +               if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
> +                       return MMC_BLK_PARTIAL;
> +               else
> +                       return MMC_BLK_SUCCESS;
> +       }
> +
>        if (blk_rq_bytes(req) != brq->data.bytes_xfered)
>                return MMC_BLK_PARTIAL;
>
>        return MMC_BLK_SUCCESS;
>  }
>
> +static int mmc_blk_packed_err_check(struct mmc_card *card,
> +                            struct mmc_async_req *areq)
> +{
> +       struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
> +                                                   mmc_active);
> +       int err, check, status;
> +       u8 ext_csd[512];
> +
> +       check = mmc_blk_err_check(card, areq);
> +
> +       if (check == MMC_BLK_SUCCESS)
> +               return check;
> +
> +       if (check == MMC_BLK_PARTIAL) {
> +               err = get_card_status(card, &status, 0);
> +               if (err)
> +                       return MMC_BLK_ABORT;
> +
> +               if (status & R1_EXP_EVENT) {
> +                       err = mmc_send_ext_csd(card, ext_csd);
> +                       if (err)
> +                               return MMC_BLK_ABORT;
> +
> +                       if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
> +                                               EXT_CSD_PACKED_FAILURE) &&
> +                                       (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> +                                        EXT_CSD_PACKED_GENERIC_ERROR)) {
> +                               if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> +                                               EXT_CSD_PACKED_INDEXED_ERROR) {
> +                                       /* Make be 0-based */
> +                                       mq_mrq->packed_fail_idx =
> +                                               ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
> +                                       return MMC_BLK_PARTIAL;
> +                               } else {
> +                                       return MMC_BLK_RETRY;
> +                               }
> +                       }
> +               } else {
> +                       return MMC_BLK_RETRY;
> +               }
> +       }
> +
> +       if (check != MMC_BLK_ABORT)
> +               return MMC_BLK_RETRY;
> +       else
> +               return MMC_BLK_ABORT;
> +}
> +
>  static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>                               struct mmc_card *card,
>                               int disable_multi,
> @@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>        mmc_queue_bounce_pre(mqrq);
>  }
>
> +static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
> +{
> +       struct request_queue *q = mq->queue;
> +       struct mmc_card *card = mq->card;
> +       struct request *cur = req, *next = NULL;
> +       struct mmc_blk_data *md = mq->data;
> +       bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> +       unsigned int req_sectors = 0, phys_segments = 0;
> +       unsigned int max_blk_count, max_phys_segs;
> +       u8 max_packed_rw = 0;
> +       u8 reqs = 0;
> +
> +       if (!(md->flags & MMC_BLK_CMD23) &&
> +                       !card->ext_csd.packed_event_en)
> +               goto no_packed;
> +
> +       if (rq_data_dir(cur) == READ)
> +               max_packed_rw = card->ext_csd.max_packed_reads;
> +       else
> +               max_packed_rw = card->ext_csd.max_packed_writes;
> +
> +       if (max_packed_rw == 0)
> +               goto no_packed;
> +
> +       if (mmc_req_rel_wr(cur) &&
> +                       (md->flags & MMC_BLK_REL_WR) &&
> +                       !en_rel_wr) {
> +               goto no_packed;
> +       }
> +
> +       max_blk_count = min(card->host->max_blk_count,
> +                       card->host->max_req_size >> 9);
> +       if (unlikely(max_blk_count > 0xffff))
> +               max_blk_count = 0xffff;
> +
> +       max_phys_segs = queue_max_segments(q);
> +       req_sectors += blk_rq_sectors(cur);
> +       phys_segments += req->nr_phys_segments;
> +
> +       if (rq_data_dir(cur) == WRITE) {
> +               req_sectors++;
> +               phys_segments++;
> +       }
> +
> +       while (reqs < max_packed_rw - 1) {
> +               next = blk_fetch_request(q);
> +               if (!next)
> +                       break;
> +
> +               if (rq_data_dir(cur) != rq_data_dir(next)) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               if (mmc_req_rel_wr(next) &&
> +                               (md->flags & MMC_BLK_REL_WR) &&
> +                               !en_rel_wr) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               req_sectors += blk_rq_sectors(next);
> +               if (req_sectors > max_blk_count) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               phys_segments +=  next->nr_phys_segments;
> +               if (phys_segments > max_phys_segs) {
> +                       blk_requeue_request(q, next);
> +                       break;
> +               }
> +
> +               list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
> +               cur = next;
> +               reqs++;
> +       }
> +
> +       if (reqs > 0) {
> +               list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
> +               return (reqs + 1);
> +       }
> +
> +no_packed:
> +       mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
> +       return reqs;
> +}
> +
> +static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
> +                              struct mmc_card *card,
> +                              struct mmc_queue *mq,
> +                              u8 reqs)
> +{
> +       struct mmc_blk_request *brq = &mqrq->brq;
> +       struct request *req = mqrq->req;
> +       struct request *prq;
> +       struct mmc_blk_data *md = mq->data;
> +       bool do_rel_wr;
> +       u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
> +       u8 i =1;
> +
> +       mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
> +               MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
> +       mqrq->packed_blocks = 0;
> +       mqrq->packed_fail_idx = -1;
> +
> +       memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
> +       packed_cmd_hdr[0] = (reqs << 16) |
> +               (((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
> +               PACKED_CMD_VER;
> +
> +       /*
> +        * Argument for each entry of packed group
> +        */
> +       list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
> +               do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
> +               /* Argument of CMD23*/
> +               packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
> +                       blk_rq_sectors(prq);
> +               /* Argument of CMD18 or CMD25 */
> +               packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
> +                       blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
> +               mqrq->packed_blocks += blk_rq_sectors(prq);
> +               i++;
> +       }
> +
> +       memset(brq, 0, sizeof(struct mmc_blk_request));
> +       brq->mrq.cmd = &brq->cmd;
> +       brq->mrq.data = &brq->data;
> +       brq->mrq.sbc = &brq->sbc;
> +       brq->mrq.stop = &brq->stop;
> +
> +       brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> +       brq->sbc.arg = MMC_CMD23_ARG_PACKED |
> +               ((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
> +       brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> +       brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
> +       brq->cmd.arg = blk_rq_pos(req);
> +       if (!mmc_card_blockaddr(card))
> +               brq->cmd.arg <<= 9;
> +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +       brq->data.blksz = 512;
> +       /*
> +        * Write separately the packd command header only for packed read.
> +        * In case of packed write, header is sent with blocks of data.
> +        */
> +       brq->data.blocks = (rq_data_dir(req) == READ) ?
> +               1 : mqrq->packed_blocks + 1;
> +       brq->data.flags |= MMC_DATA_WRITE;
> +
> +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> +       brq->stop.arg = 0;
> +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +
> +       mmc_set_data_timeout(&brq->data, card);
> +
> +       brq->data.sg = mqrq->sg;
> +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> +
> +       mqrq->mmc_active.mrq = &brq->mrq;
> +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> +
> +       mmc_queue_bounce_pre(mqrq);
> +}
> +
> +static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
> +                              struct mmc_card *card,
> +                              struct mmc_queue *mq)
> +{
> +       struct mmc_blk_request *brq = &mqrq->brq;
> +       struct request *req = mqrq->req;
> +
> +       mqrq->packed_cmd = MMC_PACKED_READ;
> +
> +       memset(brq, 0, sizeof(struct mmc_blk_request));
> +       brq->mrq.cmd = &brq->cmd;
> +       brq->mrq.data = &brq->data;
> +       brq->mrq.stop = &brq->stop;
> +
> +       brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
> +       brq->cmd.arg = blk_rq_pos(req);
> +       if (!mmc_card_blockaddr(card))
> +               brq->cmd.arg <<= 9;
> +       brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +       brq->data.blksz = 512;
> +       brq->data.blocks = mqrq->packed_blocks;
> +       brq->data.flags |= MMC_DATA_READ;
> +
> +       brq->stop.opcode = MMC_STOP_TRANSMISSION;
> +       brq->stop.arg = 0;
> +       brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +
> +       mmc_set_data_timeout(&brq->data, card);
> +
> +       brq->data.sg = mqrq->sg;
> +       brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> +
> +       mqrq->mmc_active.mrq = &brq->mrq;
> +       mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> +
> +       mmc_queue_bounce_pre(mqrq);
> +}
> +
>  static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
>                           struct mmc_blk_request *brq, struct request *req,
>                           int ret)
> @@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>        int ret = 1, disable_multi = 0, retry = 0, type;
>        enum mmc_blk_status status;
>        struct mmc_queue_req *mq_rq;
> -       struct request *req;
> +       struct request *req, *prq;
>        struct mmc_async_req *areq;
> +       u8 reqs = 0;
>
>        if (!rqc && !mq->mqrq_prev->req)
>                return 0;
>
> +       if (rqc)
> +               reqs = mmc_blk_chk_packable(mq, rqc);
> +
>        do {
>                if (rqc) {
> -                       mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> +                       if (reqs >= 2) {
> +                               mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
> +                               if (rq_data_dir(rqc) == READ) {
> +                                       areq = &mq->mqrq_cur->mmc_active;
> +                                       mmc_wait_for_req(card->host, areq->mrq);
> +                                       status = mmc_blk_packed_err_check(card, areq);
> +                                       if (status == MMC_BLK_SUCCESS) {
> +                                               mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
> +                                       } else {
> +                                               goto check_status;
> +                                       }
> +                               }
> +                       } else {
> +                               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> +                       }
>                        areq = &mq->mqrq_cur->mmc_active;
>                } else
>                        areq = NULL;
> @@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                if (!areq)
>                        return 0;
>
> +check_status:
>                mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
>                brq = &mq_rq->brq;
>                req = mq_rq->req;
> @@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                         * A block was successfully transferred.
>                         */
>                        mmc_blk_reset_success(md, type);
> -                       spin_lock_irq(&md->lock);
> -                       ret = __blk_end_request(req, 0,
> +
> +                       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> +                               int idx = mq_rq->packed_fail_idx, i = 0;
> +                               while (!list_empty(&mq_rq->packed_list)) {
> +                                       prq = list_entry_rq(mq_rq->packed_list.next);
> +                                       if (idx == i) {
> +                                               /* retry from error index */
> +                                               reqs -= idx;
> +                                               mq_rq->req = prq;
> +                                               ret = 1;
> +                                               break;
> +                                       }
> +                                       list_del_init(&prq->queuelist);
> +                                       spin_lock_irq(&md->lock);
> +                                       ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
> +                                       spin_unlock_irq(&md->lock);
> +                                       i++;
> +                               }
> +                               break;
> +                       } else {
> +                               spin_lock_irq(&md->lock);
> +                               ret = __blk_end_request(req, 0,
>                                                brq->data.bytes_xfered);
> -                       spin_unlock_irq(&md->lock);
> +                               spin_unlock_irq(&md->lock);
> +                       }
> +
>                        /*
>                         * If the blk_end_request function returns non-zero even
>                         * though all data has been transferred and no errors
> @@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>                        break;
>                }
>
> -               if (ret) {
> +               if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
>                        /*
>                         * In case of a incomplete request
>                         * prepare it again and resend.
> @@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>        return 1;
>
>  cmd_abort:
> -       spin_lock_irq(&md->lock);
> -       while (ret)
> -               ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> -       spin_unlock_irq(&md->lock);
> +       if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
> +               spin_lock_irq(&md->lock);
> +               while (ret)
> +                       ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> +               spin_unlock_irq(&md->lock);
> +       } else {
> +               while (!list_empty(&mq_rq->packed_list)) {
> +                       prq = list_entry_rq(mq_rq->packed_list.next);
> +                       list_del_init(&prq->queuelist);
> +                       spin_lock_irq(&md->lock);
> +                       __blk_end_request(prq, -EIO, blk_rq_bytes(prq));
> +                       spin_unlock_irq(&md->lock);
> +               }
> +       }
>
>  start_new_req:
>        if (rqc) {
> +               /*
> +                * If current request is packed, it need to put back.
> +                */
> +               if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
> +                       while (!list_empty(&mq->mqrq_cur->packed_list)) {
> +                               prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
> +                               if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
> +                                       list_del_init(&prq->queuelist);
> +                                       blk_requeue_request(mq->queue, prq);
> +                               } else {
> +                                       list_del_init(&prq->queuelist);
> +                               }
> +                       }
> +               }
>                mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>                mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
>        }
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index dcad59c..3a4542e 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
>
>        memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
>        memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
> +       INIT_LIST_HEAD(&mqrq_cur->packed_list);
> +       INIT_LIST_HEAD(&mqrq_prev->packed_list);
>        mq->mqrq_cur = mqrq_cur;
>        mq->mqrq_prev = mqrq_prev;
>        mq->queue->queuedata = mq;
> @@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
>        }
>  }
>
> +static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
> +                               struct mmc_queue_req *mqrq,
> +                               struct scatterlist *sg)
> +{
> +       struct scatterlist *__sg;
> +       unsigned int sg_len = 0;
> +       struct request *req;
> +       enum mmc_packed_cmd cmd;
> +
> +       cmd = mqrq->packed_cmd;
> +
> +       if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
> +               __sg =sg;
> +               sg_set_buf(__sg, mqrq->packed_cmd_hdr,
> +                               sizeof(mqrq->packed_cmd_hdr));
> +               sg_len++;
> +               if (cmd == MMC_PACKED_WR_HDR) {
> +                       sg_mark_end(__sg);
> +                       return sg_len;
> +               }
> +               __sg->page_link &= ~0x02;
> +       }
> +
> +       __sg = sg + sg_len;
> +       list_for_each_entry(req, &mqrq->packed_list, queuelist) {
> +               sg_len += blk_rq_map_sg(mq->queue, req, __sg);
> +               __sg = sg + (sg_len - 1);
> +               (__sg++)->page_link &= ~0x02;
> +       }
> +       sg_mark_end(sg + (sg_len - 1));
> +       return sg_len;
> +}
> +
>  /*
>  * Prepare the sg list(s) to be handed of to the host driver
>  */
> @@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
>        struct scatterlist *sg;
>        int i;
>
> -       if (!mqrq->bounce_buf)
> -               return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> +       if (!mqrq->bounce_buf) {
> +               if (!list_empty(&mqrq->packed_list))
> +                       return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
> +               else
> +                       return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> +       }
>
>        BUG_ON(!mqrq->bounce_sg);
>
> -       sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> +       if (!list_empty(&mqrq->packed_list))
> +               sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
> +       else
> +               sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
>
>        mqrq->bounce_sg_len = sg_len;
>
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index d2a1eb4..5d0131e 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -12,6 +12,13 @@ struct mmc_blk_request {
>        struct mmc_data         data;
>  };
>
> +enum mmc_packed_cmd {
> +       MMC_PACKED_NONE = 0,
> +       MMC_PACKED_WR_HDR,
> +       MMC_PACKED_WRITE,
> +       MMC_PACKED_READ,
> +};
> +
>  struct mmc_queue_req {
>        struct request          *req;
>        struct mmc_blk_request  brq;
> @@ -20,6 +27,11 @@ struct mmc_queue_req {
>        struct scatterlist      *bounce_sg;
>        unsigned int            bounce_sg_len;
>        struct mmc_async_req    mmc_active;
> +       struct list_head        packed_list;
> +       u32                     packed_cmd_hdr[128];
> +       unsigned int            packed_blocks;
> +       enum mmc_packed_cmd     packed_cmd;
> +       int             packed_fail_idx;
>  };
>
>  struct mmc_queue {
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 174a844..3c61d5a 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -18,6 +18,8 @@ struct mmc_request;
>  struct mmc_command {
>        u32                     opcode;
>        u32                     arg;
> +#define MMC_CMD23_ARG_REL_WR   (1 << 31)
> +#define MMC_CMD23_ARG_PACKED   ((0 << 31) | (1 << 30))
>        u32                     resp[4];
>        unsigned int            flags;          /* expected response type */
>  #define MMC_RSP_PRESENT        (1 << 0)
> @@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
>  extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
>        struct mmc_command *, int);
>  extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
> +extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);
>
>  #define MMC_ERASE_ARG          0x00000000
>  #define MMC_SECURE_ERASE_ARG   0x80000000
> --
> 1.7.0.4
> Even i just had a chance to run checkpatch. has some warnings and error
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 2/2] mmc: core: Support packed command for eMMC4.5 device

[Seungwon Jeon]

This patch supports packed command of eMMC4.5 device.
Several reads(or writes) can be grouped in packed command
and all data of the individual commands can be sent in a
single transfer on the bus.

Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
---
 drivers/mmc/card/block.c |  355 ++++++++++++++++++++++++++++++++++++++++++++--
 drivers/mmc/card/queue.c |   48 ++++++-
 drivers/mmc/card/queue.h |   12 ++
 include/linux/mmc/core.h |    3 +
 4 files changed, 404 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a1cb21f..6c49656 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -59,6 +59,13 @@ MODULE_ALIAS("mmc:block");
 #define INAND_CMD38_ARG_SECTRIM1 0x81
 #define INAND_CMD38_ARG_SECTRIM2 0x88

+#define mmc_req_rel_wr(req)	(((req->cmd_flags & REQ_FUA) || \
+			(req->cmd_flags & REQ_META)) && \
+			(rq_data_dir(req) == WRITE))
+#define PACKED_CMD_VER		0x01
+#define PACKED_CMD_RD		0x01
+#define PACKED_CMD_WR		0x02
+
 static DEFINE_MUTEX(block_mutex);

 /*
@@ -943,7 +950,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
 	 * kind.  If it was a write, we may have transitioned to
 	 * program mode, which we have to wait for it to complete.
 	 */
-	if (!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) {
+	if ((!mmc_host_is_spi(card->host) && rq_data_dir(req) != READ) ||
+			(mq_mrq->packed_cmd == MMC_PACKED_WR_HDR)) {
 		u32 status;
 		do {
 			int err = get_card_status(card, &status, 5);
@@ -980,12 +988,67 @@ static int mmc_blk_err_check(struct mmc_card *card,
 	if (!brq->data.bytes_xfered)
 		return MMC_BLK_RETRY;

+	if (mq_mrq->packed_cmd != MMC_PACKED_NONE) {
+		if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
+			return MMC_BLK_PARTIAL;
+		else
+			return MMC_BLK_SUCCESS;
+	}
+
 	if (blk_rq_bytes(req) != brq->data.bytes_xfered)
 		return MMC_BLK_PARTIAL;

 	return MMC_BLK_SUCCESS;
 }

+static int mmc_blk_packed_err_check(struct mmc_card *card,
+			     struct mmc_async_req *areq)
+{
+	struct mmc_queue_req *mq_mrq = container_of(areq, struct mmc_queue_req,
+						    mmc_active);
+	int err, check, status;
+	u8 ext_csd[512];
+
+	check = mmc_blk_err_check(card, areq);
+
+	if (check == MMC_BLK_SUCCESS)
+		return check;
+
+	if (check == MMC_BLK_PARTIAL) {
+		err = get_card_status(card, &status, 0);
+		if (err)
+			return MMC_BLK_ABORT;
+
+		if (status & R1_EXP_EVENT) {
+			err = mmc_send_ext_csd(card, ext_csd);
+			if (err)
+				return MMC_BLK_ABORT;
+
+			if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS + 0] &
+						EXT_CSD_PACKED_FAILURE) &&
+					(ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
+					 EXT_CSD_PACKED_GENERIC_ERROR)) {
+				if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
+						EXT_CSD_PACKED_INDEXED_ERROR) {
+					/* Make be 0-based */
+					mq_mrq->packed_fail_idx =
+						ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
+					return MMC_BLK_PARTIAL;
+				} else {
+					return MMC_BLK_RETRY;
+				}
+			}
+		} else {
+			return MMC_BLK_RETRY;
+		}
+	}
+
+	if (check != MMC_BLK_ABORT)
+		return MMC_BLK_RETRY;
+	else
+		return MMC_BLK_ABORT;
+}
+
 static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 			       struct mmc_card *card,
 			       int disable_multi,
@@ -1129,6 +1192,211 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
 	mmc_queue_bounce_pre(mqrq);
 }

+static u8 mmc_blk_chk_packable(struct mmc_queue *mq, struct request *req)
+{
+	struct request_queue *q = mq->queue;
+	struct mmc_card *card = mq->card;
+	struct request *cur = req, *next = NULL;
+	struct mmc_blk_data *md = mq->data;
+	bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
+	unsigned int req_sectors = 0, phys_segments = 0;
+	unsigned int max_blk_count, max_phys_segs;
+	u8 max_packed_rw = 0;
+	u8 reqs = 0;
+
+	if (!(md->flags & MMC_BLK_CMD23) &&
+			!card->ext_csd.packed_event_en)
+		goto no_packed;
+
+	if (rq_data_dir(cur) == READ)
+		max_packed_rw = card->ext_csd.max_packed_reads;
+	else
+		max_packed_rw = card->ext_csd.max_packed_writes;
+
+	if (max_packed_rw == 0)
+		goto no_packed;
+
+	if (mmc_req_rel_wr(cur) &&
+			(md->flags & MMC_BLK_REL_WR) &&
+			!en_rel_wr) {
+		goto no_packed;
+	}
+
+	max_blk_count = min(card->host->max_blk_count,
+			card->host->max_req_size >> 9);
+	if (unlikely(max_blk_count > 0xffff))
+		max_blk_count = 0xffff;
+
+	max_phys_segs = queue_max_segments(q);
+	req_sectors += blk_rq_sectors(cur);
+	phys_segments += req->nr_phys_segments;
+
+	if (rq_data_dir(cur) == WRITE) {
+		req_sectors++;
+		phys_segments++;
+	}
+
+	while (reqs < max_packed_rw - 1) {
+		next = blk_fetch_request(q);
+		if (!next)
+			break;
+
+		if (rq_data_dir(cur) != rq_data_dir(next)) {
+			blk_requeue_request(q, next);
+			break;
+		}
+
+		if (mmc_req_rel_wr(next) &&
+				(md->flags & MMC_BLK_REL_WR) &&
+				!en_rel_wr) {
+			blk_requeue_request(q, next);
+			break;
+		}
+
+		req_sectors += blk_rq_sectors(next);
+		if (req_sectors > max_blk_count) {
+			blk_requeue_request(q, next);
+			break;
+		}
+
+		phys_segments +=  next->nr_phys_segments;
+		if (phys_segments > max_phys_segs) {
+			blk_requeue_request(q, next);
+			break;
+		}
+
+		list_add_tail(&next->queuelist, &mq->mqrq_cur->packed_list);
+		cur = next;
+		reqs++;
+	}
+
+	if (reqs > 0) {
+		list_add(&req->queuelist, &mq->mqrq_cur->packed_list);
+		return (reqs + 1);
+	}
+
+no_packed:
+	mq->mqrq_cur->packed_cmd = MMC_PACKED_NONE;
+	return reqs;
+}
+
+static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
+			       struct mmc_card *card,
+			       struct mmc_queue *mq,
+			       u8 reqs)
+{
+	struct mmc_blk_request *brq = &mqrq->brq;
+	struct request *req = mqrq->req;
+	struct request *prq;
+	struct mmc_blk_data *md = mq->data;
+	bool do_rel_wr;
+	u32 *packed_cmd_hdr = mqrq->packed_cmd_hdr;
+	u8 i =1;
+
+	mqrq->packed_cmd = (rq_data_dir(req) == READ) ?
+		MMC_PACKED_WR_HDR : MMC_PACKED_WRITE;
+	mqrq->packed_blocks = 0;
+	mqrq->packed_fail_idx = -1;
+
+	memset(packed_cmd_hdr, 0, sizeof(mqrq->packed_cmd_hdr));
+	packed_cmd_hdr[0] = (reqs << 16) |
+		(((rq_data_dir(req) == READ) ? PACKED_CMD_RD: PACKED_CMD_WR) << 8) |
+		PACKED_CMD_VER;
+
+	/*
+	 * Argument for each entry of packed group
+	 */
+	list_for_each_entry(prq, &mqrq->packed_list, queuelist) {
+		do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
+		/* Argument of CMD23*/
+		packed_cmd_hdr[(i * 2)] = (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
+			blk_rq_sectors(prq);
+		/* Argument of CMD18 or CMD25 */
+		packed_cmd_hdr[((i * 2)) + 1] = mmc_card_blockaddr(card) ?
+			blk_rq_pos(prq) : blk_rq_pos(prq) << 9;
+		mqrq->packed_blocks += blk_rq_sectors(prq);
+		i++;
+	}
+
+	memset(brq, 0, sizeof(struct mmc_blk_request));
+	brq->mrq.cmd = &brq->cmd;
+	brq->mrq.data = &brq->data;
+	brq->mrq.sbc = &brq->sbc;
+	brq->mrq.stop = &brq->stop;
+
+	brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
+	brq->sbc.arg = MMC_CMD23_ARG_PACKED |
+		((rq_data_dir(req) == READ) ? 1 : mqrq->packed_blocks + 1);
+	brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
+
+	brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
+	brq->cmd.arg = blk_rq_pos(req);
+	if (!mmc_card_blockaddr(card))
+		brq->cmd.arg <<= 9;
+	brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	brq->data.blksz = 512;
+	/*
+	 * Write separately the packd command header only for packed read.
+	 * In case of packed write, header is sent with blocks of data.
+	 */
+	brq->data.blocks = (rq_data_dir(req) == READ) ?
+		1 : mqrq->packed_blocks + 1;
+	brq->data.flags |= MMC_DATA_WRITE;
+
+	brq->stop.opcode = MMC_STOP_TRANSMISSION;
+	brq->stop.arg = 0;
+	brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+
+	mmc_set_data_timeout(&brq->data, card);
+
+	brq->data.sg = mqrq->sg;
+	brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
+
+	mqrq->mmc_active.mrq = &brq->mrq;
+	mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
+
+	mmc_queue_bounce_pre(mqrq);
+}
+
+static void mmc_blk_packed_rrq_prep(struct mmc_queue_req *mqrq,
+			       struct mmc_card *card,
+			       struct mmc_queue *mq)
+{
+	struct mmc_blk_request *brq = &mqrq->brq;
+	struct request *req = mqrq->req;
+
+	mqrq->packed_cmd = MMC_PACKED_READ;
+
+	memset(brq, 0, sizeof(struct mmc_blk_request));
+	brq->mrq.cmd = &brq->cmd;
+	brq->mrq.data = &brq->data;
+	brq->mrq.stop = &brq->stop;
+
+	brq->cmd.opcode = MMC_READ_MULTIPLE_BLOCK;
+	brq->cmd.arg = blk_rq_pos(req);
+	if (!mmc_card_blockaddr(card))
+		brq->cmd.arg <<= 9;
+	brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+	brq->data.blksz = 512;
+	brq->data.blocks = mqrq->packed_blocks;
+	brq->data.flags |= MMC_DATA_READ;
+
+	brq->stop.opcode = MMC_STOP_TRANSMISSION;
+	brq->stop.arg = 0;
+	brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+
+	mmc_set_data_timeout(&brq->data, card);
+
+	brq->data.sg = mqrq->sg;
+	brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
+
+	mqrq->mmc_active.mrq = &brq->mrq;
+	mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
+
+	mmc_queue_bounce_pre(mqrq);
+}
+
 static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
 			   struct mmc_blk_request *brq, struct request *req,
 			   int ret)
@@ -1166,15 +1434,33 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	int ret = 1, disable_multi = 0, retry = 0, type;
 	enum mmc_blk_status status;
 	struct mmc_queue_req *mq_rq;
-	struct request *req;
+	struct request *req, *prq;
 	struct mmc_async_req *areq;
+	u8 reqs = 0;

 	if (!rqc && !mq->mqrq_prev->req)
 		return 0;

+	if (rqc)
+		reqs = mmc_blk_chk_packable(mq, rqc);
+
 	do {
 		if (rqc) {
-			mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
+			if (reqs >= 2) {
+				mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur, card, mq, reqs);
+				if (rq_data_dir(rqc) == READ) {
+					areq = &mq->mqrq_cur->mmc_active;
+					mmc_wait_for_req(card->host, areq->mrq);
+					status = mmc_blk_packed_err_check(card, areq);
+					if (status == MMC_BLK_SUCCESS) {
+						mmc_blk_packed_rrq_prep(mq->mqrq_cur, card, mq);
+					} else {
+						goto check_status;
+					}
+				}
+			} else {
+				mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
+			}
 			areq = &mq->mqrq_cur->mmc_active;
 		} else
 			areq = NULL;
@@ -1182,6 +1468,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 		if (!areq)
 			return 0;

+check_status:
 		mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
 		brq = &mq_rq->brq;
 		req = mq_rq->req;
@@ -1195,10 +1482,32 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			 * A block was successfully transferred.
 			 */
 			mmc_blk_reset_success(md, type);
-			spin_lock_irq(&md->lock);
-			ret = __blk_end_request(req, 0,
+
+			if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
+				int idx = mq_rq->packed_fail_idx, i = 0;
+				while (!list_empty(&mq_rq->packed_list)) {
+					prq = list_entry_rq(mq_rq->packed_list.next);
+					if (idx == i) {
+						/* retry from error index */
+						reqs -= idx;
+						mq_rq->req = prq;
+						ret = 1;
+						break;
+					}
+					list_del_init(&prq->queuelist);
+					spin_lock_irq(&md->lock);
+					ret = __blk_end_request(prq, 0, blk_rq_bytes(prq));
+					spin_unlock_irq(&md->lock);
+					i++;
+				}
+				break;
+			} else {
+				spin_lock_irq(&md->lock);
+				ret = __blk_end_request(req, 0,
 						brq->data.bytes_xfered);
-			spin_unlock_irq(&md->lock);
+				spin_unlock_irq(&md->lock);
+			}
+
 			/*
 			 * If the blk_end_request function returns non-zero even
 			 * though all data has been transferred and no errors
@@ -1257,7 +1566,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 			break;
 		}

-		if (ret) {
+		if (ret && (mq_rq->packed_cmd == MMC_PACKED_NONE)) {
 			/*
 			 * In case of a incomplete request
 			 * prepare it again and resend.
@@ -1270,13 +1579,37 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 	return 1;

  cmd_abort:
-	spin_lock_irq(&md->lock);
-	while (ret)
-		ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
-	spin_unlock_irq(&md->lock);
+	if (mq_rq->packed_cmd != MMC_PACKED_NONE) {
+		spin_lock_irq(&md->lock);
+		while (ret)
+			ret = __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
+		spin_unlock_irq(&md->lock);
+	} else {
+		while (!list_empty(&mq_rq->packed_list)) {
+			prq = list_entry_rq(mq_rq->packed_list.next);
+			list_del_init(&prq->queuelist);
+			spin_lock_irq(&md->lock);
+			__blk_end_request(prq, -EIO, blk_rq_bytes(prq));
+			spin_unlock_irq(&md->lock);
+		}
+	}

  start_new_req:
 	if (rqc) {
+		/*
+		 * If current request is packed, it need to put back.
+		 */
+		if (mq->mqrq_cur->packed_cmd != MMC_PACKED_NONE) {
+			while (!list_empty(&mq->mqrq_cur->packed_list)) {
+				prq = list_entry_rq(mq->mqrq_cur->packed_list.prev);
+				if (prq->queuelist.prev != &mq->mqrq_cur->packed_list) {
+					list_del_init(&prq->queuelist);
+					blk_requeue_request(mq->queue, prq);
+				} else {
+					list_del_init(&prq->queuelist);
+				}
+			}
+		}
 		mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
 		mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
 	}
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index dcad59c..3a4542e 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -172,6 +172,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,

 	memset(&mq->mqrq_cur, 0, sizeof(mq->mqrq_cur));
 	memset(&mq->mqrq_prev, 0, sizeof(mq->mqrq_prev));
+	INIT_LIST_HEAD(&mqrq_cur->packed_list);
+	INIT_LIST_HEAD(&mqrq_prev->packed_list);
 	mq->mqrq_cur = mqrq_cur;
 	mq->mqrq_prev = mqrq_prev;
 	mq->queue->queuedata = mq;
@@ -372,6 +374,39 @@ void mmc_queue_resume(struct mmc_queue *mq)
 	}
 }

+static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
+				struct mmc_queue_req *mqrq,
+				struct scatterlist *sg)
+{
+	struct scatterlist *__sg;
+	unsigned int sg_len = 0;
+	struct request *req;
+	enum mmc_packed_cmd cmd;
+
+	cmd = mqrq->packed_cmd;
+
+	if (cmd == MMC_PACKED_WR_HDR || cmd == MMC_PACKED_WRITE) {
+		__sg =sg;
+		sg_set_buf(__sg, mqrq->packed_cmd_hdr,
+				sizeof(mqrq->packed_cmd_hdr));
+		sg_len++;
+		if (cmd == MMC_PACKED_WR_HDR) {
+			sg_mark_end(__sg);
+			return sg_len;
+		}
+		__sg->page_link &= ~0x02;
+	}
+
+	__sg = sg + sg_len;
+	list_for_each_entry(req, &mqrq->packed_list, queuelist) {
+		sg_len += blk_rq_map_sg(mq->queue, req, __sg);
+		__sg = sg + (sg_len - 1);
+		(__sg++)->page_link &= ~0x02;
+	}
+	sg_mark_end(sg + (sg_len - 1));
+	return sg_len;
+}
+
 /*
  * Prepare the sg list(s) to be handed of to the host driver
  */
@@ -382,12 +417,19 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
 	struct scatterlist *sg;
 	int i;

-	if (!mqrq->bounce_buf)
-		return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
+	if (!mqrq->bounce_buf) {
+		if (!list_empty(&mqrq->packed_list))
+			return mmc_queue_packed_map_sg(mq, mqrq, mqrq->sg);
+		else
+			return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
+	}

 	BUG_ON(!mqrq->bounce_sg);

-	sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
+	if (!list_empty(&mqrq->packed_list))
+		sg_len = mmc_queue_packed_map_sg(mq, mqrq, mqrq->bounce_sg);
+	else
+		sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);

 	mqrq->bounce_sg_len = sg_len;

diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index d2a1eb4..5d0131e 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -12,6 +12,13 @@ struct mmc_blk_request {
 	struct mmc_data		data;
 };

+enum mmc_packed_cmd {
+	MMC_PACKED_NONE = 0,
+	MMC_PACKED_WR_HDR,
+	MMC_PACKED_WRITE,
+	MMC_PACKED_READ,
+};
+
 struct mmc_queue_req {
 	struct request		*req;
 	struct mmc_blk_request	brq;
@@ -20,6 +27,11 @@ struct mmc_queue_req {
 	struct scatterlist	*bounce_sg;
 	unsigned int		bounce_sg_len;
 	struct mmc_async_req	mmc_active;
+	struct list_head	packed_list;
+	u32			packed_cmd_hdr[128];
+	unsigned int		packed_blocks;
+	enum mmc_packed_cmd	packed_cmd;
+	int		packed_fail_idx;
 };

 struct mmc_queue {
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 174a844..3c61d5a 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -18,6 +18,8 @@ struct mmc_request;
 struct mmc_command {
 	u32			opcode;
 	u32			arg;
+#define MMC_CMD23_ARG_REL_WR	(1 << 31)
+#define MMC_CMD23_ARG_PACKED	((0 << 31) | (1 << 30))
 	u32			resp[4];
 	unsigned int		flags;		/* expected response type */
 #define MMC_RSP_PRESENT	(1 << 0)
@@ -143,6 +145,7 @@ extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
 extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
 	struct mmc_command *, int);
 extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
+extern int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd);

 #define MMC_ERASE_ARG		0x00000000
 #define MMC_SECURE_ERASE_ARG	0x80000000
--
1.7.0.4