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

[Jeff Ohlstein <johlstei@codeaurora.org>]

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.