[PATCH 1/3] ARM: tegra: Fix unchecked return value

[PATCH 1/3] ARM: tegra: Fix unchecked return value

[Joseph Lo]

From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Check a return value for tegra_powergate_remove_clamping().

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

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 2c6b3d5..4dfc75e 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -124,6 +124,9 @@ remove_clamps:
 
 	/* Remove I/O clamps. */
 	ret = tegra_powergate_remove_clamping(pwrgateid);
+	if (ret)
+		return ret;
+
 	udelay(10);
 
 	/* Clear flow controller CSR. */
-- 
1.8.1.1

[PATCH 2/3] ARM: tegra30: fix the logical detection of power on sequence of warm boot CPUs

[Joseph Lo]

The warm boo sequence of secondary CPUs should wait for the power ready
then removing the clamps.

This did not fix any known or unknown issue, but nice to have this fix.

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

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index 4dfc75e..e78d52d 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -91,7 +91,7 @@ static int tegra30_power_up_cpu(unsigned int cpu)
 	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
 		timeout = jiffies + msecs_to_jiffies(50);
 		do {
-			if (!tegra_powergate_is_powered(pwrgateid))
+			if (tegra_powergate_is_powered(pwrgateid))
 				goto remove_clamps;
 			udelay(10);
 		} while (time_before(jiffies, timeout));
-- 
1.8.1.1

[PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary

[Joseph Lo]

From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

"tegra_boot_secondary()" has many condition branches for some Tegra
SoC generations in a single function so that it's not easy to compile
a kernel only for a single SoC if one wants with some reason, debug
purpose(?). This patch provides SoC specific version of
boot_secondary(), tegra{20,30}_boot_secondary(). This could allow
any combination of SoC to be built. Those boot_secondary functions can
be preparation when we ntroduce chip specific function pointers in the
future without having chip dependent branches around.

Also removed unused definition/prototpye.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[josephl: remove the Tegra114 part of the original patch]
Signed-off-by: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/platsmp.c | 93 ++++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 55 deletions(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index e78d52d..41971ac 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -35,13 +35,8 @@
 #include "common.h"
 #include "iomap.h"
 
-extern void tegra_secondary_startup(void);
-
 static cpumask_t tegra_cpu_init_mask;
 
-#define EVP_CPU_RESET_VECTOR \
-	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)
-
 static void __cpuinit tegra_secondary_init(unsigned int cpu)
 {
 	/*
@@ -54,26 +49,48 @@ static void __cpuinit tegra_secondary_init(unsigned int cpu)
 	cpumask_set_cpu(cpu, &tegra_cpu_init_mask);
 }
 
-static int tegra20_power_up_cpu(unsigned int cpu)
+
+static int tegra20_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	/* Enable the CPU clock. */
-	tegra_enable_cpu_clock(cpu);
+	cpu = cpu_logical_map(cpu);
 
-	/* Clear flow controller CSR. */
-	flowctrl_write_cpu_csr(cpu, 0);
+	/*
+	 * Force the CPU into reset. The CPU must remain in reset when
+	 * the flow controller state is cleared (which will cause the
+	 * flow controller to stop driving reset if the CPU has been
+	 * power-gated via the flow controller). This will have no
+	 * effect on first boot of the CPU since it should already be
+	 * in reset.
+	 */
+	tegra_put_cpu_in_reset(cpu);
 
+	/*
+	 * Unhalt the CPU. If the flow controller was used to
+	 * power-gate the CPU this will cause the flow controller to
+	 * stop driving reset. The CPU will remain in reset because the
+	 * clock and reset block is now driving reset.
+	 */
+	flowctrl_write_cpu_halt(cpu, 0);
+
+	tegra_enable_cpu_clock(cpu);
+	flowctrl_write_cpu_csr(cpu, 0); /* Clear flow controller CSR. */
+	tegra_cpu_out_of_reset(cpu);
 	return 0;
 }
 
-static int tegra30_power_up_cpu(unsigned int cpu)
+static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	int ret, pwrgateid;
 	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);
+
 	/*
 	 * The power up sequence of cold boot CPU and warm boot CPU
 	 * was different.
@@ -85,7 +102,7 @@ static int tegra30_power_up_cpu(unsigned int cpu)
 	 * the IO clamps.
 	 * For cold boot CPU, do not wait. After the cold boot CPU be
 	 * booted, it will run to tegra_secondary_init() and set
-	 * tegra_cpu_init_mask which influences what tegra30_power_up_cpu()
+	 * tegra_cpu_init_mask which influences what tegra30_boot_secondary()
 	 * next time around.
 	 */
 	if (cpumask_test_cpu(cpu, &tegra_cpu_init_mask)) {
@@ -129,54 +146,20 @@ remove_clamps:
 
 	udelay(10);
 
-	/* Clear flow controller CSR. */
-	flowctrl_write_cpu_csr(cpu, 0);
-
+	flowctrl_write_cpu_csr(cpu, 0); /* Clear flow controller CSR. */
+	tegra_cpu_out_of_reset(cpu);
 	return 0;
 }
 
-static int __cpuinit tegra_boot_secondary(unsigned int cpu, struct task_struct *idle)
+static int __cpuinit tegra_boot_secondary(unsigned int cpu,
+					  struct task_struct *idle)
 {
-	int status;
-
-	cpu = cpu_logical_map(cpu);
-
-	/*
-	 * Force the CPU into reset. The CPU must remain in reset when the
-	 * flow controller state is cleared (which will cause the flow
-	 * controller to stop driving reset if the CPU has been power-gated
-	 * via the flow controller). This will have no effect on first boot
-	 * of the CPU since it should already be in reset.
-	 */
-	tegra_put_cpu_in_reset(cpu);
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_2x_SOC) && tegra_chip_id == TEGRA20)
+		return tegra20_boot_secondary(cpu, idle);
+	if (IS_ENABLED(CONFIG_ARCH_TEGRA_3x_SOC) && tegra_chip_id == TEGRA30)
+		return tegra30_boot_secondary(cpu, idle);
 
-	/*
-	 * Unhalt the CPU. If the flow controller was used to power-gate the
-	 * CPU this will cause the flow controller to stop driving reset.
-	 * The CPU will remain in reset because the clock and reset block
-	 * is now driving reset.
-	 */
-	flowctrl_write_cpu_halt(cpu, 0);
-
-	switch (tegra_chip_id) {
-	case TEGRA20:
-		status = tegra20_power_up_cpu(cpu);
-		break;
-	case TEGRA30:
-		status = tegra30_power_up_cpu(cpu);
-		break;
-	default:
-		status = -EINVAL;
-		break;
-	}
-
-	if (status)
-		goto done;
-
-	/* Take the CPU out of reset. */
-	tegra_cpu_out_of_reset(cpu);
-done:
-	return status;
+	return -EINVAL;
 }
 
 static void __init tegra_smp_prepare_cpus(unsigned int max_cpus)
-- 
1.8.1.1

Re: [PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary

[Stephen Warren]

On 02/21/2013 11:24 PM, Joseph Lo wrote:
> From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> "tegra_boot_secondary()" has many condition branches for some Tegra
> SoC generations in a single function so that it's not easy to compile
> a kernel only for a single SoC if one wants with some reason, debug
> purpose(?). This patch provides SoC specific version of
> boot_secondary(), tegra{20,30}_boot_secondary(). This could allow
> any combination of SoC to be built. Those boot_secondary functions can
> be preparation when we ntroduce chip specific function pointers in the
> future without having chip dependent branches around.
> 
> Also removed unused definition/prototpye.

That "also" is really something that should be a separate patch, since
it's not related to the code refactoring that's the main purpose of this
patch.

However, I'll let it slide this time, since I think both patches would
just end up in Tegra's cleanup branch anyway, even though I did already
point this out (multiple times?) during downstream review:-( You're
getting lucky because I don't feel like reviewing this again.

I'll apply this series once I can apply patches for 3.10.

One note to anyone else reading this patch: It does look like this is
duplicating code that was previously nicely shared in
tegra_boot_secondary(). However, I believe it's appropriate to do this
in this case, since the equivalent code for future SoCs (such as
Tegra114) is extremely different, and so the current shared code won't
end up being shared, and this would just make tegra_boot_secondary()
over-complex with conditionals when adding Tegra114 support.

Re: [PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary

[Joseph Lo]

On Sat, 2013-02-23 at 02:28 +0800, Stephen Warren wrote:
> On 02/21/2013 11:24 PM, Joseph Lo wrote:
> > From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > "tegra_boot_secondary()" has many condition branches for some Tegra
> > SoC generations in a single function so that it's not easy to compile
> > a kernel only for a single SoC if one wants with some reason, debug
> > purpose(?). This patch provides SoC specific version of
> > boot_secondary(), tegra{20,30}_boot_secondary(). This could allow
> > any combination of SoC to be built. Those boot_secondary functions can
> > be preparation when we ntroduce chip specific function pointers in the
> > future without having chip dependent branches around.
> > 
> > Also removed unused definition/prototpye.
> 
> That "also" is really something that should be a separate patch, since
> it's not related to the code refactoring that's the main purpose of this
> patch.
> 
> However, I'll let it slide this time, since I think both patches would
> just end up in Tegra's cleanup branch anyway, even though I did already
> point this out (multiple times?) during downstream review:-( You're
> getting lucky because I don't feel like reviewing this again.
> 
> I'll apply this series once I can apply patches for 3.10.
> 
> One note to anyone else reading this patch: It does look like this is
> duplicating code that was previously nicely shared in
> tegra_boot_secondary(). However, I believe it's appropriate to do this
> in this case, since the equivalent code for future SoCs (such as
> Tegra114) is extremely different, and so the current shared code won't
> end up being shared, and this would just make tegra_boot_secondary()
> over-complex with conditionals when adding Tegra114 support.

Hiroshi,

Per Stephen's comment, should we drop this patch? And refactoring this
later when I add support for Tegra114 CPU PM function.

How do you think? If no, I found a redundant blank line in this patch
that need a V2 to fix.

Thanks,
Joseph

Re: [PATCH 3/3] ARM: tegra: refactor tegra{20,30}_boot_secondary

[Stephen Warren]

On 02/22/2013 09:06 PM, Joseph Lo wrote:
> On Sat, 2013-02-23 at 02:28 +0800, Stephen Warren wrote:
>> On 02/21/2013 11:24 PM, Joseph Lo wrote:
>>> From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> "tegra_boot_secondary()" has many condition branches for some Tegra
>>> SoC generations in a single function so that it's not easy to compile
>>> a kernel only for a single SoC if one wants with some reason, debug
>>> purpose(?). This patch provides SoC specific version of
>>> boot_secondary(), tegra{20,30}_boot_secondary(). This could allow
>>> any combination of SoC to be built. Those boot_secondary functions can
>>> be preparation when we ntroduce chip specific function pointers in the
>>> future without having chip dependent branches around.
>>>
>>> Also removed unused definition/prototpye.
>>
>> That "also" is really something that should be a separate patch, since
>> it's not related to the code refactoring that's the main purpose of this
>> patch.
>>
>> However, I'll let it slide this time, since I think both patches would
>> just end up in Tegra's cleanup branch anyway, even though I did already
>> point this out (multiple times?) during downstream review:-( You're
>> getting lucky because I don't feel like reviewing this again.
>>
>> I'll apply this series once I can apply patches for 3.10.
>>
>> One note to anyone else reading this patch: It does look like this is
>> duplicating code that was previously nicely shared in
>> tegra_boot_secondary(). However, I believe it's appropriate to do this
>> in this case, since the equivalent code for future SoCs (such as
>> Tegra114) is extremely different, and so the current shared code won't
>> end up being shared, and this would just make tegra_boot_secondary()
>> over-complex with conditionals when adding Tegra114 support.
> 
> Hiroshi,
> 
> Per Stephen's comment, should we drop this patch? And refactoring this
> later when I add support for Tegra114 CPU PM function.

Well I did say it wasn't worth reposting for this.

But either way, I wasn't saying anything at all about dropping the
patch, just that the patch should have been two separate patches since
it really does two separate things.

Re: [PATCH 1/3] ARM: tegra: Fix unchecked return value

[Stephen Warren]

On 02/21/2013 11:24 PM, Joseph Lo wrote:
> Check a return value for tegra_powergate_remove_clamping().

I have applied patches 1/3 and 2/3 to Tegra's for-3.10/fixes, and 3/3 to
Tegra's for-3.10/cleanup branches.