Re: [PATCH] omap: hsmmc funtionality breaks when CONFIG_PM not define

[Adrian Hunter <adrian.hunter@nokia.com>]

kishore kadiyala wrote:
> Hi Adrian,
> 
> Is there any use case or a valid scenario  whether to use
> omap_hsmmc_ps_ops or omap_hsmmc_ops ?
> I meant using power_saving option and without power_saving option

There are two disadvantages of power_saving:
	1. It introduces latency to wake-up or re-initialize.
Waking up a sleeping card can take 10-20ms (or more).  Re-initializing
a card that was powered off entirely can take 300-1000ms.
	2. The current implementation powers-up the card even
if it just wants to suspend and power it off again.

> 
> Ideally I feel having omap_hsmmc_ps_ops as default with or without
> CONFIG_PM would be better as it internally has a state handling.

Sure, you could rename power_saving to no_power_saving, invert the
logic and drop it from the board files that had power_saving = true.
If that is what you want, make a patch and see if anyone comments.

> 
> Regards,
> Kishore
> 
> On Tue, Jul 20, 2010 at 2:43 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
>> Ghorai, Sukumar wrote:
>>>> -----Original Message-----
>>>> From: Adrian Hunter [mailto:adrian.hunter@nokia.com]
>>>> Sent: Tuesday, July 20, 2010 2:10 PM
>>>> To: Ghorai, Sukumar
>>>> Cc: linux-mmc@vger.kernel.org; linux-omap@vger.kernel.org; Shilimkar,
>>>> Santosh; Chikkature Rajashekar, Madhusudhan; Andrew Morton
>>>> Subject: Re: [PATCH] omap: hsmmc funtionality breaks when CONFIG_PM not
>>>> define
>>>>
>>>> Ghorai, Sukumar wrote:
>>>>> Adrian,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Adrian Hunter [mailto:adrian.hunter@nokia.com]
>>>>>> Sent: Tuesday, July 20, 2010 1:40 PM
>>>>>> To: Ghorai, Sukumar
>>>>>> Cc: linux-mmc@vger.kernel.org; linux-omap@vger.kernel.org; Shilimkar,
>>>>>> Santosh; Chikkature Rajashekar, Madhusudhan; Andrew Morton
>>>>>> Subject: Re: [PATCH] omap: hsmmc funtionality breaks when CONFIG_PM not
>>>>>> define
>>>>>>
>>>>>> Ghorai, Sukumar wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Adrian Hunter [mailto:adrian.hunter@nokia.com]
>>>>>>>> Sent: Tuesday, July 20, 2010 12:39 PM
>>>>>>>> To: Ghorai, Sukumar
>>>>>>>> Cc: linux-mmc@vger.kernel.org; linux-omap@vger.kernel.org; Shilimkar,
>>>>>>>> Santosh; Chikkature Rajashekar, Madhusudhan; Andrew Morton
>>>>>>>> Subject: Re: [PATCH] omap: hsmmc funtionality breaks when CONFIG_PM
>>>> not
>>>>>>>> define
>>>>>>>>
>>>>>>>> Sukumar Ghorai wrote:
>>>>>>>>>    Issue if power_saving option passed from board file and
>>>> CONFIG_PM
>>>>>>>> not define.
>>>>>>>>>    This is because hosts refer to wrong operation table and that
>>>> try
>>>>>> to
>>>>>>>> power save.
>>>>>>>>
>>>>>>>> power_saving is not related to power management.  It should work with
>>>>>> or
>>>>>>>> without
>>>>>>>> CONFIG_PM.  What problem are you seeing?
>>>>>>> [Ghorai] when CONFIG_PM is not defined but power_saving flag is TRUE,
>>>>>> then host operation table point to table which handles in state machine
>>>>>> as: ENABLE -> CARDSLEEP -> REGSLEEP -> DISABLED for power/clock cut.
>>>> And
>>>>>> do the reverse for the enable clock/power.
>>>>>>> And power saving is not required and wont work when CONFIG_PM is not
>>>>>> enabled.
>>>>>>
>>>>>> How is that a problem?  It would be useful to know what you need?
>>>>> [Ghorai] do men when CONFIG_PM is not enabled still we should do power
>>>> saving in mmc?
>>>>
>>>> Why not?
>>>>
>>>> Because issue is iclk/fclk is quite depended on PRCM framework when we
>>>> are
>>>> using omap_hsmmc_ps_ops operation table.
>>>>
>>>> iclk is left alone.
>>>>
>>>> fclk is manipulated with or without power_saving.
>>>>
>>>> I still do not understand your problem sorry :-(
>>> [Ghorai] thanks and need input again,  we have two tables -
>>> 1. static const struct mmc_host_ops omap_hsmmc_ops = {
>>> }
>>> --> This is without CONFIG_PM, assumed.
>> No it is independent of CONFIG_PM
>>
>> With CONFIG_PM
>>        - the driver will restore registers if the host controller
>>        has been powered off / on by PRCM
>>        - suspend / resume can be used
>>
>>> 2. static const struct mmc_host_ops omap_hsmmc_ps_ops = {
>>> }
>>> -> this with CONFIG_PM, assumed.
>> No it is independent of CONFIG_PM
>>
>> Without CONFIG_PM
>>        - card will be put to sleep after 1 second
>>        - card will be powered off after another 8 seconds
>>
>>> a. And you feel we should remove #1
>> Nothing should need to be removed.
>>
>>> b. use omap_hsmmc_ps_ops default.
>> With power_saving it is yes.
>>
>>> c. And it should work in all case?
>> It should work in all cases.
>>
>>>>>>> So if CONFIG_PM is not enable, then it should do simple clock
>>>>>> disable/enable, and not via the power state machine.
>>>>>>
>>>>>> If that is what you want, simply change your board file:
>>>>>>
>>>>>> #if CONFIG_PM
>>>>>>        .power_saving = true,
>>>>>> #else
>>>>>>        .power_saving = false,
>>>>>> #end
>>>>> [Ghorai] the fix I send is to guard in MMC/SD host driver to avoid
>>>> mistake in board file(s). And this file is used for multiple omap3, omap4
>>>> boards.
>>>>>>>>> Signed-off-by: Sukumar Ghorai <s-ghorai@ti.com>
>>>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>>>>> CC: Madhusudhan Chikkature <madhu.cr@ti.com>
>>>>>>>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>>>>>>>> ---
>>>>>>>>>  Tested on omap3, omap4430 ES2.0
>>>>>>>>>
>>>>>>>>>  drivers/mmc/host/omap_hsmmc.c |    2 ++
>>>>>>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mmc/host/omap_hsmmc.c
>>>>>>>> b/drivers/mmc/host/omap_hsmmc.c
>>>>>>>>> index b032828..f84eed0 100644
>>>>>>>>> --- a/drivers/mmc/host/omap_hsmmc.c
>>>>>>>>> +++ b/drivers/mmc/host/omap_hsmmc.c
>>>>>>>>> @@ -2015,9 +2015,11 @@ static int __init omap_hsmmc_probe(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>>        platform_set_drvdata(pdev, host);
>>>>>>>>>        INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
>>>>>>>>>
>>>>>>>>> +#ifdef CONFIG_PM
>>>>>>>>>        if (mmc_slot(host).power_saving)
>>>>>>>>>                mmc->ops        = &omap_hsmmc_ps_ops;
>>>>>>>>>        else
>>>>>>>>> +#endif
>>>>>>>>>                mmc->ops        = &omap_hsmmc_ops;
>>>>>>>>>
>>>>>>>>>        /*
>>>>>>>>> --
>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc"
>>>>>> in
>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>>
>>>>>>> Regards,
>>>>>>> Ghorai
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc"
>>>> in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>