Add generic PM domain support for Tegra

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 <[email protected]>

---
 drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

drivers/soc/tegra/pmc.c +12 -3

index 85b4e166273a..f8cdb7ce9755 100644
@@ -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;

drivers/soc/tegra/pmc.c +3 -1

 }
 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)

drivers/soc/tegra/pmc.c +7 -1

+
+       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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 84 lines · 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 <[email protected]>

---
 drivers/soc/tegra/pmc.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

drivers/soc/tegra/pmc.c +13 -3

index 85b4e166273a..f8cdb7ce9755 100644
@@ -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();
 

drivers/soc/tegra/pmc.c int tegra_io_rail_power_on(int id) · line 556

Quoted 5 lines

drivers/soc/tegra/pmc.c +4 -1

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

Quoted 21 lines

drivers/soc/tegra/pmc.c +3 -1

 }
 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)

drivers/soc/tegra/pmc.c int tegra_io_rail_power_off(int id) · line 591

Quoted 10 lines

drivers/soc/tegra/pmc.c +5 -2

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

Quoted 5 lines

drivers/soc/tegra/pmc.c +0 -0

 }
 EXPORT_SYMBOL(tegra_io_rail_power_off);
 

@@ -817,9 +835,15 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 

drivers/soc/tegra/pmc.c static int tegra_pmc_probe(struct platform_device *pdev) · line 841

Quoted 8 lines

drivers/soc/tegra/pmc.c +3 -2

        /* 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.

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 <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       |  9 ++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

arch/arm/mach-tegra/platsmp.c +3 -13

index f3f61dbbda97..75620ae73913 100644
@@ -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. */

drivers/soc/tegra/pmc.c +4 -0

index 99cb2fdd29e1..35ee60fd17be 100644
@@ -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) {

drivers/soc/tegra/pmc.c +1 -2

        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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 74 lines · 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 <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       |  9 ++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

arch/arm/mach-tegra/platsmp.c +3 -13

index f3f61dbbda97..75620ae73913 100644
@@ -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. */

drivers/soc/tegra/pmc.c +4 -0

index 99cb2fdd29e1..35ee60fd17be 100644
@@ -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);
 

drivers/soc/tegra/pmc.c static int tegra_powergate_set(unsigned int id, bool new_state) · line 204

Quoted 3 lines

drivers/soc/tegra/pmc.c +3 -0

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

Quoted 75 lines · Mathieu Poirier

Quoted 74 lines · 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 <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       |  9 ++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

arch/arm/mach-tegra/platsmp.c +3 -13

index f3f61dbbda97..75620ae73913 100644
@@ -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. */

drivers/soc/tegra/pmc.c +4 -0

index 99cb2fdd29e1..35ee60fd17be 100644
@@ -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);
 

Quoted 10 lines

drivers/soc/tegra/pmc.c static int tegra_powergate_set(unsigned int id, bool new_state) · line 204

Quoted 3 lines

drivers/soc/tegra/pmc.c +3 -0

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

Quoted 88 lines · Jon Hunter

Quoted 75 lines · Mathieu Poirier

Quoted 74 lines · 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 <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c | 16 +++-------------
 drivers/soc/tegra/pmc.c       |  9 ++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

arch/arm/mach-tegra/platsmp.c +3 -13

index f3f61dbbda97..75620ae73913 100644
@@ -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. */

drivers/soc/tegra/pmc.c +4 -0

index 99cb2fdd29e1..35ee60fd17be 100644
@@ -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);
 

Quoted 10 lines

drivers/soc/tegra/pmc.c static int tegra_powergate_set(unsigned int id, bool new_state) · line 204

Quoted 3 lines

drivers/soc/tegra/pmc.c +3 -0

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

Quoted 3 lines

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

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 <[email protected]>

---
 drivers/soc/tegra/pmc.c | 21 +++++++++++++++++----
 include/soc/tegra/pmc.h |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

drivers/soc/tegra/pmc.c +7 -0

index 35ee60fd17be..032dd5c17130 100644
@@ -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

drivers/soc/tegra/pmc.c +9 -3

        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);
 
        /*

include/soc/tegra/pmc.h +1 -1

index 07e332dd44fb..e9e53473a63e 100644
@@ -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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 49 lines · 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 <[email protected]>

---
 drivers/soc/tegra/pmc.c | 21 +++++++++++++++++----
 include/soc/tegra/pmc.h |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

drivers/soc/tegra/pmc.c +7 -0

index 35ee60fd17be..032dd5c17130 100644
@@ -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)

drivers/soc/tegra/pmc.c static int tegra_powergate_set(unsigned int id, bool new_state) · line 224

Quoted 4 lines

drivers/soc/tegra/pmc.c +1 -1

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

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 <[email protected]>

---
 drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 31 insertions(+)

drivers/base/power/domain.c +0 -0

index 45e3641b427d..b4120121bcac 100644
@@ -1529,6 +1529,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 

drivers/base/power/domain.c +5 -0

+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_remove);
+
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 /*
  * Device Tree based PM domain providers.

include/linux/pm_domain.h +5 -1

index db21d3995f7e..0d661998fa74 100644
@@ -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

Quoted 24 lines · 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 <[email protected]>

---
 drivers/base/power/domain.c | 26 ++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  5 +++++
 2 files changed, 31 insertions(+)

drivers/base/power/domain.c +0 -0

index 45e3641b427d..b4120121bcac 100644
@@ -1529,6 +1529,32 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 }
 EXPORT_SYMBOL_GPL(pm_genpd_init);
 

drivers/base/power/domain.c void pm_genpd_init(struct generic_pm_domain *genpd, · line 1541

Quoted 10 lines

drivers/base/power/domain.c +10 -0

+/**
+ * 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.

drivers/base/power/domain.c void pm_genpd_init(struct generic_pm_domain *genpd, · line 1549

Quoted 8 lines

drivers/base/power/domain.c +8 -0

+
+       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?

drivers/base/power/domain.c void pm_genpd_init(struct generic_pm_domain *genpd, · line 1552

Quoted 3 lines

drivers/base/power/domain.c +3 -0

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

Add the PMC driver compatible strings for Tegra132 and Tegra210.

Signed-off-by Jon Hunter <[email protected]>

---
 .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt     | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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

Signed-off-by Jon Hunter <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +11 -0

index 53aa5496c5cf..3c77373aa826 100644
@@ -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 at 7000f400 {
 	};
...
 };
+
+
+== PM Domain Nodes ==
+
+Each of the PM domain nodes represents a power-domain on the Tegra SoC

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

Quoted 45 lines · Jon Hunter

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

Signed-off-by Jon Hunter <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +16 -0

index 53aa5496c5cf..3c77373aa826 100644
@@ -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 at 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.

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt pmc at 7000f400 { · line 138

Quoted 5 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +5 -0

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

Quoted 46 lines · Rob Herring

Quoted 45 lines · Jon Hunter

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

Signed-off-by Jon Hunter <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +16 -0

index 53aa5496c5cf..3c77373aa826 100644
@@ -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 at 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.

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt pmc at 7000f400 { · line 138

Quoted 5 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +5 -0

+  - 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 at 702e2000 {
                ...
                power-domains = <&pd_audio>;
                ...
        };

Quoted 1 line

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

That is fine with me.

Jon

Quoted 57 lines · Jon Hunter

Quoted 46 lines · Rob Herring

Quoted 45 lines · Jon Hunter

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

Signed-off-by Jon Hunter <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +16 -0

index 53aa5496c5cf..3c77373aa826 100644
@@ -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 at 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.

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt pmc at 7000f400 { · line 138

Quoted 5 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +5 -0

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

Quoted 8 lines

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

        adma: adma at 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.

Quoted 67 lines · Rob Herring

Quoted 57 lines · Jon Hunter

Quoted 46 lines · Rob Herring

Quoted 45 lines · Jon Hunter

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

Signed-off-by Jon Hunter <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +16 -0

index 53aa5496c5cf..3c77373aa826 100644
@@ -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 at 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.

Quoted 8 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt pmc at 7000f400 { · line 138

Quoted 5 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +5 -0

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

Quoted 8 lines

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

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

Quoted 3 lines

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.

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 <[email protected]> and Vince Hsu <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>

---
 drivers/soc/tegra/pmc.c                     | 470 ++++++++++++++++++++++++++--
 include/dt-bindings/power/tegra-powergate.h |  36 +++
 include/soc/tegra/pmc.h                     |  39 +--

drivers/soc/tegra/pmc.c +17 -0

+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;

drivers/soc/tegra/pmc.c +24 -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;
+               }
+       }
+

drivers/soc/tegra/pmc.c +13 -0

+               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;
+

Quoted 8 lines · 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.

Quoted 8 lines

This is based upon work by Thierry Reding <[email protected]> and Vince Hsu <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>

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

[...]

Quoted 25 lines

drivers/soc/tegra/pmc.c +24 -0

+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]);

drivers/soc/tegra/pmc.c static int tegra_powergate_set(unsigned int id, bool new_state) · line 272

Quoted 3 lines

drivers/soc/tegra/pmc.c +3 -0

+
+       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!?

[...]

drivers/soc/tegra/pmc.c EXPORT_SYMBOL(tegra_powergate_remove_clamping); · line 513

Quoted 6 lines

/**
* tegra_powergate_power_on() - power on partition
* @id: partition ID

drivers/soc/tegra/pmc.c +0 -0

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

[...]

Quoted 24 lines

drivers/soc/tegra/pmc.c +24 -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;
+               }
+       }
+

drivers/soc/tegra/pmc.c static int tegra_powergate_debugfs_init(void) · line 701

Quoted 10 lines

drivers/soc/tegra/pmc.c +10 -0

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

[...]

drivers/soc/tegra/pmc.c static int tegra_powergate_debugfs_init(void) · line 851

Quoted 6 lines

drivers/soc/tegra/pmc.c +6 -0

+
+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?

Quoted 13 lines

drivers/soc/tegra/pmc.c EXPORT_SYMBOL(tegra_powergate_remove_clamping); · line 513

Quoted 7 lines

/**
* tegra_powergate_power_on() - power on partition
* @id: partition ID

drivers/soc/tegra/pmc.c +0 -0

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

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 11 lines

drivers/soc/tegra/pmc.c static int tegra_powergate_debugfs_init(void) · line 851

Quoted 6 lines

drivers/soc/tegra/pmc.c +6 -0

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

Quoted 5 lines

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!

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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?

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 51 lines · Jon Hunter

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 1 line

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!?

Quoted 52 lines · Ulf Hansson

Quoted 51 lines · Jon Hunter

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 10 lines

Quoted 4 lines

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?

[...]

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

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?

Quoted 4 lines

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?

Quoted 52 lines · Ulf Hansson

Quoted 51 lines · Jon Hunter

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 60 lines · Jon Hunter

Quoted 52 lines · Ulf Hansson

Quoted 51 lines · Jon Hunter

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 1 line

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!?

Quoted 5 lines

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! :-)

Quoted 70 lines · Ulf Hansson

Quoted 60 lines · Jon Hunter

Quoted 52 lines · Ulf Hansson

Quoted 51 lines · Jon Hunter

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

[snip]

Quoted 28 lines

Quoted 12 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 1 line

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!?

Quoted 5 lines

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.

Quoted 1 line

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()?

Quoted 12 lines · 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 <[email protected]> and Vince Hsu <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>