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

2015-01-08 Thread Thierry Reding
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


pgpBj0qOgH7Pw.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-08 Thread Thierry Reding
On Thu, Jan 08, 2015 at 11:39:57AM +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
 
 The clock references could also be retrieved via clk_get_sys(). We could add
 some more clkdev entries. If we use the domain name as the dev_id and the
 module names as the con_id's, the domain code could then retrieve the
 clocks by iterating over the module names and performing a
 clk_get_sys(domain_name, module_name) for each module. Unfortunately no such
 mechanism exists for resets.

I don't think having both clock and reset references in the device tree
is all that bad. We could possibly add a lookup mechanism for reset
controls that doesn't rely on DT, but I'm not sure it's really worth it.

Thierry


pgpwiUY2A0ir4.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-08 Thread Peter De Schrijver
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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-08 Thread Thierry Reding
On Thu, Jan 08, 2015 at 12:25:18PM +0800, Vince Hsu wrote:
 
 On 01/07/2015 10:48 PM, Thierry Reding wrote:
 * PGP Signed by an unknown key
 
 On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote:
 On 04:08:52PM Jan 07, Peter De Schrijver wrote:
 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?
 Agree. And as long as the driver has the correct reset procedure, that 
 should
 be fine to occur between power ungating and gating sequences.
 
 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.
 Yeah. By the way, that means we should start supporting runtime pm for all
 the modules to use generic power domain.
 Indeed, that'll be a prerequisite before we can merge power domain
 support. I do have a couple of local patches that add very rudimentary
 runtime PM for various drivers. For starters we could probably just do
 the
 
  pm_runtime_enable(...);
  pm_runtime_get_sync(...)
 
 in the -probe() and
 
  pm_runtime_put_sync(...);
  pm_runtime_disable(...);
 
 in the -remove() callbacks for those drivers. That's by no means
 optimal but should get us pretty close to what we do now and still
 support the generic power domains.
 Cool. Could you send me the patches?

Here are two examples:


https://github.com/thierryreding/linux/commit/36b5c34f68edb8135b9afb3e62c7ce9a527d6793

https://github.com/thierryreding/linux/commit/6a6145d9e0fcbd4f9599552181fc02f4606b6a0e

Thierry


pgphvHfnajbBi.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-08 Thread Peter De Schrijver
  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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-08 Thread Peter De Schrijver
  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

The clock references could also be retrieved via clk_get_sys(). We could add
some more clkdev entries. If we use the domain name as the dev_id and the
module names as the con_id's, the domain code could then retrieve the
clocks by iterating over the module names and performing a
clk_get_sys(domain_name, module_name) for each module. Unfortunately no such
mechanism exists for resets.

Cheers,

Peter.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Peter De Schrijver
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 vin...@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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Vince Hsu


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 vin...@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





___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Vince Hsu


On 01/07/2015 10:48 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote:

On 04:08:52PM Jan 07, Peter De Schrijver wrote:

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?

Agree. And as long as the driver has the correct reset procedure, that should
be fine to occur between power ungating and gating sequences.


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.

Yeah. By the way, that means we should start supporting runtime pm for all
the modules to use generic power domain.

Indeed, that'll be a prerequisite before we can merge power domain
support. I do have a couple of local patches that add very rudimentary
runtime PM for various drivers. For starters we could probably just do
the

pm_runtime_enable(...);
pm_runtime_get_sync(...)

in the -probe() and

pm_runtime_put_sync(...);
pm_runtime_disable(...);

in the -remove() callbacks for those drivers. That's by no means
optimal but should get us pretty close to what we do now and still
support the generic power domains.

Cool. Could you send me the patches?

Thanks,
Vince



Thierry

* Unknown Key
* 0x7F3EB3A1


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Vince Hsu


On 01/07/2015 11:12 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote:

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:

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 vin...@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?

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:

We need to control clocks and resets within the drivers. I believe the
powergate sequence is just to provide a clean hardware state. The
driver can do whatever it wants to the clocks and reset as long as
that's correct procedure.



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

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

2015-01-07 Thread Thierry Reding
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 vin...@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


pgpr5X6vjZd81.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Peter De Schrijver
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 vin...@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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Vince Hsu
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 vin...@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

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Thierry Reding
On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote:
 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 vin...@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?

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 

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

2015-01-07 Thread Peter De Schrijver
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.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Vince Hsu
On 04:08:52PM Jan 07, Peter De Schrijver wrote:
 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?
Agree. And as long as the driver has the correct reset procedure, that should
be fine to occur between power ungating and gating sequences.

 
  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.
Yeah. By the way, that means we should start supporting runtime pm for all
the modules to use generic power domain.

Thanks,
Vince

 
  How would we handle a situation where a hardware module hangs and we can
  only get it back via a reset?
  
 
 Cheers,
 
 Peter.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-07 Thread Thierry Reding
On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote:
 On 04:08:52PM Jan 07, Peter De Schrijver wrote:
  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?
 Agree. And as long as the driver has the correct reset procedure, that should
 be fine to occur between power ungating and gating sequences.
 
  
   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.
 Yeah. By the way, that means we should start supporting runtime pm for all
 the modules to use generic power domain.

Indeed, that'll be a prerequisite before we can merge power domain
support. I do have a couple of local patches that add very rudimentary
runtime PM for various drivers. For starters we could probably just do
the

pm_runtime_enable(...);
pm_runtime_get_sync(...)

in the -probe() and

pm_runtime_put_sync(...);
pm_runtime_disable(...);

in the -remove() callbacks for those drivers. That's by no means
optimal but should get us pretty close to what we do now and still
support the generic power domains.

Thierry


pgp86Hhf0rdD_.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-06 Thread Thierry Reding
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 vin...@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


pgptVtgYyWYSS.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-06 Thread Thierry Reding
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 vin...@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


pgpSJ_bHxeaDK.pgp
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-06 Thread Vince Hsu

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 vin...@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?


Thanks,
Vince

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-06 Thread Vince Hsu
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 vin...@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

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-05 Thread Vince Hsu


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 vin...@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


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2015-01-04 Thread Vince Hsu


On 12/31/2014 12:42 AM, Lucas Stach wrote:

Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu:

[...]


That's a read fence to assure the post of the previous writes through
Tegra interconnect. (copy-paster from
https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)

I see what it does, the question is more about why this is needed.
What is the Tegra interconnect? According to the TRM the Tegra contains
some standard AXI - AHB - APB bridges. That a read is needed to
assure the write is posted to the APB bus seems to imply that there is
some write buffering in one of those bridges. Can we get this documented
somewhere?

The TRM does mention a read after the write. Check the section 32.2.2.3.


Unfortunately this doesn't seem to be included in the public TRM. It
would be nice if this could be documented either in the next version of
the TRM or as a public Appnote.


It should be in the latest public TRM. Could you check again?

Thanks,
Vince

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-30 Thread Lucas Stach
Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu:

[...]

  That's a read fence to assure the post of the previous writes through
  Tegra interconnect. (copy-paster from
  https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)
  I see what it does, the question is more about why this is needed.
  What is the Tegra interconnect? According to the TRM the Tegra contains
  some standard AXI - AHB - APB bridges. That a read is needed to
  assure the write is posted to the APB bus seems to imply that there is
  some write buffering in one of those bridges. Can we get this documented
  somewhere?
 The TRM does mention a read after the write. Check the section 32.2.2.3.
 
Unfortunately this doesn't seem to be included in the public TRM. It
would be nice if this could be documented either in the next version of
the TRM or as a public Appnote.

Thanks,
Lucas


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-28 Thread Vince Hsu


On 12/26/2014 04:34 AM, Lucas Stach wrote:

Am Donnerstag, den 25.12.2014, 10:28 +0800 schrieb Vince Hsu:

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 vin...@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?

Is there anything speaking against adding this function and only accept
the GPU partition as valid parameter for now. So at least the interface
stays symmetric and can be easily extended if any future partitions have
similar characteristics as the GPU one.
The register APBDEV_PMC_GPU_RG_CNTRL_0 is only for GPU and can be used 
for assertion and deassertion. The APBDEV_PMC_REMOVE_CLAMPING_CMD_0 is 
only used for deassertion. If we have any future partitions that can be 
asserted by SW like GPU, we can improve the interface then.





Other comments inline.

Regards,
Lucas


---
   drivers/soc/tegra/pmc.c | 34 +++---
   include/soc/tegra/pmc.h |  2 ++
   2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index a2c0ceb95f8f..7798c530ead1 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id)
return -EINVAL;
   
   	/*

-* The Tegra124 GPU has a separate register (with different semantics)
-* to remove clamps.
-*/
-   if (tegra_get_chip_id() == TEGRA124) {
-   if (id == TEGRA_POWERGATE_3D) {
-   tegra_pmc_writel(0, GPU_RG_CNTRL);
-   return 0;
-   }
-   }
-
-   /*
 * Tegra 2 has a bug where PCIE and VDE clamping masks are
 * swapped relatively to the partition ids
 */
@@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id)
   EXPORT_SYMBOL(tegra_powergate_remove_clamping);
   
   /**

+ * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps
+ *
+ * The post-Tegra114 chips have a separate rail gating register to configure
+ * clamps.
+ *
+ * @assert: true to assert clamp, and false to remove
+ */
+int tegra_powergate_gpu_set_clamping(bool assert)

Those functions with a bool parameter to set/unset something are really
annoying. Please avoid this pattern. The naming of the original function
is much more intuitive.

But the original function is not sufficient. Maybe add a
tegra_powergate_assert_gpu_clamping()? That way I will prefer to adding
one more removal function for GPU. And then again that's a bloat, too.

+{
+   if (!pmc-soc)
+   return -EINVAL;
+
+   if (tegra_get_chip_id() == TEGRA124) {
+   tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL);
+   tegra_pmc_readl(GPU_RG_CNTRL);

You are reading the register back here, which to me seems like you are
trying to make sure that the write is flushed. Why is this needed?
I also observed the need to do this while working on Tegra124 PCIe in
Barebox, otherwise the partition wouldn't power up. I didn't have time
to follow up on this yet, so it would be nice if you could explain why
this is needed, or if you don't know ask HW about it.

That's a read fence to assure the post of the previous writes through
Tegra interconnect. (copy-paster from
https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)

I see what it does, the question is more about why this is needed.
What is the Tegra interconnect? According to the TRM the Tegra contains
some standard AXI - AHB - APB bridges. That a read is needed to
assure the write is posted to the APB bus seems to imply that there is
some write buffering in one of those bridges. Can we get this documented
somewhere?

The TRM does mention a read after the write. Check the section 32.2.2.3.

Thanks,
Vince



And isn't it needed for the other partition ungating function too then?

I believe yes.



Regards,
Lucas




___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-25 Thread Lucas Stach
Am Donnerstag, den 25.12.2014, 10:28 +0800 schrieb Vince Hsu:
 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 vin...@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?

Is there anything speaking against adding this function and only accept
the GPU partition as valid parameter for now. So at least the interface
stays symmetric and can be easily extended if any future partitions have
similar characteristics as the GPU one.

 
  Other comments inline.
 
  Regards,
  Lucas
 
  ---
drivers/soc/tegra/pmc.c | 34 +++---
include/soc/tegra/pmc.h |  2 ++
2 files changed, 25 insertions(+), 11 deletions(-)
 
  diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
  index a2c0ceb95f8f..7798c530ead1 100644
  --- a/drivers/soc/tegra/pmc.c
  +++ b/drivers/soc/tegra/pmc.c
  @@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id)
 return -EINVAL;

 /*
  -   * The Tegra124 GPU has a separate register (with different semantics)
  -   * to remove clamps.
  -   */
  -  if (tegra_get_chip_id() == TEGRA124) {
  -  if (id == TEGRA_POWERGATE_3D) {
  -  tegra_pmc_writel(0, GPU_RG_CNTRL);
  -  return 0;
  -  }
  -  }
  -
  -  /*
  * Tegra 2 has a bug where PCIE and VDE clamping masks are
  * swapped relatively to the partition ids
  */
  @@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id)
EXPORT_SYMBOL(tegra_powergate_remove_clamping);

/**
  + * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps
  + *
  + * The post-Tegra114 chips have a separate rail gating register to 
  configure
  + * clamps.
  + *
  + * @assert: true to assert clamp, and false to remove
  + */
  +int tegra_powergate_gpu_set_clamping(bool assert)
  Those functions with a bool parameter to set/unset something are really
  annoying. Please avoid this pattern. The naming of the original function
  is much more intuitive.
 But the original function is not sufficient. Maybe add a 
 tegra_powergate_assert_gpu_clamping()? That way I will prefer to adding 
 one more removal function for GPU. And then again that's a bloat, too.
 
  +{
  +  if (!pmc-soc)
  +  return -EINVAL;
  +
  +  if (tegra_get_chip_id() == TEGRA124) {
  +  tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL);
  +  tegra_pmc_readl(GPU_RG_CNTRL);
  You are reading the register back here, which to me seems like you are
  trying to make sure that the write is flushed. Why is this needed?
  I also observed the need to do this while working on Tegra124 PCIe in
  Barebox, otherwise the partition wouldn't power up. I didn't have time
  to follow up on this yet, so it would be nice if you could explain why
  this is needed, or if you don't know ask HW about it.
 That's a read fence to assure the post of the previous writes through 
 Tegra interconnect. (copy-paster from 
 https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)

I see what it does, the question is more about why this is needed.
What is the Tegra interconnect? According to the TRM the Tegra contains
some standard AXI - AHB - APB bridges. That a read is needed to
assure the write is posted to the APB bus seems to imply that there is
some write buffering in one of those bridges. Can we get this documented
somewhere?

And isn't it needed for the other partition ungating function too then?

Regards,
Lucas


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-24 Thread Lucas Stach
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 vin...@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.

Other comments inline.

Regards,
Lucas

 ---
  drivers/soc/tegra/pmc.c | 34 +++---
  include/soc/tegra/pmc.h |  2 ++
  2 files changed, 25 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
 index a2c0ceb95f8f..7798c530ead1 100644
 --- a/drivers/soc/tegra/pmc.c
 +++ b/drivers/soc/tegra/pmc.c
 @@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id)
   return -EINVAL;
  
   /*
 -  * The Tegra124 GPU has a separate register (with different semantics)
 -  * to remove clamps.
 -  */
 - if (tegra_get_chip_id() == TEGRA124) {
 - if (id == TEGRA_POWERGATE_3D) {
 - tegra_pmc_writel(0, GPU_RG_CNTRL);
 - return 0;
 - }
 - }
 -
 - /*
* Tegra 2 has a bug where PCIE and VDE clamping masks are
* swapped relatively to the partition ids
*/
 @@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id)
  EXPORT_SYMBOL(tegra_powergate_remove_clamping);
  
  /**
 + * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps
 + *
 + * The post-Tegra114 chips have a separate rail gating register to configure
 + * clamps.
 + *
 + * @assert: true to assert clamp, and false to remove
 + */
 +int tegra_powergate_gpu_set_clamping(bool assert)

Those functions with a bool parameter to set/unset something are really
annoying. Please avoid this pattern. The naming of the original function
is much more intuitive.

 +{
 + if (!pmc-soc)
 + return -EINVAL;
 +
 + if (tegra_get_chip_id() == TEGRA124) {
 + tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL);
 + tegra_pmc_readl(GPU_RG_CNTRL);

You are reading the register back here, which to me seems like you are
trying to make sure that the write is flushed. Why is this needed?
I also observed the need to do this while working on Tegra124 PCIe in
Barebox, otherwise the partition wouldn't power up. I didn't have time
to follow up on this yet, so it would be nice if you could explain why
this is needed, or if you don't know ask HW about it.

 + return 0;
 + }
 +
 + return -EINVAL;
 +}
 +EXPORT_SYMBOL(tegra_powergate_gpu_set_clamping);
 +
 +/**
   * tegra_powergate_sequence_power_up() - power up partition
   * @id: partition ID
   * @clk: clock for partition
 diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
 index 65a93273e72f..53d620525a9e 100644
 --- a/include/soc/tegra/pmc.h
 +++ b/include/soc/tegra/pmc.h
 @@ -109,6 +109,8 @@ int tegra_powergate_is_powered(int id);
  int tegra_powergate_power_on(int id);
  int tegra_powergate_power_off(int id);
  int tegra_powergate_remove_clamping(int id);
 +/* Only for Tegra124 and later */
 +int tegra_powergate_gpu_set_clamping(bool assert);
  
  /* Must be called with clk disabled, and returns with clk enabled */
  int tegra_powergate_sequence_power_up(int id, struct clk *clk,


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-24 Thread Dmitry Osipenko
I think ARM: tegra: is wrong prefix for this patch and soc: tegra: should be
used instead to show that it belongs to SoC driver, not arch code.

-- 
Dmitry
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-24 Thread Vince Hsu

On 12/24/2014 09:52 PM, Dmitry Osipenko wrote:

I think ARM: tegra: is wrong prefix for this patch and soc: tegra: should be
used instead to show that it belongs to SoC driver, not arch code.

Indeed. Will fix in v2.

Thanks,
Vince

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau


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

2014-12-24 Thread Vince Hsu

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 vin...@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?


Other comments inline.

Regards,
Lucas


---
  drivers/soc/tegra/pmc.c | 34 +++---
  include/soc/tegra/pmc.h |  2 ++
  2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index a2c0ceb95f8f..7798c530ead1 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id)
return -EINVAL;
  
  	/*

-* The Tegra124 GPU has a separate register (with different semantics)
-* to remove clamps.
-*/
-   if (tegra_get_chip_id() == TEGRA124) {
-   if (id == TEGRA_POWERGATE_3D) {
-   tegra_pmc_writel(0, GPU_RG_CNTRL);
-   return 0;
-   }
-   }
-
-   /*
 * Tegra 2 has a bug where PCIE and VDE clamping masks are
 * swapped relatively to the partition ids
 */
@@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id)
  EXPORT_SYMBOL(tegra_powergate_remove_clamping);
  
  /**

+ * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps
+ *
+ * The post-Tegra114 chips have a separate rail gating register to configure
+ * clamps.
+ *
+ * @assert: true to assert clamp, and false to remove
+ */
+int tegra_powergate_gpu_set_clamping(bool assert)

Those functions with a bool parameter to set/unset something are really
annoying. Please avoid this pattern. The naming of the original function
is much more intuitive.
But the original function is not sufficient. Maybe add a 
tegra_powergate_assert_gpu_clamping()? That way I will prefer to adding 
one more removal function for GPU. And then again that's a bloat, too.



+{
+   if (!pmc-soc)
+   return -EINVAL;
+
+   if (tegra_get_chip_id() == TEGRA124) {
+   tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL);
+   tegra_pmc_readl(GPU_RG_CNTRL);

You are reading the register back here, which to me seems like you are
trying to make sure that the write is flushed. Why is this needed?
I also observed the need to do this while working on Tegra124 PCIe in
Barebox, otherwise the partition wouldn't power up. I didn't have time
to follow up on this yet, so it would be nice if you could explain why
this is needed, or if you don't know ask HW about it.
That's a read fence to assure the post of the previous writes through 
Tegra interconnect. (copy-paster from 
https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)



+   return 0;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(tegra_powergate_gpu_set_clamping);
+
+/**
   * tegra_powergate_sequence_power_up() - power up partition
   * @id: partition ID
   * @clk: clock for partition
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 65a93273e72f..53d620525a9e 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -109,6 +109,8 @@ int tegra_powergate_is_powered(int id);
  int tegra_powergate_power_on(int id);
  int tegra_powergate_power_off(int id);
  int tegra_powergate_remove_clamping(int id);
+/* Only for Tegra124 and later */
+int tegra_powergate_gpu_set_clamping(bool assert);
  
  /* Must be called with clk disabled, and returns with clk enabled */

  int tegra_powergate_sequence_power_up(int id, struct clk *clk,




___
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau