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

From: Ziyuan Xu <xzy.xu@rock-chips.com>
To: Ulf Hansson <ulf.hansson@linaro.org>,
	Doug Anderson <dianders@chromium.org>
Cc: "Shawn Lin" <shawn.lin@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Xing Zheng" <zhengxing@rock-chips.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"Frank Wang" <frank.wang@rock-chips.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Elaine Zhang" <zhangqing@rock-chips.com>,
	"Will Deacon" <will.deacon@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Brian Norris" <briannorris@chromium.org>,
	"Masahiro Yamada" <yamada.masahiro@socionext.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"David Wu" <david.wu@rock-chips.com>,
	"Caesar Wang" <wxt@rock-chips.com>,
	"Jianqun Xu" <jay.xu@rock-chips.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"Shunqian Zheng" <zhengsq@rock-chips.com>
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: add eMMC's power domain support for rk3399
Date: Fri, 2 Sep 2016 22:23:15 +0800	[thread overview]
Message-ID: <10dfff58-88b2-a397-ef2e-9cd451253c40@rock-chips.com> (raw)
In-Reply-To: <CAPDyKFra7L5iUJegHH43mF4Dw=wYo0PmH2g_96fNL_=oRnGgug@mail.gmail.com>

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