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:2.051 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site