Re: [PATCH v2 6/6] msm: add SMP support for msm

[Jeff Ohlstein]

Russell King - ARM Linux wrote:
> On Tue, Dec 07, 2010 at 08:28:21PM -0800, Jeff Ohlstein wrote:
> > +int get_core_count(void)
> > +{
> > +#ifdef CONFIG_NR_CPUS
> > +	return CONFIG_NR_CPUS;
> > +#else
> > +	return 1;
> > +#endif
> >     
>
> When would CONFIG_NR_CPUS not be set when SMP is available?
>
> Note that linux/threads.h defines this to 1 if it's not already defined
> anyway.  Does this really need to be a separate function?
>
>   
Removed.
> > +int boot_secondary(unsigned int cpu, struct task_struct *idle)
> > +{
> > +	static int cold_boot_done;
> > +	int cnt = 0;
> > +	printk(KERN_DEBUG "Starting secondary CPU %d\n", cpu);
> > +
> > +	if (cold_boot_done == false) {
> > +		prepare_cold_cpu(cpu);
> > +		cold_boot_done = true;
> > +	}
> > +
> > +	pen_release = cpu;
> > +	dmac_flush_range((void *)&pen_release,
> > +			 (void *)(&pen_release + sizeof(pen_release)));
> >     
>
> Abuse of the DMA API.  See how other platforms deal with this.
>
>   
Fixed to use __cpuc_flush_dcache_area and outer_clean_range.
> > +	__asm__("sev");
> > +	dsb();
> > +
> > +	/* Use smp_cross_call() to send a soft interrupt to wake up
> > +	 * the other core.
> > +	 */
> > +	smp_cross_call(cpumask_of(cpu));
> > +
> > +	while (pen_release != 0xFFFFFFFF) {
> >     
>
> Why 0xFFFFFFFF rather than -1 like everyone else does?
>
>   
Removed due to below.
> > +		smp_rmb();
> > +		msleep_interruptible(1);
> > +		if (cnt++ >= SECONDARY_CPU_WAIT_MS)
> > +			break;
> > +	}
> >     
>
> And why not use the same loop as everyone else does?
>
>         timeout = jiffies + (1 * HZ);
>         while (time_before(jiffies, timeout)) {
>                 smp_rmb();
>                 if (pen_release == -1)
>                         break;
>
>                 udelay(10);
>         }
>
> IOW, what's the point of being different when you're trying to do the
> same task?
>
>   
Done.
> > +
> > +	return 0;
> > +}
> > +
> > +/* Mask for edge trigger PPIs except AVS_SVICINT and AVS_SVICINTSWDONE */
> > +#define GIC_PPI_EDGE_MASK 0xFFFFD7FF
> > +
> > +/* Initialization routine for secondary CPUs after they are brought out of
> > + * reset.
> > +*/
> > +void platform_secondary_init(unsigned int cpu)
> > +{
> > +	printk(KERN_DEBUG "%s: cpu:%d\n", __func__, cpu);
> > +
> > +	trace_hardirqs_off();
> >     
>
> This has been moved into the generic SMP code.
>
>   
I've rebased onto your gic patches and your smp series, and removed this.
> > +	if (!machine_is_msm8x60_sim())
> > +		writel(0x0000FFFF, MSM_QGIC_DIST_BASE + GIC_DIST_ENABLE_SET);
> >     
>
> The gic secondary CPU initialization now takes care of this in my tree.
>
>   
Removed.
> > +
> > +	/*
> > +	 * setup GIC (GIC number NOT CPU number and the base address of the
> > +	 * GIC CPU interface
> > +	 */
> > +	gic_cpu_init(0, MSM_QGIC_CPU_BASE);
> >     
>
> This has been renamed to gic_secondary_init() for 2.6.38.
>
>   
Done.

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/