Re: [PATCH 02/38] MIPS: GIC: Introduce asm/mips-gic.h with accessor functions

[Marc Zyngier <marc.zyngier@arm.com>]

On 13/08/17 05:36, Paul Burton wrote:
> This patch introduces a new header providing accessor functions for the
> MIPS Global Interrupt Controller (GIC) mirroring those provided for the
> other 2 components of the MIPS Coherent Processing System (CPS) - the
> Coherence Manager (CM) & Cluster Power Controller (CPC).
> 
> This header makes use of the new standardised CPS accessor macros where
> possible, but does require some custom accessors for cases where we have
> either a bit or a register per interrupt.
> 
> A major advantage of this over the existing
> include/linux/irqchip/mips-gic.h definitions is that code performing
> accesses can become much simpler, for example this:
> 
>   gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_TRIGGER) +
>                   GIC_INTR_OFS(intr), 1ul << GIC_INTR_BIT(intr),
>                   (unsigned long)trig << GIC_INTR_BIT(intr));
> 
> ...can become simply:
> 
>   change_gic_trig(intr, trig);
> 
> The accessors handle 32 vs 64 bit in the same way as for CM & CPC code,
> which means that GIC code will also not need to worry about the access
> size in most cases. They are also accessible outside of
> drivers/irqchip/irq-mips-gic.c which will allow for simplification in
> the use of the non-interrupt portions of the GIC (eg. counters) which
> currently require the interrupt controller driver to expose helper
> functions for access.
> 
> This patch doesn't change any existing code over to use the new
> accessors yet, since a wholesale change would be invasive & difficult to
> review. Instead follow-on patches will convert code piecemeal to use
> this new header. The one change to existing code is to rename gic_base
> to mips_gic_base & make it global, in order to fit in with the naming
> expected by the standardised CPS accessor macros.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> ---
> 
>  arch/mips/include/asm/mips-cps.h |   1 +
>  arch/mips/include/asm/mips-gic.h | 293 +++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-mips-gic.c   |  13 +-
>  3 files changed, 300 insertions(+), 7 deletions(-)
>  create mode 100644 arch/mips/include/asm/mips-gic.h
> 
> diff --git a/arch/mips/include/asm/mips-cps.h b/arch/mips/include/asm/mips-cps.h
> index 2dd737d803e1..bf02b5070a98 100644
> --- a/arch/mips/include/asm/mips-cps.h
> +++ b/arch/mips/include/asm/mips-cps.h
> @@ -107,6 +107,7 @@ static inline void clear_##unit##_##name(uint##sz##_t val)		\
>  
>  #include <asm/mips-cm.h>
>  #include <asm/mips-cpc.h>
> +#include <asm/mips-gic.h>
>  
>  /**
>   * mips_cps_numclusters - return the number of clusters present in the system
> diff --git a/arch/mips/include/asm/mips-gic.h b/arch/mips/include/asm/mips-gic.h
> new file mode 100644
> index 000000000000..8cf4bdc1a059
> --- /dev/null
> +++ b/arch/mips/include/asm/mips-gic.h
> @@ -0,0 +1,293 @@
> +/*
> + * Copyright (C) 2017 Imagination Technologies
> + * Author: Paul Burton <paul.burton@imgtec.com>
> + *
> + * 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.
> + */
> +
> +#ifndef __MIPS_ASM_MIPS_CPS_H__
> +# error Please include asm/mips-cps.h rather than asm/mips-gic.h
> +#endif
> +
> +#ifndef __MIPS_ASM_MIPS_GIC_H__
> +#define __MIPS_ASM_MIPS_GIC_H__
> +
> +#include <linux/bitops.h>
> +
> +/* The base address of the GIC registers */
> +extern void __iomem *mips_gic_base;
> +
> +/* Offsets from the GIC base address to various control blocks */
> +#define MIPS_GIC_SHARED_OFS	0x00000
> +#define MIPS_GIC_SHARED_SZ	0x08000
> +#define MIPS_GIC_LOCAL_OFS	0x08000
> +#define MIPS_GIC_LOCAL_SZ	0x04000
> +#define MIPS_GIC_REDIR_OFS	0x0c000
> +#define MIPS_GIC_REDIR_SZ	0x04000
> +#define MIPS_GIC_USER_OFS	0x10000
> +#define MIPS_GIC_USER_SZ	0x10000
> +
> +/* For read-only shared registers */
> +#define GIC_ACCESSOR_RO(sz, off, name)					\
> +	CPS_ACCESSOR_RO(gic, sz, MIPS_GIC_SHARED_OFS + off, name)
> +
> +/* For read-write shared registers */
> +#define GIC_ACCESSOR_RW(sz, off, name)					\
> +	CPS_ACCESSOR_RW(gic, sz, MIPS_GIC_SHARED_OFS + off, name)
> +
> +/* For read-only local registers */
> +#define GIC_VX_ACCESSOR_RO(sz, off, name)				\
> +	CPS_ACCESSOR_RO(gic, sz, MIPS_GIC_LOCAL_OFS + off, vl_##name)	\
> +	CPS_ACCESSOR_RO(gic, sz, MIPS_GIC_REDIR_OFS + off, vo_##name)
> +
> +/* For read-write local registers */
> +#define GIC_VX_ACCESSOR_RW(sz, off, name)				\
> +	CPS_ACCESSOR_RW(gic, sz, MIPS_GIC_LOCAL_OFS + off, vl_##name)	\
> +	CPS_ACCESSOR_RW(gic, sz, MIPS_GIC_REDIR_OFS + off, vo_##name)
> +
> +/* For read-only shared per-interrupt registers */
> +#define GIC_ACCESSOR_RO_INTR_REG(sz, off, stride, name)			\
> +static inline void __iomem *addr_gic_##name(unsigned int intr)		\
> +{									\
> +	return mips_gic_base + (off) + (intr * (stride));		\
> +}									\
> +									\
> +static inline unsigned int read_gic_##name(unsigned int intr)		\
> +{									\
> +	BUILD_BUG_ON(sz != 32);						\
> +	return __raw_readl(addr_gic_##name(intr));			\
> +}
> +
> +/* For read-write shared per-interrupt registers */
> +#define GIC_ACCESSOR_RW_INTR_REG(sz, off, stride, name)			\
> +	GIC_ACCESSOR_RO_INTR_REG(sz, off, stride, name)			\
> +									\
> +static inline void write_gic_##name(unsigned int intr,			\
> +				    unsigned int val)			\
> +{									\
> +	BUILD_BUG_ON(sz != 32);						\
> +	__raw_writel(val, addr_gic_##name(intr));			\
> +}
> +
> +/* For read-only local per-interrupt registers */
> +#define GIC_VX_ACCESSOR_RO_INTR_REG(sz, off, stride, name)		\
> +	GIC_ACCESSOR_RO_INTR_REG(sz, MIPS_GIC_LOCAL_OFS + off,		\
> +				 stride, vl_##name)			\
> +	GIC_ACCESSOR_RO_INTR_REG(sz, MIPS_GIC_REDIR_OFS + off,		\
> +				 stride, vo_##name)
> +
> +/* For read-write local per-interrupt registers */
> +#define GIC_VX_ACCESSOR_RW_INTR_REG(sz, off, stride, name)		\
> +	GIC_ACCESSOR_RW_INTR_REG(sz, MIPS_GIC_LOCAL_OFS + off,		\
> +				 stride, vl_##name)			\
> +	GIC_ACCESSOR_RW_INTR_REG(sz, MIPS_GIC_REDIR_OFS + off,		\
> +				 stride, vo_##name)
> +
> +/* For read-only shared bit-per-interrupt registers */
> +#define GIC_ACCESSOR_RO_INTR_BIT(off, name)				\
> +static inline void __iomem *addr_gic_##name(void)			\
> +{									\
> +	return mips_gic_base + (off);					\
> +}									\
> +									\
> +static inline unsigned int read_gic_##name(unsigned int intr)		\
> +{									\
> +	void __iomem *addr = addr_gic_##name();				\
> +	unsigned int val;						\
> +									\
> +	if (mips_cm_is64) {						\
> +		addr += (intr / 64) * sizeof(uint64_t);			\
> +		val = __raw_readq(addr) >> intr % 64;			\
> +	} else {							\
> +		addr += (intr / 32) * sizeof(uint32_t);			\
> +		val = __raw_readl(addr) >> intr % 32;			\
> +	}								\
> +									\
> +	return val & 0x1;						\
> +}
> +
> +/* For read-write shared bit-per-interrupt registers */
> +#define GIC_ACCESSOR_RW_INTR_BIT(off, name)				\
> +	GIC_ACCESSOR_RO_INTR_BIT(off, name)				\
> +									\
> +static inline void write_gic_##name(unsigned int intr)			\
> +{									\
> +	void __iomem *addr = addr_gic_##name();				\
> +									\
> +	if (mips_cm_is64) {						\

Given that you now refer to this symbol, wouldn't it be best to include
asm/mips-cm.h here? It is probably included via some other path, but
it'd make this file standalone.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...