| Message ID | 1402442827-2321-1-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chanwoo, On 11.06.2014 01:27, Chanwoo Choi wrote: > This patch set AUTOWAKEUP_EN bit to ARM_CORE_CONFIGURATION register > because Exynos3250 removes WFE in secure mode so that turn on automatically > after setting CORE_LOCAL_PWR_EN. Also, This patch use dbs_sev() macro > to guarantee the data synchronization of command instead of IPI_WAKEUP > because Exynos3250 don't have WFE mode in secue mode. > > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > arch/arm/mach-exynos/platsmp.c | 9 ++++++++- > arch/arm/mach-exynos/pm.c | 8 ++++++-- > arch/arm/mach-exynos/regs-pmu.h | 4 ++++ > 3 files changed, 18 insertions(+), 3 deletions(-) > This patch seems to be unneeded with Krzysztof's patch send a while ago [1]. As reported by Krzysztof, that patch apparently fixes SMP support on Exynos3250 and is much smaller and less invasive. [1] - http://thread.gmane.org/gmane.linux.kernel.samsung-soc/32809 Best regards, Tomasz
On 06/11/2014 08:35 AM, Tomasz Figa wrote: > Hi Chanwoo, > > On 11.06.2014 01:27, Chanwoo Choi wrote: >> This patch set AUTOWAKEUP_EN bit to ARM_CORE_CONFIGURATION register >> because Exynos3250 removes WFE in secure mode so that turn on automatically >> after setting CORE_LOCAL_PWR_EN. Also, This patch use dbs_sev() macro >> to guarantee the data synchronization of command instead of IPI_WAKEUP >> because Exynos3250 don't have WFE mode in secue mode. >> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> arch/arm/mach-exynos/platsmp.c | 9 ++++++++- >> arch/arm/mach-exynos/pm.c | 8 ++++++-- >> arch/arm/mach-exynos/regs-pmu.h | 4 ++++ >> 3 files changed, 18 insertions(+), 3 deletions(-) >> > > This patch seems to be unneeded with Krzysztof's patch send a while ago > [1]. As reported by Krzysztof, that patch apparently fixes SMP support > on Exynos3250 and is much smaller and less invasive. > > [1] - http://thread.gmane.org/gmane.linux.kernel.samsung-soc/32809 OK, But Krzysztof's patch didn't include set S5P_CORE_AUTOWAKEUP_EN in EXYNOS_ARM_CORE_CONFIGURATION(cpu). and then use arch_send_wakeup_ipi_mask(cpumask_of(cpu)) command instead of dsb_sev(). Exynos3250 don't need send IPI message. I'll send next patch which include only S5P_CORE_AUTOWAKEUP_EN and without sending IPI message. Krzysztof's patch used of_machine_is_compatible("samsung,exynos3250") instead of soc_is_exynos3250(). Did you agree? If you agree to use of_machine_is_compatible(), I'll use it on next patch(v2). Best Regards, Chanwoo Choi
On 11.06.2014 01:44, Chanwoo Choi wrote: > On 06/11/2014 08:35 AM, Tomasz Figa wrote: >> Hi Chanwoo, >> >> On 11.06.2014 01:27, Chanwoo Choi wrote: >>> This patch set AUTOWAKEUP_EN bit to ARM_CORE_CONFIGURATION register >>> because Exynos3250 removes WFE in secure mode so that turn on automatically >>> after setting CORE_LOCAL_PWR_EN. Also, This patch use dbs_sev() macro >>> to guarantee the data synchronization of command instead of IPI_WAKEUP >>> because Exynos3250 don't have WFE mode in secue mode. >>> >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >>> --- >>> arch/arm/mach-exynos/platsmp.c | 9 ++++++++- >>> arch/arm/mach-exynos/pm.c | 8 ++++++-- >>> arch/arm/mach-exynos/regs-pmu.h | 4 ++++ >>> 3 files changed, 18 insertions(+), 3 deletions(-) >>> >> >> This patch seems to be unneeded with Krzysztof's patch send a while ago >> [1]. As reported by Krzysztof, that patch apparently fixes SMP support >> on Exynos3250 and is much smaller and less invasive. >> >> [1] - http://thread.gmane.org/gmane.linux.kernel.samsung-soc/32809 > > OK, > But Krzysztof's patch didn't include set S5P_CORE_AUTOWAKEUP_EN in EXYNOS_ARM_CORE_CONFIGURATION(cpu). > and then use arch_send_wakeup_ipi_mask(cpumask_of(cpu)) command instead of dsb_sev(). Exynos3250 don't need > send IPI message. I don't know technical details about CPU boot-up on Exynos3250 as I haven't worked too much with this platform. According to my conversation with Krzysztof, he found S5P_CORE_AUTOWAKEUP_EN and dsb_sev() to be not needed. Instead S5P_CORE_WAKEUP_FROM_LOCAL_CFG can be set in EXYNOS_ARM_CORE1_STATUS and then normal arch_send_wakeup_ipi_mask() used. He might be able to provide more details. > > I'll send next patch which include only S5P_CORE_AUTOWAKEUP_EN and without sending IPI message. I believe Krzysztof was first with this kind of patch and I'd prefer his approach as it introduces less changes. (First version posted on 15.05 [1], with my comments addressed in v2.) [1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/31614 > > Krzysztof's patch used of_machine_is_compatible("samsung,exynos3250") instead of soc_is_exynos3250(). > Did you agree? If you agree to use of_machine_is_compatible(), I'll use it on next patch(v2). IMHO, there is no significant difference between those two. Both are bad, because they bind quirks to specific SoCs and it's hard to reuse them for new ones - every time a quirk is reused by a new SoC, a new (|| soc_is_xxx() or || of_machine_is_compatible("xxx")) must be added. Best regards, Tomasz
On ?ro, 2014-06-11 at 01:57 +0200, Tomasz Figa wrote: > On 11.06.2014 01:44, Chanwoo Choi wrote: > > On 06/11/2014 08:35 AM, Tomasz Figa wrote: > >> Hi Chanwoo, > >> > >> On 11.06.2014 01:27, Chanwoo Choi wrote: > >>> This patch set AUTOWAKEUP_EN bit to ARM_CORE_CONFIGURATION register > >>> because Exynos3250 removes WFE in secure mode so that turn on automatically > >>> after setting CORE_LOCAL_PWR_EN. Also, This patch use dbs_sev() macro > >>> to guarantee the data synchronization of command instead of IPI_WAKEUP > >>> because Exynos3250 don't have WFE mode in secue mode. > >>> > >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > >>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > >>> --- > >>> arch/arm/mach-exynos/platsmp.c | 9 ++++++++- > >>> arch/arm/mach-exynos/pm.c | 8 ++++++-- > >>> arch/arm/mach-exynos/regs-pmu.h | 4 ++++ > >>> 3 files changed, 18 insertions(+), 3 deletions(-) > >>> > >> > >> This patch seems to be unneeded with Krzysztof's patch send a while ago > >> [1]. As reported by Krzysztof, that patch apparently fixes SMP support > >> on Exynos3250 and is much smaller and less invasive. > >> > >> [1] - http://thread.gmane.org/gmane.linux.kernel.samsung-soc/32809 > > > > OK, > > But Krzysztof's patch didn't include set S5P_CORE_AUTOWAKEUP_EN in EXYNOS_ARM_CORE_CONFIGURATION(cpu). > > and then use arch_send_wakeup_ipi_mask(cpumask_of(cpu)) command instead of dsb_sev(). Exynos3250 don't need > > send IPI message. > > I don't know technical details about CPU boot-up on Exynos3250 as I > haven't worked too much with this platform. According to my conversation > with Krzysztof, he found S5P_CORE_AUTOWAKEUP_EN and dsb_sev() to be not > needed. Instead S5P_CORE_WAKEUP_FROM_LOCAL_CFG can be set in > EXYNOS_ARM_CORE1_STATUS and then normal arch_send_wakeup_ipi_mask() > used. He might be able to provide more details. I tried to avoid setting S5P_CORE_AUTOWAKEUP_EN because of simple reason: in my SoC documentation this field is not documented. The ARM_CORE_CONFIGURATION register has only "LOCAL_PWR_CFG" field and nothing more. As for the IPI wakeup - I think it makes code cleaner than adding 'if' statement for specific chipset. Best regards, Krzysztof
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index ec02422..882fb84 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -149,6 +149,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) return -ETIMEDOUT; } } + + if (soc_is_exynos3250()) + __raw_writel(EXYNOS3_COREPORESET(phys_cpu), EXYNOS_SWRESET); + /* * Send the secondary CPU a soft interrupt, thereby causing * the boot monitor to read the system wide flags register, @@ -182,7 +186,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) call_firmware_op(cpu_boot, phys_cpu); - arch_send_wakeup_ipi_mask(cpumask_of(cpu)); + if (soc_is_exynos3250()) + dsb_sev(); + else + arch_send_wakeup_ipi_mask(cpumask_of(cpu)); if (pen_release == -1) break; diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c index 87c0d34..4681f64 100644 --- a/arch/arm/mach-exynos/pm.c +++ b/arch/arm/mach-exynos/pm.c @@ -121,8 +121,12 @@ void exynos_cpu_power_down(int cpu) */ void exynos_cpu_power_up(int cpu) { - __raw_writel(S5P_CORE_LOCAL_PWR_EN, - EXYNOS_ARM_CORE_CONFIGURATION(cpu)); + u32 core_conf = 0; + + core_conf |= S5P_CORE_LOCAL_PWR_EN; + if (soc_is_exynos3250()) + core_conf |= S5P_CORE_AUTOWAKEUP_EN; + __raw_writel(core_conf, EXYNOS_ARM_CORE_CONFIGURATION(cpu)); } /** diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h index 1d13b08..674dfc2 100644 --- a/arch/arm/mach-exynos/regs-pmu.h +++ b/arch/arm/mach-exynos/regs-pmu.h @@ -128,6 +128,7 @@ #define S5P_CORE_LOCAL_PWR_EN 0x3 #define S5P_INT_LOCAL_PWR_EN 0x7 +#define S5P_CORE_AUTOWAKEUP_EN (1 << 31) /* Only for EXYNOS4210 */ #define S5P_CMU_CLKSTOP_LCD1_LOWPWR S5P_PMUREG(0x1154) @@ -186,6 +187,9 @@ #define S5P_DIS_IRQ_CORE3 S5P_PMUREG(0x1034) #define S5P_DIS_IRQ_CENTRAL3 S5P_PMUREG(0x1038) +/* For EXYNOS3 */ +#define EXYNOS3_COREPORESET(cpu) ((1 << 4) << cpu) + /* For EXYNOS5 */ #define EXYNOS5_SYS_I2C_CFG S5P_SYSREG(0x0234)