2017-07-14 17:09 GMT+08:00 Ulf Hansson <ulf.hansson@xxxxxxxxxx>:
> On 13 July 2017 at 23:45, Zhoujie Wu <zjwu@xxxxxxxxxxx> wrote:
>>
>>
>> On 07/13/2017 04:03 AM, Ulf Hansson wrote:
>>>
>>> On 13 July 2017 at 12:48, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>>>
>>>> On 13 July 2017 at 12:13, Jisheng Zhang <jszhang@xxxxxxxxxxx> wrote:
>>>>>
>>>>> On Thu, 13 Jul 2017 11:52:54 +0200 Ulf Hansson wrote:
>>>>>
>>>>>> On 13 July 2017 at 11:25, Jisheng Zhang <jszhang@xxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>> Hi Ulf,
>>>>>>>
>>>>>>> On Thu, 13 Jul 2017 11:18:32 +0200 Ulf Hansson wrote:
>>>>>>>
>>>>>>>> On 13 July 2017 at 00:16, Zhoujie Wu <zjwu@xxxxxxxxxxx> wrote:
>>>>>>>>>
>>>>>>>>> From: Hu Ziji <huziji@xxxxxxxxxxx>
>>>>>>>>>
>>>>>>>>> Add Xenon specific system-level suspend and resume support.
>>>>>>>>> Especially during resume, re-configure Xenon specific registers
>>>>>>>>> since registers setting will be lost in suspend if Xenon is power
>>>>>>>>> off.
>>>>>>>>
>>>>>>>> I recommend to start with deploying runtime PM support instead of
>>>>>>>> system PM support. Then on top of such change, you should make use of
>>>>>>>> the runtime PM centric path to get system sleep support for "free"
>>>>>>>> (and thus all the nice benefits).
>>>>>>>
>>>>>>> I'm not sure whether runtime PM is useful for xenon case. The xenon HW
>>>>>>> support ACG(Auto Clock Gating) and SDCLK-Off-While-Idle features,
>>>>>>> that's
>>>>>>> to say we even don't need to do anything but achieve the runtime PM
>>>>>>> gains.
>>>>>>
>>>>>> Yeah, but that's only internally managed by mmc controller. The clock
>>>>>> will not be unprepared/disabled, from clock tree point of view. Isn't
>>>>>> that also worth doing?
>>>>>>
>>>>> The HW is clock gated, the difference is clock itself. From power saving
>>>>> point of view, the gain is nearly zero. From latency point of view,
>>>>> could
>>>>
>>>> I assume the clock you are talking about is the "core" clock? I then
>>>> assumes that clock is used as the interface clock for the card?
>>>>
>>>> That makes me wonder, don't you have other device clocks to manage as
>>>> well? Clocks that is provided to the controller to make it functional?
>>
>> At first, really appreciate your quick and valuable feedback.
>> The core clock in this driver is the clock provided by SOC to sdh
>> controller, and there is a divider inside the controller to generate sdclk
>> which provides to sd/emmc card.
>> Actually there are two runtime power saving features inside the controller
>> per my understanding.
>> sdclk_idle_enable will cut the clock to sd/emmc card if sd bus idle for some
>> time. auto_clkgate_enable means HW will auto gate the clock to sdh
>> controller core logic.
>
> I am not sure I get the second part here. The clock to shd is enabled
> via a call to clk_prepare_enable(). Unless you explicitly call
> clk_disable_unprepare() for it, no? How can any outer logic know when
> it can be gated?
>
>> With SW runtime pm mechanism, compares with HW auto clock gating, the only
>> difference is SW cut the source of sdh clock tree, external clock gating vs
>> internal clock gating, there will be some benefits, but limited.
>
> Right.
>
>> Previously we enabled the runtime pm mechanism in our mobile products, which
>> were using the same IP(some old version, including 3 sdh slots) with auto
>> clock gating feature(the driver is sdhci-pxav3.c). The saving of power was
>> about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more
>> than 1mA/1sdh slot.
>
> 1 mA/sdh slot is a great reason to deploy runtime PM support. For a
> battery driven device that would be a significant improvement.
>
> Back in the days when I worked at ST-Ericssion, we were chasing uA
> when optimizing for power-save. :-)
>
>> I read sdhci-of-at91 driver and your recommended patch, I got your point is
>> using a light way for system sleep based on runtime pm feature. From SW
>> perspective, kill two birds with one stone, it is good.
>
> Right.
>
>> But considering about the benefits, it is not that urgent to take runtime pm
>> feature as a must, it is a better to have feature. System standby is a must
>> feature, without this patch, the system can't work well after resume.
>> Do you think it is reasonable to add complete standby support at first, then
>> take runtime pm as a next step?
>
> You can do that, but why? And will then the "next step" ever happen?
>
> Do you really want to spend efforts in getting something working for
> system suspend only, while you instead easily could deploy both
> runtime PM and system PM support at the same time?
>
As the author of this patch, I'd like to argue for myself here, although it is
not my task any more. :p
The reason of implementing system PM firstly, is definitely NOT that
runtime PM is
unnecessary. Instead, I was just not able to.
Internal customers in Marvell and external customers previously just required
simple system PM. Thus I decided to meet customers requirement at first.
Otherwise, I didn't have any platform to verify runtime PM even if runtime PM
is completed.
Those SoCs implementation vary a lot. Thus I was actually not sure
what a generic Xenon runtime PM should look like for all those Marvell products.
I previously planed to implement runtime PM when I got enough resources
(boards/BSP/supports) from customers.
>>
>>> Besides the clocks, you have the xenon mmc phy. Can't that also be put
>>> that in some low power mode at request in-activity?
>>
>> For the phy behavior, currently I don't see any SW operation for the lpm, I
>> will check with HW guys about the behaviour.
>
> Great, that would really be interesting from a runtime PM point of view.
>
> Perhaps then also ask if re-configuring the phy via xenon_phy_adj(),
> makes sense when powering off the card? Because currently you seems to
> keep the latest configuration, even if the mmc core decides to power
> off the card during system sleep. Unless I am reading the code wrong
> from the ->set_ios() ops.
>
As I know, PHY HW will also automatically enter standby mode
(power saving mode)
when ACG/SDCLK-off-while-idle are triggered. Actually there is no SW interface
to "enable" or "disable" PHY. It requires confirm from HW bros.
xenon_phy_adj() will set PHY based on current timing. For example,
when coming back
from resume, xenon_phy_adj() will re-configure PHY for legacy timing.
Thus it is unnecessary to clean PHY setting before sleep.
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
