Re: [PATCH v2 12/15] powerpc/85xx: add time base sync support fore6500
From: Scott Wood
Date: Thu Apr 25 2013 - 20:07:37 EST
On 04/24/2013 07:28:18 PM, Zhao Chenhui wrote:
On Wed, Apr 24, 2013 at 05:38:16PM -0500, Scott Wood wrote:
> 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@xxxxxxxxxxxxx>
> >> >
> >> >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@xxxxxxxxxxxxx>
> >> >Signed-off-by: Li Yang <leoli@xxxxxxxxxxxxx>
> >> >Signed-off-by: Andy Fleming <afleming@xxxxxxxxxxxxx>
> >> >---
> >> > 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
> >> >@@ -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.
>
Then, where can I get the thread information? dts?
Or, wait for upstream of the thread suppport of e6500.
It's an inherent property of e6500 (outside of some virtualization
scenarios, but you wouldn't run this code under a hypervisor) that you
have two threads per core (whether Linux uses them or not). Or you
could read TMCFG0[NTHRD] if you know you're on a chip that has TMRs but
aren't positive it's an e6500, but I wouldn't bother. If we do ever
have such a chip, there are probably other things that will need
updating.
> >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
Let's look into cpu_core_index_of_thread() in
arch/powerpc/kernel/smp.c.
int cpu_core_index_of_thread(int cpu)
{
return cpu >> threads_shift;
}
If no thread, the threads_shift is equal to 0. It can work with no
thread.
My point is that if threads are disabled, threads_shift will be 0, but
e6500 cores will still be numbered 0, 2, 4, etc.
Perhaps, I should submit this patch after the thread patches for
e6500.
Why?
-Scott
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/