Re: [PATCH 1/5] Blackfin arch: SMP supporting patchset: BF561 related code

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 18 Nov 2008 17:05:04 +0800 Bryan Wu <cooloney@kernel.org> wrote:

> From: Graf Yang <graf.yang@analog.com>
> 
> Blackfin dual core BF561 processor can support SMP like features.
> https://docs.blackfin.uclinux.org/doku.php?id=linux-kernel:smp-like
> 
> In this patch, we provide SMP extend to BF561 kernel code
> 
>
> ...
>
> --- a/arch/blackfin/mach-bf561/include/mach/mem_map.h
> +++ b/arch/blackfin/mach-bf561/include/mach/mem_map.h
> @@ -85,4 +85,124 @@
>  #define L1_SCRATCH_START	COREA_L1_SCRATCH_START
>  #define L1_SCRATCH_LENGTH	0x1000
>  
> +#ifndef __ASSEMBLY__
> +
> +#ifdef CONFIG_SMP
> +
> +#define get_l1_scratch_start_cpu(cpu)				\
> +	({ unsigned long __addr;				\
> +	   __addr = (cpu) ? COREB_L1_SCRATCH_START : COREA_L1_SCRATCH_START;\
> +	   __addr; })
> +
> +#define get_l1_code_start_cpu(cpu)				\
> +	({ unsigned long __addr;				\
> +	   __addr = (cpu) ? COREB_L1_CODE_START : COREA_L1_CODE_START;	\
> +	   __addr; })
> +
> +#define get_l1_data_a_start_cpu(cpu)				\
> +	({ unsigned long __addr;				\
> +	   __addr = (cpu) ? COREB_L1_DATA_A_START : COREA_L1_DATA_A_START;\
> +	   __addr; })
> +
> +#define get_l1_data_b_start_cpu(cpu)				\
> +	({ unsigned long __addr;				\
> +	   __addr = (cpu) ? COREB_L1_DATA_B_START : COREA_L1_DATA_B_START;\
> +	   __addr; })
> +
> +#define get_l1_scratch_start()	get_l1_scratch_start_cpu(blackfin_core_id())
> +#define get_l1_code_start()	get_l1_code_start_cpu(blackfin_core_id())
> +#define get_l1_data_a_start()	get_l1_data_a_start_cpu(blackfin_core_id())
> +#define get_l1_data_b_start()	get_l1_data_b_start_cpu(blackfin_core_id())
> +
> +#else /* !CONFIG_SMP */
> +#define get_l1_scratch_start_cpu(cpu)	L1_SCRATCH_START
> +#define get_l1_code_start_cpu(cpu)	L1_CODE_START
> +#define get_l1_data_a_start_cpu(cpu)	L1_DATA_A_START
> +#define get_l1_data_b_start_cpu(cpu)	L1_DATA_B_START
> +#define get_l1_scratch_start()		L1_SCRATCH_START
> +#define get_l1_code_start()		L1_CODE_START
> +#define get_l1_data_a_start()		L1_DATA_A_START
> +#define get_l1_data_b_start()		L1_DATA_B_START
> +#endif /* !CONFIG_SMP */

grumble.  These didn't need to be implemented as macros and hence
shouldn't have been.

Example:

	int cpu = smp_processor_id();
	get_l1_scratch_start_cpu(cpu);

that code should generate unused variable warnings on CONFIG_SMP=n.  If
it doesn't, you got lucky, because it _should_.

Also

	int cpu = smp_processor_id();
	get_l1_scratch_start_cpu(pcu);

will happily compile and run with CONFIG_SMP=n.


macros=bad,bad,bad.

>
> ...
>
> --- /dev/null
> +++ b/arch/blackfin/mach-bf561/smp.c
> @@ -0,0 +1,182 @@
> +/*
> + * File:         arch/blackfin/mach-bf561/smp.c
> + * Author:       Philippe Gerum <rpm@xenomai.org>
> + *
> + *               Copyright 2007 Analog Devices Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see the file COPYING, or write
> + * to the Free Software Foundation, Inc.,
> + * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <asm/smp.h>
> +#include <asm/dma.h>
> +
> +#define COREB_SRAM_BASE  0xff600000
> +#define COREB_SRAM_SIZE  0x4000
> +
> +extern char coreb_trampoline_start, coreb_trampoline_end;

OK, these are defined in .S and we do often put declarations for such
things in .c rather than in .h.  But I think it's better to put them in
.h anyway, to avoid possibly duplicated declarations in the future.

> +static DEFINE_SPINLOCK(boot_lock);
> +
> +static cpumask_t cpu_callin_map;
> +
>
> ...
>
> +void __cpuinit platform_secondary_init(unsigned int cpu)
> +{
> +	local_irq_disable();
> +
> +	/* Clone setup for peripheral interrupt sources from CoreA. */
> +	bfin_write_SICB_IMASK0(bfin_read_SICA_IMASK0());
> +	bfin_write_SICB_IMASK1(bfin_read_SICA_IMASK1());
> +	SSYNC();
> +
> +	/* Clone setup for IARs from CoreA. */
> +	bfin_write_SICB_IAR0(bfin_read_SICA_IAR0());
> +	bfin_write_SICB_IAR1(bfin_read_SICA_IAR1());
> +	bfin_write_SICB_IAR2(bfin_read_SICA_IAR2());
> +	bfin_write_SICB_IAR3(bfin_read_SICA_IAR3());
> +	bfin_write_SICB_IAR4(bfin_read_SICA_IAR4());
> +	bfin_write_SICB_IAR5(bfin_read_SICA_IAR5());
> +	bfin_write_SICB_IAR6(bfin_read_SICA_IAR6());
> +	bfin_write_SICB_IAR7(bfin_read_SICA_IAR7());
> +	SSYNC();
> +
> +	local_irq_enable();
> +
> +	/* Calibrate loops per jiffy value. */
> +	calibrate_delay();
> +
> +	/* Store CPU-private information to the cpu_data array. */
> +	bfin_setup_cpudata(cpu);
> +
> +	/* We are done with local CPU inits, unblock the boot CPU. */
> +	cpu_set(cpu, cpu_callin_map);
> +	spin_lock(&boot_lock);
> +	spin_unlock(&boot_lock);

Is this spin_lock()+spin_unlock() supposed to block until the secondary
CPU is running?  If so, I don't think it works.

> +}
> +
>
> ...
>