On Tue, 20 May 2014, Haojian Zhuang wrote:

Multiple CPU clusters are used in Hisilicon HiP04 SoC. Now use MCPM
framework to manage power on HiP04 SoC.

There are still unresolved issues with this patch.

[...]

+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);
+
+	if (hip04_cpu_table[cluster][cpu]) {
+		hip04_cpu_table[cluster][cpu]++;
+		spin_unlock_irq(&boot_lock);
+		return 0;
+	}
+
+	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);
+
+	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);
+	}

Sorry to insist, but I want to repeat the question I asked during the 
previous review as I consider this is important, especially if you want 
to support deep C-States with cpuidle later.  This also has implications 
if you ever want to turn off snoops in hip04_mcpm_power_down() when all 
CPUs in a cluster are down.

You said:

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

I then asked:

| It fails how?  I want to make sure if the problem is with the hardware 
| design or the code.
| 
| The assembly code could be wrong.  Are you sure this is not theactual 
| reason?
| 
| Is there some documentation for this stuff?

I also see that the snoop filter is enabled from hip04_cpu_table_init() 
for the CPU that is actually executing that code.  So that must work 
somehow...

+
+	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));
+	spin_unlock_irq(&boot_lock);
+	msleep(POLL_MSEC);

Your cover letter for this series mentionned this:

| v9:
|   * Remove delay workaround in mcpm implementation.

Why is it still there?

+	return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster, data = 0;
+	bool skip_reset = 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_reset = true;
+	} else if (hip04_cpu_table[cluster][cpu] > 1) {
+		pr_err("Cluster %d CPU%d boots multiple times\n", cluster, cpu);
+		BUG();
+	}
+	spin_unlock(&boot_lock);
+
+	v7_exit_coherency_flush(louis);
+
+	__mcpm_cpu_down(cpu, cluster);
+
+	if (!skip_reset) {
+		data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+		       CORE_DEBUG_RESET_BIT(cpu);
+		writel_relaxed(data, sysctrl + SC_CPU_RESET_REQ(cluster));
+	}
+}

As I mentioned already this is going to race with the power_up() method.  
Let me illustrate the problem:

	* CPU 0		* CPU 1
	-------		-------
	* mcpm_cpu_power_down()
	* hip04_mcpm_power_down()
	* spin_lock(&boot_lock); [lock acquired]
			* mcpm_cpu_power_up(cpu = 0)
			* spin_lock(&boot_lock); [blocked]
	* hip04_cpu_table[cluster][cpu]--; [value down to 0]
	* skip_reset = false
	* spin_unlock(&boot_lock);
			* spin_lock(&boot_lock); [now succeeds]
	* v7_exit_coherency_flush(louis); [takes a while to complete]
			* hip04_cpu_table[cluster][cpu]++; [value back to 1]
			* bring CPU0 out of reset
	* put CPU0 into reset
			* spin_unlock(&boot_lock);

Here you end up with CPU0 in reset while hip04_cpu_table[cluster][cpu] 
for CPU0 is equal to 1.  The CPU will therefore never start again as 
further calls to power_up() won't see hip04_cpu_table equal to 0 
anymore.

So... I'm asking again: are you absolutely certain that the CPU reset is 
applied at the very moment the corresponding bit is set?  Isn't it 
applied only when the CPU does execute a WFI like most other platforms? 


Nicolas