[PATCH 0/11] Add suspend/resume support for GK20A

[PATCH 0/11] Add suspend/resume support for GK20A

[Vince Hsu]

Hi,

This series includes some pieces of fixes to complete the GK20A power
on/off sequences and add the suspend/resume support.

The patches 1/11 - 4/11 are based on the linux-next-20141219.
The patches 5/11 - 11/11 are based on the branch "linux-3.19" of Ben Skeggs's
tree (http://cgit.freedesktop.org/~darktama/nouveau).

Thanks,
Vince

Vince Hsu (4): (linux-next-20141219)
  ARM: tegra: add function to control the GPU rail clamp
  memory: tegra: add mc flush support
  memory: tegra: add flush operation for Tegra124 memory clients
  ARM: tegra: add mc node for Tegra124 GPU

 arch/arm/boot/dts/tegra124.dtsi |  1 +
 drivers/memory/tegra/mc.c       | 21 +++++++++++
 drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
 drivers/soc/tegra/pmc.c         | 34 +++++++++++------
 include/soc/tegra/mc.h          | 23 +++++++++++-
 include/soc/tegra/pmc.h         |  2 +
 6 files changed, 151 insertions(+), 12 deletions(-)

Vince Hsu (7): (linux-3.19 / http://cgit.freedesktop.org/~darktama/nouveau)
  platform: switch to the new gpu rail clamping function
  platform: complete the power up/down sequence
  instmem: make nv50_instmem_priv public
  instmem: add dummy support for GK20A
  drm: export some variable and functions to resue the PM functions
  platform: add suspend/resume support
  platform: add PM runtime suspend/resume support

 drm/Kbuild                     |   1 +
 drm/core/subdev/instmem/nv50.h |   1 +
 drm/nouveau_drm.c              |  16 ++--
 drm/nouveau_drm.h              |   2 +
 drm/nouveau_platform.c         | 174 ++++++++++++++++++++++++++++++++++++++++-
 drm/nouveau_platform.h         |   3 +
 nvkm/engine/device/nve0.c      |   2 +-
 nvkm/include/subdev/instmem.h  |   1 +
 nvkm/subdev/instmem/gk20a.c    |  70 +++++++++++++++++
 nvkm/subdev/instmem/nv50.c     |   9 +--
 nvkm/subdev/instmem/nv50.h     |  14 ++++
 11 files changed, 279 insertions(+), 14 deletions(-)
 create mode 120000 drm/core/subdev/instmem/nv50.h
 create mode 100644 nvkm/subdev/instmem/gk20a.c
 create mode 100644 nvkm/subdev/instmem/nv50.h

-- 
1.9.1

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

[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@nvidia.com>
---
 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)
+{
+	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);
+		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,
-- 
1.9.1

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

[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 <vinceh@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,

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

[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 <vinceh@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,
>

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

[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 <vinceh@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

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

[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 <vinceh@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
>
>

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

[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

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

[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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]

On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote:
> On 01/06/2015 07:15 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote:
> >>On 01/05/2015 11:09 PM, Thierry Reding wrote:
> >>>>Old Signed by an unknown key
> >>>On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu wrote:
> >>>>On 12/24/2014 09:16 PM, Lucas Stach wrote:
> >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>>>>>The Tegra124 and later Tegra SoCs have a sepatate rail gating register
> >>>>>>to enable/disable the clamp. The original function
> >>>>>>tegra_powergate_remove_clamping() is not sufficient for the enable
> >>>>>>function. So add a new function which is dedicated to the GPU rail
> >>>>>>gating. Also don't refer to the powergate ID since the GPU ID makes no
> >>>>>>sense here.
> >>>>>>
> >>>>>>Signed-off-by: Vince Hsu <vinceh@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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4797 bytes --]

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

Yes, I'm leaning strongly towards doing that conversion now. The problem
with extending the reset API with flushing is that it will make the
conversion more difficult than it already is. I'm not even sure we can
go through with the conversion without breaking DT ABI stability yet
again. We may be able to do so by keeping the current code as fallback
if we can determine that no power domain is added (dev->pm_domain?). But
adding tegra_mc_flush() calls everywhere now will mean that we have to
keep even more fallback code, basically forever.

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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.

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

[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 <vinceh@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]

On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote:
> 
> On 01/07/2015 06:19 PM, Peter De Schrijver wrote:
> >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote:
> >>* PGP Signed by an unknown key
> >>
> >>On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu wrote:
> >>>On 12/24/2014 09:16 PM, Lucas Stach wrote:
> >>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>>>>The Tegra124 and later Tegra SoCs have a sepatate rail gating register
> >>>>>to enable/disable the clamp. The original function
> >>>>>tegra_powergate_remove_clamping() is not sufficient for the enable
> >>>>>function. So add a new function which is dedicated to the GPU rail
> >>>>>gating. Also don't refer to the powergate ID since the GPU ID makes no
> >>>>>sense here.
> >>>>>
> >>>>>Signed-off-by: Vince Hsu <vinceh@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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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.

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

[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.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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.

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

[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 <vinceh@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5688 bytes --]

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 <vinceh@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 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?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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 <vinceh@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
> 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().
The module can definitely flush its memory client when the driver wants
to reset the module.

e.g.
The VENC domain needs to flush swgroup ISP, ISPB, VI for domain gating.
The ISP module only needs to flush swgroup ISP when reset.

That means we have to define "nvidia,swgroup = <&pmc ISP>" for both
of the domain VENC and module ISP.

Besides that last step in the un-powergating sequence is disabling all
the module clocks. The module driver has to enable it's module clock
later if need be. So there is no clock reference problem.

Thanks,
Vince

>
> Am I missing something?
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1

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

[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.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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.

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

[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.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[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

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

[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

[PATCH 2/11] memory: tegra: add mc flush support

[Vince Hsu]

The flush operation of memory clients is needed for various IP blocks in
the Tegra SoCs to perform a clean reset.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drivers/memory/tegra/mc.c | 21 +++++++++++++++++++++
 include/soc/tegra/mc.h    | 23 ++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index fe3c44e7e1d1..a2928b4b26fe 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -62,6 +62,27 @@ static const struct of_device_id tegra_mc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
 
+int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable)
+{
+	int i;
+	const struct tegra_mc_hr *client;
+
+	if (!mc || !mc->soc->hr_clients ||
+			!mc->soc->ops || !mc->soc->ops->flush)
+		return -EINVAL;;
+
+	client = mc->soc->hr_clients;
+
+	for (i = 0; i < mc->soc->num_hr_clients; i++, client++) {
+		if (swgroup == client->swgroup) {
+			return mc->soc->ops->flush(mc, client, enable);
+		}
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(tegra_mc_flush);
+
 static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
 {
 	unsigned long long tick;
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 63deb8d9f82a..4894aec7d2a0 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -39,6 +39,21 @@ struct tegra_mc_client {
 	struct tegra_mc_la la;
 };
 
+/* hot reset */
+struct tegra_mc_hr {
+	unsigned int swgroup;
+	unsigned int ctrl;
+	unsigned int status;
+	unsigned int bit;
+};
+
+struct tegra_mc;
+
+struct tegra_mc_ops {
+	int (*flush)(struct tegra_mc *mc, const struct tegra_mc_hr *hr_client,
+				bool enable);
+};
+
 struct tegra_smmu_swgroup {
 	unsigned int swgroup;
 	unsigned int reg;
@@ -64,7 +79,6 @@ struct tegra_smmu_soc {
 	const struct tegra_smmu_ops *ops;
 };
 
-struct tegra_mc;
 struct tegra_smmu;
 
 #ifdef CONFIG_TEGRA_IOMMU_SMMU
@@ -84,6 +98,11 @@ struct tegra_mc_soc {
 	const struct tegra_mc_client *clients;
 	unsigned int num_clients;
 
+	const struct tegra_mc_hr *hr_clients;
+	unsigned int num_hr_clients;
+
+	const struct tegra_mc_ops *ops;
+
 	const unsigned int *emem_regs;
 	unsigned int num_emem_regs;
 
@@ -104,4 +123,6 @@ struct tegra_mc {
 	unsigned long tick;
 };
 
+int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable);
+
 #endif /* __SOC_TEGRA_MC_H__ */
-- 
1.9.1

Re: [PATCH 2/11] memory: tegra: add mc flush support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

On Tue, Dec 23, 2014 at 06:39:55PM +0800, Vince Hsu wrote:
> The flush operation of memory clients is needed for various IP blocks in
> the Tegra SoCs to perform a clean reset.
> 
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drivers/memory/tegra/mc.c | 21 +++++++++++++++++++++
>  include/soc/tegra/mc.h    | 23 ++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index fe3c44e7e1d1..a2928b4b26fe 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -62,6 +62,27 @@ static const struct of_device_id tegra_mc_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>  
> +int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable)
> +{
> +	int i;
> +	const struct tegra_mc_hr *client;
> +
> +	if (!mc || !mc->soc->hr_clients ||
> +			!mc->soc->ops || !mc->soc->ops->flush)
> +		return -EINVAL;;
> +
> +	client = mc->soc->hr_clients;
> +
> +	for (i = 0; i < mc->soc->num_hr_clients; i++, client++) {
> +		if (swgroup == client->swgroup) {
> +			return mc->soc->ops->flush(mc, client, enable);
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(tegra_mc_flush);

Like Lucas already mentioned in response to another patch, having a
boolean "enable" argument is suboptimal here. Now according to
documentation the proper reset sequence for clients is something like
this:

	1) set the FLUSH_ENABLE bit for the client
	2) poll the FLUSH_DONE bit for the client
	3) assert reset to the client using the CAR
	4) deassert reset to the client using the CAR
	5) clear the FLUSH_ENABLE bit for the client

This is really inconvenient because we can't flush the client using a
single operation. So I think we'll need two functions here, something
like: tegra_mc_flush_enable/disable(), or tegra_mc_flush_{,de}assert().
Or maybe even: tegra_mc_reset_{,de}assert() to mirror the reset
controller API. I suppose we could even export it using the reset
controller framework.

Doing so would allow us to have power domain DT nodes like this:

	pmc@0,7000e400 {
		power-domains {
			...

			gpu {
				resets = <&tegra_car 184>,
					 <&mc TEGRA_SWGROUP_GPU>;
				reset-names = "module", "client";
			};

			...
		};
	};

The PMC driver could then grab the "module" and "client" resets and do
something like this:

	reset_control_assert(powergate->rst_client);
	reset_control_assert(powergate->rst_module);
	reset_control_deassert(powergate->rst_module);
	reset_control_deassert(powergate->rst_client);

Optionally the above could be extended with a reset_control_status()-
loop. Alternatively reset_control_assert() would block until the
FLUSH_DONE bit is set.

Stephen, Peter, any thoughts on the above?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/11] memory: tegra: add mc flush support

[Peter De Schrijver]

On Tue, Jan 06, 2015 at 03:18:22PM +0100, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Dec 23, 2014 at 06:39:55PM +0800, Vince Hsu wrote:
> > The flush operation of memory clients is needed for various IP blocks in
> > the Tegra SoCs to perform a clean reset.
> > 
> > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > ---
> >  drivers/memory/tegra/mc.c | 21 +++++++++++++++++++++
> >  include/soc/tegra/mc.h    | 23 ++++++++++++++++++++++-
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > index fe3c44e7e1d1..a2928b4b26fe 100644
> > --- a/drivers/memory/tegra/mc.c
> > +++ b/drivers/memory/tegra/mc.c
> > @@ -62,6 +62,27 @@ static const struct of_device_id tegra_mc_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
> >  
> > +int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable)
> > +{
> > +	int i;
> > +	const struct tegra_mc_hr *client;
> > +
> > +	if (!mc || !mc->soc->hr_clients ||
> > +			!mc->soc->ops || !mc->soc->ops->flush)
> > +		return -EINVAL;;
> > +
> > +	client = mc->soc->hr_clients;
> > +
> > +	for (i = 0; i < mc->soc->num_hr_clients; i++, client++) {
> > +		if (swgroup == client->swgroup) {
> > +			return mc->soc->ops->flush(mc, client, enable);
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(tegra_mc_flush);
> 
> Like Lucas already mentioned in response to another patch, having a
> boolean "enable" argument is suboptimal here. Now according to
> documentation the proper reset sequence for clients is something like
> this:
> 
> 	1) set the FLUSH_ENABLE bit for the client
> 	2) poll the FLUSH_DONE bit for the client
> 	3) assert reset to the client using the CAR
> 	4) deassert reset to the client using the CAR
> 	5) clear the FLUSH_ENABLE bit for the client
> 

Do we ever need to do this outside a powergating or railgating sequence?

> This is really inconvenient because we can't flush the client using a
> single operation. So I think we'll need two functions here, something
> like: tegra_mc_flush_enable/disable(), or tegra_mc_flush_{,de}assert().
> Or maybe even: tegra_mc_reset_{,de}assert() to mirror the reset
> controller API. I suppose we could even export it using the reset
> controller framework.
> 
> Doing so would allow us to have power domain DT nodes like this:
> 
> 	pmc@0,7000e400 {
> 		power-domains {
> 			...
> 
> 			gpu {
> 				resets = <&tegra_car 184>,
> 					 <&mc TEGRA_SWGROUP_GPU>;
> 				reset-names = "module", "client";
> 			};
> 
> 			...
> 		};
> 	};
> 
> The PMC driver could then grab the "module" and "client" resets and do
> something like this:
> 
> 	reset_control_assert(powergate->rst_client);
> 	reset_control_assert(powergate->rst_module);
> 	reset_control_deassert(powergate->rst_module);
> 	reset_control_deassert(powergate->rst_client);
> 
> Optionally the above could be extended with a reset_control_status()-
> loop. Alternatively reset_control_assert() would block until the
> FLUSH_DONE bit is set.
> 

I think the reset_control_assert should wait for the FLUSH_DONE bit to be set
because only then all outstanding memory transactions for the client are
completed so you can't realistically claim the reset has been asserted before
the bit is set. Then you could also expose a single reset which handles both
the memory client and the module reset in CAR? Ie:

reset_control_assert(powergate->rst_module); would set FLUSH_ENABLE for the
memory client, wait for the FLUSH_DONE bit and then assert the CAR reset.
reset_control_deassert(powergate->rst_module); would deassert the CAR reset
and then clear the FLUSH_ENABLE bit. Or is there a usecase to control them
individually?

Cheers,

Peter.

Re: [PATCH 2/11] memory: tegra: add mc flush support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5040 bytes --]

On Wed, Jan 07, 2015 at 12:08:05PM +0200, Peter De Schrijver wrote:
> On Tue, Jan 06, 2015 at 03:18:22PM +0100, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Dec 23, 2014 at 06:39:55PM +0800, Vince Hsu wrote:
> > > The flush operation of memory clients is needed for various IP blocks in
> > > the Tegra SoCs to perform a clean reset.
> > > 
> > > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > > ---
> > >  drivers/memory/tegra/mc.c | 21 +++++++++++++++++++++
> > >  include/soc/tegra/mc.h    | 23 ++++++++++++++++++++++-
> > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > > index fe3c44e7e1d1..a2928b4b26fe 100644
> > > --- a/drivers/memory/tegra/mc.c
> > > +++ b/drivers/memory/tegra/mc.c
> > > @@ -62,6 +62,27 @@ static const struct of_device_id tegra_mc_of_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
> > >  
> > > +int tegra_mc_flush(struct tegra_mc *mc, unsigned int swgroup, bool enable)
> > > +{
> > > +	int i;
> > > +	const struct tegra_mc_hr *client;
> > > +
> > > +	if (!mc || !mc->soc->hr_clients ||
> > > +			!mc->soc->ops || !mc->soc->ops->flush)
> > > +		return -EINVAL;;
> > > +
> > > +	client = mc->soc->hr_clients;
> > > +
> > > +	for (i = 0; i < mc->soc->num_hr_clients; i++, client++) {
> > > +		if (swgroup == client->swgroup) {
> > > +			return mc->soc->ops->flush(mc, client, enable);
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL(tegra_mc_flush);
> > 
> > Like Lucas already mentioned in response to another patch, having a
> > boolean "enable" argument is suboptimal here. Now according to
> > documentation the proper reset sequence for clients is something like
> > this:
> > 
> > 	1) set the FLUSH_ENABLE bit for the client
> > 	2) poll the FLUSH_DONE bit for the client
> > 	3) assert reset to the client using the CAR
> > 	4) deassert reset to the client using the CAR
> > 	5) clear the FLUSH_ENABLE bit for the client
> > 
> 
> Do we ever need to do this outside a powergating or railgating sequence?

I don't think so. I worry a little that we may encounter situations
where the driver itself wants to reset the hardware module via the CAR,
though I think if we made sure that any driver resets would only happen
with a runtime PM reference held it should be safe.

Furthermore there's the issue of keeping backwards-compatibility. There
are some drivers that actually do this kind of reset today, so they need
to be carefully audited before conversion.

And we need to conditionalize the manual powergate sequences that are
currently used by the various drivers so that they don't happen when
power domains are initialized.

> > This is really inconvenient because we can't flush the client using a
> > single operation. So I think we'll need two functions here, something
> > like: tegra_mc_flush_enable/disable(), or tegra_mc_flush_{,de}assert().
> > Or maybe even: tegra_mc_reset_{,de}assert() to mirror the reset
> > controller API. I suppose we could even export it using the reset
> > controller framework.
> > 
> > Doing so would allow us to have power domain DT nodes like this:
> > 
> > 	pmc@0,7000e400 {
> > 		power-domains {
> > 			...
> > 
> > 			gpu {
> > 				resets = <&tegra_car 184>,
> > 					 <&mc TEGRA_SWGROUP_GPU>;
> > 				reset-names = "module", "client";
> > 			};
> > 
> > 			...
> > 		};
> > 	};
> > 
> > The PMC driver could then grab the "module" and "client" resets and do
> > something like this:
> > 
> > 	reset_control_assert(powergate->rst_client);
> > 	reset_control_assert(powergate->rst_module);
> > 	reset_control_deassert(powergate->rst_module);
> > 	reset_control_deassert(powergate->rst_client);
> > 
> > Optionally the above could be extended with a reset_control_status()-
> > loop. Alternatively reset_control_assert() would block until the
> > FLUSH_DONE bit is set.
> > 
> 
> I think the reset_control_assert should wait for the FLUSH_DONE bit to be set
> because only then all outstanding memory transactions for the client are
> completed so you can't realistically claim the reset has been asserted before
> the bit is set. Then you could also expose a single reset which handles both
> the memory client and the module reset in CAR? Ie:
> 
> reset_control_assert(powergate->rst_module); would set FLUSH_ENABLE for the
> memory client, wait for the FLUSH_DONE bit and then assert the CAR reset.
> reset_control_deassert(powergate->rst_module); would deassert the CAR reset
> and then clear the FLUSH_ENABLE bit. Or is there a usecase to control them
> individually?

I suppose that would be possible, but it'd mean that we need to access
registers across driver boundaries (the CAR driver would need to write
the memory controller's registers or vice versa). Doing this with
separate reset controls allows the drivers to be nicely separated.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Vince Hsu]

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
index 278d40b854c1..036935743a0a 100644
--- a/drivers/memory/tegra/tegra124.c
+++ b/drivers/memory/tegra/tegra124.c
@@ -6,6 +6,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/mm.h>
 
@@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
 	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
 };
 
+static const struct tegra_mc_hr tegra124_mc_hr[] = {
+	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
+	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
+	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
+	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
+	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
+	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
+	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
+	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
+	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
+	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
+	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
+	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
+	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
+	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
+	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
+	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
+	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
+	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
+	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
+	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
+	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},
+	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
+	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
+	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
+};
+
 #ifdef CONFIG_ARCH_TEGRA_124_SOC
+
+static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
+		u32 reg, u32 *stat)
+{
+	int i;
+	u32 cur_stat;
+	u32 prv_stat;
+
+	prv_stat = mc_readl(mc, reg);
+	for (i = 0; i < 5; i++) {
+		cur_stat = mc_readl(mc, reg);
+		if (cur_stat != prv_stat)
+			return false;
+	}
+	*stat = cur_stat;
+	return true;
+}
+
+static int tegra124_mc_flush(struct tegra_mc *mc,
+		const struct tegra_mc_hr *hr_client, bool enable)
+{
+	u32 val;
+
+	if (!mc || !hr_client)
+		return -EINVAL;
+
+	val = mc_readl(mc, hr_client->ctrl);
+	if (enable)
+		val |= BIT(hr_client->bit);
+	else
+		val &= ~BIT(hr_client->bit);
+	mc_writel(mc, val, hr_client->ctrl);
+	mc_readl(mc, hr_client->ctrl);
+
+	/* poll till the flush is done */
+	if (enable) {
+		do {
+			udelay(10);
+			val = 0;
+			if (!tegra124_stable_hotreset_check(mc, hr_client->status, &val))
+				continue;
+		} while (!(val & BIT(hr_client->bit)));
+	}
+
+	return 0;
+}
+
+static const struct tegra_mc_ops tegra124_mc_ops = {
+	.flush = tegra124_mc_flush,
+};
+
 static void tegra124_flush_dcache(struct page *page, unsigned long offset,
 				  size_t size)
 {
@@ -991,5 +1070,8 @@ const struct tegra_mc_soc tegra124_mc_soc = {
 	.num_address_bits = 34,
 	.atom_size = 32,
 	.smmu = &tegra124_smmu_soc,
+	.hr_clients = tegra124_mc_hr,
+	.num_hr_clients = ARRAY_SIZE(tegra124_mc_hr),
+	.ops = &tegra124_mc_ops,
 };
 #endif /* CONFIG_ARCH_TEGRA_124_SOC */
-- 
1.9.1

Re: [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3409 bytes --]

On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> index 278d40b854c1..036935743a0a 100644
> --- a/drivers/memory/tegra/tegra124.c
> +++ b/drivers/memory/tegra/tegra124.c
> @@ -6,6 +6,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/mm.h>
>  
> @@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
>  	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
>  };
>  
> +static const struct tegra_mc_hr tegra124_mc_hr[] = {
> +	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
> +	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
> +	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
> +	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
> +	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
> +	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
> +	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
> +	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
> +	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
> +	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
> +	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
> +	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
> +	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
> +	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
> +	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
> +	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
> +	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
> +	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
> +	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
> +	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
> +	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},

The documentation that I have says that the status register for these is
0x204.

> +	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> +	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> +	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> +};
> +
>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> +
> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> +		u32 reg, u32 *stat)
> +{
> +	int i;
> +	u32 cur_stat;
> +	u32 prv_stat;
> +
> +	prv_stat = mc_readl(mc, reg);
> +	for (i = 0; i < 5; i++) {
> +		cur_stat = mc_readl(mc, reg);
> +		if (cur_stat != prv_stat)
> +			return false;
> +	}

Why this loop? The function is already called in a polling loop below.
Also why compare to the previous value of the register? Isn't the only
thing we're interested in the value of the specific bit?

> +	*stat = cur_stat;
> +	return true;
> +}
> +
> +static int tegra124_mc_flush(struct tegra_mc *mc,
> +		const struct tegra_mc_hr *hr_client, bool enable)
> +{
> +	u32 val;
> +
> +	if (!mc || !hr_client)
> +		return -EINVAL;
> +
> +	val = mc_readl(mc, hr_client->ctrl);
> +	if (enable)
> +		val |= BIT(hr_client->bit);
> +	else
> +		val &= ~BIT(hr_client->bit);
> +	mc_writel(mc, val, hr_client->ctrl);
> +	mc_readl(mc, hr_client->ctrl);
> +
> +	/* poll till the flush is done */
> +	if (enable) {
> +		do {
> +			udelay(10);

This should probably be usleep_range(10, 20) or something.

Would it be difficult to implement this for Tegra30 and Tegra114?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Vince Hsu]

On 03:30:00PM Jan 06, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > ---
> >  drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> > index 278d40b854c1..036935743a0a 100644
> > --- a/drivers/memory/tegra/tegra124.c
> > +++ b/drivers/memory/tegra/tegra124.c
> > @@ -6,6 +6,7 @@
> >   * published by the Free Software Foundation.
> >   */
> >  
> > +#include <linux/delay.h>
> >  #include <linux/of.h>
> >  #include <linux/mm.h>
> >  
> > @@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
> >  	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
> >  };
> >  
> > +static const struct tegra_mc_hr tegra124_mc_hr[] = {
> > +	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
> > +	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
> > +	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
> > +	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
> > +	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
> > +	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
> > +	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
> > +	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
> > +	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
> > +	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
> > +	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
> > +	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
> > +	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
> > +	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
> > +	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
> > +	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
> > +	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
> > +	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
> > +	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
> > +	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
> > +	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},
> 
> The documentation that I have says that the status register for these is
> 0x204.
Oops. Thanks for catching this. Will fix.

> 
> > +	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> > +	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> > +	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> > +};
> > +
> >  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> > +
> > +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> > +		u32 reg, u32 *stat)
> > +{
> > +	int i;
> > +	u32 cur_stat;
> > +	u32 prv_stat;
> > +
> > +	prv_stat = mc_readl(mc, reg);
> > +	for (i = 0; i < 5; i++) {
> > +		cur_stat = mc_readl(mc, reg);
> > +		if (cur_stat != prv_stat)
> > +			return false;
> > +	}
> 
> Why this loop? The function is already called in a polling loop below.
> Also why compare to the previous value of the register? Isn't the only
> thing we're interested in the value of the specific bit?
I recall it's due to a HW bug that there might be a gitch if we program
the ctrl reg and then read the status reg in a short window. This function
is to make sure we have a stable status.

> 
> > +	*stat = cur_stat;
> > +	return true;
> > +}
> > +
> > +static int tegra124_mc_flush(struct tegra_mc *mc,
> > +		const struct tegra_mc_hr *hr_client, bool enable)
> > +{
> > +	u32 val;
> > +
> > +	if (!mc || !hr_client)
> > +		return -EINVAL;
> > +
> > +	val = mc_readl(mc, hr_client->ctrl);
> > +	if (enable)
> > +		val |= BIT(hr_client->bit);
> > +	else
> > +		val &= ~BIT(hr_client->bit);
> > +	mc_writel(mc, val, hr_client->ctrl);
> > +	mc_readl(mc, hr_client->ctrl);
> > +
> > +	/* poll till the flush is done */
> > +	if (enable) {
> > +		do {
> > +			udelay(10);
> 
> This should probably be usleep_range(10, 20) or something.
Maybe no. We might need some spin lock here to ensure only one flushing
operation requested and no race could happen.

> 
> Would it be difficult to implement this for Tegra30 and Tegra114?
No. But I have to check the detail in Tegra30 and Tegra114. And the biggest
problem is I don't have the boards to test.

Thanks,
Vince

Re: [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

On Tue, Jan 06, 2015 at 11:07:45PM +0800, Vince Hsu wrote:
> On 03:30:00PM Jan 06, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> > > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > > ---
> > >  drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 82 insertions(+)
> > > 
> > > diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> > > index 278d40b854c1..036935743a0a 100644
> > > --- a/drivers/memory/tegra/tegra124.c
> > > +++ b/drivers/memory/tegra/tegra124.c
> > > @@ -6,6 +6,7 @@
> > >   * published by the Free Software Foundation.
> > >   */
> > >  
> > > +#include <linux/delay.h>
> > >  #include <linux/of.h>
> > >  #include <linux/mm.h>
> > >  
> > > @@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
> > >  	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
> > >  };
> > >  
> > > +static const struct tegra_mc_hr tegra124_mc_hr[] = {
> > > +	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
> > > +	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
> > > +	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
> > > +	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
> > > +	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
> > > +	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
> > > +	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
> > > +	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
> > > +	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
> > > +	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
> > > +	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
> > > +	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
> > > +	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
> > > +	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
> > > +	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
> > > +	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
> > > +	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
> > > +	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
> > > +	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
> > > +	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
> > > +	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},
> > 
> > The documentation that I have says that the status register for these is
> > 0x204.
> Oops. Thanks for catching this. Will fix.
> 
> > 
> > > +	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> > > +	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> > > +	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> > > +};
> > > +
> > >  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> > > +
> > > +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> > > +		u32 reg, u32 *stat)
> > > +{
> > > +	int i;
> > > +	u32 cur_stat;
> > > +	u32 prv_stat;
> > > +
> > > +	prv_stat = mc_readl(mc, reg);
> > > +	for (i = 0; i < 5; i++) {
> > > +		cur_stat = mc_readl(mc, reg);
> > > +		if (cur_stat != prv_stat)
> > > +			return false;
> > > +	}
> > 
> > Why this loop? The function is already called in a polling loop below.
> > Also why compare to the previous value of the register? Isn't the only
> > thing we're interested in the value of the specific bit?
> I recall it's due to a HW bug that there might be a gitch if we program
> the ctrl reg and then read the status reg in a short window. This function
> is to make sure we have a stable status.

This warrants a comment, then.

> > > +	*stat = cur_stat;
> > > +	return true;
> > > +}
> > > +
> > > +static int tegra124_mc_flush(struct tegra_mc *mc,
> > > +		const struct tegra_mc_hr *hr_client, bool enable)
> > > +{
> > > +	u32 val;
> > > +
> > > +	if (!mc || !hr_client)
> > > +		return -EINVAL;
> > > +
> > > +	val = mc_readl(mc, hr_client->ctrl);
> > > +	if (enable)
> > > +		val |= BIT(hr_client->bit);
> > > +	else
> > > +		val &= ~BIT(hr_client->bit);
> > > +	mc_writel(mc, val, hr_client->ctrl);
> > > +	mc_readl(mc, hr_client->ctrl);
> > > +
> > > +	/* poll till the flush is done */
> > > +	if (enable) {
> > > +		do {
> > > +			udelay(10);
> > 
> > This should probably be usleep_range(10, 20) or something.
> Maybe no. We might need some spin lock here to ensure only one flushing
> operation requested and no race could happen.

We should use a mutex, then. There's no saying how long this will take
and busy-looping indefinitely is a bad idea. Though it seems to me like
we don't need to lock around the polling loop here since we're merely
reading a status register. We would only need to lock around accesses to
the control register to make sure two processes can't step on each
others's toes.

> > Would it be difficult to implement this for Tegra30 and Tegra114?
> No. But I have to check the detail in Tegra30 and Tegra114. And the biggest
> problem is I don't have the boards to test.

I can help with testing. Though that raises the question of how this can
be tested. It seems like this feature is used to make sure that all
outstanding memory requests from clients are flushed before resetting a
module. Typically Linux assumes that devices do that anyway, so if a
device is suspended or shut down, the corresponding function should
ensure that all outstanding transfers have been cancelled and are
flushed.

Also we've managed just fine so far, so I'm beginning to wonder whether
we actually need this feature on Linux. If not, how do we test that this
is indeed doing what it should? How to trigger a condition that requires
flushing and how do we determine that flushing actually fixes things?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory clients

[Vince Hsu]

On 04:27:52PM Jan 06, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 06, 2015 at 11:07:45PM +0800, Vince Hsu wrote:
> > On 03:30:00PM Jan 06, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Tue, Dec 23, 2014 at 06:39:56PM +0800, Vince Hsu wrote:
> > > > Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > > > ---
> > > >  drivers/memory/tegra/tegra124.c | 82 +++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 82 insertions(+)
> > > > 
> > > > diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> > > > index 278d40b854c1..036935743a0a 100644
> > > > --- a/drivers/memory/tegra/tegra124.c
> > > > +++ b/drivers/memory/tegra/tegra124.c
> > > > @@ -6,6 +6,7 @@
> > > >   * published by the Free Software Foundation.
> > > >   */
> > > >  
> > > > +#include <linux/delay.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/mm.h>
> > > >  
> > > > @@ -959,7 +960,85 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
> > > >  	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
> > > >  };
> > > >  
> > > > +static const struct tegra_mc_hr tegra124_mc_hr[] = {
> > > > +	{TEGRA_SWGROUP_AFI,        0x200, 0x200,  0},
> > > > +	{TEGRA_SWGROUP_AVPC,       0x200, 0x200,  1},
> > > > +	{TEGRA_SWGROUP_DC,         0x200, 0x200,  2},
> > > > +	{TEGRA_SWGROUP_DCB,        0x200, 0x200,  3},
> > > > +	{TEGRA_SWGROUP_HC,         0x200, 0x200,  6},
> > > > +	{TEGRA_SWGROUP_HDA,        0x200, 0x200,  7},
> > > > +	{TEGRA_SWGROUP_ISP2,       0x200, 0x200,  8},
> > > > +	{TEGRA_SWGROUP_MPCORE,     0x200, 0x200,  9},
> > > > +	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x200, 10},
> > > > +	{TEGRA_SWGROUP_MSENC,      0x200, 0x200, 11},
> > > > +	{TEGRA_SWGROUP_PPCS,       0x200, 0x200, 14},
> > > > +	{TEGRA_SWGROUP_SATA,       0x200, 0x200, 15},
> > > > +	{TEGRA_SWGROUP_VDE,        0x200, 0x200, 16},
> > > > +	{TEGRA_SWGROUP_VI,         0x200, 0x200, 17},
> > > > +	{TEGRA_SWGROUP_VIC,        0x200, 0x200, 18},
> > > > +	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x200, 19},
> > > > +	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x200, 20},
> > > > +	{TEGRA_SWGROUP_TSEC,       0x200, 0x200, 22},
> > > > +	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x200, 29},
> > > > +	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x200, 30},
> > > > +	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x200, 31},
> > > 
> > > The documentation that I have says that the status register for these is
> > > 0x204.
> > Oops. Thanks for catching this. Will fix.
> > 
> > > 
> > > > +	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> > > > +	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> > > > +	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> > > > +};
> > > > +
> > > >  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> > > > +
> > > > +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> > > > +		u32 reg, u32 *stat)
> > > > +{
> > > > +	int i;
> > > > +	u32 cur_stat;
> > > > +	u32 prv_stat;
> > > > +
> > > > +	prv_stat = mc_readl(mc, reg);
> > > > +	for (i = 0; i < 5; i++) {
> > > > +		cur_stat = mc_readl(mc, reg);
> > > > +		if (cur_stat != prv_stat)
> > > > +			return false;
> > > > +	}
> > > 
> > > Why this loop? The function is already called in a polling loop below.
> > > Also why compare to the previous value of the register? Isn't the only
> > > thing we're interested in the value of the specific bit?
> > I recall it's due to a HW bug that there might be a gitch if we program
> > the ctrl reg and then read the status reg in a short window. This function
> > is to make sure we have a stable status.
> 
> This warrants a comment, then.
Okay.

> 
> > > > +	*stat = cur_stat;
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static int tegra124_mc_flush(struct tegra_mc *mc,
> > > > +		const struct tegra_mc_hr *hr_client, bool enable)
> > > > +{
> > > > +	u32 val;
> > > > +
> > > > +	if (!mc || !hr_client)
> > > > +		return -EINVAL;
> > > > +
> > > > +	val = mc_readl(mc, hr_client->ctrl);
> > > > +	if (enable)
> > > > +		val |= BIT(hr_client->bit);
> > > > +	else
> > > > +		val &= ~BIT(hr_client->bit);
> > > > +	mc_writel(mc, val, hr_client->ctrl);
> > > > +	mc_readl(mc, hr_client->ctrl);
> > > > +
> > > > +	/* poll till the flush is done */
> > > > +	if (enable) {
> > > > +		do {
> > > > +			udelay(10);
> > > 
> > > This should probably be usleep_range(10, 20) or something.
> > Maybe no. We might need some spin lock here to ensure only one flushing
> > operation requested and no race could happen.
> 
> We should use a mutex, then. There's no saying how long this will take
> and busy-looping indefinitely is a bad idea. Though it seems to me like
> we don't need to lock around the polling loop here since we're merely
> reading a status register. We would only need to lock around accesses to
> the control register to make sure two processes can't step on each
> others's toes.
We can use metux definitely. If two processes touch the ctrl registers
sequentially and poll the status register in parallel, we dont' know whether
the glitch is caused by the HW bug or the concurrent ctrl register programming?
We should lock the status checking as well.

> 
> > > Would it be difficult to implement this for Tegra30 and Tegra114?
> > No. But I have to check the detail in Tegra30 and Tegra114. And the biggest
> > problem is I don't have the boards to test.
> 
> I can help with testing. Though that raises the question of how this can
> be tested. It seems like this feature is used to make sure that all
> outstanding memory requests from clients are flushed before resetting a
> module. Typically Linux assumes that devices do that anyway, so if a
> device is suspended or shut down, the corresponding function should
> ensure that all outstanding transfers have been cancelled and are
> flushed.
The flushing operation can be requested by runtime PM if the device supports
it.

> 
> Also we've managed just fine so far, so I'm beginning to wonder whether
> we actually need this feature on Linux. If not, how do we test that this
> is indeed doing what it should? How to trigger a condition that requires
> flushing and how do we determine that flushing actually fixes things?
Sorry that I can't answer how to test it because I'm not a mc expert.
What I can do is testing the power up/off sequence a lot of time and check if
the device can still work normally. We need this feature because the TRM
requires so?

Thanks,
Vince

[PATCH 4/11] ARM: tegra: add mc node for Tegra124 GPU

[Vince Hsu]

The Tegra124 GPU needs the memory controller for the memory flush operatoin.
So add the node reference of memory controller in device tree.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 4be06c6ea0c8..4109c4548b55 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -187,6 +187,7 @@
 		clock-names = "gpu", "pwr";
 		resets = <&tegra_car 184>;
 		reset-names = "gpu";
+		mc = <&mc TEGRA_SWGROUP_GPU>;
 		status = "disabled";
 	};
 
-- 
1.9.1

[PATCH nouveau 05/11] platform: switch to the new gpu rail clamping function

[Vince Hsu]

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/nouveau_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
index b307bbedd4c4..68788b17a45c 100644
--- a/drm/nouveau_platform.c
+++ b/drm/nouveau_platform.c
@@ -53,7 +53,7 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
 	reset_control_assert(gpu->rst);
 	udelay(10);
 
-	err = tegra_powergate_remove_clamping(TEGRA_POWERGATE_3D);
+	err = tegra_powergate_gpu_set_clamping(false);
 	if (err)
 		goto err_clamp;
 	udelay(10);
-- 
1.9.1

[PATCH nouveau 06/11] platform: complete the power up/down sequence

[Vince Hsu]

This patch adds some missing pieces of the rail gaing/ungating sequence that
can improve the stability in theory.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 drm/nouveau_platform.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
index 68788b17a45c..527fe2358fc9 100644
--- a/drm/nouveau_platform.c
+++ b/drm/nouveau_platform.c
@@ -25,9 +25,11 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/reset.h>
 #include <linux/regulator/consumer.h>
 #include <soc/tegra/fuse.h>
+#include <soc/tegra/mc.h>
 #include <soc/tegra/pmc.h>
 
 #include "nouveau_drm.h"
@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
 	reset_control_deassert(gpu->rst);
 	udelay(10);
 
+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
+	udelay(10);
+
 	return 0;
 
 err_clamp:
@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
 {
 	int err;
 
+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
+	udelay(10);
+
+	err = tegra_powergate_gpu_set_clamping(true);
+	if (err)
+		return err;
+	udelay(10);
+
 	reset_control_assert(gpu->rst);
 	udelay(10);
 
@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
 	return 0;
 }
 
+static int nouveau_platform_get_mc(struct device *dev,
+		struct tegra_mc **mc, unsigned int *swgroup)
+{
+	struct of_phandle_args args;
+	struct platform_device *pdev;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(dev->of_node, "mc",
+				1, 0, &args);
+	if (ret)
+		return ret;
+
+	pdev = of_find_device_by_node(args.np);
+	if (!pdev)
+		return -EINVAL;
+
+	*mc = platform_get_drvdata(pdev);
+	if (!*mc)
+		return -EINVAL;
+
+	*swgroup = args.args[0];
+
+	return 0;
+}
+
 static int nouveau_platform_probe(struct platform_device *pdev)
 {
 	struct nouveau_platform_gpu *gpu;
@@ -118,6 +156,10 @@ static int nouveau_platform_probe(struct platform_device *pdev)
 	if (IS_ERR(gpu->clk_pwr))
 		return PTR_ERR(gpu->clk_pwr);
 
+	err = nouveau_platform_get_mc(&pdev->dev, &gpu->mc, &gpu->swgroup);
+	if (err)
+		return err;
+
 	err = nouveau_platform_power_up(gpu);
 	if (err)
 		return err;
diff --git a/drm/nouveau_platform.h b/drm/nouveau_platform.h
index 58c28b5653d5..969dbddd786d 100644
--- a/drm/nouveau_platform.h
+++ b/drm/nouveau_platform.h
@@ -35,6 +35,9 @@ struct nouveau_platform_gpu {
 	struct clk *clk_pwr;
 
 	struct regulator *vdd;
+
+	struct tegra_mc *mc;
+	unsigned int swgroup;
 };
 
 struct nouveau_platform_device {
-- 
1.9.1

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Lucas Stach]

Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> This patch adds some missing pieces of the rail gaing/ungating sequence that
> can improve the stability in theory.
> 
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  drm/nouveau_platform.h |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> index 68788b17a45c..527fe2358fc9 100644
> --- a/drm/nouveau_platform.c
> +++ b/drm/nouveau_platform.c
> @@ -25,9 +25,11 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/reset.h>
>  #include <linux/regulator/consumer.h>
>  #include <soc/tegra/fuse.h>
> +#include <soc/tegra/mc.h>
>  #include <soc/tegra/pmc.h>
>  
>  #include "nouveau_drm.h"
> @@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
>  	reset_control_deassert(gpu->rst);
>  	udelay(10);
>  
> +	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> +	udelay(10);
> +
>  	return 0;
>  
>  err_clamp:
> @@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>  {
>  	int err;
>  
> +	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> +	udelay(10);
> +
> +	err = tegra_powergate_gpu_set_clamping(true);
> +	if (err)
> +		return err;
> +	udelay(10);
> +
>  	reset_control_assert(gpu->rst);
>  	udelay(10);
>  
> @@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>  	return 0;
>  }
>  
> +static int nouveau_platform_get_mc(struct device *dev,
> +		struct tegra_mc **mc, unsigned int *swgroup)

Uhm, no. If this is needed this has to be a Tegra MC function and not
burried into nouveau code. You are using knowledge about the internal
workings of the MC driver here.

Also this should probably only take the Dt node pointer as argument and
return a something like a tegra_mc_client struct that contains both the
MC device pointer and the swgroup so you can pass that to
tegra_mc_flush().

> +{
> +	struct of_phandle_args args;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_fixed_args(dev->of_node, "mc",
> +				1, 0, &args);
> +	if (ret)
> +		return ret;
> +
> +	pdev = of_find_device_by_node(args.np);
> +	if (!pdev)
> +		return -EINVAL;

This is wrong, you need to handle -EPROBE_DEFER here.

> +
> +	*mc = platform_get_drvdata(pdev);
> +	if (!*mc)
> +		return -EINVAL;
> +
> +	*swgroup = args.args[0];
> +
> +	return 0;
> +}
> +
>  static int nouveau_platform_probe(struct platform_device *pdev)
>  {
>  	struct nouveau_platform_gpu *gpu;
> @@ -118,6 +156,10 @@ static int nouveau_platform_probe(struct platform_device *pdev)
>  	if (IS_ERR(gpu->clk_pwr))
>  		return PTR_ERR(gpu->clk_pwr);
>  
> +	err = nouveau_platform_get_mc(&pdev->dev, &gpu->mc, &gpu->swgroup);
> +	if (err)
> +		return err;
> +
>  	err = nouveau_platform_power_up(gpu);
>  	if (err)
>  		return err;
> diff --git a/drm/nouveau_platform.h b/drm/nouveau_platform.h
> index 58c28b5653d5..969dbddd786d 100644
> --- a/drm/nouveau_platform.h
> +++ b/drm/nouveau_platform.h
> @@ -35,6 +35,9 @@ struct nouveau_platform_gpu {
>  	struct clk *clk_pwr;
>  
>  	struct regulator *vdd;
> +
> +	struct tegra_mc *mc;
> +	unsigned int swgroup;
>  };
>  
>  struct nouveau_platform_device {

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Vince Hsu]

On 12/24/2014 09:23 PM, Lucas Stach wrote:
> Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
>> This patch adds some missing pieces of the rail gaing/ungating sequence that
>> can improve the stability in theory.
>>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>> ---
>>   drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   drm/nouveau_platform.h |  3 +++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
>> index 68788b17a45c..527fe2358fc9 100644
>> --- a/drm/nouveau_platform.c
>> +++ b/drm/nouveau_platform.c
>> @@ -25,9 +25,11 @@
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>   #include <linux/reset.h>
>>   #include <linux/regulator/consumer.h>
>>   #include <soc/tegra/fuse.h>
>> +#include <soc/tegra/mc.h>
>>   #include <soc/tegra/pmc.h>
>>   
>>   #include "nouveau_drm.h"
>> @@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
>>   	reset_control_deassert(gpu->rst);
>>   	udelay(10);
>>   
>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
>> +	udelay(10);
>> +
>>   	return 0;
>>   
>>   err_clamp:
>> @@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>   {
>>   	int err;
>>   
>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
>> +	udelay(10);
>> +
>> +	err = tegra_powergate_gpu_set_clamping(true);
>> +	if (err)
>> +		return err;
>> +	udelay(10);
>> +
>>   	reset_control_assert(gpu->rst);
>>   	udelay(10);
>>   
>> @@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>   	return 0;
>>   }
>>   
>> +static int nouveau_platform_get_mc(struct device *dev,
>> +		struct tegra_mc **mc, unsigned int *swgroup)
> Uhm, no. If this is needed this has to be a Tegra MC function and not
> burried into nouveau code. You are using knowledge about the internal
> workings of the MC driver here.
>
> Also this should probably only take the Dt node pointer as argument and
> return a something like a tegra_mc_client struct that contains both the
> MC device pointer and the swgroup so you can pass that to
> tegra_mc_flush().
Good idea. I will have something as below in V2 if there is no other 
comments for this.

tegra_mc_client *tegra_mc_find_client(struct device_node *node)
{
     ...
     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
     ...
}

There were some discussion about this few weeks ago. I'm not sure 
whether we have some conclusion/implementation though. Thierry?

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html

>
>> +{
>> +	struct of_phandle_args args;
>> +	struct platform_device *pdev;
>> +	int ret;
>> +
>> +	ret = of_parse_phandle_with_fixed_args(dev->of_node, "mc",
>> +				1, 0, &args);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pdev = of_find_device_by_node(args.np);
>> +	if (!pdev)
>> +		return -EINVAL;
> This is wrong, you need to handle -EPROBE_DEFER here.
Indeed. Will fix.
>
>> +
>> +	*mc = platform_get_drvdata(pdev);
>> +	if (!*mc)
>> +		return -EINVAL;
>> +
>> +	*swgroup = args.args[0];
>> +
>> +	return 0;
>> +}
>> +
>>   static int nouveau_platform_probe(struct platform_device *pdev)
>>   {
>>   	struct nouveau_platform_gpu *gpu;
>> @@ -118,6 +156,10 @@ static int nouveau_platform_probe(struct platform_device *pdev)
>>   	if (IS_ERR(gpu->clk_pwr))
>>   		return PTR_ERR(gpu->clk_pwr);
>>   
>> +	err = nouveau_platform_get_mc(&pdev->dev, &gpu->mc, &gpu->swgroup);
>> +	if (err)
>> +		return err;
>> +
>>   	err = nouveau_platform_power_up(gpu);
>>   	if (err)
>>   		return err;
>> diff --git a/drm/nouveau_platform.h b/drm/nouveau_platform.h
>> index 58c28b5653d5..969dbddd786d 100644
>> --- a/drm/nouveau_platform.h
>> +++ b/drm/nouveau_platform.h
>> @@ -35,6 +35,9 @@ struct nouveau_platform_gpu {
>>   	struct clk *clk_pwr;
>>   
>>   	struct regulator *vdd;
>> +
>> +	struct tegra_mc *mc;
>> +	unsigned int swgroup;
>>   };
>>   
>>   struct nouveau_platform_device {
>

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]

On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> 
> On 12/24/2014 09:23 PM, Lucas Stach wrote:
> >Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>This patch adds some missing pieces of the rail gaing/ungating sequence that
> >>can improve the stability in theory.
> >>
> >>Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> >>---
> >>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  drm/nouveau_platform.h |  3 +++
> >>  2 files changed, 45 insertions(+)
> >>
> >>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> >>index 68788b17a45c..527fe2358fc9 100644
> >>--- a/drm/nouveau_platform.c
> >>+++ b/drm/nouveau_platform.c
> >>@@ -25,9 +25,11 @@
> >>  #include <linux/module.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/of.h>
> >>+#include <linux/of_platform.h>
> >>  #include <linux/reset.h>
> >>  #include <linux/regulator/consumer.h>
> >>  #include <soc/tegra/fuse.h>
> >>+#include <soc/tegra/mc.h>
> >>  #include <soc/tegra/pmc.h>
> >>  #include "nouveau_drm.h"
> >>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> >>  	reset_control_deassert(gpu->rst);
> >>  	udelay(10);
> >>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> >>+	udelay(10);
> >>+
> >>  	return 0;
> >>  err_clamp:
> >>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>  {
> >>  	int err;
> >>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> >>+	udelay(10);
> >>+
> >>+	err = tegra_powergate_gpu_set_clamping(true);
> >>+	if (err)
> >>+		return err;
> >>+	udelay(10);
> >>+
> >>  	reset_control_assert(gpu->rst);
> >>  	udelay(10);
> >>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>  	return 0;
> >>  }
> >>+static int nouveau_platform_get_mc(struct device *dev,
> >>+		struct tegra_mc **mc, unsigned int *swgroup)
> >Uhm, no. If this is needed this has to be a Tegra MC function and not
> >burried into nouveau code. You are using knowledge about the internal
> >workings of the MC driver here.
> >
> >Also this should probably only take the Dt node pointer as argument and
> >return a something like a tegra_mc_client struct that contains both the
> >MC device pointer and the swgroup so you can pass that to
> >tegra_mc_flush().
> Good idea. I will have something as below in V2 if there is no other
> comments for this.
> 
> tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> {
>     ...
>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
>     ...
> }
> 
> There were some discussion about this few weeks ago. I'm not sure whether we
> have some conclusion/implementation though. Thierry?
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html

I don't think client is a good fit here. Flushing is done per SWGROUP
(on all clients of the SWGROUP). So I think we'll want something like:

	gpu@0,57000000 {
		...
		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
		...
	};

In the DT and return a struct tegra_mc_swgroup along the lines of:

	struct tegra_mc_client {
		unsigned int id;
		unsigned int swgroup;

		struct list_head list;
	};

	struct tegra_mc_swgroup {
		struct list_head clients;
		unsigned int id;
	};

Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
structures, each representing a memory client pertaining to the
SWGROUP.

We probably don't want to expose these structures publicly, an opaque
type should be enough. Then you can use functions like:

	struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node);

At some point we may even need something like:

	struct tegra_mc_client *tegra_mc_find_client(struct device_node *node,
						     const char *name);

And DT content like this:

	gpu@0,57000000 {
		...
		nvidia,memory-clients = <&mc 0x58>, <&mc 0x59>;
		nvidia,memory-client-names = "read", "write";
		...
	};

This could be useful for latency allowance programming, but we can cross
that bridge when we come to it.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Vince Hsu]

On 01/05/2015 11:25 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
>> On 12/24/2014 09:23 PM, Lucas Stach wrote:
>>> Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
>>>> This patch adds some missing pieces of the rail gaing/ungating sequence that
>>>> can improve the stability in theory.
>>>>
>>>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>>>> ---
>>>>   drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>   drm/nouveau_platform.h |  3 +++
>>>>   2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
>>>> index 68788b17a45c..527fe2358fc9 100644
>>>> --- a/drm/nouveau_platform.c
>>>> +++ b/drm/nouveau_platform.c
>>>> @@ -25,9 +25,11 @@
>>>>   #include <linux/module.h>
>>>>   #include <linux/platform_device.h>
>>>>   #include <linux/of.h>
>>>> +#include <linux/of_platform.h>
>>>>   #include <linux/reset.h>
>>>>   #include <linux/regulator/consumer.h>
>>>>   #include <soc/tegra/fuse.h>
>>>> +#include <soc/tegra/mc.h>
>>>>   #include <soc/tegra/pmc.h>
>>>>   #include "nouveau_drm.h"
>>>> @@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
>>>>   	reset_control_deassert(gpu->rst);
>>>>   	udelay(10);
>>>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
>>>> +	udelay(10);
>>>> +
>>>>   	return 0;
>>>>   err_clamp:
>>>> @@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>>>   {
>>>>   	int err;
>>>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
>>>> +	udelay(10);
>>>> +
>>>> +	err = tegra_powergate_gpu_set_clamping(true);
>>>> +	if (err)
>>>> +		return err;
>>>> +	udelay(10);
>>>> +
>>>>   	reset_control_assert(gpu->rst);
>>>>   	udelay(10);
>>>> @@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>>>   	return 0;
>>>>   }
>>>> +static int nouveau_platform_get_mc(struct device *dev,
>>>> +		struct tegra_mc **mc, unsigned int *swgroup)
>>> Uhm, no. If this is needed this has to be a Tegra MC function and not
>>> burried into nouveau code. You are using knowledge about the internal
>>> workings of the MC driver here.
>>>
>>> Also this should probably only take the Dt node pointer as argument and
>>> return a something like a tegra_mc_client struct that contains both the
>>> MC device pointer and the swgroup so you can pass that to
>>> tegra_mc_flush().
>> Good idea. I will have something as below in V2 if there is no other
>> comments for this.
>>
>> tegra_mc_client *tegra_mc_find_client(struct device_node *node)
>> {
>>      ...
>>      ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
>>      ...
>> }
>>
>> There were some discussion about this few weeks ago. I'm not sure whether we
>> have some conclusion/implementation though. Thierry?
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
> I don't think client is a good fit here. Flushing is done per SWGROUP
> (on all clients of the SWGROUP). So I think we'll want something like:
>
> 	gpu@0,57000000 {
> 		...
> 		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
> 		...
> 	};
>
> In the DT and return a struct tegra_mc_swgroup along the lines of:
>
> 	struct tegra_mc_client {
> 		unsigned int id;
> 		unsigned int swgroup;
>
> 		struct list_head list;
> 	};
>
> 	struct tegra_mc_swgroup {
> 		struct list_head clients;
> 		unsigned int id;
> 	};
>
> Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
> structures, each representing a memory client pertaining to the
> SWGROUP.
Based on your suggestion above, I created a struct tegra_mc_swgroup:

struct tegra_mc_swgroup {
     unsigned int id;
     struct tegra_mc *mc;
     struct list_head head;
     struct list_head clients;
};

And added the list head in the struct tegra_mc_soc.

struct tegra_mc_soc {
     struct tegra_mc_client *clients;
     unsigned int num_clients;

     struct tegra_mc_hr *hr_clients;
     unsigned int num_hr_clients;

     struct list_head swgroups;
...

Created one function to build the swgroup list.

static int tegra_mc_build_swgroup(struct tegra_mc *mc)
{
     int i;

     for (i = 0; i < mc->soc->num_clients; i++) {
         struct tegra_mc_swgroup *sg;
         bool found = false;

         list_for_each_entry(sg, &mc->soc->swgroups, head) {
             if (sg->id == mc->soc->clients[i].swgroup) {
                 found = true;
                 break;
             }
         }

         if (!found) {
             sg = devm_kzalloc(mc->dev, sizeof(*sg), GFP_KERNEL);
             if (!sg)
                 return -ENOMEM;

             sg->id = mc->soc->clients[i].swgroup;
             sg->mc = mc;
             list_add_tail(&sg->head, &mc->soc->swgroups);
             INIT_LIST_HEAD(&sg->clients);
         }

         list_add_tail(&mc->soc->clients[i].head, &sg->clients);
     }

     return 0;
}

>
> We probably don't want to expose these structures publicly, an opaque
> type should be enough. Then you can use functions like:
>
> 	struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node);
And then I can use the tegra_find_swgroup() in GK20A driver to get the 
swgroup
and flush the memory clients by tegra_mc_flush(swgroup).

One problem is that the mc_soc and mc_clients are are defined as const. 
To build
the swgroup list dynamically, I have to discard the const. I guess you 
won't like that. :(

Thanks,
Vince

> At some point we may even need something like:
>
> 	struct tegra_mc_client *tegra_mc_find_client(struct device_node *node,
> 						     const char *name);
>
> And DT content like this:
>
> 	gpu@0,57000000 {
> 		...
> 		nvidia,memory-clients = <&mc 0x58>, <&mc 0x59>;
> 		nvidia,memory-client-names = "read", "write";
> 		...
> 	};
>
> This could be useful for latency allowance programming, but we can cross
> that bridge when we come to it.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6061 bytes --]

On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> 
> On 01/05/2015 11:25 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> >>On 12/24/2014 09:23 PM, Lucas Stach wrote:
> >>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>>>This patch adds some missing pieces of the rail gaing/ungating sequence that
> >>>>can improve the stability in theory.
> >>>>
> >>>>Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> >>>>---
> >>>>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>>  drm/nouveau_platform.h |  3 +++
> >>>>  2 files changed, 45 insertions(+)
> >>>>
> >>>>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> >>>>index 68788b17a45c..527fe2358fc9 100644
> >>>>--- a/drm/nouveau_platform.c
> >>>>+++ b/drm/nouveau_platform.c
> >>>>@@ -25,9 +25,11 @@
> >>>>  #include <linux/module.h>
> >>>>  #include <linux/platform_device.h>
> >>>>  #include <linux/of.h>
> >>>>+#include <linux/of_platform.h>
> >>>>  #include <linux/reset.h>
> >>>>  #include <linux/regulator/consumer.h>
> >>>>  #include <soc/tegra/fuse.h>
> >>>>+#include <soc/tegra/mc.h>
> >>>>  #include <soc/tegra/pmc.h>
> >>>>  #include "nouveau_drm.h"
> >>>>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> >>>>  	reset_control_deassert(gpu->rst);
> >>>>  	udelay(10);
> >>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> >>>>+	udelay(10);
> >>>>+
> >>>>  	return 0;
> >>>>  err_clamp:
> >>>>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>>>  {
> >>>>  	int err;
> >>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> >>>>+	udelay(10);
> >>>>+
> >>>>+	err = tegra_powergate_gpu_set_clamping(true);
> >>>>+	if (err)
> >>>>+		return err;
> >>>>+	udelay(10);
> >>>>+
> >>>>  	reset_control_assert(gpu->rst);
> >>>>  	udelay(10);
> >>>>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>>>  	return 0;
> >>>>  }
> >>>>+static int nouveau_platform_get_mc(struct device *dev,
> >>>>+		struct tegra_mc **mc, unsigned int *swgroup)
> >>>Uhm, no. If this is needed this has to be a Tegra MC function and not
> >>>burried into nouveau code. You are using knowledge about the internal
> >>>workings of the MC driver here.
> >>>
> >>>Also this should probably only take the Dt node pointer as argument and
> >>>return a something like a tegra_mc_client struct that contains both the
> >>>MC device pointer and the swgroup so you can pass that to
> >>>tegra_mc_flush().
> >>Good idea. I will have something as below in V2 if there is no other
> >>comments for this.
> >>
> >>tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> >>{
> >>     ...
> >>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
> >>     ...
> >>}
> >>
> >>There were some discussion about this few weeks ago. I'm not sure whether we
> >>have some conclusion/implementation though. Thierry?
> >>
> >>http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
> >I don't think client is a good fit here. Flushing is done per SWGROUP
> >(on all clients of the SWGROUP). So I think we'll want something like:
> >
> >	gpu@0,57000000 {
> >		...
> >		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
> >		...
> >	};
> >
> >In the DT and return a struct tegra_mc_swgroup along the lines of:
> >
> >	struct tegra_mc_client {
> >		unsigned int id;
> >		unsigned int swgroup;
> >
> >		struct list_head list;
> >	};
> >
> >	struct tegra_mc_swgroup {
> >		struct list_head clients;
> >		unsigned int id;
> >	};
> >
> >Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
> >structures, each representing a memory client pertaining to the
> >SWGROUP.
> Based on your suggestion above, I created a struct tegra_mc_swgroup:
> 
> struct tegra_mc_swgroup {
>     unsigned int id;
>     struct tegra_mc *mc;
>     struct list_head head;
>     struct list_head clients;
> };
> 
> And added the list head in the struct tegra_mc_soc.
> 
> struct tegra_mc_soc {
>     struct tegra_mc_client *clients;
>     unsigned int num_clients;
> 
>     struct tegra_mc_hr *hr_clients;
>     unsigned int num_hr_clients;

Why do you still need these?

>     struct list_head swgroups;

This doesn't belong in struct tegra_mc_soc because that's meant to be
static information about the specific variant of the memory-controller.
Put it in struct tegra_mc instead.

> ...
> 
> Created one function to build the swgroup list.
> 
> static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> {
>     int i;
> 
>     for (i = 0; i < mc->soc->num_clients; i++) {
>         struct tegra_mc_swgroup *sg;
>         bool found = false;
> 
>         list_for_each_entry(sg, &mc->soc->swgroups, head) {
>             if (sg->id == mc->soc->clients[i].swgroup) {
>                 found = true;
>                 break;
>             }
>         }

Can't you use your new tegra_mc_find_swgroup() function here? That way
you could turn it into something slightly more readable:

	unsigned int swgroup = mc->soc->clients[i].swgroup;
	struct tegra_mc_swgroup *group;

	group = tegra_mc_find_swgroup(mc, swgroup);
	if (!group) {
		/* allocates and adds to mc->swgroups */
		group = tegra_mc_add_swgroup(mc, swgroup);
		if (!group)
			return -ENOMEM;
	}

	list_add_tail(&group->list, &mc->swgroups);

where tegra_mc_add_swgroup() is something like this:

	group = devm_kzalloc(mc->dev, sizeof(*group), GFP_KERNEL);
	if (!group)
		return NULL;

	INIT_LIST_HEAD(&group->clients);
	group->mc = mc;
	group->id = id;

I don't like very much how this duplicates information that is already
available in tegra_mc_soc, but I can't think of a better way to couple
the SWGROUP ID with the struct tegra_mc *, so I think we'll have to
proceed with something like the above.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Vince Hsu]

On 01/06/2015 07:36 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
>> On 01/05/2015 11:25 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>> On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
>>>> On 12/24/2014 09:23 PM, Lucas Stach wrote:
>>>>> Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
>>>>>> This patch adds some missing pieces of the rail gaing/ungating sequence that
>>>>>> can improve the stability in theory.
>>>>>>
>>>>>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>>>>>> ---
>>>>>>   drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>   drm/nouveau_platform.h |  3 +++
>>>>>>   2 files changed, 45 insertions(+)
>>>>>>
>>>>>> diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
>>>>>> index 68788b17a45c..527fe2358fc9 100644
>>>>>> --- a/drm/nouveau_platform.c
>>>>>> +++ b/drm/nouveau_platform.c
>>>>>> @@ -25,9 +25,11 @@
>>>>>>   #include <linux/module.h>
>>>>>>   #include <linux/platform_device.h>
>>>>>>   #include <linux/of.h>
>>>>>> +#include <linux/of_platform.h>
>>>>>>   #include <linux/reset.h>
>>>>>>   #include <linux/regulator/consumer.h>
>>>>>>   #include <soc/tegra/fuse.h>
>>>>>> +#include <soc/tegra/mc.h>
>>>>>>   #include <soc/tegra/pmc.h>
>>>>>>   #include "nouveau_drm.h"
>>>>>> @@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
>>>>>>   	reset_control_deassert(gpu->rst);
>>>>>>   	udelay(10);
>>>>>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
>>>>>> +	udelay(10);
>>>>>> +
>>>>>>   	return 0;
>>>>>>   err_clamp:
>>>>>> @@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>>>>>   {
>>>>>>   	int err;
>>>>>> +	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
>>>>>> +	udelay(10);
>>>>>> +
>>>>>> +	err = tegra_powergate_gpu_set_clamping(true);
>>>>>> +	if (err)
>>>>>> +		return err;
>>>>>> +	udelay(10);
>>>>>> +
>>>>>>   	reset_control_assert(gpu->rst);
>>>>>>   	udelay(10);
>>>>>> @@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>>>>>   	return 0;
>>>>>>   }
>>>>>> +static int nouveau_platform_get_mc(struct device *dev,
>>>>>> +		struct tegra_mc **mc, unsigned int *swgroup)
>>>>> Uhm, no. If this is needed this has to be a Tegra MC function and not
>>>>> burried into nouveau code. You are using knowledge about the internal
>>>>> workings of the MC driver here.
>>>>>
>>>>> Also this should probably only take the Dt node pointer as argument and
>>>>> return a something like a tegra_mc_client struct that contains both the
>>>>> MC device pointer and the swgroup so you can pass that to
>>>>> tegra_mc_flush().
>>>> Good idea. I will have something as below in V2 if there is no other
>>>> comments for this.
>>>>
>>>> tegra_mc_client *tegra_mc_find_client(struct device_node *node)
>>>> {
>>>>      ...
>>>>      ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
>>>>      ...
>>>> }
>>>>
>>>> There were some discussion about this few weeks ago. I'm not sure whether we
>>>> have some conclusion/implementation though. Thierry?
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
>>> I don't think client is a good fit here. Flushing is done per SWGROUP
>>> (on all clients of the SWGROUP). So I think we'll want something like:
>>>
>>> 	gpu@0,57000000 {
>>> 		...
>>> 		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
>>> 		...
>>> 	};
>>>
>>> In the DT and return a struct tegra_mc_swgroup along the lines of:
>>>
>>> 	struct tegra_mc_client {
>>> 		unsigned int id;
>>> 		unsigned int swgroup;
>>>
>>> 		struct list_head list;
>>> 	};
>>>
>>> 	struct tegra_mc_swgroup {
>>> 		struct list_head clients;
>>> 		unsigned int id;
>>> 	};
>>>
>>> Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
>>> structures, each representing a memory client pertaining to the
>>> SWGROUP.
>> Based on your suggestion above, I created a struct tegra_mc_swgroup:
>>
>> struct tegra_mc_swgroup {
>>      unsigned int id;
>>      struct tegra_mc *mc;
>>      struct list_head head;
>>      struct list_head clients;
>> };
>>
>> And added the list head in the struct tegra_mc_soc.
>>
>> struct tegra_mc_soc {
>>      struct tegra_mc_client *clients;
>>      unsigned int num_clients;
>>
>>      struct tegra_mc_hr *hr_clients;
>>      unsigned int num_hr_clients;
> Why do you still need these?
This part is added in "[PATCH 2/11] memory: tegra: add mc flush support" 
and
"[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory 
clients"
for the hotreset registers.

>
>>      struct list_head swgroups;
> This doesn't belong in struct tegra_mc_soc because that's meant to be
> static information about the specific variant of the memory-controller.
> Put it in struct tegra_mc instead.
Okay.

>
>> ...
>>
>> Created one function to build the swgroup list.
>>
>> static int tegra_mc_build_swgroup(struct tegra_mc *mc)
>> {
>>      int i;
>>
>>      for (i = 0; i < mc->soc->num_clients; i++) {
>>          struct tegra_mc_swgroup *sg;
>>          bool found = false;
>>
>>          list_for_each_entry(sg, &mc->soc->swgroups, head) {
>>              if (sg->id == mc->soc->clients[i].swgroup) {
>>                  found = true;
>>                  break;
>>              }
>>          }
> Can't you use your new tegra_mc_find_swgroup() function here? That way
> you could turn it into something slightly more readable:
The tegra_mc_build_swgroup() is called during driver probe, and after that
the swgroup list is established and then the tegra_mc_find_swgroup() can 
work.

>
> 	unsigned int swgroup = mc->soc->clients[i].swgroup;
> 	struct tegra_mc_swgroup *group;
>
> 	group = tegra_mc_find_swgroup(mc, swgroup);
The tegra_mc_find_swgroup() is expected to be called outside the mc 
driver and
can only take the device node as argument because other drivers do not have
the mc instance information unless they call tegra_mc_find_swgroup() or
tegra_mc_find_client().


> 	if (!group) {
> 		/* allocates and adds to mc->swgroups */
> 		group = tegra_mc_add_swgroup(mc, swgroup);
> 		if (!group)
> 			return -ENOMEM;
> 	}
>
> 	list_add_tail(&group->list, &mc->swgroups);
>
> where tegra_mc_add_swgroup() is something like this:
>
> 	group = devm_kzalloc(mc->dev, sizeof(*group), GFP_KERNEL);
> 	if (!group)
> 		return NULL;
>
> 	INIT_LIST_HEAD(&group->clients);
> 	group->mc = mc;
> 	group->id = id;
>
> I don't like very much how this duplicates information that is already
> available in tegra_mc_soc, but I can't think of a better way to couple
> the SWGROUP ID with the struct tegra_mc *, so I think we'll have to
> proceed with something like the above.
Yeah.

Thanks,
Vince

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu wrote:
> 
> On 01/06/2015 07:36 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> >>>>Old Signed by an unknown key
> >>>On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> >>>>On 12/24/2014 09:23 PM, Lucas Stach wrote:
> >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> >>>>>>This patch adds some missing pieces of the rail gaing/ungating sequence that
> >>>>>>can improve the stability in theory.
> >>>>>>
> >>>>>>Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> >>>>>>---
> >>>>>>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  drm/nouveau_platform.h |  3 +++
> >>>>>>  2 files changed, 45 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> >>>>>>index 68788b17a45c..527fe2358fc9 100644
> >>>>>>--- a/drm/nouveau_platform.c
> >>>>>>+++ b/drm/nouveau_platform.c
> >>>>>>@@ -25,9 +25,11 @@
> >>>>>>  #include <linux/module.h>
> >>>>>>  #include <linux/platform_device.h>
> >>>>>>  #include <linux/of.h>
> >>>>>>+#include <linux/of_platform.h>
> >>>>>>  #include <linux/reset.h>
> >>>>>>  #include <linux/regulator/consumer.h>
> >>>>>>  #include <soc/tegra/fuse.h>
> >>>>>>+#include <soc/tegra/mc.h>
> >>>>>>  #include <soc/tegra/pmc.h>
> >>>>>>  #include "nouveau_drm.h"
> >>>>>>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> >>>>>>  	reset_control_deassert(gpu->rst);
> >>>>>>  	udelay(10);
> >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> >>>>>>+	udelay(10);
> >>>>>>+
> >>>>>>  	return 0;
> >>>>>>  err_clamp:
> >>>>>>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>>>>>  {
> >>>>>>  	int err;
> >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> >>>>>>+	udelay(10);
> >>>>>>+
> >>>>>>+	err = tegra_powergate_gpu_set_clamping(true);
> >>>>>>+	if (err)
> >>>>>>+		return err;
> >>>>>>+	udelay(10);
> >>>>>>+
> >>>>>>  	reset_control_assert(gpu->rst);
> >>>>>>  	udelay(10);
> >>>>>>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>+static int nouveau_platform_get_mc(struct device *dev,
> >>>>>>+		struct tegra_mc **mc, unsigned int *swgroup)
> >>>>>Uhm, no. If this is needed this has to be a Tegra MC function and not
> >>>>>burried into nouveau code. You are using knowledge about the internal
> >>>>>workings of the MC driver here.
> >>>>>
> >>>>>Also this should probably only take the Dt node pointer as argument and
> >>>>>return a something like a tegra_mc_client struct that contains both the
> >>>>>MC device pointer and the swgroup so you can pass that to
> >>>>>tegra_mc_flush().
> >>>>Good idea. I will have something as below in V2 if there is no other
> >>>>comments for this.
> >>>>
> >>>>tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> >>>>{
> >>>>     ...
> >>>>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
> >>>>     ...
> >>>>}
> >>>>
> >>>>There were some discussion about this few weeks ago. I'm not sure whether we
> >>>>have some conclusion/implementation though. Thierry?
> >>>>
> >>>>http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
> >>>I don't think client is a good fit here. Flushing is done per SWGROUP
> >>>(on all clients of the SWGROUP). So I think we'll want something like:
> >>>
> >>>	gpu@0,57000000 {
> >>>		...
> >>>		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
> >>>		...
> >>>	};
> >>>
> >>>In the DT and return a struct tegra_mc_swgroup along the lines of:
> >>>
> >>>	struct tegra_mc_client {
> >>>		unsigned int id;
> >>>		unsigned int swgroup;
> >>>
> >>>		struct list_head list;
> >>>	};
> >>>
> >>>	struct tegra_mc_swgroup {
> >>>		struct list_head clients;
> >>>		unsigned int id;
> >>>	};
> >>>
> >>>Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
> >>>structures, each representing a memory client pertaining to the
> >>>SWGROUP.
> >>Based on your suggestion above, I created a struct tegra_mc_swgroup:
> >>
> >>struct tegra_mc_swgroup {
> >>     unsigned int id;
> >>     struct tegra_mc *mc;
> >>     struct list_head head;
> >>     struct list_head clients;
> >>};
> >>
> >>And added the list head in the struct tegra_mc_soc.
> >>
> >>struct tegra_mc_soc {
> >>     struct tegra_mc_client *clients;
> >>     unsigned int num_clients;
> >>
> >>     struct tegra_mc_hr *hr_clients;
> >>     unsigned int num_hr_clients;
> >Why do you still need these?
> This part is added in "[PATCH 2/11] memory: tegra: add mc flush support" and
> "[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory
> clients"
> for the hotreset registers.

Right. Perhaps calling this structure tegra_mc_hotreset and the
variables *_hotresets would be a good idea to clarify that they aren't
actually per-client but per-SWGROUP.

> >>Created one function to build the swgroup list.
> >>
> >>static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> >>{
> >>     int i;
> >>
> >>     for (i = 0; i < mc->soc->num_clients; i++) {
> >>         struct tegra_mc_swgroup *sg;
> >>         bool found = false;
> >>
> >>         list_for_each_entry(sg, &mc->soc->swgroups, head) {
> >>             if (sg->id == mc->soc->clients[i].swgroup) {
> >>                 found = true;
> >>                 break;
> >>             }
> >>         }
> >Can't you use your new tegra_mc_find_swgroup() function here? That way
> >you could turn it into something slightly more readable:
> The tegra_mc_build_swgroup() is called during driver probe, and after that
> the swgroup list is established and then the tegra_mc_find_swgroup() can
> work.

But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
search the partial list. The code will be exactly the same, won't it?

> >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> >	struct tegra_mc_swgroup *group;
> >
> >	group = tegra_mc_find_swgroup(mc, swgroup);
> The tegra_mc_find_swgroup() is expected to be called outside the mc driver
> and
> can only take the device node as argument because other drivers do not have
> the mc instance information unless they call tegra_mc_find_swgroup() or
> tegra_mc_find_client().

You can do that by making a low-level function that works on the struct
tegra_mc * and an unsigned int swgroup which is only used internally and
then wrap a public function around it that deals with looking up the
struct tegra_mc * and SWGROUP ID.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Vince Hsu]

On 02:55:52PM Jan 06, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu wrote:
> > 
> > On 01/06/2015 07:36 PM, Thierry Reding wrote:
> > >> Old Signed by an unknown key
> > >
> > >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> > >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> > >>>>Old Signed by an unknown key
> > >>>On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> > >>>>On 12/24/2014 09:23 PM, Lucas Stach wrote:
> > >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> > >>>>>>This patch adds some missing pieces of the rail gaing/ungating sequence that
> > >>>>>>can improve the stability in theory.
> > >>>>>>
> > >>>>>>Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > >>>>>>---
> > >>>>>>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >>>>>>  drm/nouveau_platform.h |  3 +++
> > >>>>>>  2 files changed, 45 insertions(+)
> > >>>>>>
> > >>>>>>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> > >>>>>>index 68788b17a45c..527fe2358fc9 100644
> > >>>>>>--- a/drm/nouveau_platform.c
> > >>>>>>+++ b/drm/nouveau_platform.c
> > >>>>>>@@ -25,9 +25,11 @@
> > >>>>>>  #include <linux/module.h>
> > >>>>>>  #include <linux/platform_device.h>
> > >>>>>>  #include <linux/of.h>
> > >>>>>>+#include <linux/of_platform.h>
> > >>>>>>  #include <linux/reset.h>
> > >>>>>>  #include <linux/regulator/consumer.h>
> > >>>>>>  #include <soc/tegra/fuse.h>
> > >>>>>>+#include <soc/tegra/mc.h>
> > >>>>>>  #include <soc/tegra/pmc.h>
> > >>>>>>  #include "nouveau_drm.h"
> > >>>>>>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> > >>>>>>  	reset_control_deassert(gpu->rst);
> > >>>>>>  	udelay(10);
> > >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> > >>>>>>+	udelay(10);
> > >>>>>>+
> > >>>>>>  	return 0;
> > >>>>>>  err_clamp:
> > >>>>>>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> > >>>>>>  {
> > >>>>>>  	int err;
> > >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> > >>>>>>+	udelay(10);
> > >>>>>>+
> > >>>>>>+	err = tegra_powergate_gpu_set_clamping(true);
> > >>>>>>+	if (err)
> > >>>>>>+		return err;
> > >>>>>>+	udelay(10);
> > >>>>>>+
> > >>>>>>  	reset_control_assert(gpu->rst);
> > >>>>>>  	udelay(10);
> > >>>>>>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> > >>>>>>  	return 0;
> > >>>>>>  }
> > >>>>>>+static int nouveau_platform_get_mc(struct device *dev,
> > >>>>>>+		struct tegra_mc **mc, unsigned int *swgroup)
> > >>>>>Uhm, no. If this is needed this has to be a Tegra MC function and not
> > >>>>>burried into nouveau code. You are using knowledge about the internal
> > >>>>>workings of the MC driver here.
> > >>>>>
> > >>>>>Also this should probably only take the Dt node pointer as argument and
> > >>>>>return a something like a tegra_mc_client struct that contains both the
> > >>>>>MC device pointer and the swgroup so you can pass that to
> > >>>>>tegra_mc_flush().
> > >>>>Good idea. I will have something as below in V2 if there is no other
> > >>>>comments for this.
> > >>>>
> > >>>>tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> > >>>>{
> > >>>>     ...
> > >>>>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
> > >>>>     ...
> > >>>>}
> > >>>>
> > >>>>There were some discussion about this few weeks ago. I'm not sure whether we
> > >>>>have some conclusion/implementation though. Thierry?
> > >>>>
> > >>>>http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
> > >>>I don't think client is a good fit here. Flushing is done per SWGROUP
> > >>>(on all clients of the SWGROUP). So I think we'll want something like:
> > >>>
> > >>>	gpu@0,57000000 {
> > >>>		...
> > >>>		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
> > >>>		...
> > >>>	};
> > >>>
> > >>>In the DT and return a struct tegra_mc_swgroup along the lines of:
> > >>>
> > >>>	struct tegra_mc_client {
> > >>>		unsigned int id;
> > >>>		unsigned int swgroup;
> > >>>
> > >>>		struct list_head list;
> > >>>	};
> > >>>
> > >>>	struct tegra_mc_swgroup {
> > >>>		struct list_head clients;
> > >>>		unsigned int id;
> > >>>	};
> > >>>
> > >>>Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
> > >>>structures, each representing a memory client pertaining to the
> > >>>SWGROUP.
> > >>Based on your suggestion above, I created a struct tegra_mc_swgroup:
> > >>
> > >>struct tegra_mc_swgroup {
> > >>     unsigned int id;
> > >>     struct tegra_mc *mc;
> > >>     struct list_head head;
> > >>     struct list_head clients;
> > >>};
> > >>
> > >>And added the list head in the struct tegra_mc_soc.
> > >>
> > >>struct tegra_mc_soc {
> > >>     struct tegra_mc_client *clients;
> > >>     unsigned int num_clients;
> > >>
> > >>     struct tegra_mc_hr *hr_clients;
> > >>     unsigned int num_hr_clients;
> > >Why do you still need these?
> > This part is added in "[PATCH 2/11] memory: tegra: add mc flush support" and
> > "[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory
> > clients"
> > for the hotreset registers.
> 
> Right. Perhaps calling this structure tegra_mc_hotreset and the
> variables *_hotresets would be a good idea to clarify that they aren't
> actually per-client but per-SWGROUP.
Okay.

> 
> > >>Created one function to build the swgroup list.
> > >>
> > >>static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> > >>{
> > >>     int i;
> > >>
> > >>     for (i = 0; i < mc->soc->num_clients; i++) {
> > >>         struct tegra_mc_swgroup *sg;
> > >>         bool found = false;
> > >>
> > >>         list_for_each_entry(sg, &mc->soc->swgroups, head) {
> > >>             if (sg->id == mc->soc->clients[i].swgroup) {
> > >>                 found = true;
> > >>                 break;
> > >>             }
> > >>         }
> > >Can't you use your new tegra_mc_find_swgroup() function here? That way
> > >you could turn it into something slightly more readable:
> > The tegra_mc_build_swgroup() is called during driver probe, and after that
> > the swgroup list is established and then the tegra_mc_find_swgroup() can
> > work.
> 
> But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
> search the partial list. The code will be exactly the same, won't it?
That depends on how you define the tegra_mc_find_swgroup().

> 
> > >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> > >	struct tegra_mc_swgroup *group;
> > >
> > >	group = tegra_mc_find_swgroup(mc, swgroup);
> > The tegra_mc_find_swgroup() is expected to be called outside the mc driver
> > and
> > can only take the device node as argument because other drivers do not have
> > the mc instance information unless they call tegra_mc_find_swgroup() or
> > tegra_mc_find_client().
> 
> You can do that by making a low-level function that works on the struct
> tegra_mc * and an unsigned int swgroup which is only used internally and
> then wrap a public function around it that deals with looking up the
> struct tegra_mc * and SWGROUP ID.
Now the tegra_mc_find_swgroup() looks like below:

struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node)           
{                                                                                  
    struct of_phandle_args args;                                                   
    struct platform_device *pdev;                                                  
    struct tegra_mc *mc;                                                           
    struct tegra_mc_swgroup *sg;                                                   
    int ret;                                                                       
                                                                                   
    ret = of_parse_phandle_with_fixed_args(node, "nvidia,swgroup",                 
                1, 0, &args);                                                      
    if (ret)                                                                       
        return ERR_PTR(ret);                                                       
                                                                                   
    pdev = of_find_device_by_node(args.np);                                        
    if (!pdev)                                                                     
        return NULL;                                                               
                                                                                   
    mc = platform_get_drvdata(pdev);                                               
    if (!mc)                                                                       
        return NULL;                                                               
                                                                                   
    list_for_each_entry(sg, &mc->soc->swgroups, head) { 
        if (sg->id == args.args[0])                                                
            return sg;                                                             
    }                                                                              
                                                                                   
    return NULL;                                                                   
}                                                                                  
EXPORT_SYMBOL(tegra_mc_find_swgroup);     

I will add a find_swgroup() as internal funcion and add the add_swgroup()
per your suggestion. And keep the tegra_mc_find_swgroup() public. Is that
okay for you?

Thanks,
Vince 

Re: [PATCH nouveau 06/11] platform: complete the power up/down sequence

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 10447 bytes --]

On Tue, Jan 06, 2015 at 10:19:49PM +0800, Vince Hsu wrote:
> On 02:55:52PM Jan 06, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jan 06, 2015 at 08:13:01PM +0800, Vince Hsu wrote:
> > > 
> > > On 01/06/2015 07:36 PM, Thierry Reding wrote:
> > > >> Old Signed by an unknown key
> > > >
> > > >On Tue, Jan 06, 2015 at 05:34:01PM +0800, Vince Hsu wrote:
> > > >>On 01/05/2015 11:25 PM, Thierry Reding wrote:
> > > >>>>Old Signed by an unknown key
> > > >>>On Thu, Dec 25, 2014 at 10:42:58AM +0800, Vince Hsu wrote:
> > > >>>>On 12/24/2014 09:23 PM, Lucas Stach wrote:
> > > >>>>>Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
> > > >>>>>>This patch adds some missing pieces of the rail gaing/ungating sequence that
> > > >>>>>>can improve the stability in theory.
> > > >>>>>>
> > > >>>>>>Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> > > >>>>>>---
> > > >>>>>>  drm/nouveau_platform.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > >>>>>>  drm/nouveau_platform.h |  3 +++
> > > >>>>>>  2 files changed, 45 insertions(+)
> > > >>>>>>
> > > >>>>>>diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
> > > >>>>>>index 68788b17a45c..527fe2358fc9 100644
> > > >>>>>>--- a/drm/nouveau_platform.c
> > > >>>>>>+++ b/drm/nouveau_platform.c
> > > >>>>>>@@ -25,9 +25,11 @@
> > > >>>>>>  #include <linux/module.h>
> > > >>>>>>  #include <linux/platform_device.h>
> > > >>>>>>  #include <linux/of.h>
> > > >>>>>>+#include <linux/of_platform.h>
> > > >>>>>>  #include <linux/reset.h>
> > > >>>>>>  #include <linux/regulator/consumer.h>
> > > >>>>>>  #include <soc/tegra/fuse.h>
> > > >>>>>>+#include <soc/tegra/mc.h>
> > > >>>>>>  #include <soc/tegra/pmc.h>
> > > >>>>>>  #include "nouveau_drm.h"
> > > >>>>>>@@ -61,6 +63,9 @@ static int nouveau_platform_power_up(struct nouveau_platform_gpu *gpu)
> > > >>>>>>  	reset_control_deassert(gpu->rst);
> > > >>>>>>  	udelay(10);
> > > >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, false);
> > > >>>>>>+	udelay(10);
> > > >>>>>>+
> > > >>>>>>  	return 0;
> > > >>>>>>  err_clamp:
> > > >>>>>>@@ -77,6 +82,14 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> > > >>>>>>  {
> > > >>>>>>  	int err;
> > > >>>>>>+	tegra_mc_flush(gpu->mc, gpu->swgroup, true);
> > > >>>>>>+	udelay(10);
> > > >>>>>>+
> > > >>>>>>+	err = tegra_powergate_gpu_set_clamping(true);
> > > >>>>>>+	if (err)
> > > >>>>>>+		return err;
> > > >>>>>>+	udelay(10);
> > > >>>>>>+
> > > >>>>>>  	reset_control_assert(gpu->rst);
> > > >>>>>>  	udelay(10);
> > > >>>>>>@@ -91,6 +104,31 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
> > > >>>>>>  	return 0;
> > > >>>>>>  }
> > > >>>>>>+static int nouveau_platform_get_mc(struct device *dev,
> > > >>>>>>+		struct tegra_mc **mc, unsigned int *swgroup)
> > > >>>>>Uhm, no. If this is needed this has to be a Tegra MC function and not
> > > >>>>>burried into nouveau code. You are using knowledge about the internal
> > > >>>>>workings of the MC driver here.
> > > >>>>>
> > > >>>>>Also this should probably only take the Dt node pointer as argument and
> > > >>>>>return a something like a tegra_mc_client struct that contains both the
> > > >>>>>MC device pointer and the swgroup so you can pass that to
> > > >>>>>tegra_mc_flush().
> > > >>>>Good idea. I will have something as below in V2 if there is no other
> > > >>>>comments for this.
> > > >>>>
> > > >>>>tegra_mc_client *tegra_mc_find_client(struct device_node *node)
> > > >>>>{
> > > >>>>     ...
> > > >>>>     ret = of_parse_phandle_with_args(node, "nvidia,memory-client", ...)
> > > >>>>     ...
> > > >>>>}
> > > >>>>
> > > >>>>There were some discussion about this few weeks ago. I'm not sure whether we
> > > >>>>have some conclusion/implementation though. Thierry?
> > > >>>>
> > > >>>>http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/308703.html
> > > >>>I don't think client is a good fit here. Flushing is done per SWGROUP
> > > >>>(on all clients of the SWGROUP). So I think we'll want something like:
> > > >>>
> > > >>>	gpu@0,57000000 {
> > > >>>		...
> > > >>>		nvidia,swgroup = <&mc TEGRA_SWGROUP_GPU>;
> > > >>>		...
> > > >>>	};
> > > >>>
> > > >>>In the DT and return a struct tegra_mc_swgroup along the lines of:
> > > >>>
> > > >>>	struct tegra_mc_client {
> > > >>>		unsigned int id;
> > > >>>		unsigned int swgroup;
> > > >>>
> > > >>>		struct list_head list;
> > > >>>	};
> > > >>>
> > > >>>	struct tegra_mc_swgroup {
> > > >>>		struct list_head clients;
> > > >>>		unsigned int id;
> > > >>>	};
> > > >>>
> > > >>>Where tegra_mc_swgroup.clients is a list of struct tegra_mc_client
> > > >>>structures, each representing a memory client pertaining to the
> > > >>>SWGROUP.
> > > >>Based on your suggestion above, I created a struct tegra_mc_swgroup:
> > > >>
> > > >>struct tegra_mc_swgroup {
> > > >>     unsigned int id;
> > > >>     struct tegra_mc *mc;
> > > >>     struct list_head head;
> > > >>     struct list_head clients;
> > > >>};
> > > >>
> > > >>And added the list head in the struct tegra_mc_soc.
> > > >>
> > > >>struct tegra_mc_soc {
> > > >>     struct tegra_mc_client *clients;
> > > >>     unsigned int num_clients;
> > > >>
> > > >>     struct tegra_mc_hr *hr_clients;
> > > >>     unsigned int num_hr_clients;
> > > >Why do you still need these?
> > > This part is added in "[PATCH 2/11] memory: tegra: add mc flush support" and
> > > "[PATCH 3/11] memory: tegra: add flush operation for Tegra124 memory
> > > clients"
> > > for the hotreset registers.
> > 
> > Right. Perhaps calling this structure tegra_mc_hotreset and the
> > variables *_hotresets would be a good idea to clarify that they aren't
> > actually per-client but per-SWGROUP.
> Okay.
> 
> > 
> > > >>Created one function to build the swgroup list.
> > > >>
> > > >>static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> > > >>{
> > > >>     int i;
> > > >>
> > > >>     for (i = 0; i < mc->soc->num_clients; i++) {
> > > >>         struct tegra_mc_swgroup *sg;
> > > >>         bool found = false;
> > > >>
> > > >>         list_for_each_entry(sg, &mc->soc->swgroups, head) {
> > > >>             if (sg->id == mc->soc->clients[i].swgroup) {
> > > >>                 found = true;
> > > >>                 break;
> > > >>             }
> > > >>         }
> > > >Can't you use your new tegra_mc_find_swgroup() function here? That way
> > > >you could turn it into something slightly more readable:
> > > The tegra_mc_build_swgroup() is called during driver probe, and after that
> > > the swgroup list is established and then the tegra_mc_find_swgroup() can
> > > work.
> > 
> > But you can reuse tegra_mc_find_swgroup() instead of open-coding it to
> > search the partial list. The code will be exactly the same, won't it?
> That depends on how you define the tegra_mc_find_swgroup().
> 
> > 
> > > >	unsigned int swgroup = mc->soc->clients[i].swgroup;
> > > >	struct tegra_mc_swgroup *group;
> > > >
> > > >	group = tegra_mc_find_swgroup(mc, swgroup);
> > > The tegra_mc_find_swgroup() is expected to be called outside the mc driver
> > > and
> > > can only take the device node as argument because other drivers do not have
> > > the mc instance information unless they call tegra_mc_find_swgroup() or
> > > tegra_mc_find_client().
> > 
> > You can do that by making a low-level function that works on the struct
> > tegra_mc * and an unsigned int swgroup which is only used internally and
> > then wrap a public function around it that deals with looking up the
> > struct tegra_mc * and SWGROUP ID.
> Now the tegra_mc_find_swgroup() looks like below:
> 
> struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node)           
> {                                                                                  
>     struct of_phandle_args args;                                                   
>     struct platform_device *pdev;                                                  
>     struct tegra_mc *mc;                                                           
>     struct tegra_mc_swgroup *sg;                                                   
>     int ret;                                                                       
>                                                                                    
>     ret = of_parse_phandle_with_fixed_args(node, "nvidia,swgroup",                 
>                 1, 0, &args);                                                      
>     if (ret)                                                                       
>         return ERR_PTR(ret);                                                       
>                                                                                    
>     pdev = of_find_device_by_node(args.np);                                        
>     if (!pdev)                                                                     
>         return NULL;                                                               
>                                                                                    
>     mc = platform_get_drvdata(pdev);                                               
>     if (!mc)                                                                       
>         return NULL;                                                               
>                                                                                    
>     list_for_each_entry(sg, &mc->soc->swgroups, head) { 
>         if (sg->id == args.args[0])                                                
>             return sg;                                                             
>     }                                                                              
>                                                                                    
>     return NULL;                                                                   
> }                                                                                  
> EXPORT_SYMBOL(tegra_mc_find_swgroup);     
> 
> I will add a find_swgroup() as internal funcion and add the add_swgroup()
> per your suggestion. And keep the tegra_mc_find_swgroup() public. Is that
> okay for you?

Sounds good.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH nouveau 07/11] instmem: make nv50_instmem_priv public

[Vince Hsu]

The GK20A needs to create a dummy instemem subdev to avoid suspend/resume
problem. So make the nv50_instmem_priv non-static for now.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/core/subdev/instmem/nv50.h |  1 +
 nvkm/subdev/instmem/nv50.c     |  9 ++-------
 nvkm/subdev/instmem/nv50.h     | 14 ++++++++++++++
 3 files changed, 17 insertions(+), 7 deletions(-)
 create mode 120000 drm/core/subdev/instmem/nv50.h
 create mode 100644 nvkm/subdev/instmem/nv50.h

diff --git a/drm/core/subdev/instmem/nv50.h b/drm/core/subdev/instmem/nv50.h
new file mode 120000
index 000000000000..043e22aee880
--- /dev/null
+++ b/drm/core/subdev/instmem/nv50.h
@@ -0,0 +1 @@
+../../../../nvkm/subdev/instmem/nv50.h
\ No newline at end of file
diff --git a/nvkm/subdev/instmem/nv50.c b/nvkm/subdev/instmem/nv50.c
index 7cb3b098a08d..66428b1c2394 100644
--- a/nvkm/subdev/instmem/nv50.c
+++ b/nvkm/subdev/instmem/nv50.c
@@ -25,14 +25,9 @@
 #include <subdev/fb.h>
 #include <core/mm.h>
 
+#include "nv50.h"
 #include "priv.h"
 
-struct nv50_instmem_priv {
-	struct nouveau_instmem base;
-	spinlock_t lock;
-	u64 addr;
-};
-
 struct nv50_instobj_priv {
 	struct nouveau_instobj base;
 	struct nouveau_mem *mem;
@@ -117,7 +112,7 @@ nv50_instobj_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
 	return 0;
 }
 
-static struct nouveau_instobj_impl
+struct nouveau_instobj_impl
 nv50_instobj_oclass = {
 	.base.ofuncs = &(struct nouveau_ofuncs) {
 		.ctor = nv50_instobj_ctor,
diff --git a/nvkm/subdev/instmem/nv50.h b/nvkm/subdev/instmem/nv50.h
new file mode 100644
index 000000000000..bff03e3807ea
--- /dev/null
+++ b/nvkm/subdev/instmem/nv50.h
@@ -0,0 +1,14 @@
+#ifndef __NVKM_INSTMEM_NV50_H__
+#define __NVKM_INSTMEM_NV50_H__
+
+#include "priv.h"
+
+struct nv50_instmem_priv {
+	struct nouveau_instmem base;
+	spinlock_t lock;
+	u64 addr;
+};
+
+extern struct nouveau_instobj_impl nv50_instobj_oclass;
+
+#endif
-- 
1.9.1

[PATCH nouveau 08/11] instmem: add dummy support for GK20A

[Vince Hsu]

This is a workaround to avoid the instmem backup/restore during the suspend
and resume process in nv50 instemem driver.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/Kbuild                    |  1 +
 nvkm/engine/device/nve0.c     |  2 +-
 nvkm/include/subdev/instmem.h |  1 +
 nvkm/subdev/instmem/gk20a.c   | 70 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 nvkm/subdev/instmem/gk20a.c

diff --git a/drm/Kbuild b/drm/Kbuild
index 6461e3565afe..ea40cd653c7c 100644
--- a/drm/Kbuild
+++ b/drm/Kbuild
@@ -176,6 +176,7 @@ nouveau-y += core/subdev/instmem/base.o
 nouveau-y += core/subdev/instmem/nv04.o
 nouveau-y += core/subdev/instmem/nv40.o
 nouveau-y += core/subdev/instmem/nv50.o
+nouveau-y += core/subdev/instmem/gk20a.o
 nouveau-y += core/subdev/ltc/base.o
 nouveau-y += core/subdev/ltc/gf100.o
 nouveau-y += core/subdev/ltc/gk104.o
diff --git a/nvkm/engine/device/nve0.c b/nvkm/engine/device/nve0.c
index 732922690653..fcbdc5259c7c 100644
--- a/nvkm/engine/device/nve0.c
+++ b/nvkm/engine/device/nve0.c
@@ -170,7 +170,7 @@ nve0_identify(struct nouveau_device *device)
 		device->oclass[NVDEV_SUBDEV_FB     ] =  gk20a_fb_oclass;
 		device->oclass[NVDEV_SUBDEV_LTC    ] =  gk104_ltc_oclass;
 		device->oclass[NVDEV_SUBDEV_IBUS   ] = &gk20a_ibus_oclass;
-		device->oclass[NVDEV_SUBDEV_INSTMEM] = nv50_instmem_oclass;
+		device->oclass[NVDEV_SUBDEV_INSTMEM] = gk20a_instmem_oclass;
 		device->oclass[NVDEV_SUBDEV_VM     ] = &nvc0_vmmgr_oclass;
 		device->oclass[NVDEV_SUBDEV_BAR    ] = &gk20a_bar_oclass;
 		device->oclass[NVDEV_ENGINE_DMAOBJ ] =  nvd0_dmaeng_oclass;
diff --git a/nvkm/include/subdev/instmem.h b/nvkm/include/subdev/instmem.h
index c1df26f3230c..6264660bedce 100644
--- a/nvkm/include/subdev/instmem.h
+++ b/nvkm/include/subdev/instmem.h
@@ -48,5 +48,6 @@ nouveau_instmem(void *obj)
 extern struct nouveau_oclass *nv04_instmem_oclass;
 extern struct nouveau_oclass *nv40_instmem_oclass;
 extern struct nouveau_oclass *nv50_instmem_oclass;
+extern struct nouveau_oclass *gk20a_instmem_oclass;
 
 #endif
diff --git a/nvkm/subdev/instmem/gk20a.c b/nvkm/subdev/instmem/gk20a.c
new file mode 100644
index 000000000000..5e072d6e743f
--- /dev/null
+++ b/nvkm/subdev/instmem/gk20a.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+#include "nv50.h"
+#include "priv.h"
+
+static int
+gk20a_instmem_fini(struct nouveau_object *object, bool suspend)
+{
+	struct nouveau_instmem *imem = (void *)object;
+
+	return nouveau_subdev_fini(&imem->base, suspend);
+}
+
+static int
+gk20a_instmem_init(struct nouveau_object *object)
+{
+	struct nouveau_instmem *imem = (void *)object;
+
+	return nouveau_subdev_init(&imem->base);
+}
+
+static int
+gk20a_instmem_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
+		  struct nouveau_oclass *oclass, void *data, u32 size,
+		  struct nouveau_object **pobject)
+{
+	struct nv50_instmem_priv *priv;
+	int ret;
+
+	ret = nouveau_instmem_create(parent, engine, oclass, &priv);
+	*pobject = nv_object(priv);
+	if (ret)
+		return ret;
+
+	spin_lock_init(&priv->lock);
+	return 0;
+}
+
+struct nouveau_oclass *
+gk20a_instmem_oclass = &(struct nouveau_instmem_impl) {
+	.base.handle = NV_SUBDEV(INSTMEM, 0x50),
+	.base.ofuncs = &(struct nouveau_ofuncs) {
+		.ctor = gk20a_instmem_ctor,
+		.dtor = _nouveau_instmem_dtor,
+		.init = gk20a_instmem_init,
+		.fini = gk20a_instmem_fini,
+	},
+	.instobj = &nv50_instobj_oclass.base,
+}.base;
-- 
1.9.1

Re: [Nouveau] [PATCH nouveau 08/11] instmem: add dummy support for GK20A

[Ilia Mirkin]

On Tue, Dec 23, 2014 at 5:40 AM, Vince Hsu <vinceh@nvidia.com> wrote:
> This is a workaround to avoid the instmem backup/restore during the suspend
> and resume process in nv50 instemem driver.
>
> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drm/Kbuild                    |  1 +
>  nvkm/engine/device/nve0.c     |  2 +-
>  nvkm/include/subdev/instmem.h |  1 +
>  nvkm/subdev/instmem/gk20a.c   | 70 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 73 insertions(+), 1 deletion(-)
>  create mode 100644 nvkm/subdev/instmem/gk20a.c
>
> diff --git a/drm/Kbuild b/drm/Kbuild
> index 6461e3565afe..ea40cd653c7c 100644
> --- a/drm/Kbuild
> +++ b/drm/Kbuild
> @@ -176,6 +176,7 @@ nouveau-y += core/subdev/instmem/base.o
>  nouveau-y += core/subdev/instmem/nv04.o
>  nouveau-y += core/subdev/instmem/nv40.o
>  nouveau-y += core/subdev/instmem/nv50.o
> +nouveau-y += core/subdev/instmem/gk20a.o
>  nouveau-y += core/subdev/ltc/base.o
>  nouveau-y += core/subdev/ltc/gf100.o
>  nouveau-y += core/subdev/ltc/gk104.o
> diff --git a/nvkm/engine/device/nve0.c b/nvkm/engine/device/nve0.c
> index 732922690653..fcbdc5259c7c 100644
> --- a/nvkm/engine/device/nve0.c
> +++ b/nvkm/engine/device/nve0.c
> @@ -170,7 +170,7 @@ nve0_identify(struct nouveau_device *device)
>                 device->oclass[NVDEV_SUBDEV_FB     ] =  gk20a_fb_oclass;
>                 device->oclass[NVDEV_SUBDEV_LTC    ] =  gk104_ltc_oclass;
>                 device->oclass[NVDEV_SUBDEV_IBUS   ] = &gk20a_ibus_oclass;
> -               device->oclass[NVDEV_SUBDEV_INSTMEM] = nv50_instmem_oclass;
> +               device->oclass[NVDEV_SUBDEV_INSTMEM] = gk20a_instmem_oclass;
>                 device->oclass[NVDEV_SUBDEV_VM     ] = &nvc0_vmmgr_oclass;
>                 device->oclass[NVDEV_SUBDEV_BAR    ] = &gk20a_bar_oclass;
>                 device->oclass[NVDEV_ENGINE_DMAOBJ ] =  nvd0_dmaeng_oclass;
> diff --git a/nvkm/include/subdev/instmem.h b/nvkm/include/subdev/instmem.h
> index c1df26f3230c..6264660bedce 100644
> --- a/nvkm/include/subdev/instmem.h
> +++ b/nvkm/include/subdev/instmem.h
> @@ -48,5 +48,6 @@ nouveau_instmem(void *obj)
>  extern struct nouveau_oclass *nv04_instmem_oclass;
>  extern struct nouveau_oclass *nv40_instmem_oclass;
>  extern struct nouveau_oclass *nv50_instmem_oclass;
> +extern struct nouveau_oclass *gk20a_instmem_oclass;
>
>  #endif
> diff --git a/nvkm/subdev/instmem/gk20a.c b/nvkm/subdev/instmem/gk20a.c
> new file mode 100644
> index 000000000000..5e072d6e743f
> --- /dev/null
> +++ b/nvkm/subdev/instmem/gk20a.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + */
> +
> +#include "nv50.h"

I'm confused... what exactly depends on nv50_instmem_priv here? Why
not just create a gk20a_instmem_priv and leave the nv50 one alone?

> +#include "priv.h"
> +
> +static int
> +gk20a_instmem_fini(struct nouveau_object *object, bool suspend)
> +{
> +       struct nouveau_instmem *imem = (void *)object;
> +
> +       return nouveau_subdev_fini(&imem->base, suspend);
> +}
> +
> +static int
> +gk20a_instmem_init(struct nouveau_object *object)
> +{
> +       struct nouveau_instmem *imem = (void *)object;
> +
> +       return nouveau_subdev_init(&imem->base);
> +}

I think the style is to just link those up directly in the class
definition when they're trivial like that, i.e. point them at
_nouveau_subdev_init and such.

> +
> +static int
> +gk20a_instmem_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
> +                 struct nouveau_oclass *oclass, void *data, u32 size,
> +                 struct nouveau_object **pobject)
> +{
> +       struct nv50_instmem_priv *priv;
> +       int ret;
> +
> +       ret = nouveau_instmem_create(parent, engine, oclass, &priv);
> +       *pobject = nv_object(priv);
> +       if (ret)
> +               return ret;
> +
> +       spin_lock_init(&priv->lock);
> +       return 0;
> +}
> +
> +struct nouveau_oclass *
> +gk20a_instmem_oclass = &(struct nouveau_instmem_impl) {
> +       .base.handle = NV_SUBDEV(INSTMEM, 0x50),
> +       .base.ofuncs = &(struct nouveau_ofuncs) {
> +               .ctor = gk20a_instmem_ctor,
> +               .dtor = _nouveau_instmem_dtor,
> +               .init = gk20a_instmem_init,
> +               .fini = gk20a_instmem_fini,
> +       },
> +       .instobj = &nv50_instobj_oclass.base,
> +}.base;
> --
> 1.9.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH nouveau 08/11] instmem: add dummy support for GK20A

[Vince Hsu]

Hi,

On 12/24/2014 12:39 AM, Ilia Mirkin wrote:
> On Tue, Dec 23, 2014 at 5:40 AM, Vince Hsu <vinceh@nvidia.com> wrote:
>> This is a workaround to avoid the instmem backup/restore during the suspend
>> and resume process in nv50 instemem driver.
>>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>> ---
>>   drm/Kbuild                    |  1 +
>>   nvkm/engine/device/nve0.c     |  2 +-
>>   nvkm/include/subdev/instmem.h |  1 +
>>   nvkm/subdev/instmem/gk20a.c   | 70 +++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 73 insertions(+), 1 deletion(-)
>>   create mode 100644 nvkm/subdev/instmem/gk20a.c
>>
>> diff --git a/drm/Kbuild b/drm/Kbuild
>> index 6461e3565afe..ea40cd653c7c 100644
>> --- a/drm/Kbuild
>> +++ b/drm/Kbuild
>> @@ -176,6 +176,7 @@ nouveau-y += core/subdev/instmem/base.o
>>   nouveau-y += core/subdev/instmem/nv04.o
>>   nouveau-y += core/subdev/instmem/nv40.o
>>   nouveau-y += core/subdev/instmem/nv50.o
>> +nouveau-y += core/subdev/instmem/gk20a.o
>>   nouveau-y += core/subdev/ltc/base.o
>>   nouveau-y += core/subdev/ltc/gf100.o
>>   nouveau-y += core/subdev/ltc/gk104.o
>> diff --git a/nvkm/engine/device/nve0.c b/nvkm/engine/device/nve0.c
>> index 732922690653..fcbdc5259c7c 100644
>> --- a/nvkm/engine/device/nve0.c
>> +++ b/nvkm/engine/device/nve0.c
>> @@ -170,7 +170,7 @@ nve0_identify(struct nouveau_device *device)
>>                  device->oclass[NVDEV_SUBDEV_FB     ] =  gk20a_fb_oclass;
>>                  device->oclass[NVDEV_SUBDEV_LTC    ] =  gk104_ltc_oclass;
>>                  device->oclass[NVDEV_SUBDEV_IBUS   ] = &gk20a_ibus_oclass;
>> -               device->oclass[NVDEV_SUBDEV_INSTMEM] = nv50_instmem_oclass;
>> +               device->oclass[NVDEV_SUBDEV_INSTMEM] = gk20a_instmem_oclass;
>>                  device->oclass[NVDEV_SUBDEV_VM     ] = &nvc0_vmmgr_oclass;
>>                  device->oclass[NVDEV_SUBDEV_BAR    ] = &gk20a_bar_oclass;
>>                  device->oclass[NVDEV_ENGINE_DMAOBJ ] =  nvd0_dmaeng_oclass;
>> diff --git a/nvkm/include/subdev/instmem.h b/nvkm/include/subdev/instmem.h
>> index c1df26f3230c..6264660bedce 100644
>> --- a/nvkm/include/subdev/instmem.h
>> +++ b/nvkm/include/subdev/instmem.h
>> @@ -48,5 +48,6 @@ nouveau_instmem(void *obj)
>>   extern struct nouveau_oclass *nv04_instmem_oclass;
>>   extern struct nouveau_oclass *nv40_instmem_oclass;
>>   extern struct nouveau_oclass *nv50_instmem_oclass;
>> +extern struct nouveau_oclass *gk20a_instmem_oclass;
>>
>>   #endif
>> diff --git a/nvkm/subdev/instmem/gk20a.c b/nvkm/subdev/instmem/gk20a.c
>> new file mode 100644
>> index 000000000000..5e072d6e743f
>> --- /dev/null
>> +++ b/nvkm/subdev/instmem/gk20a.c
>> @@ -0,0 +1,70 @@
>> +/*
>> + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "nv50.h"
> I'm confused... what exactly depends on nv50_instmem_priv here? Why
> not just create a gk20a_instmem_priv and leave the nv50 one alone?
You're right. Will fix in the next version.
>
>> +#include "priv.h"
>> +
>> +static int
>> +gk20a_instmem_fini(struct nouveau_object *object, bool suspend)
>> +{
>> +       struct nouveau_instmem *imem = (void *)object;
>> +
>> +       return nouveau_subdev_fini(&imem->base, suspend);
>> +}
>> +
>> +static int
>> +gk20a_instmem_init(struct nouveau_object *object)
>> +{
>> +       struct nouveau_instmem *imem = (void *)object;
>> +
>> +       return nouveau_subdev_init(&imem->base);
>> +}
> I think the style is to just link those up directly in the class
> definition when they're trivial like that, i.e. point them at
> _nouveau_subdev_init and such.
Thanks for the hint. Will fix. :)

>
>> +
>> +static int
>> +gk20a_instmem_ctor(struct nouveau_object *parent, struct nouveau_object *engine,
>> +                 struct nouveau_oclass *oclass, void *data, u32 size,
>> +                 struct nouveau_object **pobject)
>> +{
>> +       struct nv50_instmem_priv *priv;
>> +       int ret;
>> +
>> +       ret = nouveau_instmem_create(parent, engine, oclass, &priv);
>> +       *pobject = nv_object(priv);
>> +       if (ret)
>> +               return ret;
>> +
>> +       spin_lock_init(&priv->lock);
>> +       return 0;
>> +}
>> +
>> +struct nouveau_oclass *
>> +gk20a_instmem_oclass = &(struct nouveau_instmem_impl) {
>> +       .base.handle = NV_SUBDEV(INSTMEM, 0x50),
>> +       .base.ofuncs = &(struct nouveau_ofuncs) {
>> +               .ctor = gk20a_instmem_ctor,
>> +               .dtor = _nouveau_instmem_dtor,
>> +               .init = gk20a_instmem_init,
>> +               .fini = gk20a_instmem_fini,
>> +       },
>> +       .instobj = &nv50_instobj_oclass.base,
>> +}.base;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Vince Hsu]

This patch adds some checks in the suspend/resume functions to distinguish
the dGPU and mobile GPU and exports some variables/functions so that the
nouveau platform device can reuse them.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/nouveau_drm.c | 16 +++++++++++-----
 drm/nouveau_drm.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drm/nouveau_drm.c b/drm/nouveau_drm.c
index afb93bb72f97..0ed99ef80211 100644
--- a/drm/nouveau_drm.c
+++ b/drm/nouveau_drm.c
@@ -72,6 +72,7 @@ module_param_named(modeset, nouveau_modeset, int, 0400);
 
 MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1)");
 int nouveau_runtime_pm = -1;
+EXPORT_SYMBOL(nouveau_runtime_pm);
 module_param_named(runpm, nouveau_runtime_pm, int, 0400);
 
 static struct drm_driver driver_stub;
@@ -543,7 +544,7 @@ nouveau_drm_remove(struct pci_dev *pdev)
 	nouveau_drm_device_remove(dev);
 }
 
-static int
+int
 nouveau_do_suspend(struct drm_device *dev, bool runtime)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
@@ -559,8 +560,10 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
 			return ret;
 	}
 
-	NV_INFO(drm, "evicting buffers...\n");
-	ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
+	if (dev->pdev) {
+		NV_INFO(drm, "evicting buffers...\n");
+		ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
+	}
 
 	NV_INFO(drm, "waiting for kernel channels to go idle...\n");
 	if (drm->cechan) {
@@ -612,8 +615,9 @@ fail_display:
 	}
 	return ret;
 }
+EXPORT_SYMBOL(nouveau_do_suspend);
 
-static int
+int
 nouveau_do_resume(struct drm_device *dev, bool runtime)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
@@ -635,7 +639,8 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
 		nvif_client_resume(&cli->base);
 	}
 
-	nouveau_run_vbios_init(dev);
+	if (dev->pdev)
+		nouveau_run_vbios_init(dev);
 
 	if (dev->mode_config.num_crtc) {
 		NV_INFO(drm, "resuming display...\n");
@@ -646,6 +651,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
 
 	return 0;
 }
+EXPORT_SYMBOL(nouveau_do_resume);
 
 int
 nouveau_pmops_suspend(struct device *dev)
diff --git a/drm/nouveau_drm.h b/drm/nouveau_drm.h
index 8ae36f265fb8..897d295dd1e3 100644
--- a/drm/nouveau_drm.h
+++ b/drm/nouveau_drm.h
@@ -177,6 +177,8 @@ nouveau_drm(struct drm_device *dev)
 
 int nouveau_pmops_suspend(struct device *);
 int nouveau_pmops_resume(struct device *);
+int nouveau_do_suspend(struct drm_device *dev, bool runtime);
+int nouveau_do_resume(struct drm_device *dev, bool runtime);
 
 #define nouveau_platform_device_create(p, u)                                   \
 	nouveau_platform_device_create_(p, sizeof(**u), (void **)u)
-- 
1.9.1

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Emil Velikov]

On 23/12/14 10:40, Vince Hsu wrote:
> This patch adds some checks in the suspend/resume functions to distinguish
> the dGPU and mobile GPU and exports some variables/functions so that the
> nouveau platform device can reuse them.
> 
Hi Vince,

Afaiu one needs to export a symbol as it's used by another module or
subsystem. With the follow up two patches you are not doing either one,
so I'd assume that you can just omit the EXPORT_* changes.

I could be wrong though :-)

Cheers
Emil


> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> ---
>  drm/nouveau_drm.c | 16 +++++++++++-----
>  drm/nouveau_drm.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drm/nouveau_drm.c b/drm/nouveau_drm.c
> index afb93bb72f97..0ed99ef80211 100644
> --- a/drm/nouveau_drm.c
> +++ b/drm/nouveau_drm.c
> @@ -72,6 +72,7 @@ module_param_named(modeset, nouveau_modeset, int, 0400);
>  
>  MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1)");
>  int nouveau_runtime_pm = -1;
> +EXPORT_SYMBOL(nouveau_runtime_pm);
>  module_param_named(runpm, nouveau_runtime_pm, int, 0400);
>  
>  static struct drm_driver driver_stub;
> @@ -543,7 +544,7 @@ nouveau_drm_remove(struct pci_dev *pdev)
>  	nouveau_drm_device_remove(dev);
>  }
>  
> -static int
> +int
>  nouveau_do_suspend(struct drm_device *dev, bool runtime)
>  {
>  	struct nouveau_drm *drm = nouveau_drm(dev);
> @@ -559,8 +560,10 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
>  			return ret;
>  	}
>  
> -	NV_INFO(drm, "evicting buffers...\n");
> -	ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
> +	if (dev->pdev) {
> +		NV_INFO(drm, "evicting buffers...\n");
> +		ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
> +	}
>  
>  	NV_INFO(drm, "waiting for kernel channels to go idle...\n");
>  	if (drm->cechan) {
> @@ -612,8 +615,9 @@ fail_display:
>  	}
>  	return ret;
>  }
> +EXPORT_SYMBOL(nouveau_do_suspend);
>  
> -static int
> +int
>  nouveau_do_resume(struct drm_device *dev, bool runtime)
>  {
>  	struct nouveau_drm *drm = nouveau_drm(dev);
> @@ -635,7 +639,8 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
>  		nvif_client_resume(&cli->base);
>  	}
>  
> -	nouveau_run_vbios_init(dev);
> +	if (dev->pdev)
> +		nouveau_run_vbios_init(dev);
>  
>  	if (dev->mode_config.num_crtc) {
>  		NV_INFO(drm, "resuming display...\n");
> @@ -646,6 +651,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(nouveau_do_resume);
>  
>  int
>  nouveau_pmops_suspend(struct device *dev)
> diff --git a/drm/nouveau_drm.h b/drm/nouveau_drm.h
> index 8ae36f265fb8..897d295dd1e3 100644
> --- a/drm/nouveau_drm.h
> +++ b/drm/nouveau_drm.h
> @@ -177,6 +177,8 @@ nouveau_drm(struct drm_device *dev)
>  
>  int nouveau_pmops_suspend(struct device *);
>  int nouveau_pmops_resume(struct device *);
> +int nouveau_do_suspend(struct drm_device *dev, bool runtime);
> +int nouveau_do_resume(struct drm_device *dev, bool runtime);
>  
>  #define nouveau_platform_device_create(p, u)                                   \
>  	nouveau_platform_device_create_(p, sizeof(**u), (void **)u)
> 

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Vince Hsu]

Hi Emil,

On 12/30/2014 10:34 AM, Emil Velikov wrote:
> On 23/12/14 10:40, Vince Hsu wrote:
>> This patch adds some checks in the suspend/resume functions to distinguish
>> the dGPU and mobile GPU and exports some variables/functions so that the
>> nouveau platform device can reuse them.
>>
> Hi Vince,
>
> Afaiu one needs to export a symbol as it's used by another module or
> subsystem. With the follow up two patches you are not doing either one,
> so I'd assume that you can just omit the EXPORT_* changes.
The nouveau platform device driver is built as another module - 
nouveau_platform.ko. :)

Thanks,
Vince

>
> I could be wrong though :-)
>
> Cheers
> Emil
>
>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>> ---
>>   drm/nouveau_drm.c | 16 +++++++++++-----
>>   drm/nouveau_drm.h |  2 ++
>>   2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drm/nouveau_drm.c b/drm/nouveau_drm.c
>> index afb93bb72f97..0ed99ef80211 100644
>> --- a/drm/nouveau_drm.c
>> +++ b/drm/nouveau_drm.c
>> @@ -72,6 +72,7 @@ module_param_named(modeset, nouveau_modeset, int, 0400);
>>   
>>   MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1)");
>>   int nouveau_runtime_pm = -1;
>> +EXPORT_SYMBOL(nouveau_runtime_pm);
>>   module_param_named(runpm, nouveau_runtime_pm, int, 0400);
>>   
>>   static struct drm_driver driver_stub;
>> @@ -543,7 +544,7 @@ nouveau_drm_remove(struct pci_dev *pdev)
>>   	nouveau_drm_device_remove(dev);
>>   }
>>   
>> -static int
>> +int
>>   nouveau_do_suspend(struct drm_device *dev, bool runtime)
>>   {
>>   	struct nouveau_drm *drm = nouveau_drm(dev);
>> @@ -559,8 +560,10 @@ nouveau_do_suspend(struct drm_device *dev, bool runtime)
>>   			return ret;
>>   	}
>>   
>> -	NV_INFO(drm, "evicting buffers...\n");
>> -	ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
>> +	if (dev->pdev) {
>> +		NV_INFO(drm, "evicting buffers...\n");
>> +		ttm_bo_evict_mm(&drm->ttm.bdev, TTM_PL_VRAM);
>> +	}
>>   
>>   	NV_INFO(drm, "waiting for kernel channels to go idle...\n");
>>   	if (drm->cechan) {
>> @@ -612,8 +615,9 @@ fail_display:
>>   	}
>>   	return ret;
>>   }
>> +EXPORT_SYMBOL(nouveau_do_suspend);
>>   
>> -static int
>> +int
>>   nouveau_do_resume(struct drm_device *dev, bool runtime)
>>   {
>>   	struct nouveau_drm *drm = nouveau_drm(dev);
>> @@ -635,7 +639,8 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
>>   		nvif_client_resume(&cli->base);
>>   	}
>>   
>> -	nouveau_run_vbios_init(dev);
>> +	if (dev->pdev)
>> +		nouveau_run_vbios_init(dev);
>>   
>>   	if (dev->mode_config.num_crtc) {
>>   		NV_INFO(drm, "resuming display...\n");
>> @@ -646,6 +651,7 @@ nouveau_do_resume(struct drm_device *dev, bool runtime)
>>   
>>   	return 0;
>>   }
>> +EXPORT_SYMBOL(nouveau_do_resume);
>>   
>>   int
>>   nouveau_pmops_suspend(struct device *dev)
>> diff --git a/drm/nouveau_drm.h b/drm/nouveau_drm.h
>> index 8ae36f265fb8..897d295dd1e3 100644
>> --- a/drm/nouveau_drm.h
>> +++ b/drm/nouveau_drm.h
>> @@ -177,6 +177,8 @@ nouveau_drm(struct drm_device *dev)
>>   
>>   int nouveau_pmops_suspend(struct device *);
>>   int nouveau_pmops_resume(struct device *);
>> +int nouveau_do_suspend(struct drm_device *dev, bool runtime);
>> +int nouveau_do_resume(struct drm_device *dev, bool runtime);
>>   
>>   #define nouveau_platform_device_create(p, u)                                   \
>>   	nouveau_platform_device_create_(p, sizeof(**u), (void **)u)
>>

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 980 bytes --]

On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> Hi Emil,
> 
> On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >On 23/12/14 10:40, Vince Hsu wrote:
> >>This patch adds some checks in the suspend/resume functions to distinguish
> >>the dGPU and mobile GPU and exports some variables/functions so that the
> >>nouveau platform device can reuse them.
> >>
> >Hi Vince,
> >
> >Afaiu one needs to export a symbol as it's used by another module or
> >subsystem. With the follow up two patches you are not doing either one,
> >so I'd assume that you can just omit the EXPORT_* changes.
> The nouveau platform device driver is built as another module -
> nouveau_platform.ko. :)

I'd like to hear the opinion of the nouveau people and Alex, but I'd
very much prefer if nouveau_platform.o was simply linked into the
nouveau.ko module. I don't see any good reason to keep it separate.

Something like the attached patch (untested) ought to do it.

Thierry

[-- Attachment #1.2: patch --]
[-- Type: text/plain, Size: 1048 bytes --]

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 40afc69a3778..5ab13e7939db 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -26,7 +26,7 @@ config DRM_NOUVEAU
 	  Choose this option for open-source NVIDIA support.
 
 config NOUVEAU_PLATFORM_DRIVER
-	tristate "Nouveau (NVIDIA) SoC GPUs"
+	bool "Nouveau (NVIDIA) SoC GPUs"
 	depends on DRM_NOUVEAU && ARCH_TEGRA
 	default y
 	help
diff --git a/drivers/gpu/drm/nouveau/Makefile b/drivers/gpu/drm/nouveau/Makefile
index ea40cd653c7c..13acf90c11c8 100644
--- a/drivers/gpu/drm/nouveau/Makefile
+++ b/drivers/gpu/drm/nouveau/Makefile
@@ -394,8 +394,6 @@ nouveau-$(CONFIG_ACPI) += nouveau_acpi.o
 endif
 nouveau-$(CONFIG_DRM_NOUVEAU_BACKLIGHT) += nouveau_backlight.o
 nouveau-$(CONFIG_DEBUG_FS) += nouveau_debugfs.o
+nouveau-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o
 
 obj-$(CONFIG_DRM_NOUVEAU)+= nouveau.o
-
-# platform driver
-obj-$(CONFIG_NOUVEAU_PLATFORM_DRIVER) += nouveau_platform.o

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Alexandre Courbot]

On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
>> Hi Emil,
>>
>> On 12/30/2014 10:34 AM, Emil Velikov wrote:
>> >On 23/12/14 10:40, Vince Hsu wrote:
>> >>This patch adds some checks in the suspend/resume functions to distinguish
>> >>the dGPU and mobile GPU and exports some variables/functions so that the
>> >>nouveau platform device can reuse them.
>> >>
>> >Hi Vince,
>> >
>> >Afaiu one needs to export a symbol as it's used by another module or
>> >subsystem. With the follow up two patches you are not doing either one,
>> >so I'd assume that you can just omit the EXPORT_* changes.
>> The nouveau platform device driver is built as another module -
>> nouveau_platform.ko. :)
>
> I'd like to hear the opinion of the nouveau people and Alex, but I'd
> very much prefer if nouveau_platform.o was simply linked into the
> nouveau.ko module. I don't see any good reason to keep it separate.

Yep, I agree. The decision to host platform support in a separate
module looks misleaded if it results in additional exports that we
would otherwise avoid. IIUC I did this to be able to use the module
convenience macros to register the platform driver.

>
> Something like the attached patch (untested) ought to do it.

This patch alone won't be enough for the reason I mentioned above.
However, if Vince doesn't mind handling the platform driver
registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
this would be the way to go.

Cheers,
Alex.

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Vince Hsu]

On 01/06/2015 03:50 AM, Alexandre Courbot wrote:
> On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
>>> Hi Emil,
>>>
>>> On 12/30/2014 10:34 AM, Emil Velikov wrote:
>>>> On 23/12/14 10:40, Vince Hsu wrote:
>>>>> This patch adds some checks in the suspend/resume functions to distinguish
>>>>> the dGPU and mobile GPU and exports some variables/functions so that the
>>>>> nouveau platform device can reuse them.
>>>>>
>>>> Hi Vince,
>>>>
>>>> Afaiu one needs to export a symbol as it's used by another module or
>>>> subsystem. With the follow up two patches you are not doing either one,
>>>> so I'd assume that you can just omit the EXPORT_* changes.
>>> The nouveau platform device driver is built as another module -
>>> nouveau_platform.ko. :)
>> I'd like to hear the opinion of the nouveau people and Alex, but I'd
>> very much prefer if nouveau_platform.o was simply linked into the
>> nouveau.ko module. I don't see any good reason to keep it separate.
> Yep, I agree. The decision to host platform support in a separate
> module looks misleaded if it results in additional exports that we
> would otherwise avoid. IIUC I did this to be able to use the module
> convenience macros to register the platform driver.
>
>> Something like the attached patch (untested) ought to do it.
> This patch alone won't be enough for the reason I mentioned above.
> However, if Vince doesn't mind handling the platform driver
> registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> this would be the way to go.
I definitely don't mind. :) Will fix in next version based on you guys' 
suggestions.

Thanks,
Vince

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
> On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> >> Hi Emil,
> >>
> >> On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >> >On 23/12/14 10:40, Vince Hsu wrote:
> >> >>This patch adds some checks in the suspend/resume functions to distinguish
> >> >>the dGPU and mobile GPU and exports some variables/functions so that the
> >> >>nouveau platform device can reuse them.
> >> >>
> >> >Hi Vince,
> >> >
> >> >Afaiu one needs to export a symbol as it's used by another module or
> >> >subsystem. With the follow up two patches you are not doing either one,
> >> >so I'd assume that you can just omit the EXPORT_* changes.
> >> The nouveau platform device driver is built as another module -
> >> nouveau_platform.ko. :)
> >
> > I'd like to hear the opinion of the nouveau people and Alex, but I'd
> > very much prefer if nouveau_platform.o was simply linked into the
> > nouveau.ko module. I don't see any good reason to keep it separate.
> 
> Yep, I agree. The decision to host platform support in a separate
> module looks misleaded if it results in additional exports that we
> would otherwise avoid. IIUC I did this to be able to use the module
> convenience macros to register the platform driver.
> 
> >
> > Something like the attached patch (untested) ought to do it.
> 
> This patch alone won't be enough for the reason I mentioned above.
> However, if Vince doesn't mind handling the platform driver
> registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> this would be the way to go.

If we do the conversion to generic power domains, the only Tegra-
specific API remaining will be the access to the fuse registers for the
speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
any longer and could always build the platform driver along with the PCI
driver.

I guess we could do that even now if we simply #ifdef the various Tegra-
specific parts. That in turn would have the advantage that we don't need
to #ifdef the driver registration code. And it would help separate
things in case anybody wanted to use one of the SoC GPUs in a non-Tegra
SoC.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Vince Hsu]

On 01/06/2015 07:49 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
>> On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
>>>> Hi Emil,
>>>>
>>>> On 12/30/2014 10:34 AM, Emil Velikov wrote:
>>>>> On 23/12/14 10:40, Vince Hsu wrote:
>>>>>> This patch adds some checks in the suspend/resume functions to distinguish
>>>>>> the dGPU and mobile GPU and exports some variables/functions so that the
>>>>>> nouveau platform device can reuse them.
>>>>>>
>>>>> Hi Vince,
>>>>>
>>>>> Afaiu one needs to export a symbol as it's used by another module or
>>>>> subsystem. With the follow up two patches you are not doing either one,
>>>>> so I'd assume that you can just omit the EXPORT_* changes.
>>>> The nouveau platform device driver is built as another module -
>>>> nouveau_platform.ko. :)
>>> I'd like to hear the opinion of the nouveau people and Alex, but I'd
>>> very much prefer if nouveau_platform.o was simply linked into the
>>> nouveau.ko module. I don't see any good reason to keep it separate.
>> Yep, I agree. The decision to host platform support in a separate
>> module looks misleaded if it results in additional exports that we
>> would otherwise avoid. IIUC I did this to be able to use the module
>> convenience macros to register the platform driver.
>>
>>> Something like the attached patch (untested) ought to do it.
>> This patch alone won't be enough for the reason I mentioned above.
>> However, if Vince doesn't mind handling the platform driver
>> registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
>> this would be the way to go.
> If we do the conversion to generic power domains, the only Tegra-
> specific API remaining will be the access to the fuse registers for the
> speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
> any longer and could always build the platform driver along with the PCI
> driver.
Do we really want the platform driver always built with the PCI driver even
there is no dependency between them. Actually I have some patches to
build the platform driver with !CONFIG_PCI and would like to post them maybe
later.

Thanks,
Vince

>
> I guess we could do that even now if we simply #ifdef the various Tegra-
> specific parts. That in turn would have the advantage that we don't need
> to #ifdef the driver registration code. And it would help separate
> things in case anybody wanted to use one of the SoC GPUs in a non-Tegra
> SoC.

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
> 
> On 01/06/2015 07:49 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> >>>>Hi Emil,
> >>>>
> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
> >>>>>>nouveau platform device can reuse them.
> >>>>>>
> >>>>>Hi Vince,
> >>>>>
> >>>>>Afaiu one needs to export a symbol as it's used by another module or
> >>>>>subsystem. With the follow up two patches you are not doing either one,
> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
> >>>>The nouveau platform device driver is built as another module -
> >>>>nouveau_platform.ko. :)
> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
> >>>very much prefer if nouveau_platform.o was simply linked into the
> >>>nouveau.ko module. I don't see any good reason to keep it separate.
> >>Yep, I agree. The decision to host platform support in a separate
> >>module looks misleaded if it results in additional exports that we
> >>would otherwise avoid. IIUC I did this to be able to use the module
> >>convenience macros to register the platform driver.
> >>
> >>>Something like the attached patch (untested) ought to do it.
> >>This patch alone won't be enough for the reason I mentioned above.
> >>However, if Vince doesn't mind handling the platform driver
> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> >>this would be the way to go.
> >If we do the conversion to generic power domains, the only Tegra-
> >specific API remaining will be the access to the fuse registers for the
> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
> >any longer and could always build the platform driver along with the PCI
> >driver.
> Do we really want the platform driver always built with the PCI driver even
> there is no dependency between them. Actually I have some patches to
> build the platform driver with !CONFIG_PCI and would like to post them maybe
> later.

I do see some advantage in making Nouveau build with !CONFIG_PCI because
it allows building the driver for an SoC without PCI. But I think always
compiling whatever is available won't hurt. Nouveau itself is rather big
(~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
glue is equally negligible.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Ilia Mirkin]

On Tue, Jan 6, 2015 at 9:37 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
>>
>> On 01/06/2015 07:49 PM, Thierry Reding wrote:
>> >* PGP Signed by an unknown key
>> >
>> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
>> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
>> >>>>Hi Emil,
>> >>>>
>> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
>> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
>> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
>> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
>> >>>>>>nouveau platform device can reuse them.
>> >>>>>>
>> >>>>>Hi Vince,
>> >>>>>
>> >>>>>Afaiu one needs to export a symbol as it's used by another module or
>> >>>>>subsystem. With the follow up two patches you are not doing either one,
>> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
>> >>>>The nouveau platform device driver is built as another module -
>> >>>>nouveau_platform.ko. :)
>> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
>> >>>very much prefer if nouveau_platform.o was simply linked into the
>> >>>nouveau.ko module. I don't see any good reason to keep it separate.
>> >>Yep, I agree. The decision to host platform support in a separate
>> >>module looks misleaded if it results in additional exports that we
>> >>would otherwise avoid. IIUC I did this to be able to use the module
>> >>convenience macros to register the platform driver.
>> >>
>> >>>Something like the attached patch (untested) ought to do it.
>> >>This patch alone won't be enough for the reason I mentioned above.
>> >>However, if Vince doesn't mind handling the platform driver
>> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
>> >>this would be the way to go.
>> >If we do the conversion to generic power domains, the only Tegra-
>> >specific API remaining will be the access to the fuse registers for the
>> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
>> >any longer and could always build the platform driver along with the PCI
>> >driver.
>> Do we really want the platform driver always built with the PCI driver even
>> there is no dependency between them. Actually I have some patches to
>> build the platform driver with !CONFIG_PCI and would like to post them maybe
>> later.
>
> I do see some advantage in making Nouveau build with !CONFIG_PCI because
> it allows building the driver for an SoC without PCI. But I think always
> compiling whatever is available won't hurt. Nouveau itself is rather big
> (~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
> in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
> glue is equally negligible.

The much bigger reductions in size would be from getting rid of all
the stuff that's not used at all on a GK20A, like all the firmware,
and a whole bunch of logic. I had a series that split things up into
nv04/nv50/nvc0 categories. I believe it was considered undesirable due
to increased build complexity. (It wasn't a nice modularized
separation but rather just a compile-time setting.) Shouldn't be too
hard to regenerate though.

  -ilia

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]

On Tue, Jan 06, 2015 at 09:44:01AM -0500, Ilia Mirkin wrote:
> On Tue, Jan 6, 2015 at 9:37 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
> >>
> >> On 01/06/2015 07:49 PM, Thierry Reding wrote:
> >> >* PGP Signed by an unknown key
> >> >
> >> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
> >> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> >> >>>>Hi Emil,
> >> >>>>
> >> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
> >> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
> >> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
> >> >>>>>>nouveau platform device can reuse them.
> >> >>>>>>
> >> >>>>>Hi Vince,
> >> >>>>>
> >> >>>>>Afaiu one needs to export a symbol as it's used by another module or
> >> >>>>>subsystem. With the follow up two patches you are not doing either one,
> >> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
> >> >>>>The nouveau platform device driver is built as another module -
> >> >>>>nouveau_platform.ko. :)
> >> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
> >> >>>very much prefer if nouveau_platform.o was simply linked into the
> >> >>>nouveau.ko module. I don't see any good reason to keep it separate.
> >> >>Yep, I agree. The decision to host platform support in a separate
> >> >>module looks misleaded if it results in additional exports that we
> >> >>would otherwise avoid. IIUC I did this to be able to use the module
> >> >>convenience macros to register the platform driver.
> >> >>
> >> >>>Something like the attached patch (untested) ought to do it.
> >> >>This patch alone won't be enough for the reason I mentioned above.
> >> >>However, if Vince doesn't mind handling the platform driver
> >> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> >> >>this would be the way to go.
> >> >If we do the conversion to generic power domains, the only Tegra-
> >> >specific API remaining will be the access to the fuse registers for the
> >> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
> >> >any longer and could always build the platform driver along with the PCI
> >> >driver.
> >> Do we really want the platform driver always built with the PCI driver even
> >> there is no dependency between them. Actually I have some patches to
> >> build the platform driver with !CONFIG_PCI and would like to post them maybe
> >> later.
> >
> > I do see some advantage in making Nouveau build with !CONFIG_PCI because
> > it allows building the driver for an SoC without PCI. But I think always
> > compiling whatever is available won't hurt. Nouveau itself is rather big
> > (~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
> > in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
> > glue is equally negligible.
> 
> The much bigger reductions in size would be from getting rid of all
> the stuff that's not used at all on a GK20A, like all the firmware,
> and a whole bunch of logic. I had a series that split things up into
> nv04/nv50/nvc0 categories. I believe it was considered undesirable due
> to increased build complexity. (It wasn't a nice modularized
> separation but rather just a compile-time setting.) Shouldn't be too
> hard to regenerate though.

Would you happen to remember the amount of savings for enabling only
nvc0? I'm not overly concerned with the module being 1 MiB on Tegra,
though. Most boards tend to have 2 GiB+ of memory these days and the
kernel footprint is pretty big without Nouveau anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Ilia Mirkin]

On Tue, Jan 6, 2015 at 9:50 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jan 06, 2015 at 09:44:01AM -0500, Ilia Mirkin wrote:
>> On Tue, Jan 6, 2015 at 9:37 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
>> >>
>> >> On 01/06/2015 07:49 PM, Thierry Reding wrote:
>> >> >* PGP Signed by an unknown key
>> >> >
>> >> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
>> >> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> >> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
>> >> >>>>Hi Emil,
>> >> >>>>
>> >> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
>> >> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
>> >> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
>> >> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
>> >> >>>>>>nouveau platform device can reuse them.
>> >> >>>>>>
>> >> >>>>>Hi Vince,
>> >> >>>>>
>> >> >>>>>Afaiu one needs to export a symbol as it's used by another module or
>> >> >>>>>subsystem. With the follow up two patches you are not doing either one,
>> >> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
>> >> >>>>The nouveau platform device driver is built as another module -
>> >> >>>>nouveau_platform.ko. :)
>> >> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
>> >> >>>very much prefer if nouveau_platform.o was simply linked into the
>> >> >>>nouveau.ko module. I don't see any good reason to keep it separate.
>> >> >>Yep, I agree. The decision to host platform support in a separate
>> >> >>module looks misleaded if it results in additional exports that we
>> >> >>would otherwise avoid. IIUC I did this to be able to use the module
>> >> >>convenience macros to register the platform driver.
>> >> >>
>> >> >>>Something like the attached patch (untested) ought to do it.
>> >> >>This patch alone won't be enough for the reason I mentioned above.
>> >> >>However, if Vince doesn't mind handling the platform driver
>> >> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
>> >> >>this would be the way to go.
>> >> >If we do the conversion to generic power domains, the only Tegra-
>> >> >specific API remaining will be the access to the fuse registers for the
>> >> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
>> >> >any longer and could always build the platform driver along with the PCI
>> >> >driver.
>> >> Do we really want the platform driver always built with the PCI driver even
>> >> there is no dependency between them. Actually I have some patches to
>> >> build the platform driver with !CONFIG_PCI and would like to post them maybe
>> >> later.
>> >
>> > I do see some advantage in making Nouveau build with !CONFIG_PCI because
>> > it allows building the driver for an SoC without PCI. But I think always
>> > compiling whatever is available won't hurt. Nouveau itself is rather big
>> > (~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
>> > in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
>> > glue is equally negligible.
>>
>> The much bigger reductions in size would be from getting rid of all
>> the stuff that's not used at all on a GK20A, like all the firmware,
>> and a whole bunch of logic. I had a series that split things up into
>> nv04/nv50/nvc0 categories. I believe it was considered undesirable due
>> to increased build complexity. (It wasn't a nice modularized
>> separation but rather just a compile-time setting.) Shouldn't be too
>> hard to regenerate though.
>
> Would you happen to remember the amount of savings for enabling only
> nvc0? I'm not overly concerned with the module being 1 MiB on Tegra,
> though. Most boards tend to have 2 GiB+ of memory these days and the
> kernel footprint is pretty big without Nouveau anyway.

Well, nvc0 is a big one since that's where all the firmware lives,
which is a big chunk of it, and you don't actually end up using any of
it. (And even if you did, it'd be just one of the firmware files, not
all of them).

Here was the patch: http://permalink.gmane.org/gmane.comp.video.dri.devel/102457

Looks like nvc0 only was:

NVC0: 878726 bytes
   text    data     bss     dec     hex filename
 360593  140327      80  501000   7a508 drivers/gpu/drm/nouveau/nouveau.ko

compared to the combo package:

NV04 + NV50 + NVC0: 1343482 bytes
   text    data     bss     dec     hex filename
 579171  184264     480  763915   ba80b drivers/gpu/drm/nouveau/nouveau.ko

  -ilia

Re: [Nouveau] [PATCH nouveau 09/11] drm: export some variable and functions to resue the PM functions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5170 bytes --]

On Tue, Jan 06, 2015 at 10:03:36AM -0500, Ilia Mirkin wrote:
> On Tue, Jan 6, 2015 at 9:50 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jan 06, 2015 at 09:44:01AM -0500, Ilia Mirkin wrote:
> >> On Tue, Jan 6, 2015 at 9:37 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> > On Tue, Jan 06, 2015 at 08:27:06PM +0800, Vince Hsu wrote:
> >> >>
> >> >> On 01/06/2015 07:49 PM, Thierry Reding wrote:
> >> >> >* PGP Signed by an unknown key
> >> >> >
> >> >> >On Mon, Jan 05, 2015 at 08:50:18PM +0100, Alexandre Courbot wrote:
> >> >> >>On Mon, Jan 5, 2015 at 4:32 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> >> >>>On Tue, Dec 30, 2014 at 11:18:34AM +0800, Vince Hsu wrote:
> >> >> >>>>Hi Emil,
> >> >> >>>>
> >> >> >>>>On 12/30/2014 10:34 AM, Emil Velikov wrote:
> >> >> >>>>>On 23/12/14 10:40, Vince Hsu wrote:
> >> >> >>>>>>This patch adds some checks in the suspend/resume functions to distinguish
> >> >> >>>>>>the dGPU and mobile GPU and exports some variables/functions so that the
> >> >> >>>>>>nouveau platform device can reuse them.
> >> >> >>>>>>
> >> >> >>>>>Hi Vince,
> >> >> >>>>>
> >> >> >>>>>Afaiu one needs to export a symbol as it's used by another module or
> >> >> >>>>>subsystem. With the follow up two patches you are not doing either one,
> >> >> >>>>>so I'd assume that you can just omit the EXPORT_* changes.
> >> >> >>>>The nouveau platform device driver is built as another module -
> >> >> >>>>nouveau_platform.ko. :)
> >> >> >>>I'd like to hear the opinion of the nouveau people and Alex, but I'd
> >> >> >>>very much prefer if nouveau_platform.o was simply linked into the
> >> >> >>>nouveau.ko module. I don't see any good reason to keep it separate.
> >> >> >>Yep, I agree. The decision to host platform support in a separate
> >> >> >>module looks misleaded if it results in additional exports that we
> >> >> >>would otherwise avoid. IIUC I did this to be able to use the module
> >> >> >>convenience macros to register the platform driver.
> >> >> >>
> >> >> >>>Something like the attached patch (untested) ought to do it.
> >> >> >>This patch alone won't be enough for the reason I mentioned above.
> >> >> >>However, if Vince doesn't mind handling the platform driver
> >> >> >>registration manually in nouveau_drm_init/nouveau_drm_exit, I agree
> >> >> >>this would be the way to go.
> >> >> >If we do the conversion to generic power domains, the only Tegra-
> >> >> >specific API remaining will be the access to the fuse registers for the
> >> >> >speedo value. At that point we wouldn't need the ARCH_TEGRA dependency
> >> >> >any longer and could always build the platform driver along with the PCI
> >> >> >driver.
> >> >> Do we really want the platform driver always built with the PCI driver even
> >> >> there is no dependency between them. Actually I have some patches to
> >> >> build the platform driver with !CONFIG_PCI and would like to post them maybe
> >> >> later.
> >> >
> >> > I do see some advantage in making Nouveau build with !CONFIG_PCI because
> >> > it allows building the driver for an SoC without PCI. But I think always
> >> > compiling whatever is available won't hurt. Nouveau itself is rather big
> >> > (~1 MiB of text and ~120 KiB of data) whereas the platform code weighs
> >> > in at around 1.5 KiB of text and 108 B of data. I suspect that the PCI
> >> > glue is equally negligible.
> >>
> >> The much bigger reductions in size would be from getting rid of all
> >> the stuff that's not used at all on a GK20A, like all the firmware,
> >> and a whole bunch of logic. I had a series that split things up into
> >> nv04/nv50/nvc0 categories. I believe it was considered undesirable due
> >> to increased build complexity. (It wasn't a nice modularized
> >> separation but rather just a compile-time setting.) Shouldn't be too
> >> hard to regenerate though.
> >
> > Would you happen to remember the amount of savings for enabling only
> > nvc0? I'm not overly concerned with the module being 1 MiB on Tegra,
> > though. Most boards tend to have 2 GiB+ of memory these days and the
> > kernel footprint is pretty big without Nouveau anyway.
> 
> Well, nvc0 is a big one since that's where all the firmware lives,
> which is a big chunk of it, and you don't actually end up using any of
> it. (And even if you did, it'd be just one of the firmware files, not
> all of them).
> 
> Here was the patch: http://permalink.gmane.org/gmane.comp.video.dri.devel/102457
> 
> Looks like nvc0 only was:
> 
> NVC0: 878726 bytes
>    text    data     bss     dec     hex filename
>  360593  140327      80  501000   7a508 drivers/gpu/drm/nouveau/nouveau.ko
> 
> compared to the combo package:
> 
> NV04 + NV50 + NVC0: 1343482 bytes
>    text    data     bss     dec     hex filename
>  579171  184264     480  763915   ba80b drivers/gpu/drm/nouveau/nouveau.ko

That's a pretty significant improvement. And the patch doesn't look too
bad either. But like I said, I'm not a big fan of reducing binary size
at the expense of code complexity, so I won't loose any sleep over it.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH nouveau 10/11] platform: add suspend/resume support

[Vince Hsu]

We reuse most of the suspend/resume functions of the dGPU for nouveau
platform device. Only the power up/down sequences need to be handled
separately.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/nouveau_platform.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
index 527fe2358fc9..51bfce59c498 100644
--- a/drm/nouveau_platform.c
+++ b/drm/nouveau_platform.c
@@ -211,6 +211,65 @@ static const struct of_device_id nouveau_platform_match[] = {
 MODULE_DEVICE_TABLE(of, nouveau_platform_match);
 #endif
 
+static int
+nouveau_platform_pmops_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct nouveau_device *device = nvkm_device(&drm->device);
+	struct nouveau_platform_gpu *gpu = nv_device_to_platform(device)->gpu;
+	int ret;
+
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
+	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
+		return 0;
+
+	ret = nouveau_do_suspend(drm_dev, false);
+	if (ret)
+		return ret;
+
+	ret = nouveau_platform_power_down(gpu);
+	if (ret) {
+		dev_err(dev, "failed to power down gpu (err:%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_platform_pmops_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct nouveau_device *device = nvkm_device(&drm->device);
+	struct nouveau_platform_gpu *gpu = nv_device_to_platform(device)->gpu;
+	int ret;
+
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF ||
+	    drm_dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
+		return 0;
+
+	ret = nouveau_platform_power_up(gpu);
+	if (ret) {
+		dev_err(dev, "failed to power up gpu\n");
+		return ret;
+	}
+
+	ret = nouveau_do_resume(drm_dev, false);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct dev_pm_ops nouveau_platform_pm_ops = {
+	.suspend = nouveau_platform_pmops_suspend,
+	.resume = nouveau_platform_pmops_resume,
+};
+
 struct platform_driver nouveau_platform_driver = {
 	.driver = {
 		.name = "nouveau",
@@ -218,6 +277,7 @@ struct platform_driver nouveau_platform_driver = {
 	},
 	.probe = nouveau_platform_probe,
 	.remove = nouveau_platform_remove,
+	.driver.pm = &nouveau_platform_pm_ops,
 };
 
 module_platform_driver(nouveau_platform_driver);
-- 
1.9.1

[PATCH nouveau 11/11] platform: add PM runtime suspend/resume support

[Vince Hsu]

The runtime suspend/resume flow is almost the same as the suspend/resume
except the the arguments passed to nouveau_do_{suspend,resume}.

Signed-off-by: Vince Hsu <vinceh@nvidia.com>
---
 drm/nouveau_platform.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drm/nouveau_platform.c b/drm/nouveau_platform.c
index 51bfce59c498..c71c62081b9c 100644
--- a/drm/nouveau_platform.c
+++ b/drm/nouveau_platform.c
@@ -177,6 +177,9 @@ static int nouveau_platform_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_unref;
 
+	if (nouveau_runtime_pm != 0)
+		pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 err_unref:
@@ -197,6 +200,9 @@ static int nouveau_platform_remove(struct platform_device *pdev)
 	struct nouveau_device *device = nvkm_device(&drm->device);
 	struct nouveau_platform_gpu *gpu = nv_device_to_platform(device)->gpu;
 
+	if (nouveau_runtime_pm != 0)
+		pm_runtime_disable(&pdev->dev);
+
 	nouveau_drm_device_remove(drm_dev);
 
 	return nouveau_platform_power_down(gpu);
@@ -265,9 +271,73 @@ nouveau_platform_pmops_resume(struct device *dev)
 	return 0;
 }
 
+static int
+nouveau_platform_pmops_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct nouveau_device *device = nvkm_device(&drm->device);
+	struct nouveau_platform_gpu *gpu = nv_device_to_platform(device)->gpu;
+	int ret;
+
+	ret = nouveau_do_suspend(drm_dev, true);
+	if (ret) {
+		dev_err(dev, "failed to suspend drm device (err:%d)\n", ret);
+		return ret;
+	}
+
+	ret = nouveau_platform_power_down(gpu);
+	if (ret) {
+		dev_err(dev, "failed to power down gpu (err:%d)\n", ret);
+		return ret;
+	}
+
+	drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+	return ret;
+}
+
+static int
+nouveau_platform_pmops_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm_dev = platform_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+	struct nouveau_device *device = nvkm_device(&drm->device);
+	struct nouveau_platform_gpu *gpu = nv_device_to_platform(device)->gpu;
+	int ret;
+
+	ret = nouveau_platform_power_up(gpu);
+	if (ret) {
+		dev_err(dev, "failed to power up gpu\n");
+		return ret;
+	}
+
+	ret = nouveau_do_resume(drm_dev, true);
+	if (ret) {
+		dev_err(dev, "failed to resume\n");
+		return ret;
+	}
+
+	drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
+	return ret;
+}
+
+static int
+nouveau_platform_pmops_runtime_idle(struct device *dev)
+{
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_autosuspend(dev);
+
+	return 1;
+}
+
 static const struct dev_pm_ops nouveau_platform_pm_ops = {
 	.suspend = nouveau_platform_pmops_suspend,
 	.resume = nouveau_platform_pmops_resume,
+	.runtime_suspend = nouveau_platform_pmops_runtime_suspend,
+	.runtime_resume = nouveau_platform_pmops_runtime_resume,
+	.runtime_idle = nouveau_platform_pmops_runtime_idle,
 };
 
 struct platform_driver nouveau_platform_driver = {
-- 
1.9.1