On 14 May 2014 03:43, Nicolas Pitre [off-list ref] wrote:

On Tue, 13 May 2014, Haojian Zhuang wrote:

quoted

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

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

Some more comments...

[...]

quoted

+static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
+{
+     unsigned long data;
+
+     if (!fabric)
+             return;

How could this validly be NULL?

OK. I'll make it report BUG.

quoted

+     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;
+     }
+}

The above could be easily coded in assembly for the power_up_setup
callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

        bx      lr

:LC0:   .word   .
        .word   fabric_phys_addr - .LC0

That should be it.

No. These code should be executed before new CPU on. If I transfer
them to assembler code, it means that code will be executed after
new CPU on.

Then it results me failing to make new CPU online.

quoted

+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);

Shouldn't you do the above writes only when
hip04_cpu_table[cluster][cpu] is zero?  Please see the comment in
mcpm_cpu_power_down() about unordered calls.

OK. I can add the check.

quoted

+     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);
+     }
+
+     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);
+
+     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 is still running\n", cluster, cpu);

This message is misleading.  If execution gets here, that means
mcpm_cpu_power_up() was called more than twice in a row for the same CPU
which should never happen.

OK. I'll replace the comments.

quoted

+             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));

You should not perform this outside the lock protected region as this
could race with hip04_mcpm_power_up().  Instead, this should be done
above when hip04_cpu_table[cluster][cpu] == 0 after being decremented.

No. power_down() is executed on the specified CPU. If spin_unlock() is
placed after reset operation, it means that there's no chance to
execute
the spin_unlock(). Because CPU is already in reset mode at this time.

Regards
Haojian