[PATCH v5 08/14] ARM: hisi: add hip04 SoC support
Haojian Zhuang
haojian.zhuang at linaro.org
Sun May 11 17:44:20 PDT 2014
On 11 May 2014 16:02, Haojian Zhuang <haojian.zhuang at linaro.org> wrote:
>
>
> On 8 May 2014 01:44, Nicolas Pitre <nicolas.pitre at linaro.org> wrote:
>> On Wed, 7 May 2014, Haojian Zhuang wrote:
>>
>>> +
>>> +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);
>>> +
>>> + 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?
>>
>> 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.
>
>>> +
>>> + 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.
>
>
>>> + spin_unlock_irq(&boot_lock);
>>> + msleep(POLL_MSEC);
>>
>> 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.
>
I'm sorry that it's not hang. It results that new CPU cannot be online.
>>> + 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();
>>> + }
>>> +
>>> + 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.
>
>
>>> +
>>> + __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.
>
>
>>> +
>>> + 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);
>>> +
>>> + for (tries = 0; tries < TIMEOUT_MSEC / POLL_MSEC; tries++) {
>>> + data = readl_relaxed(sysctrl +
>>> SC_CPU_RESET_STATUS(cluster));
>>> + 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.
>>
>> 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().
>
>>> + 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);
>>> +err:
>>> + return ret;
>>> +}
>>> +early_initcall(hip04_mcpm_init);
>>> +
>>> +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.
>
>>
>> Nicolas
More information about the linux-arm-kernel
mailing list