Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399

[Ulf Hansson]

On 1 September 2016 at 05:23, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
> å 2016/9/1 10:29, Ziyuan Xu åé:
>>
>> Hi,
>>
>>
>> On 2016å09æ01æ 01:42, Doug Anderson wrote:
>>>
>>> Hi,
>>>
>>> On Sun, Aug 28, 2016 at 8:25 PM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>>> wrote:
>>>>
>>>> On 2016/8/29 10:50, Elaine Zhang wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 08/27/2016 11:05 PM, Shawn Lin wrote:
>>>>>>
>>>>>> On 2016/8/27 21:41, Ziyuan Xu wrote:
>>>>>>>
>>>>>>> Control power domain for eMMC via genpd to reduce power consumption.
>>>>>>>
>>>>>>> Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
>>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@xxxxxxxxxxxxxx>
>>>>>>>
>>>>>> It looks nice to me. But this should be merged after applying that[0]
>>>>>> as your patch will break bind/unbind test for sdhci-of-arasan on
>>>>>> rk3399
>>>>>> without it[0]. Moreover, Elaine should make sure that upstreamed
>>>>>> rockchip power domain stuff would not off pd for emmc, *otherwise*, I
>>>>>> should update my patch to make sure we update clkmul every time when
>>>>>> doing suspend 2 resume..
>>>>>>
>>>>>>
>>>>> Forgot to say:
>>>>> If use pd, Although there is no call to power odd the pd_emmc,
>>>>> it will be power off when the system doing suspend 2 resume.
>>>>> (Because the system call
>>>>> __device_suspend_noirq->pm_genpd_suspend_noirq->rockchip_pd_power_off)
>>>>
>>>>
>>>> Thanks for explaining this. I checked the code a bit and actually I
>>>> don't need to updata clkmul since it was recorded, although it is still
>>>> reset to 0x10 reading from syscon. So for that, we can now pick it
>>>> up without waiting for my sdhci-of-arasan's update.
>>>>
>>>> Reviewed-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>>>
>>> This is fine to pick up _only_ if you don't care about suspend/resume.
>>> If you care about suspend/resume then someone needs to first write a
>>> patch that will re-init all "corecfg" values after power is turned on.
>>
>>
>> Do you mean corecfg_clockmultiplier and corecfg_baseclkfreq, if yes, we
>> don't need to strore/re-init it after resume.
>> corecfg_clockmultiplier is only used to fetch host->clk_mul, and
>> host->clk_mul has been a fixed value at run-time, unless driver unbind.
>> The same as corecfg_clockmultiplier, corecfg_baseclkfreq is used to
>> check the xin_clk at probe time, we don't reference it at run-time.
>
>
> correct. That is why we don't need to update it even we power off pd.
>
>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>> works fine.
>>
>>>
>>> Technically I think this should probably use "pm runtime" and not
>>> normal suspend/resume hooks.  Any time we end up pm runtime suspended
>>> then I think our power will go off (because of genpd?) and we need to
>>> restore values.
>>
>>
>> I understand your consideration. BUT genpd is in charge of on/off pd if
>> the corresponding device node has "power-domains" property. RPM is
>> unnecessary for this situation, we will not use autosuspend, right?
>
>
> That is irrelevant to rpm as what we need is just to control genpd here.
> If we need to support rmp, we need some extra patches to do it,
> including genpd off, clk-disable, etc...
>
>>
>> @shawn, what's your opinion?
>>
>>>
>>> I'm not sure if this should be done in a generic way where we try to
>>> save and restore all values in the "sdhci_arasan_soc_ctl_map" or if we
>>> should try to be smarter...
>
>
> I am prone to keep it as-is, unless we see a stronge requirement for
> coming users who argue that they have some extra corecfg_* need to
> recovery due to whatever reason.:)
>
> If we see something like this:
>
> if(of_device_is_compatible(A))
>         update X
> else if (of_device_is_compatible(B))
>         update Y
> .....
>
> Then we could consider trying to save & restrore all the values.
>
>

I was reading the discussion regarding this change and browsing the DT
documentation around this... Can you guys explain what really goes on
here, please.

To me, it seems like you are managing one device's resources in one
separate genpd. One genpd per device. Is that correct?

Perhaps each device actually has its own PM domain and thus it makes
sense to assign one genpd per device?

In other case, I would not recommend this method, mainly because my
experience tells me that it becomes hard to keep drivers portable from
a PM point of view, but also that it becomes tricky to optimize for
power savings.

No matter what, deploying runtime PM for the device's driver/bus is
important, as genpd relies on device's runtime PM status to be able to
take correct decisions.

My point is, start by deploying runtime PM in the driver, then deploy
system PM. Hopefully you can use the runtime PM centric approach, as
described here[1] and then get system PM support for "free".

Sorry for jumping into this discussions like this, I hope I don't add
too much of confusions.

Kind regards
Uffe

[1]
http://www.linaro.org/blog/core-dump/dont-waste-power-when-idle/