| Message ID | E1gXVHy-00088e-JK@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Clean up ARM SMP/CPU hotplug implementations | expand |
On Thu, Dec 13, 2018 at 7:00 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > The boot_lock is something that was required for ARM development > platforms to ensure that the delay calibration worked properly. This > is not necessary for modern platforms that have better bus bandwidth > and do not need to calibrate the delay loop for secondary cores. > Remove the boot_lock entirely. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Andy have you had a chance to test/apply this patch? I think the majority of Qcom systems use CLKSRC_QCOM which registers an arch delay timer and swiftly bypasses the calibration anyways so this code is pretty much pointless. There is a question about MSM8974 though. This appears to not use CLKSRC_QCOM but rather HAVE_ARM_ARCH_TIMER. drivers/clocksource/arm_arch_timer.c seems to not register a delay timer, which is probably something that should be fixed? Mark/Marc: is there any specific reason to why the ARM arch timer does not register an arch delay timer? Yours, Linus Walleij
Quoting Linus Walleij (2019-01-10 04:50:56) > On Thu, Dec 13, 2018 at 7:00 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > > The boot_lock is something that was required for ARM development > > platforms to ensure that the delay calibration worked properly. This > > is not necessary for modern platforms that have better bus bandwidth > > and do not need to calibrate the delay loop for secondary cores. > > Remove the boot_lock entirely. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Andy have you had a chance to test/apply this patch? > > I think the majority of Qcom systems use CLKSRC_QCOM > which registers an arch delay timer and swiftly bypasses the > calibration anyways so this code is pretty much pointless. > > There is a question about MSM8974 though. This appears > to not use CLKSRC_QCOM but rather HAVE_ARM_ARCH_TIMER. > drivers/clocksource/arm_arch_timer.c seems to > not register a delay timer, which is probably something that > should be fixed? > > Mark/Marc: is there any specific reason to why the ARM arch > timer does not register an arch delay timer? > Doesn't the ARM arch timer get registered in arch/arm/kernel/arch_timer.c via arch_timer_delay_timer_register()?
On Thu, Jan 10, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Linus Walleij (2019-01-10 04:50:56) > > Mark/Marc: is there any specific reason to why the ARM arch > > timer does not register an arch delay timer? > > Doesn't the ARM arch timer get registered in > arch/arm/kernel/arch_timer.c via arch_timer_delay_timer_register()? Ah indeed it does, OK then, this patch should be fine from that point of view. Yours, Linus Walleij
Quoting Russell King (2018-12-13 10:00:46) > The boot_lock is something that was required for ARM development > platforms to ensure that the delay calibration worked properly. This > is not necessary for modern platforms that have better bus bandwidth > and do not need to calibrate the delay loop for secondary cores. > Remove the boot_lock entirely. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- Reviewed-by: Stephen Boyd <sboyd@kernel.org>
On Thu, Jan 10, 2019 at 01:50:56PM +0100, Linus Walleij wrote: > On Thu, Dec 13, 2018 at 7:00 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > > The boot_lock is something that was required for ARM development > > platforms to ensure that the delay calibration worked properly. This > > is not necessary for modern platforms that have better bus bandwidth > > and do not need to calibrate the delay loop for secondary cores. > > Remove the boot_lock entirely. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Andy have you had a chance to test/apply this patch? I would rather folk do not _apply_ these patches - the final patch is dependent on all the preceeding patches, and if they're applied to individual trees, it will mean that the final patch has to be delayed by a complete kernel development cycle to avoid bisect breakage.
On Fri, Jan 11, 2019 at 4:09 PM Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Thu, Jan 10, 2019 at 01:50:56PM +0100, Linus Walleij wrote: > > On Thu, Dec 13, 2018 at 7:00 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > > > > The boot_lock is something that was required for ARM development > > > platforms to ensure that the delay calibration worked properly. This > > > is not necessary for modern platforms that have better bus bandwidth > > > and do not need to calibrate the delay loop for secondary cores. > > > Remove the boot_lock entirely. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > Andy have you had a chance to test/apply this patch? > > I would rather folk do not _apply_ these patches - the final patch > is dependent on all the preceeding patches, and if they're applied > to individual trees, it will mean that the final patch has to be > delayed by a complete kernel development cycle to avoid bisect > breakage. Ah good point. It's way better that you queue it up indeed. Yours, Linus Walleij
diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c index 5494c9e0c909..99a6a5e809e0 100644 --- a/arch/arm/mach-qcom/platsmp.c +++ b/arch/arm/mach-qcom/platsmp.c @@ -46,8 +46,6 @@ extern void secondary_startup_arm(void); -static DEFINE_SPINLOCK(boot_lock); - #ifdef CONFIG_HOTPLUG_CPU static void qcom_cpu_die(unsigned int cpu) { @@ -55,15 +53,6 @@ static void qcom_cpu_die(unsigned int cpu) } #endif -static void qcom_secondary_init(unsigned int cpu) -{ - /* - * Synchronise with the boot thread. - */ - spin_lock(&boot_lock); - spin_unlock(&boot_lock); -} - static int scss_release_secondary(unsigned int cpu) { struct device_node *node; @@ -281,24 +270,12 @@ static int qcom_boot_secondary(unsigned int cpu, int (*func)(unsigned int)) } /* - * set synchronisation state between this boot processor - * and the secondary one - */ - spin_lock(&boot_lock); - - /* * Send the secondary CPU a soft interrupt, thereby causing * the boot monitor to read the system wide flags register, * and branch to the address found there. */ arch_send_wakeup_ipi_mask(cpumask_of(cpu)); - /* - * now the secondary core is starting up let it run its - * calibrations, then wait for it to finish - */ - spin_unlock(&boot_lock); - return ret; } @@ -334,7 +311,6 @@ static void __init qcom_smp_prepare_cpus(unsigned int max_cpus) static const struct smp_operations smp_msm8660_ops __initconst = { .smp_prepare_cpus = qcom_smp_prepare_cpus, - .smp_secondary_init = qcom_secondary_init, .smp_boot_secondary = msm8660_boot_secondary, #ifdef CONFIG_HOTPLUG_CPU .cpu_die = qcom_cpu_die, @@ -344,7 +320,6 @@ CPU_METHOD_OF_DECLARE(qcom_smp, "qcom,gcc-msm8660", &smp_msm8660_ops); static const struct smp_operations qcom_smp_kpssv1_ops __initconst = { .smp_prepare_cpus = qcom_smp_prepare_cpus, - .smp_secondary_init = qcom_secondary_init, .smp_boot_secondary = kpssv1_boot_secondary, #ifdef CONFIG_HOTPLUG_CPU .cpu_die = qcom_cpu_die, @@ -354,7 +329,6 @@ CPU_METHOD_OF_DECLARE(qcom_smp_kpssv1, "qcom,kpss-acc-v1", &qcom_smp_kpssv1_ops) static const struct smp_operations qcom_smp_kpssv2_ops __initconst = { .smp_prepare_cpus = qcom_smp_prepare_cpus, - .smp_secondary_init = qcom_secondary_init, .smp_boot_secondary = kpssv2_boot_secondary, #ifdef CONFIG_HOTPLUG_CPU .cpu_die = qcom_cpu_die,
The boot_lock is something that was required for ARM development platforms to ensure that the delay calibration worked properly. This is not necessary for modern platforms that have better bus bandwidth and do not need to calibrate the delay loop for secondary cores. Remove the boot_lock entirely. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- arch/arm/mach-qcom/platsmp.c | 26 -------------------------- 1 file changed, 26 deletions(-)