Add generic PM domain support for Tegra

Some members of the tegra_pmc structure are missing from the kernel-doc comment for this structure. Add the missing members.

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

---
 drivers/soc/tegra/pmc.c | 2 ++
 1 file changed, 2 insertions(+)

Sparse reports the following warning for tegra_pmc_init_tsense_reset:

drivers/soc/tegra/pmc.c:741:6: warning: symbol 'tegra_pmc_init_tsense_reset' was not declared. Should it be static?

This function is only used internally by the PMC driver and so fix this by making it static.

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

---
 drivers/soc/tegra/pmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The debugfs entry for the PMC device will not be removed if the probe of the device fails on registering the restart handler and so fix this so that it is removed.

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

---
 drivers/soc/tegra/pmc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

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.

Rather than adding a test to see if the PMC register mapping is valid, fix this by removing the second mapping of the PMC registers and reserve the memory region for the PMC registers during early initialisation where the initial mapping is created. During the probing of the PMC simply check that the PMC registers have been mapped.

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

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

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

-	struct resource *res;
 	int err;
 
+	if (!pmc->base) {
+		dev_err(&pdev->dev, "base address is not configured\n");

* PGP Signed by an unknown key

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

Rather than adding a test to see if the PMC register mapping is valid, fix this by removing the second mapping of the PMC registers and reserve the memory region for the PMC registers during early initialisation where the initial mapping is created. During the probing of the PMC simply check that the PMC registers have been mapped.

Quoted 4 lines

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

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

devm_ioremap_resource() was used on purpose to make sure we get a proper name assigned to the memory region in /proc/iomem. As it is, there will be no name associated with it, which I think is unfortunate. As such I'd prefer keeping that call and instead fix the issue with the invalid mapping by making sure that pmc->base is assigned only after nothing can fail in probe anymore.

drivers/soc/tegra/pmc.c out: · line 810

Quoted 9 lines

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

index e60fc2d33c94..fdd1a8d0940f 100644
@@ -807,22 +807,17 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base = pmc->base;

The alternative that I proposed above would entail not setting this...

Quoted 5 lines

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

-	struct resource *res;
 	int err;
 
+	if (!pmc->base) {
+		dev_err(&pdev->dev, "base address is not configured\n");

drivers/soc/tegra/pmc.c out: · line 820

Quoted 10 lines

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

+		return -ENXIO;
+	}
+
 	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
 	if (err < 0)
 		return err;
 
-	/* take over the memory region from the early initialization */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pmc->base = devm_ioremap_resource(&pdev->dev, res);

... and storing the result of the mapping in "base" instead...

drivers/soc/tegra/pmc.c out: · line 824

Quoted 4 lines

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

-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
-
-	iounmap(base);

... and move the unmap to the very end of the probe function, which would look something like:

	/* take over the memory region from the early initialization */
	iounmap(pmc->base);
	pmc->base = base;

That way the mapping in "base" will automatically be undone upon error and the pmc->base will only be overridden when it's certain that the probe will succeed.

What do you think?

Quoted 28 lines · Thierry Reding

* PGP Signed by an unknown key

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

Rather than adding a test to see if the PMC register mapping is valid, fix this by removing the second mapping of the PMC registers and reserve the memory region for the PMC registers during early initialisation where the initial mapping is created. During the probing of the PMC simply check that the PMC registers have been mapped.

Quoted 4 lines

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

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

Quoted 6 lines

devm_ioremap_resource() was used on purpose to make sure we get a proper name assigned to the memory region in /proc/iomem. As it is, there will be no name associated with it, which I think is unfortunate. As such I'd prefer keeping that call and instead fix the issue with the invalid mapping by making sure that pmc->base is assigned only after nothing can fail in probe anymore.

Yes, however, you still get a valid name in /proc/iomem with this change. I made sure I tested that ...

/ # cat /proc/iomem
6000d000-6000dfff : /gpio@0,6000d000
60020000-600213ff : /dma@0,60020000
700008d4-70000b6f : /pinmux@0,700008d4
70003000-70003293 : /pinmux@0,700008d4
70006000-7000603f : serial
7000d100-7000d1ff : /i2c@0,7000d100
7000e400-7000e7ff : /pmc@0,7000e400
...

The only expection might be the non-DT case, but I am not sure we care about that for most boards? Hmm ... I wonder if I need to set "regs.name" for the non-DT case? I probably should ...

Quoted 46 lines

drivers/soc/tegra/pmc.c out: · line 810

Quoted 9 lines

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

index e60fc2d33c94..fdd1a8d0940f 100644
@@ -807,22 +807,17 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base = pmc->base;

The alternative that I proposed above would entail not setting this...

Quoted 5 lines

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

-	struct resource *res;
 	int err;
 
+	if (!pmc->base) {
+		dev_err(&pdev->dev, "base address is not configured\n");

drivers/soc/tegra/pmc.c out: · line 820

Quoted 10 lines

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

+		return -ENXIO;
+	}
+
 	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
 	if (err < 0)
 		return err;
 
-	/* take over the memory region from the early initialization */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pmc->base = devm_ioremap_resource(&pdev->dev, res);

... and storing the result of the mapping in "base" instead...

drivers/soc/tegra/pmc.c out: · line 824

Quoted 4 lines

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

-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
-
-	iounmap(base);

... and move the unmap to the very end of the probe function, which would look something like:

	/* take over the memory region from the early initialization */
	iounmap(pmc->base);
	pmc->base = base;

That way the mapping in "base" will automatically be undone upon error and the pmc->base will only be overridden when it's certain that the probe will succeed.

Quoted 1 line

What do you think?

I thought about that, but it still seems racy. You probably want to assign the new base before freeing the old and so you would need another tmp variable.

Even so, I was not sure if there could be a race here.
Ideally, you would lock, but then you need to lock everywhere that you use base. Given that my patch still provides a /proc/iomem entry with a valid name, it seems best to me.

* PGP Signed by an unknown key

Quoted 29 lines · Jon Hunter

Quoted 27 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

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

Rather than adding a test to see if the PMC register mapping is valid, fix this by removing the second mapping of the PMC registers and reserve the memory region for the PMC registers during early initialisation where the initial mapping is created. During the probing of the PMC simply check that the PMC registers have been mapped.

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

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

Ah yes, of course. You could still do it with just the two pointers if you keep the code as-is and revert back to the backed up value in case of errors. Along this line:

	base = pmc->base;

	pmc->base = devm_ioremap_resource(&pdev->dev, res);

	/* on success */
	iounmap(base);

	/* on error */
	pmc->base = base;

These pointer assignments should be atomic, so no potential for races there. I've attached a patch which should do the trick, though I have not tested it.

Quoted 4 lines

Even so, I was not sure if there could be a race here.
Ideally, you would lock, but then you need to lock everywhere that you use base. Given that my patch still provides a /proc/iomem entry with a valid name, it seems best to me.

The primary reason for the "takeover" is that except for the extra iounmap() and pointer swapping the tegra_pmc_probe() function is really a standard driver implementation. The idea behind this had always been that it should be possible to easily convert this to a proper driver if either we ended up with PSCI exclusively for SMP or defer SMP setup until the PMC driver had been probed, so that we could get rid of the early_initcall().

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 macro has also been adding 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       | 21 +++++++++++++++------
 2 files changed, 18 insertions(+), 19 deletions(-)

arch/arm/mach-tegra/platsmp.c +0 -9

index b45086666648..40cb761e7c95 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))

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

index fdd1a8d0940f..f94d970089ce 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>

* PGP Signed by an unknown key

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

Quoted 4 lines

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

A macro has also been adding for checking the status of the powergate and

"added"

Quoted 33 lines

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       | 21 +++++++++++++++------
 2 files changed, 18 insertions(+), 19 deletions(-)

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

index b45086666648..40cb761e7c95 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;
 

arch/arm/mach-tegra/platsmp.c static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle) · line 116

Quoted 2 lines

arch/arm/mach-tegra/platsmp.c +0 -0

 remove_clamps:
 	/* CPU partition is powered. Enable the CPU clock. */

I vaguely remember this being very brittle. I assume you've tested this extensively and made sure it doesn't regress?

Quoted 13 lines

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

index fdd1a8d0940f..f94d970089ce 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>

drivers/soc/tegra/pmc.c line 106

Quoted 5 lines

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

@@ -101,6 +102,8 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+#define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
+

This looks suspiciously like a register or register field definition. Maybe turn this into a static inline function, such as:

	static inline bool tegra_powergate_state(u32 status, int id)
	{
		return (status & BIT(id)) != 0;
	}

?

Quoted 4 lines

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

 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;

@@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)

drivers/soc/tegra/pmc.c static void tegra_pmc_writel(u32 value, unsigned long offset) · line 188

Quoted 6 lines

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

  */
 static int tegra_powergate_set(int id, bool new_state)
 {
-	bool status;
+	u32 status;
+	int err = 0;

Initialization to 0 doesn't seem necessary here.

The power partitions L2, HEG, CELP and C1NC do not exist for T210 but were incorrectly documented in the TRM. These will be removed from the TRM and so also remove their definitions for T210.

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

---
 drivers/soc/tegra/pmc.c | 4 ----
 1 file changed, 4 deletions(-)

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 computing a bit-mask of the valid partition IDs for a device and add a macro that will test if the partition is valid based upon this mask.

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

---
 drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

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

index 28d3106d3add..0967bba13947 100644

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

 struct tegra_pmc_soc {
 	unsigned int num_powergates;

@@ -134,6 +135,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_mask: Bit mask of valid power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {

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

 	return tegra_powergate_set(id, true);

@@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
  */
 int tegra_powergate_power_off(int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);

@@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
 {
 	u32 status;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	status = tegra_pmc_readl(PWRGATE_STATUS);

@@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	/*

* PGP Signed by an unknown key

Quoted 20 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 computing a bit-mask of the valid partition IDs for a device and add a macro that will test if the partition is valid based upon this mask.

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

---
 drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

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

index 28d3106d3add..0967bba13947 100644
@@ -103,6 +103,7 @@
 #define GPU_RG_CNTRL			0x2d4
 

drivers/soc/tegra/pmc.c line 106

Quoted 2 lines

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

 #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
+#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))

I'd prefer a static inline function here, too. If you do that you will also need to move the static inline below the struct tegra_pmc to avoid build errors. Also don't forget to check for id < 0, because it technically could be. It may also be worth checking for an upper bound just in case anybody was going to pass in a value other that the ones defined in include/soc/tegra/pmc.h.

Quoted 12 lines

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

 struct tegra_pmc_soc {
 	unsigned int num_powergates;

@@ -134,6 +135,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_mask: Bit mask of valid power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {

@@ -158,6 +160,7 @@ struct tegra_pmc {
 	bool cpu_pwr_good_en;

drivers/soc/tegra/pmc.c struct tegra_pmc { · line 163

Quoted 3 lines

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

 	u32 lp0_vec_phys;
 	u32 lp0_vec_size;
+	u32 powergates_mask;

This seems rather risky. The highest partition ID that we currently have is 29, so there is potential that 32 bits will be exceeded in the near future. It'd be more future-proof to turn this into a bitmap from the start so that we never have to worry about it.

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

Quoted 10 lines

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

 	struct mutex powergates_lock;
 };

@@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
  */
 int tegra_powergate_power_on(int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;

In a similar vein I've been meaning, for a long time, to make the ID an unsigned integer. It might be worth doing that, even with this patch applied, but it can be a separate patch.

Quoted 27 lines

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

 	return tegra_powergate_set(id, true);

@@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
  */
 int tegra_powergate_power_off(int id)
 {
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);

@@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
 {
 	u32 status;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 
 	status = tegra_pmc_readl(PWRGATE_STATUS);

@@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
+	if (!PMC_PWRGATE_IS_VALID(id))
 		return -EINVAL;
 

drivers/soc/tegra/pmc.c static int __init tegra_pmc_early_init(void) · line 1090

Quoted 7 lines

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

 	/*

@@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
 	struct device_node *np;
 	struct resource regs;
 	bool invert;
-	u32 value;
+	u32 value, i;

I'd prefer an unsized type (unsigned int) for i because there is no reason why it should be sized.

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

---
 drivers/soc/tegra/pmc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

* PGP Signed by an unknown key

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

Quoted 4 lines

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

---
 drivers/soc/tegra/pmc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

The TRM doesn't mention anything like this. Will this be updated in the TRM as well?

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

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

index 02c27004d4a8..838e1a69ec0a 100644

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

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

Signature · 1 line

2.1.4

Quoted 13 lines · Jon Hunter

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

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

index 02c27004d4a8..838e1a69ec0a 100644
@@ -9,8 +9,9 @@ Required properties:
 - 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

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt Required properties: · line 14

Quoted 4 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt +3 -1

-  above, where <chip> is tegra132.
+  For Tegra210, must contain "nvidia,tegra210-pmc". Otherwise, must contain
+  "nvidia,<chip>-pmc", plus at least one of the above, where <chip> is
+  tegra132.

Can you reorg this some so the compatible strings are a list rather than a paragraph of text. I find the last sentence a bit confusing as well. Seems like <chip> should be a list rather than just tegra132.

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

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

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };
+
+
+== PM Domain Nodes ==
+
+Each of the PM domain nodes represents a power-domain on the Tegra SoC

Quoted 7 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]>

---

Quoted 9 lines

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

Seems like that is just a failure of drivers to control all the clocks they need IMO.

Quoted 40 lines

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ 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

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

Quoted 6 lines

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

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

"reg" does not work here?

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

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

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };

Quoted 47 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]>

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };

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

Quoted 10 lines

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

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

We've had this discussiona for a couple of other SoCs, so I need to ask...

Presumably these are not device clocks that a runtime PM enabled driver should be managing for a device, right? IOW, We want to make sure that the PM domain isn't managing clocks for drivers that should be doing it.

I understand there are legitimate reasons for the PM domain to manage clocks in addition to device drivers (e.g. for synchronous reset), but just want to be sure it's not a shortcut for having a proper driver.

Quoted 63 lines · Kevin Hilman

Quoted 47 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]>

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };

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

Quoted 10 lines

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

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

We've had this discussiona for a couple of other SoCs, so I need to ask...

Quoted 7 lines

Presumably these are not device clocks that a runtime PM enabled driver should be managing for a device, right? IOW, We want to make sure that the PM domain isn't managing clocks for drivers that should be doing it.

I understand there are legitimate reasons for the PM domain to manage clocks in addition to device drivers (e.g. for synchronous reset), but just want to be sure it's not a shortcut for having a proper driver.

So some clocks may also be used by devices, but they are needed as part of the power ungating/gating sequence. The general power-up sequence for tegra is ...

1. Enable the power-domain
2. Enable the clock(s)
3. Remove signal clamps
4. De-assert reset(s)
5. Disable clocks (optional)

You may say we should only handle #1 above for the powering up sequence, but we can't do this. The reason is that there is one bit for each power-domain that controls the signaling clamps and so we need to turn on all the clocks specified in the TRM before we do this. Once we have done this and released the reset(s), we can then disable the clocks again (shown above an optional as it is not mandatory from a design perspective) and then the devices in the power-domain should enable the clocks they need as and when they want them.

Please note that I 100% agree that all clocks required by a device are handled by the device and we do not implement any short-cuts here. The only question I had was if there are clocks that may be bus clocks in the power-domain that are required by all device in the power-domain. However, may be this should be represented as a bus driver and all the devices are children of it?

Quoted 92 lines · Jon Hunter

Quoted 63 lines · Kevin Hilman

Quoted 47 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]>

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };

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

Quoted 10 lines

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

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

We've had this discussiona for a couple of other SoCs, so I need to ask...

Quoted 7 lines

Presumably these are not device clocks that a runtime PM enabled driver should be managing for a device, right? IOW, We want to make sure that the PM domain isn't managing clocks for drivers that should be doing it.

I understand there are legitimate reasons for the PM domain to manage clocks in addition to device drivers (e.g. for synchronous reset), but just want to be sure it's not a shortcut for having a proper driver.

So some clocks may also be used by devices, but they are needed as part of the power ungating/gating sequence. The general power-up sequence for tegra is ...

1. Enable the power-domain
2. Enable the clock(s)
3. Remove signal clamps
4. De-assert reset(s)
5. Disable clocks (optional)

You may say we should only handle #1 above for the powering up sequence, but we can't do this. The reason is that there is one bit for each power-domain that controls the signaling clamps and so we need to turn on all the clocks specified in the TRM before we do this. Once we have done this and released the reset(s), we can then disable the clocks again (shown above an optional as it is not mandatory from a design perspective) and then the devices in the power-domain should enable the clocks they need as and when they want them.

Quoted 6 lines

Please note that I 100% agree that all clocks required by a device are handled by the device and we do not implement any short-cuts here. The only question I had was if there are clocks that may be bus clocks in the power-domain that are required by all device in the power-domain. However, may be this should be represented as a bus driver and all the devices are children of it?

Although the "nvidia,powergate-disable-clocks" optional property I had proposed could be seen as a bit of a short-cut, it is true :-) May be I should make the disabling of clocks again mandatory for the power-up sequence.

Quoted 63 lines · Kevin Hilman

Quoted 47 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]>

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+
 Example:
 
 / SoC dts including file

@@ -114,3 +120,58 @@ pmc@7000f400 {
 	};
...
 };

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

Quoted 10 lines

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

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

We've had this discussiona for a couple of other SoCs, so I need to ask...

Quoted 7 lines

Presumably these are not device clocks that a runtime PM enabled driver should be managing for a device, right? IOW, We want to make sure that the PM domain isn't managing clocks for drivers that should be doing it.

I understand there are legitimate reasons for the PM domain to manage clocks in addition to device drivers (e.g. for synchronous reset), but just want to be sure it's not a shortcut for having a proper driver.

So some clocks may also be used by devices, but they are needed as part of the power ungating/gating sequence. The general power-up sequence for tegra is ...

1. Enable the power-domain
2. Enable the clock(s)
3. Remove signal clamps
4. De-assert reset(s)
5. Disable clocks (optional)

You may say we should only handle #1 above for the powering up sequence, but we can't do this. The reason is that there is one bit for each power-domain that controls the signaling clamps and so we need to turn on all the clocks specified in the TRM before we do this. Once we have done this and released the reset(s), we can then disable the clocks again (shown above an optional as it is not mandatory from a design perspective) and then the devices in the power-domain should enable the clocks they need as and when they want them.

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

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

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

* PGP Signed by an unknown key

Quoted 36 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]>

---

Please note that I have been debating whether I add this "nvidia,powergate-clock-disable" property or just leave the clocks disabled by default. Some downstream kernels leave the clocks enabled for the audio power-domain because the clocks required for powering up the power-domain are needed by all modules within the power-domain. However are the same time there are other power-domains that may need to be on, but not always clocked and so having the ability to specify if the clocks should be disabled seems useful. However, I can also remove this and just have the appropriate devices turn on the clocks as well.

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

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

index 838e1a69ec0a..8e4641db51a9 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.

@@ -69,6 +71,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.
 

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt Optional properties for hardware-triggered thermal reset (inside 'i2c-thermtrip' · line 77

Quoted 4 lines

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

+Optional nodes:
+- pm-domains : This node contains a hierarchy of PM domain nodes, which should
+	       match the power-domains on the Tegra SoC.
+

pm-domains is a linuxism. Perhaps name this after what Tegra calls this:
"powergates"?

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

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

+static int tegra_powergate_power_down(struct tegra_powergate *pg,
+				      bool enable_clocks)
+{
+	int err;
+
+	if (enable_clocks) {
+		err = tegra_powergate_enable_clocks(pg);
+		if (err)
+			return err;
+

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

+static int tegra_powergate_of_get_clks(struct device *dev,
+				       struct tegra_powergate *pg)
+{
+	struct clk *clk;
+	unsigned int i, count;
+	int err;
+

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

+static int tegra_powergate_of_get_resets(struct device *dev,
+					 struct tegra_powergate *pg)
+{
+	struct reset_control *rst;
+	unsigned int i, count;
+	int err;
+

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

+static struct tegra_powergate *
+tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
+			struct generic_pm_domain *parent)
+{

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

+
+		pg = tegra_powergate_add_one(pmc, child, parent);
+		if (IS_ERR(pg)) {
+			err = PTR_ERR(pg);
+			goto err;

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

+	}
+
+	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)
+			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
+		pm_genpd_remove(&pg->genpd);

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

@@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_init_tsense_reset(pmc);
 
+	err = tegra_powergate_init(pmc);
+	if (err < 0)
+		return err;
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			return err;
+			goto err_debugfs;
 	}
 
 	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);

* PGP Signed by an unknown key

On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
[...]

Quoted 1 line

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

[...]

Quoted 18 lines

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

+static int tegra_powergate_power_down(struct tegra_powergate *pg,
+				      bool enable_clocks)
+{
+	int err;
+
+	if (enable_clocks) {
+		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);

drivers/soc/tegra/pmc.c

Quoted 2 lines

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

+
+	tegra_powergate_disable_clocks(pg);

There's no guarantee that all clocks are actually disabled at this point. Will the power down and subsequent power up sequences still work properly in such cases? If not perhaps we should add some way of checking for that case here (WARN_ON?) to make sure we can fix up all drivers to release their clock enable references.

Quoted 11 lines

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

+static int tegra_powergate_of_get_clks(struct device *dev,
+				       struct tegra_powergate *pg)
+{
+	struct clk *clk;
+	unsigned int i, count;
+	int err;
+
+	/*
+	 * Determine number of clocks used by the powergate
+	 */
+	for (count = 0; ; count++) {

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

Quoted 6 lines

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

+		clk = of_clk_get(pg->of_node, count);
+		if (IS_ERR(clk))
+			break;
+
+		clk_put(clk);
+	}

of_count_phandle_with_args()?

Quoted 11 lines

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

+static int tegra_powergate_of_get_resets(struct device *dev,
+					 struct tegra_powergate *pg)
+{
+	struct reset_control *rst;
+	unsigned int i, count;
+	int err;
+
+	/*
+	 * Determine number of resets used by the powergate
+	 */
+	for (count = 0; ; count++) {

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

Quoted 6 lines

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

+		rst = of_reset_control_get_by_index(pg->of_node, count);
+		if (IS_ERR(rst))
+			break;
+
+		reset_control_put(rst);
+	}

Same here.

Quoted 4 lines

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

+static struct tegra_powergate *
+tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
+			struct generic_pm_domain *parent)
+{

[...]

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

Quoted 1 line

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

+	dev_info(pmc->dev, "added power domain %s\n", pg->genpd.name);

That's a little chatty, isn't it? Perhaps dev_dbg()?

Quoted 5 lines

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

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

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

Quoted 5 lines

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

+	int err = 0;
+
+	for_each_child_of_node(np, child) {
+		if (err)
+			goto err;

This looks weird. Isn't the same check below good enough to catch all cases?

Quoted 5 lines

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

+
+		pg = tegra_powergate_add_one(pmc, child, parent);
+		if (IS_ERR(pg)) {
+			err = PTR_ERR(pg);
+			goto err;

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

Quoted 10 lines

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

+		}
+
+		if (pg)
+			err = tegra_powergate_add(pmc, child, pg->parent);
+
+err:
+		of_node_put(child);
+
+		if (err)
+			return err;

Perhaps break here instead of return?

Quoted 14 lines

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

+	}
+
+	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)
+			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
+		pm_genpd_remove(&pg->genpd);

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

Quoted 10 lines

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

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

Are generic power domains reference counted? If not this will potentially leave dangling pointers in user drivers, won't it?

That's a problem common to many subsystems, but maybe something to be aware of.

Quoted 24 lines

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

@@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_init_tsense_reset(pmc);
 
+	err = tegra_powergate_init(pmc);
+	if (err < 0)
+		return err;
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
-			return err;
+			goto err_debugfs;
 	}
 
 	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);
-		return err;
+		goto err_restart;
 	}
 

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

Quoted 6 lines

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

 	return 0;
+
+err_restart:
+	debugfs_remove(pmc->debugfs);
+err_debugfs:
+	tegra_powergate_remove(pmc);

I prefer the labels to denote the action that is being taken rather than the error that they respond to. remove_debugfs and remove_powergate would therefore be better here, in my opinion. I think there were a couple more in this and earlier patches.

Quoted 24 lines

* PGP Signed by an unknown key

On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
[...]

Quoted 1 line

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

[...]

Quoted 18 lines

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

+static int tegra_powergate_power_down(struct tegra_powergate *pg,
+				      bool enable_clocks)
+{
+	int err;
+
+	if (enable_clocks) {
+		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);

Quoted 8 lines

drivers/soc/tegra/pmc.c

Quoted 2 lines

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

+
+	tegra_powergate_disable_clocks(pg);

There's no guarantee that all clocks are actually disabled at this point. Will the power down and subsequent power up sequences still work properly in such cases? If not perhaps we should add some way of checking for that case here (WARN_ON?) to make sure we can fix up all drivers to release their clock enable references.

The problem is that there is no easy way to check the status of a clock and whether it is enabled. Yes clk-provider.h does provide a __clk_is_enabled() API but I don't think that this is meant to be used here. May be we need a clk API for disabling a clock that will WARN if the clock is not disabled?

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

---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/Kconfig.platforms +0 -0

index 9806324fa215..e0b5bd0aff0f 100644

arch/arm64/Kconfig.platforms +0 -1

        select RESET_CONTROLLER
        help
          This enables support for the NVIDIA Tegra SoC family.
--

2.1.4

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

---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/Kconfig.platforms +0 -0

index 9806324fa215..e0b5bd0aff0f 100644

arch/arm64/Kconfig.platforms config ARCH_TEGRA · line 97

Quoted 5 lines

arch/arm64/Kconfig.platforms +2 -0

@@ -93,6 +93,8 @@ config ARCH_TEGRA
        select GENERIC_CLOCKEVENTS
        select HAVE_CLK
        select PINCTRL
+       select PM
+       select PM_GENERIC_DOMAINS

If you still want to allow ARCH_TEGRA to run without PM, you should probably change to:

select PM_GENERIC_DOMAINS if PM

Hi Ulf,

Quoted 25 lines · Ulf Hansson

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

---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/Kconfig.platforms +0 -0

index 9806324fa215..e0b5bd0aff0f 100644

arch/arm64/Kconfig.platforms config ARCH_TEGRA · line 97

Quoted 5 lines

arch/arm64/Kconfig.platforms +2 -0

@@ -93,6 +93,8 @@ config ARCH_TEGRA
        select GENERIC_CLOCKEVENTS
        select HAVE_CLK
        select PINCTRL
+       select PM
+       select PM_GENERIC_DOMAINS

If you still want to allow ARCH_TEGRA to run without PM, you should probably change to:

Quoted 1 line

select PM_GENERIC_DOMAINS if PM

Per the changelog this is deliberate. If we allow !PM, then there is a potential that you could probe a device when the power domain is not powered on. I understand that some SoCs turn on all the power-domains when !PM but this will not work for tegra because we don't register the power domain until later in the boot and so we are relying upon probe deferral to defer the probe of devices that use power-domains.

Quoted 28 lines · Jon Hunter

Hi Ulf,

Quoted 25 lines · Ulf Hansson

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

---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/Kconfig.platforms +0 -0

index 9806324fa215..e0b5bd0aff0f 100644

arch/arm64/Kconfig.platforms config ARCH_TEGRA · line 97

Quoted 5 lines

arch/arm64/Kconfig.platforms +2 -0

@@ -93,6 +93,8 @@ config ARCH_TEGRA
        select GENERIC_CLOCKEVENTS
        select HAVE_CLK
        select PINCTRL
+       select PM
+       select PM_GENERIC_DOMAINS

If you still want to allow ARCH_TEGRA to run without PM, you should probably change to:

Quoted 8 lines

Quoted 1 line

select PM_GENERIC_DOMAINS if PM

Per the changelog this is deliberate. If we allow !PM, then there is a potential that you could probe a device when the power domain is not powered on. I understand that some SoCs turn on all the power-domains when !PM but this will not work for tegra because we don't register the power domain until later in the boot and so we are relying upon probe deferral to defer the probe of devices that use power-domains.

So what you are saying is that adding the PM domain support, will fix some devices to become successfully probed as they were broken before?

Quoted 38 lines · Ulf Hansson

Quoted 28 lines · Jon Hunter

Hi Ulf,

Quoted 25 lines · Ulf Hansson

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

---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/Kconfig.platforms +0 -0

index 9806324fa215..e0b5bd0aff0f 100644

arch/arm64/Kconfig.platforms config ARCH_TEGRA · line 97

Quoted 5 lines

arch/arm64/Kconfig.platforms +2 -0

@@ -93,6 +93,8 @@ config ARCH_TEGRA
        select GENERIC_CLOCKEVENTS
        select HAVE_CLK
        select PINCTRL
+       select PM
+       select PM_GENERIC_DOMAINS

If you still want to allow ARCH_TEGRA to run without PM, you should probably change to:

Quoted 8 lines

Quoted 1 line

select PM_GENERIC_DOMAINS if PM

Per the changelog this is deliberate. If we allow !PM, then there is a potential that you could probe a device when the power domain is not powered on. I understand that some SoCs turn on all the power-domains when !PM but this will not work for tegra because we don't register the power domain until later in the boot and so we are relying upon probe deferral to defer the probe of devices that use power-domains.

Quoted 2 lines

So what you are saying is that adding the PM domain support, will fix some devices to become successfully probed as they were broken before?