Re: [EXT] Re: [PATCH v3] mmc: sdhci-xenon: Add Xenon SDHCI specific system-level PM support

[Zhoujie Wu <zjwu@marvell.com>]

Hi Ulf,

On 08/07/2017 02:23 AM, Ulf Hansson wrote:
> [...]
>
>>> I am not sure I get the second part here. The clock to shd is enabled
>>> via a call to clk_prepare_enable(). Unless you explicitly call
>>> clk_disable_unprepare() for it, no? How can any outer logic know when
>>> it can be gated?
>>
>> This is my understanding. Hope it can make you clear.
>> The clock tree is like below.
>> SOC --> [ SDH_CLK_GEN --> SDH_CONTROLLER ] --> SD/EMMC CARD
>>
>> There is one clock generator inside sdh slot IP, SOC provides the clock to
>> the sdh slot IP. This clock is enabled/disabled by SW when calling
>> clk_prepare_enable/clk_disable_unprepare.
>> The auto clock gating is not any outer logic, it is inside sdh slot IP, when
>> sdh controller has no activity, the IP will gate the clock from sdh_clk_gen
>> to sdh_controller.  sdh_clk_gen logic itself still has clock from SOC.
>> With or without runtime pm, the only difference is if sdh_clk_gen has clock
>> or not. So the power benefit is limited.
> Thanks for clarifying!
>
>>>> With SW runtime pm mechanism, compares with HW auto clock gating, the
>>>> only
>>>> difference is SW cut the source of sdh clock tree, external clock gating
>>>> vs
>>>> internal clock gating, there will be some benefits, but limited.
>>> Right.
>>>
>>>> Previously we enabled the runtime pm mechanism in our mobile products,
>>>> which
>>>> were using the same IP(some old version, including 3 sdh slots) with auto
>>>> clock gating feature(the driver is sdhci-pxav3.c).  The saving of power
>>>> was
>>>> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No
>>>> more
>>>> than 1mA/1sdh slot.
>>> 1 mA/sdh slot is a great reason to deploy runtime PM support. For a
>>> battery driven device that would be a significant improvement.
>>>
>>> Back in the days when I worked at ST-Ericssion, we were chasing uA
>>> when optimizing for power-save. :-)
>>
>> Definitely for mobile products, but now I didn't see urgent requirement for
>> our networking products.
> I see.
>
> I think what puzzles me is that you do care about saving power in
> system sleep, but not during runtime.
>
>>>
>>>> I read sdhci-of-at91 driver and your recommended patch, I got your point
>>>> is
>>>> using a light way for system sleep based on runtime pm feature. From SW
>>>> perspective, kill two birds with one stone, it is good.
>>> Right.
>>>
>>>> But considering about the benefits, it is not that urgent to take runtime
>>>> pm
>>>> feature as a must, it is a better to have feature. System standby is a
>>>> must
>>>> feature, without this patch, the system can't work well after resume.
>>>> Do you think it is reasonable to add complete standby support at first,
>>>> then
>>>> take runtime pm as a next step?
>>> You can do that, but why? And will then the "next step" ever happen?
>>>
>>> Do you really want to spend efforts in getting something working for
>>> system suspend only, while you instead easily could deploy both
>>> runtime PM and system PM support at the same time?
>>
>> As Ziji said in another mail, it takes time for next step. The runtime pm
>> need to be verified completely on all supported boards.
>> I understand from SW perspective, we'd better have both. But I need input
>> from internal customers to see if they only request system sleep or they
>> want both, and what's their priority.
> Okay. As I just responded in the other email, I rest my case. :-)
>
> [...]
>
> However, I need an ack from Adrian before I can apply this.

Thanks for your feedback. System level standby is mandatory requirement 
from our customer. It's nice you can merge it at first.
For runtime PM, it's nice to have. Actually in the past two weeks I've 
already implemented and verified the basic function on our platform.
But it took time for them for the full verification, I will submit 
runtime pm patch after hear feedback from them.
>
> Kind regards
> Uffe