Re: [PATCH v2 12/15] powerpc/85xx: add time base sync support for e6500

[Scott Wood <scottwood@freescale.com>]

On 04/24/2013 06:29:29 AM, Zhao Chenhui wrote:
> On Tue, Apr 23, 2013 at 07:04:06PM -0500, Scott Wood wrote:
> > On 04/19/2013 05:47:45 AM, Zhao Chenhui wrote:
> > >From: Chen-Hui Zhao <chenhui.zhao@freescale.com>
> > >
> > >For e6500, two threads in one core share one time base. Just need
> > >to do time base sync on first thread of one core, and skip it on
> > >the other thread.
> > >
> > >Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > >Signed-off-by: Li Yang <leoli@freescale.com>
> > >Signed-off-by: Andy Fleming <afleming@freescale.com>
> > >---
> > > arch/powerpc/platforms/85xx/smp.c |   52
> > >+++++++++++++++++++++++++++++++-----
> > > 1 files changed, 44 insertions(+), 8 deletions(-)
> > >
> > >diff --git a/arch/powerpc/platforms/85xx/smp.c
> > >b/arch/powerpc/platforms/85xx/smp.c
> > >index 74d8cde..5f3eee3 100644
> > >--- a/arch/powerpc/platforms/85xx/smp.c
> > >+++ b/arch/powerpc/platforms/85xx/smp.c
> > >@@ -26,6 +26,7 @@
> > > #include <asm/cacheflush.h>
> > > #include <asm/dbell.h>
> > > #include <asm/fsl_guts.h>
> > >+#include <asm/cputhreads.h>
> > >
> > > #include <sysdev/fsl_soc.h>
> > > #include <sysdev/mpic.h>
> > >@@ -45,6 +46,7 @@ static u64 timebase;
> > > static int tb_req;
> > > static int tb_valid;
> > > static u32 cur_booting_core;
> > >+static bool rcpmv2;
> > >
> > > #ifdef CONFIG_PPC_E500MC
> > > /* get a physical mask of online cores and booting core */
> > >@@ -53,26 +55,40 @@ static inline u32 get_phy_cpu_mask(void)
> > > 	u32 mask;
> > > 	int cpu;
> > >
> > >-	mask = 1 << cur_booting_core;
> > >-	for_each_online_cpu(cpu)
> > >-		mask |= 1 << get_hard_smp_processor_id(cpu);
> > >+	if (smt_capable()) {
> > >+		/* two threads in one core share one time base */
> > >+		mask = 1 << cpu_core_index_of_thread(cur_booting_core);
> > >+		for_each_online_cpu(cpu)
> > >+			mask |= 1 << cpu_core_index_of_thread(
> > >+					get_hard_smp_processor_id(cpu));
> > >+	} else {
> > >+		mask = 1 << cur_booting_core;
> > >+		for_each_online_cpu(cpu)
> > >+			mask |= 1 << get_hard_smp_processor_id(cpu);
> > >+	}
> >
> > Where is smt_capable defined()?  I assume somewhere in the patchset
> > but it's a pain to search 12 patches...
> >
> 
> It is defined in arch/powerpc/include/asm/topology.h.
> 	#define smt_capable()           (cpu_has_feature(CPU_FTR_SMT))
> 
> Thanks for your review again.

We shouldn't base it on CPU_FTR_SMT.  For example, e6500 doesn't claim  
that feature yet, except in our SDK kernel.  That doesn't change the  
topology of CPU numbering.

> > Is this really about whether we're SMT-capable or whether we have
> > rcpm v2?
> >
> > -Scott
> 
> I think this "if" statement can be removed. The  
> cpu_core_index_of_thread()
> can return the correct cpu number with thread or without thread.
> 
> Like this:
> static inline u32 get_phy_cpu_mask(void)
> {
> 	u32 mask;
> 	int cpu;
> 
> 	mask = 1 << cpu_core_index_of_thread(cur_booting_core);
> 	for_each_online_cpu(cpu)
> 		mask |= 1 << cpu_core_index_of_thread(
> 				get_hard_smp_processor_id(cpu));
> 
> 	return mask;
> }

Likewise, this will get it wrong if SMT is disabled or not yet  
implemented on a core.

-Scott