[PATCH 0/5] irqchip: New driver for ST's SysCfg controlled IRQs

[PATCH 0/5] irqchip: New driver for ST's SysCfg controlled IRQs

[Lee Jones]

This driver enabled IRQs which are controlled using System Configuration
registers.  Without it Performance Monitoring, Core Sight Tracing and some
L2 Caches will fail to function.

Lee Jones (5):
  irqchip: Supply new driver for STi based devices
  irqchip: irq-st: Add documentation for STi based syscfg IRQs
  ARM: STi: STiH416: Enable Cortex-A9 PMU support
  ARM: STi: STiH416: Enable PMU IRQs
  ARM: STI: Ensure requested STi's SysCfg Controlled IRQs are enabled at boot

 .../interrupt-controller/st,sti-irq-syscfg.txt     |  35 ++++
 arch/arm/boot/dts/stih416.dtsi                     |  16 ++
 arch/arm/mach-sti/Kconfig                          |   1 +
 drivers/irqchip/Kconfig                            |   7 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-st.c                           | 204 +++++++++++++++++++++
 6 files changed, 264 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
 create mode 100644 drivers/irqchip/irq-st.c

-- 
1.9.1

[PATCH 1/6] dt: bindings: Supply shared ST IRQ defines

[Lee Jones]

These defines are used to allow values used for configuration to be
easily human readable and will lessen the chance of logical mistakes.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/dt-bindings/interrupt-controller/irq-st.h | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 include/dt-bindings/interrupt-controller/irq-st.h

diff --git a/include/dt-bindings/interrupt-controller/irq-st.h b/include/dt-bindings/interrupt-controller/irq-st.h
new file mode 100644
index 0000000..4c59ace
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/irq-st.h
@@ -0,0 +1,30 @@
+/*
+ *  include/linux/irqchip/irq-st.h
+ *
+ *  Copyright (C) 2014 STMicroelectronics – All Rights Reserved
+ *
+ *  Author: Lee Jones <lee.jones@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ST_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ST_H
+
+#define ST_IRQ_SYSCFG_EXT_0		0
+#define ST_IRQ_SYSCFG_EXT_1		1
+#define ST_IRQ_SYSCFG_EXT_2		2
+#define ST_IRQ_SYSCFG_CTI_0		3
+#define ST_IRQ_SYSCFG_CTI_1		4
+#define ST_IRQ_SYSCFG_PMU_0		5
+#define ST_IRQ_SYSCFG_PMU_1		6
+#define ST_IRQ_SYSCFG_pl310_L2		7
+#define ST_IRQ_SYSCFG_DISABLED		0xFFFFFFFF
+
+#define ST_IRQ_SYSCFG_EXT_1_INV		0x1
+#define ST_IRQ_SYSCFG_EXT_2_INV		0x2
+#define ST_IRQ_SYSCFG_EXT_3_INV		0x4
+
+#endif
-- 
1.9.1

[PATCH 2/6] irqchip: Supply new driver for STi based devices

[Lee Jones]

This driver is used to enable System Configuration Register controlled
External, CTI (Core Sight), PMU (Performance Management), and PL310 L2
Cache IRQs prior to use.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/irqchip/Kconfig  |   7 ++
 drivers/irqchip/Makefile |   1 +
 drivers/irqchip/irq-st.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)
 create mode 100644 drivers/irqchip/irq-st.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bbb746e..7252de9 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -91,3 +91,10 @@ config IRQ_CROSSBAR
 	  The primary irqchip invokes the crossbar's callback which inturn allocates
 	  a free irq and configures the IP. Thus the peripheral interrupts are
 	  routed to one of the free irqchip interrupt lines.
+
+config ST_IRQCHIP
+	bool
+	select REGMAP
+	select MFD_SYSCON
+	help
+	  Enables SysCfg Controlled IRQs on STi based platforms.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 62a13e5..f859c14 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
 obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
 obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
 obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
+obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
diff --git a/drivers/irqchip/irq-st.c b/drivers/irqchip/irq-st.c
new file mode 100644
index 0000000..f31126f
--- /dev/null
+++ b/drivers/irqchip/irq-st.c
@@ -0,0 +1,206 @@
+/*
+ *  drivers/irqchip/irq-st.c
+ *
+ *  Copyright (C) 2014 STMicroelectronics – All Rights Reserved
+ *
+ *  Author: Lee Jones <lee.jones@linaro.org>
+ *
+ *  This is a re-write of Christophe Kerello's PMU driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <dt-bindings/interrupt-controller/irq-st.h>
+#include <linux/err.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define STIH415_SYSCFG_642		0x0a8
+#define STIH416_SYSCFG_7543		0x87c
+#define STIH407_SYSCFG_5102		0x198
+#define STID127_SYSCFG_734		0x088
+
+#define ST_A9_IRQ_MASK			0x001FFFFF
+#define ST_A9_IRQ_MAX_CHANS		2
+
+#define ST_A9_IRQ_EN_CTI_0		BIT(0)
+#define ST_A9_IRQ_EN_CTI_1		BIT(1)
+#define ST_A9_IRQ_EN_PMU_0		BIT(2)
+#define ST_A9_IRQ_EN_PMU_1		BIT(3)
+#define ST_A9_IRQ_EN_pl310_L2		BIT(4)
+#define ST_A9_IRQ_EN_EXT_0		BIT(5)
+#define ST_A9_IRQ_EN_EXT_1		BIT(6)
+#define ST_A9_IRQ_EN_EXT_2		BIT(7)
+
+#define ST_A9_FIQ_N_SEL(dev, chan)	(dev << (8  + (chan * 3)))
+#define ST_A9_IRQ_N_SEL(dev, chan)	(dev << (14 + (chan * 3)))
+#define ST_A9_EXTIRQ_INV_SEL(dev)	(dev << 20)
+
+struct st_irq_syscfg {
+	struct regmap *regmap;
+	unsigned int syscfg;
+	unsigned int result;
+	bool ext_inverted;
+};
+
+static const struct of_device_id st_irq_syscfg_match[] = {
+	{
+		.compatible = "st,stih415-irq-syscfg",
+		.data = (void *)STIH415_SYSCFG_642,
+	},
+	{
+		.compatible = "st,stih416-irq-syscfg",
+		.data = (void *)STIH416_SYSCFG_7543,
+	},
+	{
+		.compatible = "st,stih407-irq-syscfg",
+		.data = (void *)STIH407_SYSCFG_5102,
+	},
+	{
+		.compatible = "st,stid127-irq-syscfg",
+		.data = (void *)STID127_SYSCFG_734,
+	},
+	{}
+};
+
+static int st_irq_xlate(struct platform_device *pdev,
+			int device, int channel, bool irq)
+{
+	struct st_irq_syscfg *ddata = dev_get_drvdata(&pdev->dev);
+
+	/* Set the device enable bit. */
+	switch (device) {
+	case ST_IRQ_SYSCFG_EXT_0 :
+		ddata->result |= ST_A9_IRQ_EN_EXT_0;
+		break;
+	case ST_IRQ_SYSCFG_EXT_1 :
+		ddata->result |= ST_A9_IRQ_EN_EXT_1;
+		break;
+	case ST_IRQ_SYSCFG_EXT_2 :
+		ddata->result |= ST_A9_IRQ_EN_EXT_2;
+		break;
+	case ST_IRQ_SYSCFG_CTI_0 :
+		ddata->result |= ST_A9_IRQ_EN_CTI_0;
+		break;
+	case ST_IRQ_SYSCFG_CTI_1 :
+		ddata->result |= ST_A9_IRQ_EN_CTI_1;
+		break;
+	case ST_IRQ_SYSCFG_PMU_0 :
+		ddata->result |= ST_A9_IRQ_EN_PMU_0;
+		break;
+	case ST_IRQ_SYSCFG_PMU_1 :
+		ddata->result |= ST_A9_IRQ_EN_PMU_1;
+		break;
+	case ST_IRQ_SYSCFG_pl310_L2 :
+		ddata->result |= ST_A9_IRQ_EN_pl310_L2;
+		break;
+	case ST_IRQ_SYSCFG_DISABLED :
+		return 0;
+	default :
+		dev_err(&pdev->dev, "Unrecognised device %d\n", device);
+		return -EINVAL;
+	}
+
+	/* Select IRQ/FIQ channel for device. */
+	ddata->result |= irq ?
+		ST_A9_IRQ_N_SEL(device, channel) :
+		ST_A9_FIQ_N_SEL(device, channel);
+
+	return 0;
+}
+
+static int st_irq_syscfg_enable(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct st_irq_syscfg *ddata = dev_get_drvdata(&pdev->dev);
+	int channels, ret, i;
+	u32 device, invert;
+
+	channels = of_property_count_u32_elems(np, "st,irq-device");
+	if (channels != ST_A9_IRQ_MAX_CHANS) {
+		dev_err(&pdev->dev, "st,enable-irq-device must have 2 elems\n");
+		return -EINVAL;
+	}
+
+	channels = of_property_count_u32_elems(np, "st,fiq-device");
+	if (channels != ST_A9_IRQ_MAX_CHANS) {
+		dev_err(&pdev->dev, "st,enable-fiq-device must have 2 elems\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < channels; i++) {
+		of_property_read_u32_index(np,"st,irq-device", i, &device);
+
+		ret = st_irq_xlate(pdev, device, i, true);
+		if (ret)
+			return ret;
+
+		of_property_read_u32_index(np,"st,fiq-device", i, &device);
+
+		ret = st_irq_xlate(pdev, device, i, false);
+		if (ret)
+			return ret;
+	}
+
+	/* External IRQs may be inverted. */
+	of_property_read_u32(np, "st,invert-ext", &invert);
+	ddata->result |= ST_A9_EXTIRQ_INV_SEL(invert);
+
+	return regmap_update_bits(ddata->regmap, ddata->syscfg,
+				  ST_A9_IRQ_MASK, ddata->result);
+}
+
+int st_irq_syscfg_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct st_irq_syscfg *ddata;
+
+	match = of_match_device(st_irq_syscfg_match, &pdev->dev);
+	if (!np)
+		return -ENODEV;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+	if (IS_ERR(ddata->regmap)) {
+		dev_err(&pdev->dev, "syscfg phandle missing\n");
+		return PTR_ERR(ddata->regmap);
+	}
+
+	dev_set_drvdata(&pdev->dev, ddata);
+
+	return st_irq_syscfg_enable(pdev);
+}
+
+static int st_irq_syscfg_resume(struct device *dev)
+{
+	struct st_irq_syscfg *ddata = dev_get_drvdata(dev);
+
+	return regmap_update_bits(ddata->regmap, ddata->syscfg,
+				  ST_A9_IRQ_MASK, ddata->result);
+}
+
+static SIMPLE_DEV_PM_OPS(st_irq_syscfg_pm_ops, NULL, st_irq_syscfg_resume);
+
+static struct platform_driver st_irq_syscfg_driver = {
+	.driver = {
+		.name = "st_irq_syscfg",
+		.pm = &st_irq_syscfg_pm_ops,
+		.of_match_table = st_irq_syscfg_match,
+	},
+	.probe = st_irq_syscfg_probe,
+};
+
+static int __init st_irq_syscfg_init(void)
+{
+	return platform_driver_register(&st_irq_syscfg_driver);
+}
+core_initcall(st_irq_syscfg_init);
-- 
1.9.1

[PATCH 3/6] irqchip: irq-st: Add documentation for STi based syscfg IRQs

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../interrupt-controller/st,sti-irq-syscfg.txt     | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
new file mode 100644
index 0000000..ced6014
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
@@ -0,0 +1,35 @@
+STMicroelectronics STi System Configuration Controlled IRQs
+-----------------------------------------------------------
+
+On STi based systems; External, CTI (Core Sight), PMU (Performance Management),
+and PL310 L2 Cache IRQs are controlled using System Configuration registers.
+This driver is used to unmask them prior to use.
+
+Required properties:
+- compatible	: Should be set to one of:
+			"st,stih415-irq-syscfg"
+			"st,stih416-irq-syscfg"
+			"st,stih407-irq-syscfg"
+			"st,stid127-irq-syscfg"
+- st,syscfg	: Phandle to Cortex-A9 IRQ system config registers
+- st,irq-device	: Array of IRQs to enable - should be 2 in length
+- st,fiq-device	: Array of FIQs to enable - should be 2 in length
+
+Optional properties:
+- st,invert-ext	: External IRQs can be inverted at will.  This property inverts
+		  these IRQs using bitwise logic.  A number of defines have been
+		  provided for convenience:
+			ST_IRQ_SYSCFG_EXT_1_INV
+			ST_IRQ_SYSCFG_EXT_2_INV
+			ST_IRQ_SYSCFG_EXT_3_INV
+Example:
+
+irq-syscfg {
+	compatible    = "st,stih416-irq-syscfg";
+	st,syscfg     = <&syscfg_cpu>;
+	st,irq-device = <ST_IRQ_SYSCFG_PMU_0>,
+			<ST_IRQ_SYSCFG_PMU_1>;
+	st,fiq-device = <ST_IRQ_SYSCFG_DISABLED>,
+			<ST_IRQ_SYSCFG_DISABLED>;
+	st,invert-ext = <(ST_IRQ_SYSCFG_EXT_1_INV | ST_IRQ_SYSCFG_EXT_3_INV)>;
+};
-- 
1.9.1

[PATCH 4/6] ARM: STi: STiH416: Enable Cortex-A9 PMU support

[Lee Jones]

This is ARM's generic Performance Monitoring Unit.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 84758d7..badefd6 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -21,6 +21,12 @@
 		cache-level = <2>;
 	};
 
+	arm-pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupt-parent = <&intc>;
+		interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.9.1

[PATCH 5/6] ARM: STi: STiH416: Enable PMU IRQs

[Lee Jones]

This driver is used to enable System Configuration Register controlled
External, CTI (Core Sight), PMU (Performance Management), and PL310 L2
Cache IRQs prior to use.

Here we are enabling PMU IRQs on both channels.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index badefd6..7276125 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -11,6 +11,7 @@
 #include "stih416-pinctrl.dtsi"
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset-controller/stih416-resets.h>
+#include <dt-bindings/interrupt-controller/irq-st.h>
 / {
 	L2: cache-controller {
 		compatible = "arm,pl310-cache";
@@ -27,6 +28,15 @@
 		interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	irq-syscfg {
+		compatible    = "st,stih416-irq-syscfg";
+		st,syscfg     = <&syscfg_cpu>;
+		st,irq-device = <ST_IRQ_SYSCFG_PMU_0>,
+				<ST_IRQ_SYSCFG_PMU_1>;
+		st,fiq-device = <ST_IRQ_SYSCFG_DISABLED>,
+				<ST_IRQ_SYSCFG_DISABLED>;
+	};
+
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.9.1

[PATCH 6/6] ARM: STI: Ensure requested STi's SysCfg Controlled IRQs are enabled at boot

[Lee Jones]

This driver is used to enable System Configuration Register controlled
External, CTI (Core Sight), PMU (Performance Management), and PL310 L2
Cache IRQs prior to use.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/mach-sti/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
index 878e9ec..eae0971 100644
--- a/arch/arm/mach-sti/Kconfig
+++ b/arch/arm/mach-sti/Kconfig
@@ -1,6 +1,7 @@
 menuconfig ARCH_STI
 	bool "STMicroelectronics Consumer Electronics SOCs" if ARCH_MULTI_V7
 	select ARM_GIC
+	select ST_IRQCHIP
 	select ARM_GLOBAL_TIMER
 	select PINCTRL
 	select PINCTRL_ST
-- 
1.9.1

Re: [PATCH 3/6] irqchip: irq-st: Add documentation for STi based syscfg IRQs

[Jason Cooper]

Hey Lee,

Rob, Grant, question for you below:

On Tue, Jul 29, 2014 at 03:05:41PM +0100, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../interrupt-controller/st,sti-irq-syscfg.txt     | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> new file mode 100644
> index 0000000..ced6014
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> @@ -0,0 +1,35 @@
> +STMicroelectronics STi System Configuration Controlled IRQs
> +-----------------------------------------------------------
> +
> +On STi based systems; External, CTI (Core Sight), PMU (Performance Management),
> +and PL310 L2 Cache IRQs are controlled using System Configuration registers.
> +This driver is used to unmask them prior to use.

We don't usually refer to the driver in binding docs as they are
supposed to be OS-agnostic.

> +
> +Required properties:
> +- compatible	: Should be set to one of:
> +			"st,stih415-irq-syscfg"
> +			"st,stih416-irq-syscfg"
> +			"st,stih407-irq-syscfg"
> +			"st,stid127-irq-syscfg"
> +- st,syscfg	: Phandle to Cortex-A9 IRQ system config registers
> +- st,irq-device	: Array of IRQs to enable - should be 2 in length
> +- st,fiq-device	: Array of FIQs to enable - should be 2 in length
> +
> +Optional properties:
> +- st,invert-ext	: External IRQs can be inverted at will.  This property inverts
> +		  these IRQs using bitwise logic.  A number of defines have been
> +		  provided for convenience:
> +			ST_IRQ_SYSCFG_EXT_1_INV
> +			ST_IRQ_SYSCFG_EXT_2_INV
> +			ST_IRQ_SYSCFG_EXT_3_INV
> +Example:

Rob, Grant, should we be mingling implementation details (macros from
include files) with the binding documentation?  It just _seems_ wrong
too me.  But if we agree that the binding docs, the dts files, and their
includes are always kept together, I also don't see the harm...

> +
> +irq-syscfg {
> +	compatible    = "st,stih416-irq-syscfg";
> +	st,syscfg     = <&syscfg_cpu>;
> +	st,irq-device = <ST_IRQ_SYSCFG_PMU_0>,
> +			<ST_IRQ_SYSCFG_PMU_1>;
> +	st,fiq-device = <ST_IRQ_SYSCFG_DISABLED>,
> +			<ST_IRQ_SYSCFG_DISABLED>;
> +	st,invert-ext = <(ST_IRQ_SYSCFG_EXT_1_INV | ST_IRQ_SYSCFG_EXT_3_INV)>;
> +};

thx,

Jason.

Re: [PATCH 2/6] irqchip: Supply new driver for STi based devices

[Jason Cooper]

On Tue, Jul 29, 2014 at 03:05:40PM +0100, Lee Jones wrote:
> This driver is used to enable System Configuration Register controlled
> External, CTI (Core Sight), PMU (Performance Management), and PL310 L2
> Cache IRQs prior to use.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/irqchip/Kconfig  |   7 ++
>  drivers/irqchip/Makefile |   1 +
>  drivers/irqchip/irq-st.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 214 insertions(+)
>  create mode 100644 drivers/irqchip/irq-st.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bbb746e..7252de9 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -91,3 +91,10 @@ config IRQ_CROSSBAR
>  	  The primary irqchip invokes the crossbar's callback which inturn allocates
>  	  a free irq and configures the IP. Thus the peripheral interrupts are
>  	  routed to one of the free irqchip interrupt lines.
> +
> +config ST_IRQCHIP
> +	bool
> +	select REGMAP
> +	select MFD_SYSCON
> +	help
> +	  Enables SysCfg Controlled IRQs on STi based platforms.

Now that I have my head above water (a bit) wrt irqchip, I really don't
like the hot mess that this file has become...

> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 62a13e5..f859c14 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
>  obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
>  obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
> +obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
> diff --git a/drivers/irqchip/irq-st.c b/drivers/irqchip/irq-st.c
> new file mode 100644
> index 0000000..f31126f
> --- /dev/null
> +++ b/drivers/irqchip/irq-st.c
> @@ -0,0 +1,206 @@
> +/*
> + *  drivers/irqchip/irq-st.c
> + *
> + *  Copyright (C) 2014 STMicroelectronics – All Rights Reserved
> + *
> + *  Author: Lee Jones <lee.jones@linaro.org>
> + *
> + *  This is a re-write of Christophe Kerello's PMU driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq-st.h>
> +#include <linux/err.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define STIH415_SYSCFG_642		0x0a8
> +#define STIH416_SYSCFG_7543		0x87c
> +#define STIH407_SYSCFG_5102		0x198
> +#define STID127_SYSCFG_734		0x088
> +
> +#define ST_A9_IRQ_MASK			0x001FFFFF
> +#define ST_A9_IRQ_MAX_CHANS		2
> +
> +#define ST_A9_IRQ_EN_CTI_0		BIT(0)
> +#define ST_A9_IRQ_EN_CTI_1		BIT(1)
> +#define ST_A9_IRQ_EN_PMU_0		BIT(2)
> +#define ST_A9_IRQ_EN_PMU_1		BIT(3)
> +#define ST_A9_IRQ_EN_pl310_L2		BIT(4)

PL310

> +#define ST_A9_IRQ_EN_EXT_0		BIT(5)
> +#define ST_A9_IRQ_EN_EXT_1		BIT(6)
> +#define ST_A9_IRQ_EN_EXT_2		BIT(7)
> +
> +#define ST_A9_FIQ_N_SEL(dev, chan)	(dev << (8  + (chan * 3)))
> +#define ST_A9_IRQ_N_SEL(dev, chan)	(dev << (14 + (chan * 3)))
> +#define ST_A9_EXTIRQ_INV_SEL(dev)	(dev << 20)
> +
> +struct st_irq_syscfg {
> +	struct regmap *regmap;
> +	unsigned int syscfg;
> +	unsigned int result;

result seems odd here.  It sounds like you're storing a return value,
when in fact, it looks like you are using it to determine whether or not
to invert...  Perhaps a different variable name here?

> +	bool ext_inverted;
> +};
> +
> +static const struct of_device_id st_irq_syscfg_match[] = {
> +	{
> +		.compatible = "st,stih415-irq-syscfg",
> +		.data = (void *)STIH415_SYSCFG_642,
> +	},
> +	{
> +		.compatible = "st,stih416-irq-syscfg",
> +		.data = (void *)STIH416_SYSCFG_7543,
> +	},
> +	{
> +		.compatible = "st,stih407-irq-syscfg",
> +		.data = (void *)STIH407_SYSCFG_5102,
> +	},
> +	{
> +		.compatible = "st,stid127-irq-syscfg",
> +		.data = (void *)STID127_SYSCFG_734,
> +	},
> +	{}
> +};
> +
> +static int st_irq_xlate(struct platform_device *pdev,
> +			int device, int channel, bool irq)
> +{
> +	struct st_irq_syscfg *ddata = dev_get_drvdata(&pdev->dev);
> +
> +	/* Set the device enable bit. */
> +	switch (device) {
> +	case ST_IRQ_SYSCFG_EXT_0 :
> +		ddata->result |= ST_A9_IRQ_EN_EXT_0;
> +		break;
> +	case ST_IRQ_SYSCFG_EXT_1 :
> +		ddata->result |= ST_A9_IRQ_EN_EXT_1;
> +		break;
> +	case ST_IRQ_SYSCFG_EXT_2 :
> +		ddata->result |= ST_A9_IRQ_EN_EXT_2;
> +		break;
> +	case ST_IRQ_SYSCFG_CTI_0 :
> +		ddata->result |= ST_A9_IRQ_EN_CTI_0;
> +		break;
> +	case ST_IRQ_SYSCFG_CTI_1 :
> +		ddata->result |= ST_A9_IRQ_EN_CTI_1;
> +		break;
> +	case ST_IRQ_SYSCFG_PMU_0 :
> +		ddata->result |= ST_A9_IRQ_EN_PMU_0;
> +		break;
> +	case ST_IRQ_SYSCFG_PMU_1 :
> +		ddata->result |= ST_A9_IRQ_EN_PMU_1;
> +		break;
> +	case ST_IRQ_SYSCFG_pl310_L2 :
> +		ddata->result |= ST_A9_IRQ_EN_pl310_L2;
> +		break;
> +	case ST_IRQ_SYSCFG_DISABLED :
> +		return 0;
> +	default :
> +		dev_err(&pdev->dev, "Unrecognised device %d\n", device);

dev_dbg

> +		return -EINVAL;
> +	}
> +
> +	/* Select IRQ/FIQ channel for device. */
> +	ddata->result |= irq ?
> +		ST_A9_IRQ_N_SEL(device, channel) :
> +		ST_A9_FIQ_N_SEL(device, channel);
> +
> +	return 0;
> +}
> +
> +static int st_irq_syscfg_enable(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct st_irq_syscfg *ddata = dev_get_drvdata(&pdev->dev);
> +	int channels, ret, i;
> +	u32 device, invert;
> +


> +	channels = of_property_count_u32_elems(np, "st,irq-device");
> +	if (channels != ST_A9_IRQ_MAX_CHANS) {
> +		dev_err(&pdev->dev, "st,enable-irq-device must have 2 elems\n");
> +		return -EINVAL;
> +	}
> +
> +	channels = of_property_count_u32_elems(np, "st,fiq-device");
> +	if (channels != ST_A9_IRQ_MAX_CHANS) {
> +		dev_err(&pdev->dev, "st,enable-fiq-device must have 2 elems\n");
> +		return -EINVAL;
> +	}

I would drop these two blocks,

> +
> +	for (i = 0; i < channels; i++) {

then use ST_A9_IRQ_MAX_CHANS here

> +		of_property_read_u32_index(np,"st,irq-device", i, &device);

and dev_dbg() if of_property_read_u32_index() returns an error

> +
> +		ret = st_irq_xlate(pdev, device, i, true);
> +		if (ret)
> +			return ret;
> +
> +		of_property_read_u32_index(np,"st,fiq-device", i, &device);

for both of these.

> +
> +		ret = st_irq_xlate(pdev, device, i, false);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* External IRQs may be inverted. */
> +	of_property_read_u32(np, "st,invert-ext", &invert);
> +	ddata->result |= ST_A9_EXTIRQ_INV_SEL(invert);
> +
> +	return regmap_update_bits(ddata->regmap, ddata->syscfg,
> +				  ST_A9_IRQ_MASK, ddata->result);
> +}
> +
> +int st_irq_syscfg_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *match;
> +	struct st_irq_syscfg *ddata;
> +
> +	match = of_match_device(st_irq_syscfg_match, &pdev->dev);
> +	if (!np)

if (!match) ?

> +		return -ENODEV;
> +
> +	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
> +	if (IS_ERR(ddata->regmap)) {
> +		dev_err(&pdev->dev, "syscfg phandle missing\n");

dev_dbg

> +		return PTR_ERR(ddata->regmap);
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, ddata);
> +
> +	return st_irq_syscfg_enable(pdev);
> +}
> +
> +static int st_irq_syscfg_resume(struct device *dev)
> +{
> +	struct st_irq_syscfg *ddata = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(ddata->regmap, ddata->syscfg,
> +				  ST_A9_IRQ_MASK, ddata->result);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(st_irq_syscfg_pm_ops, NULL, st_irq_syscfg_resume);
> +
> +static struct platform_driver st_irq_syscfg_driver = {
> +	.driver = {
> +		.name = "st_irq_syscfg",
> +		.pm = &st_irq_syscfg_pm_ops,
> +		.of_match_table = st_irq_syscfg_match,
> +	},
> +	.probe = st_irq_syscfg_probe,
> +};
> +
> +static int __init st_irq_syscfg_init(void)
> +{
> +	return platform_driver_register(&st_irq_syscfg_driver);
> +}
> +core_initcall(st_irq_syscfg_init);

thx,

Jason.

Re: [PATCH 2/6] irqchip: Supply new driver for STi based devices

[Lee Jones]

On Mon, 18 Aug 2014, Jason Cooper wrote:
> On Tue, Jul 29, 2014 at 03:05:40PM +0100, Lee Jones wrote:
> > This driver is used to enable System Configuration Register controlled
> > External, CTI (Core Sight), PMU (Performance Management), and PL310 L2
> > Cache IRQs prior to use.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/irqchip/Kconfig  |   7 ++
> >  drivers/irqchip/Makefile |   1 +
> >  drivers/irqchip/irq-st.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 214 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-st.c

Wow, I forgot all about this!

I'll fixup and resubmit after the merge window.

> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index bbb746e..7252de9 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -91,3 +91,10 @@ config IRQ_CROSSBAR
> >  	  The primary irqchip invokes the crossbar's callback which inturn allocates
> >  	  a free irq and configures the IP. Thus the peripheral interrupts are
> >  	  routed to one of the free irqchip interrupt lines.
> > +
> > +config ST_IRQCHIP
> > +	bool
> > +	select REGMAP
> > +	select MFD_SYSCON
> > +	help
> > +	  Enables SysCfg Controlled IRQs on STi based platforms.
> 
> Now that I have my head above water (a bit) wrt irqchip, I really don't
> like the hot mess that this file has become...

What does that mean?

> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 62a13e5..f859c14 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -30,3 +30,4 @@ obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
> >  obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
> >  obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
> >  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
> > +obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
> > diff --git a/drivers/irqchip/irq-st.c b/drivers/irqchip/irq-st.c
> > new file mode 100644
> > index 0000000..f31126f
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-st.c

[...]

> > +#define ST_A9_IRQ_EN_pl310_L2		BIT(4)
> 
> PL310

Good spot.

[...]

> > +	/* Set the device enable bit. */
> > +	switch (device) {
> > +	case ST_IRQ_SYSCFG_EXT_0 :
> > +		ddata->result |= ST_A9_IRQ_EN_EXT_0;
> > +		break;
> > +	case ST_IRQ_SYSCFG_EXT_1 :
> > +		ddata->result |= ST_A9_IRQ_EN_EXT_1;
> > +		break;
> > +	case ST_IRQ_SYSCFG_EXT_2 :
> > +		ddata->result |= ST_A9_IRQ_EN_EXT_2;
> > +		break;
> > +	case ST_IRQ_SYSCFG_CTI_0 :
> > +		ddata->result |= ST_A9_IRQ_EN_CTI_0;
> > +		break;
> > +	case ST_IRQ_SYSCFG_CTI_1 :
> > +		ddata->result |= ST_A9_IRQ_EN_CTI_1;
> > +		break;
> > +	case ST_IRQ_SYSCFG_PMU_0 :
> > +		ddata->result |= ST_A9_IRQ_EN_PMU_0;
> > +		break;
> > +	case ST_IRQ_SYSCFG_PMU_1 :
> > +		ddata->result |= ST_A9_IRQ_EN_PMU_1;
> > +		break;
> > +	case ST_IRQ_SYSCFG_pl310_L2 :
> > +		ddata->result |= ST_A9_IRQ_EN_pl310_L2;
> > +		break;
> > +	case ST_IRQ_SYSCFG_DISABLED :
> > +		return 0;
> > +	default :
> > +		dev_err(&pdev->dev, "Unrecognised device %d\n", device);
> 
> dev_dbg

I believe dev_err() to be correct here, as this is an error
condition.

[...]

> > +	channels = of_property_count_u32_elems(np, "st,irq-device");
> > +	if (channels != ST_A9_IRQ_MAX_CHANS) {
> > +		dev_err(&pdev->dev, "st,enable-irq-device must have 2 elems\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	channels = of_property_count_u32_elems(np, "st,fiq-device");
> > +	if (channels != ST_A9_IRQ_MAX_CHANS) {
> > +		dev_err(&pdev->dev, "st,enable-fiq-device must have 2 elems\n");
> > +		return -EINVAL;
> > +	}
> 
> I would drop these two blocks,
> 
> > +
> > +	for (i = 0; i < channels; i++) {
> 
> then use ST_A9_IRQ_MAX_CHANS here
> 
> > +		of_property_read_u32_index(np,"st,irq-device", i, &device);
> 
> and dev_dbg() if of_property_read_u32_index() returns an error

But what if less than ST_A9_IRQ_MAX_CHANS channels are found?
of_property_read_u32_index() will not return an error and we will have
the incorrect number of channels?  I'm not sure that it's okay to have
less than ST_A9_IRQ_MAX_CHANS.

And I think you mean dev_err().  dev_dbg() is for code used to debug
the driver/kernel.  Useful error messages such as these should be
printed in the system log at lower printk levels.

> > +
> > +		ret = st_irq_xlate(pdev, device, i, true);
> > +		if (ret)
> > +			return ret;
> > +
> > +		of_property_read_u32_index(np,"st,fiq-device", i, &device);
> 
> for both of these.
> 
> > +
> > +		ret = st_irq_xlate(pdev, device, i, false);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* External IRQs may be inverted. */
> > +	of_property_read_u32(np, "st,invert-ext", &invert);
> > +	ddata->result |= ST_A9_EXTIRQ_INV_SEL(invert);
> > +
> > +	return regmap_update_bits(ddata->regmap, ddata->syscfg,
> > +				  ST_A9_IRQ_MASK, ddata->result);
> > +}
> > +
> > +int st_irq_syscfg_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	const struct of_device_id *match;
> > +	struct st_irq_syscfg *ddata;
> > +
> > +	match = of_match_device(st_irq_syscfg_match, &pdev->dev);
> > +	if (!np)
> 
> if (!match) ?

Whoah!  Yes, absolutely.

Actually, the match information isn't even used.  I need to take a
closer look at this.

> > +		return -ENODEV;
> > +
> > +	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> > +	if (!ddata)
> > +		return -ENOMEM;
> > +
> > +	ddata->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
> > +	if (IS_ERR(ddata->regmap)) {
> > +		dev_err(&pdev->dev, "syscfg phandle missing\n");
> 
> dev_dbg

No.  What makes you think that?

When the driver fails, to inform the user dev_err() should always be
used.  If something odd happens, but the driver can still continue
then dev_warn() should be used.  dev_dbg() should only be used for
debug information that is useful for the developer, but not for the
end user.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] irqchip: irq-st: Add documentation for STi based syscfg IRQs

[Lee Jones]

On Mon, 18 Aug 2014, Jason Cooper wrote:

> Hey Lee,
> 
> Rob, Grant, question for you below:
> 
> On Tue, Jul 29, 2014 at 03:05:41PM +0100, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../interrupt-controller/st,sti-irq-syscfg.txt     | 35 ++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> > new file mode 100644
> > index 0000000..ced6014
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/st,sti-irq-syscfg.txt
> > @@ -0,0 +1,35 @@
> > +STMicroelectronics STi System Configuration Controlled IRQs
> > +-----------------------------------------------------------
> > +
> > +On STi based systems; External, CTI (Core Sight), PMU (Performance Management),
> > +and PL310 L2 Cache IRQs are controlled using System Configuration registers.
> > +This driver is used to unmask them prior to use.
> 
> We don't usually refer to the driver in binding docs as they are
> supposed to be OS-agnostic.

I beg to differ:

$ git grep -i driver -- Documentation/devicetree/ | wc -l
355

I can understand if I was referencing Linux driver implementation
specifics, but any of the OS's drivers would also be un/masking IRQs
in order to provide this functionality.


> > +Required properties:
> > +- compatible	: Should be set to one of:
> > +			"st,stih415-irq-syscfg"
> > +			"st,stih416-irq-syscfg"
> > +			"st,stih407-irq-syscfg"
> > +			"st,stid127-irq-syscfg"
> > +- st,syscfg	: Phandle to Cortex-A9 IRQ system config registers
> > +- st,irq-device	: Array of IRQs to enable - should be 2 in length
> > +- st,fiq-device	: Array of FIQs to enable - should be 2 in length
> > +
> > +Optional properties:
> > +- st,invert-ext	: External IRQs can be inverted at will.  This property inverts
> > +		  these IRQs using bitwise logic.  A number of defines have been
> > +		  provided for convenience:
> > +			ST_IRQ_SYSCFG_EXT_1_INV
> > +			ST_IRQ_SYSCFG_EXT_2_INV
> > +			ST_IRQ_SYSCFG_EXT_3_INV
> > +Example:
> 
> Rob, Grant, should we be mingling implementation details (macros from
> include files) with the binding documentation?  It just _seems_ wrong
> too me.  But if we agree that the binding docs, the dts files, and their
> includes are always kept together, I also don't see the harm...

IMHO, we should be able to document anything here which appears in the
DTS(I) files.

> > +irq-syscfg {
> > +	compatible    = "st,stih416-irq-syscfg";
> > +	st,syscfg     = <&syscfg_cpu>;
> > +	st,irq-device = <ST_IRQ_SYSCFG_PMU_0>,
> > +			<ST_IRQ_SYSCFG_PMU_1>;
> > +	st,fiq-device = <ST_IRQ_SYSCFG_DISABLED>,
> > +			<ST_IRQ_SYSCFG_DISABLED>;
> > +	st,invert-ext = <(ST_IRQ_SYSCFG_EXT_1_INV | ST_IRQ_SYSCFG_EXT_3_INV)>;
> > +};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog