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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 07/14/2017 02:09 AM, Ulf Hansson wrote:
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?

This is my understanding. Hope it can make you clear.
The clock tree is like below.
SOC --> [ SDH_CLK_GEN --> SDH_CONTROLLER ] --> SD/EMMC CARD

There is one clock generator inside sdh slot IP, SOC provides the clock to the sdh slot IP. This clock is enabled/disabled by SW when calling clk_prepare_enable/clk_disable_unprepare. The auto clock gating is not any outer logic, it is inside sdh slot IP, when sdh controller has no activity, the IP will gate the clock from sdh_clk_gen to sdh_controller. sdh_clk_gen logic itself still has clock from SOC. With or without runtime pm, the only difference is if sdh_clk_gen has clock or not. So the power benefit is limited.


With SW runtime pm mechanism, compares with HW auto clock gating, the only
difference is SW cut the source of sdh clock tree, external clock gating vs
internal clock gating, there will be some benefits, but limited.
Right.

Previously we enabled the runtime pm mechanism in our mobile products, which
were using the same IP(some old version, including 3 sdh slots) with auto
clock gating feature(the driver is sdhci-pxav3.c).  The saving of power was
about 2~3mA@vcc_main_1.05V(28nm chip) with 3 sdh slots inside soc. No more
than 1mA/1sdh slot.
1 mA/sdh slot is a great reason to deploy runtime PM support. For a
battery driven device that would be a significant improvement.

Back in the days when I worked at ST-Ericssion, we were chasing uA
when optimizing for power-save. :-)

Definitely for mobile products, but now I didn't see urgent requirement for our networking products.

I read sdhci-of-at91 driver and your recommended patch, I got your point is
using a light way for system sleep based on runtime pm feature. From SW
perspective, kill two birds with one stone, it is good.
Right.

But considering about the benefits, it is not that urgent to take runtime pm
feature as a must, it is a better to have feature. System standby is a must
feature, without this patch, the system can't work well after resume.
Do you think it is reasonable to add complete standby support at first, then
take runtime pm as a next step?
You can do that, but why? And will then the "next step" ever happen?

Do you really want to spend efforts in getting something working for
system suspend only, while you instead easily could deploy both
runtime PM and system PM support at the same time?

As Ziji said in another mail, it takes time for next step. The runtime pm need to be verified completely on all supported boards. I understand from SW perspective, we'd better have both. But I need input from internal customers to see if they only request system sleep or they want both, and what's their priority.

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.
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



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux