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

From: Paul Burton
Date: Fri Aug 18 2017 - 12:43:36 EST

Next message: Andi Kleen: "Re: [PATCH 1/2] sched/wait: Break up long wake list walk"
Previous message: Doug Ledford: "Re: [PATCH] infiniband: avoid overflow warning"
In reply to: Marc Zyngier: "Re: [PATCH 02/38] MIPS: GIC: Introduce asm/mips-gic.h with accessor functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> > Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
> > Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > ---
> >
> > 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@xxxxxxxxxx>
> > + *
> > + * 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: signature.asc
Description: This is a digitally signed message part.

Next message: Andi Kleen: "Re: [PATCH 1/2] sched/wait: Break up long wake list walk"
Previous message: Doug Ledford: "Re: [PATCH] infiniband: avoid overflow warning"
In reply to: Marc Zyngier: "Re: [PATCH 02/38] MIPS: GIC: Introduce asm/mips-gic.h with accessor functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]