[PATCH V5 00/14] Add generic PM domain support for Tegra

[PATCH V5 00/14] Add generic PM domain support for Tegra

[Jon Hunter]

Adds generic PM domain support for Tegra SoCs but this series only
enables support for it on Tegra 64-bit devices. There is no reason why
this cannot be enable for Tegra 32-bit devices, but to keep the patch
series to a minimum only 64-bit devices are enabled so far.

This series has been boot tested on Tegra210 as well as various 32-bit
Tegra platforms.

Summary of changes since V4 [1]:
- Re-worked fix to handle base address on probe failure
- Added patch to lock around simultaneuous accesses to PMC registers
- Added patch to change powergate and rail IDs to unsigned type
- Added patch to fix testing of powergate state
- Updated patch to check for valid powergates to use a bitmap
- Updated Tegra DT PMC bindings per Rob H's feedback
- Updated Tegra power domains binding per Thierry's feedback
- Updated Tegra generic power domain support per Thierry's feedback

Summary of changes since V3 [0]:
- Dropped tegra124 support for now
- Removed MC flush support per feedback from Thierry
- Cleaned up the PMC changes per feedback from Thierry
- Added support for tegra210

Series Summary:
Patch 1-7:  PMC clean-up and fixes
Patch 8:    Adds function to remove a generic PM domain
Patch 9:    Updates DT documentation for tegra PMC
Patch 10:   Adds DT documentation for tegra generic PM domains
Patch 11:   Adds PMC generic DM domains support
Patch 12:   Adds audio clock for Tegra210 audio power-domain
Patch 13:   Adds DT bindings for Tegra210 audio power-domain
Patch 14:   Enable generic PM domains for tegra64

[0] http://comments.gmane.org/gmane.linux.ports.tegra/22944
[1] http://marc.info/?l=linux-tegra&m=144924153600529&w=2

Jon Hunter (14):
  soc: tegra: pmc: Restore base address on probe failure
  soc: tegra: pmc: Protect public functions from potential race
    conditions
  soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type
  soc: tegra: pmc: Fix testing of powergate state
  soc: tegra: pmc: Wait for powergate state to change
  soc: tegra: pmc: Fix checking of valid partitions
  soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC
  PM / Domains: Add function to remove a pm-domain
  Documentation: DT: bindings: Update NVIDIA PMC for Tegra
  Documentation: DT: bindings: Add power domain info for NVIDIA PMC
  soc: tegra: pmc: Add generic PM domain support
  clk: tegra210: Add the APB2APE audio clock
  ARM64: tegra: Add audio PM domain device node for Tegra210
  ARM64: tegra: select PM_GENERIC_DOMAINS

 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      |  67 ++-
 arch/arm/mach-tegra/platsmp.c                      |  16 +-
 arch/arm64/Kconfig.platforms                       |   2 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi           |  11 +-
 drivers/base/power/domain.c                        |  26 +
 drivers/clk/tegra/clk-id.h                         |   1 +
 drivers/clk/tegra/clk-tegra-periph.c               |   1 +
 drivers/clk/tegra/clk-tegra210.c                   |   1 +
 drivers/gpu/drm/tegra/drm.h                        |   2 +-
 drivers/soc/tegra/pmc.c                            | 603 ++++++++++++++++++---
 include/dt-bindings/clock/tegra210-car.h           |   2 +-
 include/dt-bindings/power/tegra-powergate.h        |  36 ++
 include/linux/pm_domain.h                          |   5 +
 include/soc/tegra/pmc.h                            |  73 +--
 14 files changed, 709 insertions(+), 137 deletions(-)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

-- 
2.1.4

[PATCH V5 01/14] soc: tegra: pmc: Restore base address on probe failure

[Jon Hunter]

During early initialisation, the PMC registers are mapped and the PMC SoC
data is populated in the PMC data structure. This allows other drivers
access the PMC register space, via the public tegra PMC APIs, prior to
probing the PMC device.

When the PMC device is probed, the PMC registers are mapped again and if
successful the initial mapping is freed. If the probing of the PMC device
fails after the registers are remapped, then the registers will be
unmapped and hence the pointer to the PMC registers will be invalid. This
could lead to a potential crash, because once the PMC SoC data pointer is
populated, the driver assumes that the PMC register mapping is also valid
and a user calling any of the public tegra PMC APIs could trigger an
exception because these APIs don't check that the mapping is still valid.

Fix this by restoring the original mapping for the PMC registers if the
probe on the PMC device fails and only unmapping the original mapping if
the probe succeeds.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/soc/tegra/pmc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 88c7e506177e..85b4e166273a 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -821,13 +821,11 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (IS_ERR(pmc->base))
 		return PTR_ERR(pmc->base);
 
-	iounmap(base);
-
 	pmc->clk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(pmc->clk)) {
 		err = PTR_ERR(pmc->clk);
 		dev_err(&pdev->dev, "failed to get pclk: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	pmc->dev = &pdev->dev;
@@ -839,7 +837,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			return err;
+			goto error;
 	}
 
 	err = register_restart_handler(&tegra_pmc_restart_handler);
@@ -847,10 +845,17 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		debugfs_remove(pmc->debugfs);
 		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
 			err);
-		return err;
+		goto error;
 	}
 
+	iounmap(base);
+
 	return 0;
+
+error:
+	pmc->base = base;
+
+	return err;
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V5 05/14] soc: tegra: pmc: Wait for powergate state to change

[Jon Hunter]

Currently, the function tegra_powergate_set() simply sets the desired
powergate state but does not wait for the state to change. In most cases
we should wait for the state to change before proceeding. Currently, there
is a case for tegra114 and tegra124 devices where we do not wait when
starting the secondary CPU as this is not necessary. However, this is only
done at boot time and so waiting here will only have a small impact on
boot time. Therefore, update tegra_powergate_set() to wait when setting
the powergate.

By adding this feature, we can also eliminate the polling loop from
tegra30_boot_secondary().

A function has been added for checking the status of the powergate and
so update the tegra_powergate_is_powered() to use this macro as well.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       |  9 ++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index f3f61dbbda97..75620ae73913 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * be un-gated by un-toggling the power gate register
 	 * manually.
 	 */
-	if (!tegra_pmc_cpu_is_powered(cpu)) {
-		ret = tegra_pmc_cpu_power_on(cpu);
-		if (ret)
-			return ret;
-
-		/* Wait for the power to come up. */
-		timeout = jiffies + msecs_to_jiffies(100);
-		while (!tegra_pmc_cpu_is_powered(cpu)) {
-			if (time_after(jiffies, timeout))
-				return -ETIMEDOUT;
-			udelay(10);
-		}
-	}
+	ret = tegra_pmc_cpu_power_on(cpu);
+	if (ret)
+		return ret;
 
 remove_clamps:
 	/* CPU partition is powered. Enable the CPU clock. */
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 99cb2fdd29e1..35ee60fd17be 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -28,6 +28,7 @@
 #include <linux/export.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
@@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id)
  */
 static int tegra_powergate_set(unsigned int id, bool new_state)
 {
+	bool status;
+	int err;
+
 	mutex_lock(&pmc->powergates_lock);
 
 	if (tegra_powergate_state(id) == new_state) {
@@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
 
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
+	err = readx_poll_timeout(tegra_powergate_state, id, status,
+				 status == new_state, 10, 100000);
+
 	mutex_unlock(&pmc->powergates_lock);
 
-	return 0;
+	return err;
 }
 
 /**
-- 
2.1.4

Re: [PATCH V5 05/14] soc: tegra: pmc: Wait for powergate state to change

[Mathieu Poirier]

On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> wrote:
> Currently, the function tegra_powergate_set() simply sets the desired
> powergate state but does not wait for the state to change. In most cases
> we should wait for the state to change before proceeding. Currently, there
> is a case for tegra114 and tegra124 devices where we do not wait when
> starting the secondary CPU as this is not necessary. However, this is only
> done at boot time and so waiting here will only have a small impact on
> boot time. Therefore, update tegra_powergate_set() to wait when setting
> the powergate.
>
> By adding this feature, we can also eliminate the polling loop from
> tegra30_boot_secondary().
>
> A function has been added for checking the status of the powergate and
> so update the tegra_powergate_is_powered() to use this macro as well.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>  drivers/soc/tegra/pmc.c       |  9 ++++++++-
>  2 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index f3f61dbbda97..75620ae73913 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>          * be un-gated by un-toggling the power gate register
>          * manually.
>          */
> -       if (!tegra_pmc_cpu_is_powered(cpu)) {
> -               ret = tegra_pmc_cpu_power_on(cpu);
> -               if (ret)
> -                       return ret;
> -
> -               /* Wait for the power to come up. */
> -               timeout = jiffies + msecs_to_jiffies(100);
> -               while (!tegra_pmc_cpu_is_powered(cpu)) {
> -                       if (time_after(jiffies, timeout))
> -                               return -ETIMEDOUT;
> -                       udelay(10);
> -               }
> -       }
> +       ret = tegra_pmc_cpu_power_on(cpu);
> +       if (ret)
> +               return ret;
>
>  remove_clamps:
>         /* CPU partition is powered. Enable the CPU clock. */
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 99cb2fdd29e1..35ee60fd17be 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -28,6 +28,7 @@
>  #include <linux/export.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id)
>   */
>  static int tegra_powergate_set(unsigned int id, bool new_state)
>  {
> +       bool status;
> +       int err;
> +
>         mutex_lock(&pmc->powergates_lock);
>
>         if (tegra_powergate_state(id) == new_state) {
> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>
>         tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>
> +       err = readx_poll_timeout(tegra_powergate_state, id, status,
> +                                status == new_state, 10, 100000);
> +

I understand (and agree) with the goal of this patch but I wonder (and
I don't know much about the system/context) if holding a mutex while
sleeping won't incur adverse effect on other parts of the system that
weren't use to see this wait.  One way to fix this might be to use
"mutex_trylock()" and let callers retry as they see fit if an
operation is already in progress.

>         mutex_unlock(&pmc->powergates_lock);
>
> -       return 0;
> +       return err;
>  }
>
>  /**
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 05/14] soc: tegra: pmc: Wait for powergate state to change

[Jon Hunter]

On 29/01/16 16:58, Mathieu Poirier wrote:
> On 28 January 2016 at 09:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Currently, the function tegra_powergate_set() simply sets the desired
>> powergate state but does not wait for the state to change. In most cases
>> we should wait for the state to change before proceeding. Currently, there
>> is a case for tegra114 and tegra124 devices where we do not wait when
>> starting the secondary CPU as this is not necessary. However, this is only
>> done at boot time and so waiting here will only have a small impact on
>> boot time. Therefore, update tegra_powergate_set() to wait when setting
>> the powergate.
>>
>> By adding this feature, we can also eliminate the polling loop from
>> tegra30_boot_secondary().
>>
>> A function has been added for checking the status of the powergate and
>> so update the tegra_powergate_is_powered() to use this macro as well.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>>  drivers/soc/tegra/pmc.c       |  9 ++++++++-
>>  2 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
>> index f3f61dbbda97..75620ae73913 100644
>> --- a/arch/arm/mach-tegra/platsmp.c
>> +++ b/arch/arm/mach-tegra/platsmp.c
>> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>          * be un-gated by un-toggling the power gate register
>>          * manually.
>>          */
>> -       if (!tegra_pmc_cpu_is_powered(cpu)) {
>> -               ret = tegra_pmc_cpu_power_on(cpu);
>> -               if (ret)
>> -                       return ret;
>> -
>> -               /* Wait for the power to come up. */
>> -               timeout = jiffies + msecs_to_jiffies(100);
>> -               while (!tegra_pmc_cpu_is_powered(cpu)) {
>> -                       if (time_after(jiffies, timeout))
>> -                               return -ETIMEDOUT;
>> -                       udelay(10);
>> -               }
>> -       }
>> +       ret = tegra_pmc_cpu_power_on(cpu);
>> +       if (ret)
>> +               return ret;
>>
>>  remove_clamps:
>>         /* CPU partition is powered. Enable the CPU clock. */
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 99cb2fdd29e1..35ee60fd17be 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/export.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/iopoll.h>
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/platform_device.h>
>> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id)
>>   */
>>  static int tegra_powergate_set(unsigned int id, bool new_state)
>>  {
>> +       bool status;
>> +       int err;
>> +
>>         mutex_lock(&pmc->powergates_lock);
>>
>>         if (tegra_powergate_state(id) == new_state) {
>> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>>
>>         tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>>
>> +       err = readx_poll_timeout(tegra_powergate_state, id, status,
>> +                                status == new_state, 10, 100000);
>> +
> 
> I understand (and agree) with the goal of this patch but I wonder (and
> I don't know much about the system/context) if holding a mutex while
> sleeping won't incur adverse effect on other parts of the system that
> weren't use to see this wait.  One way to fix this might be to use
> "mutex_trylock()" and let callers retry as they see fit if an
> operation is already in progress.

I could unlock the mutex after writing the TOGGLE_START register. I
don't believe that we need to waiting for a powergate to change before
writing again.

Jon

Re: [PATCH V5 05/14] soc: tegra: pmc: Wait for powergate state to change

[Jon Hunter]

On 01/02/16 13:44, Jon Hunter wrote:
> 
> On 29/01/16 16:58, Mathieu Poirier wrote:
>> On 28 January 2016 at 09:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> Currently, the function tegra_powergate_set() simply sets the desired
>>> powergate state but does not wait for the state to change. In most cases
>>> we should wait for the state to change before proceeding. Currently, there
>>> is a case for tegra114 and tegra124 devices where we do not wait when
>>> starting the secondary CPU as this is not necessary. However, this is only
>>> done at boot time and so waiting here will only have a small impact on
>>> boot time. Therefore, update tegra_powergate_set() to wait when setting
>>> the powergate.
>>>
>>> By adding this feature, we can also eliminate the polling loop from
>>> tegra30_boot_secondary().
>>>
>>> A function has been added for checking the status of the powergate and
>>> so update the tegra_powergate_is_powered() to use this macro as well.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>>>  drivers/soc/tegra/pmc.c       |  9 ++++++++-
>>>  2 files changed, 11 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
>>> index f3f61dbbda97..75620ae73913 100644
>>> --- a/arch/arm/mach-tegra/platsmp.c
>>> +++ b/arch/arm/mach-tegra/platsmp.c
>>> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>>          * be un-gated by un-toggling the power gate register
>>>          * manually.
>>>          */
>>> -       if (!tegra_pmc_cpu_is_powered(cpu)) {
>>> -               ret = tegra_pmc_cpu_power_on(cpu);
>>> -               if (ret)
>>> -                       return ret;
>>> -
>>> -               /* Wait for the power to come up. */
>>> -               timeout = jiffies + msecs_to_jiffies(100);
>>> -               while (!tegra_pmc_cpu_is_powered(cpu)) {
>>> -                       if (time_after(jiffies, timeout))
>>> -                               return -ETIMEDOUT;
>>> -                       udelay(10);
>>> -               }
>>> -       }
>>> +       ret = tegra_pmc_cpu_power_on(cpu);
>>> +       if (ret)
>>> +               return ret;
>>>
>>>  remove_clamps:
>>>         /* CPU partition is powered. Enable the CPU clock. */
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index 99cb2fdd29e1..35ee60fd17be 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -28,6 +28,7 @@
>>>  #include <linux/export.h>
>>>  #include <linux/init.h>
>>>  #include <linux/io.h>
>>> +#include <linux/iopoll.h>
>>>  #include <linux/of.h>
>>>  #include <linux/of_address.h>
>>>  #include <linux/platform_device.h>
>>> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id)
>>>   */
>>>  static int tegra_powergate_set(unsigned int id, bool new_state)
>>>  {
>>> +       bool status;
>>> +       int err;
>>> +
>>>         mutex_lock(&pmc->powergates_lock);
>>>
>>>         if (tegra_powergate_state(id) == new_state) {
>>> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>>>
>>>         tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>>>
>>> +       err = readx_poll_timeout(tegra_powergate_state, id, status,
>>> +                                status == new_state, 10, 100000);
>>> +
>>
>> I understand (and agree) with the goal of this patch but I wonder (and
>> I don't know much about the system/context) if holding a mutex while
>> sleeping won't incur adverse effect on other parts of the system that
>> weren't use to see this wait.  One way to fix this might be to use
>> "mutex_trylock()" and let callers retry as they see fit if an
>> operation is already in progress.
> 
> I could unlock the mutex after writing the TOGGLE_START register. I
> don't believe that we need to waiting for a powergate to change before
> writing again.

I was not thinking clearly here, the whole reason for the locking is to
protect the base address as we swap it during the probe.

In general, I prefer to keep it the way we have it in this patch. By
using mutex_trylock() would mean that now clients would have to handle
retrying if they cannot get the lock and make it more complex.
Powergates should not be turned on and off that frequently and so
hopefully a delay here should be tolerable. For CPUs, this should only
be used on cold boot and not during CPU hotplug events.

Cheers
Jon

Re: [PATCH V5 05/14] soc: tegra: pmc: Wait for powergate state to change

[Mathieu Poirier]

On 3 February 2016 at 02:20, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 01/02/16 13:44, Jon Hunter wrote:
>>
>> On 29/01/16 16:58, Mathieu Poirier wrote:
>>> On 28 January 2016 at 09:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Currently, the function tegra_powergate_set() simply sets the desired
>>>> powergate state but does not wait for the state to change. In most cases
>>>> we should wait for the state to change before proceeding. Currently, there
>>>> is a case for tegra114 and tegra124 devices where we do not wait when
>>>> starting the secondary CPU as this is not necessary. However, this is only
>>>> done at boot time and so waiting here will only have a small impact on
>>>> boot time. Therefore, update tegra_powergate_set() to wait when setting
>>>> the powergate.
>>>>
>>>> By adding this feature, we can also eliminate the polling loop from
>>>> tegra30_boot_secondary().
>>>>
>>>> A function has been added for checking the status of the powergate and
>>>> so update the tegra_powergate_is_powered() to use this macro as well.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  arch/arm/mach-tegra/platsmp.c | 16 +++-------------
>>>>  drivers/soc/tegra/pmc.c       |  9 ++++++++-
>>>>  2 files changed, 11 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
>>>> index f3f61dbbda97..75620ae73913 100644
>>>> --- a/arch/arm/mach-tegra/platsmp.c
>>>> +++ b/arch/arm/mach-tegra/platsmp.c
>>>> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>>>          * be un-gated by un-toggling the power gate register
>>>>          * manually.
>>>>          */
>>>> -       if (!tegra_pmc_cpu_is_powered(cpu)) {
>>>> -               ret = tegra_pmc_cpu_power_on(cpu);
>>>> -               if (ret)
>>>> -                       return ret;
>>>> -
>>>> -               /* Wait for the power to come up. */
>>>> -               timeout = jiffies + msecs_to_jiffies(100);
>>>> -               while (!tegra_pmc_cpu_is_powered(cpu)) {
>>>> -                       if (time_after(jiffies, timeout))
>>>> -                               return -ETIMEDOUT;
>>>> -                       udelay(10);
>>>> -               }
>>>> -       }
>>>> +       ret = tegra_pmc_cpu_power_on(cpu);
>>>> +       if (ret)
>>>> +               return ret;
>>>>
>>>>  remove_clamps:
>>>>         /* CPU partition is powered. Enable the CPU clock. */
>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>> index 99cb2fdd29e1..35ee60fd17be 100644
>>>> --- a/drivers/soc/tegra/pmc.c
>>>> +++ b/drivers/soc/tegra/pmc.c
>>>> @@ -28,6 +28,7 @@
>>>>  #include <linux/export.h>
>>>>  #include <linux/init.h>
>>>>  #include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>>  #include <linux/of.h>
>>>>  #include <linux/of_address.h>
>>>>  #include <linux/platform_device.h>
>>>> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id)
>>>>   */
>>>>  static int tegra_powergate_set(unsigned int id, bool new_state)
>>>>  {
>>>> +       bool status;
>>>> +       int err;
>>>> +
>>>>         mutex_lock(&pmc->powergates_lock);
>>>>
>>>>         if (tegra_powergate_state(id) == new_state) {
>>>> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>>>>
>>>>         tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>>>>
>>>> +       err = readx_poll_timeout(tegra_powergate_state, id, status,
>>>> +                                status == new_state, 10, 100000);
>>>> +
>>>
>>> I understand (and agree) with the goal of this patch but I wonder (and
>>> I don't know much about the system/context) if holding a mutex while
>>> sleeping won't incur adverse effect on other parts of the system that
>>> weren't use to see this wait.  One way to fix this might be to use
>>> "mutex_trylock()" and let callers retry as they see fit if an
>>> operation is already in progress.
>>
>> I could unlock the mutex after writing the TOGGLE_START register. I
>> don't believe that we need to waiting for a powergate to change before
>> writing again.
>
> I was not thinking clearly here, the whole reason for the locking is to
> protect the base address as we swap it during the probe.
>
> In general, I prefer to keep it the way we have it in this patch. By
> using mutex_trylock() would mean that now clients would have to handle
> retrying if they cannot get the lock and make it more complex.

Yes, the complexity is indeed pushed to the caller but that way we
know exactly what is going on.  Another way to implement this without
any locks at all would be through RCUs.  I'll let Stephen and Thierry
make the call here.

Thanks,
Mahtieu

> Powergates should not be turned on and off that frequently and so
> hopefully a delay here should be tolerable. For CPUs, this should only
> be used on cold boot and not during CPU hotplug events.
>
> Cheers
> Jon

[PATCH V5 06/14] soc: tegra: pmc: Fix checking of valid partitions

[Jon Hunter]

The tegra power partitions are referenced by a numerical ID which are
the same values programmed into the PMC registers for controlling the
partition. For a given device, the valid partition IDs may not be
contiguous and so simply checking that an ID is not greater than the
maximum ID supported may not mean it is valid. Fix this by adding a
bitmap for representing the valid partitions of a device and add a
helper function will test if the partition is valid.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/soc/tegra/pmc.c | 21 +++++++++++++++++----
 include/soc/tegra/pmc.h |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 35ee60fd17be..032dd5c17130 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -132,6 +132,7 @@ struct tegra_pmc_soc {
  * @cpu_pwr_good_en: CPU power good signal is enabled
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
+ * @powergates_valid: Bitmap of valid power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {
@@ -156,6 +157,7 @@ struct tegra_pmc {
 	bool cpu_pwr_good_en;
 	u32 lp0_vec_phys;
 	u32 lp0_vec_size;
+	DECLARE_BITMAP(powergates_valid, TEGRA_POWERGATE_MAX);
 
 	struct mutex powergates_lock;
 };
@@ -180,6 +182,11 @@ static inline bool tegra_powergate_state(int id)
 	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
 }
 
+static inline bool tegra_powergate_is_valid(int id)
+{
+	return test_bit(id, pmc->powergates_valid);
+}
+
 /**
  * tegra_powergate_set() - set the state of a partition
  * @id: partition ID
@@ -213,7 +220,7 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
  */
 int tegra_powergate_power_on(unsigned int id)
 {
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, true);
@@ -225,7 +232,7 @@ int tegra_powergate_power_on(unsigned int id)
  */
 int tegra_powergate_power_off(unsigned int id)
 {
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);
@@ -240,7 +247,7 @@ int tegra_powergate_is_powered(unsigned int id)
 {
 	int status;
 
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
@@ -258,7 +265,7 @@ int tegra_powergate_remove_clamping(unsigned int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
@@ -1118,6 +1125,7 @@ static int __init tegra_pmc_early_init(void)
 	const struct of_device_id *match;
 	struct device_node *np;
 	struct resource regs;
+	unsigned int i;
 	bool invert;
 	u32 value;
 
@@ -1167,6 +1175,11 @@ static int __init tegra_pmc_early_init(void)
 		return -ENXIO;
 	}
 
+	/* Create a bit-mask of the valid partitions */
+	for (i = 0; i < pmc->soc->num_powergates; i++)
+		if (pmc->soc->powergates[i])
+			set_bit(i, pmc->powergates_valid);
+
 	mutex_init(&pmc->powergates_lock);
 
 	/*
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 07e332dd44fb..e9e53473a63e 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -72,6 +72,7 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
 #define TEGRA_POWERGATE_AUD	27
 #define TEGRA_POWERGATE_DFD	28
 #define TEGRA_POWERGATE_VE2	29
+#define TEGRA_POWERGATE_MAX	TEGRA_POWERGATE_VE2
 
 #define TEGRA_POWERGATE_3D0	TEGRA_POWERGATE_3D
 
-- 
2.1.4

Re: [PATCH V5 06/14] soc: tegra: pmc: Fix checking of valid partitions

[Mathieu Poirier]

On 28 January 2016 at 09:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> The tegra power partitions are referenced by a numerical ID which are
> the same values programmed into the PMC registers for controlling the
> partition. For a given device, the valid partition IDs may not be
> contiguous and so simply checking that an ID is not greater than the
> maximum ID supported may not mean it is valid. Fix this by adding a
> bitmap for representing the valid partitions of a device and add a
> helper function will test if the partition is valid.
>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 21 +++++++++++++++++----
>  include/soc/tegra/pmc.h |  1 +
>  2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 35ee60fd17be..032dd5c17130 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -132,6 +132,7 @@ struct tegra_pmc_soc {
>   * @cpu_pwr_good_en: CPU power good signal is enabled
>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>   * @lp0_vec_size: size of the LP0 warm boot code
> + * @powergates_valid: Bitmap of valid power gates
>   * @powergates_lock: mutex for power gate register access
>   */
>  struct tegra_pmc {
> @@ -156,6 +157,7 @@ struct tegra_pmc {
>         bool cpu_pwr_good_en;
>         u32 lp0_vec_phys;
>         u32 lp0_vec_size;
> +       DECLARE_BITMAP(powergates_valid, TEGRA_POWERGATE_MAX);
>
>         struct mutex powergates_lock;
>  };
> @@ -180,6 +182,11 @@ static inline bool tegra_powergate_state(int id)
>         return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>  }
>
> +static inline bool tegra_powergate_is_valid(int id)
> +{
> +       return test_bit(id, pmc->powergates_valid);
> +}
> +
>  /**
>   * tegra_powergate_set() - set the state of a partition
>   * @id: partition ID
> @@ -213,7 +220,7 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>   */
>  int tegra_powergate_power_on(unsigned int id)
>  {
> -       if (!pmc->soc || id >= pmc->soc->num_powergates)
> +       if (!tegra_powergate_is_valid(id))
>                 return -EINVAL;

The "!pmc-soc" condition is no longer needed?  If so the changelog
should reflect that or a new patch that deals with just that should be
etched.  The same comment applies for the rest of the patch.

>
>         return tegra_powergate_set(id, true);
> @@ -225,7 +232,7 @@ int tegra_powergate_power_on(unsigned int id)
>   */
>  int tegra_powergate_power_off(unsigned int id)
>  {
> -       if (!pmc->soc || id >= pmc->soc->num_powergates)
> +       if (!tegra_powergate_is_valid(id))
>                 return -EINVAL;
>
>         return tegra_powergate_set(id, false);
> @@ -240,7 +247,7 @@ int tegra_powergate_is_powered(unsigned int id)
>  {
>         int status;
>
> -       if (!pmc->soc || id >= pmc->soc->num_powergates)
> +       if (!tegra_powergate_is_valid(id))
>                 return -EINVAL;
>
>         mutex_lock(&pmc->powergates_lock);
> @@ -258,7 +265,7 @@ int tegra_powergate_remove_clamping(unsigned int id)
>  {
>         u32 mask;
>
> -       if (!pmc->soc || id >= pmc->soc->num_powergates)
> +       if (!tegra_powergate_is_valid(id))
>                 return -EINVAL;
>
>         mutex_lock(&pmc->powergates_lock);
> @@ -1118,6 +1125,7 @@ static int __init tegra_pmc_early_init(void)
>         const struct of_device_id *match;
>         struct device_node *np;
>         struct resource regs;
> +       unsigned int i;
>         bool invert;
>         u32 value;
>
> @@ -1167,6 +1175,11 @@ static int __init tegra_pmc_early_init(void)
>                 return -ENXIO;
>         }
>
> +       /* Create a bit-mask of the valid partitions */
> +       for (i = 0; i < pmc->soc->num_powergates; i++)
> +               if (pmc->soc->powergates[i])
> +                       set_bit(i, pmc->powergates_valid);
> +
>         mutex_init(&pmc->powergates_lock);
>
>         /*
> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
> index 07e332dd44fb..e9e53473a63e 100644
> --- a/include/soc/tegra/pmc.h
> +++ b/include/soc/tegra/pmc.h
> @@ -72,6 +72,7 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
>  #define TEGRA_POWERGATE_AUD    27
>  #define TEGRA_POWERGATE_DFD    28
>  #define TEGRA_POWERGATE_VE2    29
> +#define TEGRA_POWERGATE_MAX    TEGRA_POWERGATE_VE2
>
>  #define TEGRA_POWERGATE_3D0    TEGRA_POWERGATE_3D
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 06/14] soc: tegra: pmc: Fix checking of valid partitions

[Jon Hunter]

On 29/01/16 17:08, Mathieu Poirier wrote:
> On 28 January 2016 at 09:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> The tegra power partitions are referenced by a numerical ID which are
>> the same values programmed into the PMC registers for controlling the
>> partition. For a given device, the valid partition IDs may not be
>> contiguous and so simply checking that an ID is not greater than the
>> maximum ID supported may not mean it is valid. Fix this by adding a
>> bitmap for representing the valid partitions of a device and add a
>> helper function will test if the partition is valid.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/soc/tegra/pmc.c | 21 +++++++++++++++++----
>>  include/soc/tegra/pmc.h |  1 +
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 35ee60fd17be..032dd5c17130 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -132,6 +132,7 @@ struct tegra_pmc_soc {
>>   * @cpu_pwr_good_en: CPU power good signal is enabled
>>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>>   * @lp0_vec_size: size of the LP0 warm boot code
>> + * @powergates_valid: Bitmap of valid power gates
>>   * @powergates_lock: mutex for power gate register access
>>   */
>>  struct tegra_pmc {
>> @@ -156,6 +157,7 @@ struct tegra_pmc {
>>         bool cpu_pwr_good_en;
>>         u32 lp0_vec_phys;
>>         u32 lp0_vec_size;
>> +       DECLARE_BITMAP(powergates_valid, TEGRA_POWERGATE_MAX);
>>
>>         struct mutex powergates_lock;
>>  };
>> @@ -180,6 +182,11 @@ static inline bool tegra_powergate_state(int id)
>>         return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>>  }
>>
>> +static inline bool tegra_powergate_is_valid(int id)
>> +{
>> +       return test_bit(id, pmc->powergates_valid);
>> +}
>> +
>>  /**
>>   * tegra_powergate_set() - set the state of a partition
>>   * @id: partition ID
>> @@ -213,7 +220,7 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
>>   */
>>  int tegra_powergate_power_on(unsigned int id)
>>  {
>> -       if (!pmc->soc || id >= pmc->soc->num_powergates)
>> +       if (!tegra_powergate_is_valid(id))
>>                 return -EINVAL;
> 
> The "!pmc-soc" condition is no longer needed?  If so the changelog
> should reflect that or a new patch that deals with just that should be
> etched.  The same comment applies for the rest of the patch.

Yes it is not necessary. I will update the changelog.

Cheers
Jon

[PATCH V5 09/14] Documentation: DT: bindings: Update NVIDIA PMC for Tegra

[Jon Hunter]

Add the PMC driver compatible strings for Tegra132 and Tegra210.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt     | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 02c27004d4a8..53aa5496c5cf 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -6,11 +6,13 @@ modes. It provides power-gating controllers for SoC and CPU power-islands.
 
 Required properties:
 - name : Should be pmc
-- compatible : For Tegra20, must contain "nvidia,tegra20-pmc".  For Tegra30,
-  must contain "nvidia,tegra30-pmc".  For Tegra114, must contain
-  "nvidia,tegra114-pmc".  For Tegra124, must contain "nvidia,tegra124-pmc".
-  Otherwise, must contain "nvidia,<chip>-pmc", plus at least one of the
-  above, where <chip> is tegra132.
+- compatible : Should contain one of the following:
+	For Tegra20 must contain "nvidia,tegra20-pmc".
+	For Tegra30 must contain "nvidia,tegra30-pmc".
+	For Tegra114 must contain "nvidia,tegra114-pmc"
+	For Tegra124 must contain "nvidia,tegra124-pmc"
+	For Tegra132 must contain "nvidia,tegra124-pmc"
+	For Tegra210 must contain "nvidia,tegra210-pmc"
 - reg : Offset and length of the register set for the device
 - clocks : Must contain an entry for each entry in clock-names.
   See ../clocks/clock-bindings.txt for details.
-- 
2.1.4

Re: [PATCH V5 09/14] Documentation: DT: bindings: Update NVIDIA PMC for Tegra

[Rob Herring]

On Thu, Jan 28, 2016 at 04:33:47PM +0000, Jon Hunter wrote:
> Add the PMC driver compatible strings for Tegra132 and Tegra210.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt     | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

Add power-domain binding documentation for the NVIDIA PMC driver in
order to support generic power-domains.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 53aa5496c5cf..3c77373aa826 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -1,5 +1,7 @@
 NVIDIA Tegra Power Management Controller (PMC)
 
+== Power Management Controller Node ==
+
 The PMC block interacts with an external Power Management Unit. The PMC
 mostly controls the entry and exit of the system from different sleep
 modes. It provides power-gating controllers for SoC and CPU power-islands.
@@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
                      Defaults to 0. Valid values are described in section 12.5.2
                      "Pinmux Support" of the Tegra4 Technical Reference Manual.
 
+Optional nodes:
+- powergates : This node contains a hierarchy of power domain nodes, which
+	       should match the powergates on the Tegra SoC.
+
 Example:
 
 / SoC dts including file
@@ -115,3 +121,52 @@ pmc@7000f400 {
 	};
 	...
 };
+
+
+== PM Domain Nodes ==
+
+Each of the PM domain nodes represents a power-domain on the Tegra SoC
+that can be power-gated by the PMC and should be named appropriately.
+
+Required properties:
+  - clocks: Must contain an entry for each clock required by the PMC for
+    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
+  - resets: Must contain an entry for each reset required by the PMC for
+    controlling a power-gate. See ../reset/reset.txt for details.
+  - nvidia,powergate: Integer cell that contains an identifier for the PMC
+    power-gate that is associated with the power-domain. Please refer to
+    the Tegra TRM for more details.
+  - #power-domain-cells: Must be 0.
+
+Example:
+
+	pmc: pmc@0,7000e400 {
+		compatible = "nvidia,tegra210-pmc";
+		reg = <0x0 0x7000e400 0x0 0x400>;
+		clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
+		clock-names = "pclk", "clk32k_in";
+
+		powergates {
+			pd_audio: aud {
+				clocks = <&tegra_car TEGRA210_CLK_APE>,
+					 <&tegra_car TEGRA210_CLK_APB2APE>;
+				resets = <&tegra_car 198>;
+				nvidia,powergate = <TEGRA_POWERGATE_AUD>;
+				#power-domain-cells = <0>;
+			};
+		};
+	};
+
+
+== PM Domain Consumers ==
+
+Hardware blocks belonging to a PM domain should contain a "power-domains"
+property that is a phandle pointing to the corresponding PM domain node.
+
+Example:
+
+	adma: adma@702e2000 {
+		...
+		power-domains = <&pd_audio>;
+		...
+	};
-- 
2.1.4

Re: [PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Rob Herring]

On Thu, Jan 28, 2016 at 04:33:48PM +0000, Jon Hunter wrote:
> Add power-domain binding documentation for the NVIDIA PMC driver in
> order to support generic power-domains.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 53aa5496c5cf..3c77373aa826 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -1,5 +1,7 @@
>  NVIDIA Tegra Power Management Controller (PMC)
>  
> +== Power Management Controller Node ==
> +
>  The PMC block interacts with an external Power Management Unit. The PMC
>  mostly controls the entry and exit of the system from different sleep
>  modes. It provides power-gating controllers for SoC and CPU power-islands.
> @@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>                       Defaults to 0. Valid values are described in section 12.5.2
>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>  
> +Optional nodes:
> +- powergates : This node contains a hierarchy of power domain nodes, which
> +	       should match the powergates on the Tegra SoC.
> +
>  Example:
>  
>  / SoC dts including file
> @@ -115,3 +121,52 @@ pmc@7000f400 {
>  	};
>  	...
>  };
> +
> +
> +== PM Domain Nodes ==
> +
> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
> +that can be power-gated by the PMC and should be named appropriately.
> +
> +Required properties:
> +  - clocks: Must contain an entry for each clock required by the PMC for
> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
> +  - resets: Must contain an entry for each reset required by the PMC for
> +    controlling a power-gate. See ../reset/reset.txt for details.
> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
> +    power-gate that is associated with the power-domain. Please refer to
> +    the Tegra TRM for more details.

Why not make this value be the power-domain cell for consumers and the 
pmc be the phandle. Then use reg property for the subnodes. That avoids 
a custom property.

> +  - #power-domain-cells: Must be 0.

Re: [PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 29/01/16 16:06, Rob Herring wrote:
> On Thu, Jan 28, 2016 at 04:33:48PM +0000, Jon Hunter wrote:
>> Add power-domain binding documentation for the NVIDIA PMC driver in
>> order to support generic power-domains.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 53aa5496c5cf..3c77373aa826 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -1,5 +1,7 @@
>>  NVIDIA Tegra Power Management Controller (PMC)
>>  
>> +== Power Management Controller Node ==
>> +
>>  The PMC block interacts with an external Power Management Unit. The PMC
>>  mostly controls the entry and exit of the system from different sleep
>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>> @@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>                       Defaults to 0. Valid values are described in section 12.5.2
>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>  
>> +Optional nodes:
>> +- powergates : This node contains a hierarchy of power domain nodes, which
>> +	       should match the powergates on the Tegra SoC.
>> +
>>  Example:
>>  
>>  / SoC dts including file
>> @@ -115,3 +121,52 @@ pmc@7000f400 {
>>  	};
>>  	...
>>  };
>> +
>> +
>> +== PM Domain Nodes ==
>> +
>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>> +that can be power-gated by the PMC and should be named appropriately.
>> +
>> +Required properties:
>> +  - clocks: Must contain an entry for each clock required by the PMC for
>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>> +  - resets: Must contain an entry for each reset required by the PMC for
>> +    controlling a power-gate. See ../reset/reset.txt for details.
>> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
>> +    power-gate that is associated with the power-domain. Please refer to
>> +    the Tegra TRM for more details.
> 
> Why not make this value be the power-domain cell for consumers and the 
> pmc be the phandle.

Is there anything wrong with using a label for the consumers? Seems
simpler, if you can just say ...

	adma: adma@702e2000 {
		...
		power-domains = <&pd_audio>;
		...
	};

> Then use reg property for the subnodes. That avoids a custom property.

That is fine with me.

Jon

Re: [PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Rob Herring]

On Wed, Feb 3, 2016 at 5:02 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 29/01/16 16:06, Rob Herring wrote:
>> On Thu, Jan 28, 2016 at 04:33:48PM +0000, Jon Hunter wrote:
>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>> order to support generic power-domains.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> index 53aa5496c5cf..3c77373aa826 100644
>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>> @@ -1,5 +1,7 @@
>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>
>>> +== Power Management Controller Node ==
>>> +
>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>  mostly controls the entry and exit of the system from different sleep
>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>> @@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>
>>> +Optional nodes:
>>> +- powergates : This node contains a hierarchy of power domain nodes, which
>>> +           should match the powergates on the Tegra SoC.
>>> +
>>>  Example:
>>>
>>>  / SoC dts including file
>>> @@ -115,3 +121,52 @@ pmc@7000f400 {
>>>      };
>>>      ...
>>>  };
>>> +
>>> +
>>> +== PM Domain Nodes ==
>>> +
>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>> +that can be power-gated by the PMC and should be named appropriately.
>>> +
>>> +Required properties:
>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>>> +  - resets: Must contain an entry for each reset required by the PMC for
>>> +    controlling a power-gate. See ../reset/reset.txt for details.
>>> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
>>> +    power-gate that is associated with the power-domain. Please refer to
>>> +    the Tegra TRM for more details.
>>
>> Why not make this value be the power-domain cell for consumers and the
>> pmc be the phandle.
>
> Is there anything wrong with using a label for the consumers? Seems
> simpler, if you can just say ...
>
>         adma: adma@702e2000 {
>                 ...
>                 power-domains = <&pd_audio>;
>                 ...
>         };

Only that is a bit unusual. It would only really matter if we had some
common parsing that we wanted to share. I'd look at other examples and
try to align. If there's no clear pattern, then it is fine.

>> Then use reg property for the subnodes. That avoids a custom property.
>
> That is fine with me.
>
> Jon

Re: [PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Jon Hunter]

On 03/02/16 15:48, Rob Herring wrote:
> On Wed, Feb 3, 2016 at 5:02 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On 29/01/16 16:06, Rob Herring wrote:
>>> On Thu, Jan 28, 2016 at 04:33:48PM +0000, Jon Hunter wrote:
>>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>>> order to support generic power-domains.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
>>>>  1 file changed, 55 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> index 53aa5496c5cf..3c77373aa826 100644
>>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>> @@ -1,5 +1,7 @@
>>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>>
>>>> +== Power Management Controller Node ==
>>>> +
>>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>>  mostly controls the entry and exit of the system from different sleep
>>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>>> @@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>>
>>>> +Optional nodes:
>>>> +- powergates : This node contains a hierarchy of power domain nodes, which
>>>> +           should match the powergates on the Tegra SoC.
>>>> +
>>>>  Example:
>>>>
>>>>  / SoC dts including file
>>>> @@ -115,3 +121,52 @@ pmc@7000f400 {
>>>>      };
>>>>      ...
>>>>  };
>>>> +
>>>> +
>>>> +== PM Domain Nodes ==
>>>> +
>>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>>> +that can be power-gated by the PMC and should be named appropriately.
>>>> +
>>>> +Required properties:
>>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>>>> +  - resets: Must contain an entry for each reset required by the PMC for
>>>> +    controlling a power-gate. See ../reset/reset.txt for details.
>>>> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
>>>> +    power-gate that is associated with the power-domain. Please refer to
>>>> +    the Tegra TRM for more details.
>>>
>>> Why not make this value be the power-domain cell for consumers and the
>>> pmc be the phandle.
>>
>> Is there anything wrong with using a label for the consumers? Seems
>> simpler, if you can just say ...
>>
>>         adma: adma@702e2000 {
>>                 ...
>>                 power-domains = <&pd_audio>;
>>                 ...
>>         };
> 
> Only that is a bit unusual. It would only really matter if we had some
> common parsing that we wanted to share. I'd look at other examples and
> try to align. If there's no clear pattern, then it is fine.

So I have been having a look at this and it appears to be mixed across
the various SoCs. It seems that those SoCs that define the generic PM
domains statically in the machine/soc code use a phandle plus a argument
to identify the power domain. However, for those that describe the
power-domain in the DT (listing clocks, resets, etc), they just use a
phandle to the power-domain itself (like I have above). It looks like if
you are describing the power domain in DT, then it is much simplier to
do this.

Given that I wish to keep the description of the power-domain in DT, it
is much easier to stick with the same conventions that the other SoCs
are using.

Cheers
Jon

Re: [PATCH V5 10/14] Documentation: DT: bindings: Add power domain info for NVIDIA PMC

[Rob Herring]

On Wed, Feb 10, 2016 at 4:57 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 03/02/16 15:48, Rob Herring wrote:
>> On Wed, Feb 3, 2016 at 5:02 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> On 29/01/16 16:06, Rob Herring wrote:
>>>> On Thu, Jan 28, 2016 at 04:33:48PM +0000, Jon Hunter wrote:
>>>>> Add power-domain binding documentation for the NVIDIA PMC driver in
>>>>> order to support generic power-domains.
>>>>>
>>>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>  .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
>>>>>  1 file changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> index 53aa5496c5cf..3c77373aa826 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>>>>> @@ -1,5 +1,7 @@
>>>>>  NVIDIA Tegra Power Management Controller (PMC)
>>>>>
>>>>> +== Power Management Controller Node ==
>>>>> +
>>>>>  The PMC block interacts with an external Power Management Unit. The PMC
>>>>>  mostly controls the entry and exit of the system from different sleep
>>>>>  modes. It provides power-gating controllers for SoC and CPU power-islands.
>>>>> @@ -70,6 +72,10 @@ Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip'
>>>>>                       Defaults to 0. Valid values are described in section 12.5.2
>>>>>                       "Pinmux Support" of the Tegra4 Technical Reference Manual.
>>>>>
>>>>> +Optional nodes:
>>>>> +- powergates : This node contains a hierarchy of power domain nodes, which
>>>>> +           should match the powergates on the Tegra SoC.
>>>>> +
>>>>>  Example:
>>>>>
>>>>>  / SoC dts including file
>>>>> @@ -115,3 +121,52 @@ pmc@7000f400 {
>>>>>      };
>>>>>      ...
>>>>>  };
>>>>> +
>>>>> +
>>>>> +== PM Domain Nodes ==
>>>>> +
>>>>> +Each of the PM domain nodes represents a power-domain on the Tegra SoC
>>>>> +that can be power-gated by the PMC and should be named appropriately.
>>>>> +
>>>>> +Required properties:
>>>>> +  - clocks: Must contain an entry for each clock required by the PMC for
>>>>> +    controlling a power-gate. See ../clocks/clock-bindings.txt for details.
>>>>> +  - resets: Must contain an entry for each reset required by the PMC for
>>>>> +    controlling a power-gate. See ../reset/reset.txt for details.
>>>>> +  - nvidia,powergate: Integer cell that contains an identifier for the PMC
>>>>> +    power-gate that is associated with the power-domain. Please refer to
>>>>> +    the Tegra TRM for more details.
>>>>
>>>> Why not make this value be the power-domain cell for consumers and the
>>>> pmc be the phandle.
>>>
>>> Is there anything wrong with using a label for the consumers? Seems
>>> simpler, if you can just say ...
>>>
>>>         adma: adma@702e2000 {
>>>                 ...
>>>                 power-domains = <&pd_audio>;
>>>                 ...
>>>         };
>>
>> Only that is a bit unusual. It would only really matter if we had some
>> common parsing that we wanted to share. I'd look at other examples and
>> try to align. If there's no clear pattern, then it is fine.
>
> So I have been having a look at this and it appears to be mixed across
> the various SoCs. It seems that those SoCs that define the generic PM
> domains statically in the machine/soc code use a phandle plus a argument
> to identify the power domain. However, for those that describe the
> power-domain in the DT (listing clocks, resets, etc), they just use a
> phandle to the power-domain itself (like I have above). It looks like if
> you are describing the power domain in DT, then it is much simplier to
> do this.
>
> Given that I wish to keep the description of the power-domain in DT, it
> is much easier to stick with the same conventions that the other SoCs
> are using.

Okay.

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Rob

[PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

Adds generic PM support to the PMC driver where the PM domains are
populated from device-tree and the PM domain consumer devices are
bound to their relevant PM domains via device-tree as well.

Update the tegra_powergate_sequence_power_up() API so that internally
it calls the same tegra_powergate_xxx functions that are used by the
tegra generic power domain code for consistency.

This is based upon work by Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
and Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/soc/tegra/pmc.c                     | 470 ++++++++++++++++++++++++++--
 include/dt-bindings/power/tegra-powergate.h |  36 +++
 include/soc/tegra/pmc.h                     |  39 +--
 3 files changed, 480 insertions(+), 65 deletions(-)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index ecb4f66819fd..915dc37e0d92 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -19,6 +19,8 @@
 
 #define pr_fmt(fmt) "tegra-pmc: " fmt
 
+#include <dt-bindings/power/tegra-powergate.h>
+
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
@@ -31,7 +33,9 @@
 #include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/reboot.h>
 #include <linux/reset.h>
 #include <linux/seq_file.h>
@@ -102,6 +106,19 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+struct tegra_powergate {
+	struct generic_pm_domain genpd;
+	struct generic_pm_domain *parent;
+	struct tegra_pmc *pmc;
+	unsigned int id;
+	struct list_head node;
+	struct device_node *of_node;
+	struct clk **clks;
+	unsigned int num_clks;
+	struct reset_control **resets;
+	unsigned int num_resets;
+};
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -134,6 +151,7 @@ struct tegra_pmc_soc {
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
  * @powergates_valid: Bitmap of valid power gates
+ * @powergates_list: list of power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {
@@ -160,6 +178,7 @@ struct tegra_pmc {
 	u32 lp0_vec_size;
 	DECLARE_BITMAP(powergates_valid, TEGRA_POWERGATE_MAX);
 
+	struct list_head powergates_list;
 	struct mutex powergates_lock;
 };
 
@@ -168,6 +187,12 @@ static struct tegra_pmc *pmc = &(struct tegra_pmc) {
 	.suspend_mode = TEGRA_SUSPEND_NONE,
 };
 
+static inline struct tegra_powergate *
+to_powergate(struct generic_pm_domain *domain)
+{
+	return container_of(domain, struct tegra_powergate, genpd);
+}
+
 static u32 tegra_pmc_readl(unsigned long offset)
 {
 	return readl(pmc->base + offset);
@@ -218,6 +243,174 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
 	return err;
 }
 
+static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
+{
+	unsigned int i;
+
+	for (i = 0; i < pg->num_clks; i++)
+		clk_disable_unprepare(pg->clks[i]);
+}
+
+static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_clks; i++) {
+		err = clk_prepare_enable(pg->clks[i]);
+		if (err)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	while (i--)
+		clk_disable_unprepare(pg->clks[i]);
+
+	return err;
+}
+
+static int tegra_powergate_reset_assert(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_resets; i++) {
+		err = reset_control_assert(pg->resets[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_powergate_reset_deassert(struct tegra_powergate *pg)
+{
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < pg->num_resets; i++) {
+		err = reset_control_deassert(pg->resets[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_powergate_power_up(struct tegra_powergate *pg,
+				    bool disable_clocks)
+{
+	int err;
+
+	err = tegra_powergate_reset_assert(pg);
+	if (err)
+		return err;
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_set(pg->id, true);
+	if (err < 0)
+		return err;
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_enable_clocks(pg);
+	if (err)
+		goto err_clks;
+
+	usleep_range(10, 20);
+
+	tegra_powergate_remove_clamping(pg->id);
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_reset_deassert(pg);
+	if (err)
+		goto err_reset;
+
+	usleep_range(10, 20);
+
+	if (disable_clocks)
+		tegra_powergate_disable_clocks(pg);
+
+	return 0;
+
+err_reset:
+	tegra_powergate_disable_clocks(pg);
+	usleep_range(10, 20);
+err_clks:
+	tegra_powergate_set(pg->id, false);
+
+	return err;
+}
+
+static int tegra_powergate_power_down(struct tegra_powergate *pg)
+{
+	int err;
+
+	err = tegra_powergate_enable_clocks(pg);
+	if (err)
+		return err;
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_reset_assert(pg);
+	if (err)
+		goto err_reset;
+
+	usleep_range(10, 20);
+
+	tegra_powergate_disable_clocks(pg);
+
+	usleep_range(10, 20);
+
+	err = tegra_powergate_set(pg->id, false);
+	if (err)
+		goto err_powergate;
+
+	return 0;
+
+err_powergate:
+	tegra_powergate_enable_clocks(pg);
+	usleep_range(10, 20);
+	tegra_powergate_reset_deassert(pg);
+	usleep_range(10, 20);
+err_reset:
+	tegra_powergate_disable_clocks(pg);
+
+	return err;
+}
+
+static int tegra_genpd_power_on(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *pg = to_powergate(domain);
+	struct tegra_pmc *pmc = pg->pmc;
+	int err;
+
+	err = tegra_powergate_power_up(pg, true);
+	if (err)
+		dev_err(pmc->dev, "failed to turn on PM domain %s: %d\n",
+			pg->of_node->name, err);
+
+	return err;
+}
+
+static int tegra_genpd_power_off(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *pg = to_powergate(domain);
+	struct tegra_pmc *pmc = pg->pmc;
+	int err;
+
+	err = tegra_powergate_power_down(pg);
+	if (err)
+		dev_err(pmc->dev, "failed to turn off PM domain %s: %d\n",
+			pg->of_node->name, err);
+
+	return err;
+}
+
 /**
  * tegra_powergate_power_on() - power on partition
  * @id: partition ID
@@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
 int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 				      struct reset_control *rst)
 {
-	int ret;
-
-	reset_control_assert(rst);
-
-	ret = tegra_powergate_power_on(id);
-	if (ret)
-		goto err_power;
-
-	ret = clk_prepare_enable(clk);
-	if (ret)
-		goto err_clk;
-
-	usleep_range(10, 20);
+	struct tegra_powergate pg;
+	int err;
 
-	ret = tegra_powergate_remove_clamping(id);
-	if (ret)
-		goto err_clamp;
+	pg.id = id;
+	pg.clks = &clk;
+	pg.num_clks = 1;
+	pg.resets = &rst;
+	pg.num_resets = 1;
 
-	usleep_range(10, 20);
-	reset_control_deassert(rst);
+	err = tegra_powergate_power_up(&pg, false);
+	if (err)
+		pr_err("failed to turn on partition %d: %d\n", id, err);
 
-	return 0;
-
-err_clamp:
-	clk_disable_unprepare(clk);
-err_clk:
-	tegra_powergate_power_off(id);
-err_power:
-	return ret;
+	return err;
 }
 EXPORT_SYMBOL(tegra_powergate_sequence_power_up);
 
@@ -487,6 +665,233 @@ static int tegra_powergate_debugfs_init(void)
 	return 0;
 }
 
+static int tegra_powergate_of_get_clks(struct device *dev,
+				       struct tegra_powergate *pg)
+{
+	struct clk *clk;
+	unsigned int i;
+	int err;
+
+	pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks",
+						  "#clock-cells");
+	if (pg->num_clks == 0)
+		return -ENODEV;
+
+	pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL);
+	if (!pg->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < pg->num_clks; i++) {
+		pg->clks[i] = of_clk_get(pg->of_node, i);
+		if (IS_ERR(pg->clks[i])) {
+			err = PTR_ERR(pg->clks[i]);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (i--)
+		clk_put(pg->clks[i]);
+
+	pg->num_clks = 0;
+
+	return err;
+}
+
+static int tegra_powergate_of_get_resets(struct device *dev,
+					 struct tegra_powergate *pg)
+{
+	struct reset_control *rst;
+	unsigned int i;
+	int err;
+
+	pg->num_resets = of_count_phandle_with_args(pg->of_node, "resets",
+					   "#reset-cells");
+	if (pg->num_resets == 0)
+		return -ENODEV;
+
+	pg->resets = devm_kcalloc(dev, pg->num_resets, sizeof(rst), GFP_KERNEL);
+	if (!pg->resets)
+		return -ENOMEM;
+
+	for (i = 0; i < pg->num_resets; i++) {
+		pg->resets[i] = of_reset_control_get_by_index(pg->of_node, i);
+		if (IS_ERR(pg->resets[i])) {
+			err = PTR_ERR(pg->resets[i]);
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (i--)
+		reset_control_put(pg->resets[i]);
+
+	pg->num_resets = 0;
+
+	return err;
+}
+
+static struct tegra_powergate *
+tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
+			struct generic_pm_domain *parent)
+{
+	struct tegra_powergate *pg;
+	unsigned int id;
+	bool off;
+	int err;
+
+	/*
+	 * If the powergate ID is missing or invalid then return NULL
+	 * to skip this one and allow any others to be created.
+	 */
+	err = of_property_read_u32(np, "nvidia,powergate", &id);
+	if (err) {
+		dev_WARN(pmc->dev, "no powergate ID for node %s\n", np->name);
+		return NULL;
+	}
+
+	if (!tegra_powergate_is_valid(id)) {
+		dev_WARN(pmc->dev, "powergate ID invalid for %s\n", np->name);
+		return NULL;
+	}
+
+	pg = devm_kzalloc(pmc->dev, sizeof(*pg), GFP_KERNEL);
+	if (!pg) {
+		err = -ENOMEM;
+		goto error;
+	}
+
+	pg->id = id;
+	pg->of_node = np;
+	pg->parent = parent;
+	pg->genpd.name = np->name;
+	pg->genpd.power_off = tegra_genpd_power_off;
+	pg->genpd.power_on = tegra_genpd_power_on;
+	pg->pmc = pmc;
+
+	err = tegra_powergate_of_get_clks(pmc->dev, pg);
+	if (err)
+		goto error;
+
+	err = tegra_powergate_of_get_resets(pmc->dev, pg);
+	if (err)
+		goto remove_clks;
+
+	off = !tegra_powergate_is_powered(pg->id);
+
+	pm_genpd_init(&pg->genpd, NULL, off);
+
+	if (pg->parent) {
+		err = pm_genpd_add_subdomain(pg->parent, &pg->genpd);
+		if (err)
+			goto remove_domain_and_resets;
+	}
+
+	err = of_genpd_add_provider_simple(pg->of_node, &pg->genpd);
+	if (err)
+		goto remove_subdomain;
+
+	list_add_tail(&pg->node, &pmc->powergates_list);
+
+	dev_dbg(pmc->dev, "added power domain %s\n", pg->genpd.name);
+
+	return pg;
+
+remove_subdomain:
+	WARN_ON(pm_genpd_remove_subdomain(pg->parent, &pg->genpd));
+remove_domain_and_resets:
+	WARN_ON(pm_genpd_remove(&pg->genpd));
+	while (pg->num_resets--)
+		reset_control_put(pg->resets[pg->num_resets]);
+remove_clks:
+	while (pg->num_clks--)
+		clk_put(pg->clks[pg->num_clks]);
+error:
+	dev_err(pmc->dev, "failed to create power domain for node %s\n",
+		np->name);
+
+	return ERR_PTR(err);
+}
+
+static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np,
+			       struct generic_pm_domain *parent)
+{
+	struct tegra_powergate *pg;
+	struct device_node *child;
+	int err = 0;
+
+	for_each_child_of_node(np, child) {
+		pg = tegra_powergate_add_one(pmc, child, parent);
+		if (IS_ERR(pg)) {
+			err = PTR_ERR(pg);
+			goto err;
+		}
+
+		if (pg)
+			err = tegra_powergate_add(pmc, child, pg->parent);
+
+err:
+		of_node_put(child);
+
+		if (err)
+			break;
+	}
+
+	return err;
+}
+
+static void tegra_powergate_remove(struct tegra_pmc *pmc)
+{
+	struct tegra_powergate *pg, *n;
+
+	list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
+		of_genpd_del_provider(pg->of_node);
+
+		if (pg->parent) {
+			if (WARN_ON(pm_genpd_remove_subdomain(pg->parent,
+							      &pg->genpd)))
+				return;
+
+			pg->parent = NULL;
+		}
+
+		if (WARN_ON(pm_genpd_remove(&pg->genpd)))
+			return;
+
+		while (pg->num_clks--)
+			clk_put(pg->clks[pg->num_clks]);
+
+		while (pg->num_resets--)
+			reset_control_put(pg->resets[pg->num_resets]);
+
+		list_del(&pg->node);
+	}
+}
+
+static int tegra_powergate_init(struct tegra_pmc *pmc)
+{
+	struct device_node *np;
+	int err;
+
+	INIT_LIST_HEAD(&pmc->powergates_list);
+
+	np = of_get_child_by_name(pmc->dev->of_node, "powergates");
+	if (!np)
+		return 0;
+
+	err = tegra_powergate_add(pmc, np, NULL);
+	if (err)
+		tegra_powergate_remove(pmc);
+
+	of_node_put(np);
+
+	return err;
+}
+
 static int tegra_io_rail_prepare(unsigned int id, unsigned long *request,
 				 unsigned long *status, unsigned int *bit)
 {
@@ -880,24 +1285,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_init_tsense_reset(pmc);
 
+	err = tegra_powergate_init(pmc);
+	if (err < 0)
+		goto error;
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			goto error;
+			goto remove_powergates;
 	}
 
 	err = register_restart_handler(&tegra_pmc_restart_handler);
 	if (err) {
-		debugfs_remove(pmc->debugfs);
 		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
 			err);
-		goto error;
+		goto remove_debugfs;
 	}
 
 	iounmap(base);
 
 	return 0;
 
+remove_debugfs:
+	debugfs_remove(pmc->debugfs);
+remove_powergates:
+	tegra_powergate_remove(pmc);
 error:
 	mutex_lock(&pmc->powergates_lock);
 	pmc->base = base;
diff --git a/include/dt-bindings/power/tegra-powergate.h b/include/dt-bindings/power/tegra-powergate.h
new file mode 100644
index 000000000000..bcab501badfc
--- /dev/null
+++ b/include/dt-bindings/power/tegra-powergate.h
@@ -0,0 +1,36 @@
+#ifndef _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+#define _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+
+#define TEGRA_POWERGATE_CPU	0
+#define TEGRA_POWERGATE_3D	1
+#define TEGRA_POWERGATE_VENC	2
+#define TEGRA_POWERGATE_PCIE	3
+#define TEGRA_POWERGATE_VDEC	4
+#define TEGRA_POWERGATE_L2	5
+#define TEGRA_POWERGATE_MPE	6
+#define TEGRA_POWERGATE_HEG	7
+#define TEGRA_POWERGATE_SATA	8
+#define TEGRA_POWERGATE_CPU1	9
+#define TEGRA_POWERGATE_CPU2	10
+#define TEGRA_POWERGATE_CPU3	11
+#define TEGRA_POWERGATE_CELP	12
+#define TEGRA_POWERGATE_3D1	13
+#define TEGRA_POWERGATE_CPU0	14
+#define TEGRA_POWERGATE_C0NC	15
+#define TEGRA_POWERGATE_C1NC	16
+#define TEGRA_POWERGATE_SOR	17
+#define TEGRA_POWERGATE_DIS	18
+#define TEGRA_POWERGATE_DISB	19
+#define TEGRA_POWERGATE_XUSBA	20
+#define TEGRA_POWERGATE_XUSBB	21
+#define TEGRA_POWERGATE_XUSBC	22
+#define TEGRA_POWERGATE_VIC	23
+#define TEGRA_POWERGATE_IRAM	24
+#define TEGRA_POWERGATE_NVDEC	25
+#define TEGRA_POWERGATE_NVJPG	26
+#define TEGRA_POWERGATE_AUD	27
+#define TEGRA_POWERGATE_DFD	28
+#define TEGRA_POWERGATE_VE2	29
+#define TEGRA_POWERGATE_MAX	TEGRA_POWERGATE_VE2
+
+#endif
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index e9e53473a63e..c028557365ad 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -19,6 +19,8 @@
 #ifndef __SOC_TEGRA_PMC_H__
 #define __SOC_TEGRA_PMC_H__
 
+#include <dt-bindings/power/tegra-powergate.h>
+
 #include <linux/reboot.h>
 
 #include <soc/tegra/pm.h>
@@ -39,43 +41,8 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
 #endif /* CONFIG_SMP */
 
 /*
- * powergate and I/O rail APIs
+ * I/O rail APIs
  */
-
-#define TEGRA_POWERGATE_CPU	0
-#define TEGRA_POWERGATE_3D	1
-#define TEGRA_POWERGATE_VENC	2
-#define TEGRA_POWERGATE_PCIE	3
-#define TEGRA_POWERGATE_VDEC	4
-#define TEGRA_POWERGATE_L2	5
-#define TEGRA_POWERGATE_MPE	6
-#define TEGRA_POWERGATE_HEG	7
-#define TEGRA_POWERGATE_SATA	8
-#define TEGRA_POWERGATE_CPU1	9
-#define TEGRA_POWERGATE_CPU2	10
-#define TEGRA_POWERGATE_CPU3	11
-#define TEGRA_POWERGATE_CELP	12
-#define TEGRA_POWERGATE_3D1	13
-#define TEGRA_POWERGATE_CPU0	14
-#define TEGRA_POWERGATE_C0NC	15
-#define TEGRA_POWERGATE_C1NC	16
-#define TEGRA_POWERGATE_SOR	17
-#define TEGRA_POWERGATE_DIS	18
-#define TEGRA_POWERGATE_DISB	19
-#define TEGRA_POWERGATE_XUSBA	20
-#define TEGRA_POWERGATE_XUSBB	21
-#define TEGRA_POWERGATE_XUSBC	22
-#define TEGRA_POWERGATE_VIC	23
-#define TEGRA_POWERGATE_IRAM	24
-#define TEGRA_POWERGATE_NVDEC	25
-#define TEGRA_POWERGATE_NVJPG	26
-#define TEGRA_POWERGATE_AUD	27
-#define TEGRA_POWERGATE_DFD	28
-#define TEGRA_POWERGATE_VE2	29
-#define TEGRA_POWERGATE_MAX	TEGRA_POWERGATE_VE2
-
-#define TEGRA_POWERGATE_3D0	TEGRA_POWERGATE_3D
-
 #define TEGRA_IO_RAIL_CSIA	0
 #define TEGRA_IO_RAIL_CSIB	1
 #define TEGRA_IO_RAIL_DSI	2
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote:
> Adds generic PM support to the PMC driver where the PM domains are
> populated from device-tree and the PM domain consumer devices are
> bound to their relevant PM domains via device-tree as well.
>
> Update the tegra_powergate_sequence_power_up() API so that internally
> it calls the same tegra_powergate_xxx functions that are used by the
> tegra generic power domain code for consistency.
>
> This is based upon work by Thierry Reding <treding@nvidia.com>
> and Vince Hsu <vinceh@nvidia.com>.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c                     | 470 ++++++++++++++++++++++++++--
>  include/dt-bindings/power/tegra-powergate.h |  36 +++
>  include/soc/tegra/pmc.h                     |  39 +--

I suggest you split the header changes into a separate patch.

Moreover, these new DT definitions should be documented in the patch
describing the new powergate DT bindings. At least a simple list
providing the available options.

[...]

>
> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < pg->num_clks; i++)
> +               clk_disable_unprepare(pg->clks[i]);
> +}
> +
> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
> +{
> +       unsigned int i;
> +       int err;
> +
> +       for (i = 0; i < pg->num_clks; i++) {
> +               err = clk_prepare_enable(pg->clks[i]);
> +               if (err)
> +                       goto out;
> +       }
> +
> +       return 0;
> +
> +out:
> +       while (i--)
> +               clk_disable_unprepare(pg->clks[i]);
> +
> +       return err;
> +}

I have seen similar code around in other PM domains, dealing with
enabling/disabling a *list* of clocks.
Perhaps we should invent a new clock API that helps with this to
prevents code duplication!?

[...]

>  /**
>   * tegra_powergate_power_on() - power on partition
>   * @id: partition ID
> @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>  int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>                                       struct reset_control *rst)

There seems to be two viable ways for a driver to control tegra powergates.

1)
$Subject patch enables the use of runtime PM.

2)
The current tegra_powergate_sequence_power_up() and
tegra_powergate_power_off() API.

It seems fragile to allow both options, but perhaps your are
protecting this with some lock to prevent concurrent accesses?

Also, I assume you need the two options in a transition phase, before
you have deployed runtime PM for these drivers?

[...]

> +static int tegra_powergate_of_get_clks(struct device *dev,
> +                                      struct tegra_powergate *pg)
> +{
> +       struct clk *clk;
> +       unsigned int i;
> +       int err;
> +
> +       pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks",
> +                                                 "#clock-cells");
> +       if (pg->num_clks == 0)
> +               return -ENODEV;
> +
> +       pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL);
> +       if (!pg->clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pg->num_clks; i++) {
> +               pg->clks[i] = of_clk_get(pg->of_node, i);
> +               if (IS_ERR(pg->clks[i])) {
> +                       err = PTR_ERR(pg->clks[i]);
> +                       goto err;
> +               }
> +       }
> +
> +       return 0;
> +
> +err:
> +       while (i--)
> +               clk_put(pg->clks[i]);
> +
> +       pg->num_clks = 0;
> +
> +       return err;
> +}

Fetching clocks like above function does, seems to be a quite common case.

As I suggested to add an enable/disable API for a clock list, the
similar can be done for creating the clock list.

Just an idea...

[...]

> +
> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
> +{
> +       struct tegra_powergate *pg, *n;
> +
> +       list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {

The tegra powergate driver will hold a list of nvidia powergates
domains, and the generic PM domain will hold a list of all generic PM
domains.

Perhaps there's a way to allow the generic PM domain to control this
by itself. If we for example used the struct device corresponding to
the powergate driver, genpd could use it to distinguish between
various instances of genpd structs..!? Maybe it would simplify the way
to deal with removing domains?

> +               of_genpd_del_provider(pg->of_node);
> +
> +               if (pg->parent) {
> +                       if (WARN_ON(pm_genpd_remove_subdomain(pg->parent,
> +                                                             &pg->genpd)))
> +                               return;
> +
> +                       pg->parent = NULL;
> +               }
> +
> +               if (WARN_ON(pm_genpd_remove(&pg->genpd)))
> +                       return;
> +
> +               while (pg->num_clks--)
> +                       clk_put(pg->clks[pg->num_clks]);
> +
> +               while (pg->num_resets--)
> +                       reset_control_put(pg->resets[pg->num_resets]);
> +
> +               list_del(&pg->node);
> +       }
> +}
> +

[...]

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 04/02/16 15:44, Ulf Hansson wrote:
> On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Adds generic PM support to the PMC driver where the PM domains are
>> populated from device-tree and the PM domain consumer devices are
>> bound to their relevant PM domains via device-tree as well.
>>
>> Update the tegra_powergate_sequence_power_up() API so that internally
>> it calls the same tegra_powergate_xxx functions that are used by the
>> tegra generic power domain code for consistency.
>>
>> This is based upon work by Thierry Reding <treding@nvidia.com>
>> and Vince Hsu <vinceh@nvidia.com>.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c                     | 470 ++++++++++++++++++++++++++--
>>  include/dt-bindings/power/tegra-powergate.h |  36 +++
>>  include/soc/tegra/pmc.h                     |  39 +--
> 
> I suggest you split the header changes into a separate patch.
> 
> Moreover, these new DT definitions should be documented in the patch
> describing the new powergate DT bindings. At least a simple list
> providing the available options.

Ok.

> [...]
> 
>>
>> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
>> +{
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < pg->num_clks; i++)
>> +               clk_disable_unprepare(pg->clks[i]);
>> +}
>> +
>> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
>> +{
>> +       unsigned int i;
>> +       int err;
>> +
>> +       for (i = 0; i < pg->num_clks; i++) {
>> +               err = clk_prepare_enable(pg->clks[i]);
>> +               if (err)
>> +                       goto out;
>> +       }
>> +
>> +       return 0;
>> +
>> +out:
>> +       while (i--)
>> +               clk_disable_unprepare(pg->clks[i]);
>> +
>> +       return err;
>> +}
> 
> I have seen similar code around in other PM domains, dealing with
> enabling/disabling a *list* of clocks.
> Perhaps we should invent a new clock API that helps with this to
> prevents code duplication!?

Yes, I have been thinking about that as well. I will have a look at that.

> [...]
> 
>>  /**
>>   * tegra_powergate_power_on() - power on partition
>>   * @id: partition ID
>> @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>>  int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>>                                       struct reset_control *rst)
> 
> There seems to be two viable ways for a driver to control tegra powergates.
> 
> 1)
> $Subject patch enables the use of runtime PM.
> 
> 2)
> The current tegra_powergate_sequence_power_up() and
> tegra_powergate_power_off() API.
> 
> It seems fragile to allow both options, but perhaps your are
> protecting this with some lock to prevent concurrent accesses?

There is a lock protecting accesses to the PMC registers which
ultimately control the power domain. However, may be it would be better
to ensure that any power-domain registered with genpd cannot be
controlled by the legacy APIs. I have added a bitmap to mark valid
power-domains to ensure that only valid power domains can be controlled
by these legacy APIs. I could mark the power-domain invalid after
registering with genpd to ensure that it cannot be accessed by the
legacy APIs.

> Also, I assume you need the two options in a transition phase, before
> you have deployed runtime PM for these drivers?

Right and some of the legacy APIs are entrenched in some drivers. So to
keep the patch set manageable it seems best to get some support in place
then start migrating the drivers.

> [...]
> 
>> +static int tegra_powergate_of_get_clks(struct device *dev,
>> +                                      struct tegra_powergate *pg)
>> +{
>> +       struct clk *clk;
>> +       unsigned int i;
>> +       int err;
>> +
>> +       pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks",
>> +                                                 "#clock-cells");
>> +       if (pg->num_clks == 0)
>> +               return -ENODEV;
>> +
>> +       pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL);
>> +       if (!pg->clks)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < pg->num_clks; i++) {
>> +               pg->clks[i] = of_clk_get(pg->of_node, i);
>> +               if (IS_ERR(pg->clks[i])) {
>> +                       err = PTR_ERR(pg->clks[i]);
>> +                       goto err;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +
>> +err:
>> +       while (i--)
>> +               clk_put(pg->clks[i]);
>> +
>> +       pg->num_clks = 0;
>> +
>> +       return err;
>> +}
> 
> Fetching clocks like above function does, seems to be a quite common case.
> 
> As I suggested to add an enable/disable API for a clock list, the
> similar can be done for creating the clock list.
> 
> Just an idea...

Ok.

> [...]
> 
>> +
>> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
>> +{
>> +       struct tegra_powergate *pg, *n;
>> +
>> +       list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
> 
> The tegra powergate driver will hold a list of nvidia powergates
> domains, and the generic PM domain will hold a list of all generic PM
> domains.
> 
> Perhaps there's a way to allow the generic PM domain to control this
> by itself. If we for example used the struct device corresponding to
> the powergate driver, genpd could use it to distinguish between
> various instances of genpd structs..!? Maybe it would simplify the way
> to deal with removing domains?

Yes, that would be ideal. However, would have require changing
genpd_init()? I am not sure how genpd would be able to access the device
struct for the powergate driver because we don't provide this via any
API I am aware of? And I am guessing that you don't wish to expose the
gpd_list to the world either.

If there is an easy way, I am open to it, but looking at it today, I am
not sure I see a simple way in which we could add a new API to do this.
However, may be I am missing something!

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

[...]

>>
>>>  /**
>>>   * tegra_powergate_power_on() - power on partition
>>>   * @id: partition ID
>>> @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>>>  int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>>>                                       struct reset_control *rst)
>>
>> There seems to be two viable ways for a driver to control tegra powergates.
>>
>> 1)
>> $Subject patch enables the use of runtime PM.
>>
>> 2)
>> The current tegra_powergate_sequence_power_up() and
>> tegra_powergate_power_off() API.
>>
>> It seems fragile to allow both options, but perhaps your are
>> protecting this with some lock to prevent concurrent accesses?
>
> There is a lock protecting accesses to the PMC registers which
> ultimately control the power domain. However, may be it would be better
> to ensure that any power-domain registered with genpd cannot be
> controlled by the legacy APIs. I have added a bitmap to mark valid
> power-domains to ensure that only valid power domains can be controlled
> by these legacy APIs. I could mark the power-domain invalid after
> registering with genpd to ensure that it cannot be accessed by the
> legacy APIs.

That seems like a good way of making it more robust!

>
>> Also, I assume you need the two options in a transition phase, before
>> you have deployed runtime PM for these drivers?
>
> Right and some of the legacy APIs are entrenched in some drivers. So to
> keep the patch set manageable it seems best to get some support in place
> then start migrating the drivers.

Thanks for elaborating on this! I get and like the idea of moving forward!

[...]

>>> +
>>> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
>>> +{
>>> +       struct tegra_powergate *pg, *n;
>>> +
>>> +       list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
>>
>> The tegra powergate driver will hold a list of nvidia powergates
>> domains, and the generic PM domain will hold a list of all generic PM
>> domains.
>>
>> Perhaps there's a way to allow the generic PM domain to control this
>> by itself. If we for example used the struct device corresponding to
>> the powergate driver, genpd could use it to distinguish between
>> various instances of genpd structs..!? Maybe it would simplify the way
>> to deal with removing domains?
>
> Yes, that would be ideal. However, would have require changing
> genpd_init()? I am not sure how genpd would be able to access the device
> struct for the powergate driver because we don't provide this via any
> API I am aware of? And I am guessing that you don't wish to expose the
> gpd_list to the world either.
>
> If there is an easy way, I am open to it, but looking at it today, I am
> not sure I see a simple way in which we could add a new API to do this.
> However, may be I am missing something!

If we add a new __pm_genpd_init() API, that could require a struct
device to be provided. That API will thus invoke the existing
pm_genpd_init() but also deal with the extra things needed here.

I would also allow such an API to return an error code.

Correspondingly, pm_genpd_remove() could be required to be provided
with a struct device.

Existing users of pm_genpd_init() can then convert to
__pm_genpd_init() whenever suitable.

Of course, another option is just to add new member in the genpd
struct for the struct *device. The caller of pm_genpd_init() could
check it, but allow it to be NULL. Although, the pm_genpd_remove() API
would require that pointer to the struct device to be set...

What do you think?

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 10/02/16 18:25, Ulf Hansson wrote:

[snip]

>>> Perhaps there's a way to allow the generic PM domain to control this
>>> by itself. If we for example used the struct device corresponding to
>>> the powergate driver, genpd could use it to distinguish between
>>> various instances of genpd structs..!? Maybe it would simplify the way
>>> to deal with removing domains?
>>
>> Yes, that would be ideal. However, would have require changing
>> genpd_init()? I am not sure how genpd would be able to access the device
>> struct for the powergate driver because we don't provide this via any
>> API I am aware of? And I am guessing that you don't wish to expose the
>> gpd_list to the world either.
>>
>> If there is an easy way, I am open to it, but looking at it today, I am
>> not sure I see a simple way in which we could add a new API to do this.
>> However, may be I am missing something!
> 
> If we add a new __pm_genpd_init() API, that could require a struct
> device to be provided. That API will thus invoke the existing
> pm_genpd_init() but also deal with the extra things needed here.
> 
> I would also allow such an API to return an error code.
> 
> Correspondingly, pm_genpd_remove() could be required to be provided
> with a struct device.
> 
> Existing users of pm_genpd_init() can then convert to
> __pm_genpd_init() whenever suitable.
> 
> Of course, another option is just to add new member in the genpd
> struct for the struct *device. The caller of pm_genpd_init() could
> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
> would require that pointer to the struct device to be set...
> 
> What do you think?

Yes, sounds good. May be it is simpler just to add a new member and let
the platform genpd driver handle it.

I am wondering if in addition to pm_genpd_remove(), we then just have a
function called pm_genpd_remove_tail(), which allows you to pass the
struct device pointer and will remove the last pm-domain from the
gpd_list and return the genpd pointer if successful. Internally, it will
call pm_genpd_remove(). It seems to me that if there are nested
pm-domains, then we probably want to remove them starting from the tail
as opposed to the head.

How does that sound?

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 10/02/16 18:25, Ulf Hansson wrote:
>
> [snip]
>
>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>> by itself. If we for example used the struct device corresponding to
>>>> the powergate driver, genpd could use it to distinguish between
>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>> to deal with removing domains?
>>>
>>> Yes, that would be ideal. However, would have require changing
>>> genpd_init()? I am not sure how genpd would be able to access the device
>>> struct for the powergate driver because we don't provide this via any
>>> API I am aware of? And I am guessing that you don't wish to expose the
>>> gpd_list to the world either.
>>>
>>> If there is an easy way, I am open to it, but looking at it today, I am
>>> not sure I see a simple way in which we could add a new API to do this.
>>> However, may be I am missing something!
>>
>> If we add a new __pm_genpd_init() API, that could require a struct
>> device to be provided. That API will thus invoke the existing
>> pm_genpd_init() but also deal with the extra things needed here.
>>
>> I would also allow such an API to return an error code.
>>
>> Correspondingly, pm_genpd_remove() could be required to be provided
>> with a struct device.
>>
>> Existing users of pm_genpd_init() can then convert to
>> __pm_genpd_init() whenever suitable.
>>
>> Of course, another option is just to add new member in the genpd
>> struct for the struct *device. The caller of pm_genpd_init() could
>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>> would require that pointer to the struct device to be set...
>>
>> What do you think?
>
> Yes, sounds good. May be it is simpler just to add a new member and let
> the platform genpd driver handle it.
>
> I am wondering if in addition to pm_genpd_remove(), we then just have a
> function called pm_genpd_remove_tail(), which allows you to pass the
> struct device pointer and will remove the last pm-domain from the
> gpd_list and return the genpd pointer if successful. Internally, it will
> call pm_genpd_remove(). It seems to me that if there are nested
> pm-domains, then we probably want to remove them starting from the tail
> as opposed to the head.
>
> How does that sound?

Why not make pm_genpd_remove() to behave as you describe for
pm_genpd_remove_tail()?
That's probably the only sane way to remove genpds anyhow!?

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 11/02/16 09:57, Ulf Hansson wrote:
> On 11 February 2016 at 10:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On 10/02/16 18:25, Ulf Hansson wrote:
>>
>> [snip]
>>
>>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>>> by itself. If we for example used the struct device corresponding to
>>>>> the powergate driver, genpd could use it to distinguish between
>>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>>> to deal with removing domains?
>>>>
>>>> Yes, that would be ideal. However, would have require changing
>>>> genpd_init()? I am not sure how genpd would be able to access the device
>>>> struct for the powergate driver because we don't provide this via any
>>>> API I am aware of? And I am guessing that you don't wish to expose the
>>>> gpd_list to the world either.
>>>>
>>>> If there is an easy way, I am open to it, but looking at it today, I am
>>>> not sure I see a simple way in which we could add a new API to do this.
>>>> However, may be I am missing something!
>>>
>>> If we add a new __pm_genpd_init() API, that could require a struct
>>> device to be provided. That API will thus invoke the existing
>>> pm_genpd_init() but also deal with the extra things needed here.
>>>
>>> I would also allow such an API to return an error code.
>>>
>>> Correspondingly, pm_genpd_remove() could be required to be provided
>>> with a struct device.
>>>
>>> Existing users of pm_genpd_init() can then convert to
>>> __pm_genpd_init() whenever suitable.
>>>
>>> Of course, another option is just to add new member in the genpd
>>> struct for the struct *device. The caller of pm_genpd_init() could
>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>>> would require that pointer to the struct device to be set...
>>>
>>> What do you think?
>>
>> Yes, sounds good. May be it is simpler just to add a new member and let
>> the platform genpd driver handle it.
>>
>> I am wondering if in addition to pm_genpd_remove(), we then just have a
>> function called pm_genpd_remove_tail(), which allows you to pass the
>> struct device pointer and will remove the last pm-domain from the
>> gpd_list and return the genpd pointer if successful. Internally, it will
>> call pm_genpd_remove(). It seems to me that if there are nested
>> pm-domains, then we probably want to remove them starting from the tail
>> as opposed to the head.
>>
>> How does that sound?
> 
> Why not make pm_genpd_remove() to behave as you describe for
> pm_genpd_remove_tail()?
> That's probably the only sane way to remove genpds anyhow!?

Simply to offer flexibility. I could see that for some devices that have
no dependencies between pm-domains and have a static list of pm-domains,
they can simply call pm_genpd_remove() for a given pm-domain. However,
that said, I can envision a case where a single pm-domain would be
removed by itself and so may be there is no benefit?

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 11/02/16 10:13, Jon Hunter wrote:
> 
> On 11/02/16 09:57, Ulf Hansson wrote:
>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> On 10/02/16 18:25, Ulf Hansson wrote:
>>>
>>> [snip]
>>>
>>>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>>>> by itself. If we for example used the struct device corresponding to
>>>>>> the powergate driver, genpd could use it to distinguish between
>>>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>>>> to deal with removing domains?
>>>>>
>>>>> Yes, that would be ideal. However, would have require changing
>>>>> genpd_init()? I am not sure how genpd would be able to access the device
>>>>> struct for the powergate driver because we don't provide this via any
>>>>> API I am aware of? And I am guessing that you don't wish to expose the
>>>>> gpd_list to the world either.
>>>>>
>>>>> If there is an easy way, I am open to it, but looking at it today, I am
>>>>> not sure I see a simple way in which we could add a new API to do this.
>>>>> However, may be I am missing something!
>>>>
>>>> If we add a new __pm_genpd_init() API, that could require a struct
>>>> device to be provided. That API will thus invoke the existing
>>>> pm_genpd_init() but also deal with the extra things needed here.
>>>>
>>>> I would also allow such an API to return an error code.
>>>>
>>>> Correspondingly, pm_genpd_remove() could be required to be provided
>>>> with a struct device.
>>>>
>>>> Existing users of pm_genpd_init() can then convert to
>>>> __pm_genpd_init() whenever suitable.
>>>>
>>>> Of course, another option is just to add new member in the genpd
>>>> struct for the struct *device. The caller of pm_genpd_init() could
>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>>>> would require that pointer to the struct device to be set...
>>>>
>>>> What do you think?
>>>
>>> Yes, sounds good. May be it is simpler just to add a new member and let
>>> the platform genpd driver handle it.
>>>
>>> I am wondering if in addition to pm_genpd_remove(), we then just have a
>>> function called pm_genpd_remove_tail(), which allows you to pass the
>>> struct device pointer and will remove the last pm-domain from the
>>> gpd_list and return the genpd pointer if successful. Internally, it will
>>> call pm_genpd_remove(). It seems to me that if there are nested
>>> pm-domains, then we probably want to remove them starting from the tail
>>> as opposed to the head.
>>>
>>> How does that sound?
>>
>> Why not make pm_genpd_remove() to behave as you describe for
>> pm_genpd_remove_tail()?
>> That's probably the only sane way to remove genpds anyhow!?
> 
> Simply to offer flexibility. I could see that for some devices that have
> no dependencies between pm-domains and have a static list of pm-domains,
> they can simply call pm_genpd_remove() for a given pm-domain. However,
> that said, I can envision a case where a single pm-domain would be
> removed by itself and so may be there is no benefit?

By the way, do you think that instead of passing the struct device * to
pm_genpd_remove(), we should just have a void *dev_id in the same way
the request_irq()/free_irq() work? In other words, it would allow people
to use the struct device or struct device_node, etc?

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

[...]

>>>
>>> Why not make pm_genpd_remove() to behave as you describe for
>>> pm_genpd_remove_tail()?
>>> That's probably the only sane way to remove genpds anyhow!?
>>
>> Simply to offer flexibility. I could see that for some devices that have
>> no dependencies between pm-domains and have a static list of pm-domains,
>> they can simply call pm_genpd_remove() for a given pm-domain. However,
>> that said, I can envision a case where a single pm-domain would be
>> removed by itself and so may be there is no benefit?
>
> By the way, do you think that instead of passing the struct device * to
> pm_genpd_remove(), we should just have a void *dev_id in the same way
> the request_irq()/free_irq() work? In other words, it would allow people
> to use the struct device or struct device_node, etc?

Hmm. Do you think that would make a difference for the power controller drivers?

I am thinking that genpd might perhaps benefit from being able to use
the device pointer for other purposes as well!?
Giving a void *, will prevent that, won't it?

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 11/02/16 10:37, Ulf Hansson wrote:
> [...]
> 
>>>>
>>>> Why not make pm_genpd_remove() to behave as you describe for
>>>> pm_genpd_remove_tail()?
>>>> That's probably the only sane way to remove genpds anyhow!?
>>>
>>> Simply to offer flexibility. I could see that for some devices that have
>>> no dependencies between pm-domains and have a static list of pm-domains,
>>> they can simply call pm_genpd_remove() for a given pm-domain. However,
>>> that said, I can envision a case where a single pm-domain would be
>>> removed by itself and so may be there is no benefit?
>>
>> By the way, do you think that instead of passing the struct device * to
>> pm_genpd_remove(), we should just have a void *dev_id in the same way
>> the request_irq()/free_irq() work? In other words, it would allow people
>> to use the struct device or struct device_node, etc?
> 
> Hmm. Do you think that would make a difference for the power controller drivers?
> 
> I am thinking that genpd might perhaps benefit from being able to use
> the device pointer for other purposes as well!?
> Giving a void *, will prevent that, won't it?

Yes it will. Ok, let's stick with struct device for now.

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

On 11 February 2016 at 11:13, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> On 11/02/16 09:57, Ulf Hansson wrote:
>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>
>>> On 10/02/16 18:25, Ulf Hansson wrote:
>>>
>>> [snip]
>>>
>>>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>>>> by itself. If we for example used the struct device corresponding to
>>>>>> the powergate driver, genpd could use it to distinguish between
>>>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>>>> to deal with removing domains?
>>>>>
>>>>> Yes, that would be ideal. However, would have require changing
>>>>> genpd_init()? I am not sure how genpd would be able to access the device
>>>>> struct for the powergate driver because we don't provide this via any
>>>>> API I am aware of? And I am guessing that you don't wish to expose the
>>>>> gpd_list to the world either.
>>>>>
>>>>> If there is an easy way, I am open to it, but looking at it today, I am
>>>>> not sure I see a simple way in which we could add a new API to do this.
>>>>> However, may be I am missing something!
>>>>
>>>> If we add a new __pm_genpd_init() API, that could require a struct
>>>> device to be provided. That API will thus invoke the existing
>>>> pm_genpd_init() but also deal with the extra things needed here.
>>>>
>>>> I would also allow such an API to return an error code.
>>>>
>>>> Correspondingly, pm_genpd_remove() could be required to be provided
>>>> with a struct device.
>>>>
>>>> Existing users of pm_genpd_init() can then convert to
>>>> __pm_genpd_init() whenever suitable.
>>>>
>>>> Of course, another option is just to add new member in the genpd
>>>> struct for the struct *device. The caller of pm_genpd_init() could
>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>>>> would require that pointer to the struct device to be set...
>>>>
>>>> What do you think?
>>>
>>> Yes, sounds good. May be it is simpler just to add a new member and let
>>> the platform genpd driver handle it.
>>>
>>> I am wondering if in addition to pm_genpd_remove(), we then just have a
>>> function called pm_genpd_remove_tail(), which allows you to pass the
>>> struct device pointer and will remove the last pm-domain from the
>>> gpd_list and return the genpd pointer if successful. Internally, it will
>>> call pm_genpd_remove(). It seems to me that if there are nested
>>> pm-domains, then we probably want to remove them starting from the tail
>>> as opposed to the head.
>>>
>>> How does that sound?
>>
>> Why not make pm_genpd_remove() to behave as you describe for
>> pm_genpd_remove_tail()?
>> That's probably the only sane way to remove genpds anyhow!?
>
> Simply to offer flexibility. I could see that for some devices that have
> no dependencies between pm-domains and have a static list of pm-domains,
> they can simply call pm_genpd_remove() for a given pm-domain. However,
> that said, I can envision a case where a single pm-domain would be
> removed by itself and so may be there is no benefit?

If I understand correctly, you agree to try with the most simple
approach first and thus without providing too much flexibility.

Anyway, I am looking forward to review your next version of the patchset! :-)

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 11/02/16 10:28, Ulf Hansson wrote:
> On 11 February 2016 at 11:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On 11/02/16 09:57, Ulf Hansson wrote:
>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>> On 10/02/16 18:25, Ulf Hansson wrote:
>>>>
>>>> [snip]
>>>>
>>>>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>>>>> by itself. If we for example used the struct device corresponding to
>>>>>>> the powergate driver, genpd could use it to distinguish between
>>>>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>>>>> to deal with removing domains?
>>>>>>
>>>>>> Yes, that would be ideal. However, would have require changing
>>>>>> genpd_init()? I am not sure how genpd would be able to access the device
>>>>>> struct for the powergate driver because we don't provide this via any
>>>>>> API I am aware of? And I am guessing that you don't wish to expose the
>>>>>> gpd_list to the world either.
>>>>>>
>>>>>> If there is an easy way, I am open to it, but looking at it today, I am
>>>>>> not sure I see a simple way in which we could add a new API to do this.
>>>>>> However, may be I am missing something!
>>>>>
>>>>> If we add a new __pm_genpd_init() API, that could require a struct
>>>>> device to be provided. That API will thus invoke the existing
>>>>> pm_genpd_init() but also deal with the extra things needed here.
>>>>>
>>>>> I would also allow such an API to return an error code.
>>>>>
>>>>> Correspondingly, pm_genpd_remove() could be required to be provided
>>>>> with a struct device.
>>>>>
>>>>> Existing users of pm_genpd_init() can then convert to
>>>>> __pm_genpd_init() whenever suitable.
>>>>>
>>>>> Of course, another option is just to add new member in the genpd
>>>>> struct for the struct *device. The caller of pm_genpd_init() could
>>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>>>>> would require that pointer to the struct device to be set...
>>>>>
>>>>> What do you think?
>>>>
>>>> Yes, sounds good. May be it is simpler just to add a new member and let
>>>> the platform genpd driver handle it.
>>>>
>>>> I am wondering if in addition to pm_genpd_remove(), we then just have a
>>>> function called pm_genpd_remove_tail(), which allows you to pass the
>>>> struct device pointer and will remove the last pm-domain from the
>>>> gpd_list and return the genpd pointer if successful. Internally, it will
>>>> call pm_genpd_remove(). It seems to me that if there are nested
>>>> pm-domains, then we probably want to remove them starting from the tail
>>>> as opposed to the head.
>>>>
>>>> How does that sound?
>>>
>>> Why not make pm_genpd_remove() to behave as you describe for
>>> pm_genpd_remove_tail()?
>>> That's probably the only sane way to remove genpds anyhow!?
>>
>> Simply to offer flexibility. I could see that for some devices that have
>> no dependencies between pm-domains and have a static list of pm-domains,
>> they can simply call pm_genpd_remove() for a given pm-domain. However,
>> that said, I can envision a case where a single pm-domain would be
>> removed by itself and so may be there is no benefit?
> 
> If I understand correctly, you agree to try with the most simple
> approach first and thus without providing too much flexibility.
> 
> Anyway, I am looking forward to review your next version of the patchset! :-)

So one snag I hit with this, is that in order to remove a pm-domain, I
first need to check if it is a subdomain of any other domains and if so
remove it as a subdomain first. Before, this was simple because I called
pm_genpd_remove_subdomain if the domain had a parent, and then called
pm_genpd_remove().

With the proposal we have discussed, I no longer have any knowledge of
whether the pm-domain is a subdomain or not because pm_genpd_remove()
would remove the tail and then return it. So this means that I now need
to handle the removal of the subdomain within pm_genpd_remove(), which
is ok, but creates more changes as I need to parse the slave_links list,
etc.

I am wondering if it would be better to add a simple function called
pm_genpd_list_get_tail(struct device *dev) that would return the last
pm-domain register for a given device and then simply call
pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()?

Let me know what you think.

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

On 11 February 2016 at 17:38, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 11/02/16 10:28, Ulf Hansson wrote:
>> On 11 February 2016 at 11:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> On 11/02/16 09:57, Ulf Hansson wrote:
>>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>
>>>>> On 10/02/16 18:25, Ulf Hansson wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>>>> Perhaps there's a way to allow the generic PM domain to control this
>>>>>>>> by itself. If we for example used the struct device corresponding to
>>>>>>>> the powergate driver, genpd could use it to distinguish between
>>>>>>>> various instances of genpd structs..!? Maybe it would simplify the way
>>>>>>>> to deal with removing domains?
>>>>>>>
>>>>>>> Yes, that would be ideal. However, would have require changing
>>>>>>> genpd_init()? I am not sure how genpd would be able to access the device
>>>>>>> struct for the powergate driver because we don't provide this via any
>>>>>>> API I am aware of? And I am guessing that you don't wish to expose the
>>>>>>> gpd_list to the world either.
>>>>>>>
>>>>>>> If there is an easy way, I am open to it, but looking at it today, I am
>>>>>>> not sure I see a simple way in which we could add a new API to do this.
>>>>>>> However, may be I am missing something!
>>>>>>
>>>>>> If we add a new __pm_genpd_init() API, that could require a struct
>>>>>> device to be provided. That API will thus invoke the existing
>>>>>> pm_genpd_init() but also deal with the extra things needed here.
>>>>>>
>>>>>> I would also allow such an API to return an error code.
>>>>>>
>>>>>> Correspondingly, pm_genpd_remove() could be required to be provided
>>>>>> with a struct device.
>>>>>>
>>>>>> Existing users of pm_genpd_init() can then convert to
>>>>>> __pm_genpd_init() whenever suitable.
>>>>>>
>>>>>> Of course, another option is just to add new member in the genpd
>>>>>> struct for the struct *device. The caller of pm_genpd_init() could
>>>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API
>>>>>> would require that pointer to the struct device to be set...
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Yes, sounds good. May be it is simpler just to add a new member and let
>>>>> the platform genpd driver handle it.
>>>>>
>>>>> I am wondering if in addition to pm_genpd_remove(), we then just have a
>>>>> function called pm_genpd_remove_tail(), which allows you to pass the
>>>>> struct device pointer and will remove the last pm-domain from the
>>>>> gpd_list and return the genpd pointer if successful. Internally, it will
>>>>> call pm_genpd_remove(). It seems to me that if there are nested
>>>>> pm-domains, then we probably want to remove them starting from the tail
>>>>> as opposed to the head.
>>>>>
>>>>> How does that sound?
>>>>
>>>> Why not make pm_genpd_remove() to behave as you describe for
>>>> pm_genpd_remove_tail()?
>>>> That's probably the only sane way to remove genpds anyhow!?
>>>
>>> Simply to offer flexibility. I could see that for some devices that have
>>> no dependencies between pm-domains and have a static list of pm-domains,
>>> they can simply call pm_genpd_remove() for a given pm-domain. However,
>>> that said, I can envision a case where a single pm-domain would be
>>> removed by itself and so may be there is no benefit?
>>
>> If I understand correctly, you agree to try with the most simple
>> approach first and thus without providing too much flexibility.
>>
>> Anyway, I am looking forward to review your next version of the patchset! :-)
>
> So one snag I hit with this, is that in order to remove a pm-domain, I
> first need to check if it is a subdomain of any other domains and if so
> remove it as a subdomain first. Before, this was simple because I called
> pm_genpd_remove_subdomain if the domain had a parent, and then called
> pm_genpd_remove().

You certainly have a point here.

One more thing, we not only have to check whether the domain is a
subdomain. we also need to check if the domain is a parent (master)
for other subdomains. It being a parent, prevents us from removing it
until all its subdomains are removed.

>
> With the proposal we have discussed, I no longer have any knowledge of
> whether the pm-domain is a subdomain or not because pm_genpd_remove()
> would remove the tail and then return it. So this means that I now need
> to handle the removal of the subdomain within pm_genpd_remove(), which
> is ok, but creates more changes as I need to parse the slave_links list,
> etc.
>
> I am wondering if it would be better to add a simple function called
> pm_genpd_list_get_tail(struct device *dev) that would return the last
> pm-domain register for a given device and then simply call
> pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()?

That should work. Please go ahead and have try!

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Adds generic PM support to the PMC driver where the PM domains are
>> populated from device-tree and the PM domain consumer devices are
>> bound to their relevant PM domains via device-tree as well.
>>
>> Update the tegra_powergate_sequence_power_up() API so that internally
>> it calls the same tegra_powergate_xxx functions that are used by the
>> tegra generic power domain code for consistency.
>>
>> This is based upon work by Thierry Reding <treding@nvidia.com>
>> and Vince Hsu <vinceh@nvidia.com>.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

[...]

>> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
>> +{
>> +       unsigned int i;
>> +
>> +       for (i = 0; i < pg->num_clks; i++)
>> +               clk_disable_unprepare(pg->clks[i]);
>> +}
>> +
>> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
>> +{
>> +       unsigned int i;
>> +       int err;
>> +
>> +       for (i = 0; i < pg->num_clks; i++) {
>> +               err = clk_prepare_enable(pg->clks[i]);
>> +               if (err)
>> +                       goto out;
>> +       }
>> +
>> +       return 0;
>> +
>> +out:
>> +       while (i--)
>> +               clk_disable_unprepare(pg->clks[i]);
>> +
>> +       return err;
>> +}
>
> I have seen similar code around in other PM domains, dealing with
> enabling/disabling a *list* of clocks.
> Perhaps we should invent a new clock API that helps with this to
> prevents code duplication!?

What about the pm_clk_* API which was built for tracking clocks
associated with devices for runtime PM.

IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your
_enable_clocks() would become pm_clk_suspend() an dyour
_disable_clocks() would become pm_clk_resume().

I might not be following the mapping between PMC and PGs though so not
sure pg->pmc->dev is the right struct device, but you get the idea.

Kevin

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 12/02/16 23:14, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> 
>> On 28 January 2016 at 17:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> Adds generic PM support to the PMC driver where the PM domains are
>>> populated from device-tree and the PM domain consumer devices are
>>> bound to their relevant PM domains via device-tree as well.
>>>
>>> Update the tegra_powergate_sequence_power_up() API so that internally
>>> it calls the same tegra_powergate_xxx functions that are used by the
>>> tegra generic power domain code for consistency.
>>>
>>> This is based upon work by Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> and Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> [...]
> 
>>> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg)
>>> +{
>>> +       unsigned int i;
>>> +
>>> +       for (i = 0; i < pg->num_clks; i++)
>>> +               clk_disable_unprepare(pg->clks[i]);
>>> +}
>>> +
>>> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg)
>>> +{
>>> +       unsigned int i;
>>> +       int err;
>>> +
>>> +       for (i = 0; i < pg->num_clks; i++) {
>>> +               err = clk_prepare_enable(pg->clks[i]);
>>> +               if (err)
>>> +                       goto out;
>>> +       }
>>> +
>>> +       return 0;
>>> +
>>> +out:
>>> +       while (i--)
>>> +               clk_disable_unprepare(pg->clks[i]);
>>> +
>>> +       return err;
>>> +}
>>
>> I have seen similar code around in other PM domains, dealing with
>> enabling/disabling a *list* of clocks.
>> Perhaps we should invent a new clock API that helps with this to
>> prevents code duplication!?
> 
> What about the pm_clk_* API which was built for tracking clocks
> associated with devices for runtime PM.
> 
> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your
> _enable_clocks() would become pm_clk_suspend() an dyour
> _disable_clocks() would become pm_clk_resume().

Very interesting, I was not aware of this.

> I might not be following the mapping between PMC and PGs though so not
> sure pg->pmc->dev is the right struct device, but you get the idea.

Yes, so this will not work here as-is, because the pmc->dev is common to
all pm-domains (it is the device that creates all the pm-domains). So to
make this work, I would need to create a device for each pm-domain and
add the clocks to that.

I see that this works very well for normal drivers, but it does not feel
so natural for pm-domains where we don't have a device struct today. By
the way, the rockchip pm-domains implementation is very much in the same
boat as tegra, where there are multiple clocks per pm-domain and it is
handled by a simple list. So I am not sure if you think that we should
be turning all pm-domains registered by pm_genpd_init() into a device
and then we can make use of these pm_clk_XXXX() APIs?

I have implemented the generic clk APIs that Ulf and I discussed for
handling multiple clocks, but if we think that this is a better way,
then I will hold off for now.

Cheers
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Ulf Hansson]

[...]

>>
>> What about the pm_clk_* API which was built for tracking clocks
>> associated with devices for runtime PM.
>>
>> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your
>> _enable_clocks() would become pm_clk_suspend() an dyour
>> _disable_clocks() would become pm_clk_resume().
>
> Very interesting, I was not aware of this.
>
>> I might not be following the mapping between PMC and PGs though so not
>> sure pg->pmc->dev is the right struct device, but you get the idea.
>
> Yes, so this will not work here as-is, because the pmc->dev is common to
> all pm-domains (it is the device that creates all the pm-domains). So to
> make this work, I would need to create a device for each pm-domain and
> add the clocks to that.
>
> I see that this works very well for normal drivers, but it does not feel
> so natural for pm-domains where we don't have a device struct today. By
> the way, the rockchip pm-domains implementation is very much in the same
> boat as tegra, where there are multiple clocks per pm-domain and it is
> handled by a simple list. So I am not sure if you think that we should
> be turning all pm-domains registered by pm_genpd_init() into a device
> and then we can make use of these pm_clk_XXXX() APIs?
>
> I have implemented the generic clk APIs that Ulf and I discussed for
> handling multiple clocks, but if we think that this is a better way,
> then I will hold off for now.

I think Kevin has a point that we already have PM clocks to build upon.
Could we perhaps try to extend that API instead to suite this needs as well?

I do realize that it will make this patchset more complicated. As I
stated earlier, this was just an idea I had, so to be clear I won't
hold back an ack for this patchset, if you decide to deal with this in
separate "improvement" step.

Kind regards
Uffe

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Jon Hunter]

On 18/02/16 16:00, Ulf Hansson wrote:
> [...]
> 
>>>
>>> What about the pm_clk_* API which was built for tracking clocks
>>> associated with devices for runtime PM.
>>>
>>> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your
>>> _enable_clocks() would become pm_clk_suspend() an dyour
>>> _disable_clocks() would become pm_clk_resume().
>>
>> Very interesting, I was not aware of this.
>>
>>> I might not be following the mapping between PMC and PGs though so not
>>> sure pg->pmc->dev is the right struct device, but you get the idea.
>>
>> Yes, so this will not work here as-is, because the pmc->dev is common to
>> all pm-domains (it is the device that creates all the pm-domains). So to
>> make this work, I would need to create a device for each pm-domain and
>> add the clocks to that.
>>
>> I see that this works very well for normal drivers, but it does not feel
>> so natural for pm-domains where we don't have a device struct today. By
>> the way, the rockchip pm-domains implementation is very much in the same
>> boat as tegra, where there are multiple clocks per pm-domain and it is
>> handled by a simple list. So I am not sure if you think that we should
>> be turning all pm-domains registered by pm_genpd_init() into a device
>> and then we can make use of these pm_clk_XXXX() APIs?
>>
>> I have implemented the generic clk APIs that Ulf and I discussed for
>> handling multiple clocks, but if we think that this is a better way,
>> then I will hold off for now.
> 
> I think Kevin has a point that we already have PM clocks to build upon.
> Could we perhaps try to extend that API instead to suite this needs as well?

We certainly could and I am not against it, however, it means that we
need to create a device structure for each pm-domain. If you and Kevin
are ok with me adding this to pm_genpd_init(), then I can give it a try.

> I do realize that it will make this patchset more complicated. As I
> stated earlier, this was just an idea I had, so to be clear I won't
> hold back an ack for this patchset, if you decide to deal with this in
> separate "improvement" step.

Thanks
Jon

Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[Kevin Hilman]

Jon Hunter <jonathanh@nvidia.com> writes:

> On 18/02/16 16:00, Ulf Hansson wrote:
>> [...]
>> 
>>>>
>>>> What about the pm_clk_* API which was built for tracking clocks
>>>> associated with devices for runtime PM.
>>>>
>>>> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your
>>>> _enable_clocks() would become pm_clk_suspend() an dyour
>>>> _disable_clocks() would become pm_clk_resume().
>>>
>>> Very interesting, I was not aware of this.
>>>
>>>> I might not be following the mapping between PMC and PGs though so not
>>>> sure pg->pmc->dev is the right struct device, but you get the idea.
>>>
>>> Yes, so this will not work here as-is, because the pmc->dev is common to
>>> all pm-domains (it is the device that creates all the pm-domains). So to
>>> make this work, I would need to create a device for each pm-domain and
>>> add the clocks to that.
>>>
>>> I see that this works very well for normal drivers, but it does not feel
>>> so natural for pm-domains where we don't have a device struct today. By
>>> the way, the rockchip pm-domains implementation is very much in the same
>>> boat as tegra, where there are multiple clocks per pm-domain and it is
>>> handled by a simple list. So I am not sure if you think that we should
>>> be turning all pm-domains registered by pm_genpd_init() into a device
>>> and then we can make use of these pm_clk_XXXX() APIs?
>>>
>>> I have implemented the generic clk APIs that Ulf and I discussed for
>>> handling multiple clocks, but if we think that this is a better way,
>>> then I will hold off for now.
>> 
>> I think Kevin has a point that we already have PM clocks to build upon.
>> Could we perhaps try to extend that API instead to suite this needs as well?
>
> We certainly could and I am not against it, however, it means that we
> need to create a device structure for each pm-domain. If you and Kevin
> are ok with me adding this to pm_genpd_init(), then I can give it a try.

At this point, I'm thinking that the added complexity of a per-pm-domain
struct device isn't justified.  Managing simple lists of clocks in the
SoC specific PM domains is quite easy to review and maintain, IMO.

So I recommend just keeping it that way for now.  If it starts to get
unwieldy for tegra, rockchip and any others, we can revisit a common way
of doing it then.

Kevin

[PATCH V5 12/14] clk: tegra210: Add the APB2APE audio clock

[Jon Hunter]

The APB2APE clock for the audio subsystem is required for powering up the
audio power domain and accessing the various modules in this subsystem on
the tegra210 device. Add this clock for tegra210.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-id.h               | 1 +
 drivers/clk/tegra/clk-tegra-periph.c     | 1 +
 drivers/clk/tegra/clk-tegra210.c         | 1 +
 include/dt-bindings/clock/tegra210-car.h | 2 +-
 4 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-id.h b/drivers/clk/tegra/clk-id.h
index 19ce0738ee76..62ea38187b71 100644
--- a/drivers/clk/tegra/clk-id.h
+++ b/drivers/clk/tegra/clk-id.h
@@ -11,6 +11,7 @@ enum clk_id {
 	tegra_clk_afi,
 	tegra_clk_amx,
 	tegra_clk_amx1,
+	tegra_clk_apb2ape,
 	tegra_clk_apbdma,
 	tegra_clk_apbif,
 	tegra_clk_ape,
diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
index 1860df1862dd..ea2b9cbf9e70 100644
--- a/drivers/clk/tegra/clk-tegra-periph.c
+++ b/drivers/clk/tegra/clk-tegra-periph.c
@@ -829,6 +829,7 @@ static struct tegra_periph_init_data gate_clks[] = {
 	GATE("xusb_gate", "osc", 143, 0, tegra_clk_xusb_gate, 0),
 	GATE("pll_p_out_cpu", "pll_p", 223, 0, tegra_clk_pll_p_out_cpu, 0),
 	GATE("pll_p_out_adsp", "pll_p", 187, 0, tegra_clk_pll_p_out_adsp, 0),
+	GATE("apb2ape", "clk_m", 107, 0, tegra_clk_apb2ape, 0),
 };
 
 static struct tegra_periph_init_data div_clks[] = {
diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
index 5d8fac7052f2..ff9741128d53 100644
--- a/drivers/clk/tegra/clk-tegra210.c
+++ b/drivers/clk/tegra/clk-tegra210.c
@@ -2204,6 +2204,7 @@ static struct tegra_clk tegra210_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_pll_c4_out1] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT1, .present = true },
 	[tegra_clk_pll_c4_out2] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT2, .present = true },
 	[tegra_clk_pll_c4_out3] = { .dt_id = TEGRA210_CLK_PLL_C4_OUT3, .present = true },
+	[tegra_clk_apb2ape] = { .dt_id = TEGRA210_CLK_APB2APE, .present = true },
 };
 
 static struct tegra_devclk devclks[] __initdata = {
diff --git a/include/dt-bindings/clock/tegra210-car.h b/include/dt-bindings/clock/tegra210-car.h
index 6f45aea49e4f..0a05b0d36ae7 100644
--- a/include/dt-bindings/clock/tegra210-car.h
+++ b/include/dt-bindings/clock/tegra210-car.h
@@ -126,7 +126,7 @@
 /* 104 */
 /* 105 */
 #define TEGRA210_CLK_D_AUDIO 106
-/* 107 ( affects abp -> ape) */
+#define TEGRA210_CLK_APB2APE 107
 /* 108 */
 /* 109 */
 /* 110 */
-- 
2.1.4

Re: [PATCH V5 12/14] clk: tegra210: Add the APB2APE audio clock

[Thierry Reding]

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

On Thu, Jan 28, 2016 at 04:33:50PM +0000, Jon Hunter wrote:
> The APB2APE clock for the audio subsystem is required for powering up the
> audio power domain and accessing the various modules in this subsystem on
> the tegra210 device. Add this clock for tegra210.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/clk/tegra/clk-id.h               | 1 +
>  drivers/clk/tegra/clk-tegra-periph.c     | 1 +
>  drivers/clk/tegra/clk-tegra210.c         | 1 +
>  include/dt-bindings/clock/tegra210-car.h | 2 +-
>  4 files changed, 4 insertions(+), 1 deletion(-)

Applied, thanks.

Thierry

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

[PATCH V5 13/14] ARM64: tegra: Add audio PM domain device node for Tegra210

[Jon Hunter]

Add the audio power-domain for tegra210. Note that this also removes the
existing "#power-domain-cells" which was incorrectly included by
commit e53095857166 ("arm64: tegra: Add Tegra210 support").

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

---

So far I have only added the audio power-domain for tegra210 as this is
what I have been testing with to date. However, once this series is
accepted then we can begin to add more.
---
 arch/arm64/boot/dts/nvidia/tegra210.dtsi | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
index bc23f4dea002..cf78082f2a3e 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
@@ -2,6 +2,7 @@
 #include <dt-bindings/gpio/tegra-gpio.h>
 #include <dt-bindings/memory/tegra210-mc.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
+#include <dt-bindings/power/tegra-powergate.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
@@ -581,7 +582,15 @@
 		clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
 		clock-names = "pclk", "clk32k_in";
 
-		#power-domain-cells = <1>;
+		powergates {
+			pd_audio: aud {
+				clocks = <&tegra_car TEGRA210_CLK_APE>,
+					 <&tegra_car TEGRA210_CLK_APB2APE>;
+				resets = <&tegra_car 198>;
+				nvidia,powergate = <TEGRA_POWERGATE_AUD>;
+				#power-domain-cells = <0>;
+			};
+		};
 	};
 
 	fuse@0,7000f800 {
-- 
2.1.4

[PATCH V5 02/14] soc: tegra: pmc: Protect public functions from potential race conditions

[Jon Hunter]

The PMC base address pointer is initialised during early boot so that
early platform code may used the PMC public functions. During the probe
of the PMC driver the base address pointer is mapped again and the initial
mapping is freed. This exposes a window where a device accessing the PMC
registers via one of the public functions, could race with the updating
of the pointer and lead to a invalid access. Furthermore, the only
protection between multiple devices attempting to access the PMC registers
is when setting the powergate state to on or off. None of the other public
functions that access the PMC registers are protected.

Use the existing mutex to protect paths that may race with regard to
accessing the PMC registers.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 85b4e166273a..f8cdb7ce9755 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
+	mutex_lock(&pmc->powergates_lock);
 	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	mutex_unlock(&pmc->powergates_lock);
+
 	return !!status;
 }
 
@@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	/*
 	 * On Tegra124 and later, the clamps for the GPU are controlled by a
 	 * separate register (with different semantics).
@@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id)
 	if (id == TEGRA_POWERGATE_3D) {
 		if (pmc->soc->has_gpu_clamps) {
 			tegra_pmc_writel(0, GPU_RG_CNTRL);
-			return 0;
+			goto out;
 		}
 	}
 
@@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id)
 
 	tegra_pmc_writel(mask, REMOVE_CLAMPING);
 
+out:
+	mutex_unlock(&pmc->powergates_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL(tegra_powergate_remove_clamping);
@@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id)
 	unsigned int bit, mask;
 	int err;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	err = tegra_io_rail_prepare(id, &request, &status, &bit);
 	if (err < 0)
-		return err;
+		goto error;
 
 	mask = 1 << bit;
 
@@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id)
 	err = tegra_io_rail_poll(status, mask, 0, 250);
 	if (err < 0) {
 		pr_info("tegra_io_rail_poll() failed: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	tegra_io_rail_unprepare();
 
-	return 0;
+error:
+	mutex_unlock(&pmc->powergates_lock);
+
+	return err < 0 ? err : 0;
 }
 EXPORT_SYMBOL(tegra_io_rail_power_on);
 
@@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id)
 	unsigned int bit, mask;
 	int err;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	err = tegra_io_rail_prepare(id, &request, &status, &bit);
 	if (err < 0) {
 		pr_info("tegra_io_rail_prepare() failed: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	mask = 1 << bit;
@@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id)
 
 	err = tegra_io_rail_poll(status, mask, mask, 250);
 	if (err < 0)
-		return err;
+		goto error;
 
 	tegra_io_rail_unprepare();
 
-	return 0;
+error:
+	mutex_unlock(&pmc->powergates_lock);
+
+	return err < 0 ? err : 0;
 }
 EXPORT_SYMBOL(tegra_io_rail_power_off);
 
@@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	/* take over the memory region from the early initialization */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	mutex_lock(&pmc->powergates_lock);
 	pmc->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
+	mutex_unlock(&pmc->powergates_lock);
+
+	if (IS_ERR(pmc->base)) {
+		err = PTR_ERR(pmc->base);
+		goto error;
+	}
 
 	pmc->clk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(pmc->clk)) {
@@ -853,7 +877,9 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	return 0;
 
 error:
+	mutex_lock(&pmc->powergates_lock);
 	pmc->base = base;
+	mutex_unlock(&pmc->powergates_lock);
 
 	return err;
 }
-- 
2.1.4

Re: [PATCH V5 02/14] soc: tegra: pmc: Protect public functions from potential race conditions

[Mathieu Poirier]

On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> wrote:
> The PMC base address pointer is initialised during early boot so that
> early platform code may used the PMC public functions. During the probe
> of the PMC driver the base address pointer is mapped again and the initial
> mapping is freed. This exposes a window where a device accessing the PMC
> registers via one of the public functions, could race with the updating
> of the pointer and lead to a invalid access. Furthermore, the only
> protection between multiple devices attempting to access the PMC registers
> is when setting the powergate state to on or off. None of the other public
> functions that access the PMC registers are protected.
>
> Use the existing mutex to protect paths that may race with regard to
> accessing the PMC registers.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 85b4e166273a..f8cdb7ce9755 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id)
>         if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>                 return -EINVAL;
>
> +       mutex_lock(&pmc->powergates_lock);
>         status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
> +       mutex_unlock(&pmc->powergates_lock);
> +
>         return !!status;
>  }
>
> @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id)
>         if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>                 return -EINVAL;
>
> +       mutex_lock(&pmc->powergates_lock);
> +
>         /*
>          * On Tegra124 and later, the clamps for the GPU are controlled by a
>          * separate register (with different semantics).
> @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id)
>         if (id == TEGRA_POWERGATE_3D) {
>                 if (pmc->soc->has_gpu_clamps) {
>                         tegra_pmc_writel(0, GPU_RG_CNTRL);
> -                       return 0;
> +                       goto out;
>                 }
>         }
>
> @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id)
>
>         tegra_pmc_writel(mask, REMOVE_CLAMPING);
>
> +out:
> +       mutex_unlock(&pmc->powergates_lock);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL(tegra_powergate_remove_clamping);
> @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id)
>         unsigned int bit, mask;
>         int err;
>
> +       mutex_lock(&pmc->powergates_lock);
> +
>         err = tegra_io_rail_prepare(id, &request, &status, &bit);
>         if (err < 0)
> -               return err;
> +               goto error;
>
>         mask = 1 << bit;
>
> @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id)
>         err = tegra_io_rail_poll(status, mask, 0, 250);
>         if (err < 0) {
>                 pr_info("tegra_io_rail_poll() failed: %d\n", err);
> -               return err;
> +               goto error;
>         }
>
>         tegra_io_rail_unprepare();
>
> -       return 0;
> +error:
> +       mutex_unlock(&pmc->powergates_lock);
> +
> +       return err < 0 ? err : 0;

Is this necessary?  Why simply not returning 'err'?  From what I see
'tegra_io_rail_power_on()' can only return a negative value or '0'.

>  }
>  EXPORT_SYMBOL(tegra_io_rail_power_on);
>
> @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id)
>         unsigned int bit, mask;
>         int err;
>
> +       mutex_lock(&pmc->powergates_lock);
> +
>         err = tegra_io_rail_prepare(id, &request, &status, &bit);
>         if (err < 0) {
>                 pr_info("tegra_io_rail_prepare() failed: %d\n", err);
> -               return err;
> +               goto error;
>         }
>
>         mask = 1 << bit;
> @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id)
>
>         err = tegra_io_rail_poll(status, mask, mask, 250);
>         if (err < 0)
> -               return err;
> +               goto error;
>
>         tegra_io_rail_unprepare();
>
> -       return 0;
> +error:
> +       mutex_unlock(&pmc->powergates_lock);
> +
> +       return err < 0 ? err : 0;

Same comment as above.

>  }
>  EXPORT_SYMBOL(tegra_io_rail_power_off);
>
> @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>
>         /* take over the memory region from the early initialization */
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       mutex_lock(&pmc->powergates_lock);
>         pmc->base = devm_ioremap_resource(&pdev->dev, res);
> -       if (IS_ERR(pmc->base))
> -               return PTR_ERR(pmc->base);
> +       mutex_unlock(&pmc->powergates_lock);

Since the mutex is released there is a window of opportunity for
devices to access an erroneous pointer.  A better approach might be to
use a temporary variable, do all the initialisation that is required
and when things look good set pmc-base to that temporary variable.

Mathieu

> +
> +       if (IS_ERR(pmc->base)) {
> +               err = PTR_ERR(pmc->base);
> +               goto error;
> +       }
>
>         pmc->clk = devm_clk_get(&pdev->dev, "pclk");
>         if (IS_ERR(pmc->clk)) {
> @@ -853,7 +877,9 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>         return 0;
>
>  error:
> +       mutex_lock(&pmc->powergates_lock);
>         pmc->base = base;
> +       mutex_unlock(&pmc->powergates_lock);
>
>         return err;
>  }
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 02/14] soc: tegra: pmc: Protect public functions from potential race conditions

[Jon Hunter]

On 29/01/16 16:20, Mathieu Poirier wrote:
> On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> wrote:
>> The PMC base address pointer is initialised during early boot so that
>> early platform code may used the PMC public functions. During the probe
>> of the PMC driver the base address pointer is mapped again and the initial
>> mapping is freed. This exposes a window where a device accessing the PMC
>> registers via one of the public functions, could race with the updating
>> of the pointer and lead to a invalid access. Furthermore, the only
>> protection between multiple devices attempting to access the PMC registers
>> is when setting the powergate state to on or off. None of the other public
>> functions that access the PMC registers are protected.
>>
>> Use the existing mutex to protect paths that may race with regard to
>> accessing the PMC registers.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 85b4e166273a..f8cdb7ce9755 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id)
>>         if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>>                 return -EINVAL;
>>
>> +       mutex_lock(&pmc->powergates_lock);
>>         status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>>         return !!status;
>>  }
>>
>> @@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id)
>>         if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>>                 return -EINVAL;
>>
>> +       mutex_lock(&pmc->powergates_lock);
>> +
>>         /*
>>          * On Tegra124 and later, the clamps for the GPU are controlled by a
>>          * separate register (with different semantics).
>> @@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id)
>>         if (id == TEGRA_POWERGATE_3D) {
>>                 if (pmc->soc->has_gpu_clamps) {
>>                         tegra_pmc_writel(0, GPU_RG_CNTRL);
>> -                       return 0;
>> +                       goto out;
>>                 }
>>         }
>>
>> @@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id)
>>
>>         tegra_pmc_writel(mask, REMOVE_CLAMPING);
>>
>> +out:
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>>         return 0;
>>  }
>>  EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>> @@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id)
>>         unsigned int bit, mask;
>>         int err;
>>
>> +       mutex_lock(&pmc->powergates_lock);
>> +
>>         err = tegra_io_rail_prepare(id, &request, &status, &bit);
>>         if (err < 0)
>> -               return err;
>> +               goto error;
>>
>>         mask = 1 << bit;
>>
>> @@ -535,12 +545,15 @@ int tegra_io_rail_power_on(int id)
>>         err = tegra_io_rail_poll(status, mask, 0, 250);
>>         if (err < 0) {
>>                 pr_info("tegra_io_rail_poll() failed: %d\n", err);
>> -               return err;
>> +               goto error;
>>         }
>>
>>         tegra_io_rail_unprepare();
>>
>> -       return 0;
>> +error:
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>> +       return err < 0 ? err : 0;
> 
> Is this necessary?  Why simply not returning 'err'?  From what I see
> 'tegra_io_rail_power_on()' can only return a negative value or '0'.

Right, this is probably not necessary and so I could simplify this.

>>  }
>>  EXPORT_SYMBOL(tegra_io_rail_power_on);
>>
>> @@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id)
>>         unsigned int bit, mask;
>>         int err;
>>
>> +       mutex_lock(&pmc->powergates_lock);
>> +
>>         err = tegra_io_rail_prepare(id, &request, &status, &bit);
>>         if (err < 0) {
>>                 pr_info("tegra_io_rail_prepare() failed: %d\n", err);
>> -               return err;
>> +               goto error;
>>         }
>>
>>         mask = 1 << bit;
>> @@ -566,11 +581,14 @@ int tegra_io_rail_power_off(int id)
>>
>>         err = tegra_io_rail_poll(status, mask, mask, 250);
>>         if (err < 0)
>> -               return err;
>> +               goto error;
>>
>>         tegra_io_rail_unprepare();
>>
>> -       return 0;
>> +error:
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>> +       return err < 0 ? err : 0;
> 
> Same comment as above.
> 
>>  }
>>  EXPORT_SYMBOL(tegra_io_rail_power_off);
>>
>> @@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>>
>>         /* take over the memory region from the early initialization */
>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +       mutex_lock(&pmc->powergates_lock);
>>         pmc->base = devm_ioremap_resource(&pdev->dev, res);
>> -       if (IS_ERR(pmc->base))
>> -               return PTR_ERR(pmc->base);
>> +       mutex_unlock(&pmc->powergates_lock);
> 
> Since the mutex is released there is a window of opportunity for
> devices to access an erroneous pointer.  A better approach might be to
> use a temporary variable, do all the initialisation that is required
> and when things look good set pmc-base to that temporary variable.

Thanks. Not sure what I was thinking here. I will fix that.

Cheers
Jon

[PATCH V5 03/14] soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type

[Jon Hunter]

The tegra powergate and rail IDs are always positive values and so change
the type to be unsigned and remove the tests to see if the ID is less
than zero. Update the Tegra DC powergate type to be an unsigned as well.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.h |  2 +-
 drivers/soc/tegra/pmc.c     | 36 ++++++++++++++++++------------------
 include/soc/tegra/pmc.h     | 35 ++++++++++++++++++-----------------
 3 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index c088f2f67eda..6431fe2397c1 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -121,7 +121,7 @@ struct tegra_dc {
 	spinlock_t lock;
 
 	struct drm_crtc base;
-	int powergate;
+	unsigned int powergate;
 	int pipe;
 
 	struct clk *clk;
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index f8cdb7ce9755..1dbdcf06a9ad 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -179,7 +179,7 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  * @id: partition ID
  * @new_state: new state of the partition
  */
-static int tegra_powergate_set(int id, bool new_state)
+static int tegra_powergate_set(unsigned int id, bool new_state)
 {
 	bool status;
 
@@ -203,9 +203,9 @@ static int tegra_powergate_set(int id, bool new_state)
  * tegra_powergate_power_on() - power on partition
  * @id: partition ID
  */
-int tegra_powergate_power_on(int id)
+int tegra_powergate_power_on(unsigned int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!pmc->soc || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	return tegra_powergate_set(id, true);
@@ -215,9 +215,9 @@ int tegra_powergate_power_on(int id)
  * tegra_powergate_power_off() - power off partition
  * @id: partition ID
  */
-int tegra_powergate_power_off(int id)
+int tegra_powergate_power_off(unsigned int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!pmc->soc || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);
@@ -228,11 +228,11 @@ EXPORT_SYMBOL(tegra_powergate_power_off);
  * tegra_powergate_is_powered() - check if partition is powered
  * @id: partition ID
  */
-int tegra_powergate_is_powered(int id)
+int tegra_powergate_is_powered(unsigned int id)
 {
 	u32 status;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!pmc->soc || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
@@ -246,11 +246,11 @@ int tegra_powergate_is_powered(int id)
  * tegra_powergate_remove_clamping() - remove power clamps for partition
  * @id: partition ID
  */
-int tegra_powergate_remove_clamping(int id)
+int tegra_powergate_remove_clamping(unsigned int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!pmc->soc || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
@@ -294,7 +294,7 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
  *
  * Must be called with clk disabled, and returns with clk enabled.
  */
-int tegra_powergate_sequence_power_up(int id, struct clk *clk,
+int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 				      struct reset_control *rst)
 {
 	int ret;
@@ -337,9 +337,9 @@ EXPORT_SYMBOL(tegra_powergate_sequence_power_up);
  * Returns the partition ID corresponding to the CPU partition ID or a
  * negative error code on failure.
  */
-static int tegra_get_cpu_powergate_id(int cpuid)
+static int tegra_get_cpu_powergate_id(unsigned int cpuid)
 {
-	if (pmc->soc && cpuid > 0 && cpuid < pmc->soc->num_cpu_powergates)
+	if (pmc->soc && cpuid < pmc->soc->num_cpu_powergates)
 		return pmc->soc->cpu_powergates[cpuid];
 
 	return -EINVAL;
@@ -349,7 +349,7 @@ static int tegra_get_cpu_powergate_id(int cpuid)
  * tegra_pmc_cpu_is_powered() - check if CPU partition is powered
  * @cpuid: CPU partition ID
  */
-bool tegra_pmc_cpu_is_powered(int cpuid)
+bool tegra_pmc_cpu_is_powered(unsigned int cpuid)
 {
 	int id;
 
@@ -364,7 +364,7 @@ bool tegra_pmc_cpu_is_powered(int cpuid)
  * tegra_pmc_cpu_power_on() - power on CPU partition
  * @cpuid: CPU partition ID
  */
-int tegra_pmc_cpu_power_on(int cpuid)
+int tegra_pmc_cpu_power_on(unsigned int cpuid)
 {
 	int id;
 
@@ -379,7 +379,7 @@ int tegra_pmc_cpu_power_on(int cpuid)
  * tegra_pmc_cpu_remove_clamping() - remove power clamps for CPU partition
  * @cpuid: CPU partition ID
  */
-int tegra_pmc_cpu_remove_clamping(int cpuid)
+int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
 {
 	int id;
 
@@ -465,7 +465,7 @@ static int tegra_powergate_debugfs_init(void)
 	return 0;
 }
 
-static int tegra_io_rail_prepare(int id, unsigned long *request,
+static int tegra_io_rail_prepare(unsigned int id, unsigned long *request,
 				 unsigned long *status, unsigned int *bit)
 {
 	unsigned long rate, value;
@@ -522,7 +522,7 @@ static void tegra_io_rail_unprepare(void)
 	tegra_pmc_writel(DPD_SAMPLE_DISABLE, DPD_SAMPLE);
 }
 
-int tegra_io_rail_power_on(int id)
+int tegra_io_rail_power_on(unsigned int id)
 {
 	unsigned long request, status, value;
 	unsigned int bit, mask;
@@ -557,7 +557,7 @@ error:
 }
 EXPORT_SYMBOL(tegra_io_rail_power_on);
 
-int tegra_io_rail_power_off(int id)
+int tegra_io_rail_power_off(unsigned int id)
 {
 	unsigned long request, status, value;
 	unsigned int bit, mask;
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index d18efe402ff1..07e332dd44fb 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -33,9 +33,9 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode);
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_SMP
-bool tegra_pmc_cpu_is_powered(int cpuid);
-int tegra_pmc_cpu_power_on(int cpuid);
-int tegra_pmc_cpu_remove_clamping(int cpuid);
+bool tegra_pmc_cpu_is_powered(unsigned int cpuid);
+int tegra_pmc_cpu_power_on(unsigned int cpuid);
+int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
 #endif /* CONFIG_SMP */
 
 /*
@@ -108,50 +108,51 @@ int tegra_pmc_cpu_remove_clamping(int cpuid);
 #define TEGRA_IO_RAIL_SYS_DDC	58
 
 #ifdef CONFIG_ARCH_TEGRA
-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);
+int tegra_powergate_is_powered(unsigned int id);
+int tegra_powergate_power_on(unsigned int id);
+int tegra_powergate_power_off(unsigned int id);
+int tegra_powergate_remove_clamping(unsigned int id);
 
 /* Must be called with clk disabled, and returns with clk enabled */
-int tegra_powergate_sequence_power_up(int id, struct clk *clk,
+int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 				      struct reset_control *rst);
 
-int tegra_io_rail_power_on(int id);
-int tegra_io_rail_power_off(int id);
+int tegra_io_rail_power_on(unsigned int id);
+int tegra_io_rail_power_off(unsigned int id);
 #else
-static inline int tegra_powergate_is_powered(int id)
+static inline int tegra_powergate_is_powered(unsigned int id)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_powergate_power_on(int id)
+static inline int tegra_powergate_power_on(unsigned int id)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_powergate_power_off(int id)
+static inline int tegra_powergate_power_off(unsigned int id)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_powergate_remove_clamping(int id)
+static inline int tegra_powergate_remove_clamping(unsigned int id)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_powergate_sequence_power_up(int id, struct clk *clk,
+static inline int tegra_powergate_sequence_power_up(unsigned int id,
+						    struct clk *clk,
 						    struct reset_control *rst)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_io_rail_power_on(int id)
+static inline int tegra_io_rail_power_on(unsigned int id)
 {
 	return -ENOSYS;
 }
 
-static inline int tegra_io_rail_power_off(int id)
+static inline int tegra_io_rail_power_off(unsigned int id)
 {
 	return -ENOSYS;
 }
-- 
2.1.4

[PATCH V5 04/14] soc: tegra: pmc: Fix testing of powergate state

[Jon Hunter]

In tegra_powergate_set() the state of the powergates is read and OR'ed
with the bit for the powergate of interest. This unsigned 32-bit value
is then compared with a boolean value to test if the powergate is
already in the desired state. When turning on a powergate, apart from
the powergate that is represented by bit 0, this test will always
return false and so we may attempt to turn on the powergate when it is
already on.

After OR'ing the bit for the powergate, check if the result is not equal
to zero before comparing with the boolean value. Add a helper function
to return the current state of a powergate.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 1dbdcf06a9ad..99cb2fdd29e1 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -174,6 +174,11 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
 	writel(value, pmc->base + offset);
 }
 
+static inline bool tegra_powergate_state(int id)
+{
+	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
+}
+
 /**
  * tegra_powergate_set() - set the state of a partition
  * @id: partition ID
@@ -181,13 +186,9 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  */
 static int tegra_powergate_set(unsigned int id, bool new_state)
 {
-	bool status;
-
 	mutex_lock(&pmc->powergates_lock);
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
-
-	if (status == new_state) {
+	if (tegra_powergate_state(id) == new_state) {
 		mutex_unlock(&pmc->powergates_lock);
 		return 0;
 	}
@@ -230,16 +231,16 @@ EXPORT_SYMBOL(tegra_powergate_power_off);
  */
 int tegra_powergate_is_powered(unsigned int id)
 {
-	u32 status;
+	int status;
 
 	if (!pmc->soc || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	status = tegra_powergate_state(id);
 	mutex_unlock(&pmc->powergates_lock);
 
-	return !!status;
+	return status;
 }
 
 /**
-- 
2.1.4

[PATCH V5 07/14] soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC

[Jon Hunter]

For Tegra124 and Tegra210, the GPU partition cannot be toggled on and off
via the APBDEV_PMC_PWRGATE_TOGGLE_0 register. For these devices, the
partition is simply powered up and down via an external regulator.
Describe in the PMC SoC data in which devices the GPU partition can be
controlled via the APBDEV_PMC_PWRGATE_TOGGLE_0 register and ensure that
no one can incorrectly try to toggle the GPU partition via the
APBDEV_PMC_PWRGATE_TOGGLE_0 register.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 032dd5c17130..ecb4f66819fd 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -110,6 +110,7 @@ struct tegra_pmc_soc {
 
 	bool has_tsense_reset;
 	bool has_gpu_clamps;
+	bool has_gpu_toggle;
 };
 
 /**
@@ -197,6 +198,9 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
 	bool status;
 	int err;
 
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return -EINVAL;
+
 	mutex_lock(&pmc->powergates_lock);
 
 	if (tegra_powergate_state(id) == new_state) {
@@ -250,6 +254,9 @@ int tegra_powergate_is_powered(unsigned int id)
 	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return -EINVAL;
+
 	mutex_lock(&pmc->powergates_lock);
 	status = tegra_powergate_state(id);
 	mutex_unlock(&pmc->powergates_lock);
@@ -968,6 +975,7 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = {
 	.cpu_powergates = tegra30_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra114_powergates[] = {
@@ -1005,6 +1013,7 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = {
 	.cpu_powergates = tegra114_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra124_powergates[] = {
@@ -1048,6 +1057,7 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = {
 	.cpu_powergates = tegra124_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const char * const tegra210_powergates[] = {
@@ -1091,6 +1101,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.cpu_powergates = tegra210_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const struct of_device_id tegra_pmc_match[] = {
-- 
2.1.4

[PATCH V5 08/14] PM / Domains: Add function to remove a pm-domain

[Jon Hunter]

The genpd framework allows users to add power-domains via the
pm_genpd_init() function, however, there is no corresponding function
to remove a power-domain. For most devices this may be fine as the power
domains are never removed, however, for devices that wish to populate
the power-domains from within a driver, having the ability to remove a
power domain if the probing of the device fails or the driver is unloaded
is necessary. Therefore, add a function to remove a power-domain. Please
note that the power domain can only be removed if there are no devices
using the power-domain and it is not linked to another domain.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 45e3641b427d..b4120121bcac 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1529,6 +1529,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 
+/**
+ * pm_genpd_remove - Remove a generic I/O PM domain object.
+ * @genpd: PM domain object to remove.
+ */
+int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	if (IS_ERR_OR_NULL(genpd))
+		return -EINVAL;
+
+	mutex_lock(&genpd->lock);
+
+	if (!list_empty(&genpd->master_links)
+	    || !list_empty(&genpd->slave_links) || genpd->device_count) {
+		mutex_unlock(&genpd->lock);
+		return -EBUSY;
+	}
+
+	mutex_lock_nested(&gpd_list_lock, SINGLE_DEPTH_NESTING);
+	list_del(&genpd->gpd_list_node);
+	mutex_unlock(&gpd_list_lock);
+	mutex_unlock(&genpd->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_remove);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index db21d3995f7e..0d661998fa74 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 				     struct generic_pm_domain *target);
 extern void pm_genpd_init(struct generic_pm_domain *genpd,
 			  struct dev_power_governor *gov, bool is_off);
+extern int pm_genpd_remove(struct generic_pm_domain *genpd);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -161,6 +162,10 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
 				 struct dev_power_governor *gov, bool is_off)
 {
 }
+static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
+{
+	return -ENOTSUPP;
+}
 #endif
 
 static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
-- 
2.1.4

Re: [PATCH V5 08/14] PM / Domains: Add function to remove a pm-domain

[Ulf Hansson]

On 28 January 2016 at 17:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> The genpd framework allows users to add power-domains via the
> pm_genpd_init() function, however, there is no corresponding function
> to remove a power-domain. For most devices this may be fine as the power
> domains are never removed, however, for devices that wish to populate
> the power-domains from within a driver, having the ability to remove a
> power domain if the probing of the device fails or the driver is unloaded
> is necessary. Therefore, add a function to remove a power-domain. Please
> note that the power domain can only be removed if there are no devices
> using the power-domain and it is not linked to another domain.
>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  5 +++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 45e3641b427d..b4120121bcac 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1529,6 +1529,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>  }
>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> +/**
> + * pm_genpd_remove - Remove a generic I/O PM domain object.
> + * @genpd: PM domain object to remove.
> + */
> +int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       if (IS_ERR_OR_NULL(genpd))
> +               return -EINVAL;
> +
> +       mutex_lock(&genpd->lock);

pm_genpd_summary_show() first locks the gpd_list_lock, then the genpd lock.

Please preserve that order here as well, to prevent potential deadlocks.

> +
> +       if (!list_empty(&genpd->master_links)
> +           || !list_empty(&genpd->slave_links) || genpd->device_count) {
> +               mutex_unlock(&genpd->lock);
> +               return -EBUSY;
> +       }
> +
> +       mutex_lock_nested(&gpd_list_lock, SINGLE_DEPTH_NESTING);

Why the nesting, can this really cause lockdep warnings?

> +       list_del(&genpd->gpd_list_node);
> +       mutex_unlock(&gpd_list_lock);
> +       mutex_unlock(&genpd->lock);

Before returning, you need to make sure there isn't a scheduled work
for powering off the genpd.
That might happen for example happen via genpd_poweroff_unused().

Otherwise, the caller of pm_genpd_remove() might free the memory while
the genpd struct is still in use...

I assume a cancel_delayed_work_sync() should do the trick here.

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_remove);
> +
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  /*
>   * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index db21d3995f7e..0d661998fa74 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                                      struct generic_pm_domain *target);
>  extern void pm_genpd_init(struct generic_pm_domain *genpd,
>                           struct dev_power_governor *gov, bool is_off);
> +extern int pm_genpd_remove(struct generic_pm_domain *genpd);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -161,6 +162,10 @@ static inline void pm_genpd_init(struct generic_pm_domain *genpd,
>                                  struct dev_power_governor *gov, bool is_off)
>  {
>  }
> +static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> +       return -ENOTSUPP;
> +}
>  #endif
>
>  static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
> --
> 2.1.4
>

Kind regards
Uffe

Re: [PATCH V5 08/14] PM / Domains: Add function to remove a pm-domain

[Jon Hunter]

On 02/02/16 15:35, Ulf Hansson wrote:
> On 28 January 2016 at 17:33, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> The genpd framework allows users to add power-domains via the
>> pm_genpd_init() function, however, there is no corresponding function
>> to remove a power-domain. For most devices this may be fine as the power
>> domains are never removed, however, for devices that wish to populate
>> the power-domains from within a driver, having the ability to remove a
>> power domain if the probing of the device fails or the driver is unloaded
>> is necessary. Therefore, add a function to remove a power-domain. Please
>> note that the power domain can only be removed if there are no devices
>> using the power-domain and it is not linked to another domain.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  5 +++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 45e3641b427d..b4120121bcac 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1529,6 +1529,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>  }
>>  EXPORT_SYMBOL_GPL(pm_genpd_init);
>>
>> +/**
>> + * pm_genpd_remove - Remove a generic I/O PM domain object.
>> + * @genpd: PM domain object to remove.
>> + */
>> +int pm_genpd_remove(struct generic_pm_domain *genpd)
>> +{
>> +       if (IS_ERR_OR_NULL(genpd))
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&genpd->lock);
> 
> pm_genpd_summary_show() first locks the gpd_list_lock, then the genpd lock.
> 
> Please preserve that order here as well, to prevent potential deadlocks.

Ok, yes good point.

>> +
>> +       if (!list_empty(&genpd->master_links)
>> +           || !list_empty(&genpd->slave_links) || genpd->device_count) {
>> +               mutex_unlock(&genpd->lock);
>> +               return -EBUSY;
>> +       }
>> +
>> +       mutex_lock_nested(&gpd_list_lock, SINGLE_DEPTH_NESTING);
> 
> Why the nesting, can this really cause lockdep warnings?

Hmmm, yes I don't believe that is needed here as it should be a
different class. Will fix.

>> +       list_del(&genpd->gpd_list_node);
>> +       mutex_unlock(&gpd_list_lock);
>> +       mutex_unlock(&genpd->lock);
> 
> Before returning, you need to make sure there isn't a scheduled work
> for powering off the genpd.
> That might happen for example happen via genpd_poweroff_unused().
> 
> Otherwise, the caller of pm_genpd_remove() might free the memory while
> the genpd struct is still in use...
> 
> I assume a cancel_delayed_work_sync() should do the trick here.

Good point. I will address that too.

Jon

[PATCH V5 14/14] ARM64: tegra: select PM_GENERIC_DOMAINS

[Jon Hunter]

Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices
dependent upon a particular power-domain are only probed when that power
domain has been powered up, requires that PM is made mandatory for tegra
64-bit devices and so select this option for tegra as well.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 8a0952275291..cdec2dfae3aa 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -102,6 +102,8 @@ config ARCH_TEGRA
 	select GENERIC_CLOCKEVENTS
 	select HAVE_CLK
 	select PINCTRL
+	select PM
+	select PM_GENERIC_DOMAINS
 	select RESET_CONTROLLER
 	help
 	  This enables support for the NVIDIA Tegra SoC family.
-- 
2.1.4