* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 2:29 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-01 2:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 2016?09?01? 01:42, Doug Anderson wrote:
> Hi,
>
> On Sun, Aug 28, 2016 at 8:25 PM, Shawn Lin <shawn.lin@rock-chips.com> 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@rock-chips.com>
>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>
>>>> 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@rock-chips.com>
> 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.
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?
@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...
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 2:29 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-01 2:29 UTC (permalink / raw)
To: Doug Anderson, Shawn Lin
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Masahiro Yamada, Rob Herring, David Wu, Caesar Wang, Jianqun 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>> Signed-off-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>
>>>> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 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.
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?
@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...
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 3:23 ` Shawn Lin
0 siblings, 0 replies; 59+ messages in thread
From: Shawn Lin @ 2016-09-01 3:23 UTC (permalink / raw)
To: Ziyuan Xu, Doug Anderson
Cc: shawn.lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
在 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@rock-chips.com>
>> 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@rock-chips.com>
>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>>
>>>>> 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@rock-chips.com>
>> 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.
>>
>>
>> -Doug
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 3:23 ` Shawn Lin
0 siblings, 0 replies; 59+ messages in thread
From: Shawn Lin @ 2016-09-01 3:23 UTC (permalink / raw)
To: linux-arm-kernel
? 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@rock-chips.com>
>> 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@rock-chips.com>
>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>>
>>>>> 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@rock-chips.com>
>> 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.
>>
>>
>> -Doug
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
>
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 3:23 ` Shawn Lin
0 siblings, 0 replies; 59+ messages in thread
From: Shawn Lin @ 2016-09-01 3:23 UTC (permalink / raw)
To: Ziyuan Xu, Doug Anderson
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Masahiro Yamada, Rob Herring, David Wu, Caesar Wang, Jianqun Xu
在 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>>>
>>>>> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> 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.
>>
>>
>> -Doug
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
>
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 13:45 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-01 13:45 UTC (permalink / raw)
To: Shawn Lin, Ziyuan Xu, Heiko Stübner
Cc: Doug Anderson, Mark Rutland, devicetree, Xing Zheng,
open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
On 1 September 2016 at 05:23, Shawn Lin <shawn.lin@rock-chips.com> 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@rock-chips.com>
>>> 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@rock-chips.com>
>>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>>>
>>>>>> 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@rock-chips.com>
>>>
>>> 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/
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 13:45 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-01 13:45 UTC (permalink / raw)
To: linux-arm-kernel
On 1 September 2016 at 05:23, Shawn Lin <shawn.lin@rock-chips.com> 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@rock-chips.com>
>>> 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@rock-chips.com>
>>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>>>
>>>>>> 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@rock-chips.com>
>>>
>>> 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/
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 13:45 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-01 13:45 UTC (permalink / raw)
To: Shawn Lin, Ziyuan Xu, Heiko Stübner
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Xing Zheng, Masahiro Yamada, Frank Wang, Catalin Marinas,
Elaine Zhang, Will Deacon, Doug Anderson,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
open list:ARM/Rockchip SoC...,
Rob Herring, David Wu, Shunqian Zheng, Jianqun Xu,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Caesar Wang
On 1 September 2016 at 05:23, Shawn Lin <shawn.lin@rock-chips.com> 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@rock-chips.com>
>>> 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@rock-chips.com>
>>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>>>
>>>>>> 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@rock-chips.com>
>>>
>>> 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/
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-01 13:45 ` Ulf Hansson
(?)
@ 2016-09-01 21:50 ` Doug Anderson
-1 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:50 UTC (permalink / raw)
To: Ulf Hansson
Cc: Shawn Lin, Ziyuan Xu, Heiko Stübner, Mark Rutland,
devicetree, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
Hi,
On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 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?
I'm not as familiar with genpd as I should be, so hopefully this makes sense.
...in hardware there is a "pd_emmc" that is the power domain for just
eMMC. That will be referenced hooked up via device tree, like:
power-domains = <&power RK3399_PD_EMMC>;
I believe that means that power will automatically be removed whenever
the device is runtime suspended or suspended.
If w're not supporting "autosuspend" and nobody is tweaking anything
manually, then it's possible (I think) that runtime suspend happens at
exactly the same time as suspend. ...but my point was that it was
cleaner to actually do it any restoring in the "runtime resume" hooks
to match what genpd does. This matches what you say: use runtime PM.
...but it also sounds like it might not be terribly important to
restore these values since they're a bit silly to begin with. If
that's true then I guess we don't need to do anything special here.
Did that all make sense (it's entirely possible it didn't since
somehow my brain still hasn't absorbed all runtime PM and genpd
concepts)
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 21:50 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 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?
I'm not as familiar with genpd as I should be, so hopefully this makes sense.
...in hardware there is a "pd_emmc" that is the power domain for just
eMMC. That will be referenced hooked up via device tree, like:
power-domains = <&power RK3399_PD_EMMC>;
I believe that means that power will automatically be removed whenever
the device is runtime suspended or suspended.
If w're not supporting "autosuspend" and nobody is tweaking anything
manually, then it's possible (I think) that runtime suspend happens at
exactly the same time as suspend. ...but my point was that it was
cleaner to actually do it any restoring in the "runtime resume" hooks
to match what genpd does. This matches what you say: use runtime PM.
...but it also sounds like it might not be terribly important to
restore these values since they're a bit silly to begin with. If
that's true then I guess we don't need to do anything special here.
Did that all make sense (it's entirely possible it didn't since
somehow my brain still hasn't absorbed all runtime PM and genpd
concepts)
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 21:50 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:50 UTC (permalink / raw)
To: Ulf Hansson
Cc: Shawn Lin, Ziyuan Xu, Heiko Stübner, Mark Rutland,
devicetree, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang
Hi,
On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 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?
I'm not as familiar with genpd as I should be, so hopefully this makes sense.
...in hardware there is a "pd_emmc" that is the power domain for just
eMMC. That will be referenced hooked up via device tree, like:
power-domains = <&power RK3399_PD_EMMC>;
I believe that means that power will automatically be removed whenever
the device is runtime suspended or suspended.
If w're not supporting "autosuspend" and nobody is tweaking anything
manually, then it's possible (I think) that runtime suspend happens at
exactly the same time as suspend. ...but my point was that it was
cleaner to actually do it any restoring in the "runtime resume" hooks
to match what genpd does. This matches what you say: use runtime PM.
...but it also sounds like it might not be terribly important to
restore these values since they're a bit silly to begin with. If
that's true then I guess we don't need to do anything special here.
Did that all make sense (it's entirely possible it didn't since
somehow my brain still hasn't absorbed all runtime PM and genpd
concepts)
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-01 21:50 ` Doug Anderson
(?)
@ 2016-09-02 10:24 ` Ulf Hansson
-1 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-02 10:24 UTC (permalink / raw)
To: Doug Anderson
Cc: Shawn Lin, Ziyuan Xu, Heiko Stübner, Mark Rutland,
devicetree, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
On 1 September 2016 at 23:50, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> 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?
>
> I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>
> ...in hardware there is a "pd_emmc" that is the power domain for just
> eMMC. That will be referenced hooked up via device tree, like:
>
> power-domains = <&power RK3399_PD_EMMC>;
Yes, I noticed that and this is what puzzles me a bit.
>
> I believe that means that power will automatically be removed whenever
> the device is runtime suspended or suspended.
Well, it depends if the genpd has a subdomain or other devices in it
being runtime resumed.
The genpd will not power off unless all devices within it are runtime
enabled+suspended and that its subdomains are also powered off.
So, in case you only have one device and no subdomains, then your
statement is correct.
>
> If w're not supporting "autosuspend" and nobody is tweaking anything
I guess you mean runtime PM autosuspend? Then why don't you support this?
Wouldn't that allow you to avoid wasting power in runtime when the
device is idle?
> manually, then it's possible (I think) that runtime suspend happens at
> exactly the same time as suspend. ...but my point was that it was
I am not sure I follow you here. You must not rely on that the device
always becomes runtime suspended during system suspend, as there are
no guarantees for this.
Instead that is something you need to take care of in the
subsystem/driver for the device, of course.
> cleaner to actually do it any restoring in the "runtime resume" hooks
> to match what genpd does. This matches what you say: use runtime PM.
Yes!
Using genpd without deploying runtime PM for the devices doesn't make
much sense, at least to me.
>
> ...but it also sounds like it might not be terribly important to
> restore these values since they're a bit silly to begin with. If
> that's true then I guess we don't need to do anything special here.
>
>
> Did that all make sense (it's entirely possible it didn't since
> somehow my brain still hasn't absorbed all runtime PM and genpd
> concepts)
No worries. I understand this might be a bit tricky, that's why I also
tries to help review related changes.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 10:24 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-02 10:24 UTC (permalink / raw)
To: linux-arm-kernel
On 1 September 2016 at 23:50, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> 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?
>
> I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>
> ...in hardware there is a "pd_emmc" that is the power domain for just
> eMMC. That will be referenced hooked up via device tree, like:
>
> power-domains = <&power RK3399_PD_EMMC>;
Yes, I noticed that and this is what puzzles me a bit.
>
> I believe that means that power will automatically be removed whenever
> the device is runtime suspended or suspended.
Well, it depends if the genpd has a subdomain or other devices in it
being runtime resumed.
The genpd will not power off unless all devices within it are runtime
enabled+suspended and that its subdomains are also powered off.
So, in case you only have one device and no subdomains, then your
statement is correct.
>
> If w're not supporting "autosuspend" and nobody is tweaking anything
I guess you mean runtime PM autosuspend? Then why don't you support this?
Wouldn't that allow you to avoid wasting power in runtime when the
device is idle?
> manually, then it's possible (I think) that runtime suspend happens at
> exactly the same time as suspend. ...but my point was that it was
I am not sure I follow you here. You must not rely on that the device
always becomes runtime suspended during system suspend, as there are
no guarantees for this.
Instead that is something you need to take care of in the
subsystem/driver for the device, of course.
> cleaner to actually do it any restoring in the "runtime resume" hooks
> to match what genpd does. This matches what you say: use runtime PM.
Yes!
Using genpd without deploying runtime PM for the devices doesn't make
much sense, at least to me.
>
> ...but it also sounds like it might not be terribly important to
> restore these values since they're a bit silly to begin with. If
> that's true then I guess we don't need to do anything special here.
>
>
> Did that all make sense (it's entirely possible it didn't since
> somehow my brain still hasn't absorbed all runtime PM and genpd
> concepts)
No worries. I understand this might be a bit tricky, that's why I also
tries to help review related changes.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 10:24 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-02 10:24 UTC (permalink / raw)
To: Doug Anderson
Cc: Shawn Lin, Ziyuan Xu, Heiko Stübner, Mark Rutland,
devicetree, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang
On 1 September 2016 at 23:50, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> 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?
>
> I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>
> ...in hardware there is a "pd_emmc" that is the power domain for just
> eMMC. That will be referenced hooked up via device tree, like:
>
> power-domains = <&power RK3399_PD_EMMC>;
Yes, I noticed that and this is what puzzles me a bit.
>
> I believe that means that power will automatically be removed whenever
> the device is runtime suspended or suspended.
Well, it depends if the genpd has a subdomain or other devices in it
being runtime resumed.
The genpd will not power off unless all devices within it are runtime
enabled+suspended and that its subdomains are also powered off.
So, in case you only have one device and no subdomains, then your
statement is correct.
>
> If w're not supporting "autosuspend" and nobody is tweaking anything
I guess you mean runtime PM autosuspend? Then why don't you support this?
Wouldn't that allow you to avoid wasting power in runtime when the
device is idle?
> manually, then it's possible (I think) that runtime suspend happens at
> exactly the same time as suspend. ...but my point was that it was
I am not sure I follow you here. You must not rely on that the device
always becomes runtime suspended during system suspend, as there are
no guarantees for this.
Instead that is something you need to take care of in the
subsystem/driver for the device, of course.
> cleaner to actually do it any restoring in the "runtime resume" hooks
> to match what genpd does. This matches what you say: use runtime PM.
Yes!
Using genpd without deploying runtime PM for the devices doesn't make
much sense, at least to me.
>
> ...but it also sounds like it might not be terribly important to
> restore these values since they're a bit silly to begin with. If
> that's true then I guess we don't need to do anything special here.
>
>
> Did that all make sense (it's entirely possible it didn't since
> somehow my brain still hasn't absorbed all runtime PM and genpd
> concepts)
No worries. I understand this might be a bit tricky, that's why I also
tries to help review related changes.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-02 10:24 ` Ulf Hansson
(?)
@ 2016-09-02 14:23 ` Ziyuan Xu
-1 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 14:23 UTC (permalink / raw)
To: Ulf Hansson, Doug Anderson
Cc: Shawn Lin, Heiko Stübner, Mark Rutland, devicetree,
Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
Hi Ulf,
On 2016年09月02日 18:24, Ulf Hansson wrote:
> On 1 September 2016 at 23:50, Doug Anderson<dianders@chromium.org> wrote:
>> >Hi,
>> >
>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@linaro.org> wrote:
>>> >>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?
>> >
>> >I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>> >
>> >...in hardware there is a "pd_emmc" that is the power domain for just
>> >eMMC. That will be referenced hooked up via device tree, like:
>> >
>> >power-domains = <&power RK3399_PD_EMMC>;
> Yes, I noticed that and this is what puzzles me a bit.
>
>> >
>> >I believe that means that power will automatically be removed whenever
>> >the device is runtime suspended or suspended.
> Well, it depends if the genpd has a subdomain or other devices in it
> being runtime resumed.
> The genpd will not power off unless all devices within it are runtime
> enabled+suspended and that its subdomains are also powered off.
>
> So, in case you only have one device and no subdomains, then your
> statement is correct.
Yup, pd_emmc is a individual power domain which is only deployed to eMMC
on rk3399. It has no subdomains.
>
>> >
>> >If w're not supporting "autosuspend" and nobody is tweaking anything
> I guess you mean runtime PM autosuspend? Then why don't you support this?
>
> Wouldn't that allow you to avoid wasting power in runtime when the
> device is idle?
pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we
support autosuspend in driver, we have to re-init context. I didn't test
the latency, if it's acceptable, we will apply it.:-)
But it's not a blocker, right?
>
>> >manually, then it's possible (I think) that runtime suspend happens at
>> >exactly the same time as suspend. ...but my point was that it was
> I am not sure I follow you here. You must not rely on that the device
> always becomes runtime suspended during system suspend, as there are
> no guarantees for this.
>
> Instead that is something you need to take care of in the
> subsystem/driver for the device, of course.
>
>> >cleaner to actually do it any restoring in the "runtime resume" hooks
>> >to match what genpd does. This matches what you say: use runtime PM.
> Yes!
>
> Using genpd without deploying runtime PM for the devices doesn't make
> much sense, at least to me.
>
>> >
>> >...but it also sounds like it might not be terribly important to
>> >restore these values since they're a bit silly to begin with. If
>> >that's true then I guess we don't need to do anything special here.
>> >
>> >
>> >Did that all make sense (it's entirely possible it didn't since
>> >somehow my brain still hasn't absorbed all runtime PM and genpd
>> >concepts)
> No worries. I understand this might be a bit tricky, that's why I also
> tries to help review related changes.
>
> Kind regards
> Uffe
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 14:23 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 14:23 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ulf,
On 2016?09?02? 18:24, Ulf Hansson wrote:
> On 1 September 2016 at 23:50, Doug Anderson<dianders@chromium.org> wrote:
>> >Hi,
>> >
>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@linaro.org> wrote:
>>> >>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?
>> >
>> >I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>> >
>> >...in hardware there is a "pd_emmc" that is the power domain for just
>> >eMMC. That will be referenced hooked up via device tree, like:
>> >
>> >power-domains = <&power RK3399_PD_EMMC>;
> Yes, I noticed that and this is what puzzles me a bit.
>
>> >
>> >I believe that means that power will automatically be removed whenever
>> >the device is runtime suspended or suspended.
> Well, it depends if the genpd has a subdomain or other devices in it
> being runtime resumed.
> The genpd will not power off unless all devices within it are runtime
> enabled+suspended and that its subdomains are also powered off.
>
> So, in case you only have one device and no subdomains, then your
> statement is correct.
Yup, pd_emmc is a individual power domain which is only deployed to eMMC
on rk3399. It has no subdomains.
>
>> >
>> >If w're not supporting "autosuspend" and nobody is tweaking anything
> I guess you mean runtime PM autosuspend? Then why don't you support this?
>
> Wouldn't that allow you to avoid wasting power in runtime when the
> device is idle?
pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we
support autosuspend in driver, we have to re-init context. I didn't test
the latency, if it's acceptable, we will apply it.:-)
But it's not a blocker, right?
>
>> >manually, then it's possible (I think) that runtime suspend happens at
>> >exactly the same time as suspend. ...but my point was that it was
> I am not sure I follow you here. You must not rely on that the device
> always becomes runtime suspended during system suspend, as there are
> no guarantees for this.
>
> Instead that is something you need to take care of in the
> subsystem/driver for the device, of course.
>
>> >cleaner to actually do it any restoring in the "runtime resume" hooks
>> >to match what genpd does. This matches what you say: use runtime PM.
> Yes!
>
> Using genpd without deploying runtime PM for the devices doesn't make
> much sense, at least to me.
>
>> >
>> >...but it also sounds like it might not be terribly important to
>> >restore these values since they're a bit silly to begin with. If
>> >that's true then I guess we don't need to do anything special here.
>> >
>> >
>> >Did that all make sense (it's entirely possible it didn't since
>> >somehow my brain still hasn't absorbed all runtime PM and genpd
>> >concepts)
> No worries. I understand this might be a bit tricky, that's why I also
> tries to help review related changes.
>
> Kind regards
> Uffe
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 14:23 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 14:23 UTC (permalink / raw)
To: Ulf Hansson, Doug Anderson
Cc: Shawn Lin, Heiko Stübner, Mark Rutland, devicetree,
Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu
Hi Ulf,
On 2016年09月02日 18:24, Ulf Hansson wrote:
> On 1 September 2016 at 23:50, Doug Anderson<dianders@chromium.org> wrote:
>> >Hi,
>> >
>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@linaro.org> wrote:
>>> >>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?
>> >
>> >I'm not as familiar with genpd as I should be, so hopefully this makes sense.
>> >
>> >...in hardware there is a "pd_emmc" that is the power domain for just
>> >eMMC. That will be referenced hooked up via device tree, like:
>> >
>> >power-domains = <&power RK3399_PD_EMMC>;
> Yes, I noticed that and this is what puzzles me a bit.
>
>> >
>> >I believe that means that power will automatically be removed whenever
>> >the device is runtime suspended or suspended.
> Well, it depends if the genpd has a subdomain or other devices in it
> being runtime resumed.
> The genpd will not power off unless all devices within it are runtime
> enabled+suspended and that its subdomains are also powered off.
>
> So, in case you only have one device and no subdomains, then your
> statement is correct.
Yup, pd_emmc is a individual power domain which is only deployed to eMMC
on rk3399. It has no subdomains.
>
>> >
>> >If w're not supporting "autosuspend" and nobody is tweaking anything
> I guess you mean runtime PM autosuspend? Then why don't you support this?
>
> Wouldn't that allow you to avoid wasting power in runtime when the
> device is idle?
pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we
support autosuspend in driver, we have to re-init context. I didn't test
the latency, if it's acceptable, we will apply it.:-)
But it's not a blocker, right?
>
>> >manually, then it's possible (I think) that runtime suspend happens at
>> >exactly the same time as suspend. ...but my point was that it was
> I am not sure I follow you here. You must not rely on that the device
> always becomes runtime suspended during system suspend, as there are
> no guarantees for this.
>
> Instead that is something you need to take care of in the
> subsystem/driver for the device, of course.
>
>> >cleaner to actually do it any restoring in the "runtime resume" hooks
>> >to match what genpd does. This matches what you say: use runtime PM.
> Yes!
>
> Using genpd without deploying runtime PM for the devices doesn't make
> much sense, at least to me.
>
>> >
>> >...but it also sounds like it might not be terribly important to
>> >restore these values since they're a bit silly to begin with. If
>> >that's true then I guess we don't need to do anything special here.
>> >
>> >
>> >Did that all make sense (it's entirely possible it didn't since
>> >somehow my brain still hasn't absorbed all runtime PM and genpd
>> >concepts)
> No worries. I understand this might be a bit tricky, that's why I also
> tries to help review related changes.
>
> Kind regards
> Uffe
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-06 12:34 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-06 12:34 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Doug Anderson, Shawn Lin, Heiko Stübner, Mark Rutland,
devicetree, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
On 2 September 2016 at 16:23, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi Ulf,
>
>
> On 2016年09月02日 18:24, Ulf Hansson wrote:
>>
>> On 1 September 2016 at 23:50, Doug Anderson<dianders@chromium.org> wrote:
>>>
>>> >Hi,
>>> >
>>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@linaro.org>
>>> > wrote:
>>>>
>>>> >>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?
>>>
>>> >
>>> >I'm not as familiar with genpd as I should be, so hopefully this makes
>>> > sense.
>>> >
>>> >...in hardware there is a "pd_emmc" that is the power domain for just
>>> >eMMC. That will be referenced hooked up via device tree, like:
>>> >
>>> >power-domains = <&power RK3399_PD_EMMC>;
>>
>> Yes, I noticed that and this is what puzzles me a bit.
>>
>>> >
>>> >I believe that means that power will automatically be removed whenever
>>> >the device is runtime suspended or suspended.
>>
>> Well, it depends if the genpd has a subdomain or other devices in it
>> being runtime resumed.
>> The genpd will not power off unless all devices within it are runtime
>> enabled+suspended and that its subdomains are also powered off.
>>
>> So, in case you only have one device and no subdomains, then your
>> statement is correct.
>
>
> Yup, pd_emmc is a individual power domain which is only deployed to eMMC on
> rk3399. It has no subdomains.
>
>>
>>> >
>>> >If w're not supporting "autosuspend" and nobody is tweaking anything
>>
>> I guess you mean runtime PM autosuspend? Then why don't you support this?
>>
>> Wouldn't that allow you to avoid wasting power in runtime when the
>> device is idle?
>
>
> pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we support
Ohh, there are already mmc drivers dealing with phys. We can add this
for you driver as well.
Regarding corecfg_*, unless that also can be managed via a generic
interface (perhaps through syscon), then you need to manage that via
genpd. Is that the case?
> autosuspend in driver, we have to re-init context. I didn't test the
Yes, that's a common scenario, many device drivers do this - as to
avoid wasting power.
> latency, if it's acceptable, we will apply it.:-)
Latency constraints may be controlled via the genpd governor.
Moreover we also have the runtime PM autosuspend feature, which is
useful when you don't wont to runtime suspend the device between each
and every request. Thus avoiding latencies when there are a "burst" of
requests.
> But it's not a blocker, right?
>
Hmm, I just want to make sure we do the right things. Else it might
just bite us in the other end.
Again, my recommendation is to start with runtime PM then continue
with system PM. I can imagine you will get system PM "free" if using
the runtime PM centric approach.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-06 12:34 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-06 12:34 UTC (permalink / raw)
To: linux-arm-kernel
On 2 September 2016 at 16:23, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi Ulf,
>
>
> On 2016?09?02? 18:24, Ulf Hansson wrote:
>>
>> On 1 September 2016 at 23:50, Doug Anderson<dianders@chromium.org> wrote:
>>>
>>> >Hi,
>>> >
>>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@linaro.org>
>>> > wrote:
>>>>
>>>> >>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?
>>>
>>> >
>>> >I'm not as familiar with genpd as I should be, so hopefully this makes
>>> > sense.
>>> >
>>> >...in hardware there is a "pd_emmc" that is the power domain for just
>>> >eMMC. That will be referenced hooked up via device tree, like:
>>> >
>>> >power-domains = <&power RK3399_PD_EMMC>;
>>
>> Yes, I noticed that and this is what puzzles me a bit.
>>
>>> >
>>> >I believe that means that power will automatically be removed whenever
>>> >the device is runtime suspended or suspended.
>>
>> Well, it depends if the genpd has a subdomain or other devices in it
>> being runtime resumed.
>> The genpd will not power off unless all devices within it are runtime
>> enabled+suspended and that its subdomains are also powered off.
>>
>> So, in case you only have one device and no subdomains, then your
>> statement is correct.
>
>
> Yup, pd_emmc is a individual power domain which is only deployed to eMMC on
> rk3399. It has no subdomains.
>
>>
>>> >
>>> >If w're not supporting "autosuspend" and nobody is tweaking anything
>>
>> I guess you mean runtime PM autosuspend? Then why don't you support this?
>>
>> Wouldn't that allow you to avoid wasting power in runtime when the
>> device is idle?
>
>
> pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we support
Ohh, there are already mmc drivers dealing with phys. We can add this
for you driver as well.
Regarding corecfg_*, unless that also can be managed via a generic
interface (perhaps through syscon), then you need to manage that via
genpd. Is that the case?
> autosuspend in driver, we have to re-init context. I didn't test the
Yes, that's a common scenario, many device drivers do this - as to
avoid wasting power.
> latency, if it's acceptable, we will apply it.:-)
Latency constraints may be controlled via the genpd governor.
Moreover we also have the runtime PM autosuspend feature, which is
useful when you don't wont to runtime suspend the device between each
and every request. Thus avoiding latencies when there are a "burst" of
requests.
> But it's not a blocker, right?
>
Hmm, I just want to make sure we do the right things. Else it might
just bite us in the other end.
Again, my recommendation is to start with runtime PM then continue
with system PM. I can imagine you will get system PM "free" if using
the runtime PM centric approach.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-06 12:34 ` Ulf Hansson
0 siblings, 0 replies; 59+ messages in thread
From: Ulf Hansson @ 2016-09-06 12:34 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Doug Anderson, Shawn Lin, Heiko Stübner, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Xing Zheng,
open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Masahiro Yamada, Rob Herring, David Wu, Caesar Wang
On 2 September 2016 at 16:23, Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Hi Ulf,
>
>
> On 2016年09月02日 18:24, Ulf Hansson wrote:
>>
>> On 1 September 2016 at 23:50, Doug Anderson<dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>>
>>> >Hi,
>>> >
>>> >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> > wrote:
>>>>
>>>> >>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?
>>>
>>> >
>>> >I'm not as familiar with genpd as I should be, so hopefully this makes
>>> > sense.
>>> >
>>> >...in hardware there is a "pd_emmc" that is the power domain for just
>>> >eMMC. That will be referenced hooked up via device tree, like:
>>> >
>>> >power-domains = <&power RK3399_PD_EMMC>;
>>
>> Yes, I noticed that and this is what puzzles me a bit.
>>
>>> >
>>> >I believe that means that power will automatically be removed whenever
>>> >the device is runtime suspended or suspended.
>>
>> Well, it depends if the genpd has a subdomain or other devices in it
>> being runtime resumed.
>> The genpd will not power off unless all devices within it are runtime
>> enabled+suspended and that its subdomains are also powered off.
>>
>> So, in case you only have one device and no subdomains, then your
>> statement is correct.
>
>
> Yup, pd_emmc is a individual power domain which is only deployed to eMMC on
> rk3399. It has no subdomains.
>
>>
>>> >
>>> >If w're not supporting "autosuspend" and nobody is tweaking anything
>>
>> I guess you mean runtime PM autosuspend? Then why don't you support this?
>>
>> Wouldn't that allow you to avoid wasting power in runtime when the
>> device is idle?
>
>
> pd_emmc manages the sdhci controller, phy and corecfg_* stuff, if we support
Ohh, there are already mmc drivers dealing with phys. We can add this
for you driver as well.
Regarding corecfg_*, unless that also can be managed via a generic
interface (perhaps through syscon), then you need to manage that via
genpd. Is that the case?
> autosuspend in driver, we have to re-init context. I didn't test the
Yes, that's a common scenario, many device drivers do this - as to
avoid wasting power.
> latency, if it's acceptable, we will apply it.:-)
Latency constraints may be controlled via the genpd governor.
Moreover we also have the runtime PM autosuspend feature, which is
useful when you don't wont to runtime suspend the device between each
and every request. Thus avoiding latencies when there are a "burst" of
requests.
> But it's not a blocker, right?
>
Hmm, I just want to make sure we do the right things. Else it might
just bite us in the other end.
Again, my recommendation is to start with runtime PM then continue
with system PM. I can imagine you will get system PM "free" if using
the runtime PM centric approach.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 4:20 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 4:20 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Shawn Lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
Hi,
On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> 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.
> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
> fine.
I guess I don't actually know how the corecfg_clockmultiplier and
corecfg_baseclkfreq fields are actually used, but I presume that they
actually do something useful and aren't used to just communicate back
to software?
I know that:
1. If I don't pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
suspend / resume.
2. If I do pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
suspend/resume (tested by reading /dev/mem directly from userspace
after suspend/resume).
Are you saying that it is unimportant that corecfg_clockmultiplier and
corecfg_baseclkfreq are wrong?
>> 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?
>
> @shawn, what's your opinion?
I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
guess we can just worry about suspend/resume, though.
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 4:20 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 4:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> 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.
> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
> fine.
I guess I don't actually know how the corecfg_clockmultiplier and
corecfg_baseclkfreq fields are actually used, but I presume that they
actually do something useful and aren't used to just communicate back
to software?
I know that:
1. If I don't pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
suspend / resume.
2. If I do pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
suspend/resume (tested by reading /dev/mem directly from userspace
after suspend/resume).
Are you saying that it is unimportant that corecfg_clockmultiplier and
corecfg_baseclkfreq are wrong?
>> 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?
>
> @shawn, what's your opinion?
I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
guess we can just worry about suspend/resume, though.
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 4:20 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 4:20 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Shawn Lin, Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
Ulf Hansson, Heiko Stübner, Xing Zheng,
open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
Masahiro Yamada, Rob Herring, David Wu, Caesar Wang
Hi,
On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> 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.
> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
> fine.
I guess I don't actually know how the corecfg_clockmultiplier and
corecfg_baseclkfreq fields are actually used, but I presume that they
actually do something useful and aren't used to just communicate back
to software?
I know that:
1. If I don't pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
suspend / resume.
2. If I do pick this patch and I suspend/resume,
corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
suspend/resume (tested by reading /dev/mem directly from userspace
after suspend/resume).
Are you saying that it is unimportant that corecfg_clockmultiplier and
corecfg_baseclkfreq are wrong?
>> 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?
>
> @shawn, what's your opinion?
I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
guess we can just worry about suspend/resume, though.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-01 4:20 ` Doug Anderson
(?)
@ 2016-09-01 6:56 ` Ziyuan Xu
-1 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-01 6:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Shawn Lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
Hi
On 2016年09月01日 12:20, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>> 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.
>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
>> fine.
> I guess I don't actually know how the corecfg_clockmultiplier and
> corecfg_baseclkfreq fields are actually used, but I presume that they
> actually do something useful and aren't used to just communicate back
> to software?
Take corecfg_clockmultiplier as example.
1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
used for further initialization.
3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
frequency to play.
I think we don't need to store it due to it's a fixed value at run-time,
even if it is reset after a power cycle, the above will not be changed
via software, except for dirver unbind .
>
> I know that:
>
> 1. If I don't pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
> suspend / resume.
>
> 2. If I do pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
> suspend/resume (tested by reading /dev/mem directly from userspace
> after suspend/resume).
>
>
> Are you saying that it is unimportant that corecfg_clockmultiplier and
> corecfg_baseclkfreq are wrong?
Yup, corecfg_* stuff will be reset after a power cycle.
I mean that we need only to guarantee they're correct at probe time.
>
>>> 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?
>>
>> @shawn, what's your opinion?
> I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
> guess we can just worry about suspend/resume, though.
>
> -Doug
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 6:56 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-01 6:56 UTC (permalink / raw)
To: linux-arm-kernel
Hi
On 2016?09?01? 12:20, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>> 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.
>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
>> fine.
> I guess I don't actually know how the corecfg_clockmultiplier and
> corecfg_baseclkfreq fields are actually used, but I presume that they
> actually do something useful and aren't used to just communicate back
> to software?
Take corecfg_clockmultiplier as example.
1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
used for further initialization.
3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
frequency to play.
I think we don't need to store it due to it's a fixed value at run-time,
even if it is reset after a power cycle, the above will not be changed
via software, except for dirver unbind .
>
> I know that:
>
> 1. If I don't pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
> suspend / resume.
>
> 2. If I do pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
> suspend/resume (tested by reading /dev/mem directly from userspace
> after suspend/resume).
>
>
> Are you saying that it is unimportant that corecfg_clockmultiplier and
> corecfg_baseclkfreq are wrong?
Yup, corecfg_* stuff will be reset after a power cycle.
I mean that we need only to guarantee they're correct at probe time.
>
>>> 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?
>>
>> @shawn, what's your opinion?
> I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
> guess we can just worry about suspend/resume, though.
>
> -Doug
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 6:56 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-01 6:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Shawn Lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang
Hi
On 2016年09月01日 12:20, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>> 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.
>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC works
>> fine.
> I guess I don't actually know how the corecfg_clockmultiplier and
> corecfg_baseclkfreq fields are actually used, but I presume that they
> actually do something useful and aren't used to just communicate back
> to software?
Take corecfg_clockmultiplier as example.
1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
used for further initialization.
3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
frequency to play.
I think we don't need to store it due to it's a fixed value at run-time,
even if it is reset after a power cycle, the above will not be changed
via software, except for dirver unbind .
>
> I know that:
>
> 1. If I don't pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
> suspend / resume.
>
> 2. If I do pick this patch and I suspend/resume,
> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
> suspend/resume (tested by reading /dev/mem directly from userspace
> after suspend/resume).
>
>
> Are you saying that it is unimportant that corecfg_clockmultiplier and
> corecfg_baseclkfreq are wrong?
Yup, corecfg_* stuff will be reset after a power cycle.
I mean that we need only to guarantee they're correct at probe time.
>
>>> 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?
>>
>> @shawn, what's your opinion?
> I haven't dug. If Runtime PM isn't enabled for sdhci-of-arasan then I
> guess we can just worry about suspend/resume, though.
>
> -Doug
>
>
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-01 6:56 ` Ziyuan Xu
(?)
@ 2016-09-01 21:29 ` Doug Anderson
-1 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:29 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Shawn Lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang, Jianqun Xu, linux-arm-kernel,
Shunqian Zheng
Hi,
On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi
>
>
> On 2016年09月01日 12:20, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>
>>>> 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.
>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>> works
>>> fine.
>>
>> I guess I don't actually know how the corecfg_clockmultiplier and
>> corecfg_baseclkfreq fields are actually used, but I presume that they
>> actually do something useful and aren't used to just communicate back
>> to software?
>
>
> Take corecfg_clockmultiplier as example.
> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
> for further initialization.
> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
> frequency to play.
>
> I think we don't need to store it due to it's a fixed value at run-time,
> even if it is reset after a power cycle, the above will not be changed via
> software, except for dirver unbind .
>
>>
>> I know that:
>>
>> 1. If I don't pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>> suspend / resume.
>>
>> 2. If I do pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>> suspend/resume (tested by reading /dev/mem directly from userspace
>> after suspend/resume).
>>
>>
>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>> corecfg_baseclkfreq are wrong?
>
>
> Yup, corecfg_* stuff will be reset after a power cycle.
> I mean that we need only to guarantee they're correct at probe time.
So are you saying that the entire purpose of "corecfg_clockmultiplier"
is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
register to get a certain value?
...and that the entire purpose of "corecfg_baseclkfreq" is that it
causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
to get a certain value?
That would have been nice to know before. I had assumed that those
"corecfg" settings did something else more useful.
If it is indeed true that these corecfg values are as silly as it
seems, then I guess it's not terribly important to re-set them after
suspend/resume. Eventually it would be nice/clean to actually do so
(in case the SDHCI driver eventually changes), but I guess we wouldn't
need to block. this patch from landing.
Can you please confirm my understanding above?
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 21:29 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:29 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi
>
>
> On 2016?09?01? 12:20, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>
>>>> 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.
>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>> works
>>> fine.
>>
>> I guess I don't actually know how the corecfg_clockmultiplier and
>> corecfg_baseclkfreq fields are actually used, but I presume that they
>> actually do something useful and aren't used to just communicate back
>> to software?
>
>
> Take corecfg_clockmultiplier as example.
> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
> for further initialization.
> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
> frequency to play.
>
> I think we don't need to store it due to it's a fixed value at run-time,
> even if it is reset after a power cycle, the above will not be changed via
> software, except for dirver unbind .
>
>>
>> I know that:
>>
>> 1. If I don't pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>> suspend / resume.
>>
>> 2. If I do pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>> suspend/resume (tested by reading /dev/mem directly from userspace
>> after suspend/resume).
>>
>>
>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>> corecfg_baseclkfreq are wrong?
>
>
> Yup, corecfg_* stuff will be reset after a power cycle.
> I mean that we need only to guarantee they're correct at probe time.
So are you saying that the entire purpose of "corecfg_clockmultiplier"
is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
register to get a certain value?
...and that the entire purpose of "corecfg_baseclkfreq" is that it
causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
to get a certain value?
That would have been nice to know before. I had assumed that those
"corecfg" settings did something else more useful.
If it is indeed true that these corecfg values are as silly as it
seems, then I guess it's not terribly important to re-set them after
suspend/resume. Eventually it would be nice/clean to actually do so
(in case the SDHCI driver eventually changes), but I guess we wouldn't
need to block. this patch from landing.
Can you please confirm my understanding above?
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-01 21:29 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-01 21:29 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Shawn Lin, Mark Rutland, devicetree, Ulf Hansson,
Heiko Stübner, Xing Zheng, open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Elaine Zhang, Will Deacon,
linux-kernel, Brian Norris, Masahiro Yamada, Rob Herring,
David Wu, Caesar Wang
Hi,
On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi
>
>
> On 2016年09月01日 12:20, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>
>>>> 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.
>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>> works
>>> fine.
>>
>> I guess I don't actually know how the corecfg_clockmultiplier and
>> corecfg_baseclkfreq fields are actually used, but I presume that they
>> actually do something useful and aren't used to just communicate back
>> to software?
>
>
> Take corecfg_clockmultiplier as example.
> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
> for further initialization.
> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
> frequency to play.
>
> I think we don't need to store it due to it's a fixed value at run-time,
> even if it is reset after a power cycle, the above will not be changed via
> software, except for dirver unbind .
>
>>
>> I know that:
>>
>> 1. If I don't pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>> suspend / resume.
>>
>> 2. If I do pick this patch and I suspend/resume,
>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>> suspend/resume (tested by reading /dev/mem directly from userspace
>> after suspend/resume).
>>
>>
>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>> corecfg_baseclkfreq are wrong?
>
>
> Yup, corecfg_* stuff will be reset after a power cycle.
> I mean that we need only to guarantee they're correct at probe time.
So are you saying that the entire purpose of "corecfg_clockmultiplier"
is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
register to get a certain value?
...and that the entire purpose of "corecfg_baseclkfreq" is that it
causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
to get a certain value?
That would have been nice to know before. I had assumed that those
"corecfg" settings did something else more useful.
If it is indeed true that these corecfg values are as silly as it
seems, then I guess it's not terribly important to re-set them after
suspend/resume. Eventually it would be nice/clean to actually do so
(in case the SDHCI driver eventually changes), but I guess we wouldn't
need to block. this patch from landing.
Can you please confirm my understanding above?
-Doug
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 2:35 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 2:35 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Rutland, devicetree, Ulf Hansson, Brian Norris,
Heiko Stübner, Xing Zheng, Masahiro Yamada, Frank Wang,
Catalin Marinas, Shawn Lin, Elaine Zhang, Will Deacon,
linux-kernel, open list:ARM/Rockchip SoC...,
Rob Herring, David Wu, Shunqian Zheng, Jianqun Xu,
linux-arm-kernel, Caesar Wang
On 2016年09月02日 05:29, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> Hi
>>
>>
>> On 2016年09月01日 12:20, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>> 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.
>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>> works
>>>> fine.
>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>> actually do something useful and aren't used to just communicate back
>>> to software?
>>
>> Take corecfg_clockmultiplier as example.
>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
>> for further initialization.
>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>> frequency to play.
>>
>> I think we don't need to store it due to it's a fixed value at run-time,
>> even if it is reset after a power cycle, the above will not be changed via
>> software, except for dirver unbind .
>>
>>> I know that:
>>>
>>> 1. If I don't pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>> suspend / resume.
>>>
>>> 2. If I do pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>> after suspend/resume).
>>>
>>>
>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>> corecfg_baseclkfreq are wrong?
>>
>> Yup, corecfg_* stuff will be reset after a power cycle.
>> I mean that we need only to guarantee they're correct at probe time.
> So are you saying that the entire purpose of "corecfg_clockmultiplier"
> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
> register to get a certain value?
> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
> to get a certain value?
Yes, on rk3399:
corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
If you re-write to either corecfg_* stuff, the corresponding CAP
register field will be changed too.
sdhci driver will fetch CAP register for initialization, we only need to
guarantee they're correct at probe time.
Did that all make sense?
>
> That would have been nice to know before. I had assumed that those
> "corecfg" settings did something else more useful.
>
> If it is indeed true that these corecfg values are as silly as it
> seems, then I guess it's not terribly important to re-set them after
> suspend/resume. Eventually it would be nice/clean to actually do so
> (in case the SDHCI driver eventually changes), but I guess we wouldn't
> need to block. this patch from landing.
>
> Can you please confirm my understanding above?
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 2:35 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 2:35 UTC (permalink / raw)
To: linux-arm-kernel
On 2016?09?02? 05:29, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> Hi
>>
>>
>> On 2016?09?01? 12:20, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>> 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.
>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>> works
>>>> fine.
>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>> actually do something useful and aren't used to just communicate back
>>> to software?
>>
>> Take corecfg_clockmultiplier as example.
>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
>> for further initialization.
>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>> frequency to play.
>>
>> I think we don't need to store it due to it's a fixed value at run-time,
>> even if it is reset after a power cycle, the above will not be changed via
>> software, except for dirver unbind .
>>
>>> I know that:
>>>
>>> 1. If I don't pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>> suspend / resume.
>>>
>>> 2. If I do pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>> after suspend/resume).
>>>
>>>
>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>> corecfg_baseclkfreq are wrong?
>>
>> Yup, corecfg_* stuff will be reset after a power cycle.
>> I mean that we need only to guarantee they're correct at probe time.
> So are you saying that the entire purpose of "corecfg_clockmultiplier"
> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
> register to get a certain value?
> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
> to get a certain value?
Yes, on rk3399:
corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
If you re-write to either corecfg_* stuff, the corresponding CAP
register field will be changed too.
sdhci driver will fetch CAP register for initialization, we only need to
guarantee they're correct at probe time.
Did that all make sense?
>
> That would have been nice to know before. I had assumed that those
> "corecfg" settings did something else more useful.
>
> If it is indeed true that these corecfg values are as silly as it
> seems, then I guess it's not terribly important to re-set them after
> suspend/resume. Eventually it would be nice/clean to actually do so
> (in case the SDHCI driver eventually changes), but I guess we wouldn't
> need to block. this patch from landing.
>
> Can you please confirm my understanding above?
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 2:35 ` Ziyuan Xu
0 siblings, 0 replies; 59+ messages in thread
From: Ziyuan Xu @ 2016-09-02 2:35 UTC (permalink / raw)
To: Doug Anderson
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
Elaine Zhang, Heiko Stübner, Xing Zheng,
open list:ARM/Rockchip SoC...,
Frank Wang, Catalin Marinas, Shawn Lin, Brian Norris,
Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Masahiro Yamada, Rob Herring, Jianqun Xu, David Wu,
Shunqian Zheng,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 2016年09月02日 05:29, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>> Hi
>>
>>
>> On 2016年09月01日 12:20, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>>> 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.
>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>> works
>>>> fine.
>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>> actually do something useful and aren't used to just communicate back
>>> to software?
>>
>> Take corecfg_clockmultiplier as example.
>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're used
>> for further initialization.
>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>> frequency to play.
>>
>> I think we don't need to store it due to it's a fixed value at run-time,
>> even if it is reset after a power cycle, the above will not be changed via
>> software, except for dirver unbind .
>>
>>> I know that:
>>>
>>> 1. If I don't pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>> suspend / resume.
>>>
>>> 2. If I do pick this patch and I suspend/resume,
>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>> after suspend/resume).
>>>
>>>
>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>> corecfg_baseclkfreq are wrong?
>>
>> Yup, corecfg_* stuff will be reset after a power cycle.
>> I mean that we need only to guarantee they're correct at probe time.
> So are you saying that the entire purpose of "corecfg_clockmultiplier"
> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
> register to get a certain value?
> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
> to get a certain value?
Yes, on rk3399:
corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
If you re-write to either corecfg_* stuff, the corresponding CAP
register field will be changed too.
sdhci driver will fetch CAP register for initialization, we only need to
guarantee they're correct at probe time.
Did that all make sense?
>
> That would have been nice to know before. I had assumed that those
> "corecfg" settings did something else more useful.
>
> If it is indeed true that these corecfg values are as silly as it
> seems, then I guess it's not terribly important to re-set them after
> suspend/resume. Eventually it would be nice/clean to actually do so
> (in case the SDHCI driver eventually changes), but I guess we wouldn't
> need to block. this patch from landing.
>
> Can you please confirm my understanding above?
>
>
> -Doug
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
2016-09-02 2:35 ` Ziyuan Xu
(?)
@ 2016-09-02 5:22 ` Doug Anderson
-1 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-02 5:22 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Mark Rutland, devicetree, Ulf Hansson, Brian Norris,
Heiko Stübner, Xing Zheng, Masahiro Yamada, Frank Wang,
Catalin Marinas, Shawn Lin, Elaine Zhang, Will Deacon,
linux-kernel, open list:ARM/Rockchip SoC...,
Rob Herring, David Wu, Shunqian Zheng, Jianqun Xu,
linux-arm-kernel, Caesar Wang
Hi,
On Thu, Sep 1, 2016 at 7:35 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>
>
> On 2016年09月02日 05:29, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>
>>> Hi
>>>
>>>
>>> On 2016年09月01日 12:20, Doug Anderson wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com>
>>>> wrote:
>>>>>>
>>>>>> 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.
>>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>>> works
>>>>> fine.
>>>>
>>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>>> actually do something useful and aren't used to just communicate back
>>>> to software?
>>>
>>>
>>> Take corecfg_clockmultiplier as example.
>>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
>>> used
>>> for further initialization.
>>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>>> frequency to play.
>>>
>>> I think we don't need to store it due to it's a fixed value at run-time,
>>> even if it is reset after a power cycle, the above will not be changed
>>> via
>>> software, except for dirver unbind .
>>>
>>>> I know that:
>>>>
>>>> 1. If I don't pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>>> suspend / resume.
>>>>
>>>> 2. If I do pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>>> after suspend/resume).
>>>>
>>>>
>>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq are wrong?
>>>
>>>
>>> Yup, corecfg_* stuff will be reset after a power cycle.
>>> I mean that we need only to guarantee they're correct at probe time.
>>
>> So are you saying that the entire purpose of "corecfg_clockmultiplier"
>> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
>> register to get a certain value?
>> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
>> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
>> to get a certain value?
>
> Yes, on rk3399:
> corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
> corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
>
> If you re-write to either corecfg_* stuff, the corresponding CAP register
> field will be changed too.
> sdhci driver will fetch CAP register for initialization, we only need to
> guarantee they're correct at probe time.
>
> Did that all make sense?
Yes. Very odd, but it makes sense.
It would still be nice to get these restored after runtime resume just
for cleanliness, but it's not a blocker IMHO.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 5:22 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-02 5:22 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Thu, Sep 1, 2016 at 7:35 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>
>
> On 2016?09?02? 05:29, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>
>>> Hi
>>>
>>>
>>> On 2016?09?01? 12:20, Doug Anderson wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com>
>>>> wrote:
>>>>>>
>>>>>> 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.
>>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>>> works
>>>>> fine.
>>>>
>>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>>> actually do something useful and aren't used to just communicate back
>>>> to software?
>>>
>>>
>>> Take corecfg_clockmultiplier as example.
>>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
>>> used
>>> for further initialization.
>>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>>> frequency to play.
>>>
>>> I think we don't need to store it due to it's a fixed value at run-time,
>>> even if it is reset after a power cycle, the above will not be changed
>>> via
>>> software, except for dirver unbind .
>>>
>>>> I know that:
>>>>
>>>> 1. If I don't pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>>> suspend / resume.
>>>>
>>>> 2. If I do pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>>> after suspend/resume).
>>>>
>>>>
>>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq are wrong?
>>>
>>>
>>> Yup, corecfg_* stuff will be reset after a power cycle.
>>> I mean that we need only to guarantee they're correct at probe time.
>>
>> So are you saying that the entire purpose of "corecfg_clockmultiplier"
>> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
>> register to get a certain value?
>> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
>> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
>> to get a certain value?
>
> Yes, on rk3399:
> corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
> corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
>
> If you re-write to either corecfg_* stuff, the corresponding CAP register
> field will be changed too.
> sdhci driver will fetch CAP register for initialization, we only need to
> guarantee they're correct at probe time.
>
> Did that all make sense?
Yes. Very odd, but it makes sense.
It would still be nice to get these restored after runtime resume just
for cleanliness, but it's not a blocker IMHO.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
@ 2016-09-02 5:22 ` Doug Anderson
0 siblings, 0 replies; 59+ messages in thread
From: Doug Anderson @ 2016-09-02 5:22 UTC (permalink / raw)
To: Ziyuan Xu
Cc: Mark Rutland, devicetree, Ulf Hansson, Brian Norris,
Heiko Stübner, Xing Zheng, Masahiro Yamada, Frank Wang,
Catalin Marinas, Shawn Lin, Elaine Zhang, Will Deacon,
linux-kernel, open list:ARM/Rockchip SoC...,
Rob Herring, David Wu, Shunqian Zheng
Hi,
On Thu, Sep 1, 2016 at 7:35 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>
>
> On 2016年09月02日 05:29, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Aug 31, 2016 at 11:56 PM, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
>>>
>>> Hi
>>>
>>>
>>> On 2016年09月01日 12:20, Doug Anderson wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Aug 31, 2016 at 7:29 PM, Ziyuan Xu <xzy.xu@rock-chips.com>
>>>> wrote:
>>>>>>
>>>>>> 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.
>>>>> BTW, I have tested suspend/resume on rk3399 prior to this sumbit, eMMC
>>>>> works
>>>>> fine.
>>>>
>>>> I guess I don't actually know how the corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq fields are actually used, but I presume that they
>>>> actually do something useful and aren't used to just communicate back
>>>> to software?
>>>
>>>
>>> Take corecfg_clockmultiplier as example.
>>> 1. sdhci driver fetch host->clk_mul from corecfg_clockmultiplier
>>> 2. mmc->f_min and mmc->f_max are calculated via host->clk_mul, they're
>>> used
>>> for further initialization.
>>> 3. if the corecfg_clockmultiplier is incorrect, sdhci will use improper
>>> frequency to play.
>>>
>>> I think we don't need to store it due to it's a fixed value at run-time,
>>> even if it is reset after a power cycle, the above will not be changed
>>> via
>>> software, except for dirver unbind .
>>>
>>>> I know that:
>>>>
>>>> 1. If I don't pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are still fine after
>>>> suspend / resume.
>>>>
>>>> 2. If I do pick this patch and I suspend/resume,
>>>> corecfg_clockmultiplier and corecfg_baseclkfreq are wrong after
>>>> suspend/resume (tested by reading /dev/mem directly from userspace
>>>> after suspend/resume).
>>>>
>>>>
>>>> Are you saying that it is unimportant that corecfg_clockmultiplier and
>>>> corecfg_baseclkfreq are wrong?
>>>
>>>
>>> Yup, corecfg_* stuff will be reset after a power cycle.
>>> I mean that we need only to guarantee they're correct at probe time.
>>
>> So are you saying that the entire purpose of "corecfg_clockmultiplier"
>> is that causes the "ClockMultiplier" field of the "EMMCCORE_CAP"
>> register to get a certain value?
>> ...and that the entire purpose of "corecfg_baseclkfreq" is that it
>> causes the "BaseClockFreqSDClock" field of the "EMMCCORE_CAP" register
>> to get a certain value?
>
> Yes, on rk3399:
> corecfg_clockmultiplier <===> EMMCCORE_CAP1[23:16] ClockMultiplier
> corecfg_baseclkfreq <===> EMMCCORE_CAP[15:8] BaseClockFreqSDClock
>
> If you re-write to either corecfg_* stuff, the corresponding CAP register
> field will be changed too.
> sdhci driver will fetch CAP register for initialization, we only need to
> guarantee they're correct at probe time.
>
> Did that all make sense?
Yes. Very odd, but it makes sense.
It would still be nice to get these restored after runtime resume just
for cleanliness, but it's not a blocker IMHO.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 59+ messages in thread