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

On 01/07/2015 06:19 PM, Peter De Schrijver wrote:
> On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote:
>> * PGP 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.
>>
>> Also adding Peter whom I had discussed this with earlier. Can we
finally
>> get this converted? I'd rather not keep complicating this custom
API to
>> avoid making the conversion even more difficult.
> Conceptually I fully agree that we should use runtime PM and powerdomains.
> However I don't think the implementation you mentioned is correct. The
resets
> of all modules in a domain need to be asserted and the memory clients need
to
> be flushed. All this needs to be done with module clocks enabled (resets
are
> synchronous).  Then all module clocks need to be disabled and then the
> partition can be powergated. After ungating, the module resets need to be
> deasserted and the FLUSH bit cleared with clocks enabled.
Yeah. I plan to have the information of all the clock client of the 
partitions and
the memory clients be defined statically in c source, e.g. pmc-tegra124.c.
All modules can declare which domain they belong to in DT. One domain can
be really power gated only when no module is awake. Note the clock 
clients of
one domain might not equal to the clocks of the module. The reset is not 
either.
So I don't get the clock and reset from module. How do you think?

Thanks,
Vince

On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu
wrote:
> 
> On 01/07/2015 06:19 PM, Peter De Schrijver wrote:
> >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote:
> >>* PGP 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.
> >>
> >>Also adding Peter whom I had discussed this with earlier. Can we
finally
> >>get this converted? I'd rather not keep complicating this
custom API to
> >>avoid making the conversion even more difficult.
> >Conceptually I fully agree that we should use runtime PM and
powerdomains.
> >However I don't think the implementation you mentioned is correct.
The resets
> >of all modules in a domain need to be asserted and the memory clients
need to
> >be flushed. All this needs to be done with module clocks enabled
(resets are
> >synchronous).  Then all module clocks need to be disabled and then the
> >partition can be powergated. After ungating, the module resets need to
be
> >deasserted and the FLUSH bit cleared with clocks enabled.
> Yeah. I plan to have the information of all the clock client of the
> partitions and
> the memory clients be defined statically in c source, e.g. pmc-tegra124.c.
> All modules can declare which domain they belong to in DT. One domain can
> be really power gated only when no module is awake. Note the clock clients
> of
> one domain might not equal to the clocks of the module. The reset is not
> either.
> So I don't get the clock and reset from module. How do you think?
This whole situation is quite messy. The above sequence basically means
that drivers can't reset hardware modules because otherwise they might
race with the power domain code. It also means that we can't powergate
modules on demand because they might be in the same power domain as one
other module that's still busy.

How would we handle a situation where a hardware module hangs and we can
only get it back via a reset?

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/20150107/71de39ea/attachment.sig>

On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote:
> > Yeah. I plan to have the information of all the clock client of the
> > partitions and
> > the memory clients be defined statically in c source, e.g.
pmc-tegra124.c.
> > All modules can declare which domain they belong to in DT. One domain
can
> > be really power gated only when no module is awake. Note the clock
clients
> > of
> > one domain might not equal to the clocks of the module. The reset is
not
> > either.
> > So I don't get the clock and reset from module. How do you think?
> 
> This whole situation is quite messy. The above sequence basically means
> that drivers can't reset hardware modules because otherwise they might
> race with the power domain code. It also means that we can't powergate
The powerdomain framework won't call any powergating method as long as a
module in the domain is still active. So as long as drivers don't try to
reset the hw without having done a pm_runtime_get(), we shouldn't have such
a race?
> modules on demand because they might be in the same power domain as one
> other module that's still busy.
> 
The powerdomain framework keeps track of which modules are active (by hooking
into runtime pm) and won't try to shutdown a domain unless all modules are
inactive.
> How would we handle a situation where a hardware module hangs and we can
> only get it back via a reset?
> 
Cheers,

Peter.

On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu
wrote:
> 
> On 01/07/2015 06:19 PM, Peter De Schrijver wrote:
> >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote:
> >>* PGP 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.
> >>
> >>Also adding Peter whom I had discussed this with earlier. Can we
finally
> >>get this converted? I'd rather not keep complicating this
custom API to
> >>avoid making the conversion even more difficult.
> >Conceptually I fully agree that we should use runtime PM and
powerdomains.
> >However I don't think the implementation you mentioned is correct.
The resets
> >of all modules in a domain need to be asserted and the memory clients
need to
> >be flushed. All this needs to be done with module clocks enabled
(resets are
> >synchronous).  Then all module clocks need to be disabled and then the
> >partition can be powergated. After ungating, the module resets need to
be
> >deasserted and the FLUSH bit cleared with clocks enabled.
> Yeah. I plan to have the information of all the clock client of the
> partitions and
> the memory clients be defined statically in c source, e.g. pmc-tegra124.c.
> All modules can declare which domain they belong to in DT. One domain can
> be really power gated only when no module is awake. Note the clock
> clients of
> one domain might not equal to the clocks of the module. The reset is
> not either.
> So I don't get the clock and reset from module. How do you think?
> 
I think it's indeed better to have a direct reference to the required clocks
to powergate/ungate a domain. As you said, there is no easy way to derive the
required clocks from the DT module declarations. My suggestion would be to
have powerdomain definitions in DT and for each domain have references to
the required clocks and resets.

Cheers,

Peter.

On 04:12:54PM Jan 07, Peter De Schrijver wrote:
> On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote:
> > 
> > On 01/07/2015 06:19 PM, Peter De Schrijver wrote:
> > >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote:
> > >>* PGP 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.
> > >>
> > >>Also adding Peter whom I had discussed this with earlier. Can
we finally
> > >>get this converted? I'd rather not keep complicating this
custom API to
> > >>avoid making the conversion even more difficult.
> > >Conceptually I fully agree that we should use runtime PM and
powerdomains.
> > >However I don't think the implementation you mentioned is
correct. The resets
> > >of all modules in a domain need to be asserted and the memory
clients need to
> > >be flushed. All this needs to be done with module clocks enabled
(resets are
> > >synchronous).  Then all module clocks need to be disabled and then
the
> > >partition can be powergated. After ungating, the module resets
need to be
> > >deasserted and the FLUSH bit cleared with clocks enabled.
> > Yeah. I plan to have the information of all the clock client of the
> > partitions and
> > the memory clients be defined statically in c source, e.g.
pmc-tegra124.c.
> > All modules can declare which domain they belong to in DT. One domain
can
> > be really power gated only when no module is awake. Note the clock
> > clients of
> > one domain might not equal to the clocks of the module. The reset is
> > not either.
> > So I don't get the clock and reset from module. How do you think?
> > 
> 
> I think it's indeed better to have a direct reference to the required
clocks
> to powergate/ungate a domain. As you said, there is no easy way to derive
the
> required clocks from the DT module declarations. My suggestion would be to
> have powerdomain definitions in DT and for each domain have references to
> the required clocks and resets.
> 
And specify the dependencies between domains in DT?

Thanks,
Vince