[PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

This series bring up secondary CPU for Tegra114. Because we are going to
support generic power domain later, the conventional powergate driver will
be removed. So we add the CPU power on/off function in PMC driver first.

This series was dependent on the patches below.
"ARM: tegra: Fix unchecked return value"
"ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
"ARM: tegra: refactor tegra{20,30}_boot_secondary"

Joseph Lo (4):
  ARM: tegra: pmc: convert PMC driver to support DT only
  ARM: tegra: pmc: add power on function for secondary CPUs
  ARM: tegra30: platsmp: replace the CPU power on function in PMC driver
  ARM: tegra114: bring up secondary CPU for SMP

 arch/arm/mach-tegra/platsmp.c |  27 +++----
 arch/arm/mach-tegra/pmc.c     | 159 ++++++++++++++++++++++++++++++++++--------
 arch/arm/mach-tegra/pmc.h     |   4 ++
 3 files changed, 149 insertions(+), 41 deletions(-)

-- 
1.8.1.1

[PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

This series bring up secondary CPU for Tegra114. Because we are going to
support generic power domain later, the conventional powergate driver will
be removed. So we add the CPU power on/off function in PMC driver first.

This series was dependent on the patches below.
"ARM: tegra: Fix unchecked return value"
"ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
"ARM: tegra: refactor tegra{20,30}_boot_secondary"

Joseph Lo (4):
  ARM: tegra: pmc: convert PMC driver to support DT only
  ARM: tegra: pmc: add power on function for secondary CPUs
  ARM: tegra30: platsmp: replace the CPU power on function in PMC driver
  ARM: tegra114: bring up secondary CPU for SMP

 arch/arm/mach-tegra/platsmp.c |  27 +++----
 arch/arm/mach-tegra/pmc.c     | 159 ++++++++++++++++++++++++++++++++++--------
 arch/arm/mach-tegra/pmc.h     |   4 ++
 3 files changed, 149 insertions(+), 41 deletions(-)

-- 
1.8.1.1

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

The Tegra kernel only support boot from DT now. Clean up the PMC driver
to support DT only, that includes:

* remove the ifdef of CONFIG_OF
* replace the static mapping of PMC addr to map from DT

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index d4fdb5f..2315e25 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -16,59 +16,59 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
-
-#include "iomap.h"
+#include <linux/of_address.h>
 
 #define PMC_CTRL		0x0
 #define PMC_CTRL_INTR_LOW	(1 << 17)
 
+static void __iomem *tegra_pmc_base;
+static bool tegra_pmc_invert_interrupt;
+
 static inline u32 tegra_pmc_readl(u32 reg)
 {
-	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	return readl_relaxed(tegra_pmc_base + reg);
 }
 
 static inline void tegra_pmc_writel(u32 val, u32 reg)
 {
-	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	writel_relaxed(val, (tegra_pmc_base + reg));
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra20-pmc" },
 	{ }
 };
-#endif
+
+static void tegra_pmc_parse_dt(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, matches);
+	if (np) {
+		tegra_pmc_base = of_iomap(np, 0);
+
+		tegra_pmc_invert_interrupt = of_property_read_bool(np,
+					     "nvidia,invert-interrupt");
+	} else {
+		/* Should not be here, the PMC DT node should always exist. */
+		BUG();
+	}
+}
 
 void __init tegra_pmc_init(void)
 {
-	/*
-	 * For now, Harmony is the only board that uses the PMC, and it wants
-	 * the signal inverted. Seaboard would too if it used the PMC.
-	 * Hopefully by the time other boards want to use the PMC, everything
-	 * will be device-tree, or they also want it inverted.
-	 */
-	bool invert_interrupt = true;
 	u32 val;
 
-#ifdef CONFIG_OF
-	if (of_have_populated_dt()) {
-		struct device_node *np;
+	if (!of_have_populated_dt())
+		return;
 
-		invert_interrupt = false;
-
-		np = of_find_matching_node(NULL, matches);
-		if (np) {
-			if (of_find_property(np, "nvidia,invert-interrupt",
-						NULL))
-				invert_interrupt = true;
-		}
-	}
-#endif
+	tegra_pmc_parse_dt();
 
 	val = tegra_pmc_readl(PMC_CTRL);
-	if (invert_interrupt)
+	if (tegra_pmc_invert_interrupt)
 		val |= PMC_CTRL_INTR_LOW;
 	else
 		val &= ~PMC_CTRL_INTR_LOW;
-- 
1.8.1.1

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

The Tegra kernel only support boot from DT now. Clean up the PMC driver
to support DT only, that includes:

* remove the ifdef of CONFIG_OF
* replace the static mapping of PMC addr to map from DT

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index d4fdb5f..2315e25 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -16,59 +16,59 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
-
-#include "iomap.h"
+#include <linux/of_address.h>
 
 #define PMC_CTRL		0x0
 #define PMC_CTRL_INTR_LOW	(1 << 17)
 
+static void __iomem *tegra_pmc_base;
+static bool tegra_pmc_invert_interrupt;
+
 static inline u32 tegra_pmc_readl(u32 reg)
 {
-	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	return readl_relaxed(tegra_pmc_base + reg);
 }
 
 static inline void tegra_pmc_writel(u32 val, u32 reg)
 {
-	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	writel_relaxed(val, (tegra_pmc_base + reg));
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra20-pmc" },
 	{ }
 };
-#endif
+
+static void tegra_pmc_parse_dt(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, matches);
+	if (np) {
+		tegra_pmc_base = of_iomap(np, 0);
+
+		tegra_pmc_invert_interrupt = of_property_read_bool(np,
+					     "nvidia,invert-interrupt");
+	} else {
+		/* Should not be here, the PMC DT node should always exist. */
+		BUG();
+	}
+}
 
 void __init tegra_pmc_init(void)
 {
-	/*
-	 * For now, Harmony is the only board that uses the PMC, and it wants
-	 * the signal inverted. Seaboard would too if it used the PMC.
-	 * Hopefully by the time other boards want to use the PMC, everything
-	 * will be device-tree, or they also want it inverted.
-	 */
-	bool invert_interrupt = true;
 	u32 val;
 
-#ifdef CONFIG_OF
-	if (of_have_populated_dt()) {
-		struct device_node *np;
+	if (!of_have_populated_dt())
+		return;
 
-		invert_interrupt = false;
-
-		np = of_find_matching_node(NULL, matches);
-		if (np) {
-			if (of_find_property(np, "nvidia,invert-interrupt",
-						NULL))
-				invert_interrupt = true;
-		}
-	}
-#endif
+	tegra_pmc_parse_dt();
 
 	val = tegra_pmc_readl(PMC_CTRL);
-	if (invert_interrupt)
+	if (tegra_pmc_invert_interrupt)
 		val |= PMC_CTRL_INTR_LOW;
 	else
 		val &= ~PMC_CTRL_INTR_LOW;
-- 
1.8.1.1

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

Adding the power on function for secondary CPUs in PMC driver, this can
help us to remove legacy powergate driver and add generic power domain
support later.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
 arch/arm/mach-tegra/pmc.h |   4 ++
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 2315e25..a2446c2 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -21,8 +21,26 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 
-#define PMC_CTRL		0x0
-#define PMC_CTRL_INTR_LOW	(1 << 17)
+#define PMC_CTRL			0x0
+#define PMC_CTRL_INTR_LOW		(1 << 17)
+#define PMC_PWRGATE_TOGGLE		0x30
+#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
+#define PMC_REMOVE_CLAMPING		0x34
+#define PMC_PWRGATE_STATUS		0x38
+
+#define TEGRA_POWERGATE_PCIE	3
+#define TEGRA_POWERGATE_VDEC	4
+#define TEGRA_POWERGATE_CPU1	9
+#define TEGRA_POWERGATE_CPU2	10
+#define TEGRA_POWERGATE_CPU3	11
+
+static u8 tegra_cpu_domains[] = {
+	0xFF,			/* not available for CPU0 */
+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+};
+static DEFINE_SPINLOCK(tegra_powergate_lock);
 
 static void __iomem *tegra_pmc_base;
 static bool tegra_pmc_invert_interrupt;
@@ -37,6 +55,89 @@ static inline void tegra_pmc_writel(u32 val, u32 reg)
 	writel_relaxed(val, (tegra_pmc_base + reg));
 }
 
+static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
+{
+	if (cpuid <= 0 || cpuid > num_possible_cpus())
+		return -EINVAL;
+	return tegra_cpu_domains[cpuid];
+}
+
+static int tegra_pmc_powergate_set(int id, bool new_state)
+{
+	bool status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tegra_powergate_lock, flags);
+
+	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
+
+	WARN_ON(status == new_state);
+
+	tegra_pmc_writel(PMC_PWRGATE_TOGGLE_START | id, PMC_PWRGATE_TOGGLE);
+
+	spin_unlock_irqrestore(&tegra_powergate_lock, flags);
+
+	return 0;
+}
+
+static bool tegra_pmc_powergate_is_powered(int id)
+{
+	u32 status;
+
+	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
+	return !!status;
+}
+
+static int tegra_pmc_powergate_remove_clamping(int id)
+{
+	u32 mask;
+
+	/*
+	 * Tegra 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, PMC_REMOVE_CLAMPING);
+
+	return 0;
+}
+
+bool tegra_pmc_cpu_is_powered(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return false;
+	return tegra_pmc_powergate_is_powered(id);
+}
+
+int tegra_pmc_cpu_power_on(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return id;
+	return tegra_pmc_powergate_set(id, true);
+}
+
+int tegra_pmc_cpu_remove_clamping(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return id;
+	return tegra_pmc_powergate_remove_clamping(id);
+}
+
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra20-pmc" },
 	{ }
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 8995ee4..7d44710 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -18,6 +18,10 @@
 #ifndef __MACH_TEGRA_PMC_H
 #define __MACH_TEGRA_PMC_H
 
+bool tegra_pmc_cpu_is_powered(int cpuid);
+int tegra_pmc_cpu_power_on(int cpuid);
+int tegra_pmc_cpu_remove_clamping(int cpuid);
+
 void tegra_pmc_init(void);
 
 #endif
-- 
1.8.1.1

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

Adding the power on function for secondary CPUs in PMC driver, this can
help us to remove legacy powergate driver and add generic power domain
support later.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
 arch/arm/mach-tegra/pmc.h |   4 ++
 2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 2315e25..a2446c2 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -21,8 +21,26 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 
-#define PMC_CTRL		0x0
-#define PMC_CTRL_INTR_LOW	(1 << 17)
+#define PMC_CTRL			0x0
+#define PMC_CTRL_INTR_LOW		(1 << 17)
+#define PMC_PWRGATE_TOGGLE		0x30
+#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
+#define PMC_REMOVE_CLAMPING		0x34
+#define PMC_PWRGATE_STATUS		0x38
+
+#define TEGRA_POWERGATE_PCIE	3
+#define TEGRA_POWERGATE_VDEC	4
+#define TEGRA_POWERGATE_CPU1	9
+#define TEGRA_POWERGATE_CPU2	10
+#define TEGRA_POWERGATE_CPU3	11
+
+static u8 tegra_cpu_domains[] = {
+	0xFF,			/* not available for CPU0 */
+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+};
+static DEFINE_SPINLOCK(tegra_powergate_lock);
 
 static void __iomem *tegra_pmc_base;
 static bool tegra_pmc_invert_interrupt;
@@ -37,6 +55,89 @@ static inline void tegra_pmc_writel(u32 val, u32 reg)
 	writel_relaxed(val, (tegra_pmc_base + reg));
 }
 
+static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
+{
+	if (cpuid <= 0 || cpuid > num_possible_cpus())
+		return -EINVAL;
+	return tegra_cpu_domains[cpuid];
+}
+
+static int tegra_pmc_powergate_set(int id, bool new_state)
+{
+	bool status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tegra_powergate_lock, flags);
+
+	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
+
+	WARN_ON(status == new_state);
+
+	tegra_pmc_writel(PMC_PWRGATE_TOGGLE_START | id, PMC_PWRGATE_TOGGLE);
+
+	spin_unlock_irqrestore(&tegra_powergate_lock, flags);
+
+	return 0;
+}
+
+static bool tegra_pmc_powergate_is_powered(int id)
+{
+	u32 status;
+
+	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
+	return !!status;
+}
+
+static int tegra_pmc_powergate_remove_clamping(int id)
+{
+	u32 mask;
+
+	/*
+	 * Tegra 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, PMC_REMOVE_CLAMPING);
+
+	return 0;
+}
+
+bool tegra_pmc_cpu_is_powered(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return false;
+	return tegra_pmc_powergate_is_powered(id);
+}
+
+int tegra_pmc_cpu_power_on(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return id;
+	return tegra_pmc_powergate_set(id, true);
+}
+
+int tegra_pmc_cpu_remove_clamping(int cpuid)
+{
+	int id;
+
+	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
+	if (IS_ERR_VALUE(id))
+		return id;
+	return tegra_pmc_powergate_remove_clamping(id);
+}
+
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra20-pmc" },
 	{ }
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 8995ee4..7d44710 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -18,6 +18,10 @@
 #ifndef __MACH_TEGRA_PMC_H
 #define __MACH_TEGRA_PMC_H
 
+bool tegra_pmc_cpu_is_powered(int cpuid);
+int tegra_pmc_cpu_power_on(int cpuid);
+int tegra_pmc_cpu_remove_clamping(int cpuid);
+
 void tegra_pmc_init(void);
 
 #endif
-- 
1.8.1.1

[PATCH 3/4] ARM: tegra30: platsmp: replace the CPU power on function in PMC driver

[Joseph Lo]

Using the CPU power on function in PMC driver to bring up secondary CPUs,
because we are going to re-factor powergate driver to support generic
power domain. It will be removed later and added the generic power domain
support in PMC driver.

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/platsmp.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 41971ac..601bd0c 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -26,11 +26,10 @@
 #include <asm/smp_scu.h>
 #include <asm/smp_plat.h>
 
-#include <mach/powergate.h>
-
 #include "fuse.h"
 #include "flowctrl.h"
 #include "reset.h"
+#include "pmc.h"
 
 #include "common.h"
 #include "iomap.h"
@@ -80,14 +79,10 @@ static int tegra20_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	int ret, pwrgateid;
+	int ret;
 	unsigned long timeout;
 
 	cpu = cpu_logical_map(cpu);
-	pwrgateid = tegra_cpu_powergate_id(cpu);
-	if (pwrgateid < 0)
-		return pwrgateid;
-
 	tegra_put_cpu_in_reset(cpu);
 	flowctrl_write_cpu_halt(cpu, 0);
 
@@ -108,7 +103,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
 		timeout = jiffies + msecs_to_jiffies(50);
 		do {
-			if (tegra_powergate_is_powered(pwrgateid))
+			if (tegra_pmc_cpu_is_powered(cpu))
 				goto remove_clamps;
 			udelay(10);
 		} while (time_before(jiffies, timeout));
@@ -120,14 +115,14 @@ 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_powergate_is_powered(pwrgateid)) {
-		ret = tegra_powergate_power_on(pwrgateid);
+	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_powergate_is_powered(pwrgateid)) {
+		while (tegra_pmc_cpu_is_powered(cpu)) {
 			if (time_after(jiffies, timeout))
 				return -ETIMEDOUT;
 			udelay(10);
@@ -140,7 +135,7 @@ remove_clamps:
 	udelay(10);
 
 	/* Remove I/O clamps. */
-	ret = tegra_powergate_remove_clamping(pwrgateid);
+	ret = tegra_pmc_cpu_remove_clamping(cpu);
 	if (ret)
 		return ret;
 
-- 
1.8.1.1

[PATCH 3/4] ARM: tegra30: platsmp: replace the CPU power on function in PMC driver

[Joseph Lo]

Using the CPU power on function in PMC driver to bring up secondary CPUs,
because we are going to re-factor powergate driver to support generic
power domain. It will be removed later and added the generic power domain
support in PMC driver.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/platsmp.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 41971ac..601bd0c 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -26,11 +26,10 @@
 #include <asm/smp_scu.h>
 #include <asm/smp_plat.h>
 
-#include <mach/powergate.h>
-
 #include "fuse.h"
 #include "flowctrl.h"
 #include "reset.h"
+#include "pmc.h"
 
 #include "common.h"
 #include "iomap.h"
@@ -80,14 +79,10 @@ static int tegra20_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	int ret, pwrgateid;
+	int ret;
 	unsigned long timeout;
 
 	cpu = cpu_logical_map(cpu);
-	pwrgateid = tegra_cpu_powergate_id(cpu);
-	if (pwrgateid < 0)
-		return pwrgateid;
-
 	tegra_put_cpu_in_reset(cpu);
 	flowctrl_write_cpu_halt(cpu, 0);
 
@@ -108,7 +103,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
 		timeout = jiffies + msecs_to_jiffies(50);
 		do {
-			if (tegra_powergate_is_powered(pwrgateid))
+			if (tegra_pmc_cpu_is_powered(cpu))
 				goto remove_clamps;
 			udelay(10);
 		} while (time_before(jiffies, timeout));
@@ -120,14 +115,14 @@ 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_powergate_is_powered(pwrgateid)) {
-		ret = tegra_powergate_power_on(pwrgateid);
+	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_powergate_is_powered(pwrgateid)) {
+		while (tegra_pmc_cpu_is_powered(cpu)) {
 			if (time_after(jiffies, timeout))
 				return -ETIMEDOUT;
 			udelay(10);
@@ -140,7 +135,7 @@ remove_clamps:
 	udelay(10);
 
 	/* Remove I/O clamps. */
-	ret = tegra_powergate_remove_clamping(pwrgateid);
+	ret = tegra_pmc_cpu_remove_clamping(cpu);
 	if (ret)
 		return ret;
 
-- 
1.8.1.1

[PATCH 4/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

The secondary CPU can be brought up by toggling the power in PMC. Then
the flow controller will release CPU to go by clearing the reset and
clamp signal automatically.

Based on the work by:
Bo Yan <byan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/platsmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 601bd0c..516aab2 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -146,6 +146,12 @@ remove_clamps:
 	return 0;
 }
 
+static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	cpu = cpu_logical_map(cpu);
+	return tegra_pmc_cpu_power_on(cpu);
+}
+
 static int __cpuinit tegra_boot_secondary(unsigned int cpu,
 					  struct task_struct *idle)
 {
@@ -153,6 +159,8 @@ static int __cpuinit tegra_boot_secondary(unsigned int cpu,
 		return tegra20_boot_secondary(cpu, idle);
 	if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC) && tegra_chip_id == TEGRA30)
 		return tegra30_boot_secondary(cpu, idle);
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_114_SOC) && tegra_chip_id == TEGRA114)
+		return tegra114_boot_secondary(cpu, idle);
 
 	return -EINVAL;
 }
-- 
1.8.1.1

[PATCH 4/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

The secondary CPU can be brought up by toggling the power in PMC. Then
the flow controller will release CPU to go by clearing the reset and
clamp signal automatically.

Based on the work by:
Bo Yan <byan@nvidia.com>

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/platsmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 601bd0c..516aab2 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -146,6 +146,12 @@ remove_clamps:
 	return 0;
 }
 
+static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	cpu = cpu_logical_map(cpu);
+	return tegra_pmc_cpu_power_on(cpu);
+}
+
 static int __cpuinit tegra_boot_secondary(unsigned int cpu,
 					  struct task_struct *idle)
 {
@@ -153,6 +159,8 @@ static int __cpuinit tegra_boot_secondary(unsigned int cpu,
 		return tegra20_boot_secondary(cpu, idle);
 	if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC) && tegra_chip_id == TEGRA30)
 		return tegra30_boot_secondary(cpu, idle);
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_114_SOC) && tegra_chip_id == TEGRA114)
+		return tegra114_boot_secondary(cpu, idle);
 
 	return -EINVAL;
 }
-- 
1.8.1.1

Re: [PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:47AM +0100, Joseph Lo wrote:
> This series bring up secondary CPU for Tegra114. Because we are going to
> support generic power domain later, the conventional powergate driver will
> be removed. So we add the CPU power on/off function in PMC driver first.
> 
> This series was dependent on the patches below.
> "ARM: tegra: Fix unchecked return value"
> "ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
> "ARM: tegra: refactor tegra{20,30}_boot_secondary"
> 

It also depends on 'ARM: tegra: don't unlock MMIO access to DBGLAR for working'
secondary core boot on Tegra114. Otherwise the secondary cores will hang at
bootup.

Cheers,

Peter.

[PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:47AM +0100, Joseph Lo wrote:
> This series bring up secondary CPU for Tegra114. Because we are going to
> support generic power domain later, the conventional powergate driver will
> be removed. So we add the CPU power on/off function in PMC driver first.
> 
> This series was dependent on the patches below.
> "ARM: tegra: Fix unchecked return value"
> "ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
> "ARM: tegra: refactor tegra{20,30}_boot_secondary"
> 

It also depends on 'ARM: tegra: don't unlock MMIO access to DBGLAR for working'
secondary core boot on Tegra114. Otherwise the secondary cores will hang at
bootup.

Cheers,

Peter.

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> Adding the power on function for secondary CPUs in PMC driver, this can
> help us to remove legacy powergate driver and add generic power domain
> support later.
> 
> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
>  arch/arm/mach-tegra/pmc.h |   4 ++
>  2 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> index 2315e25..a2446c2 100644
> --- a/arch/arm/mach-tegra/pmc.c
> +++ b/arch/arm/mach-tegra/pmc.c
> @@ -21,8 +21,26 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  
> -#define PMC_CTRL		0x0
> -#define PMC_CTRL_INTR_LOW	(1 << 17)
> +#define PMC_CTRL			0x0
> +#define PMC_CTRL_INTR_LOW		(1 << 17)
> +#define PMC_PWRGATE_TOGGLE		0x30
> +#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
> +#define PMC_REMOVE_CLAMPING		0x34
> +#define PMC_PWRGATE_STATUS		0x38
> +
> +#define TEGRA_POWERGATE_PCIE	3
> +#define TEGRA_POWERGATE_VDEC	4
> +#define TEGRA_POWERGATE_CPU1	9
> +#define TEGRA_POWERGATE_CPU2	10
> +#define TEGRA_POWERGATE_CPU3	11
> +
> +static u8 tegra_cpu_domains[] = {
> +	0xFF,			/* not available for CPU0 */

On Tegra114 we can also powergate CPU0, so this needs to be defined then.

Cheers,

Peter.

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> Adding the power on function for secondary CPUs in PMC driver, this can
> help us to remove legacy powergate driver and add generic power domain
> support later.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
>  arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
>  arch/arm/mach-tegra/pmc.h |   4 ++
>  2 files changed, 107 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> index 2315e25..a2446c2 100644
> --- a/arch/arm/mach-tegra/pmc.c
> +++ b/arch/arm/mach-tegra/pmc.c
> @@ -21,8 +21,26 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  
> -#define PMC_CTRL		0x0
> -#define PMC_CTRL_INTR_LOW	(1 << 17)
> +#define PMC_CTRL			0x0
> +#define PMC_CTRL_INTR_LOW		(1 << 17)
> +#define PMC_PWRGATE_TOGGLE		0x30
> +#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
> +#define PMC_REMOVE_CLAMPING		0x34
> +#define PMC_PWRGATE_STATUS		0x38
> +
> +#define TEGRA_POWERGATE_PCIE	3
> +#define TEGRA_POWERGATE_VDEC	4
> +#define TEGRA_POWERGATE_CPU1	9
> +#define TEGRA_POWERGATE_CPU2	10
> +#define TEGRA_POWERGATE_CPU3	11
> +
> +static u8 tegra_cpu_domains[] = {
> +	0xFF,			/* not available for CPU0 */

On Tegra114 we can also powergate CPU0, so this needs to be defined then.

Cheers,

Peter.

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT
> 
> Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> index d4fdb5f..2315e25 100644
> --- a/arch/arm/mach-tegra/pmc.c
> +++ b/arch/arm/mach-tegra/pmc.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
> + * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -16,59 +16,59 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> -
> -#include "iomap.h"
> +#include <linux/of_address.h>
>  
>  #define PMC_CTRL		0x0
>  #define PMC_CTRL_INTR_LOW	(1 << 17)
>  
> +static void __iomem *tegra_pmc_base;
> +static bool tegra_pmc_invert_interrupt;
> +
>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);
>  }
>  
>  static inline void tegra_pmc_writel(u32 val, u32 reg)
>  {
> -	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	writel_relaxed(val, (tegra_pmc_base + reg));
>  }
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id matches[] __initconst = {
>  	{ .compatible = "nvidia,tegra20-pmc" },
>  	{ }

At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
has:

pmc {
	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
	reg = <0x7000e400 0x400>;
};

otherwise the system will crash early during boot.

Cheers,

Peter.

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
>  arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> index d4fdb5f..2315e25 100644
> --- a/arch/arm/mach-tegra/pmc.c
> +++ b/arch/arm/mach-tegra/pmc.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
> + * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -16,59 +16,59 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> -
> -#include "iomap.h"
> +#include <linux/of_address.h>
>  
>  #define PMC_CTRL		0x0
>  #define PMC_CTRL_INTR_LOW	(1 << 17)
>  
> +static void __iomem *tegra_pmc_base;
> +static bool tegra_pmc_invert_interrupt;
> +
>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);
>  }
>  
>  static inline void tegra_pmc_writel(u32 val, u32 reg)
>  {
> -	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	writel_relaxed(val, (tegra_pmc_base + reg));
>  }
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id matches[] __initconst = {
>  	{ .compatible = "nvidia,tegra20-pmc" },
>  	{ }

At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
has:

pmc {
	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
	reg = <0x7000e400 0x400>;
};

otherwise the system will crash early during boot.

Cheers,

Peter.

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/21/2013 11:44 PM, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);

The change from readl to readl_relaxed is definitely unrelated to
cleaning up DT support. It needs to be a separate patch, and needs to be
mentioned in the commit description.

> +static void tegra_pmc_parse_dt(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, matches);
> +	if (np) {

Here, if (!np) BUG(); would be simpler, and allow the rest of the
function not to be indented.

>  void __init tegra_pmc_init(void)
>  {
...
> +	if (!of_have_populated_dt())
> +		return;

That won't ever be true.

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/21/2013 11:44 PM, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);

The change from readl to readl_relaxed is definitely unrelated to
cleaning up DT support. It needs to be a separate patch, and needs to be
mentioned in the commit description.

> +static void tegra_pmc_parse_dt(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, matches);
> +	if (np) {

Here, if (!np) BUG(); would be simpler, and allow the rest of the
function not to be indented.

>  void __init tegra_pmc_init(void)
>  {
...
> +	if (!of_have_populated_dt())
> +		return;

That won't ever be true.

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/21/2013 11:44 PM, Joseph Lo wrote:
> Adding the power on function for secondary CPUs in PMC driver, this can
> help us to remove legacy powergate driver and add generic power domain
> support later.

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

> +static u8 tegra_cpu_domains[] = {
> +	0xFF,			/* not available for CPU0 */
> +	TEGRA_POWERGATE_CPU1,
> +	TEGRA_POWERGATE_CPU2,
> +	TEGRA_POWERGATE_CPU3,
> +};

Per Peter's comment, you probably need SoC-specific arrays here, to
support CPU0 having a valid value or not.

> +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
> +{
> +	if (cpuid <= 0 || cpuid > num_possible_cpus())

cpuid >= num_possible_cpus()?

> +static int tegra_pmc_powergate_set(int id, bool new_state)
> +{
> +	bool status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tegra_powergate_lock, flags);
> +
> +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);

I would perform the read and the logical operations separately. Same for
the write below.

Don't you want to and with ~BIT(id) not BIT(id)?

> +static int tegra_pmc_powergate_remove_clamping(int id)
> +{
> +	u32 mask;
> +
> +	/*
> +	 * Tegra 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);

Is this just true for this one register, but not others? If it's true
everywhere, why not just fix the TEGRA_POWERGATE_* definitions?

I asked this downstream, but you didn't answer.

> +bool tegra_pmc_cpu_is_powered(int cpuid)
> +{
> +	int id;
> +
> +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> +	if (IS_ERR_VALUE(id))
> +		return false;

As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
intended for use on error-pointers, not on integer error codes.

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/21/2013 11:44 PM, Joseph Lo wrote:
> Adding the power on function for secondary CPUs in PMC driver, this can
> help us to remove legacy powergate driver and add generic power domain
> support later.

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

> +static u8 tegra_cpu_domains[] = {
> +	0xFF,			/* not available for CPU0 */
> +	TEGRA_POWERGATE_CPU1,
> +	TEGRA_POWERGATE_CPU2,
> +	TEGRA_POWERGATE_CPU3,
> +};

Per Peter's comment, you probably need SoC-specific arrays here, to
support CPU0 having a valid value or not.

> +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
> +{
> +	if (cpuid <= 0 || cpuid > num_possible_cpus())

cpuid >= num_possible_cpus()?

> +static int tegra_pmc_powergate_set(int id, bool new_state)
> +{
> +	bool status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tegra_powergate_lock, flags);
> +
> +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);

I would perform the read and the logical operations separately. Same for
the write below.

Don't you want to and with ~BIT(id) not BIT(id)?

> +static int tegra_pmc_powergate_remove_clamping(int id)
> +{
> +	u32 mask;
> +
> +	/*
> +	 * Tegra 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);

Is this just true for this one register, but not others? If it's true
everywhere, why not just fix the TEGRA_POWERGATE_* definitions?

I asked this downstream, but you didn't answer.

> +bool tegra_pmc_cpu_is_powered(int cpuid)
> +{
> +	int id;
> +
> +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> +	if (IS_ERR_VALUE(id))
> +		return false;

As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
intended for use on error-pointers, not on integer error codes.

Re: [PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

On Fri, 2013-02-22 at 20:43 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:47AM +0100, Joseph Lo wrote:
> > This series bring up secondary CPU for Tegra114. Because we are going to
> > support generic power domain later, the conventional powergate driver will
> > be removed. So we add the CPU power on/off function in PMC driver first.
> > 
> > This series was dependent on the patches below.
> > "ARM: tegra: Fix unchecked return value"
> > "ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
> > "ARM: tegra: refactor tegra{20,30}_boot_secondary"
> > 
> 
> It also depends on 'ARM: tegra: don't unlock MMIO access to DBGLAR for working'
> secondary core boot on Tegra114. Otherwise the secondary cores will hang at
> bootup.
> 
Thanks for reminder. This is definitely needed.

Joseph

[PATCH 0/4] ARM: tegra114: bring up secondary CPU for SMP

[Joseph Lo]

On Fri, 2013-02-22 at 20:43 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:47AM +0100, Joseph Lo wrote:
> > This series bring up secondary CPU for Tegra114. Because we are going to
> > support generic power domain later, the conventional powergate driver will
> > be removed. So we add the CPU power on/off function in PMC driver first.
> > 
> > This series was dependent on the patches below.
> > "ARM: tegra: Fix unchecked return value"
> > "ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs"
> > "ARM: tegra: refactor tegra{20,30}_boot_secondary"
> > 
> 
> It also depends on 'ARM: tegra: don't unlock MMIO access to DBGLAR for working'
> secondary core boot on Tegra114. Otherwise the secondary cores will hang at
> bootup.
> 
Thanks for reminder. This is definitely needed.

Joseph

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> > The Tegra kernel only support boot from DT now. Clean up the PMC driver
> > to support DT only, that includes:
> > 
> > * remove the ifdef of CONFIG_OF
> > * replace the static mapping of PMC addr to map from DT
> > 
> > -#ifdef CONFIG_OF
> >  static const struct of_device_id matches[] __initconst = {
> >  	{ .compatible = "nvidia,tegra20-pmc" },
> >  	{ }
> 
> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> has:
> 
> pmc {
> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> 	reg = <0x7000e400 0x400>;
> };
> 
I think it should be something like below, isn't it?

compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
		"nvidia,tegra20-pmc";

or should we add tegra114 and tegra30 in the DT match table?

Thanks,
Joseph

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> > The Tegra kernel only support boot from DT now. Clean up the PMC driver
> > to support DT only, that includes:
> > 
> > * remove the ifdef of CONFIG_OF
> > * replace the static mapping of PMC addr to map from DT
> > 
> > -#ifdef CONFIG_OF
> >  static const struct of_device_id matches[] __initconst = {
> >  	{ .compatible = "nvidia,tegra20-pmc" },
> >  	{ }
> 
> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> has:
> 
> pmc {
> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> 	reg = <0x7000e400 0x400>;
> };
> 
I think it should be something like below, isn't it?

compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
		"nvidia,tegra20-pmc";

or should we add tegra114 and tegra30 in the DT match table?

Thanks,
Joseph

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> > 
> > Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
> >  arch/arm/mach-tegra/pmc.h |   4 ++
> >  2 files changed, 107 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > index 2315e25..a2446c2 100644
> > --- a/arch/arm/mach-tegra/pmc.c
> > +++ b/arch/arm/mach-tegra/pmc.c
> > @@ -21,8 +21,26 @@
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> >  
> > -#define PMC_CTRL		0x0
> > -#define PMC_CTRL_INTR_LOW	(1 << 17)
> > +#define PMC_CTRL			0x0
> > +#define PMC_CTRL_INTR_LOW		(1 << 17)
> > +#define PMC_PWRGATE_TOGGLE		0x30
> > +#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
> > +#define PMC_REMOVE_CLAMPING		0x34
> > +#define PMC_PWRGATE_STATUS		0x38
> > +
> > +#define TEGRA_POWERGATE_PCIE	3
> > +#define TEGRA_POWERGATE_VDEC	4
> > +#define TEGRA_POWERGATE_CPU1	9
> > +#define TEGRA_POWERGATE_CPU2	10
> > +#define TEGRA_POWERGATE_CPU3	11
> > +
> > +static u8 tegra_cpu_domains[] = {
> > +	0xFF,			/* not available for CPU0 */
> 
> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> 
No, DON'T DO THAT!!
We don't allow any code to power gate CPU0 manually here or elsewhere.

The function here only for SMP and hotplug bring up for secondary CPU.

Thanks,
Joseph

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> > 
> > Signed-off-by: Joseph Lo <josephl@nvidia.com>
> > ---
> >  arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-
> >  arch/arm/mach-tegra/pmc.h |   4 ++
> >  2 files changed, 107 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > index 2315e25..a2446c2 100644
> > --- a/arch/arm/mach-tegra/pmc.c
> > +++ b/arch/arm/mach-tegra/pmc.c
> > @@ -21,8 +21,26 @@
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> >  
> > -#define PMC_CTRL		0x0
> > -#define PMC_CTRL_INTR_LOW	(1 << 17)
> > +#define PMC_CTRL			0x0
> > +#define PMC_CTRL_INTR_LOW		(1 << 17)
> > +#define PMC_PWRGATE_TOGGLE		0x30
> > +#define PMC_PWRGATE_TOGGLE_START	(1 << 8)
> > +#define PMC_REMOVE_CLAMPING		0x34
> > +#define PMC_PWRGATE_STATUS		0x38
> > +
> > +#define TEGRA_POWERGATE_PCIE	3
> > +#define TEGRA_POWERGATE_VDEC	4
> > +#define TEGRA_POWERGATE_CPU1	9
> > +#define TEGRA_POWERGATE_CPU2	10
> > +#define TEGRA_POWERGATE_CPU3	11
> > +
> > +static u8 tegra_cpu_domains[] = {
> > +	0xFF,			/* not available for CPU0 */
> 
> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> 
No, DON'T DO THAT!!
We don't allow any code to power gate CPU0 manually here or elsewhere.

The function here only for SMP and hotplug bring up for secondary CPU.

Thanks,
Joseph

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/22/2013 07:03 PM, Joseph Lo wrote:
> On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
>> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
>>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
>>> to support DT only, that includes:
>>>
>>> * remove the ifdef of CONFIG_OF
>>> * replace the static mapping of PMC addr to map from DT
>>>
>>> -#ifdef CONFIG_OF
>>>  static const struct of_device_id matches[] __initconst = {
>>>  	{ .compatible = "nvidia,tegra20-pmc" },
>>>  	{ }
>>
>> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
>> has:
>>
>> pmc {
>> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
>> 	reg = <0x7000e400 0x400>;
>> };
>>
> I think it should be something like below, isn't it?
> 
> compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> 		"nvidia,tegra20-pmc";
> 
> or should we add tegra114 and tegra30 in the DT match table?

The Tegra114 PMC HW is probably not 100% backwards-compatible with
previous SoCs' PMC, so the DT file should probably only list the
specific SoC, and the driver should probably include all the compatible
values it supports.

Peter, can you confirm exactly which HW versions, if any, are 100%
backwards-compatible?

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/22/2013 07:03 PM, Joseph Lo wrote:
> On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
>> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
>>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
>>> to support DT only, that includes:
>>>
>>> * remove the ifdef of CONFIG_OF
>>> * replace the static mapping of PMC addr to map from DT
>>>
>>> -#ifdef CONFIG_OF
>>>  static const struct of_device_id matches[] __initconst = {
>>>  	{ .compatible = "nvidia,tegra20-pmc" },
>>>  	{ }
>>
>> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
>> has:
>>
>> pmc {
>> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
>> 	reg = <0x7000e400 0x400>;
>> };
>>
> I think it should be something like below, isn't it?
> 
> compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> 		"nvidia,tegra20-pmc";
> 
> or should we add tegra114 and tegra30 in the DT match table?

The Tegra114 PMC HW is probably not 100% backwards-compatible with
previous SoCs' PMC, so the DT file should probably only list the
specific SoC, and the driver should probably include all the compatible
values it supports.

Peter, can you confirm exactly which HW versions, if any, are 100%
backwards-compatible?

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/22/2013 07:38 PM, Joseph Lo wrote:
> On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
>> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
>>> Adding the power on function for secondary CPUs in PMC driver, this can
>>> help us to remove legacy powergate driver and add generic power domain
>>> support later.

>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

>>> +static u8 tegra_cpu_domains[] = {
>>> +	0xFF,			/* not available for CPU0 */
>>
>> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
>>
> No, DON'T DO THAT!!
> We don't allow any code to power gate CPU0 manually here or elsewhere.
> 
> The function here only for SMP and hotplug bring up for secondary CPU.

Doesn't Tegra114 have improved HW that does actually allow
power-gating/hot-unplugging/... CPU 0, even though older HW didn't?

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/22/2013 07:38 PM, Joseph Lo wrote:
> On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
>> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
>>> Adding the power on function for secondary CPUs in PMC driver, this can
>>> help us to remove legacy powergate driver and add generic power domain
>>> support later.

>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

>>> +static u8 tegra_cpu_domains[] = {
>>> +	0xFF,			/* not available for CPU0 */
>>
>> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
>>
> No, DON'T DO THAT!!
> We don't allow any code to power gate CPU0 manually here or elsewhere.
> 
> The function here only for SMP and hotplug bring up for secondary CPU.

Doesn't Tegra114 have improved HW that does actually allow
power-gating/hot-unplugging/... CPU 0, even though older HW didn't?

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote:
> On 02/22/2013 07:38 PM, Joseph Lo wrote:
> > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> >>> Adding the power on function for secondary CPUs in PMC driver, this can
> >>> help us to remove legacy powergate driver and add generic power domain
> >>> support later.
> 
> >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> >>> +static u8 tegra_cpu_domains[] = {
> >>> +	0xFF,			/* not available for CPU0 */
> >>
> >> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> >>
> > No, DON'T DO THAT!!
> > We don't allow any code to power gate CPU0 manually here or elsewhere.
> > 
> > The function here only for SMP and hotplug bring up for secondary CPU.
> 
> Doesn't Tegra114 have improved HW that does actually allow
> power-gating/hot-unplugging/... CPU 0, even though older HW didn't?

Indeed. It support CPU0 be power gated. But the power up sequence does
not through here by toggling PMC directly. Just like the CPU idle
"powered-down" mode. The CPU0's power gating/un-gating sequence should
set up some event flags in flow-controller and controlled by
flow-controller.

Thanks,
Joseph

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote:
> On 02/22/2013 07:38 PM, Joseph Lo wrote:
> > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> >>> Adding the power on function for secondary CPUs in PMC driver, this can
> >>> help us to remove legacy powergate driver and add generic power domain
> >>> support later.
> 
> >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> >>> +static u8 tegra_cpu_domains[] = {
> >>> +	0xFF,			/* not available for CPU0 */
> >>
> >> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> >>
> > No, DON'T DO THAT!!
> > We don't allow any code to power gate CPU0 manually here or elsewhere.
> > 
> > The function here only for SMP and hotplug bring up for secondary CPU.
> 
> Doesn't Tegra114 have improved HW that does actually allow
> power-gating/hot-unplugging/... CPU 0, even though older HW didn't?

Indeed. It support CPU0 be power gated. But the power up sequence does
not through here by toggling PMC directly. Just like the CPU idle
"powered-down" mode. The CPU0's power gating/un-gating sequence should
set up some event flags in flow-controller and controlled by
flow-controller.

Thanks,
Joseph

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
> On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
> > +{
> > +	if (cpuid <= 0 || cpuid > num_possible_cpus())
> 
> cpuid >= num_possible_cpus()?
> 
Yes.

> > +static int tegra_pmc_powergate_set(int id, bool new_state)
> > +{
> > +	bool status;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&tegra_powergate_lock, flags);
> > +
> > +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
> 
> I would perform the read and the logical operations separately. Same for
> the write below.
> 
> Don't you want to and with ~BIT(id) not BIT(id)?
> 
This is what I want.
WARN_ON(!(!!(status & BIT(id)) ^ new_state));

> > +static int tegra_pmc_powergate_remove_clamping(int id)
> > +{
> > +	u32 mask;
> > +
> > +	/*
> > +	 * Tegra 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);
> 
> Is this just true for this one register, but not others? If it's true
> everywhere, why not just fix the TEGRA_POWERGATE_* definitions?
> 
> I asked this downstream, but you didn't answer.
> 
This is because the bit of the powergate id was swapped in
PMC_REMOVING_CLAMPING with others. So we only need a fix here.

> > +bool tegra_pmc_cpu_is_powered(int cpuid)
> > +{
> > +	int id;
> > +
> > +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> > +	if (IS_ERR_VALUE(id))
> > +		return false;
> 
> As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
> intended for use on error-pointers, not on integer error codes.

IS_ERR is for error pointers, IS_ERR_VALUE is for error return code,
isn't it?

Thanks,
Joseph

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Joseph Lo]

On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
> On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
> > +{
> > +	if (cpuid <= 0 || cpuid > num_possible_cpus())
> 
> cpuid >= num_possible_cpus()?
> 
Yes.

> > +static int tegra_pmc_powergate_set(int id, bool new_state)
> > +{
> > +	bool status;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&tegra_powergate_lock, flags);
> > +
> > +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
> 
> I would perform the read and the logical operations separately. Same for
> the write below.
> 
> Don't you want to and with ~BIT(id) not BIT(id)?
> 
This is what I want.
WARN_ON(!(!!(status & BIT(id)) ^ new_state));

> > +static int tegra_pmc_powergate_remove_clamping(int id)
> > +{
> > +	u32 mask;
> > +
> > +	/*
> > +	 * Tegra 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);
> 
> Is this just true for this one register, but not others? If it's true
> everywhere, why not just fix the TEGRA_POWERGATE_* definitions?
> 
> I asked this downstream, but you didn't answer.
> 
This is because the bit of the powergate id was swapped in
PMC_REMOVING_CLAMPING with others. So we only need a fix here.

> > +bool tegra_pmc_cpu_is_powered(int cpuid)
> > +{
> > +	int id;
> > +
> > +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> > +	if (IS_ERR_VALUE(id))
> > +		return false;
> 
> As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
> intended for use on error-pointers, not on integer error codes.

IS_ERR is for error pointers, IS_ERR_VALUE is for error return code,
isn't it?

Thanks,
Joseph

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Russell King - ARM Linux]

On Sat, Feb 23, 2013 at 03:28:00PM +0800, Joseph Lo wrote:
> On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
> > On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > > +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> > > +	if (IS_ERR_VALUE(id))
> > > +		return false;
> > 
> > As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
> > intended for use on error-pointers, not on integer error codes.
> 
> IS_ERR is for error pointers, IS_ERR_VALUE is for error return code,
> isn't it?

No.  Just because there's a macro somewhere doesn't mean it should be used.

The err.h stuff is only there to deal with functions which can return
kernel pointer values and error values, and resolve the conflict between
negative integers and pointers which also appear to be negative integers.

If you don't have that conflict, then making use of err.h is pointless
and is pure obfuscation.

So, just use the plain:

	if (id < 0)
		return false;

here.

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Russell King - ARM Linux]

On Sat, Feb 23, 2013 at 03:28:00PM +0800, Joseph Lo wrote:
> On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
> > On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > > +	id = tegra_pmc_get_cpu_powerdomain_id(cpuid);
> > > +	if (IS_ERR_VALUE(id))
> > > +		return false;
> > 
> > As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is
> > intended for use on error-pointers, not on integer error codes.
> 
> IS_ERR is for error pointers, IS_ERR_VALUE is for error return code,
> isn't it?

No.  Just because there's a macro somewhere doesn't mean it should be used.

The err.h stuff is only there to deal with functions which can return
kernel pointer values and error values, and resolve the conflict between
negative integers and pointers which also appear to be negative integers.

If you don't have that conflict, then making use of err.h is pointless
and is pure obfuscation.

So, just use the plain:

	if (id < 0)
		return false;

here.

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Sat, Feb 23, 2013 at 05:59:33AM +0100, Joseph Lo wrote:
> On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote:
> > On 02/22/2013 07:38 PM, Joseph Lo wrote:
> > > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> > >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> > >>> Adding the power on function for secondary CPUs in PMC driver, this can
> > >>> help us to remove legacy powergate driver and add generic power domain
> > >>> support later.
> > 
> > >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > 
> > >>> +static u8 tegra_cpu_domains[] = {
> > >>> +	0xFF,			/* not available for CPU0 */
> > >>
> > >> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> > >>
> > > No, DON'T DO THAT!!
> > > We don't allow any code to power gate CPU0 manually here or elsewhere.
> > > 
> > > The function here only for SMP and hotplug bring up for secondary CPU.
> > 
> > Doesn't Tegra114 have improved HW that does actually allow
> > power-gating/hot-unplugging/... CPU 0, even though older HW didn't?
> 
> Indeed. It support CPU0 be power gated. But the power up sequence does
> not through here by toggling PMC directly. Just like the CPU idle
> "powered-down" mode. The CPU0's power gating/un-gating sequence should
> set up some event flags in flow-controller and controlled by
> flow-controller.

Yes, obviously. We don't use this for powergating cores normally...
Sorry for the confusion.

Cheers,

Peter.

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Sat, Feb 23, 2013 at 05:59:33AM +0100, Joseph Lo wrote:
> On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote:
> > On 02/22/2013 07:38 PM, Joseph Lo wrote:
> > > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote:
> > >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote:
> > >>> Adding the power on function for secondary CPUs in PMC driver, this can
> > >>> help us to remove legacy powergate driver and add generic power domain
> > >>> support later.
> > 
> > >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> > 
> > >>> +static u8 tegra_cpu_domains[] = {
> > >>> +	0xFF,			/* not available for CPU0 */
> > >>
> > >> On Tegra114 we can also powergate CPU0, so this needs to be defined then.
> > >>
> > > No, DON'T DO THAT!!
> > > We don't allow any code to power gate CPU0 manually here or elsewhere.
> > > 
> > > The function here only for SMP and hotplug bring up for secondary CPU.
> > 
> > Doesn't Tegra114 have improved HW that does actually allow
> > power-gating/hot-unplugging/... CPU 0, even though older HW didn't?
> 
> Indeed. It support CPU0 be power gated. But the power up sequence does
> not through here by toggling PMC directly. Just like the CPU idle
> "powered-down" mode. The CPU0's power gating/un-gating sequence should
> set up some event flags in flow-controller and controlled by
> flow-controller.

Yes, obviously. We don't use this for powergating cores normally...
Sorry for the confusion.

Cheers,

Peter.

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/23/2013 12:28 AM, Joseph Lo wrote:
> On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
>> On 02/21/2013 11:44 PM, Joseph Lo wrote:
>>> Adding the power on function for secondary CPUs in PMC driver, this can
>>> help us to remove legacy powergate driver and add generic power domain
>>> support later.
>>
>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
>>> +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
>>> +{
>>> +	if (cpuid <= 0 || cpuid > num_possible_cpus())
>>
>> cpuid >= num_possible_cpus()?
>>
> Yes.
> 
>>> +static int tegra_pmc_powergate_set(int id, bool new_state)
>>> +{
>>> +	bool status;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&tegra_powergate_lock, flags);
>>> +
>>> +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
>>
>> I would perform the read and the logical operations separately. Same for
>> the write below.
>>
>> Don't you want to and with ~BIT(id) not BIT(id)?
>>
> This is what I want.
> WARN_ON(!(!!(status & BIT(id)) ^ new_state));

Oh sorry, I guess this isn't a standard read-modify-write cycle, but
something else.

So yes, the & in the register read is probably fine as you have it, but
yes there is an issue in the current WARN_ON comparison, as you say.

The WARN_ON you gave above is rather complex. Simplest might be:

u32 reg = tegra_pmc_readl(PMC_PWRGATE_STATUS);
bool old_state = (reg >> id) & 1;
WARN_ON(new_state == old_state);

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Stephen Warren]

On 02/23/2013 12:28 AM, Joseph Lo wrote:
> On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote:
>> On 02/21/2013 11:44 PM, Joseph Lo wrote:
>>> Adding the power on function for secondary CPUs in PMC driver, this can
>>> help us to remove legacy powergate driver and add generic power domain
>>> support later.
>>
>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
>>> +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid)
>>> +{
>>> +	if (cpuid <= 0 || cpuid > num_possible_cpus())
>>
>> cpuid >= num_possible_cpus()?
>>
> Yes.
> 
>>> +static int tegra_pmc_powergate_set(int id, bool new_state)
>>> +{
>>> +	bool status;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&tegra_powergate_lock, flags);
>>> +
>>> +	status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id);
>>
>> I would perform the read and the logical operations separately. Same for
>> the write below.
>>
>> Don't you want to and with ~BIT(id) not BIT(id)?
>>
> This is what I want.
> WARN_ON(!(!!(status & BIT(id)) ^ new_state));

Oh sorry, I guess this isn't a standard read-modify-write cycle, but
something else.

So yes, the & in the register read is probably fine as you have it, but
yes there is an issue in the current WARN_ON comparison, as you say.

The WARN_ON you gave above is rather complex. Simplest might be:

u32 reg = tegra_pmc_readl(PMC_PWRGATE_STATUS);
bool old_state = (reg >> id) & 1;
WARN_ON(new_state == old_state);

Re: [PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:37:06PM +0100, Stephen Warren wrote:
> On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> > +static u8 tegra_cpu_domains[] = {
> > +	0xFF,			/* not available for CPU0 */
> > +	TEGRA_POWERGATE_CPU1,
> > +	TEGRA_POWERGATE_CPU2,
> > +	TEGRA_POWERGATE_CPU3,
> > +};
> 
> Per Peter's comment, you probably need SoC-specific arrays here, to
> support CPU0 having a valid value or not.

That was a mistake, we never use these functions on CPU0. Powergating uses
a different path. So this is ok.

Cheers,

Peter.

[PATCH 2/4] ARM: tegra: pmc: add power on function for secondary CPUs

[Peter De Schrijver]

On Fri, Feb 22, 2013 at 07:37:06PM +0100, Stephen Warren wrote:
> On 02/21/2013 11:44 PM, Joseph Lo wrote:
> > Adding the power on function for secondary CPUs in PMC driver, this can
> > help us to remove legacy powergate driver and add generic power domain
> > support later.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> > +static u8 tegra_cpu_domains[] = {
> > +	0xFF,			/* not available for CPU0 */
> > +	TEGRA_POWERGATE_CPU1,
> > +	TEGRA_POWERGATE_CPU2,
> > +	TEGRA_POWERGATE_CPU3,
> > +};
> 
> Per Peter's comment, you probably need SoC-specific arrays here, to
> support CPU0 having a valid value or not.

That was a mistake, we never use these functions on CPU0. Powergating uses
a different path. So this is ok.

Cheers,

Peter.

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Peter De Schrijver]

On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> On 02/22/2013 07:03 PM, Joseph Lo wrote:
> > On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> >> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> >>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> >>> to support DT only, that includes:
> >>>
> >>> * remove the ifdef of CONFIG_OF
> >>> * replace the static mapping of PMC addr to map from DT
> >>>
> >>> -#ifdef CONFIG_OF
> >>>  static const struct of_device_id matches[] __initconst = {
> >>>  	{ .compatible = "nvidia,tegra20-pmc" },
> >>>  	{ }
> >>
> >> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> >> has:
> >>
> >> pmc {
> >> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> >> 	reg = <0x7000e400 0x400>;
> >> };
> >>
> > I think it should be something like below, isn't it?
> > 
> > compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> > 		"nvidia,tegra20-pmc";
> > 
> > or should we add tegra114 and tegra30 in the DT match table?
> 
> The Tegra114 PMC HW is probably not 100% backwards-compatible with
> previous SoCs' PMC, so the DT file should probably only list the
> specific SoC, and the driver should probably include all the compatible
> values it supports.
> 
> Peter, can you confirm exactly which HW versions, if any, are 100%
> backwards-compatible?
> 

The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
are differences in the CPU domains and 3D. The CPU domains aren't a problem I
think because we won't powergate those domains directly using the PMC. We
always program the flowcontroller to do the powergating on WFI. 3D is a
different story however. We do powergate 3D under software control and the
sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
domains for 3D while Tegra114 has only 1. Then ofcourse Tegra114 lacks the
SATA and PCIe domains because those features are missing compared to Tegra30.
But the IDs haven't been reused. All in all I think it's best to explicitly
require the driver to support the various SoCs.

Cheers,

Peter.

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Peter De Schrijver]

On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> On 02/22/2013 07:03 PM, Joseph Lo wrote:
> > On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> >> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> >>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> >>> to support DT only, that includes:
> >>>
> >>> * remove the ifdef of CONFIG_OF
> >>> * replace the static mapping of PMC addr to map from DT
> >>>
> >>> -#ifdef CONFIG_OF
> >>>  static const struct of_device_id matches[] __initconst = {
> >>>  	{ .compatible = "nvidia,tegra20-pmc" },
> >>>  	{ }
> >>
> >> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> >> has:
> >>
> >> pmc {
> >> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> >> 	reg = <0x7000e400 0x400>;
> >> };
> >>
> > I think it should be something like below, isn't it?
> > 
> > compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> > 		"nvidia,tegra20-pmc";
> > 
> > or should we add tegra114 and tegra30 in the DT match table?
> 
> The Tegra114 PMC HW is probably not 100% backwards-compatible with
> previous SoCs' PMC, so the DT file should probably only list the
> specific SoC, and the driver should probably include all the compatible
> values it supports.
> 
> Peter, can you confirm exactly which HW versions, if any, are 100%
> backwards-compatible?
> 

The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
are differences in the CPU domains and 3D. The CPU domains aren't a problem I
think because we won't powergate those domains directly using the PMC. We
always program the flowcontroller to do the powergating on WFI. 3D is a
different story however. We do powergate 3D under software control and the
sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
domains for 3D while Tegra114 has only 1. Then ofcourse Tegra114 lacks the
SATA and PCIe domains because those features are missing compared to Tegra30.
But the IDs haven't been reused. All in all I think it's best to explicitly
require the driver to support the various SoCs.

Cheers,

Peter.

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
...
>> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
>> backwards-compatible?
>>
> 
> The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> think because we won't powergate those domains directly using the PMC. We
> always program the flowcontroller to do the powergating on WFI. 3D is a
> different story however. We do powergate 3D under software control and the
> sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> SATA and PCIe domains because those features are missing compared to Tegra30.
> But the IDs haven't been reused. All in all I think it's best to explicitly
> require the driver to support the various SoCs.

That does sound like a good idea; a separate compatible value for each SoC.

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Stephen Warren]

On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
...
>> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
>> backwards-compatible?
>>
> 
> The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> think because we won't powergate those domains directly using the PMC. We
> always program the flowcontroller to do the powergating on WFI. 3D is a
> different story however. We do powergate 3D under software control and the
> sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> SATA and PCIe domains because those features are missing compared to Tegra30.
> But the IDs haven't been reused. All in all I think it's best to explicitly
> require the driver to support the various SoCs.

That does sound like a good idea; a separate compatible value for each SoC.

Re: [PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

On Mon, 2013-02-25 at 23:43 +0800, Stephen Warren wrote:
> On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> > On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> ...
> >> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
> >> backwards-compatible?
> >>
> > 
> > The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> > are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> > are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> > think because we won't powergate those domains directly using the PMC. We
> > always program the flowcontroller to do the powergating on WFI. 3D is a
> > different story however. We do powergate 3D under software control and the
> > sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> > domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> > SATA and PCIe domains because those features are missing compared to Tegra30.
> > But the IDs haven't been reused. All in all I think it's best to explicitly
> > require the driver to support the various SoCs.
> 
> That does sound like a good idea; a separate compatible value for each SoC.

OK. Will add another patch to fix the PMC compatibility in Tegra30 and
Tegra114 dts file. And also in the DT match table of PMC driver.

Thanks,
Joseph

[PATCH 1/4] ARM: tegra: pmc: convert PMC driver to support DT only

[Joseph Lo]

On Mon, 2013-02-25 at 23:43 +0800, Stephen Warren wrote:
> On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> > On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> ...
> >> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
> >> backwards-compatible?
> >>
> > 
> > The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> > are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> > are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> > think because we won't powergate those domains directly using the PMC. We
> > always program the flowcontroller to do the powergating on WFI. 3D is a
> > different story however. We do powergate 3D under software control and the
> > sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> > domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> > SATA and PCIe domains because those features are missing compared to Tegra30.
> > But the IDs haven't been reused. All in all I think it's best to explicitly
> > require the driver to support the various SoCs.
> 
> That does sound like a good idea; a separate compatible value for each SoC.

OK. Will add another patch to fix the PMC compatibility in Tegra30 and
Tegra114 dts file. And also in the DT match table of PMC driver.

Thanks,
Joseph