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

On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu
wrote:
> On 01/06/2015 07:15 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote:
> >>On 01/05/2015 11:09 PM, Thierry Reding wrote:
> >>>>Old Signed by an unknown key
> >>>On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu wrote:
> >>>>On 12/24/2014 09:16 PM, Lucas Stach wrote:
> >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince
Hsu:
> >>>>>>The Tegra124 and later Tegra SoCs have a sepatate
rail gating register
> >>>>>>to enable/disable the clamp. The original function
> >>>>>>tegra_powergate_remove_clamping() is not sufficient
for the enable
> >>>>>>function. So add a new function which is dedicated
to the GPU rail
> >>>>>>gating. Also don't refer to the powergate ID
since the GPU ID makes no
> >>>>>>sense here.
> >>>>>>
> >>>>>>Signed-off-by: Vince Hsu <vinceh at
nvidia.com>
> >>>>>To be honest I don't see the point of this patch.
> >>>>>You are bloating the PMC interface by introducing
another exported
> >>>>>function that does nothing different than what the
current function
> >>>>>already does.
> >>>>>
> >>>>>If you need a way to assert the clamp I would have
expected you to
> >>>>>introduce a common function to do this for all power
partitions.
> >>>>I thought about adding an
tegra_powergate_assert_clamping(), but that
> >>>>doesn't make sense to all the power partitions except
GPU. Note the
> >>>>difference in TRM. Any suggestion for the common function?
> >>>I don't think extending the powergate API is useful at this
point. We've
> >>>long had an open TODO item to replace this with a generic API.
I did
> >>>some prototyping a while ago to use generic power domains for
this, that
> >>>way all the details and dependencies between the partitions
could be
> >>>properly modeled.
> >>>
> >>>Can you take a look at my staging/powergate branch here:
> >>>
> >>>
https://github.com/thierryreding/linux/commits/staging/powergate
> >>>
> >>>and see if you can use that instead? The idea is to completely
hide the
> >>>details of power partitions from drivers and use runtime PM
instead.
> >>You generic power domains work is exactly what we want for
powergate
> >>eventually. :) I recall we used your prototyping in somewhere
internal tree.
> >>We have to add more to complete it though, e.g. power domain
dependency, mc
> >>flush, and clamping register difference like this patch does.
> >>
> >>But I have a question here. Since the GK20A is not powered on/off
by the PMC
> >>except the clamping control, and GK20A has its own power rail, do
we really
> >>want to hide the power sequence in the generic powergate code? We
still have
> >>to control the voltage level in the GK20A driver through the
regulator
> >>framework. It seems weird to me if we put the
regulator_{enable|disable}
> >>somewhere other than the GK20A driver.
> >I think we want both. The power domain to control the power partition
> >and the regulator in the gk20a driver for the voltage control.
> Do you mean excluding the power sequence of GK20A from the generic power
> domain?
No, what I mean is move the power gating into the PMC driver as part of
the generic power domain implementation. At the same time, keep the
control of the regulator within the gk20a driver. That way we can use
runtime PM to control the powergating but we can still control the GPU
voltage within Nouveau.

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/20150106/b1a97ab2/attachment.sig>

On 02:29:32PM Jan 06, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote:
> > On 01/06/2015 07:15 PM, Thierry Reding wrote:
> > >> Old Signed by an unknown key
> > >
> > >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote:
> > >>On 01/05/2015 11:09 PM, Thierry Reding wrote:
> > >>>>Old Signed by an unknown key
> > >>>On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu wrote:
> > >>>>On 12/24/2014 09:16 PM, Lucas Stach wrote:
> > >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb
Vince Hsu:
> > >>>>>>The Tegra124 and later Tegra SoCs have a
sepatate rail gating register
> > >>>>>>to enable/disable the clamp. The original
function
> > >>>>>>tegra_powergate_remove_clamping() is not
sufficient for the enable
> > >>>>>>function. So add a new function which is
dedicated to the GPU rail
> > >>>>>>gating. Also don't refer to the powergate
ID since the GPU ID makes no
> > >>>>>>sense here.
> > >>>>>>
> > >>>>>>Signed-off-by: Vince Hsu <vinceh at
nvidia.com>
> > >>>>>To be honest I don't see the point of this
patch.
> > >>>>>You are bloating the PMC interface by introducing
another exported
> > >>>>>function that does nothing different than what the
current function
> > >>>>>already does.
> > >>>>>
> > >>>>>If you need a way to assert the clamp I would have
expected you to
> > >>>>>introduce a common function to do this for all
power partitions.
> > >>>>I thought about adding an
tegra_powergate_assert_clamping(), but that
> > >>>>doesn't make sense to all the power partitions
except GPU. Note the
> > >>>>difference in TRM. Any suggestion for the common
function?
> > >>>I don't think extending the powergate API is useful at
this point. We've
> > >>>long had an open TODO item to replace this with a generic
API. I did
> > >>>some prototyping a while ago to use generic power domains
for this, that
> > >>>way all the details and dependencies between the
partitions could be
> > >>>properly modeled.
> > >>>
> > >>>Can you take a look at my staging/powergate branch here:
> > >>>
> > >>>
https://github.com/thierryreding/linux/commits/staging/powergate
> > >>>
> > >>>and see if you can use that instead? The idea is to
completely hide the
> > >>>details of power partitions from drivers and use runtime
PM instead.
> > >>You generic power domains work is exactly what we want for
powergate
> > >>eventually. :) I recall we used your prototyping in somewhere
internal tree.
> > >>We have to add more to complete it though, e.g. power domain
dependency, mc
> > >>flush, and clamping register difference like this patch does.
> > >>
> > >>But I have a question here. Since the GK20A is not powered
on/off by the PMC
> > >>except the clamping control, and GK20A has its own power rail,
do we really
> > >>want to hide the power sequence in the generic powergate code?
We still have
> > >>to control the voltage level in the GK20A driver through the
regulator
> > >>framework. It seems weird to me if we put the
regulator_{enable|disable}
> > >>somewhere other than the GK20A driver.
> > >I think we want both. The power domain to control the power
partition
> > >and the regulator in the gk20a driver for the voltage control.
> > Do you mean excluding the power sequence of GK20A from the generic
power
> > domain?
> 
> No, what I mean is move the power gating into the PMC driver as part of
> the generic power domain implementation. At the same time, keep the
> control of the regulator within the gk20a driver. That way we can use
> runtime PM to control the powergating but we can still control the GPU
> voltage within Nouveau.
Okay. Do you insist to introduce the generic power domain at this moment?
If so, I can try to continue your previous work and rebase this series on
that. That might take some time though.

Thanks,
Vince

On Tue, Jan 06, 2015 at 09:51:11PM +0800, Vince Hsu
wrote:
> On 02:29:32PM Jan 06, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote:
> > > On 01/06/2015 07:15 PM, Thierry Reding wrote:
> > > >> Old Signed by an unknown key
> > > >
> > > >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote:
> > > >>On 01/05/2015 11:09 PM, Thierry Reding wrote:
> > > >>>>Old Signed by an unknown key
> > > >>>On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu
wrote:
> > > >>>>On 12/24/2014 09:16 PM, Lucas Stach wrote:
> > > >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800
schrieb Vince Hsu:
> > > >>>>>>The Tegra124 and later Tegra SoCs have a
sepatate rail gating register
> > > >>>>>>to enable/disable the clamp. The original
function
> > > >>>>>>tegra_powergate_remove_clamping() is not
sufficient for the enable
> > > >>>>>>function. So add a new function which is
dedicated to the GPU rail
> > > >>>>>>gating. Also don't refer to the
powergate ID since the GPU ID makes no
> > > >>>>>>sense here.
> > > >>>>>>
> > > >>>>>>Signed-off-by: Vince Hsu <vinceh at
nvidia.com>
> > > >>>>>To be honest I don't see the point of
this patch.
> > > >>>>>You are bloating the PMC interface by
introducing another exported
> > > >>>>>function that does nothing different than
what the current function
> > > >>>>>already does.
> > > >>>>>
> > > >>>>>If you need a way to assert the clamp I would
have expected you to
> > > >>>>>introduce a common function to do this for
all power partitions.
> > > >>>>I thought about adding an
tegra_powergate_assert_clamping(), but that
> > > >>>>doesn't make sense to all the power
partitions except GPU. Note the
> > > >>>>difference in TRM. Any suggestion for the common
function?
> > > >>>I don't think extending the powergate API is
useful at this point. We've
> > > >>>long had an open TODO item to replace this with a
generic API. I did
> > > >>>some prototyping a while ago to use generic power
domains for this, that
> > > >>>way all the details and dependencies between the
partitions could be
> > > >>>properly modeled.
> > > >>>
> > > >>>Can you take a look at my staging/powergate branch
here:
> > > >>>
> > > >>>
https://github.com/thierryreding/linux/commits/staging/powergate
> > > >>>
> > > >>>and see if you can use that instead? The idea is to
completely hide the
> > > >>>details of power partitions from drivers and use
runtime PM instead.
> > > >>You generic power domains work is exactly what we want
for powergate
> > > >>eventually. :) I recall we used your prototyping in
somewhere internal tree.
> > > >>We have to add more to complete it though, e.g. power
domain dependency, mc
> > > >>flush, and clamping register difference like this patch
does.
> > > >>
> > > >>But I have a question here. Since the GK20A is not
powered on/off by the PMC
> > > >>except the clamping control, and GK20A has its own power
rail, do we really
> > > >>want to hide the power sequence in the generic powergate
code? We still have
> > > >>to control the voltage level in the GK20A driver through
the regulator
> > > >>framework. It seems weird to me if we put the
regulator_{enable|disable}
> > > >>somewhere other than the GK20A driver.
> > > >I think we want both. The power domain to control the power
partition
> > > >and the regulator in the gk20a driver for the voltage
control.
> > > Do you mean excluding the power sequence of GK20A from the
generic power
> > > domain?
> > 
> > No, what I mean is move the power gating into the PMC driver as part
of
> > the generic power domain implementation. At the same time, keep the
> > control of the regulator within the gk20a driver. That way we can use
> > runtime PM to control the powergating but we can still control the GPU
> > voltage within Nouveau.
> Okay. Do you insist to introduce the generic power domain at this moment?
> If so, I can try to continue your previous work and rebase this series on
> that. That might take some time though.
Yes, I'm leaning strongly towards doing that conversion now. The problem
with extending the reset API with flushing is that it will make the
conversion more difficult than it already is. I'm not even sure we can
go through with the conversion without breaking DT ABI stability yet
again. We may be able to do so by keeping the current code as fallback
if we can determine that no power domain is added (dev->pm_domain?). But
adding tegra_mc_flush() calls everywhere now will mean that we have to
keep even more fallback code, basically forever.

I know this is going to take some time, but the longer we defer this the
more work it will become. And we've already been pushing this back for
over a year.

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/20150106/e81113b8/attachment.sig>