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

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

[Quan Nguyen]

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     |  39 ++-
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   3 +
 drivers/gpio/gpio-xgene-sb.c                       | 331 +++++++++++++++++----
 3 files changed, 312 insertions(+), 61 deletions(-)

-- 
1.7.12.4

[PATCH v4 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 | 331 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 276 insertions(+), 55 deletions(-)

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index 282004d..b703114 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
@@ -22,14 +23,20 @@
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 #include <linux/of_gpio.h>
+#include <linux/of_irq.h>
 #include <linux/gpio/driver.h>
 #include <linux/acpi.h>
 
 #include "gpiolib.h"
 
-#define XGENE_MAX_GPIO_DS		22
-#define XGENE_MAX_GPIO_DS_IRQ		6
+#define XGENE_MAX_NGPIO			22
+#define XGENE_MAX_NIRQ			6
+#define XGENE_IRQ_START_PIN		8
+#define SBGPIO_XGENE			((XGENE_IRQ_START_PIN << 24) | \
+					(XGENE_MAX_NIRQ << 16) | \
+					(XGENE_MAX_NGPIO << 8))
 
 #define GPIO_MASK(x)			(1U << ((x) % 32))
 
@@ -39,19 +46,30 @@
 #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
+ * flags:			GPIO per-instance flags assigned by the driver
  */
 struct xgene_gpio_sb {
 	struct gpio_chip	gc;
-	u32 *irq;
-	u32 nirq;
+	void __iomem		*regs;
+	struct irq_domain	*irq_domain;
+	u32			flags;
 };
 
-static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio, int val)
+#define IRQ_START_PIN(priv)	(((priv)->flags >> 24) & 0xff)
+#define NIRQ_MAX(priv)		(((priv)->flags >> 16) & 0xff)
+#define NGPIO_MAX(priv)		(((priv)->flags >>  8) & 0xff)
+#define START_PARENT_IRQ(priv)	((priv)->flags & 0xff)
+
+static void xgene_gpio_set_bit(struct gpio_chip *gc,
+				void __iomem *reg, u32 gpio, int val)
 {
 	u32 data;
 
@@ -63,23 +81,216 @@ 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 = d->hwirq + IRQ_START_PIN(priv);
+	int lvl_type;
+	int ret;
+
+	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:
+		return -EINVAL;
+	}
+
+	ret = gpiochip_lock_as_irq(&priv->gc, gpio);
+	if (ret) {
+		dev_err(priv->gc.parent,
+		"Unable to configure XGene GPIO standby pin %d as IRQ\n",
+				gpio);
+		return ret;
+	}
+
+	if ((gpio >= IRQ_START_PIN(priv)) &&
+			(d->hwirq < NIRQ_MAX(priv))) {
+		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 void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
+{
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
+}
+
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+	.name           = "sbgpio",
+	.irq_ack        = irq_chip_ack_parent,
+	.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,
+	.irq_shutdown   = xgene_gpio_sb_irq_shutdown,
+};
+
+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;
+	unsigned int virq;
+
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		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 - IRQ_START_PIN(priv);
+	fwspec.param[1] = IRQ_TYPE_NONE;
+	virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
+	if (!virq)
+		virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
+						NUMA_NO_NODE, &fwspec);
+	return virq;
+}
+
+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 = irq_data->hwirq + IRQ_START_PIN(priv);
 
-	if (priv->irq[gpio])
-		return priv->irq[gpio];
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		return;
 
-	return -ENXIO;
+	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 = irq_data->hwirq + IRQ_START_PIN(priv);
+
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		return;
+
+	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)
+{
+	if (fwspec->param_count != 2)
+		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 type = IRQ_TYPE_NONE;
+	unsigned int i;
+	u32 ret;
+
+	ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	hwirq = fwspec->param[0];
+	if ((hwirq >= NIRQ_MAX(priv)) ||
+			(hwirq + nr_irqs > NIRQ_MAX(priv)))
+		return -EINVAL;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+				&xgene_gpio_sb_irq_chip, priv);
+
+	if (is_of_node(domain->parent->fwnode)) {
+		parent_fwspec.fwnode = domain->parent->fwnode;
+		parent_fwspec.param_count = 3;
+		parent_fwspec.param[0] = 0;/* SPI */
+		/* Skip SGIs and PPIs*/
+		parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
+		parent_fwspec.param[2] = fwspec->param[1];
+	} else if (is_fwnode_irqchip(domain->parent->fwnode)) {
+		parent_fwspec.fwnode = domain->parent->fwnode;
+		parent_fwspec.param_count = 2;
+		parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
+		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;
+
+	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 const struct of_device_id xgene_gpio_sb_of_match[] = {
+	{.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
+	{},
+};
+MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
+	{"APMC0D15", SBGPIO_XGENE},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
+#endif
+
 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;
+	const struct of_device_id *of_id;
+	struct irq_domain *parent_domain = NULL;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
+	priv->regs = regs;
+
+	of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
+	if (of_id)
+		priv->flags = (uintptr_t)of_id->data;
+#ifdef CONFIG_ACPI
+	else {
+		const struct acpi_device_id *acpi_id;
+
+		acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
+				&pdev->dev);
+		if (acpi_id)
+			priv->flags = (uintptr_t)acpi_id->driver_data;
+	}
+#endif
+	ret = platform_get_irq(pdev, 0);
+	if (ret > 0) {
+		priv->flags &= ~0xff;
+		priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
+		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,36 +334,34 @@ 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->nirq = XGENE_MAX_GPIO_DS_IRQ;
+	priv->gc.to_irq = xgene_gpio_sb_to_irq;
+	priv->gc.ngpio = NGPIO_MAX(priv);
 
-	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
-				   GFP_KERNEL);
-	if (!priv->irq)
-		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
 
-	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);
-	}
+	priv->irq_domain = irq_domain_create_hierarchy(parent_domain,
+					0, NIRQ_MAX(priv),
+					of_node_to_fwnode(pdev->dev.of_node),
+					&xgene_gpio_sb_domain_ops, priv);
+	if (!priv->irq_domain)
+		return -ENODEV;
 
-	platform_set_drvdata(pdev, priv);
+	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");
-	else
-		dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
-
-	if (priv->nirq > 0) {
-		/* Register interrupt handlers for gpio signaled acpi events */
-		acpi_gpiochip_request_interrupts(&priv->gc);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register X-Gene GPIO Standby driver\n");
+		if (priv->irq_domain)
+			irq_domain_remove(priv->irq_domain);
+		return ret;
 	}
 
+	dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
+
+	/* Register interrupt handlers for gpio signaled acpi events */
+	acpi_gpiochip_request_interrupts(&priv->gc);
+
 	return ret;
 }
 
@@ -134,28 +369,14 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev)
 {
 	struct xgene_gpio_sb *priv = platform_get_drvdata(pdev);
 
-	if (priv->nirq > 0) {
-		acpi_gpiochip_free_interrupts(&priv->gc);
-	}
+	acpi_gpiochip_free_interrupts(&priv->gc);
+
+	irq_domain_remove(priv->irq_domain);
 
 	gpiochip_remove(&priv->gc);
 	return 0;
 }
 
-static const struct of_device_id xgene_gpio_sb_of_match[] = {
-	{.compatible = "apm,xgene-gpio-sb", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
-
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
-	{"APMC0D15", 0},
-	{},
-};
-MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
-#endif
-
 static struct platform_driver xgene_gpio_sb_driver = {
 	.driver = {
 		   .name = "xgene-gpio-sb",
-- 
1.7.12.4

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

[Marc Zyngier]

On 26/01/16 07:22, 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>
> ---
>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 276 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
> index 282004d..b703114 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
> @@ -22,14 +23,20 @@
>  #include <linux/module.h>
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/acpi.h>
>  
>  #include "gpiolib.h"
>  
> -#define XGENE_MAX_GPIO_DS		22
> -#define XGENE_MAX_GPIO_DS_IRQ		6
> +#define XGENE_MAX_NGPIO			22
> +#define XGENE_MAX_NIRQ			6
> +#define XGENE_IRQ_START_PIN		8
> +#define SBGPIO_XGENE			((XGENE_IRQ_START_PIN << 24) | \
> +					(XGENE_MAX_NIRQ << 16) | \
> +					(XGENE_MAX_NGPIO << 8))
>  
>  #define GPIO_MASK(x)			(1U << ((x) % 32))
>  
> @@ -39,19 +46,30 @@
>  #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
> + * flags:			GPIO per-instance flags assigned by the driver

nit: missing @ before "flags".

>   */
>  struct xgene_gpio_sb {
>  	struct gpio_chip	gc;
> -	u32 *irq;
> -	u32 nirq;
> +	void __iomem		*regs;
> +	struct irq_domain	*irq_domain;
> +	u32			flags;
>  };
>  
> -static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio, int val)
> +#define IRQ_START_PIN(priv)	(((priv)->flags >> 24) & 0xff)
> +#define NIRQ_MAX(priv)		(((priv)->flags >> 16) & 0xff)
> +#define NGPIO_MAX(priv)		(((priv)->flags >>  8) & 0xff)
> +#define START_PARENT_IRQ(priv)	((priv)->flags & 0xff)
> +

So flags is actually a set of fields, all 8bits, called irq_start, nirq,
ngpio, and parent_irq_base (or something along those lines).

You might as well make that explicit in your structure, as there is
hardly any point in hiding this information behind a bag of bits and a
set of obscure accessors...

> +static void xgene_gpio_set_bit(struct gpio_chip *gc,
> +				void __iomem *reg, u32 gpio, int val)
>  {
>  	u32 data;
>  
> @@ -63,23 +81,216 @@ 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 = d->hwirq + IRQ_START_PIN(priv);
> +	int lvl_type;
> +	int ret;
> +
> +	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:
> +		return -EINVAL;
> +	}
> +
> +	ret = gpiochip_lock_as_irq(&priv->gc, gpio);

This has no business whatsoever in set_type. This should be done either
when the GPIO is activated as an IRQ in the domain "activate" method, or
in the "startup" method of the irqchip.

> +	if (ret) {
> +		dev_err(priv->gc.parent,
> +		"Unable to configure XGene GPIO standby pin %d as IRQ\n",
> +				gpio);
> +		return ret;
> +	}
> +
> +	if ((gpio >= IRQ_START_PIN(priv)) &&
> +			(d->hwirq < NIRQ_MAX(priv))) {

How can we end-up here if your GPIO is not part that range? This should
be guaranteed by construction.

> +		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 void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
> +{
> +	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
> +
> +	gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
> +}
> +
> +static struct irq_chip xgene_gpio_sb_irq_chip = {
> +	.name           = "sbgpio",
> +	.irq_ack        = irq_chip_ack_parent,
> +	.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,
> +	.irq_shutdown   = xgene_gpio_sb_irq_shutdown,
> +};
> +
> +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;
> +	unsigned int virq;
> +
> +	if ((gpio < IRQ_START_PIN(priv)) ||
> +			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> +		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 - IRQ_START_PIN(priv);
> +	fwspec.param[1] = IRQ_TYPE_NONE;
> +	virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
> +	if (!virq)
> +		virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
> +						NUMA_NO_NODE, &fwspec);

You should not use these low-level functions directly. Use
irq_create_fwspec_mapping, which will do the right thing.

> +	return virq;
> +}
> +
> +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>  
> -	if (priv->irq[gpio])
> -		return priv->irq[gpio];
> +	if ((gpio < IRQ_START_PIN(priv)) ||
> +			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> +		return;

Again, how can this happen?

>  
> -	return -ENXIO;
> +	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
> +			gpio * 2, 1);

This seems to program the interrupt to be active on a low level. Why?
Isn't that what set_type is supposed to do?

> +}
> +
> +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 = irq_data->hwirq + IRQ_START_PIN(priv);

It really feels like you need a hwirq_to_gpio() accessor.

> +
> +	if ((gpio < IRQ_START_PIN(priv)) ||
> +			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> +		return;

Why do we need this?

> +
> +	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)
> +{
> +	if (fwspec->param_count != 2)
> +		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 type = IRQ_TYPE_NONE;
> +	unsigned int i;
> +	u32 ret;
> +
> +	ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;

How can this fail here?

> +
> +	hwirq = fwspec->param[0];
> +	if ((hwirq >= NIRQ_MAX(priv)) ||
> +			(hwirq + nr_irqs > NIRQ_MAX(priv)))
> +		return -EINVAL;

How can this happen?

> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +				&xgene_gpio_sb_irq_chip, priv);
> +
> +	if (is_of_node(domain->parent->fwnode)) {
> +		parent_fwspec.fwnode = domain->parent->fwnode;
> +		parent_fwspec.param_count = 3;
> +		parent_fwspec.param[0] = 0;/* SPI */
> +		/* Skip SGIs and PPIs*/
> +		parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
> +		parent_fwspec.param[2] = fwspec->param[1];
> +	} else if (is_fwnode_irqchip(domain->parent->fwnode)) {
> +		parent_fwspec.fwnode = domain->parent->fwnode;
> +		parent_fwspec.param_count = 2;
> +		parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
> +		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;
> +
> +	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 const struct of_device_id xgene_gpio_sb_of_match[] = {
> +	{.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
> +	{"APMC0D15", SBGPIO_XGENE},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
> +#endif
> +
>  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;
> +	const struct of_device_id *of_id;
> +	struct irq_domain *parent_domain = NULL;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>  	if (IS_ERR(regs))
>  		return PTR_ERR(regs);
>  
> +	priv->regs = regs;
> +
> +	of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
> +	if (of_id)
> +		priv->flags = (uintptr_t)of_id->data;

Wait. Everything is hardcoded? So why do we have to deal with looking
into that structure if nothing is actually parametrized?

> +#ifdef CONFIG_ACPI
> +	else {
> +		const struct acpi_device_id *acpi_id;
> +
> +		acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
> +				&pdev->dev);
> +		if (acpi_id)
> +			priv->flags = (uintptr_t)acpi_id->driver_data;
> +	}
> +#endif

nit: you can write this as

	if (of_id) {
		...
#ifdef ...
	} else {
		...
#endif
	}


Which preserves the Linux coding style.

> +	ret = platform_get_irq(pdev, 0);
> +	if (ret > 0) {
> +		priv->flags &= ~0xff;
> +		priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
> +		parent_domain = irq_get_irq_data(ret)->domain;
> +	}

This is rather ugly. You have the interrupt-parent property. Why don't
you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
do you have that the interrupts are going to be sorted in the DT? There
is no such garantee in the documentation.

> +	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,36 +334,34 @@ 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->nirq = XGENE_MAX_GPIO_DS_IRQ;
> +	priv->gc.to_irq = xgene_gpio_sb_to_irq;
> +	priv->gc.ngpio = NGPIO_MAX(priv);

So we do have duplicated information everywhere. Great.

>  
> -	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
> -				   GFP_KERNEL);
> -	if (!priv->irq)
> -		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
>  
> -	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);
> -	}
> +	priv->irq_domain = irq_domain_create_hierarchy(parent_domain,
> +					0, NIRQ_MAX(priv),
> +					of_node_to_fwnode(pdev->dev.of_node),
> +					&xgene_gpio_sb_domain_ops, priv);
> +	if (!priv->irq_domain)
> +		return -ENODEV;
>  
> -	platform_set_drvdata(pdev, priv);
> +	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");
> -	else
> -		dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
> -
> -	if (priv->nirq > 0) {
> -		/* Register interrupt handlers for gpio signaled acpi events */
> -		acpi_gpiochip_request_interrupts(&priv->gc);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to register X-Gene GPIO Standby driver\n");
> +		if (priv->irq_domain)

How can that not be true, given that you've tested irq_domain just above?

> +			irq_domain_remove(priv->irq_domain);
> +		return ret;
>  	}
>  
> +	dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
> +
> +	/* Register interrupt handlers for gpio signaled acpi events */
> +	acpi_gpiochip_request_interrupts(&priv->gc);
> +
>  	return ret;
>  }
>  
> @@ -134,28 +369,14 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev)
>  {
>  	struct xgene_gpio_sb *priv = platform_get_drvdata(pdev);
>  
> -	if (priv->nirq > 0) {
> -		acpi_gpiochip_free_interrupts(&priv->gc);
> -	}
> +	acpi_gpiochip_free_interrupts(&priv->gc);
> +
> +	irq_domain_remove(priv->irq_domain);
>  
>  	gpiochip_remove(&priv->gc);
>  	return 0;
>  }
>  
> -static const struct of_device_id xgene_gpio_sb_of_match[] = {
> -	{.compatible = "apm,xgene-gpio-sb", },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
> -
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
> -	{"APMC0D15", 0},
> -	{},
> -};
> -MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
> -#endif
> -
>  static struct platform_driver xgene_gpio_sb_driver = {
>  	.driver = {
>  		   .name = "xgene-gpio-sb",
> 

Thanks,

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

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

[Quan Nguyen]

On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 26/01/16 07:22, 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>
> > ---
> >  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 276 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
> > index 282004d..b703114 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
> > @@ -22,14 +23,20 @@
> >  #include <linux/module.h>
> >  #include <linux/io.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/of_gpio.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/acpi.h>
> >
> >  #include "gpiolib.h"
> >
> > -#define XGENE_MAX_GPIO_DS            22
> > -#define XGENE_MAX_GPIO_DS_IRQ                6
> > +#define XGENE_MAX_NGPIO                      22
> > +#define XGENE_MAX_NIRQ                       6
> > +#define XGENE_IRQ_START_PIN          8
> > +#define SBGPIO_XGENE                 ((XGENE_IRQ_START_PIN << 24) | \
> > +                                     (XGENE_MAX_NIRQ << 16) | \
> > +                                     (XGENE_MAX_NGPIO << 8))
> >
> >  #define GPIO_MASK(x)                 (1U << ((x) % 32))
> >
> > @@ -39,19 +46,30 @@
> >  #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
> > + * flags:                    GPIO per-instance flags assigned by the driver
>
> nit: missing @ before "flags".

let me fix it.

>
> >   */
> >  struct xgene_gpio_sb {
> >       struct gpio_chip        gc;
> > -     u32 *irq;
> > -     u32 nirq;
> > +     void __iomem            *regs;
> > +     struct irq_domain       *irq_domain;
> > +     u32                     flags;
> >  };
> >
> > -static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio, int val)
> > +#define IRQ_START_PIN(priv)  (((priv)->flags >> 24) & 0xff)
> > +#define NIRQ_MAX(priv)               (((priv)->flags >> 16) & 0xff)
> > +#define NGPIO_MAX(priv)              (((priv)->flags >>  8) & 0xff)
> > +#define START_PARENT_IRQ(priv)       ((priv)->flags & 0xff)
> > +
>
> So flags is actually a set of fields, all 8bits, called irq_start, nirq,
> ngpio, and parent_irq_base (or something along those lines).
>
> You might as well make that explicit in your structure, as there is
> hardly any point in hiding this information behind a bag of bits and a
> set of obscure accessors...

Yes, I agree. Let me remove those lines and make them explicitly in structure.
But I might remove ngpio in struct gpio_chip as it is redundant.

>
> > +static void xgene_gpio_set_bit(struct gpio_chip *gc,
> > +                             void __iomem *reg, u32 gpio, int val)
> >  {
> >       u32 data;
> >
> > @@ -63,23 +81,216 @@ 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 = d->hwirq + IRQ_START_PIN(priv);
> > +     int lvl_type;
> > +     int ret;
> > +
> > +     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:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = gpiochip_lock_as_irq(&priv->gc, gpio);
>
> This has no business whatsoever in set_type. This should be done either
> when the GPIO is activated as an IRQ in the domain "activate" method, or
> in the "startup" method of the irqchip.

The irq pin can do high/low level as well as edge rising/falling,
while its parent(GIC) can only be high level/edge rising. Hence, there
is need to configure the irq pin to indicate its parent irq chip when
there is "high" or "low" on the pin, very much like an invert as the
code below:
xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL, d->hwirq,
lvl_type);

>
> > +     if (ret) {
> > +             dev_err(priv->gc.parent,
> > +             "Unable to configure XGene GPIO standby pin %d as IRQ\n",
> > +                             gpio);
> > +             return ret;
> > +     }
> > +
> > +     if ((gpio >= IRQ_START_PIN(priv)) &&
> > +                     (d->hwirq < NIRQ_MAX(priv))) {
>
> How can we end-up here if your GPIO is not part that range? This should
> be guaranteed by construction.

I agree, let me remove it.

>
> > +             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 void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
> > +{
> > +     struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
> > +
> > +     gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
> > +}
> > +
> > +static struct irq_chip xgene_gpio_sb_irq_chip = {
> > +     .name           = "sbgpio",
> > +     .irq_ack        = irq_chip_ack_parent,
> > +     .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,
> > +     .irq_shutdown   = xgene_gpio_sb_irq_shutdown,
> > +};
> > +
> > +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;
> > +     unsigned int virq;
> > +
> > +     if ((gpio < IRQ_START_PIN(priv)) ||
> > +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> > +             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 - IRQ_START_PIN(priv);
> > +     fwspec.param[1] = IRQ_TYPE_NONE;
> > +     virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
> > +     if (!virq)
> > +             virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
> > +                                             NUMA_NO_NODE, &fwspec);
>
> You should not use these low-level functions directly. Use
> irq_create_fwspec_mapping, which will do the right thing.

Yes, agree, the code should be much better.

Let me change:

virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
if (!virq)
virq =  irq_domain_alloc_irqs(priv->irq_domain, 1, NUMA_NO_NODE, &fwspec);
return virq;

to:
return irq_create_fwspec_mapping(&fwspec);

>
> > +     return virq;
> > +}
> > +
> > +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 = irq_data->hwirq + IRQ_START_PIN(priv);
> >
> > -     if (priv->irq[gpio])
> > -             return priv->irq[gpio];
> > +     if ((gpio < IRQ_START_PIN(priv)) ||
> > +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> > +             return;
>
> Again, how can this happen?

let me remove this redundant code.

>
> >
> > -     return -ENXIO;
> > +     xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
> > +                     gpio * 2, 1);
>
> This seems to program the interrupt to be active on a low level. Why?
> Isn't that what set_type is supposed to do?

set_type currently does it, so this activate can be removed, but
deactivate() is need as it helps to convert the pin back to gpio
function.

>
> > +}
> > +
> > +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>
> It really feels like you need a hwirq_to_gpio() accessor.

Yes. I'll add it.

>
> > +
> > +     if ((gpio < IRQ_START_PIN(priv)) ||
> > +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
> > +             return;
>
> Why do we need this?

Again, let me remove it.

>
> > +
> > +     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)
> > +{
> > +     if (fwspec->param_count != 2)
> > +             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 type = IRQ_TYPE_NONE;
> > +     unsigned int i;
> > +     u32 ret;
> > +
> > +     ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
> > +     if (ret)
> > +             return ret;
>
> How can this fail here?

This could fail if wrong param_count was detected in _translate().

>
> > +
> > +     hwirq = fwspec->param[0];
> > +     if ((hwirq >= NIRQ_MAX(priv)) ||
> > +                     (hwirq + nr_irqs > NIRQ_MAX(priv)))
> > +             return -EINVAL;
>
> How can this happen?

This is for case of out of range hwirq.

>
> > +
> > +     for (i = 0; i < nr_irqs; i++)
> > +             irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > +                             &xgene_gpio_sb_irq_chip, priv);
> > +
> > +     if (is_of_node(domain->parent->fwnode)) {
> > +             parent_fwspec.fwnode = domain->parent->fwnode;
> > +             parent_fwspec.param_count = 3;
> > +             parent_fwspec.param[0] = 0;/* SPI */
> > +             /* Skip SGIs and PPIs*/
> > +             parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
> > +             parent_fwspec.param[2] = fwspec->param[1];
> > +     } else if (is_fwnode_irqchip(domain->parent->fwnode)) {
> > +             parent_fwspec.fwnode = domain->parent->fwnode;
> > +             parent_fwspec.param_count = 2;
> > +             parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
> > +             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;
> > +
> > +     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 const struct of_device_id xgene_gpio_sb_of_match[] = {
> > +     {.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
> > +     {"APMC0D15", SBGPIO_XGENE},
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
> > +#endif
> > +
> >  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;
> > +     const struct of_device_id *of_id;
> > +     struct irq_domain *parent_domain = NULL;
> >
> >       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
> >       if (IS_ERR(regs))
> >               return PTR_ERR(regs);
> >
> > +     priv->regs = regs;
> > +
> > +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
> > +     if (of_id)
> > +             priv->flags = (uintptr_t)of_id->data;
>
> Wait. Everything is hardcoded? So why do we have to deal with looking
> into that structure if nothing is actually parametrized?

There will be other instances with difference number of irq pins /gpio
/start_irq_base etc.

>
> > +#ifdef CONFIG_ACPI
> > +     else {
> > +             const struct acpi_device_id *acpi_id;
> > +
> > +             acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
> > +                             &pdev->dev);
> > +             if (acpi_id)
> > +                     priv->flags = (uintptr_t)acpi_id->driver_data;
> > +     }
> > +#endif
>
> nit: you can write this as
>
>         if (of_id) {
>                 ...
> #ifdef ...
>         } else {
>                 ...
> #endif
>         }
>
>
> Which preserves the Linux coding style.
>

Thanks, let me change the code that way.

> > +     ret = platform_get_irq(pdev, 0);
> > +     if (ret > 0) {
> > +             priv->flags &= ~0xff;
> > +             priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
> > +             parent_domain = irq_get_irq_data(ret)->domain;
> > +     }
>
> This is rather ugly. You have the interrupt-parent property. Why don't
> you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
> do you have that the interrupts are going to be sorted in the DT? There
> is no such garantee in the documentation.

I decided to keep them because I still found difficult with ACPI
table, which does not have interrupt-parent property. This code works
with both DT and ACPI so I keep it.

>
> > +     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,36 +334,34 @@ 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->nirq = XGENE_MAX_GPIO_DS_IRQ;
> > +     priv->gc.to_irq = xgene_gpio_sb_to_irq;
> > +     priv->gc.ngpio = NGPIO_MAX(priv);
>
> So we do have duplicated information everywhere. Great.

Let me fix it :-D

>
> >
> > -     priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
> > -                                GFP_KERNEL);
> > -     if (!priv->irq)
> > -             return -ENOMEM;
> > +     platform_set_drvdata(pdev, priv);
> >
> > -     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);
> > -     }
> > +     priv->irq_domain = irq_domain_create_hierarchy(parent_domain,
> > +                                     0, NIRQ_MAX(priv),
> > +                                     of_node_to_fwnode(pdev->dev.of_node),
> > +                                     &xgene_gpio_sb_domain_ops, priv);
> > +     if (!priv->irq_domain)
> > +             return -ENODEV;
> >
> > -     platform_set_drvdata(pdev, priv);
> > +     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");
> > -     else
> > -             dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
> > -
> > -     if (priv->nirq > 0) {
> > -             /* Register interrupt handlers for gpio signaled acpi events */
> > -             acpi_gpiochip_request_interrupts(&priv->gc);
> > +     if (ret) {
> > +             dev_err(&pdev->dev,
> > +                     "failed to register X-Gene GPIO Standby driver\n");
> > +             if (priv->irq_domain)
>
> How can that not be true, given that you've tested irq_domain just above?

Yes, this is redundant, I'll definitely remove it

>
> > +                     irq_domain_remove(priv->irq_domain);
> > +             return ret;
> >       }
> >
> > +     dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
> > +
> > +     /* Register interrupt handlers for gpio signaled acpi events */
> > +     acpi_gpiochip_request_interrupts(&priv->gc);
> > +
> >       return ret;
> >  }
> >
> > @@ -134,28 +369,14 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev)
> >  {
> >       struct xgene_gpio_sb *priv = platform_get_drvdata(pdev);
> >
> > -     if (priv->nirq > 0) {
> > -             acpi_gpiochip_free_interrupts(&priv->gc);
> > -     }
> > +     acpi_gpiochip_free_interrupts(&priv->gc);
> > +
> > +     irq_domain_remove(priv->irq_domain);
> >
> >       gpiochip_remove(&priv->gc);
> >       return 0;
> >  }
> >
> > -static const struct of_device_id xgene_gpio_sb_of_match[] = {
> > -     {.compatible = "apm,xgene-gpio-sb", },
> > -     {},
> > -};
> > -MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
> > -
> > -#ifdef CONFIG_ACPI
> > -static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
> > -     {"APMC0D15", 0},
> > -     {},
> > -};
> > -MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
> > -#endif
> > -
> >  static struct platform_driver xgene_gpio_sb_driver = {
> >       .driver = {
> >                  .name = "xgene-gpio-sb",
> >
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Thanks M. for this detail review,

-- Quan Nguyen

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

[Marc Zyngier]

On 26/01/16 16:27, Quan Nguyen wrote:
> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> On 26/01/16 07:22, 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>
>>> ---
>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 276 insertions(+), 55 deletions(-)

[...]

>>> +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 = d->hwirq + IRQ_START_PIN(priv);
>>> +     int lvl_type;
>>> +     int ret;
>>> +
>>> +     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:
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     ret = gpiochip_lock_as_irq(&priv->gc, gpio);
>>
>> This has no business whatsoever in set_type. This should be done either
>> when the GPIO is activated as an IRQ in the domain "activate" method, or
>> in the "startup" method of the irqchip.
> 
> The irq pin can do high/low level as well as edge rising/falling,
> while its parent(GIC) can only be high level/edge rising. Hence, there
> is need to configure the irq pin to indicate its parent irq chip when
> there is "high" or "low" on the pin, very much like an invert as the
> code below:
> xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL, d->hwirq,
> lvl_type);

I don't get it. your "gpiochip_lock_as_irq" doesn't seem to go anywhere
the GIC, and doesn't have any knowledge of the trigger. So what is the
trick?

>>
>>> +     if (ret) {
>>> +             dev_err(priv->gc.parent,
>>> +             "Unable to configure XGene GPIO standby pin %d as IRQ\n",
>>> +                             gpio);
>>> +             return ret;
>>> +     }
>>> +
>>> +     if ((gpio >= IRQ_START_PIN(priv)) &&
>>> +                     (d->hwirq < NIRQ_MAX(priv))) {
>>
>> How can we end-up here if your GPIO is not part that range? This should
>> be guaranteed by construction.
> 
> I agree, let me remove it.
> 
>>
>>> +             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 void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
>>> +{
>>> +     struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
>>> +
>>> +     gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
>>> +}
>>> +
>>> +static struct irq_chip xgene_gpio_sb_irq_chip = {
>>> +     .name           = "sbgpio",
>>> +     .irq_ack        = irq_chip_ack_parent,
>>> +     .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,
>>> +     .irq_shutdown   = xgene_gpio_sb_irq_shutdown,
>>> +};
>>> +
>>> +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;
>>> +     unsigned int virq;
>>> +
>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>> +             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 - IRQ_START_PIN(priv);
>>> +     fwspec.param[1] = IRQ_TYPE_NONE;
>>> +     virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
>>> +     if (!virq)
>>> +             virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
>>> +                                             NUMA_NO_NODE, &fwspec);
>>
>> You should not use these low-level functions directly. Use
>> irq_create_fwspec_mapping, which will do the right thing.
> 
> Yes, agree, the code should be much better.
> 
> Let me change:
> 
> virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
> if (!virq)
> virq =  irq_domain_alloc_irqs(priv->irq_domain, 1, NUMA_NO_NODE, &fwspec);
> return virq;
> 
> to:
> return irq_create_fwspec_mapping(&fwspec);
> 
>>
>>> +     return virq;
>>> +}
>>> +
>>> +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>>>
>>> -     if (priv->irq[gpio])
>>> -             return priv->irq[gpio];
>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>> +             return;
>>
>> Again, how can this happen?
> 
> let me remove this redundant code.
> 
>>
>>>
>>> -     return -ENXIO;
>>> +     xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
>>> +                     gpio * 2, 1);
>>
>> This seems to program the interrupt to be active on a low level. Why?
>> Isn't that what set_type is supposed to do?
> 
> set_type currently does it, so this activate can be removed, but
> deactivate() is need as it helps to convert the pin back to gpio
> function.
> 
>>
>>> +}
>>> +
>>> +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>>
>> It really feels like you need a hwirq_to_gpio() accessor.
> 
> Yes. I'll add it.
> 
>>
>>> +
>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>> +             return;
>>
>> Why do we need this?
> 
> Again, let me remove it.
> 
>>
>>> +
>>> +     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)
>>> +{
>>> +     if (fwspec->param_count != 2)
>>> +             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 type = IRQ_TYPE_NONE;
>>> +     unsigned int i;
>>> +     u32 ret;
>>> +
>>> +     ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
>>> +     if (ret)
>>> +             return ret;
>>
>> How can this fail here?
> 
> This could fail if wrong param_count was detected in _translate().

But you only get there if translate succeeded the first place when
called from irq_create_fwspec_mapping -> irq_domain_translate, which
happens before trying any allocation.

So I'm still stating that this cannot fail in any way.

> 
>>
>>> +
>>> +     hwirq = fwspec->param[0];
>>> +     if ((hwirq >= NIRQ_MAX(priv)) ||
>>> +                     (hwirq + nr_irqs > NIRQ_MAX(priv)))
>>> +             return -EINVAL;
>>
>> How can this happen?
> 
> This is for case of out of range hwirq.

Then it would be better placed in the translate method, so that we can
abort early.

>>
>>> +
>>> +     for (i = 0; i < nr_irqs; i++)
>>> +             irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>>> +                             &xgene_gpio_sb_irq_chip, priv);
>>> +
>>> +     if (is_of_node(domain->parent->fwnode)) {
>>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>>> +             parent_fwspec.param_count = 3;
>>> +             parent_fwspec.param[0] = 0;/* SPI */
>>> +             /* Skip SGIs and PPIs*/
>>> +             parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
>>> +             parent_fwspec.param[2] = fwspec->param[1];
>>> +     } else if (is_fwnode_irqchip(domain->parent->fwnode)) {
>>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>>> +             parent_fwspec.param_count = 2;
>>> +             parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
>>> +             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;
>>> +
>>> +     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 const struct of_device_id xgene_gpio_sb_of_match[] = {
>>> +     {.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
>>> +     {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
>>> +     {"APMC0D15", SBGPIO_XGENE},
>>> +     {},
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
>>> +#endif
>>> +
>>>  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;
>>> +     const struct of_device_id *of_id;
>>> +     struct irq_domain *parent_domain = NULL;
>>>
>>>       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>       if (!priv)
>>> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>>>       if (IS_ERR(regs))
>>>               return PTR_ERR(regs);
>>>
>>> +     priv->regs = regs;
>>> +
>>> +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
>>> +     if (of_id)
>>> +             priv->flags = (uintptr_t)of_id->data;
>>
>> Wait. Everything is hardcoded? So why do we have to deal with looking
>> into that structure if nothing is actually parametrized?
> 
> There will be other instances with difference number of irq pins /gpio
> /start_irq_base etc.

Then it has to be described in DT right now.

> 
>>
>>> +#ifdef CONFIG_ACPI
>>> +     else {
>>> +             const struct acpi_device_id *acpi_id;
>>> +
>>> +             acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
>>> +                             &pdev->dev);
>>> +             if (acpi_id)
>>> +                     priv->flags = (uintptr_t)acpi_id->driver_data;
>>> +     }
>>> +#endif
>>
>> nit: you can write this as
>>
>>         if (of_id) {
>>                 ...
>> #ifdef ...
>>         } else {
>>                 ...
>> #endif
>>         }
>>
>>
>> Which preserves the Linux coding style.
>>
> 
> Thanks, let me change the code that way.
> 
>>> +     ret = platform_get_irq(pdev, 0);
>>> +     if (ret > 0) {
>>> +             priv->flags &= ~0xff;
>>> +             priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
>>> +             parent_domain = irq_get_irq_data(ret)->domain;
>>> +     }
>>
>> This is rather ugly. You have the interrupt-parent property. Why don't
>> you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
>> do you have that the interrupts are going to be sorted in the DT? There
>> is no such garantee in the documentation.
> 
> I decided to keep them because I still found difficult with ACPI
> table, which does not have interrupt-parent property. This code works
> with both DT and ACPI so I keep it.

Then again: what guarantees that you will have:
- the lowest interrupt listed first?
- a set contiguous interrupts?

Your DT binding doesn't specify anything of that sort, so I could write
a DT that uses interrupts 7 5 and 142, in that order. It would be legal,
and yet things would explode. So please be clear in your DT binding
about what you do support.

Thanks,

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

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

[Quan Nguyen]

On Wed, Jan 27, 2016 at 12:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 26/01/16 16:27, Quan Nguyen wrote:
>> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>
>>> On 26/01/16 07:22, 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>
>>>> ---
>>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 276 insertions(+), 55 deletions(-)
>
> [...]
>
>>>> +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 = d->hwirq + IRQ_START_PIN(priv);
>>>> +     int lvl_type;
>>>> +     int ret;
>>>> +
>>>> +     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:
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>> +     ret = gpiochip_lock_as_irq(&priv->gc, gpio);
>>>
>>> This has no business whatsoever in set_type. This should be done either
>>> when the GPIO is activated as an IRQ in the domain "activate" method, or
>>> in the "startup" method of the irqchip.
>>
>> The irq pin can do high/low level as well as edge rising/falling,
>> while its parent(GIC) can only be high level/edge rising. Hence, there
>> is need to configure the irq pin to indicate its parent irq chip when
>> there is "high" or "low" on the pin, very much like an invert as the
>> code below:
>> xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL, d->hwirq,
>> lvl_type);
>
> I don't get it. your "gpiochip_lock_as_irq" doesn't seem to go anywhere
> the GIC, and doesn't have any knowledge of the trigger. So what is the
> trick?

Sorry Marc, it was me who misunderstood here.
And yes, gpiochip_lock_as_irq() should go to activate method.

>
>>>
>>>> +     if (ret) {
>>>> +             dev_err(priv->gc.parent,
>>>> +             "Unable to configure XGene GPIO standby pin %d as IRQ\n",
>>>> +                             gpio);
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     if ((gpio >= IRQ_START_PIN(priv)) &&
>>>> +                     (d->hwirq < NIRQ_MAX(priv))) {
>>>
>>> How can we end-up here if your GPIO is not part that range? This should
>>> be guaranteed by construction.
>>
>> I agree, let me remove it.
>>
>>>
>>>> +             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 void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
>>>> +{
>>>> +     struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
>>>> +
>>>> +     gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
>>>> +}
>>>> +
>>>> +static struct irq_chip xgene_gpio_sb_irq_chip = {
>>>> +     .name           = "sbgpio",
>>>> +     .irq_ack        = irq_chip_ack_parent,
>>>> +     .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,
>>>> +     .irq_shutdown   = xgene_gpio_sb_irq_shutdown,
>>>> +};
>>>> +
>>>> +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;
>>>> +     unsigned int virq;
>>>> +
>>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>>> +             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 - IRQ_START_PIN(priv);
>>>> +     fwspec.param[1] = IRQ_TYPE_NONE;
>>>> +     virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
>>>> +     if (!virq)
>>>> +             virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
>>>> +                                             NUMA_NO_NODE, &fwspec);
>>>
>>> You should not use these low-level functions directly. Use
>>> irq_create_fwspec_mapping, which will do the right thing.
>>
>> Yes, agree, the code should be much better.
>>
>> Let me change:
>>
>> virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
>> if (!virq)
>> virq =  irq_domain_alloc_irqs(priv->irq_domain, 1, NUMA_NO_NODE, &fwspec);
>> return virq;
>>
>> to:
>> return irq_create_fwspec_mapping(&fwspec);
>>
>>>
>>>> +     return virq;
>>>> +}
>>>> +
>>>> +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>>>>
>>>> -     if (priv->irq[gpio])
>>>> -             return priv->irq[gpio];
>>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>>> +             return;
>>>
>>> Again, how can this happen?
>>
>> let me remove this redundant code.
>>
>>>
>>>>
>>>> -     return -ENXIO;
>>>> +     xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
>>>> +                     gpio * 2, 1);
>>>
>>> This seems to program the interrupt to be active on a low level. Why?
>>> Isn't that what set_type is supposed to do?
>>
>> set_type currently does it, so this activate can be removed, but
>> deactivate() is need as it helps to convert the pin back to gpio
>> function.
>>
>>>
>>>> +}
>>>> +
>>>> +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 = irq_data->hwirq + IRQ_START_PIN(priv);
>>>
>>> It really feels like you need a hwirq_to_gpio() accessor.
>>
>> Yes. I'll add it.
>>
>>>
>>>> +
>>>> +     if ((gpio < IRQ_START_PIN(priv)) ||
>>>> +                     (gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
>>>> +             return;
>>>
>>> Why do we need this?
>>
>> Again, let me remove it.
>>
>>>
>>>> +
>>>> +     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)
>>>> +{
>>>> +     if (fwspec->param_count != 2)
>>>> +             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 type = IRQ_TYPE_NONE;
>>>> +     unsigned int i;
>>>> +     u32 ret;
>>>> +
>>>> +     ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
>>>> +     if (ret)
>>>> +             return ret;
>>>
>>> How can this fail here?
>>
>> This could fail if wrong param_count was detected in _translate().
>
> But you only get there if translate succeeded the first place when
> called from irq_create_fwspec_mapping -> irq_domain_translate, which
> happens before trying any allocation.
>
> So I'm still stating that this cannot fail in any way.
>

I think I understand now. If so, calling translate here is redundant,
and hence, let me remove these code.

>>
>>>
>>>> +
>>>> +     hwirq = fwspec->param[0];
>>>> +     if ((hwirq >= NIRQ_MAX(priv)) ||
>>>> +                     (hwirq + nr_irqs > NIRQ_MAX(priv)))
>>>> +             return -EINVAL;
>>>
>>> How can this happen?
>>
>> This is for case of out of range hwirq.
>
> Then it would be better placed in the translate method, so that we can
> abort early.
>

And here also, let me move these into translate method instead.

>>>
>>>> +
>>>> +     for (i = 0; i < nr_irqs; i++)
>>>> +             irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>>>> +                             &xgene_gpio_sb_irq_chip, priv);
>>>> +
>>>> +     if (is_of_node(domain->parent->fwnode)) {
>>>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>>>> +             parent_fwspec.param_count = 3;
>>>> +             parent_fwspec.param[0] = 0;/* SPI */
>>>> +             /* Skip SGIs and PPIs*/
>>>> +             parent_fwspec.param[1] = hwirq + START_PARENT_IRQ(priv) - 32;
>>>> +             parent_fwspec.param[2] = fwspec->param[1];
>>>> +     } else if (is_fwnode_irqchip(domain->parent->fwnode)) {
>>>> +             parent_fwspec.fwnode = domain->parent->fwnode;
>>>> +             parent_fwspec.param_count = 2;
>>>> +             parent_fwspec.param[0] = hwirq + START_PARENT_IRQ(priv);
>>>> +             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;
>>>> +
>>>> +     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 const struct of_device_id xgene_gpio_sb_of_match[] = {
>>>> +     {.compatible = "apm,xgene-gpio-sb", .data = (const void *)SBGPIO_XGENE},
>>>> +     {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, xgene_gpio_sb_of_match);
>>>> +
>>>> +#ifdef CONFIG_ACPI
>>>> +static const struct acpi_device_id xgene_gpio_sb_acpi_match[] = {
>>>> +     {"APMC0D15", SBGPIO_XGENE},
>>>> +     {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, xgene_gpio_sb_acpi_match);
>>>> +#endif
>>>> +
>>>>  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;
>>>> +     const struct of_device_id *of_id;
>>>> +     struct irq_domain *parent_domain = NULL;
>>>>
>>>>       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>>>>       if (!priv)
>>>> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>>>>       if (IS_ERR(regs))
>>>>               return PTR_ERR(regs);
>>>>
>>>> +     priv->regs = regs;
>>>> +
>>>> +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
>>>> +     if (of_id)
>>>> +             priv->flags = (uintptr_t)of_id->data;
>>>
>>> Wait. Everything is hardcoded? So why do we have to deal with looking
>>> into that structure if nothing is actually parametrized?
>>
>> There will be other instances with difference number of irq pins /gpio
>> /start_irq_base etc.
>
> Then it has to be described in DT right now.
>

What I was thinking is to have other id to match with difference
instances and these code can be use for ACPI also. Let say
"apm,xgene2-gpio-sb"
Please help correcting me if it is not right.

>>
>>>
>>>> +#ifdef CONFIG_ACPI
>>>> +     else {
>>>> +             const struct acpi_device_id *acpi_id;
>>>> +
>>>> +             acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
>>>> +                             &pdev->dev);
>>>> +             if (acpi_id)
>>>> +                     priv->flags = (uintptr_t)acpi_id->driver_data;
>>>> +     }
>>>> +#endif
>>>
>>> nit: you can write this as
>>>
>>>         if (of_id) {
>>>                 ...
>>> #ifdef ...
>>>         } else {
>>>                 ...
>>> #endif
>>>         }
>>>
>>>
>>> Which preserves the Linux coding style.
>>>
>>
>> Thanks, let me change the code that way.
>>
>>>> +     ret = platform_get_irq(pdev, 0);
>>>> +     if (ret > 0) {
>>>> +             priv->flags &= ~0xff;
>>>> +             priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
>>>> +             parent_domain = irq_get_irq_data(ret)->domain;
>>>> +     }
>>>
>>> This is rather ugly. You have the interrupt-parent property. Why don't
>>> you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
>>> do you have that the interrupts are going to be sorted in the DT? There
>>> is no such garantee in the documentation.
>>
>> I decided to keep them because I still found difficult with ACPI
>> table, which does not have interrupt-parent property. This code works
>> with both DT and ACPI so I keep it.
>
> Then again: what guarantees that you will have:
> - the lowest interrupt listed first?
> - a set contiguous interrupts?
>
> Your DT binding doesn't specify anything of that sort, so I could write
> a DT that uses interrupts 7 5 and 142, in that order. It would be legal,
> and yet things would explode. So please be clear in your DT binding
> about what you do support.

Thanks Marc for this suggestion. I'll update DT binding document to
state that the first/lowest interrupt must be specified

Thanks Marc,
-- Quan Nguyen

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

[Marc Zyngier]

Quan,

On 27/01/16 12:48, Quan Nguyen wrote:
> On Wed, Jan 27, 2016 at 12:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 26/01/16 16:27, Quan Nguyen wrote:
>>> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>
>>>> On 26/01/16 07:22, 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>
>>>>> ---
>>>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 276 insertions(+), 55 deletions(-)
>>
>> [...]
>>>>> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>>>>>       if (IS_ERR(regs))
>>>>>               return PTR_ERR(regs);
>>>>>
>>>>> +     priv->regs = regs;
>>>>> +
>>>>> +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
>>>>> +     if (of_id)
>>>>> +             priv->flags = (uintptr_t)of_id->data;
>>>>
>>>> Wait. Everything is hardcoded? So why do we have to deal with looking
>>>> into that structure if nothing is actually parametrized?
>>>
>>> There will be other instances with difference number of irq pins /gpio
>>> /start_irq_base etc.
>>
>> Then it has to be described in DT right now.
>>
> 
> What I was thinking is to have other id to match with difference
> instances and these code can be use for ACPI also. Let say
> "apm,xgene2-gpio-sb"
> Please help correcting me if it is not right.

I still think this is the wrong thing to do. You are hiding magic values
in the driver, for no good reason. If ACPI has such a broken model that
it cannot give you the various parameters you need, then this is an ACPI
problem you can solve in the ACPI-specific code. Alternatively, you
could also fix your ACPI tables to be less braindead.

But please do not turn the DT code into the same mess for the sake of
using the lowest common denominator. Have a set of properties describing
the HW, a compatibility string to nicely identify the revision, and use
that. You can still hardcode things for ACPI if you desperately need it.

Thanks,

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

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

[Quan Nguyen]

On Wed, Jan 27, 2016 at 8:10 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Quan,
>
> On 27/01/16 12:48, Quan Nguyen wrote:
>> On Wed, Jan 27, 2016 at 12:39 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 26/01/16 16:27, Quan Nguyen wrote:
>>>> On Tue, Jan 26, 2016 at 5:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>
>>>>> On 26/01/16 07:22, 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>
>>>>>> ---
>>>>>>  drivers/gpio/gpio-xgene-sb.c | 331 ++++++++++++++++++++++++++++++++++++-------
>>>>>>  1 file changed, 276 insertions(+), 55 deletions(-)
>>>
>>> [...]
>>>>>> @@ -90,6 +301,32 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
>>>>>>       if (IS_ERR(regs))
>>>>>>               return PTR_ERR(regs);
>>>>>>
>>>>>> +     priv->regs = regs;
>>>>>> +
>>>>>> +     of_id = of_match_device(xgene_gpio_sb_of_match, &pdev->dev);
>>>>>> +     if (of_id)
>>>>>> +             priv->flags = (uintptr_t)of_id->data;
>>>>>
>>>>> Wait. Everything is hardcoded? So why do we have to deal with looking
>>>>> into that structure if nothing is actually parametrized?
>>>>
>>>> There will be other instances with difference number of irq pins /gpio
>>>> /start_irq_base etc.
>>>
>>> Then it has to be described in DT right now.
>>>
>>
>> What I was thinking is to have other id to match with difference
>> instances and these code can be use for ACPI also. Let say
>> "apm,xgene2-gpio-sb"
>> Please help correcting me if it is not right.
>
> I still think this is the wrong thing to do. You are hiding magic values
> in the driver, for no good reason. If ACPI has such a broken model that
> it cannot give you the various parameters you need, then this is an ACPI
> problem you can solve in the ACPI-specific code. Alternatively, you
> could also fix your ACPI tables to be less braindead.
>
> But please do not turn the DT code into the same mess for the sake of
> using the lowest common denominator. Have a set of properties describing
> the HW, a compatibility string to nicely identify the revision, and use
> that. You can still hardcode things for ACPI if you desperately need it.
>
>

Thanks, Marc.
I'll try with set of properties describing HW and may utilize _DSD
method for ACPI.

-- Quan Nguyen

[PATCH v4 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     | 39 ++++++++++++++++++----
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
index dae1300..2fb76d8 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
@@ -16,9 +26,14 @@ Required properties:
 		1 = active low
 - gpio-controller: Marks the device node as a GPIO controller.
 - interrupts: Shall contain exactly 6 interrupts.
+- 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.
 
 Example:
-	sbgpio: sbgpio@17001000 {
+	sbgpio: gpio@17001000{
 		compatible = "apm,xgene-gpio-sb";
 		reg = <0x0 0x17001000 0x0 0x400>;
 		#gpio-cells = <2>;
@@ -29,4 +44,16 @@ Example:
 				<0x0 0x2b 0x1>,
 				<0x0 0x2c 0x1>,
 				<0x0 0x2d 0x1>;
+		interrupt-parent = <&gic>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+	};
+
+	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 v4 2/3] Documentation: gpio: Update description for X-Gene standby GPIO controller DTS binding

[Rob Herring]

On Tue, Jan 26, 2016 at 02:22:14PM +0700, 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     | 39 ++++++++++++++++++----
>  1 file changed, 33 insertions(+), 6 deletions(-)

Did you change something here? I've already acked this. Please add acks 
when posting new versions.

Rob

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

[Quan Nguyen]

On Fri, Jan 29, 2016 at 9:46 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 26, 2016 at 02:22:14PM +0700, 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     | 39 ++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 6 deletions(-)
>
> Did you change something here? I've already acked this. Please add acks
> when posting new versions.
>
Yes, I did, Rob.
I really appreciate your ack in last version. But some changes were
added on this version and some more on new DT properties description
to come in my next version. I'm going to send out new version shortly
based on recent review comments and I'd appreciate with either ack or
nak.

Thanks,
-- Quan Nguyen

[PATCH v4 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