diff mbox

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

Message ID 1366368468-29143-12-git-send-email-chenhui.zhao@freescale.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

chenhui zhao April 19, 2013, 10:47 a.m. UTC 
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(-)

Comments

Scott Wood April 24, 2013, 12:04 a.m. UTC | #1 
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...

Is this really about whether we're SMT-capable or whether we have rcpm  
v2?

-Scott
chenhui zhao April 24, 2013, 11:29 a.m. UTC | #2 
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.

> 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;
}

-Chenhui
chenhui zhao April 25, 2013, 12:28 a.m. UTC | #3 
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@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
> >> >@@ -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.

> >> 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

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.

Perhaps, I should submit this patch after the thread patches for e6500.

-Chenhui
chenhui zhao April 28, 2013, 9:56 a.m. UTC | #4 
On Thu, Apr 25, 2013 at 07:07:24PM -0500, Scott Wood wrote:
> 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@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
> >> >> >@@ -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.
> 

But how to know that there are TMRs on a chip except by CPU_FTR_SMT.

> >> >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

Even if threads are disabled, the threads_shift derived from dts is right.
But, if there aren't the thread related patches existed in SDK, the threads_shift
gets a wrong value on T4.

-Chenhui
diff mbox

Patch

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);
+	}
 
 	return mask;
 }
 
 static void mpc85xx_timebase_freeze(int freeze)
 {
-	struct ccsr_rcpm __iomem *rcpm = guts_regs;
+	u32 *addr;
 	u32 mask = get_phy_cpu_mask();
 
+	if (rcpmv2)
+		addr = &((struct ccsr_rcpm_v2 *)guts_regs)->pctbenr;
+	else
+		addr = &((struct ccsr_rcpm *)guts_regs)->ctbenr;
+
 	if (freeze)
-		clrbits32(&rcpm->ctbenr, mask);
+		clrbits32(addr, mask);
 	else
-		setbits32(&rcpm->ctbenr, mask);
+		setbits32(addr, mask);
 
-	/* read back to push the previos write */
-	in_be32(&rcpm->ctbenr);
+	/* read back to push the previous write */
+	in_be32(addr);
 }
+
 #else
 static void mpc85xx_timebase_freeze(int freeze)
 {
@@ -94,6 +110,16 @@  static void mpc85xx_give_timebase(void)
 {
 	unsigned long flags;
 
+#ifdef CONFIG_PPC_E500MC
+	/*
+	 * If the booting thread is not the first thread of the core,
+	 * skip time base sync.
+	 */
+	if (smt_capable() &&
+		cur_booting_core != cpu_first_thread_sibling(cur_booting_core))
+		return;
+#endif
+
 	local_irq_save(flags);
 
 	while (!tb_req)
@@ -117,6 +143,12 @@  static void mpc85xx_take_timebase(void)
 {
 	unsigned long flags;
 
+#ifdef CONFIG_PPC_E500MC
+	if (smt_capable() &&
+		cur_booting_core != cpu_first_thread_sibling(cur_booting_core))
+		return;
+#endif
+
 	local_irq_save(flags);
 
 	tb_req = 1;
@@ -457,6 +489,7 @@  static const struct of_device_id mpc85xx_smp_guts_ids[] = {
 	{ .compatible = "fsl,p1023-guts", },
 	{ .compatible = "fsl,p2020-guts", },
 	{ .compatible = "fsl,qoriq-rcpm-1.0", },
+	{ .compatible = "fsl,qoriq-rcpm-2.0", },
 	{},
 };
 
@@ -483,6 +516,9 @@  void __init mpc85xx_smp_init(void)
 
 	np = of_find_matching_node(NULL, mpc85xx_smp_guts_ids);
 	if (np) {
+		if (of_device_is_compatible(np, "fsl,qoriq-rcpm-2.0"))
+			rcpmv2 = true;
+
 		guts_regs = of_iomap(np, 0);
 		of_node_put(np);
 		if (!guts_regs) {