Re: [PATCH 2/3] irqchip/gic: support RealView variant setup

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

Hi Linus,

On 23/10/15 23:15, Linus Walleij wrote:
> The ARM RealView PB11MPCore reference design has some special
> bits in a system controller register to set up the GIC in one
> of three modes: legacy, new with DCC, new without DCC. The
> register is also used to enable FIQ.
> 
> Since the platform will not boot unless this register is set
> up to "new with DCC" mode, we need a special quirk to be
> compiled-in for the RealView platforms.
> 
> If we find the right compatible string on the GIC TestChip,
> we enable this quirk by looking up the system controller and
> enabling the special bits.
> 
> We depend on the CONFIG_REALVIEW_DT Kconfig symbol as the old
> boardfile code has the same fix hardcoded, and this is only
> needed for the attempts to modernize the RealView code using
> device tree.
> 
> After fixing this, the PB11MPCore boots with device tree
> only.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Put the IRQCHIP_DECLARE() in the add-on irq-gic-realview.c file
>   and have it call down to gic_of_init() after its special
>   initialization
> - Created irq-gic.h to export functions inside irq-gic.c. Part of
>   me wanted to use irq-gic-common.h so as not to proliferate the
>   header files, but I felt it was encapsulating the functions in
>   irq-gic-common.c so it seemed dirty, better to give irq-gic.c
>   its own header file.
> - Broke out this irqchip stuff from the rest of the series so as
>   not to stress the irqchip maintainers. It has no dependencies
>   on the other patches anyways, and can be merged stand-alone.
> ---
>  drivers/irqchip/Makefile           |  1 +
>  drivers/irqchip/irq-gic-realview.c | 43 ++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-gic.c          |  3 ++-
>  drivers/irqchip/irq-gic.h          |  7 +++++++
>  4 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/irq-gic-realview.c
>  create mode 100644 drivers/irqchip/irq-gic.h
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index bb3048f00e64..7a7d4182777d 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
> +obj-$(CONFIG_REALVIEW_DT)		+= irq-gic-realview.o
>  obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
> diff --git a/drivers/irqchip/irq-gic-realview.c b/drivers/irqchip/irq-gic-realview.c
> new file mode 100644
> index 000000000000..bb5583c07667
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic-realview.c
> @@ -0,0 +1,43 @@
> +/*
> + * Special GIC quirks for the ARM RealView
> + * Copyright (C) 2015 Linus Walleij
> + */
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/bitops.h>
> +#include <linux/irqchip.h>
> +
> +#include "irq-gic.h"
> +
> +#define REALVIEW_SYS_LOCK_OFFSET	0x20
> +#define REALVIEW_PB11MP_SYS_PLD_CTRL1	0x74
> +#define VERSATILE_LOCK_VAL		0xA05F
> +#define PLD_INTMODE_MASK		BIT(22)|BIT(23)|BIT(24)
> +#define PLD_INTMODE_LEGACY		0x0
> +#define PLD_INTMODE_NEW_DCC		BIT(22)
> +#define PLD_INTMODE_NEW_NO_DCC		BIT(23)
> +#define PLD_INTMODE_FIQ_ENABLE		BIT(24)
> +
> +static int __init
> +realview_gic_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	static struct regmap *map;
> +
> +	/* The PB11MPCore GIC needs to be configured in the syscon */
> +	map = syscon_regmap_lookup_by_compatible("arm,realview-pb11mp-syscon");
> +	if (!IS_ERR(map)) {
> +		/* new irq mode with no DCC */
> +		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET,
> +			     VERSATILE_LOCK_VAL);
> +		regmap_update_bits(map, REALVIEW_PB11MP_SYS_PLD_CTRL1,
> +				   PLD_INTMODE_NEW_NO_DCC,
> +				   PLD_INTMODE_MASK);
> +		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET, 0x0000);
> +		pr_info("TC11MP GIC: set up interrupt controller to NEW mode, no DCC\n");
> +	} else {
> +		pr_err("TC11MP GIC setup: could not find syscon\n");

At this point, you probably want to error out, because the GIC is 
probably not usable.

> +	}
> +	return gic_of_init(node, parent);
> +}
> +IRQCHIP_DECLARE(armtc11mp_gic, "arm,tc11mp-gic", realview_gic_of_init);
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 982c09c2d791..9ec8cf5137d9 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -50,6 +50,7 @@
>  #include <asm/virt.h>
>  
>  #include "irq-gic-common.h"
> +#include "irq-gic.h"
>  
>  union gic_base {
>  	void __iomem *common_base;
> @@ -1141,7 +1142,7 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  	return true;
>  }
>  
> -static int __init
> +int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>  	void __iomem *cpu_base;
> diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
> new file mode 100644
> index 000000000000..3c45a540c235
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic.h
> @@ -0,0 +1,7 @@
> +#include <linux/of.h>
> +
> +/*
> + * Subdrivers that need some preparatory work can initialize their
> + * chips and call this to register their GICs.
> + */
> +int gic_of_init(struct device_node *node, struct device_node *parent);
> 

I'm not exactly fond of yet another include file, and I rather put this 
in include/irqchip/arm-gic.h (where this was until I recently removed 
it).

How about the following (untested) patch on top of yours:

diff --git a/drivers/irqchip/irq-gic-realview.c b/drivers/irqchip/irq-gic-realview.c
index bb5583c..aa46eb2 100644
--- a/drivers/irqchip/irq-gic-realview.c
+++ b/drivers/irqchip/irq-gic-realview.c
@@ -7,8 +7,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/bitops.h>
 #include <linux/irqchip.h>
-
-#include "irq-gic.h"
+#include <linux/irqchip/arm-gic.h>
 
 #define REALVIEW_SYS_LOCK_OFFSET	0x20
 #define REALVIEW_PB11MP_SYS_PLD_CTRL1	0x74
@@ -37,6 +36,7 @@ realview_gic_of_init(struct device_node *node, struct device_node *parent)
 		pr_info("TC11MP GIC: set up interrupt controller to NEW mode, no DCC\n");
 	} else {
 		pr_err("TC11MP GIC setup: could not find syscon\n");
+		return -ENXIO;
 	}
 	return gic_of_init(node, parent);
 }
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index aea463e..428f9c1 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -49,7 +49,6 @@
 #include <asm/virt.h>
 
 #include "irq-gic-common.h"
-#include "irq-gic.h"
 
 #ifdef CONFIG_ARM64
 #include <asm/cpufeature.h>
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
deleted file mode 100644
index 3c45a54..0000000
--- a/drivers/irqchip/irq-gic.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#include <linux/of.h>
-
-/*
- * Subdrivers that need some preparatory work can initialize their
- * chips and call this to register their GICs.
- */
-int gic_of_init(struct device_node *node, struct device_node *parent);
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index bae69e5..d0a29db 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -103,6 +103,16 @@ struct device_node;
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 int gic_cpu_if_down(unsigned int gic_nr);
 
+/*
+ * Subdrivers that need some preparatory work can initialize their
+ * chips and call this to register their GICs.
+ */
+int gic_of_init(struct device_node *node, struct device_node *parent);
+
+/*
+ * Legacy platforms not converted to DT yet must use this to init
+ * their GIC
+ */
 void gic_init(unsigned int nr, int start,
 	      void __iomem *dist , void __iomem *cpu);
 
Thanks,

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