Device Tree support for RealView PB11MPCore

The RealView ARM11MPCore enables parity, eventmon and shared override in the cache controller through its current boardfile, but the code and DT bindings for the ARM L220 is currently lacking the ability to set this up from DT. Add the required bool parameters.

Cc devicetree at vger.kernel.org

Signed-off-by Linus Walleij <[email protected]>

---
I know this patch mixes code and DT changes but it is silly to split such a small patch. Will submit this to Russell's patch tracker if it looks OK to the DT people. (Or if they are quiet.)

---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++++----
 arch/arm/mm/cache-l2x0.c                       | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/arm/l2cc.txt +0 -2

index 06c88a4d28ac..4d262e9b3464 100644
@@ -67,12 +67,14 @@ Optional properties:
   disable if zero.
 - arm,prefetch-offset : Override prefetch offset value. Valid values are
   0-7, 15, 23, and 31.
-- arm,shared-override : The default behavior of the pl310 cache controller with
-  respect to the shareable attribute is to transform "normal memory

Quoted 1 line · Linus Walleij

+- arm,eventmon-enable : enable the event monitor on the L2 cache (PL220 only).

I don't think we should introduce a DT property for this: if we support the event monitor, then the event monitor support code should be controlling this bit.

The RealView ARM11MPCore enables parity, eventmon and shared override in the cache controller through its current boardfile, but the code and DT bindings for the ARM L220 is currently lacking the ability to set this up from DT. Add the required bool parameters.

Cc devicetree at vger.kernel.org

Signed-off-by Linus Walleij <[email protected]>

---
I know this patch mixes code and DT changes but it is silly to split such a small patch. Will submit this to Russell's patch tracker if it looks OK to the DT people. (Or if they are quiet.)

---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++++----
 arch/arm/mm/cache-l2x0.c                       | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/arm/l2cc.txt +0 -0

index 06c88a4d28ac..4d262e9b3464 100644

Documentation/devicetree/bindings/arm/l2cc.txt +0 -0

 - prefetch-data : Data prefetch. Value: <0> (forcibly disable), <1>
   (forcibly enable), property absent (retain settings set by firmware)
 - prefetch-instr : Instruction prefetch. Value: <0> (forcibly disable),

arch/arm/mm/cache-l2x0.c +15 -1

index 493692d838c6..d4e9fa2594f3 100644
@@ -1060,6 +1060,21 @@ static void __init l2x0_of_parse(const struct device_node *np,
                val |= (dirty - 1) << L2X0_AUX_CTRL_DIRTY_LATENCY_SHIFT;
        }
 
+       if (of_property_read_bool(np, "arm,parity-enable")) {
+               mask &= ~L2C_AUX_CTRL_PARITY_ENABLE;
+               val |= L2C_AUX_CTRL_PARITY_ENABLE;
+       }
+
+       if (of_property_read_bool(np, "arm,eventmon-enable")) {
+               mask &= ~L2C_AUX_CTRL_EVTMON_ENABLE;
+               val |= L2C_AUX_CTRL_EVTMON_ENABLE;
+       }
+
+       if (of_property_read_bool(np, "arm,shared-override")) {
+               mask &= ~L2C_AUX_CTRL_SHARED_OVERRIDE;
+               val |= L2C_AUX_CTRL_SHARED_OVERRIDE;
+       }
+
        ret = l2x0_cache_size_of_parse(np, aux_val, aux_mask, &assoc, SZ_256K);
        if (ret)
                return;
--

2.4.3

The GIC bindings for the ARM11MPCore need to differentiate between the GIC on the Test Chip and the one on the evaluation baseboard. Split the binding in two and define new compatible-strings.

Cc devicetree at vger.kernel.org

Signed-off-by Linus Walleij <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt | 3 ++-
 drivers/irqchip/irq-gic.c                     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/arm/gic.txt +2 -1

index 2da059a4790c..a5445622c216 100644
@@ -15,7 +15,8 @@ Main node required properties:
        "arm,cortex-a15-gic"
        "arm,cortex-a9-gic"
        "arm,cortex-a7-gic"
-       "arm,arm11mp-gic"
+       "arm,tc11mp-gic"
+       "arm,pb11mp-gic"
        "brcm,brahma-b15-gic"
        "arm,arm1176jzf-devchip-gic"
        "qcom,msm-8660-qgic"

drivers/irqchip/irq-gic.c +2 -2

index 982c09c2d791..5376d1cb0a4f 100644
@@ -1184,7 +1184,8 @@ gic_of_init(struct device_node *node, struct device_node *parent)
        return 0;
 }
 IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
-IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init);
+IRQCHIP_DECLARE(armtc11mp_gic, "arm,tc11mp-gic", gic_of_init);
+IRQCHIP_DECLARE(armpb11mp_gic, "arm,pb11mp-gic", gic_of_init);
 IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
--

2.4.3

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 <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
Looking for an ACK for this (unless the irqchip maintainers barf at it) so I can take it through the ARM SoC tree.

---
 drivers/irqchip/Makefile           |  1 +
 drivers/irqchip/irq-gic-realview.c | 39 ++++++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic-realview.h |  5 +++++
 drivers/irqchip/irq-gic.c          |  4 ++++
 4 files changed, 49 insertions(+)
 create mode 100644 drivers/irqchip/irq-gic-realview.c
 create mode 100644 drivers/irqchip/irq-gic-realview.h

drivers/irqchip/Makefile +1 -0

index bb3048f00e64..7a7d4182777d 100644
@@ -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

drivers/irqchip/irq-gic-realview.c new file +39 -0

new file mode 100644
index 000000000000..2e8a4b129f5f
@@ -0,0 +1,39 @@
+/*
+ * 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>
+
+#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)
+
+void __init realview_gic_init(struct device_node *np)
+{
+	static struct regmap *map;
+
+	/* The PB11MPCore GIC needs to be configured in the syscon */
+	if (of_device_is_compatible(np, "arm,tc11mp-gic")) {
+		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");
+		}
+	}
+}

drivers/irqchip/irq-gic-realview.h new file +5 -0

new file mode 100644
index 000000000000..06ebdfc523a8
@@ -0,0 +1,5 @@
+#ifdef CONFIG_REALVIEW_DT
+void realview_gic_init(struct device_node *np);
+#else
+static void realview_gic_init(struct device_node *np) {}
+#endif

drivers/irqchip/irq-gic.c +1 -0

index 5376d1cb0a4f..bd021e1e4847 100644
@@ -50,6 +50,7 @@
 #include <asm/virt.h>
 
 #include "irq-gic-common.h"
+#include "irq-gic-realview.h"
 
 union gic_base {
 	void __iomem *common_base;

Instead of having the irqchip being a static struct, make it part of the per-instance data so we can assign it a dynamic name. This has the usable side effect of displaying the GIC with an instance number as GIC0, GIC1 ... GICn in /proc/interrupts, which is helpful when debugging cascaded GICs, such as on the ARM PB11MPCore.

Cc Thomas Gleixner <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
Fellas please say what you think about this:
Yes / No / Linus is an idiot
This can be applied directly to the irqchip tree if you like it, AFAIK it has no dependencies.

---
 drivers/irqchip/irq-gic.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

drivers/irqchip/irq-gic.c +9 -17

index bd021e1e4847..478279cf9517 100644
@@ -58,6 +58,7 @@ union gic_base {
 };
 
 struct gic_chip_data {
+	struct irq_chip chip;
 	union gic_base dist_base;
 	union gic_base cpu_base;
 #ifdef CONFIG_CPU_PM

@@ -369,22 +370,6 @@ static void gic_handle_cascade_irq(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static struct irq_chip gic_chip = {
-	.name			= "GIC",
-	.irq_mask		= gic_mask_irq,
-	.irq_unmask		= gic_unmask_irq,
-	.irq_eoi		= gic_eoi_irq,
-	.irq_set_type		= gic_set_type,
-#ifdef CONFIG_SMP
-	.irq_set_affinity	= gic_set_affinity,
-#endif
-	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
-	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
-	.flags			= IRQCHIP_SET_TYPE_MASKED |
-				  IRQCHIP_SKIP_SET_WAKE |
-				  IRQCHIP_MASK_ON_SUSPEND,
-};
-
 static struct irq_chip gic_eoimode1_chip = {
 	.name			= "GICv2",
 	.irq_mask		= gic_eoimode1_mask_irq,

@@ -880,7 +865,8 @@ void __init gic_init_physaddr(struct device_node *node)
 static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
-	struct irq_chip *chip = &gic_chip;
+	struct gic_chip_data *gic = d->host_data;
+	struct irq_chip *chip = &gic->chip;
 
 	if (static_key_true(&supports_deactivate)) {
 		if (d->host_data == (void *)&gic_data[0])

@@ -989,6 +975,22 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	gic = &gic_data[gic_nr];
+
+	/* Initialize irq_chip */
+	gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+	gic->chip.irq_mask = gic_mask_irq;
+	gic->chip.irq_unmask = gic_unmask_irq;
+	gic->chip.irq_eoi = gic_eoi_irq;

drivers/clk/versatile/clk-icst.c +1 -0

+		return ERR_PTR(ret);

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 11 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +7 -0

@@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
      init.flags = CLK_IS_ROOT;
      init.parent_names = (parent_name ? &parent_name : NULL);
      init.num_parents = (parent_name ? 1 : 0);
+     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+     if (IS_ERR(icst->map)) {
+             int ret;
+
+             pr_err("could not initialize ICST regmap\n");
+             kfree(icst);
+             ret = PTR_ERR(icst->map);

drivers/clk/versatile/clk-icst.c:183 icst_clk_register() error: dereferencing freed memory 'icst' drivers/clk/versatile/clk-icst.c:184 icst_clk_register() warn: possible memory leak of 'pclone'

Quoted 17 lines · Stephen Boyd

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 11 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +7 -0

@@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
      init.flags = CLK_IS_ROOT;
      init.parent_names = (parent_name ? &parent_name : NULL);
      init.num_parents = (parent_name ? 1 : 0);
+     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+     if (IS_ERR(icst->map)) {
+             int ret;
+
+             pr_err("could not initialize ICST regmap\n");
+             kfree(icst);
+             ret = PTR_ERR(icst->map);

drivers/clk/versatile/clk-icst.c:183 icst_clk_register() error: dereferencing freed memory 'icst' drivers/clk/versatile/clk-icst.c:184 icst_clk_register() warn: possible memory leak of 'pclone'

The pclone warning is correct, nice catch. (Fixing it.)

Quoted 17 lines · Stephen Boyd

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 11 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +7 -0

@@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
      init.flags = CLK_IS_ROOT;
      init.parent_names = (parent_name ? &parent_name : NULL);
      init.num_parents = (parent_name ? 1 : 0);
+     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+     if (IS_ERR(icst->map)) {
+             int ret;
+
+             pr_err("could not initialize ICST regmap\n");
+             kfree(icst);
+             ret = PTR_ERR(icst->map);

drivers/clk/versatile/clk-icst.c:183 icst_clk_register() error: dereferencing freed memory 'icst' drivers/clk/versatile/clk-icst.c:184 icst_clk_register() warn: possible memory leak of 'pclone'

The pclone warning is correct, nice catch. (Fixing it.)

But for the second warning, whatever static checker you're using for this is unable to handle error pointers:

    clk = clk_register(dev, &icst->hw);
    if (IS_ERR(clk))
        kfree(icst);

    return clk;

It is quite obvious that returning clk (which may be an error code) is OK here.

If you want, I may need to add some specific annotation to shut up the static checker, any hints?

Quoted 2 lines

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 2 lines

drivers/clk/versatile/clk-icst.c +2 -0

+             kfree(icst);
+             ret = PTR_ERR(icst->map);

Quoted 30 lines · Linus Walleij

Quoted 17 lines · Stephen Boyd

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 11 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +7 -0

@@ -151,10 +174,19 @@ struct clk *icst_clk_register(struct device *dev,
      init.flags = CLK_IS_ROOT;
      init.parent_names = (parent_name ? &parent_name : NULL);
      init.num_parents = (parent_name ? 1 : 0);
+     icst->map = regmap_init_mmio(NULL, base, &icst_regmap_conf);
+     if (IS_ERR(icst->map)) {
+             int ret;
+
+             pr_err("could not initialize ICST regmap\n");
+             kfree(icst);
+             ret = PTR_ERR(icst->map);

drivers/clk/versatile/clk-icst.c:183 icst_clk_register() error: dereferencing freed memory 'icst' drivers/clk/versatile/clk-icst.c:184 icst_clk_register() warn: possible memory leak of 'pclone'

The pclone warning is correct, nice catch. (Fixing it.)

But for the second warning, whatever static checker you're using for this is unable to handle error pointers:

    clk = clk_register(dev, &icst->hw);
    if (IS_ERR(clk))
        kfree(icst);

    return clk;

Quoted 6 lines

It is quite obvious that returning clk (which may be an error code) is OK here.

If you want, I may need to add some specific annotation to shut up the static checker, any hints?

Huh? I'm totally lost. I use sparse and smatch.

Quoted 2 lines

Quoted 2 lines

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 183

Quoted 2 lines

drivers/clk/versatile/clk-icst.c +2 -0

+             kfree(icst);
+             ret = PTR_ERR(icst->map);

We just freed icst, and then we dereferenced it in the next line. What does that have to do with error pointers?

drivers/clk/versatile/clk-icst.c +6 -1

-	}
+	icst->map = map;
 	icst->hw.init = &init;
 	icst->params = pclone;
 	icst->vcoreg_off = desc->vco_offset;

@@ -194,4 +182,28 @@ struct clk *icst_clk_register(struct device *dev,
 
 	return clk;
 }
+
+struct clk *icst_clk_register(struct device *dev,
+			const struct clk_icst_desc *desc,
+			const char *name,
+			const char *parent_name,

drivers/clk/versatile/clk-icst.c +2 -0

+	}
+	return icst_clk_setup(dev, desc, name, parent_name, map);

drivers/clk/versatile/clk-icst.c +0 -1

 			const struct clk_icst_desc *desc,
 			const char *name,
 			const char *parent_name,
-			void __iomem *base)

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt new file +0 -0

new file mode 100644
index 000000000000..19eb3aa765c7

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt new file +6 -0

+- compatible: must be one of "arm,syscon-icst525" or "arm,syscon-icst307"
+- #clock-cells: must be <0>
+- clocks: parent clock, since the ICST needs a parent clock to derive its
+  frequency from, this attribute is compulsory.
+
+Example:

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt new file +4 -0

+		clocks = <&xtal24mhz>;
+	};
+	(...)
+};

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt line 20

Quoted 5 lines · Linus Walleij

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt new file +5 -0

+Required properties:
+- lock-offset: the offset address into the system controller where the
+  unlocking register is located
+- vco-offset: the offset address into the system controller where the
+  ICST control register is located (even 32 bit address)

Is there any reason why we don't use a reg property for this?

Quoted 9 lines · Stephen Boyd

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt line 20

Quoted 5 lines · Linus Walleij

Documentation/devicetree/bindings/clock/arm-syscon-icst.txt new file +5 -0

+Required properties:
+- lock-offset: the offset address into the system controller where the
+  unlocking register is located
+- vco-offset: the offset address into the system controller where the
+  ICST control register is located (even 32 bit address)

Is there any reason why we don't use a reg property for this?

Usually reg = <> is used with two (or more) tokens:

reg = <phys_addr size>;

The exception being things like I2C addresses which are just one token.

Since in this case, there is a "mother" reg property in the syscon-compatible node, which we are indexing into, it is confusing to use the same name for subnodes.

Also there is a bunch of precedents doing it like this for sybdevices to system controllers, just git grep offset Documentation/devicetree/bindings will give you a bunch of them.

drivers/clk/versatile/clk-icst.c struct clk *icst_clk_register(struct device *dev, · line 273

Quoted 5 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +5 -0

+
+     if (of_device_is_compatible(np, "arm,syscon-icst525"))
+             icst_desc.params = &icst525_params;
+     else if (of_device_is_compatible(np, "arm,syscon-icst307"))
+             icst_desc.params = &icst307_params;

I guess if we add anymore here we should use an of_device_id array instead.

Quoted 7 lines · Stephen Boyd

Quoted 5 lines · Linus Walleij

drivers/clk/versatile/clk-icst.c +5 -0

+
+     if (of_device_is_compatible(np, "arm,syscon-icst525"))
+             icst_desc.params = &icst525_params;
+     else if (of_device_is_compatible(np, "arm,syscon-icst307"))
+             icst_desc.params = &icst307_params;

Quoted 2 lines

I guess if we add anymore here we should use an of_device_id array instead.

As it happens those two are gonna be it.

ARM never created any more integrated ICST devices, and they stopped using them since. Those two are the required ones.

[Me]

Quoted 3 lines

ARM never created any more integrated ICST devices, and they stopped using them since. Those two are the required ones.

As ARM didn't create any ICST devices at all, that's hardly surprising. These devices are created by Integrated Circuit Systems, Inc. The 525 is a parallel-loaded clock generator, the 307 is a serial-loaded clock generator.

However, they have no "standard" software interface - indeed, the 525 is marketed as a device that needs no processor or software to be used, so it doesn't have a "software" interface as such.

ARM Ltd's hardware on these boards provides interfaces to these, however the underlying ICST support I wrote was factored to separate out the interface from the chip support - I haven't been tracking what's been going on with these, but I hope that separation has been kept as it's entirely logical, and describing these things in DT as an ARM Ltd device, combining the ICST device itself with its interface would be wrong.

This adds support for the ARM syscon ICST clocks to initialized directly from the device tree syscon node on ARM Integrator, Versatile and RealView reference designs.

Cc Michael Turquette <[email protected]>

Cc Stephen Boyd <[email protected]>

Cc linux-clk at vger.kernel.org

Signed-off-by Linus Walleij <[email protected]>

---