2013-10-17 16:38:24

by cinifr

[permalink] [raw]
Subject: [Add SMP support for Allwinner A20: PATCH V5 0/3]

V1 V2:
The patchs add smp support for Allwinner A20. It add cpuregister node in dts for smp configure. The patchs also add a options for phy count timer to replace vir count timer as ARM arch timer clocksource.
V3 Changes since V2:
It delete platform.h and delete some code in platsmp.c that's not necessary. It delete phy count timer support because most linux kernel platform want to use vir count timer especially for kvm. SMP need arch timer as clocksource, It does use virtual counter timer and no longer use physical counter timer, so bootloader ***must*** set CNTVOFF register for a20 before kernel booting. I have add support set CNTVOFF register for uboot, if you want to test it in a20 board, you need update your sunxi uboot from: https://groups.google.com/forum/#!topic/linux-sunxi/O0Za7H5_jQI
V4 Changes since V3:
It use smp_prepare_cpus replacing early_init as cpuconfigure map init funcation caller and use sun7i replacing sunxi7i as funcation name and variable name.
V5 Changes since V4:
Move smp from struct DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)") to struct DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family") in file sunxi.c

*** BLURB HERE ***

Fan Rong (3):
Add smp support for Allwinner A20(sunxi 7i).
Add cpuconfig nodes in dts for smp configure.
Add arch count timer node in dts for Allwinner A20(sunxi 7i).

arch/arm/boot/dts/sun7i-a20.dtsi | 15 ++++++
arch/arm/mach-sunxi/Makefile | 2 +
arch/arm/mach-sunxi/headsmp.S | 18 +++++++
arch/arm/mach-sunxi/platsmp.c | 114 +++++++++++++++++++++++++++++++++++++++
arch/arm/mach-sunxi/sunxi.c | 3 ++
5 files changed, 152 insertions(+)
create mode 100644 arch/arm/mach-sunxi/headsmp.S
create mode 100644 arch/arm/mach-sunxi/platsmp.c
mode change 100644 => 100755 arch/arm/mach-sunxi/sunxi.c

--
1.8.1.2


2013-10-17 16:39:08

by cinifr

[permalink] [raw]
Subject: [Add SMP support for Allwinner A20: PATCH V5 1/3] Add smp support for Allwinner A20(sunxi 7i).

This patch adds SMP support for the Allwinner A20 SoC. This SoC uses an IP to, among other things, handle the CPU-related configuration, like the power clamp, the boot address of the secondary CPUS, etc. We thus need to map this IP during the prepare_cpu SMP operation, before bringing up the secondary CPU in the secondary_startup operation.

Signed-off-by: Fan Rong <[email protected]>
---
arch/arm/mach-sunxi/Makefile | 2 +
arch/arm/mach-sunxi/headsmp.S | 18 +++++++
arch/arm/mach-sunxi/platsmp.c | 114 ++++++++++++++++++++++++++++++++++++++++++
arch/arm/mach-sunxi/sunxi.c | 3 ++
4 files changed, 137 insertions(+)
create mode 100644 arch/arm/mach-sunxi/headsmp.S
create mode 100644 arch/arm/mach-sunxi/platsmp.c
mode change 100644 => 100755 arch/arm/mach-sunxi/sunxi.c

diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
index 93bebfc..d7f1ef4 100644
--- a/arch/arm/mach-sunxi/Makefile
+++ b/arch/arm/mach-sunxi/Makefile
@@ -1 +1,3 @@
obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
+obj-$(CONFIG_ARCH_SUNXI) += platsmp.o
+obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
new file mode 100644
index 0000000..48c9d33
--- /dev/null
+++ b/arch/arm/mach-sunxi/headsmp.S
@@ -0,0 +1,18 @@
+/*
+ * SMP support for A20
+ *
+ * Copyright (C) 2013 Fan Rong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/linkage.h>
+#include <linux/init.h>
+
+.section ".text.head", "ax"
+ENTRY(sun7i_secondary_startup)
+ msr cpsr_fsxc,#0xd3
+ b secondary_startup
+ENDPROC(sun7i_secondary_startup)
diff --git a/arch/arm/mach-sunxi/platsmp.c b/arch/arm/mach-sunxi/platsmp.c
new file mode 100644
index 0000000..fa5adde
--- /dev/null
+++ b/arch/arm/mach-sunxi/platsmp.c
@@ -0,0 +1,114 @@
+/*
+ * linux/arch/arm/mach-sun7i/platsmp.c
+ *
+ * Copyright (C) 2013 Fan Rong <[email protected]>
+ * All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/smp.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/smp.h>
+
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+
+/*
+ * CPU Configure module support
+ * 1: Software reset for smp cpus
+ * 2: Configure for smp cpus including boot.
+ * 3: Three 64-bit idle counters and two 64-bit common counters
+ * it is needed for smp cpus
+ */
+void __iomem *sun7i_cc_base; /*CPU Configure Base*/
+extern void sun7i_secondary_startup(void);
+
+/*
+ * CPUCFG
+ */
+#define SUN7I_CPUCFG_BOOTADDR 0x01a4
+
+#define SUN7I_CPUCFG_GENCTL 0x0184
+#define SUN7I_CPUCFG_DBGCTL0 0x01e0
+#define SUN7I_CPUCFG_DBGCTL1 0x01e4
+
+#define SUN7I_CPU1_PWR_CLAMP 0x01b0
+#define SUN7I_CPU1_PWROFF_REG 0x01b4
+#define SUN7I_CPUX_RESET_CTL(x) (0x40 + (x)*0x40)
+
+static struct of_device_id sun7i_cc_ids[] = {
+ { .compatible = "allwinner,sun7i-a20-cpuconfig"},
+ { /*sentinel*/ }
+};
+
+static int sun7i_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+ long paddr;
+ uint32_t pwr_reg;
+ uint32_t j = 0xff << 1;
+ if (!sun7i_cc_base) {
+ pr_debug("error map cpu configure\n");
+ return -ENOSYS;
+ }
+ /* Set boot addr */
+ paddr = virt_to_phys(sun7i_secondary_startup);
+ writel(paddr, sun7i_cc_base + SUN7I_CPUCFG_BOOTADDR);
+
+ /* Assert cpu core reset */
+ writel(0, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
+
+ /* Ensure CPU reset also invalidates L1 caches */
+ pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
+ pwr_reg &= ~BIT(cpu);
+ writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
+
+ /* DBGPWRDUP hold low */
+ pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
+ pwr_reg &= ~BIT(cpu);
+ writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
+
+ /* Ramp up power to CPU1 */
+ do {
+ writel(j, sun7i_cc_base + SUN7I_CPU1_PWR_CLAMP);
+ j = j >> 1;
+ } while (j != 0);
+
+ mdelay(10);
+
+ pwr_reg = readl(sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
+ pwr_reg &= ~1;
+ writel(pwr_reg, sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
+ mdelay(1);
+
+ /* Release CPU reset */
+ writel(3, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
+
+ /* Unlock CPU */
+ pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
+ pwr_reg |= BIT(cpu);
+ writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
+
+ return 0;
+}
+
+static void __init sun7i_init_cpuconfig_map(unsigned int max_cpus)
+{
+ struct device_node *np;
+ np = of_find_matching_node(NULL, sun7i_cc_ids);
+ if (WARN(!np, "unable to setup cup configure"))
+ return;
+ sun7i_cc_base = of_iomap(np, 0);
+ if (WARN(!sun7i_cc_base, "failed to map cup configure base address"))
+ return;
+}
+
+struct smp_operations sun7i_smp_ops __initdata = {
+ .smp_boot_secondary = sun7i_boot_secondary,
+ .smp_prepare_cpus = sun7i_init_cpuconfig_map,
+};
diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
old mode 100644
new mode 100755
index f184f6c..545269d
--- a/arch/arm/mach-sunxi/sunxi.c
+++ b/arch/arm/mach-sunxi/sunxi.c
@@ -26,6 +26,8 @@
#include <asm/mach/map.h>
#include <asm/system_misc.h>

+extern struct smp_operations sun7i_smp_ops;
+
#define SUN4I_WATCHDOG_CTRL_REG 0x00
#define SUN4I_WATCHDOG_CTRL_RESTART BIT(0)
#define SUN4I_WATCHDOG_MODE_REG 0x04
@@ -155,6 +157,7 @@ static const char * const sun7i_board_dt_compat[] = {
};

DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
+ .smp = smp_ops(sun7i_smp_ops),
.init_machine = sunxi_dt_init,
.init_time = sunxi_timer_init,
.dt_compat = sun7i_board_dt_compat,
--
1.8.1.2

2013-10-17 16:39:32

by cinifr

[permalink] [raw]
Subject: [Add SMP support for Allwinner A20: PATCH V5 2/3] Add cpuconfig nodes in dts for smp configure.

Cpu Configure regiser can be used for reset and boot smp cpus in allwiner's A20.

Signed-off-by: Fan Rong <[email protected]>
---
arch/arm/boot/dts/sun7i-a20.dtsi | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index e46cfed..6543b3f 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -270,6 +270,11 @@
reg = <0x01c23800 0x200>;
};

+ cpuconfig: cpuconfig@01c25c00 {
+ compatible = "allwinner,sun7i-a20-cpuconfig";
+ reg = <0x01c25c00 0x400>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
--
1.8.1.2

2013-10-17 16:39:57

by cinifr

[permalink] [raw]
Subject: [Add SMP support for Allwinner A20: PATCH V5 3/3] Add arch count timer node in dts for Allwinner A20(sunxi 7i).

Linux kernel usually use virtual arch timer for smp cpu tick. But the arch timer register VCTOFF normally is very different between the two cpus in A20 afer hardware reset, so SMP Cpus will see different time tick. It will cause kernel crash currently. You have two choices to fix it: 1 Simpley use physical arch timer, 2 Set VCTOFF to same value for each cpu in bootloader. The first choice will cause some other problem for kernel, especially for KVM, guest OS want to use virtual arch timer. So the second choice is perfect. So if you want to use arch timer for smp cpu tick, you must ensure you bootload have set same VCTOFF for all cpus.

Signed-off-by: Fan Rong <[email protected]>
---
arch/arm/boot/dts/sun7i-a20.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 6543b3f..dc01d5a 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -410,5 +410,15 @@
#interrupt-cells = <3>;
interrupts = <1 9 0xf04>;
};
+
+ timer {
+ compatible ="arm,armv7-timer";
+ interrupts = <1 13 0x308>,
+ <1 14 0x308>,
+ <1 11 0x308>,
+ <1 10 0x308>;
+ clock-frequency = <24000000>;
+ };
+
};
};
--
1.8.1.2

2013-10-22 00:48:38

by cinifr

[permalink] [raw]
Subject: Re: [Add SMP support for Allwinner A20: PATCH V5 1/3] Add smp support for Allwinner A20(sunxi 7i).

HI Maxime,
How about the new V5 patch?
Fan
Thanks


On 18 October 2013 00:37, Fan Rong <[email protected]> wrote:
> This patch adds SMP support for the Allwinner A20 SoC. This SoC uses an IP to, among other things, handle the CPU-related configuration, like the power clamp, the boot address of the secondary CPUS, etc. We thus need to map this IP during the prepare_cpu SMP operation, before bringing up the secondary CPU in the secondary_startup operation.
>
> Signed-off-by: Fan Rong <[email protected]>
> ---
> arch/arm/mach-sunxi/Makefile | 2 +
> arch/arm/mach-sunxi/headsmp.S | 18 +++++++
> arch/arm/mach-sunxi/platsmp.c | 114 ++++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-sunxi/sunxi.c | 3 ++
> 4 files changed, 137 insertions(+)
> create mode 100644 arch/arm/mach-sunxi/headsmp.S
> create mode 100644 arch/arm/mach-sunxi/platsmp.c
> mode change 100644 => 100755 arch/arm/mach-sunxi/sunxi.c
>
> diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> index 93bebfc..d7f1ef4 100644
> --- a/arch/arm/mach-sunxi/Makefile
> +++ b/arch/arm/mach-sunxi/Makefile
> @@ -1 +1,3 @@
> obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
> +obj-$(CONFIG_ARCH_SUNXI) += platsmp.o
> +obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> new file mode 100644
> index 0000000..48c9d33
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -0,0 +1,18 @@
> +/*
> + * SMP support for A20
> + *
> + * Copyright (C) 2013 Fan Rong <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/linkage.h>
> +#include <linux/init.h>
> +
> +.section ".text.head", "ax"
> +ENTRY(sun7i_secondary_startup)
> + msr cpsr_fsxc,#0xd3
> + b secondary_startup
> +ENDPROC(sun7i_secondary_startup)
> diff --git a/arch/arm/mach-sunxi/platsmp.c b/arch/arm/mach-sunxi/platsmp.c
> new file mode 100644
> index 0000000..fa5adde
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/platsmp.c
> @@ -0,0 +1,114 @@
> +/*
> + * linux/arch/arm/mach-sun7i/platsmp.c
> + *
> + * Copyright (C) 2013 Fan Rong <[email protected]>
> + * All Rights Reserved
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/smp.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/smp.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +
> +/*
> + * CPU Configure module support
> + * 1: Software reset for smp cpus
> + * 2: Configure for smp cpus including boot.
> + * 3: Three 64-bit idle counters and two 64-bit common counters
> + * it is needed for smp cpus
> + */
> +void __iomem *sun7i_cc_base; /*CPU Configure Base*/
> +extern void sun7i_secondary_startup(void);
> +
> +/*
> + * CPUCFG
> + */
> +#define SUN7I_CPUCFG_BOOTADDR 0x01a4
> +
> +#define SUN7I_CPUCFG_GENCTL 0x0184
> +#define SUN7I_CPUCFG_DBGCTL0 0x01e0
> +#define SUN7I_CPUCFG_DBGCTL1 0x01e4
> +
> +#define SUN7I_CPU1_PWR_CLAMP 0x01b0
> +#define SUN7I_CPU1_PWROFF_REG 0x01b4
> +#define SUN7I_CPUX_RESET_CTL(x) (0x40 + (x)*0x40)
> +
> +static struct of_device_id sun7i_cc_ids[] = {
> + { .compatible = "allwinner,sun7i-a20-cpuconfig"},
> + { /*sentinel*/ }
> +};
> +
> +static int sun7i_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> + long paddr;
> + uint32_t pwr_reg;
> + uint32_t j = 0xff << 1;
> + if (!sun7i_cc_base) {
> + pr_debug("error map cpu configure\n");
> + return -ENOSYS;
> + }
> + /* Set boot addr */
> + paddr = virt_to_phys(sun7i_secondary_startup);
> + writel(paddr, sun7i_cc_base + SUN7I_CPUCFG_BOOTADDR);
> +
> + /* Assert cpu core reset */
> + writel(0, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
> +
> + /* Ensure CPU reset also invalidates L1 caches */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
> + pwr_reg &= ~BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
> +
> + /* DBGPWRDUP hold low */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> + pwr_reg &= ~BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> +
> + /* Ramp up power to CPU1 */
> + do {
> + writel(j, sun7i_cc_base + SUN7I_CPU1_PWR_CLAMP);
> + j = j >> 1;
> + } while (j != 0);
> +
> + mdelay(10);
> +
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
> + pwr_reg &= ~1;
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
> + mdelay(1);
> +
> + /* Release CPU reset */
> + writel(3, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
> +
> + /* Unlock CPU */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> + pwr_reg |= BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> +
> + return 0;
> +}
> +
> +static void __init sun7i_init_cpuconfig_map(unsigned int max_cpus)
> +{
> + struct device_node *np;
> + np = of_find_matching_node(NULL, sun7i_cc_ids);
> + if (WARN(!np, "unable to setup cup configure"))
> + return;
> + sun7i_cc_base = of_iomap(np, 0);
> + if (WARN(!sun7i_cc_base, "failed to map cup configure base address"))
> + return;
> +}
> +
> +struct smp_operations sun7i_smp_ops __initdata = {
> + .smp_boot_secondary = sun7i_boot_secondary,
> + .smp_prepare_cpus = sun7i_init_cpuconfig_map,
> +};
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> old mode 100644
> new mode 100755
> index f184f6c..545269d
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -26,6 +26,8 @@
> #include <asm/mach/map.h>
> #include <asm/system_misc.h>
>
> +extern struct smp_operations sun7i_smp_ops;
> +
> #define SUN4I_WATCHDOG_CTRL_REG 0x00
> #define SUN4I_WATCHDOG_CTRL_RESTART BIT(0)
> #define SUN4I_WATCHDOG_MODE_REG 0x04
> @@ -155,6 +157,7 @@ static const char * const sun7i_board_dt_compat[] = {
> };
>
> DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
> + .smp = smp_ops(sun7i_smp_ops),
> .init_machine = sunxi_dt_init,
> .init_time = sunxi_timer_init,
> .dt_compat = sun7i_board_dt_compat,
> --
> 1.8.1.2
>

2013-10-23 11:30:09

by Maxime Ripard

[permalink] [raw]
Subject: Re: [Add SMP support for Allwinner A20: PATCH V5 0/3]

On Fri, Oct 18, 2013 at 12:37:04AM +0800, Fan Rong wrote:
> V1 V2:
> The patchs add smp support for Allwinner A20. It add cpuregister node in dts for smp configure. The patchs also add a options for phy count timer to replace vir count timer as ARM arch timer clocksource.
> V3 Changes since V2:
> It delete platform.h and delete some code in platsmp.c that's not necessary. It delete phy count timer support because most linux kernel platform want to use vir count timer especially for kvm. SMP need arch timer as clocksource, It does use virtual counter timer and no longer use physical counter timer, so bootloader ***must*** set CNTVOFF register for a20 before kernel booting. I have add support set CNTVOFF register for uboot, if you want to test it in a20 board, you need update your sunxi uboot from: https://groups.google.com/forum/#!topic/linux-sunxi/O0Za7H5_jQI
> V4 Changes since V3:
> It use smp_prepare_cpus replacing early_init as cpuconfigure map init funcation caller and use sun7i replacing sunxi7i as funcation name and variable name.
> V5 Changes since V4:
> Move smp from struct DT_MACHINE_START(SUNXI_DT, "Allwinner A1X (Device Tree)") to struct DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family") in file sunxi.c
>
> *** BLURB HERE ***

Seriously?

I explained to you several times what we were expecting, with references
to examples of what you should do, and you're still not listening?

You just make me feel like I'm wasting my time here. If you don't want
to make any effort, I won't either.

This is my last warning. Here is what is wrong about your patchset:
- Wrap your commit log and cover letter to 80 chars
- You don't have any introduction letter
- The prefix is wrong, again, while you had it fixed in your v4

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (1.83 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-10-23 11:30:08

by Maxime Ripard

[permalink] [raw]
Subject: Re: [Add SMP support for Allwinner A20: PATCH V5 3/3] Add arch count timer node in dts for Allwinner A20(sunxi 7i).

On Fri, Oct 18, 2013 at 12:37:07AM +0800, Fan Rong wrote:
> Linux kernel usually use virtual arch timer for smp cpu tick. But the
> arch timer register VCTOFF normally is very different between the two
> cpus in A20 afer hardware reset, so SMP Cpus will see different time
> tick. It will cause kernel crash currently. You have two choices to
> fix it: 1 Simpley use physical arch timer, 2 Set VCTOFF to same value
> for each cpu in bootloader. The first choice will cause some other
> problem for kernel, especially for KVM, guest OS want to use virtual
> arch timer. So the second choice is perfect. So if you want to use
> arch timer for smp cpu tick, you must ensure you bootload have set
> same VCTOFF for all cpus.
>

Is your patchset working without that last patch?

I'm not really wanting to apply it right now.

It relies on a bootloader behaviour that is not implemented by any
bootloader we have openly available right now. I will apply it after
it's been merged in at least u-boot-sunxi.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (1.09 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments

2013-10-25 10:25:12

by Maxime Ripard

[permalink] [raw]
Subject: Re: [Add SMP support for Allwinner A20: PATCH V5 1/3] Add smp support for Allwinner A20(sunxi 7i).

On Fri, Oct 18, 2013 at 12:37:05AM +0800, Fan Rong wrote:
> This patch adds SMP support for the Allwinner A20 SoC. This SoC uses
> an IP to, among other things, handle the CPU-related configuration,
> like the power clamp, the boot address of the secondary CPUS, etc. We
> thus need to map this IP during the prepare_cpu SMP operation, before
> bringing up the secondary CPU in the secondary_startup operation.

Please wrap this to 80 chars, and use sun7i in your commit title.

>
> Signed-off-by: Fan Rong <[email protected]>
> ---
> arch/arm/mach-sunxi/Makefile | 2 +
> arch/arm/mach-sunxi/headsmp.S | 18 +++++++
> arch/arm/mach-sunxi/platsmp.c | 114 ++++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-sunxi/sunxi.c | 3 ++
> 4 files changed, 137 insertions(+)
> create mode 100644 arch/arm/mach-sunxi/headsmp.S
> create mode 100644 arch/arm/mach-sunxi/platsmp.c
> mode change 100644 => 100755 arch/arm/mach-sunxi/sunxi.c
>
> diff --git a/arch/arm/mach-sunxi/Makefile b/arch/arm/mach-sunxi/Makefile
> index 93bebfc..d7f1ef4 100644
> --- a/arch/arm/mach-sunxi/Makefile
> +++ b/arch/arm/mach-sunxi/Makefile
> @@ -1 +1,3 @@
> obj-$(CONFIG_ARCH_SUNXI) += sunxi.o
> +obj-$(CONFIG_ARCH_SUNXI) += platsmp.o
> +obj-$(CONFIG_ARCH_SUNXI) += headsmp.o
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> new file mode 100644
> index 0000000..48c9d33
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -0,0 +1,18 @@
> +/*
> + * SMP support for A20
> + *
> + * Copyright (C) 2013 Fan Rong <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/linkage.h>
> +#include <linux/init.h>
> +
> +.section ".text.head", "ax"
> +ENTRY(sun7i_secondary_startup)
> + msr cpsr_fsxc,#0xd3
> + b secondary_startup
> +ENDPROC(sun7i_secondary_startup)
> diff --git a/arch/arm/mach-sunxi/platsmp.c b/arch/arm/mach-sunxi/platsmp.c
> new file mode 100644
> index 0000000..fa5adde
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/platsmp.c
> @@ -0,0 +1,114 @@
> +/*
> + * linux/arch/arm/mach-sun7i/platsmp.c
> + *
> + * Copyright (C) 2013 Fan Rong <[email protected]>
> + * All Rights Reserved
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/smp.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/smp.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +
> +/*
> + * CPU Configure module support
> + * 1: Software reset for smp cpus
> + * 2: Configure for smp cpus including boot.
> + * 3: Three 64-bit idle counters and two 64-bit common counters
> + * it is needed for smp cpus
> + */
> +void __iomem *sun7i_cc_base; /*CPU Configure Base*/
> +extern void sun7i_secondary_startup(void);
> +
> +/*
> + * CPUCFG
> + */
> +#define SUN7I_CPUCFG_BOOTADDR 0x01a4
> +
> +#define SUN7I_CPUCFG_GENCTL 0x0184
> +#define SUN7I_CPUCFG_DBGCTL0 0x01e0
> +#define SUN7I_CPUCFG_DBGCTL1 0x01e4
> +
> +#define SUN7I_CPU1_PWR_CLAMP 0x01b0
> +#define SUN7I_CPU1_PWROFF_REG 0x01b4
> +#define SUN7I_CPUX_RESET_CTL(x) (0x40 + (x)*0x40)
> +
> +static struct of_device_id sun7i_cc_ids[] = {
> + { .compatible = "allwinner,sun7i-a20-cpuconfig"},
> + { /*sentinel*/ }
> +};
> +
> +static int sun7i_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> + long paddr;
> + uint32_t pwr_reg;
> + uint32_t j = 0xff << 1;
> + if (!sun7i_cc_base) {
> + pr_debug("error map cpu configure\n");
> + return -ENOSYS;
> + }
> + /* Set boot addr */
> + paddr = virt_to_phys(sun7i_secondary_startup);
> + writel(paddr, sun7i_cc_base + SUN7I_CPUCFG_BOOTADDR);
> +
> + /* Assert cpu core reset */
> + writel(0, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
> +
> + /* Ensure CPU reset also invalidates L1 caches */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
> + pwr_reg &= ~BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_GENCTL);
> +
> + /* DBGPWRDUP hold low */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> + pwr_reg &= ~BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> +
> + /* Ramp up power to CPU1 */
> + do {
> + writel(j, sun7i_cc_base + SUN7I_CPU1_PWR_CLAMP);
> + j = j >> 1;
> + } while (j != 0);

In your first version, you were starting by writing 0xff, while here the
first iteration of the loop writes (0xff << 1), is that intentionnal ?

Maybe you could move the j variable affectation just before the loop so
that we can more easily spot such mistakes.

> +
> + mdelay(10);
> +
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
> + pwr_reg &= ~1;
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPU1_PWROFF_REG);
> + mdelay(1);
> +
> + /* Release CPU reset */
> + writel(3, sun7i_cc_base + SUN7I_CPUX_RESET_CTL(cpu));
> +
> + /* Unlock CPU */
> + pwr_reg = readl(sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> + pwr_reg |= BIT(cpu);
> + writel(pwr_reg, sun7i_cc_base + SUN7I_CPUCFG_DBGCTL1);
> +
> + return 0;
> +}
> +
> +static void __init sun7i_init_cpuconfig_map(unsigned int max_cpus)
> +{
> + struct device_node *np;

Add a new line here.

> + np = of_find_matching_node(NULL, sun7i_cc_ids);
> + if (WARN(!np, "unable to setup cup configure"))
^ cpu
> + return;

Add a new line here.

> + sun7i_cc_base = of_iomap(np, 0);
> + if (WARN(!sun7i_cc_base, "failed to map cup configure base address"))
^ cpu
> + return;

I think I would panic here instead of issuing a warning.

> +}
> +
> +struct smp_operations sun7i_smp_ops __initdata = {
> + .smp_boot_secondary = sun7i_boot_secondary,
> + .smp_prepare_cpus = sun7i_init_cpuconfig_map,
> +};
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> old mode 100644
> new mode 100755
> index f184f6c..545269d
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -26,6 +26,8 @@
> #include <asm/mach/map.h>
> #include <asm/system_misc.h>
>
> +extern struct smp_operations sun7i_smp_ops;
> +
> #define SUN4I_WATCHDOG_CTRL_REG 0x00
> #define SUN4I_WATCHDOG_CTRL_RESTART BIT(0)
> #define SUN4I_WATCHDOG_MODE_REG 0x04
> @@ -155,6 +157,7 @@ static const char * const sun7i_board_dt_compat[] = {
> };
>
> DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
> + .smp = smp_ops(sun7i_smp_ops),

Please align it with the other affectations.

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (6.68 kB)
signature.asc (836.00 B)
Digital signature
Download all attachments