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

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.

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/20150105/fae76020/attachment.sig>

On 01/05/2015 11:09 PM, 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.
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.

Thanks,
Vince
>
> 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.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1

On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu
wrote:
> 
> On 01/05/2015 11:09 PM, 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.
> 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.

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/9c572300/attachment-0001.sig>

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.

Cheers,

Peter.

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