[PATCH 5/7] ARM: EXYNOS4: Add support external GIC

[Will Deacon]

Hi Marc,

On Fri, Oct 07, 2011 at 10:44:59AM +0100, Marc Zyngier wrote:
> So to make my suggestion completely clear, here's a patch I'm now
> carrying in my tree. It's only been test compiled on EXYNOS4, but works
> nicely on my 11MP. It turns both dist_base and cpu_base into per-cpu
> variables, removes these callbacks, removes your private copy of
> gic_cpu_init, and makes struct gic_chip_data private again.
> 
> What do you think?

This looks like the right sort of idea, although I'm deeply suspicious about
per-cpu base addresses for the GIC distributor. It would be nice if the
Samsung guys can elaborate on what's going on here. Few comments inline.

> Subject: [PATCH] ARM: gic: allow GIC to support non-banked setups
> 
> The GIC support code is heavily using the fact that hardware
> implementations are exposing banked registers. Unfortunately, it
> looks like at least one GIC implementation (EXYNOS4) offers both
> the distributor and the CPU interfaces at different addresses,
> depending on the CPU.
> 
> This problem is solved by turning the distributor and CPU interface
> addresses into per-cpu variables. The EXYNOS4 code is updated not
> to mess with the GIC internals while handling interrupts, and
> struct gic_chip_data is back to being private.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
> ---
>  arch/arm/common/gic.c               |   76 +++++++++++++++++++++++++++--------
>  arch/arm/include/asm/hardware/gic.h |   17 +------
>  arch/arm/mach-exynos4/cpu.c         |   14 ------
>  arch/arm/mach-exynos4/platsmp.c     |   28 ++-----------
>  4 files changed, 66 insertions(+), 69 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index b574931..c7521dd 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -37,6 +37,20 @@
>  #include <asm/mach/irq.h>
>  #include <asm/hardware/gic.h>
>  
> +struct gic_chip_data {
> +	unsigned int irq_offset;
> +	void __percpu __iomem **dist_base;
> +	void __percpu __iomem **cpu_base;
> +#ifdef CONFIG_CPU_PM
> +	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
> +	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
> +	u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
> +	u32 __percpu *saved_ppi_enable;
> +	u32 __percpu *saved_ppi_conf;
> +#endif
> +	unsigned int gic_irqs;
> +};

I think you can use DECLARE_PER_CPU(void __iomem *dist_base) etc. instead.

> +static inline void __iomem *gic_data_dist_base(struct gic_chip_data *data)
> +{
> +	return *__this_cpu_ptr(data->dist_base);
> +}

Then you can use __get_cpu_var here (same applies throughout).

> +static void __cpuinit exynos4_secondary_init(unsigned int cpu)
>  {
>  	void __iomem *dist_base = S5P_VA_GIC_DIST +
> -				(gic_bank_offset * smp_processor_id());
> +				(gic_bank_offset * cpu_logical_map(cpu));

Again, I'm deeply suspicious of this code :) Is there not a common memory
alias for the distributor across all of the CPUs?

Will