Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756019AbbDNQaE (ORCPT ); Tue, 14 Apr 2015 12:30:04 -0400 Received: from foss.arm.com ([217.140.101.70]:34393 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755460AbbDNQ36 (ORCPT ); Tue, 14 Apr 2015 12:29:58 -0400 Date: Tue, 14 Apr 2015 17:29:53 +0100 From: Mark Rutland To: Kumar Gala Cc: "linux-arm-msm@vger.kernel.org" , Abhimanyu Kapur , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "arm@kernel.org" , "devicetree@vger.kernel.org" , Catalin Marinas , Will Deacon Subject: Re: [RFC PATCH 5/5] arm64: qcom: add cpu operations Message-ID: <20150414162953.GL28709@leverpostej> References: <1428601031-5366-1-git-send-email-galak@codeaurora.org> <1428601031-5366-6-git-send-email-galak@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428601031-5366-6-git-send-email-galak@codeaurora.org> Thread-Topic: [RFC PATCH 5/5] arm64: qcom: add cpu operations Accept-Language: en-GB, en-US Content-Language: en-US acceptlanguage: en-GB, en-US User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4113 Lines: 137 > diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt > index 8b9e0a9..35cabe5 100644 > --- a/Documentation/devicetree/bindings/arm/cpus.txt > +++ b/Documentation/devicetree/bindings/arm/cpus.txt > @@ -185,6 +185,8 @@ nodes to be present and contain the properties described below. > be one of: > "psci" > "spin-table" In the case of these two, there's documentation on what the OS, FW, and HW are expected to do. There's a PSCI spec, and spin-table is documented in booting.txt (which is admittedly not fantastic). > + "qcom,arm-cortex-acc" However, this has no semantics associated with it. Per the code below it seems to encompass more than just poking the APSS ACC, so the name isn't great either. [...] > + * Based on arch/arm64/kernel/smp_spin_table.c This is not a phrase that will ever make me happy. [...] > +static DEFINE_RAW_SPINLOCK(boot_lock); We got rid of this for spin-table. It's pointless. > +DEFINE_PER_CPU(int, cold_boot_done); This looks suspicious. > +#if 0 > +static int cold_boot_flags[] = { > + 0, > + QCOM_SCM_FLAG_COLDBOOT_CPU1, > + QCOM_SCM_FLAG_COLDBOOT_CPU2, > + QCOM_SCM_FLAG_COLDBOOT_CPU3, > +}; > +#endif I take it this shouldn't be here? [...] > +static int power_on_l2_msm8916(struct device_node *l2ccc_node, u32 pon_mask, > + int cpu) > +{ > + u32 pon_status; > + void __iomem *l2_base; > + > + l2_base = of_iomap(l2ccc_node, 0); > + if (!l2_base) > + return -ENOMEM; I didn't see any mention of an L2 requiring power-up in the rest of the series... [...] > +static void write_pen_release(u64 val) > +{ > + void *start = (void *)&secondary_holding_pen_release; > + unsigned long size = sizeof(secondary_holding_pen_release); > + > + secondary_holding_pen_release = val; > + smp_wmb(); > + __flush_dcache_area(start, size); > +} > + > +static int secondary_pen_release(unsigned int cpu) > +{ > + unsigned long timeout; > + > + /* > + * Set synchronisation state between this boot processor > + * and the secondary one > + */ > + raw_spin_lock(&boot_lock); > + write_pen_release(cpu_logical_map(cpu)); > + > + timeout = jiffies + (1 * HZ); > + while (time_before(jiffies, timeout)) { > + if (secondary_holding_pen_release == INVALID_HWID) > + break; > + udelay(10); > + } > + raw_spin_unlock(&boot_lock); > + > + return secondary_holding_pen_release != INVALID_HWID ? -ENOSYS : 0; > +} So you want to share the pen, but duplicate the code for managing it? > +static int __init msm_cpu_init(struct device_node *dn, unsigned int cpu) > +{ > + /* Mark CPU0 cold boot flag as done */ > + if (!cpu && !per_cpu(cold_boot_done, cpu)) > + per_cpu(cold_boot_done, cpu) = true; > + > + return 0; > +} [...] > +static int msm_cpu_boot(unsigned int cpu) > +{ > + int ret = 0; > + > + if (per_cpu(cold_boot_done, cpu) == false) { > + ret = msm_unclamp_secondary_arm_cpu(cpu); > + if (ret) > + return ret; > + per_cpu(cold_boot_done, cpu) = true; > + } > + return secondary_pen_release(cpu); > +} Ah, so cold_boot_done is for pseudo-hotplug. Absolute NAK to that. The only thing this gives you over spin-table is one-time powering up of the CPUs that can be performed prior to entry to Linux. If you do that, you can trivially share the spin-table code by setting each CPU's enable-method to "spin-table". That won't give you cpuidle or actual hotplug. For those you'll need PSCI. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/