enable HiP04 SoC

Since CONFIG_ARM_LPAE could be set in ARM V7, add new two configurations CONFIG_ARCH_MULTI_V7_NONLPAE & CONFIG_ARCH_MULTI_V7_LPAE.

If we want to enable CONFIG_ARM_LPAE configuration, we should enable CONFIG_ARCH_MULTI_V7 & disable both CONFIG_ARCH_MULTI_V6 and CONFIG_ARCH_MULTI_V7_NONLPAE.

Signed-off-by Haojian Zhuang <[email protected]>

Quoted 8 lines · Haojian Zhuang

Since CONFIG_ARM_LPAE could be set in ARM V7, add new two configurations CONFIG_ARCH_MULTI_V7_NONLPAE & CONFIG_ARCH_MULTI_V7_LPAE.

If we want to enable CONFIG_ARM_LPAE configuration, we should enable CONFIG_ARCH_MULTI_V7 & disable both CONFIG_ARCH_MULTI_V6 and CONFIG_ARCH_MULTI_V7_NONLPAE.

Signed-off-by Haojian Zhuang <[email protected]>

I don't like this. Platforms should just depend on CONFIG_ARCH_MULTI_V7 && LPAE (or && !LPAE) instead.

Platforms that _can_ boot without LPAE should not require it, even if they boot with less available memory.

-Olof

Quoted 13 lines · Olof Johansson

Quoted 8 lines · Haojian Zhuang

Since CONFIG_ARM_LPAE could be set in ARM V7, add new two configurations CONFIG_ARCH_MULTI_V7_NONLPAE & CONFIG_ARCH_MULTI_V7_LPAE.

If we want to enable CONFIG_ARM_LPAE configuration, we should enable CONFIG_ARCH_MULTI_V7 & disable both CONFIG_ARCH_MULTI_V6 and CONFIG_ARCH_MULTI_V7_NONLPAE.

Signed-off-by Haojian Zhuang <[email protected]>

I don't like this. Platforms should just depend on CONFIG_ARCH_MULTI_V7 && LPAE (or && !LPAE) instead.

Quoted 5 lines

Platforms that _can_ boot without LPAE should not require it, even if they boot with less available memory.

-Olof

Do you mean that I should remove CONFIG_ARCH_MULTI_V7_NONLPAE & CONFIG_ARCH_MULTI_V7_LPAE & multi_v7_lpae_defconfig?

Add the CONFIG_MCPM_QUAD_CLUSTER configuration to enlarge cluster number from 2 to 4.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/Kconfig            | 9 +++++++++
 arch/arm/include/asm/mcpm.h | 5 +++++
 2 files changed, 14 insertions(+)

arch/arm/Kconfig +0 -0

index 0e52a2c..624829f 100644
@@ -1575,6 +1575,15 @@ config MCPM
 	  for (multi-)cluster based systems, such as big.LITTLE based
 	  systems.

Quoted 17 lines · Haojian Zhuang

Add the CONFIG_MCPM_QUAD_CLUSTER configuration to enlarge cluster number from 2 to 4.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/Kconfig            | 9 +++++++++
 arch/arm/include/asm/mcpm.h | 5 +++++
 2 files changed, 14 insertions(+)

arch/arm/Kconfig +0 -0

index 0e52a2c..624829f 100644
@@ -1575,6 +1575,15 @@ config MCPM
 	  for (multi-)cluster based systems, such as big.LITTLE based
 	  systems.
 

arch/arm/Kconfig config MCPM · line 1586

Quoted 9 lines

arch/arm/Kconfig +9 -0

+config MCPM_QUAD_CLUSTER
+	bool "Enable Quad clusters in MCPM"
+	depends on MCPM
+	help
+	  To avoid wasting resources unnecessarily, MCPM only supports up
+	  to 2 clusters by default.
+	  Platforms with 3 or 4 clusters that use MCPM must select this
+	  option to allow the additional clusters to be managed.
+

Why don't you add an integer config instead, i.e. MCPM_MAX_CLUSTERS, and let that set MAX_NR_CLUSTERS?

Quoted 18 lines · Olof Johansson

Quoted 17 lines · Haojian Zhuang

Add the CONFIG_MCPM_QUAD_CLUSTER configuration to enlarge cluster number from 2 to 4.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/Kconfig            | 9 +++++++++
 arch/arm/include/asm/mcpm.h | 5 +++++
 2 files changed, 14 insertions(+)

arch/arm/Kconfig +0 -0

index 0e52a2c..624829f 100644
@@ -1575,6 +1575,15 @@ config MCPM
      for (multi-)cluster based systems, such as big.LITTLE based
      systems.
 

Quoted 12 lines

arch/arm/Kconfig config MCPM · line 1586

Quoted 9 lines

arch/arm/Kconfig +9 -0

+config MCPM_QUAD_CLUSTER
+   bool "Enable Quad clusters in MCPM"
+   depends on MCPM
+   help
+     To avoid wasting resources unnecessarily, MCPM only supports up
+     to 2 clusters by default.
+     Platforms with 3 or 4 clusters that use MCPM must select this
+     option to allow the additional clusters to be managed.
+

Why don't you add an integer config instead, i.e. MCPM_MAX_CLUSTERS, and let that set MAX_NR_CLUSTERS?

I requested for this config symbol not to be visible to users (the prompt string should be removed) and be selected by those platforms that need it instead. And there is no way to specify an actual value with 'select'. And that wouldn't work in a multi-platform config where different platforms could fight over a different value anyway.

I also prefer to keep some control on the actual number that can be used here so not to let people go overboard with it. If this grows too big we'll have to consider some dynamic allocation instead.

See previous discussion here:

http://article.gmane.org/gmane.linux.ports.arm.kernel/315228

Add the CONFIG_MCPM_QUAD_CLUSTER configuration to enlarge cluster number from 2 to 4.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/Kconfig            | 9 +++++++++
 arch/arm/include/asm/mcpm.h | 5 +++++
 2 files changed, 14 insertions(+)

arch/arm/Kconfig +0 -0

index 0e52a2c..624829f 100644

arch/arm/Kconfig +7 -0

+	depends on MCPM
+	help
+	  To avoid wasting resources unnecessarily, MCPM only supports up
+	  to 2 clusters by default.
+	  Platforms with 3 or 4 clusters that use MCPM must select this
+	  option to allow the additional clusters to be managed.
+
 config BIG_LITTLE
 	bool "big.LITTLE support (Experimental)"
 	depends on CPU_V7 && SMP

arch/arm/include/asm/mcpm.h +5 -0

index 608516e..fc8d70d 100644
@@ -20,7 +20,12 @@
  * to consider dynamic allocation.
  */
 #define MAX_CPUS_PER_CLUSTER	4
+
+#ifdef CONFIG_MCPM_QUAD_CLUSTER
+#define MAX_NR_CLUSTERS		4
+#else
 #define MAX_NR_CLUSTERS		2
+#endif
 
 #ifndef __ASSEMBLY__
 

Signature · 1 line

1.9.1

Hisilicon Hi3xxx is based on Cortex A9 Core. Now HiP04 SoC is based on Cortex A15 Core. Since multiple clusters is used in HiP04 SoC, it could be based on MCPM.

And HiP04 supports LPAE to support large memory.

Signed-off-by Haojian Zhuang <[email protected]>

---
 .../bindings/arm/hisilicon/hisilicon.txt           |  19 ++
 .../devicetree/bindings/clock/hip04-clock.txt      |  20 ++
 arch/arm/mach-hisi/Kconfig                         |  10 +-
 arch/arm/mach-hisi/Makefile                        |   1 +
 arch/arm/mach-hisi/core.h                          |   2 +
 arch/arm/mach-hisi/hisilicon.c                     |  12 +
 arch/arm/mach-hisi/platmcpm.c                      | 314 +++++++++++++++++++++
 7 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/hip04-clock.txt
 create mode 100644 arch/arm/mach-hisi/platmcpm.c

Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +4 -0

index df0a452..20736b0 100644
@@ -4,6 +4,10 @@ Hisilicon Platforms Device Tree Bindings
 Hi4511 Board
 Required root node properties:
 	- compatible = "hisilicon,hi3620-hi4511";
+HiP04 D01 Board
+Required root node properties:
+	- compatible = "hisilicon,hip04-d01";
+
 
 Hisilicon system controller
 

Hisilicon Hi3xxx is based on Cortex A9 Core. Now HiP04 SoC is based on Cortex A15 Core. Since multiple clusters is used in HiP04 SoC, it could be based on MCPM.

And HiP04 supports LPAE to support large memory.

Signed-off-by Haojian Zhuang <[email protected]>

---
 .../bindings/arm/hisilicon/hisilicon.txt           |  19 ++
 .../devicetree/bindings/clock/hip04-clock.txt      |  20 ++
 arch/arm/mach-hisi/Kconfig                         |  10 +-
 arch/arm/mach-hisi/Makefile                        |   1 +
 arch/arm/mach-hisi/core.h                          |   2 +
 arch/arm/mach-hisi/hisilicon.c                     |  12 +
 arch/arm/mach-hisi/platmcpm.c                      | 314 +++++++++++++++++++++
 7 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/hip04-clock.txt
 create mode 100644 arch/arm/mach-hisi/platmcpm.c

arch/arm/mach-hisi/platmcpm.c new file +113 -0

new file mode 100644
index 0000000..e0d7afd
@@ -0,0 +1,314 @@
+/*
+ * Copyright (c) 2013-2014 Linaro Ltd.
+ * Copyright (c) 2013-2014 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/delay.h>
+#include <linux/io.h>
+#include <linux/memblock.h>
+#include <linux/of_address.h>
+
+#include <asm/cputype.h>
+#include <asm/cp15.h>
+#include <asm/mcpm.h>
+
+#include "core.h"
+
+/* bits definition in SC_CPU_RESET_REQ[x]/SC_CPU_RESET_DREQ[x]
+ * 1 -- unreset; 0 -- reset
+ */
+#define CORE_RESET_BIT(x)		(1 << x)
+#define NEON_RESET_BIT(x)		(1 << (x + 4))
+#define CORE_DEBUG_RESET_BIT(x)		(1 << (x + 9))
+#define CLUSTER_L2_RESET_BIT		(1 << 8)
+#define CLUSTER_DEBUG_RESET_BIT		(1 << 13)
+
+/*
+ * bits definition in SC_CPU_RESET_STATUS[x]
+ * 1 -- reset status; 0 -- unreset status
+ */
+#define CORE_RESET_STATUS(x)		(1 << x)
+#define NEON_RESET_STATUS(x)		(1 << (x + 4))
+#define CORE_DEBUG_RESET_STATUS(x)	(1 << (x + 9))
+#define CLUSTER_L2_RESET_STATUS		(1 << 8)
+#define CLUSTER_DEBUG_RESET_STATUS	(1 << 13)
+#define CORE_WFI_STATUS(x)		(1 << (x + 16))
+#define CORE_WFE_STATUS(x)		(1 << (x + 20))
+#define CORE_DEBUG_ACK(x)		(1 << (x + 24))
+
+#define SC_CPU_RESET_REQ(x)		(0x520 + (x << 3))	/* reset */
+#define SC_CPU_RESET_DREQ(x)		(0x524 + (x << 3))	/* unreset */
+#define SC_CPU_RESET_STATUS(x)		(0x1520 + (x << 3))
+
+#define FAB_SF_MODE			0x0c
+#define FAB_SF_INVLD			0x10
+
+/* bits definition in FB_SF_INVLD */
+#define FB_SF_INVLD_START		(1 << 8)
+
+#define HIP04_MAX_CLUSTERS		4
+#define HIP04_MAX_CPUS_PER_CLUSTER	4
+
+#define POLL_MSEC	10
+#define TIMEOUT_MSEC	1000
+
+struct hip04_secondary_cpu_data {
+	u32	bootwrapper_phys;
+	u32	bootwrapper_size;
+	u32	bootwrapper_magic;
+	u32	relocation_entry;
+	u32	relocation_size;
+};
+
+static void __iomem *relocation, *sysctrl, *fabric;
+static int hip04_cpu_table[HIP04_MAX_CLUSTERS][HIP04_MAX_CPUS_PER_CLUSTER];
+static DEFINE_SPINLOCK(boot_lock);
+static struct hip04_secondary_cpu_data hip04_boot;
+
+static bool hip04_cluster_down(unsigned int cluster)
+{
+	int i;
+
+	for (i = 0; i < HIP04_MAX_CPUS_PER_CLUSTER; i++)
+		if (hip04_cpu_table[cluster][i])
+			return false;
+	return true;
+}
+
+static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
+{
+	unsigned long data;
+
+	if (!fabric)
+		return;
+	data = readl_relaxed(fabric + FAB_SF_MODE);
+	if (on)
+		data |= 1 << cluster;
+	else
+		data &= ~(1 << cluster);
+	writel_relaxed(data, fabric + FAB_SF_MODE);
+	while (1) {
+		if (data == readl_relaxed(fabric + FAB_SF_MODE))
+			break;
+	}
+}
+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+	unsigned long data, mask;
+
+	if (!relocation || !sysctrl)
+		return -ENODEV;
+	if (cluster >= HIP04_MAX_CLUSTERS || cpu >= HIP04_MAX_CPUS_PER_CLUSTER)
+		return -EINVAL;
+
+	spin_lock_irq(&boot_lock);
+	writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+	writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+	writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+	writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c new file +18 -0

+	return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster;
+	bool skip_wfi = false;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	__mcpm_cpu_going_down(cpu, cluster);
+
+	spin_lock(&boot_lock);
+	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+	hip04_cpu_table[cluster][cpu]--;
+	if (hip04_cpu_table[cluster][cpu] == 1) {

arch/arm/mach-hisi/platmcpm.c new file +11 -0

+
+	if (!skip_wfi)
+		wfi();
+}
+
+static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int cluster)
+{
+	unsigned int data, tries;
+
+	BUG_ON(cluster >= HIP04_MAX_CLUSTERS ||
+	       cpu >= HIP04_MAX_CPUS_PER_CLUSTER);

arch/arm/mach-hisi/platmcpm.c new file +118 -0

+		return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static void hip04_mcpm_powered_up(void)
+{
+	if (!relocation)
+		return;
+	spin_lock(&boot_lock);
+	writel_relaxed(0, relocation);
+	writel_relaxed(0, relocation + 4);
+	writel_relaxed(0, relocation + 8);
+	writel_relaxed(0, relocation + 12);
+	spin_unlock(&boot_lock);
+}
+
+static const struct mcpm_platform_ops hip04_mcpm_ops = {
+	.power_up		= hip04_mcpm_power_up,
+	.power_down		= hip04_mcpm_power_down,
+	.wait_for_powerdown	= hip04_mcpm_wait_for_powerdown,
+	.powered_up		= hip04_mcpm_powered_up,
+};
+
+static bool __init hip04_cpu_table_init(void)
+{
+	unsigned int mpidr, cpu, cluster;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	if (cluster >= HIP04_MAX_CLUSTERS ||
+	    cpu >= HIP04_MAX_CPUS_PER_CLUSTER) {
+		pr_err("%s: boot CPU is out of bound!\n", __func__);
+		return false;
+	}
+	hip04_set_snoop_filter(cluster, 1);
+	hip04_cpu_table[cluster][cpu] = 1;
+	return true;
+}
+
+static int __init hip04_mcpm_init(void)
+{
+	struct device_node *np, *np_fab;
+	int ret = -ENODEV;
+
+	np = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+	if (!np)
+		goto err;
+	np_fab = of_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
+	if (!np_fab)
+		goto err;
+
+	if (of_property_read_u32(np, "bootwrapper-phys",
+				 &hip04_boot.bootwrapper_phys)) {
+		pr_err("failed to get bootwrapper-phys\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "bootwrapper-size",
+				 &hip04_boot.bootwrapper_size)) {
+		pr_err("failed to get bootwrapper-size\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "bootwrapper-magic",
+				 &hip04_boot.bootwrapper_magic)) {
+		pr_err("failed to get bootwrapper-magic\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "relocation-entry",
+				 &hip04_boot.relocation_entry)) {
+		pr_err("failed to get relocation-entry\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "relocation-size",
+				 &hip04_boot.relocation_size)) {
+		pr_err("failed to get relocation-size\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	relocation = ioremap(hip04_boot.relocation_entry,
+			     hip04_boot.relocation_size);
+	if (!relocation) {
+		pr_err("failed to map relocation space\n");
+		ret = -ENOMEM;
+		goto err;
+	}
+	sysctrl = of_iomap(np, 0);
+	if (!sysctrl) {
+		pr_err("failed to get sysctrl base\n");
+		ret = -ENOMEM;
+		goto err_sysctrl;
+	}
+	fabric = of_iomap(np_fab, 0);
+	if (!fabric) {
+		pr_err("failed to get fabric base\n");
+		ret = -ENOMEM;
+		goto err_fabric;
+	}
+
+	if (!hip04_cpu_table_init())
+		return -EINVAL;
+	ret = mcpm_platform_register(&hip04_mcpm_ops);
+	if (!ret) {
+		mcpm_sync_init(NULL);
+		pr_info("HiP04 MCPM initialized\n");
+	}
+	return ret;
+err_fabric:
+	iounmap(sysctrl);
+err_sysctrl:
+	iounmap(relocation);

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 123

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

Quoted 3 lines

Quoted 2 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

Quoted 2 lines

What is that msleep() for?

Without this delay, I'll meet hang.

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+     return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster;
+     bool skip_wfi = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     spin_lock(&boot_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     hip04_cpu_table[cluster][cpu]--;
+     if (hip04_cpu_table[cluster][cpu] == 1) {
+             /* A power_up request went ahead of us. */
+             skip_wfi = true;
+     } else if (hip04_cpu_table[cluster][cpu] > 1) {
+             pr_err("Cluster %d CPU%d is still running\n", cluster,

cpu);
+ BUG();

Quoted 9 lines

arch/arm/mach-hisi/platmcpm.c line 160

Quoted 5 lines

arch/arm/mach-hisi/platmcpm.c new file +5 -0

+     }
+
+     spin_unlock(&boot_lock);
+
+     v7_exit_coherency_flush(louis);

Don't you have to turn off snoop filters in the case of a complete cluster down as well?

I still have some issue on disabling a whole cluster. So making it completely
will be finished later in additional patch.

Quoted 9 lines

arch/arm/mach-hisi/platmcpm.c line 165

Quoted 5 lines

arch/arm/mach-hisi/platmcpm.c new file +5 -0

+
+     __mcpm_cpu_down(cpu, cluster);
+
+     isb();
+     dsb();

Those isb/dsb are redundant here. They're implicit from __mcpm_cpu_down().

OK. I'll remove it.

Quoted 28 lines

Quoted 16 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     if (!skip_wfi)
+             wfi();
+}
+
+static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int

cluster)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+{
+     unsigned int data, tries;
+
+     BUG_ON(cluster >= HIP04_MAX_CLUSTERS ||
+            cpu >= HIP04_MAX_CPUS_PER_CLUSTER);
+
+     for (tries = 0; tries < TIMEOUT_MSEC / POLL_MSEC; tries++) {
+             data = readl_relaxed(sysctrl +

SC_CPU_RESET_STATUS(cluster));

arch/arm/mach-hisi/platmcpm.c line 186

Quoted 7 lines

arch/arm/mach-hisi/platmcpm.c new file +7 -0

+             if (!(data & CORE_WFI_STATUS(cpu))) {
+                     msleep(POLL_MSEC);
+                     continue;
+             }
+             data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+                    CORE_DEBUG_RESET_BIT(cpu);
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_REQ(cluster));

You are not supposed to perform any changes here. This method is there only to wait for given CPU to be down. It is not guaranteed it will be called. Furthermore this can race with the power_up method.

Quoted 6 lines

Normally you should perform this reset business from hip04_mcpm_power_down() while the lock is held. Now... this assumes that any power/reset would be effective only when the CPU executes a WFI. Is this the case on this hardware? If not then we'll have to think about a different strategy.

OK. I can move reset operation in hip04_mcpm_power_down().

Quoted 124 lines

Quoted 124 lines

arch/arm/mach-hisi/platmcpm.c new file +52 -0

+             return 0;
+     }
+
+     return -ETIMEDOUT;
+}
+
+static void hip04_mcpm_powered_up(void)
+{
+     if (!relocation)
+             return;
+     spin_lock(&boot_lock);
+     writel_relaxed(0, relocation);
+     writel_relaxed(0, relocation + 4);
+     writel_relaxed(0, relocation + 8);
+     writel_relaxed(0, relocation + 12);
+     spin_unlock(&boot_lock);
+}
+
+static const struct mcpm_platform_ops hip04_mcpm_ops = {
+     .power_up               = hip04_mcpm_power_up,
+     .power_down             = hip04_mcpm_power_down,
+     .wait_for_powerdown     = hip04_mcpm_wait_for_powerdown,
+     .powered_up             = hip04_mcpm_powered_up,
+};
+
+static bool __init hip04_cpu_table_init(void)
+{
+     unsigned int mpidr, cpu, cluster;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     if (cluster >= HIP04_MAX_CLUSTERS ||
+         cpu >= HIP04_MAX_CPUS_PER_CLUSTER) {
+             pr_err("%s: boot CPU is out of bound!\n", __func__);
+             return false;
+     }
+     hip04_set_snoop_filter(cluster, 1);
+     hip04_cpu_table[cluster][cpu] = 1;
+     return true;
+}
+
+static int __init hip04_mcpm_init(void)
+{
+     struct device_node *np, *np_fab;
+     int ret = -ENODEV;
+
+     np = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+     if (!np)
+             goto err;
+     np_fab = of_find_compatible_node(NULL, NULL,

"hisilicon,hip04-fabric");

arch/arm/mach-hisi/platmcpm.c new file +71 -0

+     if (!np_fab)
+             goto err;
+
+     if (of_property_read_u32(np, "bootwrapper-phys",
+                              &hip04_boot.bootwrapper_phys)) {
+             pr_err("failed to get bootwrapper-phys\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "bootwrapper-size",
+                              &hip04_boot.bootwrapper_size)) {
+             pr_err("failed to get bootwrapper-size\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "bootwrapper-magic",
+                              &hip04_boot.bootwrapper_magic)) {
+             pr_err("failed to get bootwrapper-magic\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "relocation-entry",
+                              &hip04_boot.relocation_entry)) {
+             pr_err("failed to get relocation-entry\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "relocation-size",
+                              &hip04_boot.relocation_size)) {
+             pr_err("failed to get relocation-size\n");
+             ret = -EINVAL;
+             goto err;
+     }
+
+     relocation = ioremap(hip04_boot.relocation_entry,
+                          hip04_boot.relocation_size);
+     if (!relocation) {
+             pr_err("failed to map relocation space\n");
+             ret = -ENOMEM;
+             goto err;
+     }
+     sysctrl = of_iomap(np, 0);
+     if (!sysctrl) {
+             pr_err("failed to get sysctrl base\n");
+             ret = -ENOMEM;
+             goto err_sysctrl;
+     }
+     fabric = of_iomap(np_fab, 0);
+     if (!fabric) {
+             pr_err("failed to get fabric base\n");
+             ret = -ENOMEM;
+             goto err_fabric;
+     }
+
+     if (!hip04_cpu_table_init())
+             return -EINVAL;
+     ret = mcpm_platform_register(&hip04_mcpm_ops);
+     if (!ret) {
+             mcpm_sync_init(NULL);
+             pr_info("HiP04 MCPM initialized\n");
+     }
+     return ret;
+err_fabric:
+     iounmap(sysctrl);
+err_sysctrl:
+     iounmap(relocation);
+err:
+     return ret;
+}
+early_initcall(hip04_mcpm_init);
+

Quoted 9 lines

arch/arm/mach-hisi/platmcpm.c line 314

Quoted 5 lines

arch/arm/mach-hisi/platmcpm.c new file +5 -0

+bool __init hip04_smp_init_ops(void)
+{
+     mcpm_smp_set_ops();
+     return true;
+}

You should be able to call mcpm_smp_set_ops() directly from hip04_mcpm_init(), and only if there was no errors.

if (!mdesc->smp_init || !mdesc->smp_init()) {
        if (psci_smp_available())
                smp_set_ops(&psci_smp_ops);
        else if (mdesc->smp)
                smp_set_ops(mdesc->smp);
}

Since the above code checks the return value of mdesc->smp_init(), I think it's better to call mcpm_smp_set_ops() in mdesc->smp_init().

Otherwise, my mdesc->smp_init() only return true without doing anything. It'll confuse others to understand the code.

Quoted 2 lines

Nicolas

Quoted 44 lines · Haojian Zhuang

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 123

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 8 lines

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

Quoted 7 lines

Quoted 2 lines

What is that msleep() for?

Without this delay, I'll meet hang.

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Quoted 34 lines

Quoted 33 lines

Quoted 23 lines

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+     return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster;
+     bool skip_wfi = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     spin_lock(&boot_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     hip04_cpu_table[cluster][cpu]--;
+     if (hip04_cpu_table[cluster][cpu] == 1) {
+             /* A power_up request went ahead of us. */
+             skip_wfi = true;
+     } else if (hip04_cpu_table[cluster][cpu] > 1) {
+             pr_err("Cluster %d CPU%d is still running\n", cluster,

cpu);

arch/arm/mach-hisi/platmcpm.c line 160

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+             BUG();
+     }
+
+     spin_unlock(&boot_lock);
+
+     v7_exit_coherency_flush(louis);

Don't you have to turn off snoop filters in the case of a complete cluster down as well?

Quoted 3 lines

I still have some issue on disabling a whole cluster. So making it completely
will be finished later in additional patch.

Quoted 190 lines

Quoted 11 lines

arch/arm/mach-hisi/platmcpm.c line 165

Quoted 7 lines

arch/arm/mach-hisi/platmcpm.c new file +5 -0

+
+     __mcpm_cpu_down(cpu, cluster);
+
+     isb();
+     dsb();

Those isb/dsb are redundant here. They're implicit from __mcpm_cpu_down().

OK. I'll remove it.

Quoted 28 lines

Quoted 16 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     if (!skip_wfi)
+             wfi();
+}
+
+static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int

cluster)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+{
+     unsigned int data, tries;
+
+     BUG_ON(cluster >= HIP04_MAX_CLUSTERS ||
+            cpu >= HIP04_MAX_CPUS_PER_CLUSTER);
+
+     for (tries = 0; tries < TIMEOUT_MSEC / POLL_MSEC; tries++) {
+             data = readl_relaxed(sysctrl +

SC_CPU_RESET_STATUS(cluster));

arch/arm/mach-hisi/platmcpm.c line 186

Quoted 7 lines

arch/arm/mach-hisi/platmcpm.c new file +7 -0

+             if (!(data & CORE_WFI_STATUS(cpu))) {
+                     msleep(POLL_MSEC);
+                     continue;
+             }
+             data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+                    CORE_DEBUG_RESET_BIT(cpu);
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_REQ(cluster));

You are not supposed to perform any changes here. This method is there only to wait for given CPU to be down. It is not guaranteed it will be called. Furthermore this can race with the power_up method.

Quoted 6 lines

Normally you should perform this reset business from hip04_mcpm_power_down() while the lock is held. Now... this assumes that any power/reset would be effective only when the CPU executes a WFI. Is this the case on this hardware? If not then we'll have to think about a different strategy.

OK. I can move reset operation in hip04_mcpm_power_down().

Quoted 124 lines

Quoted 124 lines

arch/arm/mach-hisi/platmcpm.c new file +52 -0

+             return 0;
+     }
+
+     return -ETIMEDOUT;
+}
+
+static void hip04_mcpm_powered_up(void)
+{
+     if (!relocation)
+             return;
+     spin_lock(&boot_lock);
+     writel_relaxed(0, relocation);
+     writel_relaxed(0, relocation + 4);
+     writel_relaxed(0, relocation + 8);
+     writel_relaxed(0, relocation + 12);
+     spin_unlock(&boot_lock);
+}
+
+static const struct mcpm_platform_ops hip04_mcpm_ops = {
+     .power_up               = hip04_mcpm_power_up,
+     .power_down             = hip04_mcpm_power_down,
+     .wait_for_powerdown     = hip04_mcpm_wait_for_powerdown,
+     .powered_up             = hip04_mcpm_powered_up,
+};
+
+static bool __init hip04_cpu_table_init(void)
+{
+     unsigned int mpidr, cpu, cluster;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     if (cluster >= HIP04_MAX_CLUSTERS ||
+         cpu >= HIP04_MAX_CPUS_PER_CLUSTER) {
+             pr_err("%s: boot CPU is out of bound!\n", __func__);
+             return false;
+     }
+     hip04_set_snoop_filter(cluster, 1);
+     hip04_cpu_table[cluster][cpu] = 1;
+     return true;
+}
+
+static int __init hip04_mcpm_init(void)
+{
+     struct device_node *np, *np_fab;
+     int ret = -ENODEV;
+
+     np = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+     if (!np)
+             goto err;
+     np_fab = of_find_compatible_node(NULL, NULL,

"hisilicon,hip04-fabric");

arch/arm/mach-hisi/platmcpm.c new file +71 -0

+     if (!np_fab)
+             goto err;
+
+     if (of_property_read_u32(np, "bootwrapper-phys",
+                              &hip04_boot.bootwrapper_phys)) {
+             pr_err("failed to get bootwrapper-phys\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "bootwrapper-size",
+                              &hip04_boot.bootwrapper_size)) {
+             pr_err("failed to get bootwrapper-size\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "bootwrapper-magic",
+                              &hip04_boot.bootwrapper_magic)) {
+             pr_err("failed to get bootwrapper-magic\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "relocation-entry",
+                              &hip04_boot.relocation_entry)) {
+             pr_err("failed to get relocation-entry\n");
+             ret = -EINVAL;
+             goto err;
+     }
+     if (of_property_read_u32(np, "relocation-size",
+                              &hip04_boot.relocation_size)) {
+             pr_err("failed to get relocation-size\n");
+             ret = -EINVAL;
+             goto err;
+     }
+
+     relocation = ioremap(hip04_boot.relocation_entry,
+                          hip04_boot.relocation_size);
+     if (!relocation) {
+             pr_err("failed to map relocation space\n");
+             ret = -ENOMEM;
+             goto err;
+     }
+     sysctrl = of_iomap(np, 0);
+     if (!sysctrl) {
+             pr_err("failed to get sysctrl base\n");
+             ret = -ENOMEM;
+             goto err_sysctrl;
+     }
+     fabric = of_iomap(np_fab, 0);
+     if (!fabric) {
+             pr_err("failed to get fabric base\n");
+             ret = -ENOMEM;
+             goto err_fabric;
+     }
+
+     if (!hip04_cpu_table_init())
+             return -EINVAL;
+     ret = mcpm_platform_register(&hip04_mcpm_ops);
+     if (!ret) {
+             mcpm_sync_init(NULL);
+             pr_info("HiP04 MCPM initialized\n");
+     }
+     return ret;
+err_fabric:
+     iounmap(sysctrl);
+err_sysctrl:
+     iounmap(relocation);
+err:
+     return ret;
+}
+early_initcall(hip04_mcpm_init);
+

Quoted 9 lines

arch/arm/mach-hisi/platmcpm.c line 314

Quoted 5 lines

arch/arm/mach-hisi/platmcpm.c new file +5 -0

+bool __init hip04_smp_init_ops(void)
+{
+     mcpm_smp_set_ops();
+     return true;
+}

You should be able to call mcpm_smp_set_ops() directly from hip04_mcpm_init(), and only if there was no errors.

if (!mdesc->smp_init || !mdesc->smp_init()) {
        if (psci_smp_available())
                smp_set_ops(&psci_smp_ops);
        else if (mdesc->smp)
                smp_set_ops(mdesc->smp);
}

Quoted 5 lines

Since the above code checks the return value of mdesc->smp_init(), I think it's better to call mcpm_smp_set_ops() in mdesc->smp_init().

Otherwise, my mdesc->smp_init() only return true without doing anything. It'll confuse others to understand the code.

Quoted 45 lines · Haojian Zhuang

Quoted 44 lines · Haojian Zhuang

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 123

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

That's my point. At least on systems with the CCI, you risk hanging the system if you enable snoops for a CPU that is not yet running.

Yet you cannot enable the CPU and then turn on snoops as you have the opposite race in that case.

So the only way to be absolutely safe is to let the waking-up CPU turn on snoops for itself _before_ it starts using the cache. That is what the power_up_setup callback provided to mcpm_sync_init() is all about.

It must be written all in assembly as there is no stack available yet when it is called. See arch/arm/mach-vexpress/dcscb_setup.S for an example skeleton where cci_enable_port_for_self is called when the affinity level is 1 (cluster level) and nothing special is done when the affinity level is 0 (CPU level). All the synchronization and race avoidance is all handled for you in arch/arm/common/mcpm_head.S already.

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

What is that msleep() for?

Without this delay, I'll meet hang.

Quoted 3 lines

Quoted 3 lines

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Could you confirm that the IPI is actually what causes you trouble?

If so maybe you should make sure that the CPU is actually masking out IRQs at all times before they're unmasked by the kernel.

Quoted 34 lines

Quoted 34 lines

Quoted 33 lines

Quoted 23 lines

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+     return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster;
+     bool skip_wfi = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     spin_lock(&boot_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     hip04_cpu_table[cluster][cpu]--;
+     if (hip04_cpu_table[cluster][cpu] == 1) {
+             /* A power_up request went ahead of us. */
+             skip_wfi = true;
+     } else if (hip04_cpu_table[cluster][cpu] > 1) {
+             pr_err("Cluster %d CPU%d is still running\n", cluster,

cpu);

arch/arm/mach-hisi/platmcpm.c line 160

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+             BUG();
+     }
+
+     spin_unlock(&boot_lock);
+
+     v7_exit_coherency_flush(louis);

Don't you have to turn off snoop filters in the case of a complete cluster down as well?

Quoted 3 lines

Quoted 3 lines

I still have some issue on disabling a whole cluster. So making it completely
will be finished later in additional patch.

Most likely related to my commentabove.

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

You should be able to call mcpm_smp_set_ops() directly from hip04_mcpm_init(), and only if there was no errors.

if (!mdesc->smp_init || !mdesc->smp_init()) {
        if (psci_smp_available())
                smp_set_ops(&psci_smp_ops);
        else if (mdesc->smp)
                smp_set_ops(mdesc->smp);
}

Quoted 5 lines

Quoted 5 lines

Since the above code checks the return value of mdesc->smp_init(), I think it's better to call mcpm_smp_set_ops() in mdesc->smp_init().

Otherwise, my mdesc->smp_init() only return true without doing anything. It'll confuse others to understand the code.

Simply don't provide a mdesc->smp_init at all. If you have a default value for mdesc->smp then it'll be replaced when hip04_mcpm_init() completes successfully.

Nicolas

Quoted 66 lines · Nicolas Pitre

Quoted 45 lines · Haojian Zhuang

Quoted 44 lines · Haojian Zhuang

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 123

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 6 lines

arch/arm/mach-hisi/platmcpm.c new file +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

That's my point. At least on systems with the CCI, you risk hanging the system if you enable snoops for a CPU that is not yet running.

Yet you cannot enable the CPU and then turn on snoops as you have the opposite race in that case.

So the only way to be absolutely safe is to let the waking-up CPU turn on snoops for itself _before_ it starts using the cache. That is what the power_up_setup callback provided to mcpm_sync_init() is all about.

Quoted 7 lines

It must be written all in assembly as there is no stack available yet when it is called. See arch/arm/mach-vexpress/dcscb_setup.S for an example skeleton where cci_enable_port_for_self is called when the affinity level is 1 (cluster level) and nothing special is done when the affinity level is 0 (CPU level). All the synchronization and race avoidance is all handled for you in arch/arm/common/mcpm_head.S already.

There's no CCI in HiP04. Their own fabric is used instead.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

What is that msleep() for?

Without this delay, I'll meet hang.

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Could you confirm that the IPI is actually what causes you trouble?

If so maybe you should make sure that the CPU is actually masking out IRQs at all times before they're unmasked by the kernel.

I think so. But I don't have any hardware debugger to confirm it.

I think that unnecessary interrupt causes new CPU in interrupt mode, then this CPU can't be online.

If I remove the code to send IPI, the new CPU could be online right.

Quoted 75 lines · Haojian Zhuang

Quoted 73 lines · Nicolas Pitre

Quoted 45 lines · Haojian Zhuang

Quoted 44 lines · Haojian Zhuang

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

(quoted diff) +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

(quoted diff) +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

That's my point. At least on systems with the CCI, you risk hanging the system if you enable snoops for a CPU that is not yet running.

Yet you cannot enable the CPU and then turn on snoops as you have the opposite race in that case.

So the only way to be absolutely safe is to let the waking-up CPU turn on snoops for itself _before_ it starts using the cache. That is what the power_up_setup callback provided to mcpm_sync_init() is all about.

It must be written all in assembly as there is no stack available yet when it is called. See arch/arm/mach-vexpress/dcscb_setup.S for an example skeleton where cci_enable_port_for_self is called when the affinity level is 1 (cluster level) and nothing special is done when the affinity level is 0 (CPU level). All the synchronization and race avoidance is all handled for you in arch/arm/common/mcpm_head.S already.

Quoted 1 line

There's no CCI in HiP04. Their own fabric is used instead.

OK. Still, in order to support deep C-States with cpuidle, the waking-up of CPUs must happen asynchronously, and in that case the CPU being awaken must do snooping setup by itself. So I'd be interested to see if that can be down that way as well for HiP04, at least to be similar in structure to other backend implementations, and possibly for being cpuidle ready as well.

Quoted 21 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

What is that msleep() for?

Without this delay, I'll meet hang.

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Could you confirm that the IPI is actually what causes you trouble?

If so maybe you should make sure that the CPU is actually masking out IRQs at all times before they're unmasked by the kernel.

I think so. But I don't have any hardware debugger to confirm it.

Quoted 4 lines

I think that unnecessary interrupt causes new CPU in interrupt mode, then this CPU can't be online.

If I remove the code to send IPI, the new CPU could be online right.

If so that's a bug in the firmware/bootloader/whatever code that needs to be fixed. CPUs must never unmasq IRQs if they're not ready to cope with them.

Quoted 77 lines · Nicolas Pitre

Quoted 75 lines · Haojian Zhuang

Quoted 73 lines · Nicolas Pitre

Quoted 45 lines · Haojian Zhuang

Quoted 44 lines · Haojian Zhuang

Quoted 33 lines · Nicolas Pitre

Quoted 17 lines · Haojian Zhuang

(quoted diff) +8 -0

+
+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >=

HIP04_MAX_CPUS_PER_CLUSTER)

(quoted diff) +8 -0

+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Isn't this racy? What if the new CPU starts executing code before snoops are enabled for it?

Quoted 5 lines

Normally we solve this race by letting the waking-up CPU turn on its snoops by itself from an assembly code callback provided as argument to mcpm_sync_init(). This is also the only way to eventually support deep C-states with cpuidle.

There's no race. At here, I enable snoop without unreset new CPU.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));

At here, I make new CPU out of reset state.

That's my point. At least on systems with the CCI, you risk hanging the system if you enable snoops for a CPU that is not yet running.

Yet you cannot enable the CPU and then turn on snoops as you have the opposite race in that case.

So the only way to be absolutely safe is to let the waking-up CPU turn on snoops for itself _before_ it starts using the cache. That is what the power_up_setup callback provided to mcpm_sync_init() is all about.

It must be written all in assembly as there is no stack available yet when it is called. See arch/arm/mach-vexpress/dcscb_setup.S for an example skeleton where cci_enable_port_for_self is called when the affinity level is 1 (cluster level) and nothing special is done when the affinity level is 0 (CPU level). All the synchronization and race avoidance is all handled for you in arch/arm/common/mcpm_head.S already.

Quoted 9 lines

Quoted 1 line

There's no CCI in HiP04. Their own fabric is used instead.

OK. Still, in order to support deep C-States with cpuidle, the waking-up of CPUs must happen asynchronously, and in that case the CPU being awaken must do snooping setup by itself. So I'd be interested to see if that can be down that way as well for HiP04, at least to be similar in structure to other backend implementations, and possibly for being cpuidle ready as well.

But it fails on my platform if I execute snooping setup on the new CPU.

Quoted 25 lines

Quoted 24 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

What is that msleep() for?

Without this delay, I'll meet hang.

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Could you confirm that the IPI is actually what causes you trouble?

If so maybe you should make sure that the CPU is actually masking out IRQs at all times before they're unmasked by the kernel.

I think so. But I don't have any hardware debugger to confirm it.

I think that unnecessary interrupt causes new CPU in interrupt mode, then this CPU can't be online.

If I remove the code to send IPI, the new CPU could be online right.

Quoted 4 lines

If so that's a bug in the firmware/bootloader/whatever code that needs to be fixed. CPUs must never unmasq IRQs if they're not ready to cope with them.

I agree that it should be fixed in firmware. But I need some time. So I want to keep the workaround until the bug is fixed in firmware.

Quoted 12 lines · Haojian Zhuang

Quoted 10 lines · Nicolas Pitre

Quoted 2 lines

There's no CCI in HiP04. Their own fabric is used instead.

OK. Still, in order to support deep C-States with cpuidle, the waking-up of CPUs must happen asynchronously, and in that case the CPU being awaken must do snooping setup by itself. So I'd be interested to see if that can be down that way as well for HiP04, at least to be similar in structure to other backend implementations, and possibly for being cpuidle ready as well.

Quoted 1 line

But it fails on my platform if I execute snooping setup on the new CPU.

It fails how? I want to make sure if the problem is with the hardware design or the code.

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

What is that msleep() for?

Without this delay, I'll meet hang.

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Since I'm using reset to wakeup CPU from wfi/wfe status, I don't need IPI at here. If I remove the msleep() here, it seems that IPI caused my system hang. So it's a workaround in my system.

Could you confirm that the IPI is actually what causes you trouble?

If so maybe you should make sure that the CPU is actually masking out IRQs at all times before they're unmasked by the kernel.

I think so. But I don't have any hardware debugger to confirm it.

I think that unnecessary interrupt causes new CPU in interrupt mode, then this CPU can't be online.

If I remove the code to send IPI, the new CPU could be online right.

Quoted 7 lines

Quoted 4 lines

If so that's a bug in the firmware/bootloader/whatever code that needs to be fixed. CPUs must never unmasq IRQs if they're not ready to cope with them.

I agree that it should be fixed in firmware. But I need some time. So I want to keep the workaround until the bug is fixed in firmware.

This is still a very bad workaround. As an alternative, there might be a way to simply mask the IPI at the GIC level and unmask it from the powered_up method.

Also... if this is going to be fixed in the firmware, maybe not including such a workaround into the upstream submission will keep the incentive alive for having it actually fixed.

Quoted 16 lines · Nicolas Pitre

Quoted 13 lines · Haojian Zhuang

Quoted 10 lines · Nicolas Pitre

Quoted 2 lines

There's no CCI in HiP04. Their own fabric is used instead.

OK. Still, in order to support deep C-States with cpuidle, the waking-up of CPUs must happen asynchronously, and in that case the CPU being awaken must do snooping setup by itself. So I'd be interested to see if that can be down that way as well for HiP04, at least to be similar in structure to other backend implementations, and possibly for being cpuidle ready as well.

But it fails on my platform if I execute snooping setup on the new CPU.

Quoted 3 lines

It fails how? I want to make sure if the problem is with the hardware design or the code.

The failure isn't caused by your assembly code. I even rewrite it.

It's caused by hardware design. Hisilicon guys said the snooping setup should be set before new CPU on. I tried and got the same result.

Select HiP04 SoC configuration by hip04_defconfig.

Signed-off-by Haojian Zhuang <[email protected]>

Quoted 3 lines · Haojian Zhuang

Select HiP04 SoC configuration by hip04_defconfig.

Signed-off-by Haojian Zhuang <[email protected]>

We're letting each vendor have one defconfig. This should coexist and/or add to the hi3xxx defconfig instead of creating a new one.

-Olof

From: Kefeng Wang <[email protected]>

ARM Performance Monitor Units are available on the HiP04. So add the support in the dtsi.

Simply tested with perf.

Signed-off-by Kefeng Wang <[email protected]>

Signed-off-by Haojian Zhuang <[email protected]>

Quoted 7 lines · Haojian Zhuang

From: Kefeng Wang <[email protected]>

ARM Performance Monitor Units are available on the HiP04. So add the support in the dtsi.

Simply tested with perf.

Quoted 2 lines

Signed-off-by Kefeng Wang <[email protected]>

Signed-off-by Haojian Zhuang <[email protected]>

This should be squashed into the original dts introduction patch, no need to add these separately.

-Olof