Nouveau - Jan 2015 - [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp

> > And specify the dependencies between domains in DT?
> 
> I think the dependencies could be in the driver. Of course the power
> domains are per-SoC data, so really shouldn't be in the DTS either (the
> data is all implied by the compatible value) but there's no good way to
> get at the clocks and resets without DT, so I think that's a reasonable
> trade-off.
> 
> It seems to me like there are only two dependencies:
> 
> 	DIS and DISB depend on SOR
> 	VE depends on DIS
> 
> That's according to 5.6.6 "Programming Guide for Power Gating and
> Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules
are
> part of seemingly unrelated domains. Especially SOR seems to cover a
> large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and
> HDA2HDMI).
> 
> Given that we may want to more fine-grainedly control clocks to save
> power, don't we need to control clocks and resets within the drivers? I
> think the runtime PM framework makes sure to call this in the right
> order, so for suspend, the sequence would be:
> 
> 	1) device->suspend
> 	2) domain->suspend
> 
> and for resume:
> 
> 	1) domain->resume
> 	2) device->resume
> 
> But then we're back to square one, namely that the MC flush doesn't
work
> properly, since it needs to be implemented in domain->suspend. Does that
> mean we can't clock-gate modules? In order to ensure a proper powergate
> sequence, the domain code would need to clk_enable() the module clock to
> make sure it stays on during the reset sequence. But if the domain code
> has a reference to the clock, then the driver can't clock-gate the
> module anymore by calling clk_disable().
> 
> Am I missing something?
> 
There's a difference between having a reference and actually enabling the
clock. the domain powergate method will only be called when the clocks of
all modules in the domain are off. So the powergate method can then turn them
on again, do the module resets and client flushes and then disable them
again. Same for ungate. So I don't see a problem here?

Cheers,

Peter.

On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver
wrote:
> > > And specify the dependencies between domains in DT?
> > 
> > I think the dependencies could be in the driver. Of course the power
> > domains are per-SoC data, so really shouldn't be in the DTS either
(the
> > data is all implied by the compatible value) but there's no good
way to
> > get at the clocks and resets without DT, so I think that's a
reasonable
> > trade-off.
> > 
> > It seems to me like there are only two dependencies:
> > 
> > 	DIS and DISB depend on SOR
> > 	VE depends on DIS
> > 
> > That's according to 5.6.6 "Programming Guide for Power Gating
and
> > Ungating" of the Tegra K1 TRM. It also seems like a bunch of
modules are
> > part of seemingly unrelated domains. Especially SOR seems to cover a
> > large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and
> > HDA2HDMI).
> > 
> > Given that we may want to more fine-grainedly control clocks to save
> > power, don't we need to control clocks and resets within the
drivers? I
> > think the runtime PM framework makes sure to call this in the right
> > order, so for suspend, the sequence would be:
> > 
> > 	1) device->suspend
> > 	2) domain->suspend
> > 
> > and for resume:
> > 
> > 	1) domain->resume
> > 	2) device->resume
> > 
> > But then we're back to square one, namely that the MC flush
doesn't work
> > properly, since it needs to be implemented in domain->suspend. Does
that
> > mean we can't clock-gate modules? In order to ensure a proper
powergate
> > sequence, the domain code would need to clk_enable() the module clock
to
> > make sure it stays on during the reset sequence. But if the domain
code
> > has a reference to the clock, then the driver can't clock-gate the
> > module anymore by calling clk_disable().
> > 
> > Am I missing something?
> > 
> 
> There's a difference between having a reference and actually enabling
the
> clock.
My point was that as long as anyone was keeping a reference the clock
couldn't in fact be turned off.
> the domain powergate method will only be called when the clocks of
> all modules in the domain are off.
No, the power domain will be disabled when all devices in the domain are
idle.
> So the powergate method can then turn them on again, do the module
> resets and client flushes and then disable them again. Same for
> ungate. So I don't see a problem here?
I think that could work, but we'd need to make sure that drivers that
use runtime PM and are connected to a power domain enable clocks only
after taking a runtime PM reference and disable the clocks before they
release that reference.

So to simplify things, maybe all clock handling for drivers should be
moved into runtime PM operations, and whenever the driver needs the
clock enabled it takes a runtime PM reference. That would ensure that
clocks aren't accidentally left on.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20150108/35dacd1d/attachment.sig>

On Thu, Jan 08, 2015 at 12:41:54PM +0100, Thierry Reding
wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver wrote:
> > > > And specify the dependencies between domains in DT?
> > > 
> > > I think the dependencies could be in the driver. Of course the
power
> > > domains are per-SoC data, so really shouldn't be in the DTS
either (the
> > > data is all implied by the compatible value) but there's no
good way to
> > > get at the clocks and resets without DT, so I think that's a
reasonable
> > > trade-off.
> > > 
> > > It seems to me like there are only two dependencies:
> > > 
> > > 	DIS and DISB depend on SOR
> > > 	VE depends on DIS
> > > 
> > > That's according to 5.6.6 "Programming Guide for Power
Gating and
> > > Ungating" of the Tegra K1 TRM. It also seems like a bunch of
modules are
> > > part of seemingly unrelated domains. Especially SOR seems to
cover a
> > > large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and
> > > HDA2HDMI).
> > > 
> > > Given that we may want to more fine-grainedly control clocks to
save
> > > power, don't we need to control clocks and resets within the
drivers? I
> > > think the runtime PM framework makes sure to call this in the
right
> > > order, so for suspend, the sequence would be:
> > > 
> > > 	1) device->suspend
> > > 	2) domain->suspend
> > > 
> > > and for resume:
> > > 
> > > 	1) domain->resume
> > > 	2) device->resume
> > > 
> > > But then we're back to square one, namely that the MC flush
doesn't work
> > > properly, since it needs to be implemented in domain->suspend.
Does that
> > > mean we can't clock-gate modules? In order to ensure a proper
powergate
> > > sequence, the domain code would need to clk_enable() the module
clock to
> > > make sure it stays on during the reset sequence. But if the
domain code
> > > has a reference to the clock, then the driver can't
clock-gate the
> > > module anymore by calling clk_disable().
> > > 
> > > Am I missing something?
> > > 
> > 
> > There's a difference between having a reference and actually
enabling the
> > clock.
> 
> My point was that as long as anyone was keeping a reference the clock
> couldn't in fact be turned off.
> 
> > the domain powergate method will only be called when the clocks of
> > all modules in the domain are off.
> 
> No, the power domain will be disabled when all devices in the domain are
> idle.
> 
> > So the powergate method can then turn them on again, do the module
> > resets and client flushes and then disable them again. Same for
> > ungate. So I don't see a problem here?
> 
> I think that could work, but we'd need to make sure that drivers that
> use runtime PM and are connected to a power domain enable clocks only
> after taking a runtime PM reference and disable the clocks before they
> release that reference.
> 
> So to simplify things, maybe all clock handling for drivers should be
> moved into runtime PM operations, and whenever the driver needs the
> clock enabled it takes a runtime PM reference. That would ensure that
> clocks aren't accidentally left on.
Yes. That's exactly what needs to happen (apart from some special cases
where
the driver also controls some clocks for the external signals like display.
those clocks most likely still need to be handled by the drivers).

Cheers,

Peter.