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

[Paul Burton <paul.burton@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 8695 bytes --]

Hi Marc,

On Friday, 18 August 2017 04:10:20 PDT Marc Zyngier wrote:
> 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.

The inclusion is actually sort of the other way right now - asm/mips-cps.h 
includes each of asm/mips-cm.h, asm/mips-cpc.h & asm/mips-gic.h. This allows 
us to have code in asm/mips-cps.h which makes use of combinations of those 3 
blocks (CM, CPC & GIC) that form the MIPS Coherent Processing System.

Right now, well once the "MIPS: Initial multi-cluster support" series gets 
merged, the code in asm/mips-cps.h only really requires the CM & CPC headers 
but it seemed good to be consistent & include the GIC one in the same way.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]