Re: [PATCH v2, 5/5] PowerPC/mpc85xx: Add CPU hotplug support for E6500

[Scott Wood <scottwood@freescale.com>]

On Wed, Aug 26, 2015 at 08:09:48PM +0800, Chenhui Zhao wrote:
> +	.globl	booting_thread_hwid
> +booting_thread_hwid:
> +	.long  INVALID_THREAD_HWID
> +	.align 3

The commit message goes into no detail about the changes you're making to
thread handling, nor are there relevant comments.

> +/*
> + * r3 = the thread physical id
> + */
> +_GLOBAL(book3e_stop_thread)
> +	li	r4, 1
> +	sld	r4, r4, r3
> +	mtspr	SPRN_TENC, r4
> +	isync
> +	blr

Why did the C code not have an isync, if it's required here?

>  _GLOBAL(fsl_secondary_thread_init)
>  	/* Enable branch prediction */
>  	lis     r3,BUCSR_INIT@h
> @@ -197,8 +236,10 @@ _GLOBAL(fsl_secondary_thread_init)
>  	 * but the low bit right by two bits so that the cpu numbering is
>  	 * continuous.
>  	 */
> -	mfspr	r3, SPRN_PIR
> -	rlwimi	r3, r3, 30, 2, 30
> +	bl	10f
> +10:	mflr	r5
> +	addi	r5,r5,(booting_thread_hwid - 10b)
> +	lwz	r3,0(r5)
>  	mtspr	SPRN_PIR, r3
>  #endif

I assume the reason for this is that, unlike the kexec case, the cpu has
been reset so PIR has been reset?  Don't make me guess -- document.

> @@ -245,6 +286,30 @@ _GLOBAL(generic_secondary_smp_init)
>  	mr	r3,r24
>  	mr	r4,r25
>  	bl	book3e_secondary_core_init
> +
> +/*
> + * If we want to boot Thread1, start Thread1 and stop Thread0.
> + * Note that only Thread0 will run the piece of code.
> + */

What ensures that only thread 0 runs this?  Especially if we're entering
via kdump on thread 1?

s/the piece/this piece/

> +	LOAD_REG_ADDR(r3, booting_thread_hwid)
> +	lwz     r4, 0(r3)
> +	cmpwi	r4, INVALID_THREAD_HWID
> +	beq	20f
> +	cmpw	r4, r24
> +	beq	20f

Do all cores get released from the spin table before the first thread
gets kicked?

> +
> +	/* start Thread1 */
> +	LOAD_REG_ADDR(r5, fsl_secondary_thread_init)
> +	ld	r4, 0(r5)
> +	li	r3, 1
> +	bl	book3e_start_thread
> +
> +	/* stop Thread0 */
> +	li	r3, 0
> +	bl	book3e_stop_thread
> +10:
> +	b	10b
> +20:
>  #endif
>  
>  generic_secondary_common_init:
> diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
> index 73eb994..61f68ad 100644
> --- a/arch/powerpc/platforms/85xx/smp.c
> +++ b/arch/powerpc/platforms/85xx/smp.c
> @@ -181,17 +181,11 @@ static inline u32 read_spin_table_addr_l(void *spin_table)
>  static void wake_hw_thread(void *info)
>  {
>  	void fsl_secondary_thread_init(void);
> -	unsigned long imsr1, inia1;
> -	int nr = *(const int *)info;
> +	unsigned long inia;
> +	int hw_cpu = get_hard_smp_processor_id(*(const int *)info);
>  
> -	imsr1 = MSR_KERNEL;
> -	inia1 = *(unsigned long *)fsl_secondary_thread_init;
> -
> -	mttmr(TMRN_IMSR1, imsr1);
> -	mttmr(TMRN_INIA1, inia1);
> -	mtspr(SPRN_TENS, TEN_THREAD(1));
> -
> -	smp_generic_kick_cpu(nr);
> +	inia = *(unsigned long *)fsl_secondary_thread_init;
> +	book3e_start_thread(cpu_thread_in_core(hw_cpu), inia);
>  }
>  #endif
>  
> @@ -279,7 +273,6 @@ static int smp_85xx_kick_cpu(int nr)
>  	int ret = 0;
>  #ifdef CONFIG_PPC64
>  	int primary = nr;
> -	int primary_hw = get_hard_smp_processor_id(primary);
>  #endif
>  
>  	WARN_ON(nr < 0 || nr >= num_possible_cpus());
> @@ -287,33 +280,43 @@ static int smp_85xx_kick_cpu(int nr)
>  	pr_debug("kick CPU #%d\n", nr);
>  
>  #ifdef CONFIG_PPC64
> +	booting_thread_hwid = INVALID_THREAD_HWID;
>  	/* Threads don't use the spin table */
> -	if (cpu_thread_in_core(nr) != 0) {
> -		int primary = cpu_first_thread_sibling(nr);
> +	if (threads_per_core == 2) {
> +		booting_thread_hwid = get_hard_smp_processor_id(nr);

What does setting booting_thread_hwid to INVALID_THREAD_HWID here
accomplish?  If threads_per_core != 2 it would never have been set to
anything else, and if threads_per_core == 2 you immediately overwrite it.

> +		primary = cpu_first_thread_sibling(nr);
>  
>  		if (WARN_ON_ONCE(!cpu_has_feature(CPU_FTR_SMT)))
>  			return -ENOENT;
>  
> -		if (cpu_thread_in_core(nr) != 1) {
> -			pr_err("%s: cpu %d: invalid hw thread %d\n",
> -			       __func__, nr, cpu_thread_in_core(nr));
> -			return -ENOENT;
> -		}
> -
> -		if (!cpu_online(primary)) {
> -			pr_err("%s: cpu %d: primary %d not online\n",
> -			       __func__, nr, primary);
> -			return -ENOENT;
> +		/*
> +		 * If either one of threads in the same core is online,
> +		 * use the online one to start the other.
> +		 */
> +		if (qoriq_pm_ops)
> +			qoriq_pm_ops->cpu_up_prepare(nr);

cpu_up_prepare does rcpm_v2_cpu_exit_state(cpu, E500_PM_PH20).  How do
you know the cpu is already in PH20?  What if this is initial boot?  Are
you relying on it being a no-op in that case?

> +
> +		if (cpu_online(primary)) {
> +			smp_call_function_single(primary,
> +					wake_hw_thread, &nr, 1);
> +			goto done;
> +		} else if (cpu_online(primary + 1)) {
> +			smp_call_function_single(primary + 1,
> +					wake_hw_thread, &nr, 1);
> +			goto done;
>  		}
>  
> -		smp_call_function_single(primary, wake_hw_thread, &nr, 0);
> -		return 0;
> +		/* If both threads are offline, continue to star primary cpu */

s/star/start/

> +	} else if (threads_per_core > 2) {
> +		pr_err("Do not support more than 2 threads per CPU.");

WARN_ONCE(1, "More than 2 threads per core not supported: %d\n",
	  threads_per_core);

-Scott