From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751445AbbH1FNr (ORCPT ); Fri, 28 Aug 2015 01:13:47 -0400 Received: from mail-bl2on0138.outbound.protection.outlook.com ([65.55.169.138]:54998 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750733AbbH1FNp (ORCPT ); Fri, 28 Aug 2015 01:13:45 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=scottwood@freescale.com; Message-ID: <1440738814.16577.171.camel@freescale.com> Subject: Re: [PATCH v2,5/5] PowerPC/mpc85xx: Add CPU hotplug support for E6500 From: Scott Wood To: Chenhui Zhao CC: , , Date: Fri, 28 Aug 2015 00:13:34 -0500 In-Reply-To: <1440726149.31670.2@remotesmtp.freescale.net> References: <1440590988-25594-1-git-send-email-chenhui.zhao@freescale.com> <1440590988-25594-5-git-send-email-chenhui.zhao@freescale.com> <20150826224213.GC10582@home.buserror.net> <1440726149.31670.2@remotesmtp.freescale.net> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.0-fta1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Originating-IP: [2601:448:8100:f9f:12bf:48ff:fe84:c9a0] X-ClientProxiedBy: DM2PR10CA0008.namprd10.prod.outlook.com (25.160.213.18) To BLUPR03MB1474.namprd03.prod.outlook.com (25.163.81.16) X-Microsoft-Exchange-Diagnostics: 1;BLUPR03MB1474;2:/mV49BLsy26xMSVON75DHvGoCXKBiK7emYN9wWbNEOS7LX2S1e0unKjwvcCgbdUT7kP0v2hbfsz/hKxxrPy8u9MbjfUvuTLEc9BvVmqlg7ktwnsoaU1CPzVzp1iD/4jjH/QA/m0aJHf2yzTsD+VbvCdGO96roFG14BWQ/yLRfeg=;3:pxjMmSVRksEIxHttGCXTPQi637/HLGzsSA5jYi4IxKM5oZT84+ZJJG2Omgm+hgCqz7At7UmT5SLZQlDIIajFXRJuJmNlQ1a6CMQUCz3ykDIafQK1iYDutkqIRf+ON6Y3ZR3Lw0ax6r7pXcfqzH8+/Q==;25:4kf0lIQTACcSdFQxkoWchBtOW/TktoK0PvThKrETJNtsRQD3u3rzwSdZdtL5YrbVsaBLYLJOQU0WMTu3O39wTCzese6vPQwoSeLJUeNGKyDhZ+OfZ0jNvYS29N3XA/cPsrc+gv+SpfivKCdlhoATm6qP991+jID/MGAPSW0aFm4ZQncPuRqIYP3tnPq4wVyVNbSMawdInysfnVyyVMr4t/670z4igbu4ucw+W5mRCO9e7lGwVhADBjKQQbekcee5MsPRDkXEnhz21c3jSafqag== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BLUPR03MB1474; X-Microsoft-Exchange-Diagnostics: 1;BLUPR03MB1474;20:am/ardY1rU+6uNSNrNEyoohBkWcrZCCDYQ7n+lyY9MfpXWsO1KLLqP6/BuAXbdsmYVpAFz8BdM1ovfZmPKkbgXwwMaHZqhnqkzc7b7UP1qyuny4zQUMtMveqx/mb/QDf58IzeHHE8TAokiWzN1KVZxxyk2gUzHC9hswcy+lOR5DTfDOPsI28lmrzytesoCwMwVOslD95SF3VwMGDOsKsdNmoNiuyQ1lCv19n/V77zhMWgL5j4Mi9dQudCBP5qquAgoMvahjWnHmiB2K4fw3/qLAIAEl32E7OBdFn0qbJq5D978994c/hPyoqVkGGmg9emYC8PeZ7efRWNfakOiT+mao3syFOC4ULNHedP5hg2loo8pxbAvRvl/6EfEtLudatjsejBGDMX20Q04CVmXdxyuRkO7Pj2R6CMU/3m2W7lUH448cL6M+aXQ5dX3EGW0bWWFvYfjpJkB24fn1CIIpuEBsNEbIllFkElFCsb8S6+3DrtXuMN8v0qEqeVqBcVnWc;4:6SqEuhJUKU3ou4fMov1Uwd4Yftqaeo6ydm5Z4nNANjRWYjws4CdT4qdFswp2Hv5Y83mvoF6XLhAfPjUM62yynnKv3cyil6CNRRwRaQvpBnGcGft7fnxxR6azwBJYdgu4YIYtwq8uDQ7wiPImM9zIOKjB+Yajy+50UtnKJj42FojvHMVuPRINrVlMW5ghKTzdQWMMgz68egp6EsaGqGtcXk45fJbLROufD9v6+pvciTWJuvMjPq37ptsCoC0KurQbLjbXSXC9uFCfOWyV6vvkMPqPlBvUJVzjvGnXN++9hv8E6WLH0D4FTZDqueAzfwS3 X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(8121501046)(5005006)(3002001);SRVR:BLUPR03MB1474;BCL:0;PCL:0;RULEID:;SRVR:BLUPR03MB1474; X-Forefront-PRVS: 0682FC00E8 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6009001)(377424004)(24454002)(377454003)(199003)(189002)(5001860100001)(189998001)(107886002)(5001830100001)(77096005)(5001960100002)(87976001)(86362001)(81156007)(97736004)(4001540100001)(19580405001)(19580395003)(93886004)(5820100001)(92566002)(23676002)(4001450100002)(50466002)(105586002)(36756003)(110136002)(42186005)(50226001)(101416001)(46102003)(77156002)(2950100001)(50986999)(76176999)(103116003)(33646002)(68736005)(122386002)(5007970100001)(40100003)(64706001)(106356001)(47776003)(5004730100002)(62966003)(99106002)(3826002)(5001840100002)(4001430100001);DIR:OUT;SFP:1102;SCL:1;SRVR:BLUPR03MB1474;H:[IPv6:2601:448:8100:f9f:12bf:48ff:fe84:c9a0];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTFVQUjAzTUIxNDc0OzIzOnEyMWVkcGFVSDZKS1dpVUE2clQ1UEF3bE5H?= =?utf-8?B?ejY3MlQwRFNxM1Y0Z0diREZzcGNvbExMbTVVYktUcHUwaGk3ejJRSXdjb3R1?= =?utf-8?B?LzBjc3RoTGN6UzJoaUFjTnBCN3EwYU9pbVVScHZvUDk5dU5lcVV3VGtGRnVq?= =?utf-8?B?eTRmTWZuNU54QW9iVGNHZ1JwUmpGU0M1WC9zaVB3RmQ0cFdqN3ZTVVlYUzd6?= =?utf-8?B?TUdBdDRXQy9NU2RKMzMyaUpCUy9jNVV0WFh6V3FXYTlYZTZBZHVucmRiUFVk?= =?utf-8?B?Y3diL1o5K3E3dFJLRkJBa1NQOGpEWXdETzVlbk83aWhIT0VIWkpYUHZrOXJv?= =?utf-8?B?a0FDTnE2UTY4R1RGUVIyTzBRWUZkMDhkbS9Sc0ZITDRleFFZZGdKQ2RBT29k?= =?utf-8?B?KzNQTjgyYkJVVzZyTkRLVVF2cFpCMFlVMklPMlM5MUxwM2FSQmJ3dGQ1ZEZh?= =?utf-8?B?SzdNaXNBb3N5SXJ4cm1yakt2VDRmVDBPQlptM0luTklkWU5pdFJkR05ldW5H?= =?utf-8?B?R25oaGszeURYaXEvcWtKOFpVZW1vRktIYkJFMStQQ1lxaFRNMVJkbXg4eW8v?= =?utf-8?B?R2wzb1IvZm1aZ3lCKzVHbkhzVjYzLzUzd0pmd29kRFB0cXVueEdRQXNSaERy?= =?utf-8?B?RjZ2cEljTlFIekpySkpnWnIwL1VRaG5na0c5WkVHQ0RKMUU1Tm1FdWkrSmE2?= =?utf-8?B?OTdvS3hiRjREdWgxSkNGc2JKQmtVUFF0MG5Zb2J3cHZwbk9CKzcyTEQ0c2d2?= =?utf-8?B?WWE4cWpxU05sTkNyemRMc0hGUnNpaGVST3NVZzlWQm84TXhkVGJJdTEyUnpV?= =?utf-8?B?ZS82TlBjU0J0KzRrTlVXMXYyVzFyVFlNVk02TE5YK3RrOGF6cTBJWXNHZUR0?= =?utf-8?B?WUNUanRQOG9BeldnLzY4KzdiTmM4SGQzZVFYdjI0dU5TZUVQMWkzZndMYitp?= =?utf-8?B?THJnYVdTaHNzUXhTWVY3UWZKc0ZmMW5FVEtVM1pFWmtRTDhGUlNRSmp0aVpQ?= =?utf-8?B?bHBRUU5hb3RJTlQ5N0ZIbE9oYjlyazVvT2FtQWxBR3p5bTl1aDZlWGY5dDUx?= =?utf-8?B?S0tlc1A1TFZJZDFSM3l3TkpFNms0bXlHaCtCa2hFdERnOTI3WU8xNFc3dFlp?= =?utf-8?B?bmUwbGxva0RyVUdQbTV5bnVHM3lzNnJndmRIc0Y0WCs1ZlhHMWw1djNseVgz?= =?utf-8?B?Tis5UnVDbmF0T250LzZEZzkvSkNQOThxcGo1bjhhODBLRk0vZjdnOSsyejVo?= =?utf-8?B?d0FxSG9VNFc0d2MzZWszaWpZSGdnRm5sSThaVFllY0owKzBEVjhwb2oyTlg4?= =?utf-8?B?bGFlNGk3S0Q4aVN5QmNqM0x5Z0JITVhmdzdEL3NvSG9FYnRNNm1lbS9YN1Fv?= =?utf-8?B?YmpZVURqYzJwM3V1YTdTVTdQNnlvTDFhUk1YekFGUkl1ZFZjUnhJbjJtLzc3?= =?utf-8?B?b0Z1Q042a2JJZ1EwY3hKcWVOUTBqZElHTWZLbVBhYTNpRS9EUnVseFFWS0w5?= =?utf-8?B?RHFuaDdvbWRER0c2Tkd3cklRRHc2c0FwREdPMy9uYVpyeVdLUmxHcVJoUFN5?= =?utf-8?B?WUNsRExyUWNuNUJUcVFrOVVHUi9Rc212Z1p2QmIvd3YreHhHVkM0YVBpSEV2?= =?utf-8?B?N1dRSVFveG92Z0c3VEFMMDFCMTRDcExFangxZ3huTEYxUHBObkFmL2hPeDh4?= =?utf-8?B?SHVFcXc3S3FLRUhmM0VrQ21ubHlwSFJ2NGZqWXFjVHNNbzZKYXZnQTA2UTBw?= =?utf-8?B?QlBhdXNxVHBQWlgxeS91Z2hpcjl5T3h0eWh1TVhrY2MzTmdCd1AxZTdkZ2Fs?= =?utf-8?B?MUx3dXVhWVovd0IxT2ZwbVIrdmZsbnA0MjBQRVNrQWNNV2dIWEtPUDlrSHE1?= =?utf-8?Q?zfDS7F1e5U4=3D?= X-Microsoft-Exchange-Diagnostics: 1;BLUPR03MB1474;5:VPg8ZVHy7dYpHM6ZxghCXRoHOymBISyW2tkvAjUmZ8ltXBjp36M0ak8pGJ4DX1ftOCUtsvEAr2n4LfMN2Dhjnccv4IKP0n4gvglzgLQ0bScBJCBfdzS6FiHWMISb2f0+OIoigcnBpi2rQCbu3PNwGw==;24:rcV7e0R15PhwVpFcNLBBlAlOyRFUbVhUKgSZkCs/LdumU+QG3SbkNwa6NDNQV1F9EU49keomnj0hZauK98zfDie7Q0gc9ZKx4JkbL3woV64=;20:ATFmfSzkcTnc8MrjRnbc+HUEQ2IAy5AkJ4JHQGayP6yD9TUDHcriEGC7EJCwMuDV+r1sHpR3vkaSlUczZff9Ig== X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Aug 2015 05:13:42.4155 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR03MB1474 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2015-08-28 at 09:42 +0800, Chenhui Zhao wrote: > On Thu, Aug 27, 2015 at 6:42 AM, Scott Wood > wrote: > > 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. > > OK. Will add some 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? > > Just make sure "mtspr" has completed before the routine returns. > > > > > > > > _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. > > We can not rely on the value saved in SPRN_PIR. Every time running > fsl_secondary_thread_init, SPRN_PIR may not always has a reset value. > Using booting_thread_hwid to ensure SPRN_PIR has a correct value. But when will the cpu ever be in a state other than "reset PIR value and reset BUCSR value" or "Software-desired PIR value and BUCSR value"? > > > @@ -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? > > This piece of code will be executed only when core resets (Thead0 will > start first). This is not true with kexec/kdump. > Thead1 will run fsl_secondary_thread_init() to start. > > How can kdump run this on Thread1? I know little about kexec. kexec/kdump involves booting a new kernel image without resetting the hardware. + /* 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. > > booting_thread_hwid is valid only for the case that one core has two > threads (e6500). For e5500 and e500mc, one core one thread, > "booting_thread_hwid" is invalid. > > "booting_thread_hwid" will determine starting which thread in > generic_secondary_smp_init(). You didn't answer my question. > > > + /* > > > + * 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? > > Yes, if the cpu is in PH20, it will exit; if not, cpu_up_prepare() is > equal to a no-op. This warrants a comment. -Scott