[PATCH v6 0/3] Enable X-Gene standby GPIO as interrupt controller

[PATCH v6 0/3] Enable X-Gene standby GPIO as interrupt controller

[Quan Nguyen]

V6 Changes:
        - Remove redundant irq_ack and irq_shutdown entries
V5 Changes:
        - Add optional properties to describe HW specifics
        - Use irq_create_fwspec_mapping() instead of low level functions
        - Move gpiochip_lock_as_irq() to activate method
        - Remove redundant checks
        - Add optional HW properties description to DT binding document
        - Update "interrupts" property description in DT binding document.
V4 Changes:
        - Convert to hierarchical irq domain
V3 Changes:
        - Picking up from version 2 of "Y Vo <yvo@apm.com>"
        - Get HW resource information from DT
        - Avoid keeping parent IRQ controller
V2 Changes:
        - Support X-Gene standby GPIO as an interrupt controller.

Quan Nguyen (3):
  gpio: xgene: Enable X-Gene standby GPIO as interrupt controller
  Documentation: gpio: Update description for X-Gene standby GPIO
    controller DTS binding
  arm64: dts: Update X-Gene standby GPIO controller DTS entries

 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |  47 +++-
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   3 +
 drivers/gpio/gpio-xgene-sb.c                       | 265 ++++++++++++++++++---
 3 files changed, 276 insertions(+), 39 deletions(-)

-- 
1.7.12.4

[PATCH v6 1/3] gpio: xgene: Enable X-Gene standby GPIO as interrupt controller

[Quan Nguyen]

Enable X-Gene standby GPIO controller as interrupt controller to provide
its own resources. This avoids ambiguity where GIC interrupt resource is
use as X-Gene standby GPIO interrupt resource in user driver.

Signed-off-by: Y Vo <yvo@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
 drivers/gpio/gpio-xgene-sb.c | 265 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 233 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index 282004d..b993b86 100644
--- a/drivers/gpio/gpio-xgene-sb.c
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -2,8 +2,9 @@
  * AppliedMicro X-Gene SoC GPIO-Standby Driver
  *
  * Copyright (c) 2014, Applied Micro Circuits Corporation
- * Author: 	Tin Huynh <tnhuynh@apm.com>.
- * 		Y Vo <yvo@apm.com>.
+ * Author:	Tin Huynh <tnhuynh@apm.com>.
+ *		Y Vo <yvo@apm.com>.
+ *		Quan Nguyen <qnguyen@apm.com>.
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -28,9 +29,14 @@
 
 #include "gpiolib.h"
 
-#define XGENE_MAX_GPIO_DS		22
-#define XGENE_MAX_GPIO_DS_IRQ		6
+/* Common property names */
+#define XGENE_NIRQ_PROPERTY		"apm,nr-irqs"
+#define XGENE_NGPIO_PROPERTY		"apm,nr-gpios"
+#define XGENE_IRQ_START_PROPERTY	"apm,irq-start"
 
+#define XGENE_DFLT_MAX_NGPIO		22
+#define XGENE_DFLT_MAX_NIRQ		6
+#define XGENE_DFLT_IRQ_START_PIN	8
 #define GPIO_MASK(x)			(1U << ((x) % 32))
 
 #define MPA_GPIO_INT_LVL		0x0290
@@ -39,19 +45,32 @@
 #define MPA_GPIO_IN_ADDR 		0x02a4
 #define MPA_GPIO_SEL_LO 		0x0294
 
+#define GPIO_INT_LEVEL_H	0x000001
+#define GPIO_INT_LEVEL_L	0x000000
+
 /**
  * struct xgene_gpio_sb - GPIO-Standby private data structure.
  * @gc:				memory-mapped GPIO controllers.
- * @irq:			Mapping GPIO pins and interrupt number
- * nirq:			Number of GPIO pins that supports interrupt
+ * @regs:			GPIO register base offset
+ * @irq_domain:			GPIO interrupt domain
+ * @irq_start:			GPIO pin that start support interrupt
+ * @nirq:			Number of GPIO pins that supports interrupt
+ * @parent_irq_base:		Start parent HWIRQ
  */
 struct xgene_gpio_sb {
 	struct gpio_chip	gc;
-	u32 *irq;
-	u32 nirq;
+	void __iomem		*regs;
+	struct irq_domain	*irq_domain;
+	u16			irq_start;
+	u16			nirq;
+	u16			parent_irq_base;
 };
 
-static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio, int val)
+#define HWIRQ_TO_GPIO(priv, hwirq) ((hwirq) + (priv)->irq_start)
+#define GPIO_TO_HWIRQ(priv, gpio) ((gpio) - (priv)->irq_start)
+
+static void xgene_gpio_set_bit(struct gpio_chip *gc,
+				void __iomem *reg, u32 gpio, int val)
 {
 	u32 data;
 
@@ -63,23 +82,170 @@ static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio
 	gc->write_reg(reg, data);
 }
 
-static int apm_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
+static int xgene_gpio_sb_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+	int gpio = HWIRQ_TO_GPIO(priv, d->hwirq);
+	int lvl_type = GPIO_INT_LEVEL_H;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		lvl_type = GPIO_INT_LEVEL_H;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		lvl_type = GPIO_INT_LEVEL_L;
+		break;
+	default:
+		break;
+	}
+
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+			gpio * 2, 1);
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL,
+			d->hwirq, lvl_type);
+
+	/* Propagate IRQ type setting to parent */
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		return irq_chip_set_type_parent(d, IRQ_TYPE_EDGE_RISING);
+	else
+		return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
+}
+
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+	.name           = "sbgpio",
+	.irq_eoi	= irq_chip_eoi_parent,
+	.irq_mask       = irq_chip_mask_parent,
+	.irq_unmask     = irq_chip_unmask_parent,
+	.irq_set_type   = xgene_gpio_sb_irq_set_type,
+};
+
+static int xgene_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 {
 	struct xgene_gpio_sb *priv = gpiochip_get_data(gc);
+	struct irq_fwspec fwspec;
+
+	if ((gpio < priv->irq_start) ||
+			(gpio > HWIRQ_TO_GPIO(priv, priv->nirq)))
+		return -ENXIO;
+
+	if (gc->parent->of_node)
+		fwspec.fwnode = of_node_to_fwnode(gc->parent->of_node);
+	else
+		fwspec.fwnode = gc->parent->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = GPIO_TO_HWIRQ(priv, gpio);
+	fwspec.param[1] = IRQ_TYPE_NONE;
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+static void xgene_gpio_sb_domain_activate(struct irq_domain *d,
+		struct irq_data *irq_data)
+{
+	struct xgene_gpio_sb *priv = d->host_data;
+	u32 gpio = HWIRQ_TO_GPIO(priv, irq_data->hwirq);
+
+	if (gpiochip_lock_as_irq(&priv->gc, gpio)) {
+		dev_err(priv->gc.parent,
+		"Unable to configure XGene GPIO standby pin %d as IRQ\n",
+				gpio);
+		return;
+	}
+
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+			gpio * 2, 1);
+}
+
+static void xgene_gpio_sb_domain_deactivate(struct irq_domain *d,
+		struct irq_data *irq_data)
+{
+	struct xgene_gpio_sb *priv = d->host_data;
+	u32 gpio = HWIRQ_TO_GPIO(priv, irq_data->hwirq);
+
+	gpiochip_unlock_as_irq(&priv->gc, gpio);
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+			gpio * 2, 0);
+}
+
+static int xgene_gpio_sb_domain_translate(struct irq_domain *d,
+		struct irq_fwspec *fwspec,
+		unsigned long *hwirq,
+		unsigned int *type)
+{
+	struct xgene_gpio_sb *priv = d->host_data;
 
-	if (priv->irq[gpio])
-		return priv->irq[gpio];
+	if ((fwspec->param_count != 2) ||
+		(fwspec->param[0] >= priv->nirq))
+		return -EINVAL;
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1];
+	return 0;
+}
+
+static int xgene_gpio_sb_domain_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	struct xgene_gpio_sb *priv = domain->host_data;
+	irq_hw_number_t hwirq;
+	unsigned int i;
+
+	hwirq = fwspec->param[0];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+				&xgene_gpio_sb_irq_chip, priv);
+
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	if (is_of_node(parent_fwspec.fwnode)) {
+		parent_fwspec.param_count = 3;
+		parent_fwspec.param[0] = 0;/* SPI */
+		/* Skip SGIs and PPIs*/
+		parent_fwspec.param[1] = hwirq + priv->parent_irq_base - 32;
+		parent_fwspec.param[2] = fwspec->param[1];
+	} else if (is_fwnode_irqchip(parent_fwspec.fwnode)) {
+		parent_fwspec.param_count = 2;
+		parent_fwspec.param[0] = hwirq + priv->parent_irq_base;
+		parent_fwspec.param[1] = fwspec->param[1];
+	} else
+		return -EINVAL;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+			&parent_fwspec);
+}
+
+static void xgene_gpio_sb_domain_free(struct irq_domain *domain,
+		unsigned int virq,
+		unsigned int nr_irqs)
+{
+	struct irq_data *d;
+	unsigned int i;
 
-	return -ENXIO;
+	for (i = 0; i < nr_irqs; i++) {
+		d = irq_domain_get_irq_data(domain, virq + i);
+		irq_domain_reset_irq_data(d);
+	}
 }
 
+static const struct irq_domain_ops xgene_gpio_sb_domain_ops = {
+	.translate      = xgene_gpio_sb_domain_translate,
+	.alloc          = xgene_gpio_sb_domain_alloc,
+	.free           = xgene_gpio_sb_domain_free,
+	.activate	= xgene_gpio_sb_domain_activate,
+	.deactivate	= xgene_gpio_sb_domain_deactivate,
+};
+
 static int xgene_gpio_sb_probe(struct platform_device *pdev)
 {
 	struct xgene_gpio_sb *priv;
-	u32 ret, i;
-	u32 default_lines[] = {0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D};
+	u32 ret;
 	struct resource *res;
 	void __iomem *regs;
+	struct irq_domain *parent_domain = NULL;
+	struct fwnode_handle *fwnode;
+	u32 val32;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -90,6 +256,18 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
+	priv->regs = regs;
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret > 0) {
+		priv->parent_irq_base = irq_get_irq_data(ret)->hwirq;
+		parent_domain = irq_get_irq_data(ret)->domain;
+	}
+	if (!parent_domain) {
+		dev_err(&pdev->dev, "unable to obtain parent domain\n");
+		return -ENODEV;
+	}
+
 	ret = bgpio_init(&priv->gc, &pdev->dev, 4,
 			regs + MPA_GPIO_IN_ADDR,
 			regs + MPA_GPIO_OUT_ADDR, NULL,
@@ -97,30 +275,51 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
         if (ret)
                 return ret;
 
-	priv->gc.to_irq = apm_gpio_sb_to_irq;
-	priv->gc.ngpio = XGENE_MAX_GPIO_DS;
+	priv->gc.to_irq = xgene_gpio_sb_to_irq;
 
-	priv->nirq = XGENE_MAX_GPIO_DS_IRQ;
+	/* Retrieve start irq pin, use default if property not found */
+	priv->irq_start = XGENE_DFLT_IRQ_START_PIN;
+	if (!device_property_read_u32(&pdev->dev,
+					XGENE_IRQ_START_PROPERTY, &val32))
+		priv->irq_start = val32;
 
-	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
-				   GFP_KERNEL);
-	if (!priv->irq)
-		return -ENOMEM;
+	/* Retrieve number irqs, use default if property not found */
+	priv->nirq = XGENE_DFLT_MAX_NIRQ;
+	if (!device_property_read_u32(&pdev->dev, XGENE_NIRQ_PROPERTY, &val32))
+		priv->nirq = val32;
 
-	for (i = 0; i < priv->nirq; i++) {
-		priv->irq[default_lines[i]] = platform_get_irq(pdev, i);
-		xgene_gpio_set_bit(&priv->gc, regs + MPA_GPIO_SEL_LO,
-                                   default_lines[i] * 2, 1);
-		xgene_gpio_set_bit(&priv->gc, regs + MPA_GPIO_INT_LVL, i, 1);
-	}
+	/* Retrieve number gpio, use default if property not found */
+	priv->gc.ngpio = XGENE_DFLT_MAX_NGPIO;
+	if (!device_property_read_u32(&pdev->dev, XGENE_NGPIO_PROPERTY, &val32))
+		priv->gc.ngpio = val32;
+
+	dev_info(&pdev->dev, "Support %d gpios, %d irqs start from pin %d\n",
+			priv->gc.ngpio, priv->nirq, priv->irq_start);
 
 	platform_set_drvdata(pdev, priv);
 
-	ret = gpiochip_add_data(&priv->gc, priv);
-	if (ret)
-		dev_err(&pdev->dev, "failed to register X-Gene GPIO Standby driver\n");
+	if (pdev->dev.of_node)
+		fwnode = of_node_to_fwnode(pdev->dev.of_node);
 	else
-		dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
+		fwnode = pdev->dev.fwnode;
+
+	priv->irq_domain = irq_domain_create_hierarchy(parent_domain,
+					0, priv->nirq, fwnode,
+					&xgene_gpio_sb_domain_ops, priv);
+	if (!priv->irq_domain)
+		return -ENODEV;
+
+	priv->gc.irqdomain = priv->irq_domain;
+
+	ret = gpiochip_add_data(&priv->gc, priv);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register X-Gene GPIO Standby driver\n");
+		irq_domain_remove(priv->irq_domain);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
 
 	if (priv->nirq > 0) {
 		/* Register interrupt handlers for gpio signaled acpi events */
@@ -138,6 +337,8 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev)
 		acpi_gpiochip_free_interrupts(&priv->gc);
 	}
 
+	irq_domain_remove(priv->irq_domain);
+
 	gpiochip_remove(&priv->gc);
 	return 0;
 }
-- 
1.7.12.4

Re: [PATCH v6 1/3] gpio: xgene: Enable X-Gene standby GPIO as interrupt controller

[Marc Zyngier]

On 15/02/16 04:57, Quan Nguyen wrote:
> Enable X-Gene standby GPIO controller as interrupt controller to provide
> its own resources. This avoids ambiguity where GIC interrupt resource is
> use as X-Gene standby GPIO interrupt resource in user driver.
> 
> Signed-off-by: Y Vo <yvo@apm.com>
> Signed-off-by: Quan Nguyen <qnguyen@apm.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

[PATCH v6 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Quan Nguyen]

Update description for X-Gene standby GPIO controller DTS binding to
support GPIO line configuration as input, output or external IRQ pin.

Signed-off-by: Y Vo <yvo@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     | 47 ++++++++++++++++++----
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
index dae1300..7b8b4cb 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
@@ -1,10 +1,20 @@
 APM X-Gene Standby GPIO controller bindings
 
-This is a gpio controller in the standby domain.
-
-There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15,
-only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping
-is currently 1-to-1 on interrupts 0x28 thru 0x2d.
+This is a gpio controller in the standby domain. It also supports interrupt in
+some particular pins which are sourced to its parent interrupt controller
+as diagram below:
+                           +-----------------+
+                           | X-Gene standby  |
+                           | GPIO controller +--------- GPIO_0
++------------+             |                 | ...
+| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
+| controller |  EXT_INT_0  |                 | ...
+| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
+|            |  ...        |                 |
+|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
+|            +-------------+                 | ...
+|            |             |                 +--------- GPIO_MAX
++------------+             +-----------------+
 
 Required properties:
 - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
@@ -15,10 +25,18 @@ Required properties:
 		0 = active high
 		1 = active low
 - gpio-controller: Marks the device node as a GPIO controller.
-- interrupts: Shall contain exactly 6 interrupts.
+- interrupts: The EXT_INT_0 parent interrupt resource must be listed first.
+- interrupt-parent: Phandle of the parent interrupt controller.
+- interrupt-cells: Should be two.
+       - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N.
+       - second cell is used to specify flags.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- apm,nr-gpios: Optional, specify number of gpios pin.
+- apm,nr-irqs: Optional, specify number of interrupt pins.
+- apm,irq-start: Optional, specify lowest gpio pin support interrupt.
 
 Example:
-	sbgpio: sbgpio@17001000 {
+	sbgpio: gpio@17001000{
 		compatible = "apm,xgene-gpio-sb";
 		reg = <0x0 0x17001000 0x0 0x400>;
 		#gpio-cells = <2>;
@@ -29,4 +47,19 @@ Example:
 				<0x0 0x2b 0x1>,
 				<0x0 0x2c 0x1>,
 				<0x0 0x2d 0x1>;
+		interrupt-parent = <&gic>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		apm,nr-gpios = <22>;
+		apm,nr-irqs = <6>;
+		apm,irq-start = <8>;
+	};
+
+	testuser {
+		compatible = "example,testuser";
+		/* Use the GPIO_13/EXT_INT_5 line as an active high triggered
+		 * level interrupt
+		 */
+		interrupts = <5 4>;
+		interrupt-parent = <&sbgpio>;
 	};
-- 
1.7.12.4

Re: [PATCH v6 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Marc Zyngier]

On 15/02/16 04:57, Quan Nguyen wrote:
> Update description for X-Gene standby GPIO controller DTS binding to
> support GPIO line configuration as input, output or external IRQ pin.
> 
> Signed-off-by: Y Vo <yvo@apm.com>
> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
> ---
>  .../devicetree/bindings/gpio/gpio-xgene-sb.txt     | 47 ++++++++++++++++++----
>  1 file changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
> index dae1300..7b8b4cb 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
> @@ -1,10 +1,20 @@
>  APM X-Gene Standby GPIO controller bindings
>  
> -This is a gpio controller in the standby domain.
> -
> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15,
> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping
> -is currently 1-to-1 on interrupts 0x28 thru 0x2d.
> +This is a gpio controller in the standby domain. It also supports interrupt in
> +some particular pins which are sourced to its parent interrupt controller
> +as diagram below:
> +                           +-----------------+
> +                           | X-Gene standby  |
> +                           | GPIO controller +--------- GPIO_0
> ++------------+             |                 | ...
> +| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
> +| controller |  EXT_INT_0  |                 | ...
> +| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
> +|            |  ...        |                 |
> +|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
> +|            +-------------+                 | ...
> +|            |             |                 +--------- GPIO_MAX
> ++------------+             +-----------------+
>  
>  Required properties:
>  - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
> @@ -15,10 +25,18 @@ Required properties:
>  		0 = active high
>  		1 = active low
>  - gpio-controller: Marks the device node as a GPIO controller.
> -- interrupts: Shall contain exactly 6 interrupts.
> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first.
> +- interrupt-parent: Phandle of the parent interrupt controller.
> +- interrupt-cells: Should be two.
> +       - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N.
> +       - second cell is used to specify flags.
> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- apm,nr-gpios: Optional, specify number of gpios pin.
> +- apm,nr-irqs: Optional, specify number of interrupt pins.
> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt.

This is quite ambiguous. Is that relative to the GIC? Assuming this is
the case, you should then document it, specify the type of interrupt
(SPI?), and whether this is 0- or 32-based (the code seems to indicate
that is is 0-based).

>  
>  Example:
> -	sbgpio: sbgpio@17001000 {
> +	sbgpio: gpio@17001000{
>  		compatible = "apm,xgene-gpio-sb";
>  		reg = <0x0 0x17001000 0x0 0x400>;
>  		#gpio-cells = <2>;
> @@ -29,4 +47,19 @@ Example:
>  				<0x0 0x2b 0x1>,
>  				<0x0 0x2c 0x1>,
>  				<0x0 0x2d 0x1>;
> +		interrupt-parent = <&gic>;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		apm,nr-gpios = <22>;
> +		apm,nr-irqs = <6>;
> +		apm,irq-start = <8>;
> +	};
> +
> +	testuser {
> +		compatible = "example,testuser";
> +		/* Use the GPIO_13/EXT_INT_5 line as an active high triggered
> +		 * level interrupt
> +		 */
> +		interrupts = <5 4>;
> +		interrupt-parent = <&sbgpio>;
>  	};
> 

Thanks,

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

Re: [PATCH v6 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Quan Nguyen]

On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 15/02/16 04:57, Quan Nguyen wrote:
>> Update description for X-Gene standby GPIO controller DTS binding to
>> support GPIO line configuration as input, output or external IRQ pin.
>>
>> Signed-off-by: Y Vo <yvo@apm.com>
>> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
>> ---
>>  .../devicetree/bindings/gpio/gpio-xgene-sb.txt     | 47 ++++++++++++++++++----
>>  1 file changed, 40 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>> index dae1300..7b8b4cb 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>> @@ -1,10 +1,20 @@
>>  APM X-Gene Standby GPIO controller bindings
>>
>> -This is a gpio controller in the standby domain.
>> -
>> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15,
>> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping
>> -is currently 1-to-1 on interrupts 0x28 thru 0x2d.
>> +This is a gpio controller in the standby domain. It also supports interrupt in
>> +some particular pins which are sourced to its parent interrupt controller
>> +as diagram below:
>> +                           +-----------------+
>> +                           | X-Gene standby  |
>> +                           | GPIO controller +--------- GPIO_0
>> ++------------+             |                 | ...
>> +| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
>> +| controller |  EXT_INT_0  |                 | ...
>> +| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
>> +|            |  ...        |                 |
>> +|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
>> +|            +-------------+                 | ...
>> +|            |             |                 +--------- GPIO_MAX
>> ++------------+             +-----------------+
>>
>>  Required properties:
>>  - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
>> @@ -15,10 +25,18 @@ Required properties:
>>               0 = active high
>>               1 = active low
>>  - gpio-controller: Marks the device node as a GPIO controller.
>> -- interrupts: Shall contain exactly 6 interrupts.
>> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first.
>> +- interrupt-parent: Phandle of the parent interrupt controller.
>> +- interrupt-cells: Should be two.
>> +       - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N.
>> +       - second cell is used to specify flags.
>> +- interrupt-controller: Marks the device node as an interrupt controller.
>> +- apm,nr-gpios: Optional, specify number of gpios pin.
>> +- apm,nr-irqs: Optional, specify number of interrupt pins.
>> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt.
>
> This is quite ambiguous. Is that relative to the GIC? Assuming this is
> the case, you should then document it, specify the type of interrupt
> (SPI?), and whether this is 0- or 32-based (the code seems to indicate
> that is is 0-based).
>

Hi Marc, Let me explain by diagram below.
As you can also see in diagram above, the lowest gpio pin that
supports interrupt is GPIO_8 (EXT_INT_0),
and hence, apm,irq-start=<8> as default.

                                           +-----------------+
                                           | X-Gene standby  |
                                           | GPIO controller +--------- GPIO_0
+------------+                             |                 | ...
| Parent IRQ |                             |
+--------- GPIO_[apm,irq-start]/EXT_INT_0
| controller |  EXT_INT_0                  |                 | ...
| (GICv2)    +-----------------------------+
+--------- GPIO_[apm,nr-irqs + apm,irq-start - 1]/EXT_INT_[apm,nr-irqs
- 1]
|            |  ...                        |                 |
|            |  EXT_INT_[apm,nr-irqs - 1]  |
+--------- GPIO_[apm,nr-irqs + apm,irq-start]
|            +-----------------------------+                 | ...
|            |                             |                 +--------- GPIO_MAX
+------------+                             +-----------------+

To fix this ambiguity, I'm thinking to change it to:
"apm,irq-start: Optional, specify index of first gpio pin
corresponding to EXT_INT_0, default is 8."

Thanks,
-- Quan Nguyen

Re: [PATCH v6 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Marc Zyngier]

On 15/02/16 15:31, Quan Nguyen wrote:
> On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 15/02/16 04:57, Quan Nguyen wrote:
>>> Update description for X-Gene standby GPIO controller DTS binding to
>>> support GPIO line configuration as input, output or external IRQ pin.
>>>
>>> Signed-off-by: Y Vo <yvo@apm.com>
>>> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
>>> ---
>>>  .../devicetree/bindings/gpio/gpio-xgene-sb.txt     | 47 ++++++++++++++++++----
>>>  1 file changed, 40 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>> index dae1300..7b8b4cb 100644
>>> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>> @@ -1,10 +1,20 @@
>>>  APM X-Gene Standby GPIO controller bindings
>>>
>>> -This is a gpio controller in the standby domain.
>>> -
>>> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15,
>>> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping
>>> -is currently 1-to-1 on interrupts 0x28 thru 0x2d.
>>> +This is a gpio controller in the standby domain. It also supports interrupt in
>>> +some particular pins which are sourced to its parent interrupt controller
>>> +as diagram below:
>>> +                           +-----------------+
>>> +                           | X-Gene standby  |
>>> +                           | GPIO controller +--------- GPIO_0
>>> ++------------+             |                 | ...
>>> +| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
>>> +| controller |  EXT_INT_0  |                 | ...
>>> +| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
>>> +|            |  ...        |                 |
>>> +|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
>>> +|            +-------------+                 | ...
>>> +|            |             |                 +--------- GPIO_MAX
>>> ++------------+             +-----------------+
>>>
>>>  Required properties:
>>>  - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
>>> @@ -15,10 +25,18 @@ Required properties:
>>>               0 = active high
>>>               1 = active low
>>>  - gpio-controller: Marks the device node as a GPIO controller.
>>> -- interrupts: Shall contain exactly 6 interrupts.
>>> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first.
>>> +- interrupt-parent: Phandle of the parent interrupt controller.
>>> +- interrupt-cells: Should be two.
>>> +       - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N.
>>> +       - second cell is used to specify flags.
>>> +- interrupt-controller: Marks the device node as an interrupt controller.
>>> +- apm,nr-gpios: Optional, specify number of gpios pin.
>>> +- apm,nr-irqs: Optional, specify number of interrupt pins.
>>> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt.
>>
>> This is quite ambiguous. Is that relative to the GIC? Assuming this is
>> the case, you should then document it, specify the type of interrupt
>> (SPI?), and whether this is 0- or 32-based (the code seems to indicate
>> that is is 0-based).
>>
> 
> Hi Marc, Let me explain by diagram below.
> As you can also see in diagram above, the lowest gpio pin that
> supports interrupt is GPIO_8 (EXT_INT_0),
> and hence, apm,irq-start=<8> as default.
> 
>                                            +-----------------+
>                                            | X-Gene standby  |
>                                            | GPIO controller +--------- GPIO_0
> +------------+                             |                 | ...
> | Parent IRQ |                             |
> +--------- GPIO_[apm,irq-start]/EXT_INT_0
> | controller |  EXT_INT_0                  |                 | ...
> | (GICv2)    +-----------------------------+
> +--------- GPIO_[apm,nr-irqs + apm,irq-start - 1]/EXT_INT_[apm,nr-irqs
> - 1]
> |            |  ...                        |                 |
> |            |  EXT_INT_[apm,nr-irqs - 1]  |
> +--------- GPIO_[apm,nr-irqs + apm,irq-start]
> |            +-----------------------------+                 | ...
> |            |                             |                 +--------- GPIO_MAX
> +------------+                             +-----------------+
> 
> To fix this ambiguity, I'm thinking to change it to:
> "apm,irq-start: Optional, specify index of first gpio pin
> corresponding to EXT_INT_0, default is 8."

I think you are missing the point I'm trying to make. There are two ways
to refer to an SPI: either by its absolute number (INT32 for example) or
by its relative number (SPI0). SPI0 and INT32 are the same interrupt.
Just with a different base.

My question is: which base are you using? By looking at the code, I
think you're using the absolute version (INTx). And as your controller
seems to be very GIC specific, you should add a pointer to the GIC
documentation.

Thanks,

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

Re: [PATCH v6 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Quan Nguyen]

On Mon, Feb 15, 2016 at 11:08 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 15/02/16 15:31, Quan Nguyen wrote:
>> On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 15/02/16 04:57, Quan Nguyen wrote:
>>>> Update description for X-Gene standby GPIO controller DTS binding to
>>>> support GPIO line configuration as input, output or external IRQ pin.
>>>>
>>>> Signed-off-by: Y Vo <yvo@apm.com>
>>>> Signed-off-by: Quan Nguyen <qnguyen@apm.com>
>>>> ---
>>>>  .../devicetree/bindings/gpio/gpio-xgene-sb.txt     | 47 ++++++++++++++++++----
>>>>  1 file changed, 40 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>>> index dae1300..7b8b4cb 100644
>>>> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
>>>> @@ -1,10 +1,20 @@
>>>>  APM X-Gene Standby GPIO controller bindings
>>>>
>>>> -This is a gpio controller in the standby domain.
>>>> -
>>>> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15,
>>>> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping
>>>> -is currently 1-to-1 on interrupts 0x28 thru 0x2d.
>>>> +This is a gpio controller in the standby domain. It also supports interrupt in
>>>> +some particular pins which are sourced to its parent interrupt controller
>>>> +as diagram below:
>>>> +                           +-----------------+
>>>> +                           | X-Gene standby  |
>>>> +                           | GPIO controller +--------- GPIO_0
>>>> ++------------+             |                 | ...
>>>> +| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
>>>> +| controller |  EXT_INT_0  |                 | ...
>>>> +| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
>>>> +|            |  ...        |                 |
>>>> +|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
>>>> +|            +-------------+                 | ...
>>>> +|            |             |                 +--------- GPIO_MAX
>>>> ++------------+             +-----------------+
>>>>
>>>>  Required properties:
>>>>  - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
>>>> @@ -15,10 +25,18 @@ Required properties:
>>>>               0 = active high
>>>>               1 = active low
>>>>  - gpio-controller: Marks the device node as a GPIO controller.
>>>> -- interrupts: Shall contain exactly 6 interrupts.
>>>> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first.
>>>> +- interrupt-parent: Phandle of the parent interrupt controller.
>>>> +- interrupt-cells: Should be two.
>>>> +       - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N.
>>>> +       - second cell is used to specify flags.
>>>> +- interrupt-controller: Marks the device node as an interrupt controller.
>>>> +- apm,nr-gpios: Optional, specify number of gpios pin.
>>>> +- apm,nr-irqs: Optional, specify number of interrupt pins.
>>>> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt.
>>>
>>> This is quite ambiguous. Is that relative to the GIC? Assuming this is
>>> the case, you should then document it, specify the type of interrupt
>>> (SPI?), and whether this is 0- or 32-based (the code seems to indicate
>>> that is is 0-based).
>>>
>>
>> Hi Marc, Let me explain by diagram below.
>> As you can also see in diagram above, the lowest gpio pin that
>> supports interrupt is GPIO_8 (EXT_INT_0),
>> and hence, apm,irq-start=<8> as default.
>>
>>                                            +-----------------+
>>                                            | X-Gene standby  |
>>                                            | GPIO controller +--------- GPIO_0
>> +------------+                             |                 | ...
>> | Parent IRQ |                             |
>> +--------- GPIO_[apm,irq-start]/EXT_INT_0
>> | controller |  EXT_INT_0                  |                 | ...
>> | (GICv2)    +-----------------------------+
>> +--------- GPIO_[apm,nr-irqs + apm,irq-start - 1]/EXT_INT_[apm,nr-irqs
>> - 1]
>> |            |  ...                        |                 |
>> |            |  EXT_INT_[apm,nr-irqs - 1]  |
>> +--------- GPIO_[apm,nr-irqs + apm,irq-start]
>> |            +-----------------------------+                 | ...
>> |            |                             |                 +--------- GPIO_MAX
>> +------------+                             +-----------------+
>>
>> To fix this ambiguity, I'm thinking to change it to:
>> "apm,irq-start: Optional, specify index of first gpio pin
>> corresponding to EXT_INT_0, default is 8."
>
> I think you are missing the point I'm trying to make. There are two ways
> to refer to an SPI: either by its absolute number (INT32 for example) or
> by its relative number (SPI0). SPI0 and INT32 are the same interrupt.
> Just with a different base.
>
> My question is: which base are you using? By looking at the code, I
> think you're using the absolute version (INTx). And as your controller
> seems to be very GIC specific, you should add a pointer to the GIC
> documentation.
>

Sorry, Marc, I was misunderstood.

It is relative number, EXT_INT_0 corresponding with SPI40 and so on.
So, how about adding more specific description into the diagram as below ?

--- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
@@ -3,18 +3,18 @@ APM X-Gene Standby GPIO controller bindings
 This is a gpio controller in the standby domain. It also supports interrupt in
 some particular pins which are sourced to its parent interrupt controller
 as diagram below:
-                           +-----------------+
-                           | X-Gene standby  |
-                           | GPIO controller +--------- GPIO_0
-+------------+             |                 | ...
-| Parent IRQ |             |                 +--------- GPIO_8/EXT_INT_0
-| controller |  EXT_INT_0  |                 | ...
-| (GICv2)    +-------------+                 +--------- GPIO_[N+8]/EXT_INT_N
-|            |  ...        |                 |
-|            |  EXT_INT_N  |                 +--------- GPIO_[N+9]
-|            +-------------+                 | ...
-|            |             |                 +--------- GPIO_MAX
-+------------+             +-----------------+
+                            +-----------------+
+                            | X-Gene standby  |
+                            | GPIO controller +------ GPIO_0
++------------+              |                 | ...
+| Parent IRQ | EXT_INT_0    |                 +------ GPIO_8/EXT_INT_0
+| controller | (SPI40)      |                 | ...
+| (GICv2)    +--------------+                 +------ GPIO_[N+8]/EXT_INT_N
+|            |   ...        |                 |
+|            | EXT_INT_N    |                 +------ GPIO_[N+9]
+|            | (SPI[40 + N])|                 | ...
+|            +--------------+                 +------ GPIO_MAX
++------------+              +-----------------+

 Required properties:
 - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller


Thanks,
-- Quan Nguyen

[PATCH v6 3/3] arm64: dts: Update X-Gene standby GPIO controller DTS entries

[Quan Nguyen]

Update APM X-Gene standby GPIO controller DTS entries to enable it
as interrupt controller.

Signed-off-by: Y Vo <yvo@apm.com>
Signed-off-by: Quan Nguyen <qnguyen@apm.com>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index fe30f76..0f733da 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -883,6 +883,9 @@
 					<0x0 0x2b 0x1>,
 					<0x0 0x2c 0x1>,
 					<0x0 0x2d 0x1>;
+			interrupt-parent = <&gic>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
 		};
 
 		rtc: rtc@10510000 {
-- 
1.7.12.4