From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [PATCH 10/19] ARM64 / ACPI: Get the enable method for SMP initialization in ACPI way Date: Mon, 04 Aug 2014 17:56:29 +0800 Message-ID: <53DF58CD.9000704@linaro.org> References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-11-git-send-email-hanjun.guo@linaro.org> <20140731065426.GA876@quad.lixom.net> <53DA2110.8060000@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53DA2110.8060000@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Olof Johansson , Geoff Levand Cc: Catalin Marinas , "Rafael J. Wysocki" , Mark Rutland , Graeme Gregory , Arnd Bergmann , Grant Likely , Sudeep Holla , Will Deacon , Jason Cooper , Marc Zyngier , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Robert Richter , Lv Zheng , Robert Moore , Lorenzo Pieralisi , Liviu Dudau , Randy Dunlap , Charles.Garcia-Tobin@arm.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-acpi-private@linaro.or List-Id: linux-acpi@vger.kernel.org Hi Olof, On 2014-7-31 18:57, Hanjun Guo wrote: > On 2014-7-31 14:54, Olof Johansson wrote: [...] >>> +static void __init acpi_smp_init_cpus(void) >>> +{ >>> + int cpu; >>> + >>> + for_each_possible_cpu(cpu) { >>> + if (cpu_acpi_read_ops(cpu) != 0) >>> + continue; >>> + >>> + cpu_ops[cpu]->cpu_init(NULL, cpu); >>> + } >>> +} >>> + >>> +void __init smp_init_cpus(void) >>> +{ >>> + if (acpi_disabled) >>> + of_smp_init_cpus(); >>> + else >>> + acpi_smp_init_cpus(); >> >> I'm liking these deeply split code paths less and less every time I see >> them. :( >> >> I would prefer to set up shared state in separate functions, but keep the >> control flow the same. Right now you're splitting it completely. >> >> I.e. split data setup between the two, but do the loop calling cpu_init() >> the same way. (Yes, that will require you to refactor the DT code path >> a bit too...) > > OK, I will dive into the code and figure out if I can fix that as you > suggested, thanks for your comments :) After some investigation of the code, it seems that it is pretty hard to do so, the major gap to do that is DT needs the device node of CPU to init CPUs, but ACPI just needs the logical CPU number. In DT mode, it needs to search the DT and get the CPU device node to get its enable method, but in ACPI mode, it is a flag to indicate the CPU enable (boot) method. the code can be modified as below, but the logic is the same: diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 40f38f4..71a625b 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -320,6 +320,17 @@ void __init smp_init_cpus(void) unsigned int i, cpu = 1; bool bootcpu_valid = false; + if (!acpi_disabled) { + for_each_possible_cpu(cpu) { + if (cpu_read_ops(NULL, cpu) != 0) + continue; + + cpu_ops[cpu]->cpu_init(NULL, cpu); + } + + return; + } + while ((dn = of_find_node_by_type(dn, "cpu"))) { const u32 *cell; u64 hwid; Does it make sense to you? Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752098AbaHDJ6P (ORCPT ); Mon, 4 Aug 2014 05:58:15 -0400 Received: from mail-pa0-f50.google.com ([209.85.220.50]:41089 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751530AbaHDJ6K (ORCPT ); Mon, 4 Aug 2014 05:58:10 -0400 Message-ID: <53DF58CD.9000704@linaro.org> Date: Mon, 04 Aug 2014 17:56:29 +0800 From: Hanjun Guo User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Olof Johansson , Geoff Levand CC: Catalin Marinas , "Rafael J. Wysocki" , Mark Rutland , Graeme Gregory , Arnd Bergmann , Grant Likely , Sudeep Holla , Will Deacon , Jason Cooper , Marc Zyngier , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Robert Richter , Lv Zheng , Robert Moore , Lorenzo Pieralisi , Liviu Dudau , Randy Dunlap , Charles.Garcia-Tobin@arm.com, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-acpi-private@linaro.org, Tomasz Nowicki Subject: Re: [PATCH 10/19] ARM64 / ACPI: Get the enable method for SMP initialization in ACPI way References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-11-git-send-email-hanjun.guo@linaro.org> <20140731065426.GA876@quad.lixom.net> <53DA2110.8060000@linaro.org> In-Reply-To: <53DA2110.8060000@linaro.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Olof, On 2014-7-31 18:57, Hanjun Guo wrote: > On 2014-7-31 14:54, Olof Johansson wrote: [...] >>> +static void __init acpi_smp_init_cpus(void) >>> +{ >>> + int cpu; >>> + >>> + for_each_possible_cpu(cpu) { >>> + if (cpu_acpi_read_ops(cpu) != 0) >>> + continue; >>> + >>> + cpu_ops[cpu]->cpu_init(NULL, cpu); >>> + } >>> +} >>> + >>> +void __init smp_init_cpus(void) >>> +{ >>> + if (acpi_disabled) >>> + of_smp_init_cpus(); >>> + else >>> + acpi_smp_init_cpus(); >> >> I'm liking these deeply split code paths less and less every time I see >> them. :( >> >> I would prefer to set up shared state in separate functions, but keep the >> control flow the same. Right now you're splitting it completely. >> >> I.e. split data setup between the two, but do the loop calling cpu_init() >> the same way. (Yes, that will require you to refactor the DT code path >> a bit too...) > > OK, I will dive into the code and figure out if I can fix that as you > suggested, thanks for your comments :) After some investigation of the code, it seems that it is pretty hard to do so, the major gap to do that is DT needs the device node of CPU to init CPUs, but ACPI just needs the logical CPU number. In DT mode, it needs to search the DT and get the CPU device node to get its enable method, but in ACPI mode, it is a flag to indicate the CPU enable (boot) method. the code can be modified as below, but the logic is the same: diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 40f38f4..71a625b 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -320,6 +320,17 @@ void __init smp_init_cpus(void) unsigned int i, cpu = 1; bool bootcpu_valid = false; + if (!acpi_disabled) { + for_each_possible_cpu(cpu) { + if (cpu_read_ops(NULL, cpu) != 0) + continue; + + cpu_ops[cpu]->cpu_init(NULL, cpu); + } + + return; + } + while ((dn = of_find_node_by_type(dn, "cpu"))) { const u32 *cell; u64 hwid; Does it make sense to you? Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 From: hanjun.guo@linaro.org (Hanjun Guo) Date: Mon, 04 Aug 2014 17:56:29 +0800 Subject: [PATCH 10/19] ARM64 / ACPI: Get the enable method for SMP initialization in ACPI way In-Reply-To: <53DA2110.8060000@linaro.org> References: <1406206825-15590-1-git-send-email-hanjun.guo@linaro.org> <1406206825-15590-11-git-send-email-hanjun.guo@linaro.org> <20140731065426.GA876@quad.lixom.net> <53DA2110.8060000@linaro.org> Message-ID: <53DF58CD.9000704@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Olof, On 2014-7-31 18:57, Hanjun Guo wrote: > On 2014-7-31 14:54, Olof Johansson wrote: [...] >>> +static void __init acpi_smp_init_cpus(void) >>> +{ >>> + int cpu; >>> + >>> + for_each_possible_cpu(cpu) { >>> + if (cpu_acpi_read_ops(cpu) != 0) >>> + continue; >>> + >>> + cpu_ops[cpu]->cpu_init(NULL, cpu); >>> + } >>> +} >>> + >>> +void __init smp_init_cpus(void) >>> +{ >>> + if (acpi_disabled) >>> + of_smp_init_cpus(); >>> + else >>> + acpi_smp_init_cpus(); >> >> I'm liking these deeply split code paths less and less every time I see >> them. :( >> >> I would prefer to set up shared state in separate functions, but keep the >> control flow the same. Right now you're splitting it completely. >> >> I.e. split data setup between the two, but do the loop calling cpu_init() >> the same way. (Yes, that will require you to refactor the DT code path >> a bit too...) > > OK, I will dive into the code and figure out if I can fix that as you > suggested, thanks for your comments :) After some investigation of the code, it seems that it is pretty hard to do so, the major gap to do that is DT needs the device node of CPU to init CPUs, but ACPI just needs the logical CPU number. In DT mode, it needs to search the DT and get the CPU device node to get its enable method, but in ACPI mode, it is a flag to indicate the CPU enable (boot) method. the code can be modified as below, but the logic is the same: diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 40f38f4..71a625b 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -320,6 +320,17 @@ void __init smp_init_cpus(void) unsigned int i, cpu = 1; bool bootcpu_valid = false; + if (!acpi_disabled) { + for_each_possible_cpu(cpu) { + if (cpu_read_ops(NULL, cpu) != 0) + continue; + + cpu_ops[cpu]->cpu_init(NULL, cpu); + } + + return; + } + while ((dn = of_find_node_by_type(dn, "cpu"))) { const u32 *cell; u64 hwid; Does it make sense to you? Thanks Hanjun