Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Jaehoon Chung]

Hi All,

Sorry for reply..I didn't fully read about this patch history.
So i will resend the my opinion after reading the mail-history

Best Regards,
Jaehoon Chung

On 12/21/2012 06:56 PM, Ulf Hansson wrote:
> On 21 December 2012 09:35, Maya Erez  wrote:
>> Hi Ulf,
>>
>> Thanks for the info. We will consider our controller behavior in suspend.
>> Would it be acceptable by you to keep the polling for BKOPS completion under
>> a special CAPS2 flag?
> 
> Not sure what you propose here. I guess some host cap would be needed
> to be able to cope with those drivers which uses runtime PM for normal
> suspend.
> 
> But, the "polling method" as such shall not be done just because of
> preventing those drivers entering runtime susend state.
> pm_runtime_get* must be used here, I think.
> Then of course you need to poll for the BKOPS status.
> 
>>
>> Thanks,
>> Maya
>>
>> -Original Message-
>> From: linux-mmc-ow...@vger.kernel.org
>> [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
>> Sent: Thursday, December 13, 2012 12:18 PM
>> To: me...@codeaurora.org
>> Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
>> open list
>> Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS
>>
>> On 12 December 2012 13:32,   wrote:
>>> Hi Ulf,
>>>
>>> Sorry for the late response.
>>> See my reply below.
>>>
>>> Thanks,
>>> Maya
>>>
>>> On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
>>>> Hi Maya,
>>>>
>>>> On 4 December 2012 22:17,   wrote:
>>>>> Hi Ulf,
>>>>>
>>>>> Let me try to better explain:
>>>>> The idea behind the periodic BKOPS is to check the card's need for
>>>>> BKOPS periodically in order to prevent an urgent BKOPS need by the card.
>>>>> In order to actually manage to prevent the urgent BKOPS need, the
>>>>> host should give the card enough time to perform the BKOPS (when it
>>>>> recognizes BKOPS need), otherwise there is no point in having the
>>>>> periodic BKOPS.
>>>>> The above results in the following:
>>>>> 1. After starting non-urgent BKOPS we "delay" getting into suspend
>>>>> by polling on the card's status (explanation below), in order to
>>>>> give the card time to perform the BKOPS. This has no effect on the
>>>>> power consumption since the same BKOPS operations that were
>>>>> performed after the foregound operation just moved to another
>>>>> location, meaning before going into suspend.
>>>>
>>>> I am not sure what you are talking about here, runtime suspend or
>>>> system suspend? Polling the card's status will not prevent any of
>>>> this. So you have got this wrong.
>>>
>>> I am referring to the runtime suspend.
>>> Our controller implements the runtime suspend and is not using the
>>> default implementation of core/bus.c.
>>
>> This is not the "default runtime suspend" for a host device, but for the
>> card device. Do not mix this up with runtime pm for a host device.
>>
>> Right now runtime pm for the card _device_ is only enabled for SDIO.
>> Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
>> fully suspended when it is not needed and thus save a lot of power. For
>> example when a WLAN interface goes up/down.
>>
>>> This is the reason why in our implementation polling the card status
>>> "delays" the runtime suspend while it is not the case when using the
>>> default runtime suspend implementation.
>>> I can try to explain here what our controller is doing but since it is
>>> specific to us then I guess it is not relevant to the discussion.
>>> Our controller is calling mmc_suspend_host in runtime suspend, which
>>> issues an HPI to stop the BKOPS.
>>
>> So, doing mmc_suspend_host in you runtime_suspend callback, is that really
>> what you want to do?
>>
>> 1.
>> You will introduce significant latencies (I have seen SD-cards which needs
>> more than 1 s to initialize, eMMC is better but we are anyway talking
>> several 100 ms) once new requests arrives after the host as entered the
>> runtime suspend state.
>>
>> 2.
>> SD cards has no "power off" notification, so you will actually stress test
>> the SD cards internal FTL to be crash safe by cutting the power to it more
>> 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 21 December 2012 09:35, Maya Erez  wrote:
> Hi Ulf,
>
> Thanks for the info. We will consider our controller behavior in suspend.
> Would it be acceptable by you to keep the polling for BKOPS completion under
> a special CAPS2 flag?

Not sure what you propose here. I guess some host cap would be needed
to be able to cope with those drivers which uses runtime PM for normal
suspend.

But, the "polling method" as such shall not be done just because of
preventing those drivers entering runtime susend state.
pm_runtime_get* must be used here, I think.
Then of course you need to poll for the BKOPS status.

>
> Thanks,
> Maya
>
> -Original Message-
> From: linux-mmc-ow...@vger.kernel.org
> [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
> Sent: Thursday, December 13, 2012 12:18 PM
> To: me...@codeaurora.org
> Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
> open list
> Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS
>
> On 12 December 2012 13:32,   wrote:
>> Hi Ulf,
>>
>> Sorry for the late response.
>> See my reply below.
>>
>> Thanks,
>> Maya
>>
>> On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
>>> Hi Maya,
>>>
>>> On 4 December 2012 22:17,   wrote:
>>>> Hi Ulf,
>>>>
>>>> Let me try to better explain:
>>>> The idea behind the periodic BKOPS is to check the card's need for
>>>> BKOPS periodically in order to prevent an urgent BKOPS need by the card.
>>>> In order to actually manage to prevent the urgent BKOPS need, the
>>>> host should give the card enough time to perform the BKOPS (when it
>>>> recognizes BKOPS need), otherwise there is no point in having the
>>>> periodic BKOPS.
>>>> The above results in the following:
>>>> 1. After starting non-urgent BKOPS we "delay" getting into suspend
>>>> by polling on the card's status (explanation below), in order to
>>>> give the card time to perform the BKOPS. This has no effect on the
>>>> power consumption since the same BKOPS operations that were
>>>> performed after the foregound operation just moved to another
>>>> location, meaning before going into suspend.
>>>
>>> I am not sure what you are talking about here, runtime suspend or
>>> system suspend? Polling the card's status will not prevent any of
>>> this. So you have got this wrong.
>>
>> I am referring to the runtime suspend.
>> Our controller implements the runtime suspend and is not using the
>> default implementation of core/bus.c.
>
> This is not the "default runtime suspend" for a host device, but for the
> card device. Do not mix this up with runtime pm for a host device.
>
> Right now runtime pm for the card _device_ is only enabled for SDIO.
> Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
> fully suspended when it is not needed and thus save a lot of power. For
> example when a WLAN interface goes up/down.
>
>> This is the reason why in our implementation polling the card status
>> "delays" the runtime suspend while it is not the case when using the
>> default runtime suspend implementation.
>> I can try to explain here what our controller is doing but since it is
>> specific to us then I guess it is not relevant to the discussion.
>> Our controller is calling mmc_suspend_host in runtime suspend, which
>> issues an HPI to stop the BKOPS.
>
> So, doing mmc_suspend_host in you runtime_suspend callback, is that really
> what you want to do?
>
> 1.
> You will introduce significant latencies (I have seen SD-cards which needs
> more than 1 s to initialize, eMMC is better but we are anyway talking
> several 100 ms) once new requests arrives after the host as entered the
> runtime suspend state.
>
> 2.
> SD cards has no "power off" notification, so you will actually stress test
> the SD cards internal FTL to be crash safe by cutting the power to it more
> often.
>
> 3.
> You will prevent SD-cards from doing it's back ground operations, which is
> done automatically and not like in a controlled manner as for eMMC.
>
> So of course, you save some power, but is the consequences worth it? :-)
>
>> Now that I understand that this code is not needed for all the host
>> drivers I will add a flag to decide if polling is required when doing
>> an unblocking BKOPS.
>
> You must not poll to prevent this!
>
> Instead you need to prevent the host from going into runtime suspend state,
> which is simply done by 

RE: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Maya Erez]

Hi Ulf,

Thanks for the info. We will consider our controller behavior in suspend.
Would it be acceptable by you to keep the polling for BKOPS completion under
a special CAPS2 flag?

Thanks,
Maya

-Original Message-
From: linux-mmc-ow...@vger.kernel.org
[mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
Sent: Thursday, December 13, 2012 12:18 PM
To: me...@codeaurora.org
Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
open list
Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

On 12 December 2012 13:32,   wrote:
> Hi Ulf,
>
> Sorry for the late response.
> See my reply below.
>
> Thanks,
> Maya
>
> On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
>> Hi Maya,
>>
>> On 4 December 2012 22:17,   wrote:
>>> Hi Ulf,
>>>
>>> Let me try to better explain:
>>> The idea behind the periodic BKOPS is to check the card's need for 
>>> BKOPS periodically in order to prevent an urgent BKOPS need by the card.
>>> In order to actually manage to prevent the urgent BKOPS need, the 
>>> host should give the card enough time to perform the BKOPS (when it 
>>> recognizes BKOPS need), otherwise there is no point in having the 
>>> periodic BKOPS.
>>> The above results in the following:
>>> 1. After starting non-urgent BKOPS we "delay" getting into suspend 
>>> by polling on the card's status (explanation below), in order to 
>>> give the card time to perform the BKOPS. This has no effect on the 
>>> power consumption since the same BKOPS operations that were 
>>> performed after the foregound operation just moved to another 
>>> location, meaning before going into suspend.
>>
>> I am not sure what you are talking about here, runtime suspend or 
>> system suspend? Polling the card's status will not prevent any of 
>> this. So you have got this wrong.
>
> I am referring to the runtime suspend.
> Our controller implements the runtime suspend and is not using the 
> default implementation of core/bus.c.

This is not the "default runtime suspend" for a host device, but for the
card device. Do not mix this up with runtime pm for a host device.

Right now runtime pm for the card _device_ is only enabled for SDIO.
Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
fully suspended when it is not needed and thus save a lot of power. For
example when a WLAN interface goes up/down.

> This is the reason why in our implementation polling the card status 
> "delays" the runtime suspend while it is not the case when using the 
> default runtime suspend implementation.
> I can try to explain here what our controller is doing but since it is 
> specific to us then I guess it is not relevant to the discussion.
> Our controller is calling mmc_suspend_host in runtime suspend, which 
> issues an HPI to stop the BKOPS.

So, doing mmc_suspend_host in you runtime_suspend callback, is that really
what you want to do?

1.
You will introduce significant latencies (I have seen SD-cards which needs
more than 1 s to initialize, eMMC is better but we are anyway talking
several 100 ms) once new requests arrives after the host as entered the
runtime suspend state.

2.
SD cards has no "power off" notification, so you will actually stress test
the SD cards internal FTL to be crash safe by cutting the power to it more
often.

3.
You will prevent SD-cards from doing it's back ground operations, which is
done automatically and not like in a controlled manner as for eMMC.

So of course, you save some power, but is the consequences worth it? :-)

> Now that I understand that this code is not needed for all the host 
> drivers I will add a flag to decide if polling is required when doing 
> an unblocking BKOPS.

You must not poll to prevent this!

Instead you need to prevent the host from going into runtime suspend state,
which is simply done by pm_runtime_get_sync for the host device.
Although, it _must_ not be done for drivers not doing mmc_suspend_host in
their runtime suspend callbacks. Since then it will prevent these from doing
runtime power save actions, which is not ok.

> Other host drivers that actually suspend on runtime suspend can enable 
> this flag and allow BKOPS to be active for a longer period.
> I will prepare a new patch and send it for review.
>
>>
>>> 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be 
>>> efficient since we don't want to wait until the host is ready to get 
>>> into suspend and then prevent him from suspending by doing BKOPS 
>>> operations that can take a long time. It is better to start the 
>>> BKOPS earlier.
>>
>> I did not suggest to use PM_SUSPE

RE: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Maya Erez]

Hi Ulf,

Thanks for the info. We will consider our controller behavior in suspend.
Would it be acceptable by you to keep the polling for BKOPS completion under
a special CAPS2 flag?

Thanks,
Maya

-Original Message-
From: linux-mmc-ow...@vger.kernel.org
[mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
Sent: Thursday, December 13, 2012 12:18 PM
To: me...@codeaurora.org
Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
open list
Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

On 12 December 2012 13:32,  me...@codeaurora.org wrote:
 Hi Ulf,

 Sorry for the late response.
 See my reply below.

 Thanks,
 Maya

 On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
 Hi Maya,

 On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for 
 BKOPS periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the 
 host should give the card enough time to perform the BKOPS (when it 
 recognizes BKOPS need), otherwise there is no point in having the 
 periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend 
 by polling on the card's status (explanation below), in order to 
 give the card time to perform the BKOPS. This has no effect on the 
 power consumption since the same BKOPS operations that were 
 performed after the foregound operation just moved to another 
 location, meaning before going into suspend.

 I am not sure what you are talking about here, runtime suspend or 
 system suspend? Polling the card's status will not prevent any of 
 this. So you have got this wrong.

 I am referring to the runtime suspend.
 Our controller implements the runtime suspend and is not using the 
 default implementation of core/bus.c.

This is not the default runtime suspend for a host device, but for the
card device. Do not mix this up with runtime pm for a host device.

Right now runtime pm for the card _device_ is only enabled for SDIO.
Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
fully suspended when it is not needed and thus save a lot of power. For
example when a WLAN interface goes up/down.

 This is the reason why in our implementation polling the card status 
 delays the runtime suspend while it is not the case when using the 
 default runtime suspend implementation.
 I can try to explain here what our controller is doing but since it is 
 specific to us then I guess it is not relevant to the discussion.
 Our controller is calling mmc_suspend_host in runtime suspend, which 
 issues an HPI to stop the BKOPS.

So, doing mmc_suspend_host in you runtime_suspend callback, is that really
what you want to do?

1.
You will introduce significant latencies (I have seen SD-cards which needs
more than 1 s to initialize, eMMC is better but we are anyway talking
several 100 ms) once new requests arrives after the host as entered the
runtime suspend state.

2.
SD cards has no power off notification, so you will actually stress test
the SD cards internal FTL to be crash safe by cutting the power to it more
often.

3.
You will prevent SD-cards from doing it's back ground operations, which is
done automatically and not like in a controlled manner as for eMMC.

So of course, you save some power, but is the consequences worth it? :-)

 Now that I understand that this code is not needed for all the host 
 drivers I will add a flag to decide if polling is required when doing 
 an unblocking BKOPS.

You must not poll to prevent this!

Instead you need to prevent the host from going into runtime suspend state,
which is simply done by pm_runtime_get_sync for the host device.
Although, it _must_ not be done for drivers not doing mmc_suspend_host in
their runtime suspend callbacks. Since then it will prevent these from doing
runtime power save actions, which is not ok.

 Other host drivers that actually suspend on runtime suspend can enable 
 this flag and allow BKOPS to be active for a longer period.
 I will prepare a new patch and send it for review.


 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be 
 efficient since we don't want to wait until the host is ready to get 
 into suspend and then prevent him from suspending by doing BKOPS 
 operations that can take a long time. It is better to start the 
 BKOPS earlier.

 I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM 
 for the card device. It can be an option to implement this feature on 
 top of a workqueue. At least worth to consider.


 We consider to call mmc_start_bkops every time MMC becomes idle, to 
 check the need for BKOPS. I will test it and include it in the next patch.


 I am not too familiar with the controllers code and also my 
 understanding in runtime suspend is very basic, so feel free to 
 correct me if I'm

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 21 December 2012 09:35, Maya Erez me...@codeaurora.org wrote:
 Hi Ulf,

 Thanks for the info. We will consider our controller behavior in suspend.
 Would it be acceptable by you to keep the polling for BKOPS completion under
 a special CAPS2 flag?

Not sure what you propose here. I guess some host cap would be needed
to be able to cope with those drivers which uses runtime PM for normal
suspend.

But, the polling method as such shall not be done just because of
preventing those drivers entering runtime susend state.
pm_runtime_get* must be used here, I think.
Then of course you need to poll for the BKOPS status.


 Thanks,
 Maya

 -Original Message-
 From: linux-mmc-ow...@vger.kernel.org
 [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
 Sent: Thursday, December 13, 2012 12:18 PM
 To: me...@codeaurora.org
 Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
 open list
 Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

 On 12 December 2012 13:32,  me...@codeaurora.org wrote:
 Hi Ulf,

 Sorry for the late response.
 See my reply below.

 Thanks,
 Maya

 On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
 Hi Maya,

 On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for
 BKOPS periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the
 host should give the card enough time to perform the BKOPS (when it
 recognizes BKOPS need), otherwise there is no point in having the
 periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend
 by polling on the card's status (explanation below), in order to
 give the card time to perform the BKOPS. This has no effect on the
 power consumption since the same BKOPS operations that were
 performed after the foregound operation just moved to another
 location, meaning before going into suspend.

 I am not sure what you are talking about here, runtime suspend or
 system suspend? Polling the card's status will not prevent any of
 this. So you have got this wrong.

 I am referring to the runtime suspend.
 Our controller implements the runtime suspend and is not using the
 default implementation of core/bus.c.

 This is not the default runtime suspend for a host device, but for the
 card device. Do not mix this up with runtime pm for a host device.

 Right now runtime pm for the card _device_ is only enabled for SDIO.
 Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
 fully suspended when it is not needed and thus save a lot of power. For
 example when a WLAN interface goes up/down.

 This is the reason why in our implementation polling the card status
 delays the runtime suspend while it is not the case when using the
 default runtime suspend implementation.
 I can try to explain here what our controller is doing but since it is
 specific to us then I guess it is not relevant to the discussion.
 Our controller is calling mmc_suspend_host in runtime suspend, which
 issues an HPI to stop the BKOPS.

 So, doing mmc_suspend_host in you runtime_suspend callback, is that really
 what you want to do?

 1.
 You will introduce significant latencies (I have seen SD-cards which needs
 more than 1 s to initialize, eMMC is better but we are anyway talking
 several 100 ms) once new requests arrives after the host as entered the
 runtime suspend state.

 2.
 SD cards has no power off notification, so you will actually stress test
 the SD cards internal FTL to be crash safe by cutting the power to it more
 often.

 3.
 You will prevent SD-cards from doing it's back ground operations, which is
 done automatically and not like in a controlled manner as for eMMC.

 So of course, you save some power, but is the consequences worth it? :-)

 Now that I understand that this code is not needed for all the host
 drivers I will add a flag to decide if polling is required when doing
 an unblocking BKOPS.

 You must not poll to prevent this!

 Instead you need to prevent the host from going into runtime suspend state,
 which is simply done by pm_runtime_get_sync for the host device.
 Although, it _must_ not be done for drivers not doing mmc_suspend_host in
 their runtime suspend callbacks. Since then it will prevent these from doing
 runtime power save actions, which is not ok.

 Other host drivers that actually suspend on runtime suspend can enable
 this flag and allow BKOPS to be active for a longer period.
 I will prepare a new patch and send it for review.


 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
 efficient since we don't want to wait until the host is ready to get
 into suspend and then prevent him from suspending by doing BKOPS
 operations that can take a long time. It is better to start the
 BKOPS earlier.

 I did not suggest to use

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Jaehoon Chung]

Hi All,

Sorry for reply..I didn't fully read about this patch history.
So i will resend the my opinion after reading the mail-history

Best Regards,
Jaehoon Chung

On 12/21/2012 06:56 PM, Ulf Hansson wrote:
 On 21 December 2012 09:35, Maya Erez me...@codeaurora.org wrote:
 Hi Ulf,

 Thanks for the info. We will consider our controller behavior in suspend.
 Would it be acceptable by you to keep the polling for BKOPS completion under
 a special CAPS2 flag?
 
 Not sure what you propose here. I guess some host cap would be needed
 to be able to cope with those drivers which uses runtime PM for normal
 suspend.
 
 But, the polling method as such shall not be done just because of
 preventing those drivers entering runtime susend state.
 pm_runtime_get* must be used here, I think.
 Then of course you need to poll for the BKOPS status.
 

 Thanks,
 Maya

 -Original Message-
 From: linux-mmc-ow...@vger.kernel.org
 [mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Ulf Hansson
 Sent: Thursday, December 13, 2012 12:18 PM
 To: me...@codeaurora.org
 Cc: Jaehoon Chung; linux-...@vger.kernel.org; linux-arm-...@vger.kernel.org;
 open list
 Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

 On 12 December 2012 13:32,  me...@codeaurora.org wrote:
 Hi Ulf,

 Sorry for the late response.
 See my reply below.

 Thanks,
 Maya

 On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
 Hi Maya,

 On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for
 BKOPS periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the
 host should give the card enough time to perform the BKOPS (when it
 recognizes BKOPS need), otherwise there is no point in having the
 periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend
 by polling on the card's status (explanation below), in order to
 give the card time to perform the BKOPS. This has no effect on the
 power consumption since the same BKOPS operations that were
 performed after the foregound operation just moved to another
 location, meaning before going into suspend.

 I am not sure what you are talking about here, runtime suspend or
 system suspend? Polling the card's status will not prevent any of
 this. So you have got this wrong.

 I am referring to the runtime suspend.
 Our controller implements the runtime suspend and is not using the
 default implementation of core/bus.c.

 This is not the default runtime suspend for a host device, but for the
 card device. Do not mix this up with runtime pm for a host device.

 Right now runtime pm for the card _device_ is only enabled for SDIO.
 Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be
 fully suspended when it is not needed and thus save a lot of power. For
 example when a WLAN interface goes up/down.

 This is the reason why in our implementation polling the card status
 delays the runtime suspend while it is not the case when using the
 default runtime suspend implementation.
 I can try to explain here what our controller is doing but since it is
 specific to us then I guess it is not relevant to the discussion.
 Our controller is calling mmc_suspend_host in runtime suspend, which
 issues an HPI to stop the BKOPS.

 So, doing mmc_suspend_host in you runtime_suspend callback, is that really
 what you want to do?

 1.
 You will introduce significant latencies (I have seen SD-cards which needs
 more than 1 s to initialize, eMMC is better but we are anyway talking
 several 100 ms) once new requests arrives after the host as entered the
 runtime suspend state.

 2.
 SD cards has no power off notification, so you will actually stress test
 the SD cards internal FTL to be crash safe by cutting the power to it more
 often.

 3.
 You will prevent SD-cards from doing it's back ground operations, which is
 done automatically and not like in a controlled manner as for eMMC.

 So of course, you save some power, but is the consequences worth it? :-)

 Now that I understand that this code is not needed for all the host
 drivers I will add a flag to decide if polling is required when doing
 an unblocking BKOPS.

 You must not poll to prevent this!

 Instead you need to prevent the host from going into runtime suspend state,
 which is simply done by pm_runtime_get_sync for the host device.
 Although, it _must_ not be done for drivers not doing mmc_suspend_host in
 their runtime suspend callbacks. Since then it will prevent these from doing
 runtime power save actions, which is not ok.

 Other host drivers that actually suspend on runtime suspend can enable
 this flag and allow BKOPS to be active for a longer period.
 I will prepare a new patch and send it for review.


 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
 efficient since we don't

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 12 December 2012 13:32,   wrote:
> Hi Ulf,
>
> Sorry for the late response.
> See my reply below.
>
> Thanks,
> Maya
>
> On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
>> Hi Maya,
>>
>> On 4 December 2012 22:17,   wrote:
>>> Hi Ulf,
>>>
>>> Let me try to better explain:
>>> The idea behind the periodic BKOPS is to check the card's need for BKOPS
>>> periodically in order to prevent an urgent BKOPS need by the card.
>>> In order to actually manage to prevent the urgent BKOPS need, the host
>>> should give the card enough time to perform the BKOPS (when it
>>> recognizes
>>> BKOPS need), otherwise there is no point in having the periodic BKOPS.
>>> The above results in the following:
>>> 1. After starting non-urgent BKOPS we "delay" getting into suspend by
>>> polling on the card's status (explanation below), in order to give the
>>> card time to perform the BKOPS. This has no effect on the power
>>> consumption since the same BKOPS operations that were performed after
>>> the
>>> foregound operation just moved to another location, meaning before going
>>> into suspend.
>>
>> I am not sure what you are talking about here, runtime suspend or
>> system suspend? Polling the card's status will not prevent any of
>> this. So you have got this wrong.
>
> I am referring to the runtime suspend.
> Our controller implements the runtime suspend and is not using the default
> implementation of core/bus.c.

This is not the "default runtime suspend" for a host device, but for
the card device. Do not mix this up with runtime pm for a host device.

Right now runtime pm for the card _device_ is only enabled for SDIO.
Thus SDIO drivers can use runtime pm to actually trigger an SDIO card
to be fully suspended when it is not needed and thus save a lot of
power. For example when a WLAN interface goes up/down.

> This is the reason why in our implementation polling the card status
> "delays" the runtime suspend while it is not the case when using the
> default runtime suspend implementation.
> I can try to explain here what our controller is doing but since it is
> specific to us then I guess it is not relevant to the discussion.
> Our controller is calling mmc_suspend_host in runtime suspend, which
> issues an HPI to stop the BKOPS.

So, doing mmc_suspend_host in you runtime_suspend callback, is that
really what you want to do?

1.
You will introduce significant latencies (I have seen SD-cards which
needs more than 1 s to initialize, eMMC is better but we are anyway
talking several 100 ms) once new requests arrives after the host as
entered the runtime suspend state.

2.
SD cards has no "power off" notification, so you will actually stress
test the SD cards internal FTL to be crash safe by cutting the power
to it more often.

3.
You will prevent SD-cards from doing it's back ground operations,
which is done automatically and not like in a controlled manner as for
eMMC.

So of course, you save some power, but is the consequences worth it? :-)

> Now that I understand that this code is not needed for all the host
> drivers I will add a flag to decide if polling is required when doing an
> unblocking BKOPS.

You must not poll to prevent this!

Instead you need to prevent the host from going into runtime suspend
state, which is simply done by pm_runtime_get_sync for the host
device.
Although, it _must_ not be done for drivers not doing mmc_suspend_host
in their runtime suspend callbacks. Since then it will prevent these
from doing runtime power save actions, which is not ok.

> Other host drivers that actually suspend on runtime suspend can enable
> this flag and allow BKOPS to be active for a longer period.
> I will prepare a new patch and send it for review.
>
>>
>>> 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
>>> efficient since we don't want to wait until the host is ready to get
>>> into
>>> suspend and then prevent him from suspending by doing BKOPS operations
>>> that can take a long time. It is better to start the BKOPS earlier.
>>
>> I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
>> the card device. It can be an option to implement this feature on top
>> of a workqueue. At least worth to consider.
>>
>
> We consider to call mmc_start_bkops every time MMC becomes idle, to
> check the need for BKOPS. I will test it and include it in the next patch.
>
>>>
>>> I am not too familiar with the controllers code and also my
>>> understanding
>>> in runtime suspend is very basic, so feel free to correct me if I'm
>>> wrong
>>> here or the behavior in other controllers is different from msm_sdcc.
>>> mmc_claim_host calls host->ops->enable. This API is implemented per
>>> controller but as far as I understand it, this API must prevent suspend,
>>> otherwise we might go into suspend while there is bus activity. By doing
>>> get_card_status we call mmc_claim_host and this call is the one that
>>> "delays" getting into suspend.
>>
>> host->ops->enable is the old way of implementing 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 12 December 2012 13:32,  me...@codeaurora.org wrote:
 Hi Ulf,

 Sorry for the late response.
 See my reply below.

 Thanks,
 Maya

 On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
 Hi Maya,

 On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for BKOPS
 periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the host
 should give the card enough time to perform the BKOPS (when it
 recognizes
 BKOPS need), otherwise there is no point in having the periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend by
 polling on the card's status (explanation below), in order to give the
 card time to perform the BKOPS. This has no effect on the power
 consumption since the same BKOPS operations that were performed after
 the
 foregound operation just moved to another location, meaning before going
 into suspend.

 I am not sure what you are talking about here, runtime suspend or
 system suspend? Polling the card's status will not prevent any of
 this. So you have got this wrong.

 I am referring to the runtime suspend.
 Our controller implements the runtime suspend and is not using the default
 implementation of core/bus.c.

This is not the default runtime suspend for a host device, but for
the card device. Do not mix this up with runtime pm for a host device.

Right now runtime pm for the card _device_ is only enabled for SDIO.
Thus SDIO drivers can use runtime pm to actually trigger an SDIO card
to be fully suspended when it is not needed and thus save a lot of
power. For example when a WLAN interface goes up/down.

 This is the reason why in our implementation polling the card status
 delays the runtime suspend while it is not the case when using the
 default runtime suspend implementation.
 I can try to explain here what our controller is doing but since it is
 specific to us then I guess it is not relevant to the discussion.
 Our controller is calling mmc_suspend_host in runtime suspend, which
 issues an HPI to stop the BKOPS.

So, doing mmc_suspend_host in you runtime_suspend callback, is that
really what you want to do?

1.
You will introduce significant latencies (I have seen SD-cards which
needs more than 1 s to initialize, eMMC is better but we are anyway
talking several 100 ms) once new requests arrives after the host as
entered the runtime suspend state.

2.
SD cards has no power off notification, so you will actually stress
test the SD cards internal FTL to be crash safe by cutting the power
to it more often.

3.
You will prevent SD-cards from doing it's back ground operations,
which is done automatically and not like in a controlled manner as for
eMMC.

So of course, you save some power, but is the consequences worth it? :-)

 Now that I understand that this code is not needed for all the host
 drivers I will add a flag to decide if polling is required when doing an
 unblocking BKOPS.

You must not poll to prevent this!

Instead you need to prevent the host from going into runtime suspend
state, which is simply done by pm_runtime_get_sync for the host
device.
Although, it _must_ not be done for drivers not doing mmc_suspend_host
in their runtime suspend callbacks. Since then it will prevent these
from doing runtime power save actions, which is not ok.

 Other host drivers that actually suspend on runtime suspend can enable
 this flag and allow BKOPS to be active for a longer period.
 I will prepare a new patch and send it for review.


 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
 efficient since we don't want to wait until the host is ready to get
 into
 suspend and then prevent him from suspending by doing BKOPS operations
 that can take a long time. It is better to start the BKOPS earlier.

 I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
 the card device. It can be an option to implement this feature on top
 of a workqueue. At least worth to consider.


 We consider to call mmc_start_bkops every time MMC becomes idle, to
 check the need for BKOPS. I will test it and include it in the next patch.


 I am not too familiar with the controllers code and also my
 understanding
 in runtime suspend is very basic, so feel free to correct me if I'm
 wrong
 here or the behavior in other controllers is different from msm_sdcc.
 mmc_claim_host calls host-ops-enable. This API is implemented per
 controller but as far as I understand it, this API must prevent suspend,
 otherwise we might go into suspend while there is bus activity. By doing
 get_card_status we call mmc_claim_host and this call is the one that
 delays getting into suspend.

 host-ops-enable is the old way of implementing runtime power save
 for host drivers. Nowadays most drivers is using runtime PM instead.

 When you say that mmc_claim_host will 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Ulf,

Sorry for the late response.
See my reply below.

Thanks,
Maya

On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
> Hi Maya,
>
> On 4 December 2012 22:17,   wrote:
>> Hi Ulf,
>>
>> Let me try to better explain:
>> The idea behind the periodic BKOPS is to check the card's need for BKOPS
>> periodically in order to prevent an urgent BKOPS need by the card.
>> In order to actually manage to prevent the urgent BKOPS need, the host
>> should give the card enough time to perform the BKOPS (when it
>> recognizes
>> BKOPS need), otherwise there is no point in having the periodic BKOPS.
>> The above results in the following:
>> 1. After starting non-urgent BKOPS we "delay" getting into suspend by
>> polling on the card's status (explanation below), in order to give the
>> card time to perform the BKOPS. This has no effect on the power
>> consumption since the same BKOPS operations that were performed after
>> the
>> foregound operation just moved to another location, meaning before going
>> into suspend.
>
> I am not sure what you are talking about here, runtime suspend or
> system suspend? Polling the card's status will not prevent any of
> this. So you have got this wrong.

I am referring to the runtime suspend.
Our controller implements the runtime suspend and is not using the default
implementation of core/bus.c.
This is the reason why in our implementation polling the card status
"delays" the runtime suspend while it is not the case when using the
default runtime suspend implementation.
I can try to explain here what our controller is doing but since it is
specific to us then I guess it is not relevant to the discussion.
Our controller is calling mmc_suspend_host in runtime suspend, which
issues an HPI to stop the BKOPS.
Now that I understand that this code is not needed for all the host
drivers I will add a flag to decide if polling is required when doing an
unblocking BKOPS.
Other host drivers that actually suspend on runtime suspend can enable
this flag and allow BKOPS to be active for a longer period.
I will prepare a new patch and send it for review.

>
>> 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
>> efficient since we don't want to wait until the host is ready to get
>> into
>> suspend and then prevent him from suspending by doing BKOPS operations
>> that can take a long time. It is better to start the BKOPS earlier.
>
> I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
> the card device. It can be an option to implement this feature on top
> of a workqueue. At least worth to consider.
>

We consider to call mmc_start_bkops every time MMC becomes idle, to
check the need for BKOPS. I will test it and include it in the next patch.

>>
>> I am not too familiar with the controllers code and also my
>> understanding
>> in runtime suspend is very basic, so feel free to correct me if I'm
>> wrong
>> here or the behavior in other controllers is different from msm_sdcc.
>> mmc_claim_host calls host->ops->enable. This API is implemented per
>> controller but as far as I understand it, this API must prevent suspend,
>> otherwise we might go into suspend while there is bus activity. By doing
>> get_card_status we call mmc_claim_host and this call is the one that
>> "delays" getting into suspend.
>
> host->ops->enable is the old way of implementing runtime power save
> for host drivers. Nowadays most drivers is using runtime PM instead.
>
> When you say that mmc_claim_host will prevent suspend, I suppose you
> mean that host->ops->disable wont be called? That is definitely not
> the same as preventing a system suspend, and moreover it should not.
> If you think that the host must be prevented from entering runtime
> power save (runtime_supend or host->ops->disable), you must elaborate
> more on this, because I don't understand why this is needed.
>
Again, this is specific to our controller, so you can just ignore that.

>> If this is not the case in other controllers than the BKOPS will not
>> prevent the suspend and BKOPS will be interrupted.
>> As for the effect on the battery consumption, this is probably something
>> specific to our controller, so sorry if I created a confusion.
>>
>> Additional comments inline.
>>
>> Thanks,
>> Maya
>>
>> On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
>>> On 3 December 2012 10:49,   wrote:
 Hi Jaehoon,

 With this patch we don't expect to see any degradation. Thanks for
 verifying that.
 The test plan would be to run the lmdd and iozone benchmarks with this
 patch and verify that the performance is not degraded.
 I verified it with the msm_sdcc controller.

 Chris - We do expect it to influence the battery consumption, since we
 now
 delay getting into suspend (since mmc_start_bkops which is called
 after
 the delayed work is executed claims the host).
 The solution for that should be done by the controller which can
 shorten
 the timeout given to 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Ulf,

Sorry for the late response.
See my reply below.

Thanks,
Maya

On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote:
 Hi Maya,

 On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for BKOPS
 periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the host
 should give the card enough time to perform the BKOPS (when it
 recognizes
 BKOPS need), otherwise there is no point in having the periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend by
 polling on the card's status (explanation below), in order to give the
 card time to perform the BKOPS. This has no effect on the power
 consumption since the same BKOPS operations that were performed after
 the
 foregound operation just moved to another location, meaning before going
 into suspend.

 I am not sure what you are talking about here, runtime suspend or
 system suspend? Polling the card's status will not prevent any of
 this. So you have got this wrong.

I am referring to the runtime suspend.
Our controller implements the runtime suspend and is not using the default
implementation of core/bus.c.
This is the reason why in our implementation polling the card status
delays the runtime suspend while it is not the case when using the
default runtime suspend implementation.
I can try to explain here what our controller is doing but since it is
specific to us then I guess it is not relevant to the discussion.
Our controller is calling mmc_suspend_host in runtime suspend, which
issues an HPI to stop the BKOPS.
Now that I understand that this code is not needed for all the host
drivers I will add a flag to decide if polling is required when doing an
unblocking BKOPS.
Other host drivers that actually suspend on runtime suspend can enable
this flag and allow BKOPS to be active for a longer period.
I will prepare a new patch and send it for review.


 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
 efficient since we don't want to wait until the host is ready to get
 into
 suspend and then prevent him from suspending by doing BKOPS operations
 that can take a long time. It is better to start the BKOPS earlier.

 I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
 the card device. It can be an option to implement this feature on top
 of a workqueue. At least worth to consider.


We consider to call mmc_start_bkops every time MMC becomes idle, to
check the need for BKOPS. I will test it and include it in the next patch.


 I am not too familiar with the controllers code and also my
 understanding
 in runtime suspend is very basic, so feel free to correct me if I'm
 wrong
 here or the behavior in other controllers is different from msm_sdcc.
 mmc_claim_host calls host-ops-enable. This API is implemented per
 controller but as far as I understand it, this API must prevent suspend,
 otherwise we might go into suspend while there is bus activity. By doing
 get_card_status we call mmc_claim_host and this call is the one that
 delays getting into suspend.

 host-ops-enable is the old way of implementing runtime power save
 for host drivers. Nowadays most drivers is using runtime PM instead.

 When you say that mmc_claim_host will prevent suspend, I suppose you
 mean that host-ops-disable wont be called? That is definitely not
 the same as preventing a system suspend, and moreover it should not.
 If you think that the host must be prevented from entering runtime
 power save (runtime_supend or host-ops-disable), you must elaborate
 more on this, because I don't understand why this is needed.

Again, this is specific to our controller, so you can just ignore that.

 If this is not the case in other controllers than the BKOPS will not
 prevent the suspend and BKOPS will be interrupted.
 As for the effect on the battery consumption, this is probably something
 specific to our controller, so sorry if I created a confusion.

 Additional comments inline.

 Thanks,
 Maya

 On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
 On 3 December 2012 10:49,  me...@codeaurora.org wrote:
 Hi Jaehoon,

 With this patch we don't expect to see any degradation. Thanks for
 verifying that.
 The test plan would be to run the lmdd and iozone benchmarks with this
 patch and verify that the performance is not degraded.
 I verified it with the msm_sdcc controller.

 Chris - We do expect it to influence the battery consumption, since we
 now
 delay getting into suspend (since mmc_start_bkops which is called
 after
 the delayed work is executed claims the host).
 The solution for that should be done by the controller which can
 shorten
 the timeout given to pm_schedule_suspend by the periodic BKOPS idle
 time.
 Does it make sense to you?

 Thanks,
 Maya
 On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

Hi Maya,

On 4 December 2012 22:17,   wrote:
> Hi Ulf,
>
> Let me try to better explain:
> The idea behind the periodic BKOPS is to check the card's need for BKOPS
> periodically in order to prevent an urgent BKOPS need by the card.
> In order to actually manage to prevent the urgent BKOPS need, the host
> should give the card enough time to perform the BKOPS (when it recognizes
> BKOPS need), otherwise there is no point in having the periodic BKOPS.
> The above results in the following:
> 1. After starting non-urgent BKOPS we "delay" getting into suspend by
> polling on the card's status (explanation below), in order to give the
> card time to perform the BKOPS. This has no effect on the power
> consumption since the same BKOPS operations that were performed after the
> foregound operation just moved to another location, meaning before going
> into suspend.

I am not sure what you are talking about here, runtime suspend or
system suspend? Polling the card's status will not prevent any of
this. So you have got this wrong.

> 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
> efficient since we don't want to wait until the host is ready to get into
> suspend and then prevent him from suspending by doing BKOPS operations
> that can take a long time. It is better to start the BKOPS earlier.

I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
the card device. It can be an option to implement this feature on top
of a workqueue. At least worth to consider.

>
> I am not too familiar with the controllers code and also my understanding
> in runtime suspend is very basic, so feel free to correct me if I'm wrong
> here or the behavior in other controllers is different from msm_sdcc.
> mmc_claim_host calls host->ops->enable. This API is implemented per
> controller but as far as I understand it, this API must prevent suspend,
> otherwise we might go into suspend while there is bus activity. By doing
> get_card_status we call mmc_claim_host and this call is the one that
> "delays" getting into suspend.

host->ops->enable is the old way of implementing runtime power save
for host drivers. Nowadays most drivers is using runtime PM instead.

When you say that mmc_claim_host will prevent suspend, I suppose you
mean that host->ops->disable wont be called? That is definitely not
the same as preventing a system suspend, and moreover it should not.
If you think that the host must be prevented from entering runtime
power save (runtime_supend or host->ops->disable), you must elaborate
more on this, because I don't understand why this is needed.

> If this is not the case in other controllers than the BKOPS will not
> prevent the suspend and BKOPS will be interrupted.
> As for the effect on the battery consumption, this is probably something
> specific to our controller, so sorry if I created a confusion.
>
> Additional comments inline.
>
> Thanks,
> Maya
>
> On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
>> On 3 December 2012 10:49,   wrote:
>>> Hi Jaehoon,
>>>
>>> With this patch we don't expect to see any degradation. Thanks for
>>> verifying that.
>>> The test plan would be to run the lmdd and iozone benchmarks with this
>>> patch and verify that the performance is not degraded.
>>> I verified it with the msm_sdcc controller.
>>>
>>> Chris - We do expect it to influence the battery consumption, since we
>>> now
>>> delay getting into suspend (since mmc_start_bkops which is called after
>>> the delayed work is executed claims the host).
>>> The solution for that should be done by the controller which can shorten
>>> the timeout given to pm_schedule_suspend by the periodic BKOPS idle
>>> time.
>>> Does it make sense to you?
>>>
>>> Thanks,
>>> Maya
>>> On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi Maya,

 Thank you a lot for working idle time BKOPS.

 I tested with this patch. It's working fine.(Suspend/resume is also
 working well.)
 Test controller is sdhci controller.
 When i tested the performance with iozone, i didn't find that
 performance
 is decreased.
 Well, as Chris is mentioned, do you have any test plan?
 So I will test more with this patch, because i want to test with dw-mmc
 controller, too.

 On 11/25/2012 08:56 PM, Maya Erez wrote:
> Devices have various maintenance operations need to perform
> internally.
> In order to reduce latencies during time critical operations like read
> and write, it is better to execute maintenance operations in other
> times - when the host is not being serviced. Such operations are
> called
> Background operations (BKOPS).
> The device notifies the status of the BKOPS need by updating
> BKOPS_STATUS
> (EXT_CSD byte [246]).
>
> According to the standard a host that supports BKOPS shall check the
> status periodically and start background operations as needed, so that
> the device has enough time for its 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

Hi Maya,

On 4 December 2012 22:17,  me...@codeaurora.org wrote:
 Hi Ulf,

 Let me try to better explain:
 The idea behind the periodic BKOPS is to check the card's need for BKOPS
 periodically in order to prevent an urgent BKOPS need by the card.
 In order to actually manage to prevent the urgent BKOPS need, the host
 should give the card enough time to perform the BKOPS (when it recognizes
 BKOPS need), otherwise there is no point in having the periodic BKOPS.
 The above results in the following:
 1. After starting non-urgent BKOPS we delay getting into suspend by
 polling on the card's status (explanation below), in order to give the
 card time to perform the BKOPS. This has no effect on the power
 consumption since the same BKOPS operations that were performed after the
 foregound operation just moved to another location, meaning before going
 into suspend.

I am not sure what you are talking about here, runtime suspend or
system suspend? Polling the card's status will not prevent any of
this. So you have got this wrong.

 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
 efficient since we don't want to wait until the host is ready to get into
 suspend and then prevent him from suspending by doing BKOPS operations
 that can take a long time. It is better to start the BKOPS earlier.

I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM for
the card device. It can be an option to implement this feature on top
of a workqueue. At least worth to consider.


 I am not too familiar with the controllers code and also my understanding
 in runtime suspend is very basic, so feel free to correct me if I'm wrong
 here or the behavior in other controllers is different from msm_sdcc.
 mmc_claim_host calls host-ops-enable. This API is implemented per
 controller but as far as I understand it, this API must prevent suspend,
 otherwise we might go into suspend while there is bus activity. By doing
 get_card_status we call mmc_claim_host and this call is the one that
 delays getting into suspend.

host-ops-enable is the old way of implementing runtime power save
for host drivers. Nowadays most drivers is using runtime PM instead.

When you say that mmc_claim_host will prevent suspend, I suppose you
mean that host-ops-disable wont be called? That is definitely not
the same as preventing a system suspend, and moreover it should not.
If you think that the host must be prevented from entering runtime
power save (runtime_supend or host-ops-disable), you must elaborate
more on this, because I don't understand why this is needed.

 If this is not the case in other controllers than the BKOPS will not
 prevent the suspend and BKOPS will be interrupted.
 As for the effect on the battery consumption, this is probably something
 specific to our controller, so sorry if I created a confusion.

 Additional comments inline.

 Thanks,
 Maya

 On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
 On 3 December 2012 10:49,  me...@codeaurora.org wrote:
 Hi Jaehoon,

 With this patch we don't expect to see any degradation. Thanks for
 verifying that.
 The test plan would be to run the lmdd and iozone benchmarks with this
 patch and verify that the performance is not degraded.
 I verified it with the msm_sdcc controller.

 Chris - We do expect it to influence the battery consumption, since we
 now
 delay getting into suspend (since mmc_start_bkops which is called after
 the delayed work is executed claims the host).
 The solution for that should be done by the controller which can shorten
 the timeout given to pm_schedule_suspend by the periodic BKOPS idle
 time.
 Does it make sense to you?

 Thanks,
 Maya
 On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi Maya,

 Thank you a lot for working idle time BKOPS.

 I tested with this patch. It's working fine.(Suspend/resume is also
 working well.)
 Test controller is sdhci controller.
 When i tested the performance with iozone, i didn't find that
 performance
 is decreased.
 Well, as Chris is mentioned, do you have any test plan?
 So I will test more with this patch, because i want to test with dw-mmc
 controller, too.

 On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform
 internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are
 called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating
 BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Ulf,

Let me try to better explain:
The idea behind the periodic BKOPS is to check the card's need for BKOPS
periodically in order to prevent an urgent BKOPS need by the card.
In order to actually manage to prevent the urgent BKOPS need, the host
should give the card enough time to perform the BKOPS (when it recognizes
BKOPS need), otherwise there is no point in having the periodic BKOPS.
The above results in the following:
1. After starting non-urgent BKOPS we "delay" getting into suspend by
polling on the card's status (explanation below), in order to give the
card time to perform the BKOPS. This has no effect on the power
consumption since the same BKOPS operations that were performed after the
foregound operation just moved to another location, meaning before going
into suspend.
2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
efficient since we don't want to wait until the host is ready to get into
suspend and then prevent him from suspending by doing BKOPS operations
that can take a long time. It is better to start the BKOPS earlier.

I am not too familiar with the controllers code and also my understanding
in runtime suspend is very basic, so feel free to correct me if I'm wrong
here or the behavior in other controllers is different from msm_sdcc.
mmc_claim_host calls host->ops->enable. This API is implemented per
controller but as far as I understand it, this API must prevent suspend,
otherwise we might go into suspend while there is bus activity. By doing
get_card_status we call mmc_claim_host and this call is the one that
"delays" getting into suspend.
If this is not the case in other controllers than the BKOPS will not
prevent the suspend and BKOPS will be interrupted.
As for the effect on the battery consumption, this is probably something
specific to our controller, so sorry if I created a confusion.

Additional comments inline.

Thanks,
Maya

On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
> On 3 December 2012 10:49,   wrote:
>> Hi Jaehoon,
>>
>> With this patch we don't expect to see any degradation. Thanks for
>> verifying that.
>> The test plan would be to run the lmdd and iozone benchmarks with this
>> patch and verify that the performance is not degraded.
>> I verified it with the msm_sdcc controller.
>>
>> Chris - We do expect it to influence the battery consumption, since we
>> now
>> delay getting into suspend (since mmc_start_bkops which is called after
>> the delayed work is executed claims the host).
>> The solution for that should be done by the controller which can shorten
>> the timeout given to pm_schedule_suspend by the periodic BKOPS idle
>> time.
>> Does it make sense to you?
>>
>> Thanks,
>> Maya
>> On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
>>> Hi Maya,
>>>
>>> Thank you a lot for working idle time BKOPS.
>>>
>>> I tested with this patch. It's working fine.(Suspend/resume is also
>>> working well.)
>>> Test controller is sdhci controller.
>>> When i tested the performance with iozone, i didn't find that
>>> performance
>>> is decreased.
>>> Well, as Chris is mentioned, do you have any test plan?
>>> So I will test more with this patch, because i want to test with dw-mmc
>>> controller, too.
>>>
>>> On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform
 internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are
 called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating
 BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.

 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.
>
> What host controller suspend timeout are you referring to here?
>
> If you are thinking of the runtime PM autosuspend timeout used in many
> host driver, then you might have missunderstand how runtime PM is used
> in host drivers.
> This has nothing to do with BKOPS as such, unless you think that the
> card must be kept clocked during BKOPS operations, but then this needs
> to be stated somewhere in this patch and that is not the case.
>
> Moreover, I could not find any new 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 3 December 2012 10:49,   wrote:
> Hi Jaehoon,
>
> With this patch we don't expect to see any degradation. Thanks for
> verifying that.
> The test plan would be to run the lmdd and iozone benchmarks with this
> patch and verify that the performance is not degraded.
> I verified it with the msm_sdcc controller.
>
> Chris - We do expect it to influence the battery consumption, since we now
> delay getting into suspend (since mmc_start_bkops which is called after
> the delayed work is executed claims the host).
> The solution for that should be done by the controller which can shorten
> the timeout given to pm_schedule_suspend by the periodic BKOPS idle time.
> Does it make sense to you?
>
> Thanks,
> Maya
> On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
>> Hi Maya,
>>
>> Thank you a lot for working idle time BKOPS.
>>
>> I tested with this patch. It's working fine.(Suspend/resume is also
>> working well.)
>> Test controller is sdhci controller.
>> When i tested the performance with iozone, i didn't find that performance
>> is decreased.
>> Well, as Chris is mentioned, do you have any test plan?
>> So I will test more with this patch, because i want to test with dw-mmc
>> controller, too.
>>
>> On 11/25/2012 08:56 PM, Maya Erez wrote:
>>> Devices have various maintenance operations need to perform internally.
>>> In order to reduce latencies during time critical operations like read
>>> and write, it is better to execute maintenance operations in other
>>> times - when the host is not being serviced. Such operations are called
>>> Background operations (BKOPS).
>>> The device notifies the status of the BKOPS need by updating
>>> BKOPS_STATUS
>>> (EXT_CSD byte [246]).
>>>
>>> According to the standard a host that supports BKOPS shall check the
>>> status periodically and start background operations as needed, so that
>>> the device has enough time for its maintenance operations.
>>>
>>> This patch adds support for this periodic check of the BKOPS status.
>>> Since foreground operations are of higher priority than background
>>> operations the host will check the need for BKOPS when it is idle,
>>> and in case of an incoming request the BKOPS operation will be
>>> interrupted.
>>>
>>> When the mmcqd thread is idle, a delayed work is created to check the
>>> need for BKOPS. The time to start the delayed work is calculated based
>>> on the host controller suspend timeout, in case it was set. If not, a
>>> default time is used.

What host controller suspend timeout are you referring to here?

If you are thinking of the runtime PM autosuspend timeout used in many
host driver, then you might have missunderstand how runtime PM is used
in host drivers.
This has nothing to do with BKOPS as such, unless you think that the
card must be kept clocked during BKOPS operations, but then this needs
to be stated somewhere in this patch and that is not the case.

Moreover, I could not find any new timeout added for the _host_ struct
in this patch.

>>> If BKOPS are required in level 1, which is non-blocking, there will be
>>> polling of the card status to wait for the BKOPS completion and prevent
>>> suspend that will interrupt the BKOPS.

Not sure of what suspend you are talking about here. But for sure
BKOPS must _never_ prevent a system suspend.

You might want to prevent a host from being runtime suspended though,
but that is not accomplished in this patch.

>>> If the card raised an exception, the need for urgent BKOPS (level 2/3)
>>> will be checked immediately and if needed, the BKOPS will be performed
>>> without waiting for the next idle time.
>>>
>>> Signed-off-by: Maya Erez 
>>>
>>> ---
>>> This patch is based on the periodic BKOPS implementation in version 8 of
>>> "support BKOPS feature for eMMC" patch.
>>> The patch was modified to answer the following issues:
>>> - In order to prevent a race condition between going into suspend and
>>> starting BKOPS,
>>>   the suspend timeout of the host controller is taking into accound in
>>> determination of the start time
>>>   of the delayed work
>>> - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
>>> was moved to the beginning of the function
>>> - Also, the check of doing_bkops should be protected when determing if
>>> an HPI is needed due to the same reason.
>>>
>>> Changes in v3:
>>> - Move the call to stop_bkops to block.c.
>>>   This allows us to remove the mmc_claim_host from inside the
>>> function and doesn't cause additional degradation
>>>   due to un-neccessary calim host operation
>>>
>>> Changes in v2:
>>> - Check the number of written / discarded sectors as the trigger for
>>> checking the BKOPS need.
>>> - Code review fixes
>>>
>>> ---
>>>  drivers/mmc/card/block.c |8 ++-
>>>  drivers/mmc/card/queue.c |2 +
>>>  drivers/mmc/core/core.c  |  178
>>> +++---
>>>  drivers/mmc/core/mmc.c   |   23 ++
>>>  include/linux/mmc/card.h |   35 +
>>>  

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

On 3 December 2012 10:49,  me...@codeaurora.org wrote:
 Hi Jaehoon,

 With this patch we don't expect to see any degradation. Thanks for
 verifying that.
 The test plan would be to run the lmdd and iozone benchmarks with this
 patch and verify that the performance is not degraded.
 I verified it with the msm_sdcc controller.

 Chris - We do expect it to influence the battery consumption, since we now
 delay getting into suspend (since mmc_start_bkops which is called after
 the delayed work is executed claims the host).
 The solution for that should be done by the controller which can shorten
 the timeout given to pm_schedule_suspend by the periodic BKOPS idle time.
 Does it make sense to you?

 Thanks,
 Maya
 On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi Maya,

 Thank you a lot for working idle time BKOPS.

 I tested with this patch. It's working fine.(Suspend/resume is also
 working well.)
 Test controller is sdhci controller.
 When i tested the performance with iozone, i didn't find that performance
 is decreased.
 Well, as Chris is mentioned, do you have any test plan?
 So I will test more with this patch, because i want to test with dw-mmc
 controller, too.

 On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating
 BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.

 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.

What host controller suspend timeout are you referring to here?

If you are thinking of the runtime PM autosuspend timeout used in many
host driver, then you might have missunderstand how runtime PM is used
in host drivers.
This has nothing to do with BKOPS as such, unless you think that the
card must be kept clocked during BKOPS operations, but then this needs
to be stated somewhere in this patch and that is not the case.

Moreover, I could not find any new timeout added for the _host_ struct
in this patch.

 If BKOPS are required in level 1, which is non-blocking, there will be
 polling of the card status to wait for the BKOPS completion and prevent
 suspend that will interrupt the BKOPS.

Not sure of what suspend you are talking about here. But for sure
BKOPS must _never_ prevent a system suspend.

You might want to prevent a host from being runtime suspended though,
but that is not accomplished in this patch.

 If the card raised an exception, the need for urgent BKOPS (level 2/3)
 will be checked immediately and if needed, the BKOPS will be performed
 without waiting for the next idle time.

 Signed-off-by: Maya Erez me...@codeaurora.org

 ---
 This patch is based on the periodic BKOPS implementation in version 8 of
 support BKOPS feature for eMMC patch.
 The patch was modified to answer the following issues:
 - In order to prevent a race condition between going into suspend and
 starting BKOPS,
   the suspend timeout of the host controller is taking into accound in
 determination of the start time
   of the delayed work
 - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
 was moved to the beginning of the function
 - Also, the check of doing_bkops should be protected when determing if
 an HPI is needed due to the same reason.

 Changes in v3:
 - Move the call to stop_bkops to block.c.
   This allows us to remove the mmc_claim_host from inside the
 function and doesn't cause additional degradation
   due to un-neccessary calim host operation

 Changes in v2:
 - Check the number of written / discarded sectors as the trigger for
 checking the BKOPS need.
 - Code review fixes

 ---
  drivers/mmc/card/block.c |8 ++-
  drivers/mmc/card/queue.c |2 +
  drivers/mmc/core/core.c  |  178
 +++---
  drivers/mmc/core/mmc.c   |   23 ++
  include/linux/mmc/card.h |   35 +
  include/linux/mmc/core.h |3 +
  6 files changed, 237 insertions(+), 12 deletions(-)

 diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
 index 172a768..40b4ae3 100644
 --- 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Ulf,

Let me try to better explain:
The idea behind the periodic BKOPS is to check the card's need for BKOPS
periodically in order to prevent an urgent BKOPS need by the card.
In order to actually manage to prevent the urgent BKOPS need, the host
should give the card enough time to perform the BKOPS (when it recognizes
BKOPS need), otherwise there is no point in having the periodic BKOPS.
The above results in the following:
1. After starting non-urgent BKOPS we delay getting into suspend by
polling on the card's status (explanation below), in order to give the
card time to perform the BKOPS. This has no effect on the power
consumption since the same BKOPS operations that were performed after the
foregound operation just moved to another location, meaning before going
into suspend.
2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be
efficient since we don't want to wait until the host is ready to get into
suspend and then prevent him from suspending by doing BKOPS operations
that can take a long time. It is better to start the BKOPS earlier.

I am not too familiar with the controllers code and also my understanding
in runtime suspend is very basic, so feel free to correct me if I'm wrong
here or the behavior in other controllers is different from msm_sdcc.
mmc_claim_host calls host-ops-enable. This API is implemented per
controller but as far as I understand it, this API must prevent suspend,
otherwise we might go into suspend while there is bus activity. By doing
get_card_status we call mmc_claim_host and this call is the one that
delays getting into suspend.
If this is not the case in other controllers than the BKOPS will not
prevent the suspend and BKOPS will be interrupted.
As for the effect on the battery consumption, this is probably something
specific to our controller, so sorry if I created a confusion.

Additional comments inline.

Thanks,
Maya

On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote:
 On 3 December 2012 10:49,  me...@codeaurora.org wrote:
 Hi Jaehoon,

 With this patch we don't expect to see any degradation. Thanks for
 verifying that.
 The test plan would be to run the lmdd and iozone benchmarks with this
 patch and verify that the performance is not degraded.
 I verified it with the msm_sdcc controller.

 Chris - We do expect it to influence the battery consumption, since we
 now
 delay getting into suspend (since mmc_start_bkops which is called after
 the delayed work is executed claims the host).
 The solution for that should be done by the controller which can shorten
 the timeout given to pm_schedule_suspend by the periodic BKOPS idle
 time.
 Does it make sense to you?

 Thanks,
 Maya
 On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi Maya,

 Thank you a lot for working idle time BKOPS.

 I tested with this patch. It's working fine.(Suspend/resume is also
 working well.)
 Test controller is sdhci controller.
 When i tested the performance with iozone, i didn't find that
 performance
 is decreased.
 Well, as Chris is mentioned, do you have any test plan?
 So I will test more with this patch, because i want to test with dw-mmc
 controller, too.

 On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform
 internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are
 called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating
 BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.

 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.

 What host controller suspend timeout are you referring to here?

 If you are thinking of the runtime PM autosuspend timeout used in many
 host driver, then you might have missunderstand how runtime PM is used
 in host drivers.
 This has nothing to do with BKOPS as such, unless you think that the
 card must be kept clocked during BKOPS operations, but then this needs
 to be stated somewhere in this patch and that is not the case.

 Moreover, I could not find any new timeout added for the _host_ struct
 in this patch.
Yes, I was referring to the runtime PM autosuspend timeout. Since we want
to give the BKOPS time to be performed before going into 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Jaehoon,

With this patch we don't expect to see any degradation. Thanks for
verifying that.
The test plan would be to run the lmdd and iozone benchmarks with this
patch and verify that the performance is not degraded.
I verified it with the msm_sdcc controller.

Chris - We do expect it to influence the battery consumption, since we now
delay getting into suspend (since mmc_start_bkops which is called after
the delayed work is executed claims the host).
The solution for that should be done by the controller which can shorten
the timeout given to pm_schedule_suspend by the periodic BKOPS idle time.
Does it make sense to you?

Thanks,
Maya
On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
> Hi Maya,
>
> Thank you a lot for working idle time BKOPS.
>
> I tested with this patch. It's working fine.(Suspend/resume is also
> working well.)
> Test controller is sdhci controller.
> When i tested the performance with iozone, i didn't find that performance
> is decreased.
> Well, as Chris is mentioned, do you have any test plan?
> So I will test more with this patch, because i want to test with dw-mmc
> controller, too.
>
> On 11/25/2012 08:56 PM, Maya Erez wrote:
>> Devices have various maintenance operations need to perform internally.
>> In order to reduce latencies during time critical operations like read
>> and write, it is better to execute maintenance operations in other
>> times - when the host is not being serviced. Such operations are called
>> Background operations (BKOPS).
>> The device notifies the status of the BKOPS need by updating
>> BKOPS_STATUS
>> (EXT_CSD byte [246]).
>>
>> According to the standard a host that supports BKOPS shall check the
>> status periodically and start background operations as needed, so that
>> the device has enough time for its maintenance operations.
>>
>> This patch adds support for this periodic check of the BKOPS status.
>> Since foreground operations are of higher priority than background
>> operations the host will check the need for BKOPS when it is idle,
>> and in case of an incoming request the BKOPS operation will be
>> interrupted.
>>
>> When the mmcqd thread is idle, a delayed work is created to check the
>> need for BKOPS. The time to start the delayed work is calculated based
>> on the host controller suspend timeout, in case it was set. If not, a
>> default time is used.
>> If BKOPS are required in level 1, which is non-blocking, there will be
>> polling of the card status to wait for the BKOPS completion and prevent
>> suspend that will interrupt the BKOPS.
>> If the card raised an exception, the need for urgent BKOPS (level 2/3)
>> will be checked immediately and if needed, the BKOPS will be performed
>> without waiting for the next idle time.
>>
>> Signed-off-by: Maya Erez 
>>
>> ---
>> This patch is based on the periodic BKOPS implementation in version 8 of
>> "support BKOPS feature for eMMC" patch.
>> The patch was modified to answer the following issues:
>> - In order to prevent a race condition between going into suspend and
>> starting BKOPS,
>>   the suspend timeout of the host controller is taking into accound in
>> determination of the start time
>>   of the delayed work
>> - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
>> was moved to the beginning of the function
>> - Also, the check of doing_bkops should be protected when determing if
>> an HPI is needed due to the same reason.
>>
>> Changes in v3:
>> - Move the call to stop_bkops to block.c.
>>   This allows us to remove the mmc_claim_host from inside the
>> function and doesn't cause additional degradation
>>   due to un-neccessary calim host operation
>>
>> Changes in v2:
>> - Check the number of written / discarded sectors as the trigger for
>> checking the BKOPS need.
>> - Code review fixes
>>
>> ---
>>  drivers/mmc/card/block.c |8 ++-
>>  drivers/mmc/card/queue.c |2 +
>>  drivers/mmc/core/core.c  |  178
>> +++---
>>  drivers/mmc/core/mmc.c   |   23 ++
>>  include/linux/mmc/card.h |   35 +
>>  include/linux/mmc/core.h |3 +
>>  6 files changed, 237 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 172a768..40b4ae3 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>> struct request *req)
>>  struct mmc_blk_data *md = mq->data;
>>  struct mmc_card *card = md->queue.card;
>>
>> -if (req && !mq->mqrq_prev->req)
>> +if (req && !mq->mqrq_prev->req) {
>>  /* claim host only for the first request */
>>  mmc_claim_host(card->host);
>> +if (card->ext_csd.bkops_en &&
>> +card->bkops_info.started_delayed_bkops) {
>> +card->bkops_info.started_delayed_bkops = false;
>> +mmc_stop_bkops(card);
> We didn't 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Jaehoon,

With this patch we don't expect to see any degradation. Thanks for
verifying that.
The test plan would be to run the lmdd and iozone benchmarks with this
patch and verify that the performance is not degraded.
I verified it with the msm_sdcc controller.

Chris - We do expect it to influence the battery consumption, since we now
delay getting into suspend (since mmc_start_bkops which is called after
the delayed work is executed claims the host).
The solution for that should be done by the controller which can shorten
the timeout given to pm_schedule_suspend by the periodic BKOPS idle time.
Does it make sense to you?

Thanks,
Maya
On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote:
 Hi Maya,

 Thank you a lot for working idle time BKOPS.

 I tested with this patch. It's working fine.(Suspend/resume is also
 working well.)
 Test controller is sdhci controller.
 When i tested the performance with iozone, i didn't find that performance
 is decreased.
 Well, as Chris is mentioned, do you have any test plan?
 So I will test more with this patch, because i want to test with dw-mmc
 controller, too.

 On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating
 BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.

 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.
 If BKOPS are required in level 1, which is non-blocking, there will be
 polling of the card status to wait for the BKOPS completion and prevent
 suspend that will interrupt the BKOPS.
 If the card raised an exception, the need for urgent BKOPS (level 2/3)
 will be checked immediately and if needed, the BKOPS will be performed
 without waiting for the next idle time.

 Signed-off-by: Maya Erez me...@codeaurora.org

 ---
 This patch is based on the periodic BKOPS implementation in version 8 of
 support BKOPS feature for eMMC patch.
 The patch was modified to answer the following issues:
 - In order to prevent a race condition between going into suspend and
 starting BKOPS,
   the suspend timeout of the host controller is taking into accound in
 determination of the start time
   of the delayed work
 - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
 was moved to the beginning of the function
 - Also, the check of doing_bkops should be protected when determing if
 an HPI is needed due to the same reason.

 Changes in v3:
 - Move the call to stop_bkops to block.c.
   This allows us to remove the mmc_claim_host from inside the
 function and doesn't cause additional degradation
   due to un-neccessary calim host operation

 Changes in v2:
 - Check the number of written / discarded sectors as the trigger for
 checking the BKOPS need.
 - Code review fixes

 ---
  drivers/mmc/card/block.c |8 ++-
  drivers/mmc/card/queue.c |2 +
  drivers/mmc/core/core.c  |  178
 +++---
  drivers/mmc/core/mmc.c   |   23 ++
  include/linux/mmc/card.h |   35 +
  include/linux/mmc/core.h |3 +
  6 files changed, 237 insertions(+), 12 deletions(-)

 diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
 index 172a768..40b4ae3 100644
 --- a/drivers/mmc/card/block.c
 +++ b/drivers/mmc/card/block.c
 @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
 struct request *req)
  struct mmc_blk_data *md = mq-data;
  struct mmc_card *card = md-queue.card;

 -if (req  !mq-mqrq_prev-req)
 +if (req  !mq-mqrq_prev-req) {
  /* claim host only for the first request */
  mmc_claim_host(card-host);
 +if (card-ext_csd.bkops_en 
 +card-bkops_info.started_delayed_bkops) {
 +card-bkops_info.started_delayed_bkops = false;
 +mmc_stop_bkops(card);
 We didn't need to check whether mmc_stop_bkops is success or not?
 If mmc_stop_bkops() is failed, then bkops is continuously running.

 Best Regards,
 Jaehoon Chung

 +}
 +}

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Jaehoon Chung]

Hi Maya,

Thank you a lot for working idle time BKOPS.

I tested with this patch. It's working fine.(Suspend/resume is also working 
well.)
Test controller is sdhci controller. 
When i tested the performance with iozone, i didn't find that performance is 
decreased.
Well, as Chris is mentioned, do you have any test plan?
So I will test more with this patch, because i want to test with dw-mmc 
controller, too.

On 11/25/2012 08:56 PM, Maya Erez wrote:
> Devices have various maintenance operations need to perform internally.
> In order to reduce latencies during time critical operations like read
> and write, it is better to execute maintenance operations in other
> times - when the host is not being serviced. Such operations are called
> Background operations (BKOPS).
> The device notifies the status of the BKOPS need by updating BKOPS_STATUS
> (EXT_CSD byte [246]).
> 
> According to the standard a host that supports BKOPS shall check the
> status periodically and start background operations as needed, so that
> the device has enough time for its maintenance operations.
> 
> This patch adds support for this periodic check of the BKOPS status.
> Since foreground operations are of higher priority than background
> operations the host will check the need for BKOPS when it is idle,
> and in case of an incoming request the BKOPS operation will be
> interrupted.
> 
> When the mmcqd thread is idle, a delayed work is created to check the
> need for BKOPS. The time to start the delayed work is calculated based
> on the host controller suspend timeout, in case it was set. If not, a
> default time is used.
> If BKOPS are required in level 1, which is non-blocking, there will be
> polling of the card status to wait for the BKOPS completion and prevent
> suspend that will interrupt the BKOPS.
> If the card raised an exception, the need for urgent BKOPS (level 2/3)
> will be checked immediately and if needed, the BKOPS will be performed
> without waiting for the next idle time.
> 
> Signed-off-by: Maya Erez 
> 
> ---
> This patch is based on the periodic BKOPS implementation in version 8 of 
> "support BKOPS feature for eMMC" patch.
> The patch was modified to answer the following issues:
> - In order to prevent a race condition between going into suspend and 
> starting BKOPS, 
>   the suspend timeout of the host controller is taking into accound in 
> determination of the start time 
>   of the delayed work
> - Since mmc_start_bkops is called from two contexts now, mmc_claim_host was 
> moved to the beginning of the function
> - Also, the check of doing_bkops should be protected when determing if an HPI 
> is needed due to the same reason.
> 
> Changes in v3:
> - Move the call to stop_bkops to block.c. 
>   This allows us to remove the mmc_claim_host from inside the function 
> and doesn't cause additional degradation 
>   due to un-neccessary calim host operation
> 
> Changes in v2:
> - Check the number of written / discarded sectors as the trigger for 
> checking the BKOPS need.
> - Code review fixes
> 
> ---
>  drivers/mmc/card/block.c |8 ++-
>  drivers/mmc/card/queue.c |2 +
>  drivers/mmc/core/core.c  |  178 
> +++---
>  drivers/mmc/core/mmc.c   |   23 ++
>  include/linux/mmc/card.h |   35 +
>  include/linux/mmc/core.h |3 +
>  6 files changed, 237 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 172a768..40b4ae3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, 
> struct request *req)
>   struct mmc_blk_data *md = mq->data;
>   struct mmc_card *card = md->queue.card;
>  
> - if (req && !mq->mqrq_prev->req)
> + if (req && !mq->mqrq_prev->req) {
>   /* claim host only for the first request */
>   mmc_claim_host(card->host);
> + if (card->ext_csd.bkops_en &&
> + card->bkops_info.started_delayed_bkops) {
> + card->bkops_info.started_delayed_bkops = false;
> + mmc_stop_bkops(card);
We didn't need to check whether mmc_stop_bkops is success or not?
If mmc_stop_bkops() is failed, then bkops is continuously running.

Best Regards,
Jaehoon Chung

> + }
> + }
>  
>   ret = mmc_blk_part_switch(card, md);
>   if (ret) {
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index fadf52e..9d0c96a 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
>  {
>   struct mmc_queue *mq = d;
>   struct request_queue *q = mq->queue;
> + struct mmc_card *card = mq->card;
>  
>   current->flags |= PF_MEMALLOC;
>  
> @@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
>   set_current_state(TASK_RUNNING);
> 

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Jaehoon Chung]

Hi Maya,

Thank you a lot for working idle time BKOPS.

I tested with this patch. It's working fine.(Suspend/resume is also working 
well.)
Test controller is sdhci controller. 
When i tested the performance with iozone, i didn't find that performance is 
decreased.
Well, as Chris is mentioned, do you have any test plan?
So I will test more with this patch, because i want to test with dw-mmc 
controller, too.

On 11/25/2012 08:56 PM, Maya Erez wrote:
 Devices have various maintenance operations need to perform internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating BKOPS_STATUS
 (EXT_CSD byte [246]).
 
 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.
 
 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.
 
 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.
 If BKOPS are required in level 1, which is non-blocking, there will be
 polling of the card status to wait for the BKOPS completion and prevent
 suspend that will interrupt the BKOPS.
 If the card raised an exception, the need for urgent BKOPS (level 2/3)
 will be checked immediately and if needed, the BKOPS will be performed
 without waiting for the next idle time.
 
 Signed-off-by: Maya Erez me...@codeaurora.org
 
 ---
 This patch is based on the periodic BKOPS implementation in version 8 of 
 support BKOPS feature for eMMC patch.
 The patch was modified to answer the following issues:
 - In order to prevent a race condition between going into suspend and 
 starting BKOPS, 
   the suspend timeout of the host controller is taking into accound in 
 determination of the start time 
   of the delayed work
 - Since mmc_start_bkops is called from two contexts now, mmc_claim_host was 
 moved to the beginning of the function
 - Also, the check of doing_bkops should be protected when determing if an HPI 
 is needed due to the same reason.
 
 Changes in v3:
 - Move the call to stop_bkops to block.c. 
   This allows us to remove the mmc_claim_host from inside the function 
 and doesn't cause additional degradation 
   due to un-neccessary calim host operation
 
 Changes in v2:
 - Check the number of written / discarded sectors as the trigger for 
 checking the BKOPS need.
 - Code review fixes
 
 ---
  drivers/mmc/card/block.c |8 ++-
  drivers/mmc/card/queue.c |2 +
  drivers/mmc/core/core.c  |  178 
 +++---
  drivers/mmc/core/mmc.c   |   23 ++
  include/linux/mmc/card.h |   35 +
  include/linux/mmc/core.h |3 +
  6 files changed, 237 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
 index 172a768..40b4ae3 100644
 --- a/drivers/mmc/card/block.c
 +++ b/drivers/mmc/card/block.c
 @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, 
 struct request *req)
   struct mmc_blk_data *md = mq-data;
   struct mmc_card *card = md-queue.card;
  
 - if (req  !mq-mqrq_prev-req)
 + if (req  !mq-mqrq_prev-req) {
   /* claim host only for the first request */
   mmc_claim_host(card-host);
 + if (card-ext_csd.bkops_en 
 + card-bkops_info.started_delayed_bkops) {
 + card-bkops_info.started_delayed_bkops = false;
 + mmc_stop_bkops(card);
We didn't need to check whether mmc_stop_bkops is success or not?
If mmc_stop_bkops() is failed, then bkops is continuously running.

Best Regards,
Jaehoon Chung

 + }
 + }
  
   ret = mmc_blk_part_switch(card, md);
   if (ret) {
 diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
 index fadf52e..9d0c96a 100644
 --- a/drivers/mmc/card/queue.c
 +++ b/drivers/mmc/card/queue.c
 @@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
  {
   struct mmc_queue *mq = d;
   struct request_queue *q = mq-queue;
 + struct mmc_card *card = mq-card;
  
   current-flags |= PF_MEMALLOC;
  
 @@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
   set_current_state(TASK_RUNNING);
   break;
   }
 + mmc_start_delayed_bkops(card);

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Chris Ball]

Hi Maya,

On Sun, Nov 25 2012, me...@codeaurora.org wrote:
> I managed to find a solution in which there is no need to check the number
> of written / discarded sectors as a trigger for BKOPS status check.
> I moved the code that checks the need to stop the BKOPS to mmc/block code,
> in which there is no need for additional claim_host and remove_host
> operations.
> Please review this patch.

Looks good -- if we find that counting with a bitmask of eraseblocks is
necessary it can be added on later.

Do you have a testing plan for the patch?  I wonder if we could test
the effects on battery life and latency.

Thanks!

- Chris.
-- 
Chris Ball  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Chris Ball]

Hi Maya,

On Sun, Nov 25 2012, me...@codeaurora.org wrote:
 I managed to find a solution in which there is no need to check the number
 of written / discarded sectors as a trigger for BKOPS status check.
 I moved the code that checks the need to stop the BKOPS to mmc/block code,
 in which there is no need for additional claim_host and remove_host
 operations.
 Please review this patch.

Looks good -- if we find that counting with a bitmask of eraseblocks is
necessary it can be added on later.

Do you have a testing plan for the patch?  I wonder if we could test
the effects on battery life and latency.

Thanks!

- Chris.
-- 
Chris Ball   c...@laptop.org   http://printf.net/
One Laptop Per Child
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Chris,

I managed to find a solution in which there is no need to check the number
of written / discarded sectors as a trigger for BKOPS status check.
I moved the code that checks the need to stop the BKOPS to mmc/block code,
in which there is no need for additional claim_host and remove_host
operations.
Please review this patch.

Thanks,
Maya
On Sun, November 25, 2012 3:56 am, Maya Erez wrote:
> Devices have various maintenance operations need to perform internally.
> In order to reduce latencies during time critical operations like read
> and write, it is better to execute maintenance operations in other
> times - when the host is not being serviced. Such operations are called
> Background operations (BKOPS).
> The device notifies the status of the BKOPS need by updating BKOPS_STATUS
> (EXT_CSD byte [246]).
>
> According to the standard a host that supports BKOPS shall check the
> status periodically and start background operations as needed, so that
> the device has enough time for its maintenance operations.
>
> This patch adds support for this periodic check of the BKOPS status.
> Since foreground operations are of higher priority than background
> operations the host will check the need for BKOPS when it is idle,
> and in case of an incoming request the BKOPS operation will be
> interrupted.
>
> When the mmcqd thread is idle, a delayed work is created to check the
> need for BKOPS. The time to start the delayed work is calculated based
> on the host controller suspend timeout, in case it was set. If not, a
> default time is used.
> If BKOPS are required in level 1, which is non-blocking, there will be
> polling of the card status to wait for the BKOPS completion and prevent
> suspend that will interrupt the BKOPS.
> If the card raised an exception, the need for urgent BKOPS (level 2/3)
> will be checked immediately and if needed, the BKOPS will be performed
> without waiting for the next idle time.
>
> Signed-off-by: Maya Erez 
>
> ---
> This patch is based on the periodic BKOPS implementation in version 8 of
> "support BKOPS feature for eMMC" patch.
> The patch was modified to answer the following issues:
> - In order to prevent a race condition between going into suspend and
> starting BKOPS,
>   the suspend timeout of the host controller is taking into accound in
> determination of the start time
>   of the delayed work
> - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
> was moved to the beginning of the function
> - Also, the check of doing_bkops should be protected when determing if an
> HPI is needed due to the same reason.
>
> Changes in v3:
> - Move the call to stop_bkops to block.c.
>   This allows us to remove the mmc_claim_host from inside the function
> and doesn't cause additional degradation
>   due to un-neccessary calim host operation
>
> Changes in v2:
> - Check the number of written / discarded sectors as the trigger for
> checking the BKOPS need.
> - Code review fixes
>
> ---
>  drivers/mmc/card/block.c |8 ++-
>  drivers/mmc/card/queue.c |2 +
>  drivers/mmc/core/core.c  |  178
> +++---
>  drivers/mmc/core/mmc.c   |   23 ++
>  include/linux/mmc/card.h |   35 +
>  include/linux/mmc/core.h |3 +
>  6 files changed, 237 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 172a768..40b4ae3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
>   struct mmc_blk_data *md = mq->data;
>   struct mmc_card *card = md->queue.card;
>
> - if (req && !mq->mqrq_prev->req)
> + if (req && !mq->mqrq_prev->req) {
>   /* claim host only for the first request */
>   mmc_claim_host(card->host);
> + if (card->ext_csd.bkops_en &&
> + card->bkops_info.started_delayed_bkops) {
> + card->bkops_info.started_delayed_bkops = false;
> + mmc_stop_bkops(card);
> + }
> + }
>
>   ret = mmc_blk_part_switch(card, md);
>   if (ret) {
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index fadf52e..9d0c96a 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
>  {
>   struct mmc_queue *mq = d;
>   struct request_queue *q = mq->queue;
> + struct mmc_card *card = mq->card;
>
>   current->flags |= PF_MEMALLOC;
>
> @@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
>   set_current_state(TASK_RUNNING);
>   break;
>   }
> + mmc_start_delayed_bkops(card);
>   up(>thread_sem);
>   schedule();
>   down(>thread_sem);
> diff --git 

[PATCH v3] mmc: core: Add support for idle time BKOPS

[Maya Erez]

Devices have various maintenance operations need to perform internally.
In order to reduce latencies during time critical operations like read
and write, it is better to execute maintenance operations in other
times - when the host is not being serviced. Such operations are called
Background operations (BKOPS).
The device notifies the status of the BKOPS need by updating BKOPS_STATUS
(EXT_CSD byte [246]).

According to the standard a host that supports BKOPS shall check the
status periodically and start background operations as needed, so that
the device has enough time for its maintenance operations.

This patch adds support for this periodic check of the BKOPS status.
Since foreground operations are of higher priority than background
operations the host will check the need for BKOPS when it is idle,
and in case of an incoming request the BKOPS operation will be
interrupted.

When the mmcqd thread is idle, a delayed work is created to check the
need for BKOPS. The time to start the delayed work is calculated based
on the host controller suspend timeout, in case it was set. If not, a
default time is used.
If BKOPS are required in level 1, which is non-blocking, there will be
polling of the card status to wait for the BKOPS completion and prevent
suspend that will interrupt the BKOPS.
If the card raised an exception, the need for urgent BKOPS (level 2/3)
will be checked immediately and if needed, the BKOPS will be performed
without waiting for the next idle time.

Signed-off-by: Maya Erez 

---
This patch is based on the periodic BKOPS implementation in version 8 of 
"support BKOPS feature for eMMC" patch.
The patch was modified to answer the following issues:
- In order to prevent a race condition between going into suspend and starting 
BKOPS, 
  the suspend timeout of the host controller is taking into accound in 
determination of the start time 
  of the delayed work
- Since mmc_start_bkops is called from two contexts now, mmc_claim_host was 
moved to the beginning of the function
- Also, the check of doing_bkops should be protected when determing if an HPI 
is needed due to the same reason.

Changes in v3:
- Move the call to stop_bkops to block.c. 
  This allows us to remove the mmc_claim_host from inside the function and 
doesn't cause additional degradation 
  due to un-neccessary calim host operation

Changes in v2:
- Check the number of written / discarded sectors as the trigger for 
checking the BKOPS need.
- Code review fixes

---
 drivers/mmc/card/block.c |8 ++-
 drivers/mmc/card/queue.c |2 +
 drivers/mmc/core/core.c  |  178 +++---
 drivers/mmc/core/mmc.c   |   23 ++
 include/linux/mmc/card.h |   35 +
 include/linux/mmc/core.h |3 +
 6 files changed, 237 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 172a768..40b4ae3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct 
request *req)
struct mmc_blk_data *md = mq->data;
struct mmc_card *card = md->queue.card;
 
-   if (req && !mq->mqrq_prev->req)
+   if (req && !mq->mqrq_prev->req) {
/* claim host only for the first request */
mmc_claim_host(card->host);
+   if (card->ext_csd.bkops_en &&
+   card->bkops_info.started_delayed_bkops) {
+   card->bkops_info.started_delayed_bkops = false;
+   mmc_stop_bkops(card);
+   }
+   }
 
ret = mmc_blk_part_switch(card, md);
if (ret) {
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index fadf52e..9d0c96a 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
 {
struct mmc_queue *mq = d;
struct request_queue *q = mq->queue;
+   struct mmc_card *card = mq->card;
 
current->flags |= PF_MEMALLOC;
 
@@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
set_current_state(TASK_RUNNING);
break;
}
+   mmc_start_delayed_bkops(card);
up(>thread_sem);
schedule();
down(>thread_sem);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 06c42cf..72ae15b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -253,9 +253,36 @@ mmc_start_request(struct mmc_host *host, struct 
mmc_request *mrq)
 }
 
 /**
+ * mmc_start_delayed_bkops() - Start a delayed work to check for
+ *  the need of non urgent BKOPS
+ *
+ * @card: MMC card to start BKOPS on
+ */
+void mmc_start_delayed_bkops(struct mmc_card *card)
+{
+   if (!card || !card->ext_csd.bkops_en || mmc_card_doing_bkops(card))
+   return;
+
+  

[PATCH v3] mmc: core: Add support for idle time BKOPS

[Maya Erez]

Devices have various maintenance operations need to perform internally.
In order to reduce latencies during time critical operations like read
and write, it is better to execute maintenance operations in other
times - when the host is not being serviced. Such operations are called
Background operations (BKOPS).
The device notifies the status of the BKOPS need by updating BKOPS_STATUS
(EXT_CSD byte [246]).

According to the standard a host that supports BKOPS shall check the
status periodically and start background operations as needed, so that
the device has enough time for its maintenance operations.

This patch adds support for this periodic check of the BKOPS status.
Since foreground operations are of higher priority than background
operations the host will check the need for BKOPS when it is idle,
and in case of an incoming request the BKOPS operation will be
interrupted.

When the mmcqd thread is idle, a delayed work is created to check the
need for BKOPS. The time to start the delayed work is calculated based
on the host controller suspend timeout, in case it was set. If not, a
default time is used.
If BKOPS are required in level 1, which is non-blocking, there will be
polling of the card status to wait for the BKOPS completion and prevent
suspend that will interrupt the BKOPS.
If the card raised an exception, the need for urgent BKOPS (level 2/3)
will be checked immediately and if needed, the BKOPS will be performed
without waiting for the next idle time.

Signed-off-by: Maya Erez me...@codeaurora.org

---
This patch is based on the periodic BKOPS implementation in version 8 of 
support BKOPS feature for eMMC patch.
The patch was modified to answer the following issues:
- In order to prevent a race condition between going into suspend and starting 
BKOPS, 
  the suspend timeout of the host controller is taking into accound in 
determination of the start time 
  of the delayed work
- Since mmc_start_bkops is called from two contexts now, mmc_claim_host was 
moved to the beginning of the function
- Also, the check of doing_bkops should be protected when determing if an HPI 
is needed due to the same reason.

Changes in v3:
- Move the call to stop_bkops to block.c. 
  This allows us to remove the mmc_claim_host from inside the function and 
doesn't cause additional degradation 
  due to un-neccessary calim host operation

Changes in v2:
- Check the number of written / discarded sectors as the trigger for 
checking the BKOPS need.
- Code review fixes

---
 drivers/mmc/card/block.c |8 ++-
 drivers/mmc/card/queue.c |2 +
 drivers/mmc/core/core.c  |  178 +++---
 drivers/mmc/core/mmc.c   |   23 ++
 include/linux/mmc/card.h |   35 +
 include/linux/mmc/core.h |3 +
 6 files changed, 237 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 172a768..40b4ae3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct 
request *req)
struct mmc_blk_data *md = mq-data;
struct mmc_card *card = md-queue.card;
 
-   if (req  !mq-mqrq_prev-req)
+   if (req  !mq-mqrq_prev-req) {
/* claim host only for the first request */
mmc_claim_host(card-host);
+   if (card-ext_csd.bkops_en 
+   card-bkops_info.started_delayed_bkops) {
+   card-bkops_info.started_delayed_bkops = false;
+   mmc_stop_bkops(card);
+   }
+   }
 
ret = mmc_blk_part_switch(card, md);
if (ret) {
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index fadf52e..9d0c96a 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
 {
struct mmc_queue *mq = d;
struct request_queue *q = mq-queue;
+   struct mmc_card *card = mq-card;
 
current-flags |= PF_MEMALLOC;
 
@@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
set_current_state(TASK_RUNNING);
break;
}
+   mmc_start_delayed_bkops(card);
up(mq-thread_sem);
schedule();
down(mq-thread_sem);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 06c42cf..72ae15b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -253,9 +253,36 @@ mmc_start_request(struct mmc_host *host, struct 
mmc_request *mrq)
 }
 
 /**
+ * mmc_start_delayed_bkops() - Start a delayed work to check for
+ *  the need of non urgent BKOPS
+ *
+ * @card: MMC card to start BKOPS on
+ */
+void mmc_start_delayed_bkops(struct mmc_card *card)
+{
+   if (!card || !card-ext_csd.bkops_en || mmc_card_doing_bkops(card))
+   return;
+

Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[merez]

Hi Chris,

I managed to find a solution in which there is no need to check the number
of written / discarded sectors as a trigger for BKOPS status check.
I moved the code that checks the need to stop the BKOPS to mmc/block code,
in which there is no need for additional claim_host and remove_host
operations.
Please review this patch.

Thanks,
Maya
On Sun, November 25, 2012 3:56 am, Maya Erez wrote:
 Devices have various maintenance operations need to perform internally.
 In order to reduce latencies during time critical operations like read
 and write, it is better to execute maintenance operations in other
 times - when the host is not being serviced. Such operations are called
 Background operations (BKOPS).
 The device notifies the status of the BKOPS need by updating BKOPS_STATUS
 (EXT_CSD byte [246]).

 According to the standard a host that supports BKOPS shall check the
 status periodically and start background operations as needed, so that
 the device has enough time for its maintenance operations.

 This patch adds support for this periodic check of the BKOPS status.
 Since foreground operations are of higher priority than background
 operations the host will check the need for BKOPS when it is idle,
 and in case of an incoming request the BKOPS operation will be
 interrupted.

 When the mmcqd thread is idle, a delayed work is created to check the
 need for BKOPS. The time to start the delayed work is calculated based
 on the host controller suspend timeout, in case it was set. If not, a
 default time is used.
 If BKOPS are required in level 1, which is non-blocking, there will be
 polling of the card status to wait for the BKOPS completion and prevent
 suspend that will interrupt the BKOPS.
 If the card raised an exception, the need for urgent BKOPS (level 2/3)
 will be checked immediately and if needed, the BKOPS will be performed
 without waiting for the next idle time.

 Signed-off-by: Maya Erez me...@codeaurora.org

 ---
 This patch is based on the periodic BKOPS implementation in version 8 of
 support BKOPS feature for eMMC patch.
 The patch was modified to answer the following issues:
 - In order to prevent a race condition between going into suspend and
 starting BKOPS,
   the suspend timeout of the host controller is taking into accound in
 determination of the start time
   of the delayed work
 - Since mmc_start_bkops is called from two contexts now, mmc_claim_host
 was moved to the beginning of the function
 - Also, the check of doing_bkops should be protected when determing if an
 HPI is needed due to the same reason.

 Changes in v3:
 - Move the call to stop_bkops to block.c.
   This allows us to remove the mmc_claim_host from inside the function
 and doesn't cause additional degradation
   due to un-neccessary calim host operation

 Changes in v2:
 - Check the number of written / discarded sectors as the trigger for
 checking the BKOPS need.
 - Code review fixes

 ---
  drivers/mmc/card/block.c |8 ++-
  drivers/mmc/card/queue.c |2 +
  drivers/mmc/core/core.c  |  178
 +++---
  drivers/mmc/core/mmc.c   |   23 ++
  include/linux/mmc/card.h |   35 +
  include/linux/mmc/core.h |3 +
  6 files changed, 237 insertions(+), 12 deletions(-)

 diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
 index 172a768..40b4ae3 100644
 --- a/drivers/mmc/card/block.c
 +++ b/drivers/mmc/card/block.c
 @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
 struct request *req)
   struct mmc_blk_data *md = mq-data;
   struct mmc_card *card = md-queue.card;

 - if (req  !mq-mqrq_prev-req)
 + if (req  !mq-mqrq_prev-req) {
   /* claim host only for the first request */
   mmc_claim_host(card-host);
 + if (card-ext_csd.bkops_en 
 + card-bkops_info.started_delayed_bkops) {
 + card-bkops_info.started_delayed_bkops = false;
 + mmc_stop_bkops(card);
 + }
 + }

   ret = mmc_blk_part_switch(card, md);
   if (ret) {
 diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
 index fadf52e..9d0c96a 100644
 --- a/drivers/mmc/card/queue.c
 +++ b/drivers/mmc/card/queue.c
 @@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d)
  {
   struct mmc_queue *mq = d;
   struct request_queue *q = mq-queue;
 + struct mmc_card *card = mq-card;

   current-flags |= PF_MEMALLOC;

 @@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d)
   set_current_state(TASK_RUNNING);
   break;
   }
 + mmc_start_delayed_bkops(card);
   up(mq-thread_sem);
   schedule();
   down(mq-thread_sem);
 diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
 index 06c42cf..72ae15b 100644
 --- a/drivers/mmc/core/core.c