[Query] mmc: core "MMC_PM_KEEP_POWER"

[Query] mmc: core "MMC_PM_KEEP_POWER"

[Ulf Hansson]

Hi,

In mmc_resume_host we clear the MMC_PM_KEEP_POWER flag from the pm_flags 
bitfield. This is done by a patch from Eliad Peller a while ago, "mmc: 
clear MMC_PM_KEEP_POWER flag on resume"

I would like to understand if there are any reason to why we want to 
clear this flag after we done a resume. I think it will add complexity 
to an sdio function driver since it must update this flag for after each 
suspend/resume sequence. Should it not just be enough to do this during 
the initialization of the sdio func driver.


Br
Ulf Hansson

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Eliad Peller]

hi Ulf,

On Tue, Nov 22, 2011 at 3:29 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> In mmc_resume_host we clear the MMC_PM_KEEP_POWER flag from the pm_flags
> bitfield. This is done by a patch from Eliad Peller a while ago, "mmc: clear
> MMC_PM_KEEP_POWER flag on resume"
>
> I would like to understand if there are any reason to why we want to clear
> this flag after we done a resume. I think it will add complexity to an sdio
> function driver since it must update this flag for after each suspend/resume
> sequence. Should it not just be enough to do this during the initialization
> of the sdio func driver.
>
please check out the sdio_set_host_pm_flags() documentation:

/**
 *	sdio_set_host_pm_flags - set wanted host power management capabilities
 *	@func: SDIO function attached to host
 *
 *	Set a capability bitmask corresponding to wanted host controller
 *	power management features for the upcoming suspend state.
 *	This must be called, if needed, each time the suspend method of
 *	the function driver is called, and must contain only bits that
 *	were returned by sdio_get_host_pm_caps().
 *	The host doesn't need to be claimed, nor the function active,
 *	for this information to be set.
 */
int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)

as we have to set the flags on each suspend, we have to clear them on resume.

regards,
Eliad.

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Nicolas Pitre]

On Tue, 22 Nov 2011, Eliad Peller wrote:

> hi Ulf,
> 
> On Tue, Nov 22, 2011 at 3:29 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
> > In mmc_resume_host we clear the MMC_PM_KEEP_POWER flag from the pm_flags
> > bitfield. This is done by a patch from Eliad Peller a while ago, "mmc: clear
> > MMC_PM_KEEP_POWER flag on resume"
> >
> > I would like to understand if there are any reason to why we want to clear
> > this flag after we done a resume. I think it will add complexity to an sdio
> > function driver since it must update this flag for after each suspend/resume
> > sequence. Should it not just be enough to do this during the initialization
> > of the sdio func driver.
> >
> please check out the sdio_set_host_pm_flags() documentation:
> 
> /**
>  *	sdio_set_host_pm_flags - set wanted host power management capabilities
>  *	@func: SDIO function attached to host
>  *
>  *	Set a capability bitmask corresponding to wanted host controller
>  *	power management features for the upcoming suspend state.
>  *	This must be called, if needed, each time the suspend method of
>  *	the function driver is called, and must contain only bits that
>  *	were returned by sdio_get_host_pm_caps().
>  *	The host doesn't need to be claimed, nor the function active,
>  *	for this information to be set.
>  */
> int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)
> 
> as we have to set the flags on each suspend, we have to clear them on resume.

When I introduced this interface, the flags were cleared by the core 
code upon resume.


Nicolas

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Chris Ball]

Hi,

On Tue, Nov 22 2011, Ulf Hansson wrote:
> Hi,
>
> In mmc_resume_host we clear the MMC_PM_KEEP_POWER flag from the
> pm_flags bitfield. This is done by a patch from Eliad Peller a while
> ago, "mmc: clear MMC_PM_KEEP_POWER flag on resume"
>
> I would like to understand if there are any reason to why we want to
> clear this flag after we done a resume. I think it will add complexity
> to an sdio function driver since it must update this flag for after
> each suspend/resume sequence. Should it not just be enough to do this
> during the initialization of the sdio func driver.

The sdio function driver should just call sdio_set_host_pm_flags() in
its suspend function, if it wants to stay awake at the next suspend.
It's just one line of code, so I don't think it's overly complex.

We clear the flag because that's how the API is defined --
sdio_set_host_pm_flags() says "This must be called, if needed, each
time the suspend method of the function driver is called".

There's a separate question of "Why is the API this way, and should
we change it?".  I think the API ended up this way because it was
created for a wifi device where a suspend would usually happen while
you're unassociated, so you don't need to preserve power (because
nothing interesting can happen to wake you up), but will sometimes
happen while you're associated, in which case you do.  It sounds like
you're considering a device that *always* wants to stay awake, which
is different to the rest of the users of this API.

But it's just the difference between (your proposal) having
sdio_set_host_pm_flags() be called during init, and (the API) having
sdio_set_host_pm_flags() be called during suspend.  It's the same
line of code either way, so I don't see why it's annoying this way.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Ulf Hansson]

Hi Chris,

Thanks for the quick reply! I understand the idea better now. Just a 
minor follow-up question..

The power_restore function for sdio also uses this flag, which is a 
little bit strange I think. Especially since the mmc_power_off|up is 
always called no matter of the value of this flag.

Br
Ulf Hansson

Chris Ball wrote:
> Hi,
> 
> On Tue, Nov 22 2011, Ulf Hansson wrote:
>> Hi,
>>
>> In mmc_resume_host we clear the MMC_PM_KEEP_POWER flag from the
>> pm_flags bitfield. This is done by a patch from Eliad Peller a while
>> ago, "mmc: clear MMC_PM_KEEP_POWER flag on resume"
>>
>> I would like to understand if there are any reason to why we want to
>> clear this flag after we done a resume. I think it will add complexity
>> to an sdio function driver since it must update this flag for after
>> each suspend/resume sequence. Should it not just be enough to do this
>> during the initialization of the sdio func driver.
> 
> The sdio function driver should just call sdio_set_host_pm_flags() in
> its suspend function, if it wants to stay awake at the next suspend.
> It's just one line of code, so I don't think it's overly complex.
> 
> We clear the flag because that's how the API is defined --
> sdio_set_host_pm_flags() says "This must be called, if needed, each
> time the suspend method of the function driver is called".
> 
> There's a separate question of "Why is the API this way, and should
> we change it?".  I think the API ended up this way because it was
> created for a wifi device where a suspend would usually happen while
> you're unassociated, so you don't need to preserve power (because
> nothing interesting can happen to wake you up), but will sometimes
> happen while you're associated, in which case you do.  It sounds like
> you're considering a device that *always* wants to stay awake, which
> is different to the rest of the users of this API.
> 
> But it's just the difference between (your proposal) having
> sdio_set_host_pm_flags() be called during init, and (the API) having
> sdio_set_host_pm_flags() be called during suspend.  It's the same
> line of code either way, so I don't see why it's annoying this way.
> 
> Thanks,
> 
> - Chris.

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Chris Ball]

Hi Ulf,

On Tue, Nov 22 2011, Ulf Hansson wrote:
> Thanks for the quick reply! I understand the idea better now. Just a
> minor follow-up question..
>
> The power_restore function for sdio also uses this flag, which is a
> little bit strange I think.

Yeah, it's being used to detect whether we're in the middle of a powered
resume, and therefore shouldn't be trying to reinit the card.  Then the
value of MMC_PM_KEEP_POWER gets reset once the resume's finished.

> Especially since the mmc_power_off|up is always called no matter of
> the value of this flag.

Hm, I don't think that's true.

mmc_suspend_host() checks !mmc_card_keep_power(host) before mmc_power_off()
mmc_resume_host() checks !mmc_card_keep_power(host) before mmc_power_up()

static inline int mmc_card_keep_power(struct mmc_host *host)
{
        return host->pm_flags & MMC_PM_KEEP_POWER;
}

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [Query] mmc: core "MMC_PM_KEEP_POWER"

[Ulf Hansson]

Chris Ball wrote:
> Hi Ulf,
> 
> On Tue, Nov 22 2011, Ulf Hansson wrote:
>> Thanks for the quick reply! I understand the idea better now. Just a
>> minor follow-up question..
>>
>> The power_restore function for sdio also uses this flag, which is a
>> little bit strange I think.
> 
> Yeah, it's being used to detect whether we're in the middle of a powered
> resume, and therefore shouldn't be trying to reinit the card.  Then the
> value of MMC_PM_KEEP_POWER gets reset once the resume's finished.
> 
>> Especially since the mmc_power_off|up is always called no matter of
>> the value of this flag.
> 
> Hm, I don't think that's true.

I were thinking of mmc_power_save|restore_host, those functions is 
always doing power_off|on.


> 
> mmc_suspend_host() checks !mmc_card_keep_power(host) before mmc_power_off()
> mmc_resume_host() checks !mmc_card_keep_power(host) before mmc_power_up()
> 
> static inline int mmc_card_keep_power(struct mmc_host *host)
> {
>         return host->pm_flags & MMC_PM_KEEP_POWER;
> }
> 
> Thanks,
> 
> - Chris.

BR
Uffe