[PATCH 0/8] soc: tegra: pmc: Various fixes

[PATCH 0/8] soc: tegra: pmc: Various fixes

[Jon Hunter]

This series contains a few fixes on the Tegra PMC driver. Some of these
patches were original included as part of the series to add generic PM
domain support for Tegra [0]. However, to keep these patch series a
manageable size, I have separate them in to this series and I have added
a couple more fixes as well.

[0] http://marc.info/?l=linux-tegra&m=145399885003830&w=2

Jon Hunter (8):
  soc: tegra: pmc: Remove non-existing L2 partition for Tegra124
  soc: tegra: pmc: Restore base address on probe failure
  soc: tegra: pmc: Protect public functions from potential race
    conditions
  soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type
  soc: tegra: pmc: Fix testing of powergate state
  soc: tegra: pmc: Fix verification of valid partitions
  soc: tegra: pmc: Remove additional check for a valid partition
  soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

 drivers/gpu/drm/tegra/drm.h |   2 +-
 drivers/soc/tegra/pmc.c     | 131 +++++++++++++++++++++++++++++---------------
 include/soc/tegra/pmc.h     |  35 ++++++------
 3 files changed, 105 insertions(+), 63 deletions(-)

-- 
2.1.4

[PATCH 1/8] soc: tegra: pmc: Remove non-existing L2 partition for Tegra124

[Jon Hunter]

Tegra124 does not have an L2 power partition and the L2 cache is part of
the cluster 0 non-CPU (CONC) partition. Remove the L2 as a valid
partition for Tegra124. The TRM also shows that there is no L2 partition
for Tegra124.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 88c7e506177e..922430322877 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -967,7 +967,6 @@ static const char * const tegra124_powergates[] = {
 	[TEGRA_POWERGATE_VENC] = "venc",
 	[TEGRA_POWERGATE_PCIE] = "pcie",
 	[TEGRA_POWERGATE_VDEC] = "vdec",
-	[TEGRA_POWERGATE_L2] = "l2",
 	[TEGRA_POWERGATE_MPE] = "mpe",
 	[TEGRA_POWERGATE_HEG] = "heg",
 	[TEGRA_POWERGATE_SATA] = "sata",
-- 
2.1.4

[PATCH 2/8] soc: tegra: pmc: Restore base address on probe failure

[Jon Hunter]

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

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

Fix this by updating the mapping and freeing the original mapping only if
probing the PMC device is successful.

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

Please note that in order to properly fix this, we should lock around the
update on the base address pointer. The 3rd patch of this series adds the
locking and with the locking in place we can then also drop this "tmp"
variable added here.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 922430322877..900f72f0d757 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -807,7 +807,7 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base = pmc->base;
+	void __iomem *base, *tmp;
 	struct resource *res;
 	int err;
 
@@ -817,11 +817,9 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	/* take over the memory region from the early initialization */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pmc->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(pmc->base))
-		return PTR_ERR(pmc->base);
-
-	iounmap(base);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
 	pmc->clk = devm_clk_get(&pdev->dev, "pclk");
 	if (IS_ERR(pmc->clk)) {
@@ -850,6 +848,10 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	tmp = pmc->base;
+	pmc->base = base;
+	iounmap(tmp);
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 3/8] soc: tegra: pmc: Protect public functions from potential race conditions

[Jon Hunter]

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

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

Note that functions tegra_io_rail_prepare()/poll() either return a
negative value on failure or zero on success. Therefore, it is not
necessary to check if the return value is less than zero and so only
test that the return value is not zero to test for failure. This
simplifies the error handling with the mutex locking in place.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 900f72f0d757..5449d1aa14e8 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -235,7 +235,10 @@ int tegra_powergate_is_powered(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
+	mutex_lock(&pmc->powergates_lock);
 	status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id);
+	mutex_unlock(&pmc->powergates_lock);
+
 	return !!status;
 }
 
@@ -250,6 +253,8 @@ int tegra_powergate_remove_clamping(int id)
 	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
 		return -EINVAL;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	/*
 	 * On Tegra124 and later, the clamps for the GPU are controlled by a
 	 * separate register (with different semantics).
@@ -257,7 +262,7 @@ int tegra_powergate_remove_clamping(int id)
 	if (id == TEGRA_POWERGATE_3D) {
 		if (pmc->soc->has_gpu_clamps) {
 			tegra_pmc_writel(0, GPU_RG_CNTRL);
-			return 0;
+			goto out;
 		}
 	}
 
@@ -274,6 +279,9 @@ int tegra_powergate_remove_clamping(int id)
 
 	tegra_pmc_writel(mask, REMOVE_CLAMPING);
 
+out:
+	mutex_unlock(&pmc->powergates_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL(tegra_powergate_remove_clamping);
@@ -520,9 +528,11 @@ int tegra_io_rail_power_on(int id)
 	unsigned int bit, mask;
 	int err;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	err = tegra_io_rail_prepare(id, &request, &status, &bit);
-	if (err < 0)
-		return err;
+	if (err)
+		goto error;
 
 	mask = 1 << bit;
 
@@ -533,14 +543,17 @@ int tegra_io_rail_power_on(int id)
 	tegra_pmc_writel(value, request);
 
 	err = tegra_io_rail_poll(status, mask, 0, 250);
-	if (err < 0) {
+	if (err) {
 		pr_info("tegra_io_rail_poll() failed: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	tegra_io_rail_unprepare();
 
-	return 0;
+error:
+	mutex_unlock(&pmc->powergates_lock);
+
+	return err;
 }
 EXPORT_SYMBOL(tegra_io_rail_power_on);
 
@@ -550,10 +563,12 @@ int tegra_io_rail_power_off(int id)
 	unsigned int bit, mask;
 	int err;
 
+	mutex_lock(&pmc->powergates_lock);
+
 	err = tegra_io_rail_prepare(id, &request, &status, &bit);
-	if (err < 0) {
+	if (err) {
 		pr_info("tegra_io_rail_prepare() failed: %d\n", err);
-		return err;
+		goto error;
 	}
 
 	mask = 1 << bit;
@@ -565,12 +580,15 @@ int tegra_io_rail_power_off(int id)
 	tegra_pmc_writel(value, request);
 
 	err = tegra_io_rail_poll(status, mask, mask, 250);
-	if (err < 0)
-		return err;
+	if (err)
+		goto error;
 
 	tegra_io_rail_unprepare();
 
-	return 0;
+error:
+	mutex_unlock(&pmc->powergates_lock);
+
+	return err;
 }
 EXPORT_SYMBOL(tegra_io_rail_power_off);
 
@@ -807,7 +825,7 @@ out:
 
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
-	void __iomem *base, *tmp;
+	void __iomem *base;
 	struct resource *res;
 	int err;
 
@@ -848,9 +866,10 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	tmp = pmc->base;
+	mutex_lock(&pmc->powergates_lock);
+	iounmap(pmc->base);
 	pmc->base = base;
-	iounmap(tmp);
+	mutex_unlock(&pmc->powergates_lock);
 
 	return 0;
 }
-- 
2.1.4

[PATCH 4/8] soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type

[Jon Hunter]

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

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

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

[PATCH 5/8] soc: tegra: pmc: Fix testing of powergate state

[Jon Hunter]

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

After OR'ing the bit for the powergate, check if the result is not equal
to zero before comparing with the boolean value. Add a helper function
to return the current state of a powergate and use this in both
tegra_powergate_set() and tegra_powergate_is_powered() where we check
the powergate status.

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

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

[PATCH 6/8] soc: tegra: pmc: Fix verification of valid partitions

[Jon Hunter]

The tegra power partitions are referenced by a numerical ID which are
the same values programmed into the PMC registers for controlling the
partition. For a given device, the valid partition IDs may not be
contiguous and so simply checking that an ID is not greater than the
maximum ID supported may not mean it is valid. Fix this by checking if
the powergate is defined in the list of powergates for the Tegra SoC.

Add a helper function for checking valid powergates and use where we
need to verify if the powergate ID is valid or not.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 9e8d359baf0e..e75782b47267 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -179,6 +179,11 @@ static inline bool tegra_powergate_state(int id)
 	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
 }
 
+static inline bool tegra_powergate_is_valid(int id)
+{
+	return (pmc->soc && pmc->soc->powergates[id]);
+}
+
 /**
  * tegra_powergate_set() - set the state of a partition
  * @id: partition ID
@@ -206,7 +211,7 @@ static int tegra_powergate_set(unsigned int id, bool new_state)
  */
 int tegra_powergate_power_on(unsigned int id)
 {
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, true);
@@ -218,7 +223,7 @@ int tegra_powergate_power_on(unsigned int id)
  */
 int tegra_powergate_power_off(unsigned int id)
 {
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	return tegra_powergate_set(id, false);
@@ -233,7 +238,7 @@ int tegra_powergate_is_powered(unsigned int id)
 {
 	int status;
 
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
@@ -251,7 +256,7 @@ int tegra_powergate_remove_clamping(unsigned int id)
 {
 	u32 mask;
 
-	if (!pmc->soc || id >= pmc->soc->num_powergates)
+	if (!tegra_powergate_is_valid(id))
 		return -EINVAL;
 
 	mutex_lock(&pmc->powergates_lock);
-- 
2.1.4

[PATCH 7/8] soc: tegra: pmc: Remove additional check for a valid partition

[Jon Hunter]

The function tegra_powergate_is_powered() verifies that the partition
being queried is valid and so there is no need to check this before
calling tegra_powergate_is_powered() in powergate_show(). So remove this
extra check.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index e75782b47267..8e358dbffaed 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -434,16 +434,18 @@ static struct notifier_block tegra_pmc_restart_handler = {
 static int powergate_show(struct seq_file *s, void *data)
 {
 	unsigned int i;
+	int status;
 
 	seq_printf(s, " powergate powered\n");
 	seq_printf(s, "------------------\n");
 
 	for (i = 0; i < pmc->soc->num_powergates; i++) {
-		if (!pmc->soc->powergates[i])
+		status = tegra_powergate_is_powered(i);
+		if (status < 0)
 			continue;
 
 		seq_printf(s, " %9s %7s\n", pmc->soc->powergates[i],
-			   tegra_powergate_is_powered(i) ? "yes" : "no");
+			   status ? "yes" : "no");
 	}
 
 	return 0;
-- 
2.1.4

[PATCH 8/8] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Jon Hunter]

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

Furthermore, because we cannot use the APBDEV_PMC_PWRGATE_STATUS_0
register to determine if the GPU partition is powered, use the
APBDEV_PMC_CLAMP_STATUS_0 register to determine if the GPU partition
is powered. The APBDEV_PMC_CLAMP_STATUS_0 register is bit wise
compatible with the APBDEV_PMC_PWRGATE_STATUS_0 register and if the
clamp is disabled (ie. the appropriate bit == 0), then this indicates
the partition is powered.

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 8e358dbffaed..442232269df3 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -52,6 +52,8 @@
 #define  DPD_SAMPLE_ENABLE		(1 << 0)
 #define  DPD_SAMPLE_DISABLE		(0 << 0)
 
+#define CLAMP_STATUS			0x2c
+
 #define PWRGATE_TOGGLE			0x30
 #define  PWRGATE_TOGGLE_START		(1 << 8)
 
@@ -109,6 +111,7 @@ struct tegra_pmc_soc {
 
 	bool has_tsense_reset;
 	bool has_gpu_clamps;
+	bool has_gpu_toggle;
 };
 
 /**
@@ -176,7 +179,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
 
 static inline bool tegra_powergate_state(int id)
 {
-	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return (tegra_pmc_readl(CLAMP_STATUS) & BIT(id)) == 0;
+	else
+		return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
 }
 
 static inline bool tegra_powergate_is_valid(int id)
@@ -191,6 +197,9 @@ static inline bool tegra_powergate_is_valid(int id)
  */
 static int tegra_powergate_set(unsigned int id, bool new_state)
 {
+	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
+		return -EINVAL;
+
 	mutex_lock(&pmc->powergates_lock);
 
 	if (tegra_powergate_state(id) == new_state) {
@@ -951,6 +960,7 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = {
 	.cpu_powergates = tegra30_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra114_powergates[] = {
@@ -988,6 +998,7 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = {
 	.cpu_powergates = tegra114_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = false,
+	.has_gpu_toggle = true,
 };
 
 static const char * const tegra124_powergates[] = {
@@ -1030,6 +1041,7 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = {
 	.cpu_powergates = tegra124_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const char * const tegra210_powergates[] = {
@@ -1073,6 +1085,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.cpu_powergates = tegra210_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_gpu_toggle = false,
 };
 
 static const struct of_device_id tegra_pmc_match[] = {
-- 
2.1.4

Re: [PATCH 0/8] soc: tegra: pmc: Various fixes

[Jon Hunter]

+Mathieu, sorry I should have copied you. I recall that you had some
comments on this. I guess you are not on linux-tegra, but you can view
the patches here:

http://marc.info/?l=linux-tegra&r=1&b=201602&w=2

Jon

On 11/02/16 18:03, Jon Hunter wrote:
> This series contains a few fixes on the Tegra PMC driver. Some of these
> patches were original included as part of the series to add generic PM
> domain support for Tegra [0]. However, to keep these patch series a
> manageable size, I have separate them in to this series and I have added
> a couple more fixes as well.
> 
> [0] http://marc.info/?l=linux-tegra&m=145399885003830&w=2
> 
> Jon Hunter (8):
>   soc: tegra: pmc: Remove non-existing L2 partition for Tegra124
>   soc: tegra: pmc: Restore base address on probe failure
>   soc: tegra: pmc: Protect public functions from potential race
>     conditions
>   soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type
>   soc: tegra: pmc: Fix testing of powergate state
>   soc: tegra: pmc: Fix verification of valid partitions
>   soc: tegra: pmc: Remove additional check for a valid partition
>   soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC
> 
>  drivers/gpu/drm/tegra/drm.h |   2 +-
>  drivers/soc/tegra/pmc.c     | 131 +++++++++++++++++++++++++++++---------------
>  include/soc/tegra/pmc.h     |  35 ++++++------
>  3 files changed, 105 insertions(+), 63 deletions(-)
> 

Re: [PATCH 0/8] soc: tegra: pmc: Various fixes

[Mathieu Poirier]

On 11 February 2016 at 11:15, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> +Mathieu, sorry I should have copied you. I recall that you had some
> comments on this. I guess you are not on linux-tegra, but you can view
> the patches here:
>
> http://marc.info/?l=linux-tegra&r=1&b=201602&w=2

Thanks for fixing patch 2 and 3, this is much better.  I've also
reviewed the other patches and things do look good to me.

For the whole set,
Reviewed-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

>
> Jon
>
> On 11/02/16 18:03, Jon Hunter wrote:
>> This series contains a few fixes on the Tegra PMC driver. Some of these
>> patches were original included as part of the series to add generic PM
>> domain support for Tegra [0]. However, to keep these patch series a
>> manageable size, I have separate them in to this series and I have added
>> a couple more fixes as well.
>>
>> [0] http://marc.info/?l=linux-tegra&m=145399885003830&w=2
>>
>> Jon Hunter (8):
>>   soc: tegra: pmc: Remove non-existing L2 partition for Tegra124
>>   soc: tegra: pmc: Restore base address on probe failure
>>   soc: tegra: pmc: Protect public functions from potential race
>>     conditions
>>   soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type
>>   soc: tegra: pmc: Fix testing of powergate state
>>   soc: tegra: pmc: Fix verification of valid partitions
>>   soc: tegra: pmc: Remove additional check for a valid partition
>>   soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC
>>
>>  drivers/gpu/drm/tegra/drm.h |   2 +-
>>  drivers/soc/tegra/pmc.c     | 131 +++++++++++++++++++++++++++++---------------
>>  include/soc/tegra/pmc.h     |  35 ++++++------
>>  3 files changed, 105 insertions(+), 63 deletions(-)
>>

Re: [PATCH 8/8] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Jon Hunter]

On 11/02/16 18:03, Jon Hunter wrote:
> 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.
> 
> Furthermore, because we cannot use the APBDEV_PMC_PWRGATE_STATUS_0
> register to determine if the GPU partition is powered, use the
> APBDEV_PMC_CLAMP_STATUS_0 register to determine if the GPU partition
> is powered. The APBDEV_PMC_CLAMP_STATUS_0 register is bit wise
> compatible with the APBDEV_PMC_PWRGATE_STATUS_0 register and if the
> clamp is disabled (ie. the appropriate bit == 0), then this indicates
> the partition is powered.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 8e358dbffaed..442232269df3 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -52,6 +52,8 @@
>  #define  DPD_SAMPLE_ENABLE		(1 << 0)
>  #define  DPD_SAMPLE_DISABLE		(0 << 0)
>  
> +#define CLAMP_STATUS			0x2c
> +
>  #define PWRGATE_TOGGLE			0x30
>  #define  PWRGATE_TOGGLE_START		(1 << 8)
>  
> @@ -109,6 +111,7 @@ struct tegra_pmc_soc {
>  
>  	bool has_tsense_reset;
>  	bool has_gpu_clamps;
> +	bool has_gpu_toggle;
>  };
>  
>  /**
> @@ -176,7 +179,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>  
>  static inline bool tegra_powergate_state(int id)
>  {
> -	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
> +	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
> +		return (tegra_pmc_readl(CLAMP_STATUS) & BIT(id)) == 0;

Well, this is still not right. For devices with
!pmc->soc->has_gpu_toggle, these devices have their own separate
register for removing the clamp and so the APBDEV_PMC_CLAMP_STATUS_0
register will not tell us the status. I need to look at this some more.

Jon

Re: [PATCH 8/8] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Thierry Reding]

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

On Fri, Feb 12, 2016 at 09:38:46AM +0000, Jon Hunter wrote:
> 
> On 11/02/16 18:03, Jon Hunter wrote:
> > 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.
> > 
> > Furthermore, because we cannot use the APBDEV_PMC_PWRGATE_STATUS_0
> > register to determine if the GPU partition is powered, use the
> > APBDEV_PMC_CLAMP_STATUS_0 register to determine if the GPU partition
> > is powered. The APBDEV_PMC_CLAMP_STATUS_0 register is bit wise
> > compatible with the APBDEV_PMC_PWRGATE_STATUS_0 register and if the
> > clamp is disabled (ie. the appropriate bit == 0), then this indicates
> > the partition is powered.
> > 
> > Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/soc/tegra/pmc.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 8e358dbffaed..442232269df3 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -52,6 +52,8 @@
> >  #define  DPD_SAMPLE_ENABLE		(1 << 0)
> >  #define  DPD_SAMPLE_DISABLE		(0 << 0)
> >  
> > +#define CLAMP_STATUS			0x2c
> > +
> >  #define PWRGATE_TOGGLE			0x30
> >  #define  PWRGATE_TOGGLE_START		(1 << 8)
> >  
> > @@ -109,6 +111,7 @@ struct tegra_pmc_soc {
> >  
> >  	bool has_tsense_reset;
> >  	bool has_gpu_clamps;
> > +	bool has_gpu_toggle;
> >  };
> >  
> >  /**
> > @@ -176,7 +179,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
> >  
> >  static inline bool tegra_powergate_state(int id)
> >  {
> > -	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
> > +	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
> > +		return (tegra_pmc_readl(CLAMP_STATUS) & BIT(id)) == 0;
> 
> Well, this is still not right. For devices with
> !pmc->soc->has_gpu_toggle, these devices have their own separate
> register for removing the clamp and so the APBDEV_PMC_CLAMP_STATUS_0
> register will not tell us the status. I need to look at this some more.

Any objections to me applying patches 1-7 while you work on fixing this
last one?

Thierry

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

Re: [PATCH 8/8] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Jon Hunter]

On 12/02/16 17:25, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Feb 12, 2016 at 09:38:46AM +0000, Jon Hunter wrote:
>>
>> On 11/02/16 18:03, Jon Hunter wrote:
>>> 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.
>>>
>>> Furthermore, because we cannot use the APBDEV_PMC_PWRGATE_STATUS_0
>>> register to determine if the GPU partition is powered, use the
>>> APBDEV_PMC_CLAMP_STATUS_0 register to determine if the GPU partition
>>> is powered. The APBDEV_PMC_CLAMP_STATUS_0 register is bit wise
>>> compatible with the APBDEV_PMC_PWRGATE_STATUS_0 register and if the
>>> clamp is disabled (ie. the appropriate bit == 0), then this indicates
>>> the partition is powered.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/soc/tegra/pmc.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index 8e358dbffaed..442232269df3 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -52,6 +52,8 @@
>>>  #define  DPD_SAMPLE_ENABLE		(1 << 0)
>>>  #define  DPD_SAMPLE_DISABLE		(0 << 0)
>>>  
>>> +#define CLAMP_STATUS			0x2c
>>> +
>>>  #define PWRGATE_TOGGLE			0x30
>>>  #define  PWRGATE_TOGGLE_START		(1 << 8)
>>>  
>>> @@ -109,6 +111,7 @@ struct tegra_pmc_soc {
>>>  
>>>  	bool has_tsense_reset;
>>>  	bool has_gpu_clamps;
>>> +	bool has_gpu_toggle;
>>>  };
>>>  
>>>  /**
>>> @@ -176,7 +179,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>>>  
>>>  static inline bool tegra_powergate_state(int id)
>>>  {
>>> -	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>>> +	if (id == TEGRA_POWERGATE_3D && !pmc->soc->has_gpu_toggle)
>>> +		return (tegra_pmc_readl(CLAMP_STATUS) & BIT(id)) == 0;
>>
>> Well, this is still not right. For devices with
>> !pmc->soc->has_gpu_toggle, these devices have their own separate
>> register for removing the clamp and so the APBDEV_PMC_CLAMP_STATUS_0
>> register will not tell us the status. I need to look at this some more.
> 
> Any objections to me applying patches 1-7 while you work on fixing this
> last one?

Not at all. Please go ahead.

Cheers
Jon

[PATCH V2] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Jon Hunter]

For Tegra124 and Tegra210, the GPU partition cannot be toggled on and
off via the APBDEV_PMC_PWRGATE_TOGGLE_0 register. For these devices, the
partition is simply powered up and down via an external regulator.
For these devices, there is a separate register for controlling the
signal clamping of the partition and this is described in the PMC SoC
data by the "has_gpu_clamp" variable. Use this variable to determine if
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.

Furthermore, we cannot use the APBDEV_PMC_PWRGATE_STATUS_0 register to
determine if the GPU partition is powered for Tegra124 and Tegra210.
However, if the GPU partition is powered, then the signal clamp for the
GPU partition should be removed and so use bit 0 of the
APBDEV_PMC_GPU_RG_CNTRL_0 register to determine if the clamp has been
removed (bit[0] = 0) and the GPU partition is powered.

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

Changes v1-v2:
- Drop the has_gpu_toggle and use has_gpu_clamps as it is not necessary
  to have two variables.
- Correct the register used for reading the clamps status for the GPU
  partition

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

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 8e358dbffaed..e4fd40fa27e8 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -176,7 +176,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
 
 static inline bool tegra_powergate_state(int id)
 {
-	return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
+	if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
+		return (tegra_pmc_readl(GPU_RG_CNTRL) & 0x1) == 0;
+	else
+		return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
 }
 
 static inline bool tegra_powergate_is_valid(int id)
@@ -191,6 +194,9 @@ static inline bool tegra_powergate_is_valid(int id)
  */
 static int tegra_powergate_set(unsigned int id, bool new_state)
 {
+	if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
+		return -EINVAL;
+
 	mutex_lock(&pmc->powergates_lock);
 
 	if (tegra_powergate_state(id) == new_state) {
-- 
2.1.4

Re: [PATCH 0/8] soc: tegra: pmc: Various fixes

[Thierry Reding]

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

On Thu, Feb 11, 2016 at 06:03:18PM +0000, Jon Hunter wrote:
> This series contains a few fixes on the Tegra PMC driver. Some of these
> patches were original included as part of the series to add generic PM
> domain support for Tegra [0]. However, to keep these patch series a
> manageable size, I have separate them in to this series and I have added
> a couple more fixes as well.
> 
> [0] http://marc.info/?l=linux-tegra&m=145399885003830&w=2
> 
> Jon Hunter (8):
>   soc: tegra: pmc: Remove non-existing L2 partition for Tegra124
>   soc: tegra: pmc: Restore base address on probe failure
>   soc: tegra: pmc: Protect public functions from potential race
>     conditions
>   soc: tegra: pmc: Change powergate and rail IDs to be an unsigned type
>   soc: tegra: pmc: Fix testing of powergate state
>   soc: tegra: pmc: Fix verification of valid partitions
>   soc: tegra: pmc: Remove additional check for a valid partition
>   soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC
> 
>  drivers/gpu/drm/tegra/drm.h |   2 +-
>  drivers/soc/tegra/pmc.c     | 131 +++++++++++++++++++++++++++++---------------
>  include/soc/tegra/pmc.h     |  35 ++++++------
>  3 files changed, 105 insertions(+), 63 deletions(-)

Applied patches 1-7 and v2 of patch 8, all with Mathieu's Reviewed-by.

Thanks,
Thierry

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

Re: [PATCH V2] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Mathieu Poirier]

On 15 February 2016 at 05:38, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> 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.
> For these devices, there is a separate register for controlling the
> signal clamping of the partition and this is described in the PMC SoC
> data by the "has_gpu_clamp" variable. Use this variable to determine if
> 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.
>
> Furthermore, we cannot use the APBDEV_PMC_PWRGATE_STATUS_0 register to
> determine if the GPU partition is powered for Tegra124 and Tegra210.
> However, if the GPU partition is powered, then the signal clamp for the
> GPU partition should be removed and so use bit 0 of the
> APBDEV_PMC_GPU_RG_CNTRL_0 register to determine if the clamp has been
> removed (bit[0] = 0) and the GPU partition is powered.
>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>
> Changes v1-v2:
> - Drop the has_gpu_toggle and use has_gpu_clamps as it is not necessary
>   to have two variables.
> - Correct the register used for reading the clamps status for the GPU
>   partition
>
>  drivers/soc/tegra/pmc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 8e358dbffaed..e4fd40fa27e8 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -176,7 +176,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>
>  static inline bool tegra_powergate_state(int id)
>  {
> -       return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
> +               return (tegra_pmc_readl(GPU_RG_CNTRL) & 0x1) == 0;
> +       else
> +               return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;

From what I deduce power is on if bit 0 of register GPU_RG_CNTRL is
equal to 0.  In the other case power is on if bit 'id' of register
PWRGATE_STATUS is equal to 1.  If I'm right this is highly confusing
and some comments  would be appreciated.  Changing

"return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;"

for

"return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) == 1;"

would also help.

>  }
>
>  static inline bool tegra_powergate_is_valid(int id)
> @@ -191,6 +194,9 @@ static inline bool tegra_powergate_is_valid(int id)
>   */
>  static int tegra_powergate_set(unsigned int id, bool new_state)
>  {
> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
> +               return -EINVAL;
> +
>         mutex_lock(&pmc->powergates_lock);
>
>         if (tegra_powergate_state(id) == new_state) {
> --
> 2.1.4

With the above in mind:

Reviewed-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Re: [PATCH V2] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Jon Hunter]

On 26/02/16 16:06, Mathieu Poirier wrote:
> On 15 February 2016 at 05:38, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> 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.
>> For these devices, there is a separate register for controlling the
>> signal clamping of the partition and this is described in the PMC SoC
>> data by the "has_gpu_clamp" variable. Use this variable to determine if
>> 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.
>>
>> Furthermore, we cannot use the APBDEV_PMC_PWRGATE_STATUS_0 register to
>> determine if the GPU partition is powered for Tegra124 and Tegra210.
>> However, if the GPU partition is powered, then the signal clamp for the
>> GPU partition should be removed and so use bit 0 of the
>> APBDEV_PMC_GPU_RG_CNTRL_0 register to determine if the clamp has been
>> removed (bit[0] = 0) and the GPU partition is powered.
>>
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>
>> Changes v1-v2:
>> - Drop the has_gpu_toggle and use has_gpu_clamps as it is not necessary
>>   to have two variables.
>> - Correct the register used for reading the clamps status for the GPU
>>   partition
>>
>>  drivers/soc/tegra/pmc.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 8e358dbffaed..e4fd40fa27e8 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -176,7 +176,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>>
>>  static inline bool tegra_powergate_state(int id)
>>  {
>> -       return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
>> +               return (tegra_pmc_readl(GPU_RG_CNTRL) & 0x1) == 0;
>> +       else
>> +               return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
> 
> From what I deduce power is on if bit 0 of register GPU_RG_CNTRL is
> equal to 0.  In the other case power is on if bit 'id' of register
> PWRGATE_STATUS is equal to 1.  If I'm right this is highly confusing
> and some comments  would be appreciated.  Changing
> 
> "return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;"
> 
> for
> 
> "return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) == 1;"

Yes, it is a bit confusing, but the above does not work. You are
comparing a bit with 1. What you need is:

"return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) == BIT(id);"

> would also help.
> 
>>  }
>>
>>  static inline bool tegra_powergate_is_valid(int id)
>> @@ -191,6 +194,9 @@ static inline bool tegra_powergate_is_valid(int id)
>>   */
>>  static int tegra_powergate_set(unsigned int id, bool new_state)
>>  {
>> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
>> +               return -EINVAL;
>> +
>>         mutex_lock(&pmc->powergates_lock);
>>
>>         if (tegra_powergate_state(id) == new_state) {
>> --
>> 2.1.4
> 
> With the above in mind:
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Thanks. Thierry has queued this version and so if you feel strongly we
could update in a follow-up.

Jon

Re: [PATCH V2] soc: tegra: pmc: Ensure GPU partition can be toggled on/off by PMC

[Mathieu Poirier]

On 26 February 2016 at 09:43, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 26/02/16 16:06, Mathieu Poirier wrote:
>> On 15 February 2016 at 05:38, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> 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.
>>> For these devices, there is a separate register for controlling the
>>> signal clamping of the partition and this is described in the PMC SoC
>>> data by the "has_gpu_clamp" variable. Use this variable to determine if
>>> 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.
>>>
>>> Furthermore, we cannot use the APBDEV_PMC_PWRGATE_STATUS_0 register to
>>> determine if the GPU partition is powered for Tegra124 and Tegra210.
>>> However, if the GPU partition is powered, then the signal clamp for the
>>> GPU partition should be removed and so use bit 0 of the
>>> APBDEV_PMC_GPU_RG_CNTRL_0 register to determine if the clamp has been
>>> removed (bit[0] = 0) and the GPU partition is powered.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>
>>> Changes v1-v2:
>>> - Drop the has_gpu_toggle and use has_gpu_clamps as it is not necessary
>>>   to have two variables.
>>> - Correct the register used for reading the clamps status for the GPU
>>>   partition
>>>
>>>  drivers/soc/tegra/pmc.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>> index 8e358dbffaed..e4fd40fa27e8 100644
>>> --- a/drivers/soc/tegra/pmc.c
>>> +++ b/drivers/soc/tegra/pmc.c
>>> @@ -176,7 +176,10 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>>>
>>>  static inline bool tegra_powergate_state(int id)
>>>  {
>>> -       return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>>> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
>>> +               return (tegra_pmc_readl(GPU_RG_CNTRL) & 0x1) == 0;
>>> +       else
>>> +               return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;
>>
>> From what I deduce power is on if bit 0 of register GPU_RG_CNTRL is
>> equal to 0.  In the other case power is on if bit 'id' of register
>> PWRGATE_STATUS is equal to 1.  If I'm right this is highly confusing
>> and some comments  would be appreciated.  Changing
>>
>> "return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) != 0;"
>>
>> for
>>
>> "return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) == 1;"
>
> Yes, it is a bit confusing, but the above does not work. You are
> comparing a bit with 1. What you need is:
>
> "return (tegra_pmc_readl(PWRGATE_STATUS) & BIT(id)) == BIT(id);"

Ah yes, of course.

>
>> would also help.
>>
>>>  }
>>>
>>>  static inline bool tegra_powergate_is_valid(int id)
>>> @@ -191,6 +194,9 @@ static inline bool tegra_powergate_is_valid(int id)
>>>   */
>>>  static int tegra_powergate_set(unsigned int id, bool new_state)
>>>  {
>>> +       if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
>>> +               return -EINVAL;
>>> +
>>>         mutex_lock(&pmc->powergates_lock);
>>>
>>>         if (tegra_powergate_state(id) == new_state) {
>>> --
>>> 2.1.4
>>
>> With the above in mind:
>>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Thanks. Thierry has queued this version and so if you feel strongly we
> could update in a follow-up.

The decision is for Thierry to make - I'm fine with the current code.

>
> Jon
>