[PATCH v7 09/11] ARM: hi3xxx: add hotplug support
Olof Johansson
olof at lixom.net
Tue Aug 27 22:21:00 EDT 2013
Hi,
Some comments below.
On Tue, Aug 20, 2013 at 10:31:11AM +0800, Haojian Zhuang wrote:
> From: Zhangfei Gao <zhangfei.gao at linaro.org>
>
> Enable hotplug support on hi3xxx platform
>
> How to test:
> cat proc/interrupts
> echo 0 > /sys/devices/system/cpu/cpuX/online
> cat proc/interrupts
> echo 1 > /sys/devices/system/cpu/cpuX/online
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao at linaro.org>
> Tested-by: Zhang Mingjun <zhang.mingjun at linaro.org>
> ---
> arch/arm/mach-hi3xxx/Makefile | 1 +
> arch/arm/mach-hi3xxx/core.h | 5 ++
> arch/arm/mach-hi3xxx/hi3xxx.c | 1 +
> arch/arm/mach-hi3xxx/hotplug.c | 154 +++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-hi3xxx/platsmp.c | 5 ++
> 5 files changed, 166 insertions(+)
> create mode 100644 arch/arm/mach-hi3xxx/hotplug.c
>
> diff --git a/arch/arm/mach-hi3xxx/Makefile b/arch/arm/mach-hi3xxx/Makefile
> index 0917f1c..c597cbf 100644
> --- a/arch/arm/mach-hi3xxx/Makefile
> +++ b/arch/arm/mach-hi3xxx/Makefile
> @@ -4,3 +4,4 @@
>
> obj-y += hi3xxx.o system.o
> obj-$(CONFIG_SMP) += platsmp.o
> +obj-$(CONFIG_HOTPLUG_CPU) += hotplug.o
> diff --git a/arch/arm/mach-hi3xxx/core.h b/arch/arm/mach-hi3xxx/core.h
> index 2cfd643..c2ce35d 100644
> --- a/arch/arm/mach-hi3xxx/core.h
> +++ b/arch/arm/mach-hi3xxx/core.h
> @@ -11,4 +11,9 @@ extern void hs_map_io(void);
> extern void hs_restart(enum reboot_mode, const char *cmd);
> extern struct smp_operations hs_smp_ops;
>
> +extern void __init hs_hotplug_init(void);
> +extern void hs_cpu_die(unsigned int cpu);
> +extern int hs_cpu_kill(unsigned int cpu);
> +extern void hs_set_cpu(int cpu, bool enable);
> +
> #endif
> diff --git a/arch/arm/mach-hi3xxx/hi3xxx.c b/arch/arm/mach-hi3xxx/hi3xxx.c
> index 567a0d5..01ac68b 100644
> --- a/arch/arm/mach-hi3xxx/hi3xxx.c
> +++ b/arch/arm/mach-hi3xxx/hi3xxx.c
> @@ -24,6 +24,7 @@
> static void __init hi3xxx_timer_init(void)
> {
> hs_map_io();
> + hs_hotplug_init();
> of_clk_init(NULL);
> clocksource_of_init();
> }
> diff --git a/arch/arm/mach-hi3xxx/hotplug.c b/arch/arm/mach-hi3xxx/hotplug.c
> new file mode 100644
> index 0000000..c67f8a2
> --- /dev/null
> +++ b/arch/arm/mach-hi3xxx/hotplug.c
> @@ -0,0 +1,154 @@
> +/*
> + * Copyright (c) 2013 Linaro Ltd.
> + * Copyright (c) 2013 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + */
> +#include <linux/cpu.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <asm/cacheflush.h>
> +#include <asm/smp_plat.h>
> +#include "core.h"
> +
> +enum {
> + HI3620_CTRL,
> + HI3716_CTRL,
> +};
> +
> +static void __iomem *ctrl_base;
> +static int id;
> +
> +void hs_set_cpu(int cpu, bool enable)
> +{
> + u32 val = 0;
> +
> + if (!ctrl_base)
> + return;
> +
> + if (id == HI3620_CTRL) {
> + if (enable) {
> + /* MTCMOS */
> + writel_relaxed((0x01 << (cpu + 3)), ctrl_base + 0xD0);
0xD0 should be 0xd0 (we usually don't use all-caps hex)
> + writel_relaxed((0x1011 << cpu), ctrl_base + 0x414);
> + writel_relaxed((0x401011 << cpu), ctrl_base + 0x410);
You can skip the outermost parens here.
These numbers look pretty magic. Can you make it slightly easier for someone
reading the code to figure out what's going on?
> + /* ISO disable */
> + writel((0x01 << (cpu + 3)), ctrl_base + 0x0C4);
0x0C4 should be 0xc4. Also, it's clearer if you use 0x8 << cpu instead.
> +
> + /* WFI Mask */
> + val = readl(ctrl_base + 0x200);
> + val &= ~(0x1 << (cpu+28));
Same here, don't use cpu + 28, modify the constant instead. Same for the other
occurrances below.
> + writel(val, ctrl_base + 0x200);
> +
> + /* Enable core */
> + writel_relaxed((0x01 << cpu), ctrl_base + 0xf4);
> + /* Unreset */
> + writel_relaxed((0x401011 << cpu), ctrl_base + 0x414);
> + } else {
> + /* iso enable */
> + writel_relaxed((0x01 << (cpu + 3)), ctrl_base + 0xC0);
> +
> + /* MTCMOS */
> + writel_relaxed((0x01 << (cpu + 3)), ctrl_base + 0xD4);
> +
> + /* wfi mask */
> + val = readl_relaxed(ctrl_base + 0x200);
> + val |= (0x1 << (cpu+28));
> + writel_relaxed(val, ctrl_base + 0x200);
> +
> + /* disable core*/
> + writel_relaxed((0x01 << cpu), ctrl_base + 0xf8);
> + /* Reset */
> + writel_relaxed((0x401011 << cpu), ctrl_base + 0x410);
> + }
> + } else if (id == HI3716_CTRL) {
> + if (enable) {
> + /* power on cpu1 */
> + val = readl_relaxed(ctrl_base + 0x1000);
> + val &= ~(0x1 << 8);
> + val |= (0x1 << 7);
> + val &= ~(0x1 << 3);
> + writel_relaxed(val, ctrl_base + 0x1000);
> +
> + /* unreset */
> + val = readl_relaxed(ctrl_base + 0x50);
> + val &= ~(0x1 << 17);
> + writel_relaxed(val, ctrl_base + 0x50);
> + } else {
> + /* power down cpu1 */
> + val = readl_relaxed(ctrl_base + 0x1000);
> + val &= ~(0x1 << 8);
> + val |= (0x1 << 7);
> + val |= (0x1 << 3);
> + writel_relaxed(val, ctrl_base + 0x1000);
> +
> + /* reset */
> + val = readl_relaxed(ctrl_base + 0x50);
> + val |= (0x1 << 17);
> + writel_relaxed(val, ctrl_base + 0x50);
> + }
> + }
So the function above really shares no code path between the different SoCs.
Instead of doing it all in nested if/else bodies, please split it up in two
functions, one for each SoC. You can still keep the enable/disable paths under
if/else though.
> +}
> +
> +void __init hs_hotplug_init(void)
> +{
> + struct device_node *node;
> +
> + node = of_find_compatible_node(NULL, NULL, "hisilicon,cpuctrl");
> + if (node) {
> + ctrl_base = of_iomap(node, 0);
> + id = HI3716_CTRL;
You should use something more specific than just hisilicon,cpuctrl here, if
it's truly tied to the SoC (i.e. this ID here).
hisilicon,hi3716-cpuctrl seems appropriate. Note that the bindings need
to be revised for this.
> + return;
> + }
> + node = of_find_compatible_node(NULL, NULL, "hisilicon,sctrl");
> + if (node) {
> + ctrl_base = of_iomap(node, 0);
> + id = HI3620_CTRL;
Same here.
> + return;
> + }
> +}
> +
> +static inline void cpu_enter_lowpower(void)
> +{
> + unsigned int v;
> +
> + flush_cache_all();
> + asm volatile(
> + /*
> + * Turn off coherency and L1 D-cache
> + */
Move the comment up above the asm volatile
> + " mrc p15, 0, %0, c1, c0, 1\n"
> + " bic %0, %0, #0x40\n"
> + " mcr p15, 0, %0, c1, c0, 1\n"
> + " mrc p15, 0, %0, c1, c0, 0\n"
> + " bic %0, %0, #0x04\n"
> + " mcr p15, 0, %0, c1, c0, 0\n"
> + : "=&r" (v)
> + : "r" (0)
> + : "cc");
> +}
> +
> +void hs_cpu_die(unsigned int cpu)
> +{
> + cpu_enter_lowpower();
> + hs_set_cpu_jump(cpu, phys_to_virt(0));
> + cpu_do_idle();
> +
> + /* We should have never returned from idle */
> + panic("cpu %d unexpectedly exit from shutdown\n", cpu);
> +}
> +
> +int hs_cpu_kill(unsigned int cpu)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(50);
> +
> + while (hs_get_cpu_jump(cpu))
> + if (time_after(jiffies, timeout))
> + return 0;
> + hs_set_cpu(cpu, false);
> + return 1;
> +}
> diff --git a/arch/arm/mach-hi3xxx/platsmp.c b/arch/arm/mach-hi3xxx/platsmp.c
> index a76a3cc..6a08630 100644
> --- a/arch/arm/mach-hi3xxx/platsmp.c
> +++ b/arch/arm/mach-hi3xxx/platsmp.c
> @@ -32,6 +32,7 @@ static void __init hs_smp_prepare_cpus(unsigned int max_cpus)
>
> static int hs_boot_secondary(unsigned int cpu, struct task_struct *idle)
> {
> + hs_set_cpu(cpu, true);
> hs_set_cpu_jump(cpu, secondary_startup);
> arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> return 0;
> @@ -40,4 +41,8 @@ static int hs_boot_secondary(unsigned int cpu, struct task_struct *idle)
> struct smp_operations hs_smp_ops __initdata = {
> .smp_prepare_cpus = hs_smp_prepare_cpus,
> .smp_boot_secondary = hs_boot_secondary,
> +#ifdef CONFIG_HOTPLUG_CPU
> + .cpu_die = hs_cpu_die,
> + .cpu_kill = hs_cpu_kill,
> +#endif
> };
> --
> 1.8.1.2
>
More information about the linux-arm-kernel
mailing list