[PATCH v2 0/9] Add STM32 EXTI interrupt controller support

[PATCH v2 0/9] Add STM32 EXTI interrupt controller support

[Maxime Coquelin]

Hi,

This v2 is only a rebase on top of v4.6-rc1 and also changes a variable name.

The series adds support to EXTI interrupt controller and GPIO IRQ support in
STM32 pinctrl driver.

The STM32 external interrupt controller consists of edge detectors that
generate interrupts requests or wake-up events.

Each line can be independently configured as interrupt or wake-up source,
and triggers either on rising, fallin or both edges. Each line can also
be masked independently.

Regards,
Maxime

Changes since v1:
-----------------
 - Rebased on top of v4.6-rc1
 - Change variable name from virq to irq (Linus W.)

Maxime Coquelin (9):
  Documentation: dt-bindings: Document STM32 EXTI controller bindings
  drivers: irqchip: Add STM32 external interrupts support
  ARM: STM32: Select external interrupts controller
  ARM: dts: Add EXTI controller node to stm32f429
  Documentation: dt-bindings: Add IRQ related properties of STM32
    pinctrl
  pinctrl: Add IRQ support to STM32 gpios
  ARM: dts: Add GPIO irq support to STM2F429
  ARM: dts: Declare push button as GPIO key on stm32f429 Disco board
  ARM: config: Enable GPIO Key driver in stm32_defconfig

 .../interrupt-controller/st,stm32-exti.txt         |  20 +++
 .../bindings/pinctrl/st,stm32-pinctrl.txt          |   3 +
 arch/arm/Kconfig                                   |   1 +
 arch/arm/boot/dts/stm32429i-eval.dts               |  18 +++
 arch/arm/boot/dts/stm32f429-disco.dts              |  13 ++
 arch/arm/boot/dts/stm32f429.dtsi                   |  10 ++
 arch/arm/configs/stm32_defconfig                   |   6 +-
 drivers/irqchip/Kconfig                            |   4 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-stm32-exti.c                   | 169 +++++++++++++++++++++
 drivers/pinctrl/stm32/Kconfig                      |   1 +
 drivers/pinctrl/stm32/pinctrl-stm32.c              |  68 +++++++++
 12 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
 create mode 100644 drivers/irqchip/irq-stm32-exti.c

-- 
1.9.1

[PATCH v2 1/9] Documentation: dt-bindings: Document STM32 EXTI controller bindings

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 .../bindings/interrupt-controller/st,stm32-exti.txt  | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
new file mode 100644
index 000000000000..6e7703d4ff5b
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
@@ -0,0 +1,20 @@
+STM32 External Interrupt Controller
+
+Required properties:
+
+- compatible: Should be "st,stm32-exti"
+- reg: Specifies base physical address and size of the registers
+- interrupt-controller: Indentifies the node as an interrupt controller
+- #interrupt-cells: Specifies the number of cells to encode an interrupt
+  specifier, shall be 2
+- interrupts: interrupts references to primary interrupt controller
+
+Example:
+
+exti: interrupt-controller@40013c00 {
+	compatible = "st,stm32-exti";
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	reg = <0x40013C00 0x400>;
+	interrupts = <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <42>, <62>, <76>;
+};
-- 
1.9.1

[PATCH v2 2/9] drivers: irqchip: Add STM32 external interrupts support

[Maxime Coquelin]

The STM32 external interrupt controller consists of edge detectors that
generate interrupts requests or wake-up events.

Each line can be independently configured as interrupt or wake-up source,
and triggers either on rising, fallin or both edges. Each line can also
be masked independently.

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/irqchip/Kconfig          |   4 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-stm32-exti.c | 169 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100644 drivers/irqchip/irq-stm32-exti.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e124793e224..149a30e0eec0 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -244,3 +244,7 @@ config IRQ_MXS
 config MVEBU_ODMI
 	bool
 	select GENERIC_MSI_IRQ_DOMAIN
+
+config STM32_EXTI
+	bool
+	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b03cfcbbac6b..2caeae000307 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -65,3 +65,4 @@ obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
 obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
+obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
new file mode 100644
index 000000000000..02bfa807db6c
--- /dev/null
+++ b/drivers/irqchip/irq-stm32-exti.c
@@ -0,0 +1,169 @@
+/*
+ * Copyright (C) Maxime Coquelin 2015
+ * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/bitops.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define EXTI_IMR	0x0
+#define EXTI_EMR	0x4
+#define EXTI_RTSR	0x8
+#define EXTI_FTSR	0xc
+#define EXTI_SWIER	0x10
+#define EXTI_PR		0x14
+
+static void stm32_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct irq_chip_generic *gc = domain->gc->gc[0];
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long pending;
+	int n;
+
+	chained_irq_enter(chip, desc);
+
+	pending = irq_reg_readl(gc, EXTI_PR);
+	for_each_set_bit(n, &pending, BITS_PER_LONG) {
+		generic_handle_irq(irq_find_mapping(domain, n));
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int stm32_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	u32 rtsr, ftsr;
+	int pin = data->hwirq;
+
+	irq_gc_lock(gc);
+
+	rtsr = irq_reg_readl(gc, EXTI_RTSR);
+	ftsr = irq_reg_readl(gc, EXTI_FTSR);
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		rtsr |= BIT(pin);
+		ftsr &= ~BIT(pin);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		rtsr &= ~BIT(pin);
+		ftsr |= BIT(pin);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		rtsr |= BIT(pin);
+		ftsr |= BIT(pin);
+		break;
+	default:
+		irq_gc_unlock(gc);
+		return -EINVAL;
+	}
+
+	irq_reg_writel(gc, rtsr, EXTI_RTSR);
+	irq_reg_writel(gc, ftsr, EXTI_FTSR);
+
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+static int stm32_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	int pin = data->hwirq;
+	u32 emr;
+
+	irq_gc_lock(gc);
+
+	emr = irq_reg_readl(gc, EXTI_EMR);
+	if (on)
+		emr |= BIT(pin);
+	else
+		emr &= ~BIT(pin);
+	irq_reg_writel(gc, emr, EXTI_EMR);
+
+	irq_gc_unlock(gc);
+
+	return 0;
+}
+
+static int __init stm32_exti_init(struct device_node *node,
+				  struct device_node *parent)
+{
+	int nr_irqs, nr_exti, ret, i;
+	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
+	struct irq_domain *domain;
+	struct irq_chip_generic *gc;
+	void *base;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s: Unable to map registers\n", node->full_name);
+		return -ENOMEM;
+	}
+
+	/* Determine number of irqs supported */
+	writel_relaxed(~0UL, base + EXTI_RTSR);
+	nr_exti = fls(readl_relaxed(base + EXTI_RTSR));
+	writel_relaxed(0, base + EXTI_RTSR);
+
+	pr_info("%s: %d External IRQs detected\n", node->full_name, nr_exti);
+
+	domain = irq_domain_add_linear(node, nr_exti,
+				       &irq_generic_chip_ops, NULL);
+	if (!domain) {
+		pr_err("%s: Could not register interrupt domain.\n",
+				node->name);
+		ret = -ENOMEM;
+		goto out_unmap;
+	}
+
+	ret = irq_alloc_domain_generic_chips(domain, nr_exti, 1, "exti",
+					     handle_edge_irq, clr, 0, 0);
+	if (ret) {
+		pr_err("%s: Could not allocate generic interrupt chip.\n",
+			node->full_name);
+		goto out_free_domain;
+	}
+
+	gc = domain->gc->gc[0];
+	gc->reg_base                         = base;
+	gc->chip_types->type               = IRQ_TYPE_EDGE_BOTH;
+	gc->chip_types->chip.name          = gc->chip_types[0].chip.name;
+	gc->chip_types->chip.irq_ack       = irq_gc_ack_set_bit;
+	gc->chip_types->chip.irq_mask      = irq_gc_mask_clr_bit;
+	gc->chip_types->chip.irq_unmask    = irq_gc_mask_set_bit;
+	gc->chip_types->chip.irq_set_type  = stm32_irq_set_type;
+	gc->chip_types->chip.irq_set_wake  = stm32_irq_set_wake;
+	gc->chip_types->regs.ack           = EXTI_PR;
+	gc->chip_types->regs.mask          = EXTI_IMR;
+	gc->chip_types->handler            = handle_edge_irq;
+
+	nr_irqs = of_irq_count(node);
+	for (i = 0; i < nr_irqs; i++) {
+		unsigned int irq = irq_of_parse_and_map(node, i);
+
+		irq_set_handler_data(irq, domain);
+		irq_set_chained_handler(irq, stm32_irq_handler);
+	}
+
+	return 0;
+
+out_free_domain:
+	irq_domain_remove(domain);
+out_unmap:
+	iounmap(base);
+	return ret;
+}
+
+IRQCHIP_DECLARE(stm32_exti, "st,stm32-exti", stm32_exti_init);
-- 
1.9.1

[PATCH v2 3/9] ARM: STM32: Select external interrupts controller

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index cdfa6c2b7626..13b230a731eb 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -884,6 +884,7 @@ config ARCH_STM32
 	select CLKSRC_STM32
 	select PINCTRL
 	select RESET_CONTROLLER
+	select STM32_EXTI
 	help
 	  Support for STMicroelectronics STM32 processors.
 
-- 
1.9.1

[PATCH v2 4/9] ARM: dts: Add EXTI controller node to stm32f429

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index 35df462559ca..1a189d44ad38 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -176,6 +176,14 @@
 			reg = <0x40013800 0x400>;
 		};
 
+		exti: interrupt-controller@40013c00 {
+			compatible = "st,stm32-exti";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			reg = <0x40013C00 0x400>;
+			interrupts = <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <42>, <62>, <76>;
+		};
+
 		pin-controller {
 			#address-cells = <1>;
 			#size-cells = <1>;
-- 
1.9.1

[PATCH v2 5/9] Documentation: dt-bindings: Add IRQ related properties of STM32 pinctrl

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
index 7b4800cc251e..dd95becba966 100644
--- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
@@ -13,6 +13,9 @@ Required properies:
  - #size-cells	: The value of this property must be 1
  - ranges	: defines mapping between pin controller node (parent) to
    gpio-bank node (children).
+ - interrupt-parent: phandle of the interrupt parent to which the external
+   GPIO interrupts are forwarded to.
+ - st,syscfg: phandle of the syscfg node used for IRQ mux selection.
  - pins-are-numbered: Specify the subnodes are using numbered pinmux to
    specify pins.
 
-- 
1.9.1

[PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

This patch adds IRQ support to STM32 gpios.

The EXTI controller has 16 lines dedicated to GPIOs.
EXTI line n can be connected to only line n of one of the GPIO ports, for
example EXTI0 can be connected to either PA0, or PB0, or PC0...
This port selection is done by specifying the port number into System
Config registers.

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/pinctrl/stm32/Kconfig         |  1 +
 drivers/pinctrl/stm32/pinctrl-stm32.c | 68 +++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/pinctrl/stm32/Kconfig b/drivers/pinctrl/stm32/Kconfig
index 0f28841b2332..b5cac5bfd0cd 100644
--- a/drivers/pinctrl/stm32/Kconfig
+++ b/drivers/pinctrl/stm32/Kconfig
@@ -6,6 +6,7 @@ config PINCTRL_STM32
 	select PINMUX
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select MFD_SYSCON
 
 config PINCTRL_STM32F429
 	bool "STMicroelectronics STM32F429 pin control" if COMPILE_TEST && !MACH_STM32F429
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 8deb566ed4cd..f2fa717894dc 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -8,6 +8,8 @@
 #include <linux/clk.h>
 #include <linux/gpio/driver.h>
 #include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -20,6 +22,7 @@
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 
@@ -77,6 +80,9 @@ struct stm32_pinctrl {
 	struct stm32_gpio_bank *banks;
 	unsigned nbanks;
 	const struct stm32_pinctrl_match_data *match_data;
+	struct irq_domain	*domain;
+	struct regmap		*regmap;
+	struct regmap_field	*irqmux[STM32_GPIO_PINS_PER_BANK];
 };
 
 static inline int stm32_gpio_pin(int gpio)
@@ -174,6 +180,22 @@ static int stm32_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
+
+static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
+	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
+	unsigned int irq;
+
+	regmap_field_write(pctl->irqmux[offset], bank->range.id);
+
+	irq = irq_create_mapping(pctl->domain, offset);
+	if (!irq)
+		return -ENXIO;
+
+	return irq;
+}
+
 static struct gpio_chip stm32_gpio_template = {
 	.request		= stm32_gpio_request,
 	.free			= stm32_gpio_free,
@@ -181,6 +203,7 @@ static struct gpio_chip stm32_gpio_template = {
 	.set			= stm32_gpio_set,
 	.direction_input	= stm32_gpio_direction_input,
 	.direction_output	= stm32_gpio_direction_output,
+	.to_irq			= stm32_gpio_to_irq,
 };
 
 /* Pinctrl functions */
@@ -704,6 +727,47 @@ static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl,
 	return 0;
 }
 
+static int stm32_pctrl_dt_setup_irq(struct platform_device *pdev,
+			   struct stm32_pinctrl *pctl)
+{
+	struct device_node *np = pdev->dev.of_node, *parent;
+	struct device *dev = &pdev->dev;
+	struct regmap *rm;
+	int offset, ret, i;
+
+	parent = of_irq_find_parent(np);
+	if (!parent)
+		return -ENXIO;
+
+	pctl->domain = irq_find_host(parent);
+	if (!pctl->domain)
+		return -ENXIO;
+
+	pctl->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+	if (IS_ERR(pctl->regmap))
+		return PTR_ERR(pctl->regmap);
+
+	rm = pctl->regmap;
+
+	ret = of_property_read_u32_index(np, "st,syscfg", 1, &offset);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < STM32_GPIO_PINS_PER_BANK; i++) {
+		struct reg_field mux;
+
+		mux.reg = offset + (i / 4) * 4;
+		mux.lsb = (i % 4) * 4;
+		mux.msb = mux.lsb + 3;
+
+		pctl->irqmux[i] = devm_regmap_field_alloc(dev, rm, mux);
+		if (IS_ERR(pctl->irqmux[i]))
+			return PTR_ERR(pctl->irqmux[i]);
+	}
+
+	return 0;
+}
+
 static int stm32_pctrl_build_state(struct platform_device *pdev)
 {
 	struct stm32_pinctrl *pctl = platform_get_drvdata(pdev);
@@ -796,6 +860,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = stm32_pctrl_dt_setup_irq(pdev, pctl);
+	if (ret)
+		return ret;
+
 	pins = devm_kcalloc(&pdev->dev, pctl->match_data->npins, sizeof(*pins),
 			    GFP_KERNEL);
 	if (!pins)
-- 
1.9.1

[PATCH v2 7/9] ARM: dts: Add GPIO irq support to STM2F429

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index 1a189d44ad38..68247625a8d5 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -189,6 +189,8 @@
 			#size-cells = <1>;
 			compatible = "st,stm32f429-pinctrl";
 			ranges = <0 0x40020000 0x3000>;
+			interrupt-parent = <&exti>;
+			st,syscfg = <&syscfg 0x8>;
 			pins-are-numbered;
 
 			gpioa: gpio@40020000 {
-- 
1.9.1

[PATCH v2 8/9] ARM: dts: Declare push button as GPIO key on stm32f429 Disco board

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/boot/dts/stm32429i-eval.dts  | 18 ++++++++++++++++++
 arch/arm/boot/dts/stm32f429-disco.dts | 13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
index 6bfc5959dac3..0fd78e4e8a45 100644
--- a/arch/arm/boot/dts/stm32429i-eval.dts
+++ b/arch/arm/boot/dts/stm32429i-eval.dts
@@ -47,6 +47,7 @@
 
 /dts-v1/;
 #include "stm32f429.dtsi"
+#include <dt-bindings/input/input.h>
 
 / {
 	model = "STMicroelectronics STM32429i-EVAL board";
@@ -82,6 +83,23 @@
 		};
 	};
 
+	gpio_keys {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		autorepeat;
+		button@0 {
+			label = "Wake up";
+			linux,code = <KEY_WAKEUP>;
+			gpios = <&gpioa 0 0>;
+		};
+		button@1 {
+			label = "Tamper";
+			linux,code = <KEY_RESTART>;
+			gpios = <&gpioc 13 0>;
+		};
+	};
+
 	usbotg_hs_phy: usbphy {
 		#phy-cells = <0>;
 		compatible = "usb-nop-xceiv";
diff --git a/arch/arm/boot/dts/stm32f429-disco.dts b/arch/arm/boot/dts/stm32f429-disco.dts
index 01408073dd53..7d0415e80668 100644
--- a/arch/arm/boot/dts/stm32f429-disco.dts
+++ b/arch/arm/boot/dts/stm32f429-disco.dts
@@ -47,6 +47,7 @@
 
 /dts-v1/;
 #include "stm32f429.dtsi"
+#include <dt-bindings/input/input.h>
 
 / {
 	model = "STMicroelectronics STM32F429i-DISCO board";
@@ -75,6 +76,18 @@
 			linux,default-trigger = "heartbeat";
 		};
 	};
+
+	gpio_keys {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		autorepeat;
+		button@0 {
+			label = "User";
+			linux,code = <KEY_HOME>;
+			gpios = <&gpioa 0 0>;
+		};
+	};
 };
 
 &clk_hse {
-- 
1.9.1

[PATCH v2 9/9] ARM: config: Enable GPIO Key driver in stm32_defconfig

[Maxime Coquelin]

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 arch/arm/configs/stm32_defconfig | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index 1e5ec2a0e4cf..e7b56d4f1798 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -38,7 +38,11 @@ CONFIG_DEVTMPFS_MOUNT=y
 # CONFIG_FW_LOADER is not set
 # CONFIG_BLK_DEV is not set
 CONFIG_EEPROM_93CX6=y
-# CONFIG_INPUT is not set
+# CONFIG_INPUT_LEDS is not set
+# CONFIG_INPUT_MOUSEDEV is not set
+# CONFIG_KEYBOARD_ATKBD is not set
+CONFIG_KEYBOARD_GPIO=y
+# CONFIG_INPUT_MOUSE is not set
 # CONFIG_SERIO is not set
 # CONFIG_VT is not set
 # CONFIG_UNIX98_PTYS is not set
-- 
1.9.1

Re: [PATCH v2 1/9] Documentation: dt-bindings: Document STM32 EXTI controller bindings

[Rob Herring]

On Thu, Mar 31, 2016 at 05:09:31PM +0200, Maxime Coquelin wrote:
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  .../bindings/interrupt-controller/st,stm32-exti.txt  | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
> new file mode 100644
> index 000000000000..6e7703d4ff5b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
> @@ -0,0 +1,20 @@
> +STM32 External Interrupt Controller
> +
> +Required properties:
> +
> +- compatible: Should be "st,stm32-exti"
> +- reg: Specifies base physical address and size of the registers
> +- interrupt-controller: Indentifies the node as an interrupt controller
> +- #interrupt-cells: Specifies the number of cells to encode an interrupt
> +  specifier, shall be 2
> +- interrupts: interrupts references to primary interrupt controller

Need to define how many and what is the order?

Are these 1:1 mapping? You could use interrupt-map here to define the 
mapping.

> +
> +Example:
> +
> +exti: interrupt-controller@40013c00 {
> +	compatible = "st,stm32-exti";
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	reg = <0x40013C00 0x400>;
> +	interrupts = <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <42>, <62>, <76>;
> +};
> -- 
> 1.9.1
> 

Re: [PATCH v2 5/9] Documentation: dt-bindings: Add IRQ related properties of STM32 pinctrl

[Rob Herring]

On Thu, Mar 31, 2016 at 05:09:35PM +0200, Maxime Coquelin wrote:
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/9] Documentation: dt-bindings: Document STM32 EXTI controller bindings

[Maxime Coquelin]

Hi Rob,

2016-04-04 7:15 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Thu, Mar 31, 2016 at 05:09:31PM +0200, Maxime Coquelin wrote:
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> ---
>>  .../bindings/interrupt-controller/st,stm32-exti.txt  | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
>> new file mode 100644
>> index 000000000000..6e7703d4ff5b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.txt
>> @@ -0,0 +1,20 @@
>> +STM32 External Interrupt Controller
>> +
>> +Required properties:
>> +
>> +- compatible: Should be "st,stm32-exti"
>> +- reg: Specifies base physical address and size of the registers
>> +- interrupt-controller: Indentifies the node as an interrupt controller
>> +- #interrupt-cells: Specifies the number of cells to encode an interrupt
>> +  specifier, shall be 2
>> +- interrupts: interrupts references to primary interrupt controller
>
> Need to define how many and what is the order?
The exti driver uses of_irq_count() to count the number of interrupts.
The order doesn't matter in my implementation, I will come back on
this point below.

>
> Are these 1:1 mapping? You could use interrupt-map here to define the
> mapping.

No, this is not 1:1 mapping.
This is the mapping for STM32F429 (With 'n' managed by a mux in Syscfg
(from GPIOA to GPIOI)):
EXTI0 (GPIOn0) : NVIC 6
EXTI1 (GPIOn1) : NVIC 7
EXTI2 (GPIOn2) : NVIC 8
EXTI3 (GPIOn3) : NVIC 9
EXTI4 (GPIOn4) : NVIC 10
EXTI5 (GPIOn5) : NVIC 23
EXTI6 (GPIOn6) : NVIC 23
EXTI7 (GPIOn7) : NVIC 23
EXTI8 (GPIOn8) : NVIC 23
EXTI9 (GPIOn9) : NVIC 23
EXTI10 (GPIOn10) : NVIC 40
EXTI11 (GPIOn11) : NVIC 40
EXTI12 (GPIOn12) : NVIC 40
EXTI13 (GPIOn13) : NVIC 40
EXTI14 (GPIOn14) : NVIC 40
EXTI15 (GPIOn15) : NVIC 40
EXTI16 (PVD) : NVIC 1
EXTI17 (RTC Alarm) : NVIC 41
EXTI18 (USB OTG FS Wakeup) : NVIC 42
EXTI19 (Ethernet Wakeup) : NVIC 62
EXTI20 (USB OTG HS Wakeup) : NVIC 76
EXTI21 (RTC Tamper) : NVIC 2
EXTI22 (RTC Wakeup) : NVIC 3

Ideally, we should define a kind of mapping in the DT node.
However, from what I understand 'interrupt-map' is not intended to be used
in an interrupt controller (it would not even be parsed in of_irq_parse_raw()).

Any ideas?

Thanks for the review,
Maxime

Re: [PATCH v2 2/9] drivers: irqchip: Add STM32 external interrupts support

[Linus Walleij]

On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> +static void stm32_irq_handler(struct irq_desc *desc)
> +{
> +       struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +       struct irq_chip_generic *gc = domain->gc->gc[0];
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       unsigned long pending;
> +       int n;
> +
> +       chained_irq_enter(chip, desc);
> +
> +       pending = irq_reg_readl(gc, EXTI_PR);
> +       for_each_set_bit(n, &pending, BITS_PER_LONG) {
> +               generic_handle_irq(irq_find_mapping(domain, n));
> +       }
> +
> +       chained_irq_exit(chip, desc);
> +}

Is this one of those cases where you should re-read the status register
on every iteration, so as to avoid exiting and immediately re-entering
the irq handler?

C.g irq-vic.c:

static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
{
        u32 stat, irq;
        int handled = 0;

        while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
                irq = ffs(stat) - 1;
                handle_domain_irq(vic->domain, irq, regs);
                handled = 1;
        }

        return handled;
}

Yours,
Linus Walleij

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Linus Walleij]

On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
> +       unsigned int irq;
> +
> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);

No. You must implement the irqchip and GPIO controllers to
be orthogonal, doing things like this creates a semantic that
assumes .to_irq() is always called before using the IRQ and
that is not guaranteed at all. A consumer may very well
use an interrupt right off the irqchip without this being called
first. All this function should do is translate a number. No
other semantics.

This needs to be done from the irqchip (sorry).

Yours,
Linus Walleij

Re: [PATCH v2 2/9] drivers: irqchip: Add STM32 external interrupts support

[Linus Walleij]

On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> +       gc = domain->gc->gc[0];
> +       gc->reg_base                         = base;
> +       gc->chip_types->type               = IRQ_TYPE_EDGE_BOTH;
> +       gc->chip_types->chip.name          = gc->chip_types[0].chip.name;
> +       gc->chip_types->chip.irq_ack       = irq_gc_ack_set_bit;
> +       gc->chip_types->chip.irq_mask      = irq_gc_mask_clr_bit;
> +       gc->chip_types->chip.irq_unmask    = irq_gc_mask_set_bit;
> +       gc->chip_types->chip.irq_set_type  = stm32_irq_set_type;
> +       gc->chip_types->chip.irq_set_wake  = stm32_irq_set_wake;
> +       gc->chip_types->regs.ack           = EXTI_PR;
> +       gc->chip_types->regs.mask          = EXTI_IMR;
> +       gc->chip_types->handler            = handle_edge_irq;

If this is used by a GPIO chip (as happens in another part of
the series), you need to set up the .irq_request_resources()
and .irq_release_resources() to call gpiochip_lock_as_irq()
and gpiochip_unlock_as_irq().

As with the other comment on the GPIO patch, the separation
of concerns between irqchip and gpiochip is a bit artificial
here and breaks down somewhat.

Yours,
Linus Walleij

Re: [PATCH v2 2/9] drivers: irqchip: Add STM32 external interrupts support

[Maxime Coquelin]

Hi Linus,

Sorry for the late reply, I was off last week.

2016-04-08 11:38 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> +static void stm32_irq_handler(struct irq_desc *desc)
>> +{
>> +       struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> +       struct irq_chip_generic *gc = domain->gc->gc[0];
>> +       struct irq_chip *chip = irq_desc_get_chip(desc);
>> +       unsigned long pending;
>> +       int n;
>> +
>> +       chained_irq_enter(chip, desc);
>> +
>> +       pending = irq_reg_readl(gc, EXTI_PR);
>> +       for_each_set_bit(n, &pending, BITS_PER_LONG) {
>> +               generic_handle_irq(irq_find_mapping(domain, n));
>> +       }
>> +
>> +       chained_irq_exit(chip, desc);
>> +}
>
> Is this one of those cases where you should re-read the status register
> on every iteration, so as to avoid exiting and immediately re-entering
> the irq handler?
>
> C.g irq-vic.c:
>
> static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
> {
>         u32 stat, irq;
>         int handled = 0;
>
>         while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>                 irq = ffs(stat) - 1;
>                 handle_domain_irq(vic->domain, irq, regs);
>                 handled = 1;
>         }
>
>         return handled;
> }

Indeed, it would be better doing it like this.
Do you think I could even do this with two nested loops to reduce the
number of reg accesses?
It would look like this (just compiled, not tested):

static void stm32_irq_handler(struct irq_desc *desc)
{
    struct irq_domain *domain = irq_desc_get_handler_data(desc);
    struct irq_chip_generic *gc = domain->gc->gc[0];
    struct irq_chip *chip = irq_desc_get_chip(desc);
    unsigned long pending;
    int n;

    chained_irq_enter(chip, desc);

    while ((pending = irq_reg_readl(gc, EXTI_PR))) {
        for_each_set_bit(n, &pending, BITS_PER_LONG) {
            generic_handle_irq(irq_find_mapping(domain, n));
        }
    }

    chained_irq_exit(chip, desc);
}

Thanks,
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

Hi Linus

2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>> +       unsigned int irq;
>> +
>> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);
>
> No. You must implement the irqchip and GPIO controllers to
> be orthogonal, doing things like this creates a semantic that
> assumes .to_irq() is always called before using the IRQ and
> that is not guaranteed at all. A consumer may very well
> use an interrupt right off the irqchip without this being called
> first. All this function should do is translate a number. No
> other semantics.
>
> This needs to be done from the irqchip (sorry).

Actually, the register written here is not part of the irqchip.
It is a system config register that is only used when using a GPIO as
external interrupt.
Its aim is to mux the GPIO bank on a line.
For example on line 1, it can be muxed to select either gpioa1,
gpiob1, gpioc1, ...
How could I do it in the irqchip that has no gpio knowledge?

In case the consumer uses an interrupt right off the irqchip, it
should not access this register.
For example the RTC wakeup lines, where we will reference directly the irqchip.

That said, do you still believe the implementation is wrong? Do I miss
something?

Best regards,
Maxime

Re: [PATCH v2 2/9] drivers: irqchip: Add STM32 external interrupts support

[Linus Walleij]

On Tue, Apr 19, 2016 at 10:00 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> 2016-04-08 11:38 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:

>>         while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>>                 irq = ffs(stat) - 1;
>>                 handle_domain_irq(vic->domain, irq, regs);
>>                 handled = 1;
>>         }
>>
>>         return handled;
>> }
>
> Indeed, it would be better doing it like this.
> Do you think I could even do this with two nested loops to reduce the
> number of reg accesses?
> It would look like this (just compiled, not tested):
>
> static void stm32_irq_handler(struct irq_desc *desc)
> {
>     struct irq_domain *domain = irq_desc_get_handler_data(desc);
>     struct irq_chip_generic *gc = domain->gc->gc[0];
>     struct irq_chip *chip = irq_desc_get_chip(desc);
>     unsigned long pending;
>     int n;
>
>     chained_irq_enter(chip, desc);
>
>     while ((pending = irq_reg_readl(gc, EXTI_PR))) {
>         for_each_set_bit(n, &pending, BITS_PER_LONG) {
>             generic_handle_irq(irq_find_mapping(domain, n));
>         }
>     }

Looks clever! :)

If it also works, I'm in for it.

Yours,
Linus Walleij

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Linus Walleij]

On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>> <mcoquelin.stm32@gmail.com> wrote:
>>
>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>> +       unsigned int irq;
>>> +
>>> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>
>> No. You must implement the irqchip and GPIO controllers to
>> be orthogonal, doing things like this creates a semantic that
>> assumes .to_irq() is always called before using the IRQ and
>> that is not guaranteed at all. A consumer may very well
>> use an interrupt right off the irqchip without this being called
>> first. All this function should do is translate a number. No
>> other semantics.
>>
>> This needs to be done from the irqchip (sorry).
>
> Actually, the register written here is not part of the irqchip.
> It is a system config register that is only used when using a GPIO as
> external interrupt.
> Its aim is to mux the GPIO bank on a line.

Then it should be done in .request() for the GPIO, not in
.to_irq().

It should *also* be done in the set-up path for the irqchip
side, if that line is used without any interaction with the
gpio side of things.

> For example on line 1, it can be muxed to select either gpioa1,
> gpiob1, gpioc1, ...
> How could I do it in the irqchip that has no gpio knowledge?

I don't understand this.

We are discussing an irqchip that is tightly coupled with
a gpiochip. Usually d->hwirq is the same as the GPIO offset
but that varies.

The point is that each IRQ that ever get used
has a 1-to-1 relationship to a certain GPIO line, and if that
relationship cannot be resolved from the irqchip side,
something is wrong. The irqchip needs to enable the
GPIO line it is backing to recieve interrupts without any
requirements that .to_irq() have been called first.

If to_irq() does something else than translate to an IRQ
something is wrong.

Yours,
Linus Walleij

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Linus Walleij]

On Fri, Apr 29, 2016 at 10:53 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>>> <mcoquelin.stm32@gmail.com> wrote:
>>>
>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>> +{
>>>> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>>> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>>> +       unsigned int irq;
>>>> +
>>>> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>>
>>> No. You must implement the irqchip and GPIO controllers to
>>> be orthogonal, doing things like this creates a semantic that
>>> assumes .to_irq() is always called before using the IRQ and
>>> that is not guaranteed at all. A consumer may very well
>>> use an interrupt right off the irqchip without this being called
>>> first. All this function should do is translate a number. No
>>> other semantics.
>>>
>>> This needs to be done from the irqchip (sorry).
>>
>> Actually, the register written here is not part of the irqchip.
>> It is a system config register that is only used when using a GPIO as
>> external interrupt.
>> Its aim is to mux the GPIO bank on a line.
>
> Then it should be done in .request() for the GPIO, not in
> .to_irq().
>
> It should *also* be done in the set-up path for the irqchip
> side, if that line is used without any interaction with the
> gpio side of things.

Or, hm, maybe not in the irqchip then if it is as you say
that the interrupt can be used anyway, without this being
set up.

But it should certainly not be done in .to_irq(), rather in
.request().

Yours,
Linus Walleij

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-29 10:53 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>>> <mcoquelin.stm32@gmail.com> wrote:
>>>
>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>> +{
>>>> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>>> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>>> +       unsigned int irq;
>>>> +
>>>> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>>
>>> No. You must implement the irqchip and GPIO controllers to
>>> be orthogonal, doing things like this creates a semantic that
>>> assumes .to_irq() is always called before using the IRQ and
>>> that is not guaranteed at all. A consumer may very well
>>> use an interrupt right off the irqchip without this being called
>>> first. All this function should do is translate a number. No
>>> other semantics.
>>>
>>> This needs to be done from the irqchip (sorry).
>>
>> Actually, the register written here is not part of the irqchip.
>> It is a system config register that is only used when using a GPIO as
>> external interrupt.
>> Its aim is to mux the GPIO bank on a line.
>
> Then it should be done in .request() for the GPIO, not in
> .to_irq().
Problem is that at request time, we don't know whether the GPIO is to
be used as an interrupt or not.

I think I may have not been clear enough in the HW architecture description.
Indeed, I used the term "mux", but should instead use the term "selector".

The idea is that between the GPIO controllers and the EXTI controller,
there is one selector for each line number, to select the controller used as
interrupt source.

For example, there is a selector on line 0 to select between gpioa0, gpiob0,
gpioc0,.., gpiok0, which one is connected to exti0.

It means there is a HW restriction that makes impossible to use both GPIOA0
or GPIOB0 as interrupts at the same time.

It looks like this: http://pastebin.com/raw/cs2WiNKZ
You can directly check section 12.2.5 of the stm32f429 reference manual:
http://www2.st.com/resource/en/reference_manual/dm00031020.pdf

For example, we can imagine a board where gpioa0 is used SD's card detect,
and gpiob0 used to control a led.

If we set the mux at request time, gpiob0 request may overwrite the mux
configuration set by gpioa0, whereas it is not used as interrupt.

That is the reason why I did it in .to_irq().

>
> It should *also* be done in the set-up path for the irqchip
> side, if that line is used without any interaction with the
> gpio side of things.
Actually, a line is either used by a GPIO, (exclusive) or another purpose.
In the case of a GPIO line, I think we should always go through the gpio.

>
>> For example on line 1, it can be muxed to select either gpioa1,
>> gpiob1, gpioc1, ...
>> How could I do it in the irqchip that has no gpio knowledge?
>
> I don't understand this.
>
> We are discussing an irqchip that is tightly coupled with
> a gpiochip. Usually d->hwirq is the same as the GPIO offset
> but that varies.
>
> The point is that each IRQ that ever get used
> has a 1-to-1 relationship to a certain GPIO line, and if that
> relationship cannot be resolved from the irqchip side,
> something is wrong. The irqchip needs to enable the
> GPIO line it is backing to recieve interrupts without any
> requirements that .to_irq() have been called first.

Actually, this is not a 1:1 relationship, as for example, exti0 can be connected
to either gpioa0, or gpiob0,..., or gpiok0.

So for the exti entries that are connected to gpios, I don't see how
we can reference it
from the exti controller,
>
> If to_irq() does something else than translate to an IRQ
> something is wrong.

Ok, so maybe we need an intermediate layer between gpio and exti to manage the
selectors sysconf?
I wiil have a look in that direction, but I think it could be over-engineered.

Thanks!
Maxime

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Linus Walleij]

On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> 2016-04-29 10:53 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Tue, Apr 19, 2016 at 11:04 AM, Maxime Coquelin
>> <mcoquelin.stm32@gmail.com> wrote:
>>> 2016-04-08 11:43 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
>>>> On Thu, Mar 31, 2016 at 5:09 PM, Maxime Coquelin
>>>> <mcoquelin.stm32@gmail.com> wrote:
>>>>
>>>>> +static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>>>>> +{
>>>>> +       struct stm32_pinctrl *pctl = dev_get_drvdata(chip->parent);
>>>>> +       struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
>>>>> +       unsigned int irq;
>>>>> +
>>>>> +       regmap_field_write(pctl->irqmux[offset], bank->range.id);
>>>>
>>>> No. You must implement the irqchip and GPIO controllers to
>>>> be orthogonal, doing things like this creates a semantic that
>>>> assumes .to_irq() is always called before using the IRQ and
>>>> that is not guaranteed at all. A consumer may very well
>>>> use an interrupt right off the irqchip without this being called
>>>> first. All this function should do is translate a number. No
>>>> other semantics.
>>>>
>>>> This needs to be done from the irqchip (sorry).
>>>
>>> Actually, the register written here is not part of the irqchip.
>>> It is a system config register that is only used when using a GPIO as
>>> external interrupt.
>>> Its aim is to mux the GPIO bank on a line.
>>
>> Then it should be done in .request() for the GPIO, not in
>> .to_irq().
>
> Problem is that at request time, we don't know whether the GPIO is to
> be used as an interrupt or not.

Well the fact that .to_irq() is called does not mean that you know
it will be used as an interrupt either. Sorry. It is just a translation
function, not an allocation function.

> I think I may have not been clear enough in the HW architecture description.
> Indeed, I used the term "mux", but should instead use the term "selector".
>
> The idea is that between the GPIO controllers and the EXTI controller,
> there is one selector for each line number, to select the controller used as
> interrupt source.
>
> For example, there is a selector on line 0 to select between gpioa0, gpiob0,
> gpioc0,.., gpiok0, which one is connected to exti0.
>
> It means there is a HW restriction that makes impossible to use both GPIOA0
> or GPIOB0 as interrupts at the same time.
>
> It looks like this: http://pastebin.com/raw/cs2WiNKZ
> You can directly check section 12.2.5 of the stm32f429 reference manual:
> http://www2.st.com/resource/en/reference_manual/dm00031020.pdf

Nice ASCII art, include that into the commit message :)

> For example, we can imagine a board where gpioa0 is used SD's card detect,
> and gpiob0 used to control a led.
>
> If we set the mux at request time, gpiob0 request may overwrite the mux
> configuration set by gpioa0, whereas it is not used as interrupt.
>
> That is the reason why I did it in .to_irq().

Well now I am even *more* convinced that this should not happen in
.to_irq(). .to_irq() should not do *anything*.

This needs to happen as part of the irqchip setting up the actual
interrupt.

And it seems the problem is that this driver does *not* define its
own irqchip, but it *should*.

What you want to do is implement an hierarchical irqdomain in your
irqchip, which is what other drivers of this type are doing, see:
drivers/gpio/gpio-xgene-sb.c
irq_domain_create_hierarchy() is your friend.

>> It should *also* be done in the set-up path for the irqchip
>> side, if that line is used without any interaction with the
>> gpio side of things.
>
> Actually, a line is either used by a GPIO, (exclusive) or another purpose.
> In the case of a GPIO line, I think we should always go through the gpio.

This is a textbook example of a hierarchichal irq domain I think.

>> The point is that each IRQ that ever get used
>> has a 1-to-1 relationship to a certain GPIO line, and if that
>> relationship cannot be resolved from the irqchip side,
>> something is wrong. The irqchip needs to enable the
>> GPIO line it is backing to recieve interrupts without any
>> requirements that .to_irq() have been called first.
>
> Actually, this is not a 1:1 relationship, as for example, exti0 can be connected
> to either gpioa0, or gpiob0,..., or gpiok0.

That is what hierarchical irqdomain is for.

You should expose an irqchip from the gpio driver, that give you
unique irq translations for *all* GPIO lines.

Then, at runtime, if the hierarchical irqdomain cannot map
(i.e. mux) the interrupt onto one of the available lines,
you will get an error.

Yours,
Linus Walleij

Re: [PATCH v2 6/9] pinctrl: Add IRQ support to STM32 gpios

[Maxime Coquelin]

2016-04-30 13:32 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Apr 29, 2016 at 1:19 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>> It looks like this: http://pastebin.com/raw/cs2WiNKZ
>> You can directly check section 12.2.5 of the stm32f429 reference manual:
>> http://www2.st.com/resource/en/reference_manual/dm00031020.pdf
>
> Nice ASCII art, include that into the commit message :)

I will! :)

>
>> For example, we can imagine a board where gpioa0 is used SD's card detect,
>> and gpiob0 used to control a led.
>>
>> If we set the mux at request time, gpiob0 request may overwrite the mux
>> configuration set by gpioa0, whereas it is not used as interrupt.
>>
>> That is the reason why I did it in .to_irq().
>
> Well now I am even *more* convinced that this should not happen in
> .to_irq(). .to_irq() should not do *anything*.
>
> This needs to happen as part of the irqchip setting up the actual
> interrupt.
>
> And it seems the problem is that this driver does *not* define its
> own irqchip, but it *should*.
>
> What you want to do is implement an hierarchical irqdomain in your
> irqchip, which is what other drivers of this type are doing, see:
> drivers/gpio/gpio-xgene-sb.c
> irq_domain_create_hierarchy() is your friend.
>
>>> It should *also* be done in the set-up path for the irqchip
>>> side, if that line is used without any interaction with the
>>> gpio side of things.
>>
>> Actually, a line is either used by a GPIO, (exclusive) or another purpose.
>> In the case of a GPIO line, I think we should always go through the gpio.
>
> This is a textbook example of a hierarchichal irq domain I think.
>
>>> The point is that each IRQ that ever get used
>>> has a 1-to-1 relationship to a certain GPIO line, and if that
>>> relationship cannot be resolved from the irqchip side,
>>> something is wrong. The irqchip needs to enable the
>>> GPIO line it is backing to recieve interrupts without any
>>> requirements that .to_irq() have been called first.
>>
>> Actually, this is not a 1:1 relationship, as for example, exti0 can be connected
>> to either gpioa0, or gpiob0,..., or gpiok0.
>
> That is what hierarchical irqdomain is for.
>
> You should expose an irqchip from the gpio driver, that give you
> unique irq translations for *all* GPIO lines.
>
> Then, at runtime, if the hierarchical irqdomain cannot map
> (i.e. mux) the interrupt onto one of the available lines,
> you will get an error.

Thanks a lot for this valuable feedback.
I will have a look at hierachical irq domains, and hope to come back this week
with an implementation.

Regards,
Maxime