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

[Ziyuan Xu]

Hi Ulf,


On 2016å09æ02æ 18:24, Ulf Hansson wrote:
> On 1 September 2016 at 23:50, Doug Anderson<dianders@xxxxxxxxxxxx>  wrote:
> > >Hi,
> > >
> > >On Thu, Sep 1, 2016 at 6:45 AM, Ulf Hansson<ulf.hansson@xxxxxxxxxx>  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