[PATCH v5 08/14] ARM: hisi: add hip04 SoC support
Nicolas Pitre
nicolas.pitre at linaro.org
Mon May 12 12:39:59 PDT 2014
On Mon, 12 May 2014, Haojian Zhuang wrote:
> 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.
That's my point. At least on systems with the CCI, you risk hanging the
system if you enable snoops for a CPU that is not yet running.
Yet you cannot enable the CPU and then turn on snoops as you have the
opposite race in that case.
So the only way to be absolutely safe is to let the waking-up CPU turn
on snoops for itself _before_ it starts using the cache. That is what
the power_up_setup callback provided to mcpm_sync_init() is all about.
It must be written all in assembly as there is no stack available yet
when it is called. See arch/arm/mach-vexpress/dcscb_setup.S for an
example skeleton where cci_enable_port_for_self is called when the
affinity level is 1 (cluster level) and nothing special is done when the
affinity level is 0 (CPU level). All the synchronization and race
avoidance is all handled for you in arch/arm/common/mcpm_head.S already.
> >
> >
> >>> + 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.
Could you confirm that the IPI is actually what causes you trouble?
If so maybe you should make sure that the CPU is actually masking out
IRQs at all times before they're unmasked by the kernel.
> >>> + 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.
Most likely related to my commentabove.
> >
> >
> >>> +
> >>> + __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.
Simply don't provide a mdesc->smp_init at all. If you have a default
value for mdesc->smp then it'll be replaced when hip04_mcpm_init()
completes successfully.
Nicolas
More information about the linux-arm-kernel
mailing list