| Message ID | 1361514267-12111-3-git-send-email-josephl@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/21/2013 11:24 PM, Joseph Lo wrote: > From: Hiroshi Doyu <hdoyu@nvidia.com> > > "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.
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@nvidia.com> > > > > "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
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@nvidia.com> >>> >>> "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.
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)