From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757134AbbHZWm0 (ORCPT ); Wed, 26 Aug 2015 18:42:26 -0400 Received: from mail-bn1bon0146.outbound.protection.outlook.com ([157.56.111.146]:11966 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752608AbbHZWmY (ORCPT ); Wed, 26 Aug 2015 18:42:24 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=scottwood@freescale.com; Date: Wed, 26 Aug 2015 17:42:13 -0500 From: Scott Wood To: Chenhui Zhao CC: , , Subject: Re: [PATCH v2,5/5] PowerPC/mpc85xx: Add CPU hotplug support for E6500 Message-ID: <20150826224213.GC10582@home.buserror.net> References: <1440590988-25594-1-git-send-email-chenhui.zhao@freescale.com> <1440590988-25594-5-git-send-email-chenhui.zhao@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <1440590988-25594-5-git-send-email-chenhui.zhao@freescale.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Originating-IP: [2601:448:8100:f9f:12bf:48ff:fe84:c9a0] X-ClientProxiedBy: CY1PR22CA0040.namprd22.prod.outlook.com (25.162.32.178) To BY1PR03MB1483.namprd03.prod.outlook.com (25.162.210.141) X-Microsoft-Exchange-Diagnostics: 1;BY1PR03MB1483;2:SzxDpg9RiyrCYG+myxludq0NuppnAy9TlBi04X1x6w+5BmZTw1G3SaexQWDZwKdu0O9R9sOk5Ii3l/KCilkaqZiRjdmPNk4HV5PFRA64Z2q0xoNtrlJB1hEfIWv0gTdy2hQU5jNuxKelUED265EI5QisWovt8q1yGeK9/pXarIw=;3:8IXOylPO7/KUhgRyl9eGb1Sc61dPHtsVdNhA+wz59qFojEgVITD5zEGa8USQL2YmTj2D/AyIvTAdMh6L7BJz8sb6esY44dSciwsg/tjNf6bPAdjfe1smAckL2hsu+tCAPG//q+KP7BE8su/6SsHOgw==;25:magQCUmZC9LvTyN5/WEzsaepgN/YHFQEsvP/HsAtLqhFxlSLnXsSpYA+IlJZpPZ0gQx9gESkfOVi34O59yEMP8H0NFFl3yLnwuecHz9tfwRC2iV4ZmqXm4IQC1HJZAMHzdg2sK4i78qwavJoVJ5c+ApDS1Z7VvIUShXy8vouBc5i2vGLb8YEq5LyxcNhJMLcgN4k8UUbW1lUDGNaUqiEYWT30m5wHKFp0epA7R9SnFMbLxE2lOBvfkl/etbp4rz28JWx730n/ve5/CJhea1PNg== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR03MB1483; X-Microsoft-Exchange-Diagnostics: 1;BY1PR03MB1483;20:IzBm3ghVR7Sy6XcmmaMIltEXrhWTZtvrS7V+AVspJD1PcZjMOUAaBtzdjahktPix2QD9q+8qsyrSSwsG/d5ShDh/bGELoYA/AFdDbeT+lrKS9Sqv7RM2QPo0smmBo/ZAJjXF9F02A+qwJENSvN9TEnOL3fkH86xuNULE9wzEhRQw4hnMu+KuW3ujEBTzKW3xm1mcunOUEquYa0KufjTU/mFSXy8PqXTymyBmczZHpDD+LIbNM/td4XEkiEG+YrlbGKL24c/hnhvGVuJVVDsd/EOweVdnpOM2nAeRlFMfU5+VkBLNUYh4m2xIg158CNM+w+6OwHxQJQhLAI+o/0hWXFoobcsmYt3ma7JbQFKCt/YLTkBjieweWlZNLPYeXQ1z6OTig4nO1yczyJBW1Sr+awka2MXwutub93mcSyBeiEUd3ZYvENresjDQrQr4cT2qRezrRAdccMmkMOGQ+dDvDIKrnqYGBjTtZ89uwNc2nFrdaAmEmDGuVhjePtc/+GV8;4:WH8YM5QvRBheDcfWJ7ryQ/xRbjzxEnvGCqc7KXIu45HuZt0CU2OQCuDig3QPxNLZxhwaedn44Mn+MMUraS6zBFRTxMq8Lrhs+zIXwbPjO5lGgDhTvt5GhQqjTzsufn4t1RviQ4WjecTEoM4s1619LCwVroiEup8mCdnNuoz7GQdPikKKnz2GL8NEA2hvakssL5PIDgo0kyq3PwwRz+4OVsPabbDYhBgC0uhWW5McekisTp1hkZbXwz/xT/uHasqGCwRloiNje2wuaVtvl3liuGrtgoR5777ful2uf72UpgI24FWqLJSPN5eyBq2KsTUl X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(8121501046)(3002001);SRVR:BY1PR03MB1483;BCL:0;PCL:0;RULEID:;SRVR:BY1PR03MB1483; X-Forefront-PRVS: 0680FADD48 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6009001)(199003)(189002)(24454002)(69596002)(4001450100002)(33656002)(4001350100001)(189998001)(46406003)(68736005)(97756001)(92566002)(23726002)(50466002)(110136002)(101416001)(40100003)(77156002)(122386002)(77096005)(5001960100002)(107886002)(42186005)(5001860100001)(2950100001)(64706001)(83506001)(5004730100002)(105586002)(86362001)(5001830100001)(50986999)(62966003)(76176999)(5007970100001)(4001540100001)(47776003)(53416004)(19580405001)(87976001)(97736004)(106356001)(54356999)(81156007)(46102003)(3826002)(4001430100001);DIR:OUT;SFP:1102;SCL:1;SRVR:BY1PR03MB1483;H:home.buserror.net;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BY1PR03MB1483;23:qURdtEMwieGPeXBm7maIexP6ETVODN7qy7ZuDVLyU?= =?us-ascii?Q?LAbCH5uz9FPZptnyMQC+rmxzJgRqi2/+fpp9eWvJsIOuWeCenwdbwYs3rTte?= =?us-ascii?Q?d/lzj+iLF0wZERn6G168xJjODL+YycRkWglfiqVgdZ9cOrRDUVdUSVrHK6Wi?= =?us-ascii?Q?H5QTHIAcEwVpywoq9JZFFvxGaR0xzXMdgigCQ2WNqIb9lP8ybr6Dn4afbJb3?= =?us-ascii?Q?5qokMjCJrXi3hwLAOi140eu5jZJKH/mT0djSApGLR59Ic78chUYEruIpg648?= =?us-ascii?Q?+PyhkBGfzWAvkAZriWrVqfYBpzi6zBeV0rxUJKBMLfxwS4nMWKE+teucW5T5?= =?us-ascii?Q?zsU5zx0dW3mt744RFaACRRqa22nXqa32iNVzurDucBkngxDD/WiqDflqt/eD?= =?us-ascii?Q?zyVvmsbFY49H4bBhMaSiQq0SeAn2rUIIccdEkcL6T3Ta5c2I5wjPDxecV+gG?= =?us-ascii?Q?JmmD5GVtyb/Cj+oOUNfU2UqRxcEEJwRm3Rm2ocOWlsnmmxuNwonlfUIC/tSW?= =?us-ascii?Q?ROrfjEnU7fyBh6DhKqiECmgNswVcmv18NHHtbYbKUkHeubwnPMdbiPmcAKLp?= =?us-ascii?Q?ywDN3fx81OEyZG6UuaNfZBWrSqvntItEHdsdb9JBa2HgoJhw+FjRd/wz2fOI?= =?us-ascii?Q?IqhicvnGXdclmUHViBwgO8ViAUOclxtG1PDrhZbQZ16aG8EvtVVHtHXt+y06?= =?us-ascii?Q?QxQykfqJKX+5NLoRktMnlCEDg6iuyJc5juaN26Ye1Y06/vGhdsVpTMpihSYx?= =?us-ascii?Q?RPHCI2vx0Sahrb9lfpm5uvZePiyjPFsvlegz50hpRKcMqoHg1LsaJ2qCw06f?= =?us-ascii?Q?iBmdXM8/rI56mdg/1lOWvuDvWfq79EhaNqsORfSjRgxqbsOkIQCgeFeV+R/W?= =?us-ascii?Q?25Ri/iNcXCUxrY+36KafmE4Ox/MrWCl5k+/+giHKBBlNCkyTPVnMK4qfiWRh?= =?us-ascii?Q?xUPRSkTbkNXDiVTkgaXv1unlPXV3vt6D2wdkziKLCKLK3k4rbJ0mvEB3m4kl?= =?us-ascii?Q?BAXADCO4A4nn8jcWw/BOm9Pmi01mquM8fLcRSrvZh30crin8Z4l+IDa0KDGW?= =?us-ascii?Q?Zno8wXSVAqXbiqeAq8cKWJpu9NX4S97/H8PcePmtpM6IK+wIr5+wZW7ye1+X?= =?us-ascii?Q?ovy4az176YyRe7RZQfnRTdcGHDiZufX29Am1DHiNae/zLzXGPpP0qgk4qxap?= =?us-ascii?Q?t8Xok7Tb0ACYf/Km1Sm8LK/k6oNV3ZA5sXw71lIcuDXMUMPlflep1DokERg5?= =?us-ascii?Q?MfaxWWPKK3M5tclsGbZa4AQOqWZAYgPd8b9juCnIWvL4fOjMuD0zOKFEzuG3?= =?us-ascii?B?Zz09?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR03MB1483;5:ypLtRf4aaNJ8TgS43Pn6lrl3HFNrKuFPsQSF1yQS8xUm44XKeVznd3129P5A3yfwNKM9MJtvnhvSS8mcgZqMnUGAkqRcxAYy61mx5XWPPG3mUjUCrbOAvmYS9c2A9a3Rpy9b3aenIS1D5ucIJwft4w==;24:l3eBNSBHpQSmj6a3DstUYoCFyJBQO6mAweE1CwYda4UnSnDh5z+B3QAPKs3nHhQZ5cUdAQb7SkEfZc5lHekxSM/bOE93S37j6mtmeJz01Ug=;20:6hjsg/Dd5o67OosWLB11oCEifwx0wjFlSVe+gSOIgFsXwLSo5QQOXal/NJh/l+ItjFx1lP48VUzN5YW4QM5wtg== X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2015 22:42:20.7102 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR03MB1483 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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