[PATCH v9 05/14] ARM: hisi: enable MCPM implementation

[Nicolas Pitre]

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