lkml.org 
[lkml]   [2015]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 5/5] arm64: qcom: add cpu operations
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 8b9e0a9..35cabe5 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -185,6 +185,8 @@ nodes to be present and contain the properties described below.
> be one of:
> "psci"
> "spin-table"

In the case of these two, there's documentation on what the OS, FW, and
HW are expected to do. There's a PSCI spec, and spin-table is documented
in booting.txt (which is admittedly not fantastic).

> + "qcom,arm-cortex-acc"

However, this has no semantics associated with it. Per the code below
it seems to encompass more than just poking the APSS ACC, so the name
isn't great either.

[...]

> + * Based on arch/arm64/kernel/smp_spin_table.c

This is not a phrase that will ever make me happy.

[...]

> +static DEFINE_RAW_SPINLOCK(boot_lock);

We got rid of this for spin-table. It's pointless.

> +DEFINE_PER_CPU(int, cold_boot_done);

This looks suspicious.

> +#if 0
> +static int cold_boot_flags[] = {
> + 0,
> + QCOM_SCM_FLAG_COLDBOOT_CPU1,
> + QCOM_SCM_FLAG_COLDBOOT_CPU2,
> + QCOM_SCM_FLAG_COLDBOOT_CPU3,
> +};
> +#endif

I take it this shouldn't be here?

[...]

> +static int power_on_l2_msm8916(struct device_node *l2ccc_node, u32 pon_mask,
> + int cpu)
> +{
> + u32 pon_status;
> + void __iomem *l2_base;
> +
> + l2_base = of_iomap(l2ccc_node, 0);
> + if (!l2_base)
> + return -ENOMEM;

I didn't see any mention of an L2 requiring power-up in the rest of the
series...

[...]

> +static void write_pen_release(u64 val)
> +{
> + void *start = (void *)&secondary_holding_pen_release;
> + unsigned long size = sizeof(secondary_holding_pen_release);
> +
> + secondary_holding_pen_release = val;
> + smp_wmb();
> + __flush_dcache_area(start, size);
> +}
> +
> +static int secondary_pen_release(unsigned int cpu)
> +{
> + unsigned long timeout;
> +
> + /*
> + * Set synchronisation state between this boot processor
> + * and the secondary one
> + */
> + raw_spin_lock(&boot_lock);
> + write_pen_release(cpu_logical_map(cpu));
> +
> + timeout = jiffies + (1 * HZ);
> + while (time_before(jiffies, timeout)) {
> + if (secondary_holding_pen_release == INVALID_HWID)
> + break;
> + udelay(10);
> + }
> + raw_spin_unlock(&boot_lock);
> +
> + return secondary_holding_pen_release != INVALID_HWID ? -ENOSYS : 0;
> +}

So you want to share the pen, but duplicate the code for managing it?

> +static int __init msm_cpu_init(struct device_node *dn, unsigned int cpu)
> +{
> + /* Mark CPU0 cold boot flag as done */
> + if (!cpu && !per_cpu(cold_boot_done, cpu))
> + per_cpu(cold_boot_done, cpu) = true;
> +
> + return 0;
> +}

[...]

> +static int msm_cpu_boot(unsigned int cpu)
> +{
> + int ret = 0;
> +
> + if (per_cpu(cold_boot_done, cpu) == false) {
> + ret = msm_unclamp_secondary_arm_cpu(cpu);
> + if (ret)
> + return ret;
> + per_cpu(cold_boot_done, cpu) = true;
> + }
> + return secondary_pen_release(cpu);
> +}

Ah, so cold_boot_done is for pseudo-hotplug. Absolute NAK to that.

The only thing this gives you over spin-table is one-time powering up of
the CPUs that can be performed prior to entry to Linux. If you do that,
you can trivially share the spin-table code by setting each CPU's
enable-method to "spin-table".

That won't give you cpuidle or actual hotplug. For those you'll need
PSCI.

Mark.


\
 
 \ /
  Last update: 2015-04-14 19:01    [W:0.274 / U:2.768 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site