Re: [PATCH 6/9] ARM: tegra: Export tegra_powergate_power_on

[Peter De Schrijver <pdeschrijver@nvidia.com>]

On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > 
> > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > someday which means we would need to control the domain and therefore would
> > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > driver doesn't know anything about the ISP hw otherwise.
> > > 
> > > Can't we make powergates reference counted so that they don't get
> > > disabled as long as there are any users? Looking for example at the
> > 
> > We could, but then why not switch to the powerdomain code and make powering
> > off a domain a NOP until we sorted out the context save/restore or fixed
> > the framework to allow for suspend without turning off the domains?
> 
> Well, one of the reasons why I'm not sure it's worth the effort at this
> point is that we can't get rid of the tegra_powergate_*() API anyway
> because of backwards compatibility. So we're going to add code (without
> getting rid of old code) merely to support some generic framework. That
> doesn't sound very useful to me.
> 

We can also convert the existing users to genpd. Today there are only 2 users
(gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
an impossible task.

> > > display controller driver, modules don't seem to care overly much about
> > > the powergate's state except that it needs to be turned on before they
> > > touch some of the registers.
> > > 
> > > From a bit of experimentation it also seems like the sequence encoded
> > > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > > couldn't find an authoritative reference for that either, so I'm tempted
> > > to conclude that it was simply cargo-culted from the dark-ages.
> > > 
> > > So I'm thinking that if we ever move to use power domains for this, we
> > > may be able to just drop any extra handling (well, we'd need to keep it
> > > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > > and reset resources.
> > > 
> > > On the other hand, given that we already need to keep the existing code
> > > for backwards-compatibility, I'm not sure there's a real advantage in
> > > turning them into power domains, since we'd be adding extra code without
> > > an clear gains (especially since it seems like we'd need even more code
> > > to properly handle suspend/resume in drivers that need powergates).
> > > 
> > 
> > Unless we fix the framework to require context save/restore for suspend.
> > There is a good reason to do that. context save/restore requires energy
> > as well, so it's not a given that turning off domains in system suspend
> > will save power.
> 
> I'm not sure I follow. "require context save/restore for suspend" is
> what many drivers currently don't support, hence we can't use the
> generic PM domains. Perhaps what you're saying is that the PM domain
> core code should be enhanced so that domains can be marked so that they
> stay on during a suspend/resume cycle.
> 

Exactly.

Cheers,

Peter.