Add generic PM domain support for Tegra SoCs

From: Vince Hsu <[email protected]>

Add of_reset_control_get_by_index() to allow the drivers to get reset device without knowing its name.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Updated stub function to return -ENOTSUPP instead
of -ENOSYS which should only be used for system calls.]

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

---
v3: addressed comments from Philipp
v2: minor changes according to Alex's comments

---
 drivers/reset/core.c  | 40 +++++++++++++++++++++++++++++-----------
 include/linux/reset.h |  9 +++++++++
 2 files changed, 38 insertions(+), 11 deletions(-)

drivers/reset/core.c +29 -11

index 7955e00d04d4..81ae17d15480 100644
@@ -141,27 +141,24 @@ int reset_control_status(struct reset_control *rstc)
 EXPORT_SYMBOL_GPL(reset_control_status);
 
 /**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * of_reset_control_get_by_index - Lookup and obtain a reference to a reset
+ * controller by index.
  * @node: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
+ * @index: index of the reset controller
  *
- * Use of id names is optional.
+ * This is to be used to perform a list of resets for a device or power domain
+ * in whatever order. Returns a struct reset_control or IS_ERR() condition
+ * containing errno.
  */
-struct reset_control *of_reset_control_get(struct device_node *node,
-					   const char *id)
+struct reset_control *of_reset_control_get_by_index(struct device_node *node,
+					   int index)
 {
 	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
 	struct reset_controller_dev *r, *rcdev;
 	struct of_phandle_args args;
-	int index = 0;
 	int rstc_id;
 	int ret;
 
-	if (id)
-		index = of_property_match_string(node,
-						 "reset-names", id);
 	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
 					 index, &args);
 	if (ret)

@@ -202,6 +199,27 @@ struct reset_control *of_reset_control_get(struct device_node *node,
 
 	return rstc;
 }
+EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
+
+/**
+ * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @node: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ *
+ * Use of id names is optional.
+ */
+struct reset_control *of_reset_control_get(struct device_node *node,
+					   const char *id)
+{
+	int index = 0;
+
+	if (id)
+		index = of_property_match_string(node,
+						 "reset-names", id);
+	return of_reset_control_get_by_index(node, index);
+}
 EXPORT_SYMBOL_GPL(of_reset_control_get);
 
 /**

include/linux/reset.h +9 -0

index da5602bd77d7..26f1b53386d8 100644
@@ -38,6 +38,9 @@ static inline struct reset_control *devm_reset_control_get_optional(
 struct reset_control *of_reset_control_get(struct device_node *node,
 					   const char *id);
 
+struct reset_control *of_reset_control_get_by_index(
+					struct device_node *node, int index);
+
 #else
 
 static inline int reset_control_reset(struct reset_control *rstc)

@@ -92,6 +95,12 @@ static inline struct reset_control *of_reset_control_get(
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct reset_control *of_reset_control_get_by_index(
+				struct device_node *node, int index)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */
 
 #endif

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

* PGP Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Quoted 20 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Quoted 9 lines

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

* PGP Signed by an unknown key

Quoted 31 lines · Peter De Schrijver

Quoted 29 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Quoted 4 lines

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary.

Quoted 34 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 31 lines · Peter De Schrijver

Quoted 29 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Quoted 7 lines

Quoted 4 lines

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary.

I see your point and it is interesting. The trouble is that we would need to test every memory client in every power domain to prove this. So I don't think that is a trivial thing to do. Furthermore, looking at what we have done in kernel used for android products (which probably stress PM the most) this is done and so I don't know of any shipping product that stresses PM that does not do this. May be someone else might. I personally would not be comfortable removing this without testing, but as I mentioned it is not a trivial thing to test correctly. However, I will let you and the other maintainers decide what's best here.

* PGP Signed by an unknown key

Quoted 31 lines · Peter De Schrijver

Quoted 29 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Quoted 4 lines

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary.

Quoted 39 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 35 lines · Peter De Schrijver

Quoted 20 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Quoted 9 lines

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

Quoted 3 lines

My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary.

The peripheral driver doesn't know how long a request is queued in the memory controller. So it can't be responsible for ensuring this.

* PGP Signed by an unknown key

Quoted 40 lines · Peter De Schrijver

Quoted 39 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 35 lines · Peter De Schrijver

Quoted 20 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 8 lines · Jon Hunter

The Tegra memory controller implements a flush feature to flush pending accesses and prevent further accesses from occurring. This feature is used when powering down IP blocks to ensure the IP block is in a good state. The flushes are organised by software groups and IP blocks are assigned in hardware to the different software groups. Add helper functions for requesting a handle to an MC flush for a given software group and enabling/disabling the MC flush itself.

Quoted 8 lines

This is based upon a change by Vince Hsu <[email protected]>.

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

---
 drivers/memory/tegra/mc.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/mc.h |   2 +
 include/soc/tegra/mc.h    |  34 ++++++++++++++
 3 files changed, 146 insertions(+)

Quoted 9 lines

Do we know if this is actually necessary? I remember having a discussion with Arnd Bergmann a while ago, and the Linux driver model kind of assumes that by the time a device is disabled all outstanding accesses will have stopped.

Do we have a way to determine that this even makes a difference? Can we trigger a case where not doing this would cause breakage and see that adding this fixes that particular issue?

Most likely it is. The memory controller can still be processing requests when the peripheral domain is powergated. This would mean the response cannot be delivered in that case. So we need to be sure there are no outstanding requests before shutting down the domain.

Quoted 6 lines

Quoted 3 lines

My point is that that's the driver's responsibility anyway, hence making the explicit flush unnecessary.

The peripheral driver doesn't know how long a request is queued in the memory controller. So it can't be responsible for ensuring this.

Surely whenever the peripheral reports that the operation is done (be that via some DMA controller interrupt or syncpoint increment) the operation would have flushed from the memory controller. Drivers are already supposed to make sure this is the case when they are removed or suspended, so this would mean that we'd be adding all this code for no real gain.

It would also explain why we're currently not seeing any such problems.

From: Vince Hsu <[email protected]>

This patch adds the hot reset register table and flush related callback functions for Tegra30.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Removed tegra_mc_ops and added
metastable_flush_reads.]

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

---
v3: removal of tegra_mc_ops

---
 drivers/memory/tegra/tegra30.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/memory/tegra/tegra30.c +0 -0

index 1abcd8f6f3ba..3b4987f39b52 100644

drivers/memory/tegra/tegra30.c +7 -0

+	{TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
+	{TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
+	{TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
+	{TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
+	{TEGRA_SWGROUP_EPP,        0x200, 0x204,  4},
+	{TEGRA_SWGROUP_G2,         0x200, 0x204,  5},
+	{TEGRA_SWGROUP_HC,         0x200, 0x204,  6},

* PGP Signed by an unknown key

Quoted 24 lines · Jon Hunter

From: Vince Hsu <[email protected]>

This patch adds the hot reset register table and flush related callback functions for Tegra30.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Removed tegra_mc_ops and added
metastable_flush_reads.]

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

---
v3: removal of tegra_mc_ops

---
 drivers/memory/tegra/tegra30.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

drivers/memory/tegra/tegra30.c +0 -0

index 1abcd8f6f3ba..3b4987f39b52 100644
@@ -6,6 +6,7 @@
  * published by the Free Software Foundation.
  */
 

drivers/memory/tegra/tegra30.c line 9

Quoted 1 line

drivers/memory/tegra/tegra30.c +1 -0

+#include <linux/device.h>

What do you need this for?

drivers/memory/tegra/tegra30.c static const struct tegra_smmu_swgroup tegra30_swgroups[] = { · line 940

Quoted 8 lines

drivers/memory/tegra/tegra30.c +1 -0

 #include <linux/of.h>
 #include <linux/mm.h>
 

@@ -936,6 +937,26 @@ static const struct tegra_smmu_swgroup tegra30_swgroups[] = {
 	{ .name = "isp",  .swgroup = TEGRA_SWGROUP_ISP,  .reg = 0x258 },
 };
 
+static struct tegra_mc_flush tegra30_mc_flush[] = {

const

Quoted 11 lines

drivers/memory/tegra/tegra30.c +11 -0

+	{TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
+	{TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
+	{TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
+	{TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
+	{TEGRA_SWGROUP_EPP,        0x200, 0x204,  4},
+	{TEGRA_SWGROUP_G2,         0x200, 0x204,  5},
+	{TEGRA_SWGROUP_HC,         0x200, 0x204,  6},
+	{TEGRA_SWGROUP_HDA,        0x200, 0x204,  7},
+	{TEGRA_SWGROUP_ISP,        0x200, 0x204,  8},
+	{TEGRA_SWGROUP_MPCORE,     0x200, 0x204,  9},
+	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x204, 10},

drivers/memory/tegra/tegra30.c static const struct tegra_smmu_swgroup tegra30_swgroups[] = { · line 957

Quoted 6 lines

drivers/memory/tegra/tegra30.c +6 -0

+	{TEGRA_SWGROUP_MPE,        0x200, 0x204, 11},
+	{TEGRA_SWGROUP_NV,         0x200, 0x204, 12},
+	{TEGRA_SWGROUP_NV2,        0x200, 0x204, 13},
+	{TEGRA_SWGROUP_PPCS,       0x200, 0x204, 14},
+	{TEGRA_SWGROUP_VDE,        0x200, 0x204, 16},
+	{TEGRA_SWGROUP_VI,         0x200, 0x204, 17},

Spaces around { and }, please.

From: Vince Hsu <[email protected]>

This patch adds the hot reset register table and flush related callback functions for Tegra114.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Removed tegra_mc_ops and added
metastable_flush_reads.]

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

---
 drivers/memory/tegra/tegra114.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

drivers/memory/tegra/tegra114.c +0 -0

index 9f579589e800..ba33c402ed68 100644

drivers/memory/tegra/tegra114.c +6 -0

+	{TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
+	{TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
+	{TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
+	{TEGRA_SWGROUP_EPP,        0x200, 0x204,  4},
+	{TEGRA_SWGROUP_G2,         0x200, 0x204,  5},
+	{TEGRA_SWGROUP_HC,         0x200, 0x204,  6},

From: Vince Hsu <[email protected]>

This patch adds the hot reset register table and flush related callback functions for Tegra124.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Removed tegra_mc_ops and added
metastable_flush_reads.]

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

---
 drivers/memory/tegra/tegra124.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

From: Vince Hsu <[email protected]>

Tegra114 has a HW bug that the PLLD/PLLD2 lock bit cannot be asserted when the DIS power domain is during up-powergating process but the clamp to this

domain is not removed yet. That causes a timeout and aborts the power sequence, although the PLLD/PLLD2 has already locked. To remove the false alarm, we don't use the lock for PLLD/PLLD2. Just wait 1ms and treat the clocks as locked.

Signed-off-by Vince Hsu <[email protected]>

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

---
 drivers/clk/tegra/clk-tegra114.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/clk/tegra/clk-tegra114.c +2 -4

index 8237d16b4075..2e5c20c7c088 100644
@@ -456,8 +456,7 @@ static struct tegra_clk_pll_params pll_d_params = {
 	.lock_delay = 1000,
 	.div_nmp = &pllp_nmp,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-		 TEGRA_PLL_USE_LOCK,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
 };
 
 static struct tegra_clk_pll_params pll_d2_params = {

@@ -474,8 +473,7 @@ static struct tegra_clk_pll_params pll_d2_params = {
 	.lock_delay = 1000,
 	.div_nmp = &pllp_nmp,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-		 TEGRA_PLL_USE_LOCK,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
 };
 
 static struct pdiv_map pllu_p[] = {

Signature · 1 line

2.1.4

Quoted 4 lines · Jon Hunter

From: Vince Hsu <[email protected]>

Tegra114 has a HW bug that the PLLD/PLLD2 lock bit cannot be asserted when the DIS power domain is during up-powergating process but the clamp to this

I think there is missing 'off' in this sentence?

ie. ... 'the DIS power domain is off during up-powergating process'

Also 'un-powergating sequence' would be nicer.

Quoted 6 lines · Peter De Schrijver

Quoted 4 lines · Jon Hunter

From: Vince Hsu <[email protected]>

Tegra114 has a HW bug that the PLLD/PLLD2 lock bit cannot be asserted when the DIS power domain is during up-powergating process but the clamp to this

Quoted 5 lines

I think there is missing 'off' in this sentence?

ie. ... 'the DIS power domain is off during up-powergating process'

Also 'un-powergating sequence' would be nicer.

Yes agree. I will re-word that.

From d5cbecd4e97332cd8373b9c4893731eb8e063660 Mon Sep 17 00:00:00 2001

Quoted 9 lines · Jon Hunter

Quoted 6 lines · Peter De Schrijver

Quoted 4 lines · Jon Hunter

From: Vince Hsu <[email protected]>

Tegra114 has a HW bug that the PLLD/PLLD2 lock bit cannot be asserted when the DIS power domain is during up-powergating process but the clamp to this

Quoted 7 lines

Quoted 5 lines

I think there is missing 'off' in this sentence?

ie. ... 'the DIS power domain is off during up-powergating process'

Also 'un-powergating sequence' would be nicer.

Yes agree. I will re-word that.

Updated version ...

From d5cbecd4e97332cd8373b9c4893731eb8e063660 Mon Sep 17 00:00:00 2001
From: Vince Hsu <[email protected]>
Date: Wed, 11 Mar 2015 16:46:04 +0800
Subject: [PATCH 06/19] clk: tegra: remove TEGRA_PLL_USE_LOCK for PLLD/PLLD2

Tegra114 has a HW bug where the PLLD/PLLD2 lock bit cannot be asserted while turning on the Display power domain and before the clamp to this domain has been removed. This issue causes a timeout and aborts the power up sequence, even though the PLLD/PLLD2 has already locked. To avoid this, don't use the lock for PLLD/PLLD2, just wait 1ms and treat the clocks as locked.

Signed-off-by Vince Hsu <[email protected]>

[jonathanh at nvidia.com: Updated the changelog description]

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

---
 drivers/clk/tegra/clk-tegra114.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/clk/tegra/clk-tegra114.c +2 -4

index 8237d16b4075..2e5c20c7c088 100644
@@ -456,8 +456,7 @@ static struct tegra_clk_pll_params pll_d_params = {
 	.lock_delay = 1000,
 	.div_nmp = &pllp_nmp,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-		 TEGRA_PLL_USE_LOCK,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
 };
 
 static struct tegra_clk_pll_params pll_d2_params = {

@@ -474,8 +473,7 @@ static struct tegra_clk_pll_params pll_d2_params = {
 	.lock_delay = 1000,
 	.div_nmp = &pllp_nmp,
 	.freq_table = pll_d_freq_table,
-	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-		 TEGRA_PLL_USE_LOCK,
+	.flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
 };
 
 static struct pdiv_map pllu_p[] = {

Signature · 1 line

2.1.4

Currently, the function tegra_powergate_set() simply sets the desired powergate state but does not wait for the state to change. In some circumstances this can be desirable. However, in most cases we should wait for the state to change before proceeding. Therefore, add a parameter to tegra_powergate_set() to indicate whether we should wait for the state to change.

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

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

---
 arch/arm/mach-tegra/platsmp.c | 18 ++++--------------
 drivers/soc/tegra/pmc.c       | 29 +++++++++++++++++++++++------
 include/soc/tegra/pmc.h       |  2 +-
 3 files changed, 28 insertions(+), 21 deletions(-)

arch/arm/mach-tegra/platsmp.c +4 -14

index b45086666648..13982b5936c0 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, true);
+	if (ret)
+		return ret;
 
 remove_clamps:
 	/* CPU partition is powered. Enable the CPU clock. */

@@ -162,7 +152,7 @@ static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		 * also initial power state in flow controller. After that,
 		 * the CPU's power state is maintained by flow controller.
 		 */
-		ret = tegra_pmc_cpu_power_on(cpu);
+		ret = tegra_pmc_cpu_power_on(cpu, false);
 	}
 
 	return ret;

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

index 300f11e0c3bb..c0635bdd4ee3 100644

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

 {
+	unsigned long timeout;
 	bool status;
+	int ret = 0;
 
 	mutex_lock(&pmc->powergates_lock);
 

@@ -190,9 +192,23 @@ static int tegra_powergate_set(int id, bool new_state)
 
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
+	if (wait) {
+		timeout = jiffies + msecs_to_jiffies(100);
+		ret = -ETIMEDOUT;
+
+		while (time_before(jiffies, timeout)) {
+			status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
+			if (status == new_state) {
+				ret = 0;
+				break;
+			}
+			udelay(10);
+		}
+	}
+
 	mutex_unlock(&pmc->powergates_lock);
 
-	return 0;
+	return ret;
 }
 
 /**

@@ -204,7 +220,7 @@ int tegra_powergate_power_on(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	return tegra_powergate_set(id, true);
+	return tegra_powergate_set(id, true, true);
 }
 
 /**

@@ -216,7 +232,7 @@ int tegra_powergate_power_off(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	return tegra_powergate_set(id, false);
+	return tegra_powergate_set(id, false, true);
 }
 EXPORT_SYMBOL(tegra_powergate_power_off);
 

* PGP Signed by an unknown key

Quoted 58 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 some circumstances this can be desirable. However, in most cases we should wait for the state to change before proceeding. Therefore, add a parameter to tegra_powergate_set() to indicate whether we should wait for the state to change.

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

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

---
 arch/arm/mach-tegra/platsmp.c | 18 ++++--------------
 drivers/soc/tegra/pmc.c       | 29 +++++++++++++++++++++++------
 include/soc/tegra/pmc.h       |  2 +-
 3 files changed, 28 insertions(+), 21 deletions(-)

arch/arm/mach-tegra/platsmp.c +4 -14

index b45086666648..13982b5936c0 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, true);
+	if (ret)
+		return ret;
 
 remove_clamps:
 	/* CPU partition is powered. Enable the CPU clock. */

@@ -162,7 +152,7 @@ static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		 * also initial power state in flow controller. After that,
 		 * the CPU's power state is maintained by flow controller.
 		 */
-		ret = tegra_pmc_cpu_power_on(cpu);
+		ret = tegra_pmc_cpu_power_on(cpu, false);
 	}
 
 	return ret;

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

index 300f11e0c3bb..c0635bdd4ee3 100644

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

Quoted 2 lines

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

@@ -175,9 +175,11 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  * @id: partition ID
  * @new_state: new state of the partition

The comment here isn't updated.

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

Quoted 3 lines

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

  */
-static int tegra_powergate_set(int id, bool new_state)
+static int tegra_powergate_set(int id, bool new_state, bool wait)

Can we please not chain boolean parameters, it makes the call sites impossible to parse for humans. Instead, can we simply leave tegra_powergate_set() as it is and add another function, such as tegra_powergate_set_sync() or tegra_powergate_set_and_wait(), to achieve the same? You may have to split out a tegra_powergate_set_unlocked() or similar to make sure you get to keep the lock across both operations:

	static int tegra_powergate_set_unlocked(int id, bool new_state)
	{
		...
	}

	static int tegra_powergate_set(int id, bool new_state)
	{
		int err;

		mutex_lock(&pmc->powergates_lock);
		err = tegra_powergate_set_unlocked(id, new_state);
		mutex_unlock(&pmc->powergates_lock);

		return err;
	}

	/*
	 * Must be called with pmc->powergates_lock mutex held.
	 */
	static int tegra_powergate_wait(int id, bool new_state)
	{
		...
	}

	static int tegra_powergate_set_and_wait(int id, bool new_state)
	{
		int err;

		mutex_lock(&pmc->powergates_lock);

		err = tegra_powergate_set_unlocked(id, new_state);
		if (err < 0)
			goto unlock;

		err = tegra_powergate_wait(id, new_state);
		if (err < 0)
			goto unlock;

	unlock:
		mutex_unlock(&pmc->powergates_lock);
		return err;
	}

Quoted 50 lines

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

 {
+	unsigned long timeout;
 	bool status;
+	int ret = 0;
 
 	mutex_lock(&pmc->powergates_lock);
 

@@ -190,9 +192,23 @@ static int tegra_powergate_set(int id, bool new_state)
 
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
+	if (wait) {
+		timeout = jiffies + msecs_to_jiffies(100);
+		ret = -ETIMEDOUT;
+
+		while (time_before(jiffies, timeout)) {
+			status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
+			if (status == new_state) {
+				ret = 0;
+				break;
+			}
+			udelay(10);
+		}
+	}
+
 	mutex_unlock(&pmc->powergates_lock);
 
-	return 0;
+	return ret;
 }
 
 /**

@@ -204,7 +220,7 @@ int tegra_powergate_power_on(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	return tegra_powergate_set(id, true);
+	return tegra_powergate_set(id, true, true);
 }
 
 /**

@@ -216,7 +232,7 @@ int tegra_powergate_power_off(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	return tegra_powergate_set(id, false);
+	return tegra_powergate_set(id, false, true);
 }
 EXPORT_SYMBOL(tegra_powergate_power_off);
 

drivers/soc/tegra/pmc.c bool tegra_pmc_cpu_is_powered(int cpuid) · line 372

Quoted 8 lines

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

@@ -351,8 +367,9 @@ bool tegra_pmc_cpu_is_powered(int cpuid)
 /**
  * tegra_pmc_cpu_power_on() - power on CPU partition
  * @cpuid: CPU partition ID
+ * @wait:  Wait for CPU state to transition
  */
-int tegra_pmc_cpu_power_on(int cpuid)
+int tegra_pmc_cpu_power_on(int cpuid, bool wait)

This one is probably fine since it's the only boolean parameter so far. That said, I see that we call this exactly twice, so I wonder if there'd be any harm in having the second occurrence wait as well and hence get rid of the parameter.

The following clean-ups have been made to the PMC code:

1. Add a macro for determining the state of a PMC powergate
2. Use the readx_poll_timeout() function instead of implementing a local polling loop with a timeout.
3. Use a case-statement in the function that removes the powergate clamp instead of multiple if-statements to improve readability.

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

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

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

index c0635bdd4ee3..180d434deec5 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>

@@ -56,6 +57,8 @@
 #define  PWRGATE_TOGGLE_START		(1 << 8)
 
 #define REMOVE_CLAMPING			0x34
+#define  REMOVE_CLAMPING_VDEC		(1 << 3)
+#define  REMOVE_CLAMPING_PCIE		(1 << 4)
 
 #define PWRGATE_STATUS			0x38
 

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

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

@@ -177,15 +182,14 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  */
 static int tegra_powergate_set(int id, bool new_state, bool wait)
 {
-	unsigned long timeout;
-	bool status;
 	int ret = 0;
+	u32 val;
 
 	mutex_lock(&pmc->powergates_lock);
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	val = tegra_pmc_readl(PWRGATE_STATUS);
 
-	if (status == new_state) {
+	if (PMC_PWRGATE_STATE(val, id) == new_state) {
 		mutex_unlock(&pmc->powergates_lock);
 		return 0;
 	}

@@ -193,17 +197,9 @@ static int tegra_powergate_set(int id, bool new_state, bool wait)
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
 	if (wait) {
-		timeout = jiffies + msecs_to_jiffies(100);
-		ret = -ETIMEDOUT;
-
-		while (time_before(jiffies, timeout)) {
-			status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
-			if (status == new_state) {
-				ret = 0;
-				break;
-			}
-			udelay(10);
-		}
+		ret = readx_poll_timeout(tegra_pmc_readl, PWRGATE_STATUS,
+				val, PMC_PWRGATE_STATE(val, id) == new_state,
+				10, 100000);
 	}
 
 	mutex_unlock(&pmc->powergates_lock);

@@ -242,13 +238,10 @@ EXPORT_SYMBOL(tegra_powergate_power_off);
  */

drivers/soc/tegra/pmc.c +21 -8

 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	/*
-	 * On Tegra124 and later, the clamps for the GPU are controlled by a
-	 * separate register (with different semantics).
+	 * In most cases the bit for removing the IO clamping is the same as
+	 * the powergate partition ID, however, this is not always the case.
+	 * Furthermore, on Tegra124 and later, the clamps for the GPU are
+	 * controlled by a separate register (with different semantics). The
+	 * following case-statement handles these exceptions.
 	 */
-	if (id == TEGRA_POWERGATE_3D) {
+	val = 1 << id;
+	reg = REMOVE_CLAMPING;
+
+	switch (id) {
+	case TEGRA_POWERGATE_3D:
 		if (pmc->soc->has_gpu_clamps) {
-			tegra_pmc_writel(0, GPU_RG_CNTRL);
-			return 0;
+			val = 0;
+			reg  = GPU_RG_CNTRL;
 		}
+		break;
+	case TEGRA_POWERGATE_VDEC:
+		val = REMOVE_CLAMPING_VDEC;
+		break;
+	case TEGRA_POWERGATE_PCIE:
+		val = REMOVE_CLAMPING_PCIE;
+		break;
+	default:
+		break;
 	}
 
-	/*
-	 * Tegra 2 has a bug where PCIE and VDE clamping masks are
-	 * swapped relatively to the partition ids

* PGP Signed by an unknown key

Quoted 35 lines · Jon Hunter

The following clean-ups have been made to the PMC code:

1. Add a macro for determining the state of a PMC powergate
2. Use the readx_poll_timeout() function instead of implementing a local polling loop with a timeout.
3. Use a case-statement in the function that removes the powergate clamp instead of multiple if-statements to improve readability.

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

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

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

index c0635bdd4ee3..180d434deec5 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>

@@ -56,6 +57,8 @@
 #define  PWRGATE_TOGGLE_START		(1 << 8)
 
 #define REMOVE_CLAMPING			0x34
+#define  REMOVE_CLAMPING_VDEC		(1 << 3)
+#define  REMOVE_CLAMPING_PCIE		(1 << 4)
 
 #define PWRGATE_STATUS			0x38
 

drivers/soc/tegra/pmc.c line 107

Quoted 4 lines

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

@@ -101,6 +104,8 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+#define PMC_PWRGATE_STATE(val, id)	(!!(val & BIT(id)))

I find double-negations very difficult to read. ((value & BIT(id)) != 0) is clearer in my opinion. Also I'd suggest status or value as the first parameter name, for consistency with other parts of the driver.

Quoted 46 lines

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

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

@@ -177,15 +182,14 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
  */
 static int tegra_powergate_set(int id, bool new_state, bool wait)
 {
-	unsigned long timeout;
-	bool status;
 	int ret = 0;
+	u32 val;
 
 	mutex_lock(&pmc->powergates_lock);
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	val = tegra_pmc_readl(PWRGATE_STATUS);
 
-	if (status == new_state) {
+	if (PMC_PWRGATE_STATE(val, id) == new_state) {
 		mutex_unlock(&pmc->powergates_lock);
 		return 0;
 	}

@@ -193,17 +197,9 @@ static int tegra_powergate_set(int id, bool new_state, bool wait)
 	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
 
 	if (wait) {
-		timeout = jiffies + msecs_to_jiffies(100);
-		ret = -ETIMEDOUT;
-
-		while (time_before(jiffies, timeout)) {
-			status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
-			if (status == new_state) {
-				ret = 0;
-				break;
-			}
-			udelay(10);
-		}
+		ret = readx_poll_timeout(tegra_pmc_readl, PWRGATE_STATUS,
+				val, PMC_PWRGATE_STATE(val, id) == new_state,
+				10, 100000);
 	}
 
 	mutex_unlock(&pmc->powergates_lock);

@@ -242,13 +238,10 @@ EXPORT_SYMBOL(tegra_powergate_power_off);
  */

drivers/soc/tegra/pmc.c EXPORT_SYMBOL(tegra_powergate_power_off); · line 244

Quoted 10 lines

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

 int tegra_powergate_is_powered(int id)
 {
-	u32 status;
-
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
-	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
-	return !!status;
+	return PMC_PWRGATE_STATE(tegra_pmc_readl(PWRGATE_STATUS), id);

I'd prefer to keep the two steps here. As a general rule I try never to mix reading or writing a register with other code on the same line.

drivers/soc/tegra/pmc.c int tegra_powergate_is_powered(int id) · line 253

Quoted 9 lines

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

 }
 
 /**

@@ -257,34 +250,39 @@ int tegra_powergate_is_powered(int id)
  */
 int tegra_powergate_remove_clamping(int id)
 {
-	u32 mask;
+	u32 val, reg;

Side note: Please use value instead of val since that's the form used elsewhere in the driver. Also reg would be more appropriately called offset or similar because that's what it really is. It would also be more naturally an unsized type, such as unsigned int. But...

Quoted 36 lines

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

 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
 	/*
-	 * On Tegra124 and later, the clamps for the GPU are controlled by a
-	 * separate register (with different semantics).
+	 * In most cases the bit for removing the IO clamping is the same as
+	 * the powergate partition ID, however, this is not always the case.
+	 * Furthermore, on Tegra124 and later, the clamps for the GPU are
+	 * controlled by a separate register (with different semantics). The
+	 * following case-statement handles these exceptions.
 	 */
-	if (id == TEGRA_POWERGATE_3D) {
+	val = 1 << id;
+	reg = REMOVE_CLAMPING;
+
+	switch (id) {
+	case TEGRA_POWERGATE_3D:
 		if (pmc->soc->has_gpu_clamps) {
-			tegra_pmc_writel(0, GPU_RG_CNTRL);
-			return 0;
+			val = 0;
+			reg  = GPU_RG_CNTRL;
 		}
+		break;
+	case TEGRA_POWERGATE_VDEC:
+		val = REMOVE_CLAMPING_VDEC;
+		break;
+	case TEGRA_POWERGATE_PCIE:
+		val = REMOVE_CLAMPING_PCIE;
+		break;
+	default:
+		break;
 	}
 

drivers/soc/tegra/pmc.c int tegra_powergate_is_powered(int id) · line 285

Quoted 13 lines

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

-	/*
-	 * Tegra 2 has a bug where PCIE and VDE clamping masks are
-	 * swapped relatively to the partition ids
-	 */
-	if (id == TEGRA_POWERGATE_VDEC)
-		mask = (1 << TEGRA_POWERGATE_PCIE);
-	else if (id == TEGRA_POWERGATE_PCIE)
-		mask = (1 << TEGRA_POWERGATE_VDEC);
-	else
-		mask = (1 << id);
-
-	tegra_pmc_writel(mask, REMOVE_CLAMPING);
+	tegra_pmc_writel(val, reg);

... to be honest, I think the previous code was more straightforward, so I'd prefer if you dropped this particular hunk.

Add support to the tegra dc driver for generic PM domains. However, to ensure backward compatibility with older device tree blobs ensure that the driver can work with or without generic PM domains. In order to migrate to generic PM domain infrastructure the necessary changes are:

1. If the "power-domains" property is present in the DT device node then generic PM domains is supported and pm_runtime_enable() should be called for the device. Furthermore, the enabling and disabling of the power-domain is handled via calling pm_runtime_get/put, respectively.

2. To ensure that clocks are managed consistently when generic PM domains are used and are not used, drivers should be migrated to use the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() functions instead of the current tegra_powergate_sequence_power_up() and tegra_powergate_power_off(). The purpose of the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() APIs is to mimick the behaviour of the tegra generic power-domain code, such that if generic power domains are not supported the functionality is the same.

3. The main difference between the tegra_powergate_sequence_power_up() API and the tegra_powergate_power_on_legacy() is that the clock used to enable the powergate is not kept enabled when using the tegra_powergate_power_on_legacy() API. Therefore, drivers must enable the clocks they need after calling tegra_powergate_power_on_legacy() and disable these clocks before calling tegra_powergate_power_off_legacy().

* PGP Signed by an unknown key

Quoted 6 lines · Jon Hunter

Add support to the tegra dc driver for generic PM domains. However, to ensure backward compatibility with older device tree blobs ensure that the driver can work with or without generic PM domains. In order to migrate to generic PM domain infrastructure the necessary changes are:

Quoted 4 lines

1. If the "power-domains" property is present in the DT device node then generic PM domains is supported and pm_runtime_enable() should be called for the device. Furthermore, the enabling and disabling of the power-domain is handled via calling pm_runtime_get/put, respectively.

The device could be PM runtime enabled even if no power-domains property is set. Couldn't we check something more direct, like for example if the dev->pm_domain is non-NULL?

Perhaps it'd be worth converting the driver to use runtime PM first, and move all the powergate and clock handling into suspend/resume functions, and then we can conditionalize whether or not we call the legacy API iff dev->pm_domain == NULL?

Quoted 10 lines

2. To ensure that clocks are managed consistently when generic PM domains are used and are not used, drivers should be migrated to use the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() functions instead of the current tegra_powergate_sequence_power_up() and tegra_powergate_power_off(). The purpose of the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() APIs is to mimick the behaviour of the tegra generic power-domain code, such that if generic power domains are not supported the functionality is the same.

Quoted 7 lines

3. The main difference between the tegra_powergate_sequence_power_up() API and the tegra_powergate_power_on_legacy() is that the clock used to enable the powergate is not kept enabled when using the tegra_powergate_power_on_legacy() API. Therefore, drivers must enable the clocks they need after calling tegra_powergate_power_on_legacy() and disable these clocks before calling tegra_powergate_power_off_legacy().

This seems like it should go into the previous patch. I'm not sure I understand why this is necessary. Wouldn't it be easier to update the drivers to properly cope with this? We'll need to move them to runtime PM at some point anyway, so if we do that first, we should be able to hide all these details within the driver's suspend/resume functions but without the need for the API churn here.

Quoted 9 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 6 lines · Jon Hunter

Add support to the tegra dc driver for generic PM domains. However, to ensure backward compatibility with older device tree blobs ensure that the driver can work with or without generic PM domains. In order to migrate to generic PM domain infrastructure the necessary changes are:

Quoted 8 lines

Quoted 4 lines

1. If the "power-domains" property is present in the DT device node then generic PM domains is supported and pm_runtime_enable() should be called for the device. Furthermore, the enabling and disabling of the power-domain is handled via calling pm_runtime_get/put, respectively.

The device could be PM runtime enabled even if no power-domains property is set. Couldn't we check something more direct, like for example if the dev->pm_domain is non-NULL?

Yes could do that instead.

Quoted 4 lines

Perhaps it'd be worth converting the driver to use runtime PM first, and move all the powergate and clock handling into suspend/resume functions, and then we can conditionalize whether or not we call the legacy API iff dev->pm_domain == NULL?

May be that would be a cleaner transition than trying to do it all in one go.

Quoted 2 lines · Jon Hunter

May be that would be a cleaner transition than trying to do it all in one go.

I have a couple of patches in my tree to do this for DRM as part of an effort to restore DPMS. It's fairly tricky to get right in DRM and requires all sorts of changes to the driver.

Add support to the tegra PCI driver for generic PM domains. However, to ensure backward compatibility with older device tree blobs ensure that the driver can work with or without generic PM domains. In order to migrate to generic PM domain infrastructure the necessary changes are:

1. If the "power-domains" property is present in the DT device node then generic PM domains is supported and pm_runtime_enable() should be called for the device. Furthermore, the enabling and disabling of the power-domain is handled via calling pm_runtime_get/put, respectively.

2. To ensure that clocks are managed consistently when generic PM domains are used and are not used, drivers should be migrated to use the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() functions instead of the current tegra_powergate_sequence_power_up() and tegra_powergate_power_off(). The purpose of the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() APIs is to mimick the behaviour of the tegra generic power-domain code, such that if generic power domains are not supported the functionality is the same.

3. The main difference between the tegra_powergate_sequence_power_up() API and the tegra_powergate_power_on_legacy() is that the clock used to enable the powergate is not kept enabled when using the tegra_powergate_power_on_legacy() API. Therefore, drivers must enable the clocks they need after calling tegra_powergate_power_on_legacy() and disable these clocks before calling tegra_powergate_power_off_legacy().

The helper functions for handling the powering on and off of the PCI controller have been updated to support generic PM domains. The tegra_pcie_power_off() was missing code to disable the clocks enabled in the tegra_pcie_power_on() function and so this has been added.

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

---
 drivers/pci/host/pci-tegra.c | 49 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 12 deletions(-)

drivers/pci/host/pci-tegra.c +27 -12

index 10c05718dbfd..acd1f311eee5 100644
@@ -40,6 +40,7 @@
 #include <linux/pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>

@@ -908,19 +909,32 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 
 static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 {
+	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	int err;
 
-	/* TODO: disable and unprepare clocks? */
-
 	err = phy_power_off(pcie->phy);
 	if (err < 0)
 		dev_warn(pcie->dev, "failed to power off PHY: %d\n", err);
 
 	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
 
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
+	clk_disable_unprepare(pcie->pll_e);
+	if (soc->has_cml_clk)
+		clk_disable_unprepare(pcie->cml_clk);
+	clk_disable_unprepare(pcie->afi_clk);
+	clk_disable_unprepare(pcie->pex_clk);
+
+	if (pm_runtime_enabled(pcie->dev)) {
+		err = pm_runtime_put_sync(pcie->dev);
+	} else {
+		err = tegra_powergate_power_off_legacy(TEGRA_POWERGATE_PCIE,
+						       pcie->pex_clk,
+						       pcie->pex_rst);
+	}
+
+	if (err < 0)
+		dev_warn(pcie->dev, "powergate power-down failed: %d\n", err);
 
 	err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)

@@ -934,20 +948,28 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 
 	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
-
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 
 	/* enable regulators */
 	err = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)
 		dev_err(pcie->dev, "failed to enable regulators: %d\n", err);
 
-	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
-						pcie->pex_clk,
-						pcie->pex_rst);
-	if (err) {
-		dev_err(pcie->dev, "powerup sequence failed: %d\n", err);
+	if (pm_runtime_enabled(pcie->dev)) {
+		err = pm_runtime_get_sync(pcie->dev);
+	} else {
+		err = tegra_powergate_power_on_legacy(TEGRA_POWERGATE_PCIE,
+						      pcie->pex_clk,
+						      pcie->pex_rst);
+	}
+
+	if (err < 0) {

* PGP Signed by an unknown key

Quoted 21 lines · Jon Hunter

Add support to the tegra PCI driver for generic PM domains. However, to ensure backward compatibility with older device tree blobs ensure that the driver can work with or without generic PM domains. In order to migrate to generic PM domain infrastructure the necessary changes are:

1. If the "power-domains" property is present in the DT device node then generic PM domains is supported and pm_runtime_enable() should be called for the device. Furthermore, the enabling and disabling of the power-domain is handled via calling pm_runtime_get/put, respectively.

2. To ensure that clocks are managed consistently when generic PM domains are used and are not used, drivers should be migrated to use the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() functions instead of the current tegra_powergate_sequence_power_up() and tegra_powergate_power_off(). The purpose of the tegra_powergate_power_on_legacy() and tegra_powergate_power_off_legacy() APIs is to mimick the behaviour of the tegra generic power-domain code, such that if generic power domains are not supported the functionality is the same.

Quoted 7 lines

3. The main difference between the tegra_powergate_sequence_power_up() API and the tegra_powergate_power_on_legacy() is that the clock used to enable the powergate is not kept enabled when using the tegra_powergate_power_on_legacy() API. Therefore, drivers must enable the clocks they need after calling tegra_powergate_power_on_legacy() and disable these clocks before calling tegra_powergate_power_off_legacy().

This is the same comment as in the DRM patch, and it applies to all devices that use powergating, so it should move into the preparatory patch rather than be repeated in all patches.

Quoted 93 lines

The helper functions for handling the powering on and off of the PCI controller have been updated to support generic PM domains. The tegra_pcie_power_off() was missing code to disable the clocks enabled in the tegra_pcie_power_on() function and so this has been added.

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

---
 drivers/pci/host/pci-tegra.c | 49 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 12 deletions(-)

drivers/pci/host/pci-tegra.c +33 -12

index 10c05718dbfd..acd1f311eee5 100644
@@ -40,6 +40,7 @@
 #include <linux/pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>

@@ -908,19 +909,32 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 
 static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 {
+	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	int err;
 
-	/* TODO: disable and unprepare clocks? */
-
 	err = phy_power_off(pcie->phy);
 	if (err < 0)
 		dev_warn(pcie->dev, "failed to power off PHY: %d\n", err);
 
 	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
 
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
+	clk_disable_unprepare(pcie->pll_e);
+	if (soc->has_cml_clk)
+		clk_disable_unprepare(pcie->cml_clk);
+	clk_disable_unprepare(pcie->afi_clk);
+	clk_disable_unprepare(pcie->pex_clk);
+
+	if (pm_runtime_enabled(pcie->dev)) {
+		err = pm_runtime_put_sync(pcie->dev);
+	} else {
+		err = tegra_powergate_power_off_legacy(TEGRA_POWERGATE_PCIE,
+						       pcie->pex_clk,
+						       pcie->pex_rst);
+	}
+
+	if (err < 0)
+		dev_warn(pcie->dev, "powergate power-down failed: %d\n", err);
 
 	err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)

@@ -934,20 +948,28 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 
 	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
-
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 
 	/* enable regulators */
 	err = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)
 		dev_err(pcie->dev, "failed to enable regulators: %d\n", err);
 
-	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
-						pcie->pex_clk,
-						pcie->pex_rst);
-	if (err) {
-		dev_err(pcie->dev, "powerup sequence failed: %d\n", err);
+	if (pm_runtime_enabled(pcie->dev)) {
+		err = pm_runtime_get_sync(pcie->dev);
+	} else {
+		err = tegra_powergate_power_on_legacy(TEGRA_POWERGATE_PCIE,
+						      pcie->pex_clk,
+						      pcie->pex_rst);
+	}
+
+	if (err < 0) {
+		dev_err(pcie->dev, "powergate power-up failed: %d\n", err);
+		return err;
+	}
+
+	err = clk_prepare_enable(pcie->pex_clk);
+	if (err < 0) {

drivers/pci/host/pci-tegra.c static int tegra_pcie_power_on(struct tegra_pcie *pcie) · line 975

Quoted 4 lines

drivers/pci/host/pci-tegra.c +1 -0

+		dev_err(pcie->dev, "failed to enable PEX clock: %d\n", err);
 		return err;
 	}
 

Couldn't we simply move the above code into suspend/resume functions and then call pm_runtime_get() and pm_runtime_put() instead of tegra_pcie_power_on() and tegra_pcie_power_off()?

drivers/pci/host/pci-tegra.c static int tegra_pcie_probe(struct platform_device *pdev) · line 2027

Quoted 6 lines

drivers/pci/host/pci-tegra.c +2 -0

@@ -2001,6 +2023,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 
 	pcibios_min_mem = 0;
 
+	if (of_property_read_bool(pdev->dev.of_node, "power-domains"))
+		pm_runtime_enable(&pdev->dev);

Then of course we'd have to call pm_runtime_enable() irrespective of the property and check for the presence of power domain support via another method (like the dev->pm_domain field).

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      | 99 ++++++++++++++++++++++
 1 file changed, 99 insertions(+)

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

index 02c27004d4a8..93357a450576 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.

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

+
 Example:
 
 / SoC dts including file

@@ -113,3 +119,96 @@ 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: