[PATCH v2 0/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[PATCH v2 0/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Y Vo]

V2 Changes:
	- support X-Gene standby GPIO as an interrupt controller.

Y Vo (3):
  gpio: xgene: add support to configure GPIO line as input, output or  
      external IRQ pin
  Documentation: gpio: Update description for X-Gene standby GPIO    
    controller DTS binding
  arm64: dts: Update APM X-Gene standby GPIO controller DTS entries

 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |   23 ++-
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |   15 +-
 drivers/gpio/gpio-xgene-sb.c                       |  234 +++++++++++++++++---
 3 files changed, 223 insertions(+), 49 deletions(-)

[PATCH v2 1/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Y Vo]

Add support to configure GPIO line as input, output or external IRQ pin.

Signed-off-by: Y Vo <yvo@apm.com>
---
 drivers/gpio/gpio-xgene-sb.c |  234 +++++++++++++++++++++++++++++++++++------
 1 files changed, 199 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index d57068b..e67ab4c 100644
--- a/drivers/gpio/gpio-xgene-sb.c
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2014, Applied Micro Circuits Corporation
  * 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
@@ -32,14 +33,24 @@
 
 #define XGENE_MAX_GPIO_DS		22
 #define XGENE_MAX_GPIO_DS_IRQ		6
+#define XGENE_GPIO8_HWIRQ		0x48
+#define XGENE_GPIO8_IDX			8
 
 #define GPIO_MASK(x)			(1U << ((x) % 32))
 
 #define MPA_GPIO_INT_LVL		0x0290
 #define MPA_GPIO_OE_ADDR		0x029c
 #define MPA_GPIO_OUT_ADDR		0x02a0
-#define MPA_GPIO_IN_ADDR 		0x02a4
-#define MPA_GPIO_SEL_LO 		0x0294
+#define MPA_GPIO_IN_ADDR		0x02a4
+#define MPA_GPIO_SEL_LO			0x0294
+
+#define GPIO_INT_LVL_LEVEL_HIGH         0x000001
+#define GPIO_INT_LVL_LEVEL_LOW          0x000000
+
+#define XGENE_HWIRQ_TO_GPIO(hwirq)	((hwirq) + XGENE_GPIO8_IDX)
+#define XGENE_GPIO_TO_HWIRQ(gpio)	((gpio) - XGENE_GPIO8_IDX)
+#define GIC_IRQ_TO_GPIO_IRQ(hwirq)	((hwirq) - XGENE_GPIO8_HWIRQ)
+#define GPIO_IRQ_TO_GIC_IRQ(hwirq)	((hwirq) + XGENE_GPIO8_HWIRQ)
 
 /**
  * struct xgene_gpio_sb - GPIO-Standby private data structure.
@@ -49,18 +60,14 @@
  */
 struct xgene_gpio_sb {
 	struct bgpio_chip	bgc;
-	u32 *irq;
+	void __iomem *regs;
+	struct irq_domain *gic_domain;
+	struct irq_domain *irq_domain;
 	u32 nirq;
 };
 
-static inline struct xgene_gpio_sb *to_xgene_gpio_sb(struct gpio_chip *gc)
-{
-	struct bgpio_chip *bgc = to_bgpio_chip(gc);
-
-	return container_of(bgc, struct xgene_gpio_sb, bgc);
-}
-
-static void xgene_gpio_set_bit(struct bgpio_chip *bgc, void __iomem *reg, u32 gpio, int val)
+static void xgene_gpio_set_bit(struct bgpio_chip *bgc,
+				void __iomem *reg, u32 gpio, int val)
 {
 	u32 data;
 
@@ -72,21 +79,152 @@ static void xgene_gpio_set_bit(struct bgpio_chip *bgc, void __iomem *reg, u32 gp
 	bgc->write_reg(reg, data);
 }
 
-static int apm_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
+/*
+ * NOP functions
+ */
+static void xgene_gpio_sb_nop(struct irq_data *data) { }
+
+static void xgene_gpio_sb_irq_mask(struct irq_data *d)
+{
+	unsigned int gic_irq;
+	struct irq_data *irqdata;
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+	gic_irq = irq_find_mapping(priv->gic_domain,
+			GPIO_IRQ_TO_GIC_IRQ(d->hwirq));
+	irqdata = irq_get_irq_data(gic_irq);
+	if (!irqdata || !irqdata->chip)
+		return;
+
+	if (irqdata->chip->irq_mask)
+		irqdata->chip->irq_mask(irqdata);
+}
+
+static void xgene_gpio_sb_irq_unmask(struct irq_data *d)
+{
+	unsigned int gic_irq;
+	struct irq_data *irqdata;
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+	gic_irq = irq_find_mapping(priv->gic_domain,
+			GPIO_IRQ_TO_GIC_IRQ(d->hwirq));
+	irqdata = irq_get_irq_data(gic_irq);
+	if (!irqdata || !irqdata->chip)
+		return;
+
+	if (irqdata->chip->irq_unmask)
+		irqdata->chip->irq_unmask(irqdata);
+}
+
+static int xgene_gpio_sb_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	int hwirq = d->hwirq;
+	int gpio = XGENE_HWIRQ_TO_GPIO(hwirq);
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+	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_LVL_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		lvl_type = GPIO_INT_LVL_LEVEL_LOW;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = gpiochip_lock_as_irq(&priv->bgc.gc, gpio);
+	if (ret) {
+		dev_err(priv->bgc.gc.dev,
+		"Unable to configure XGene GPIO standby pin %d as IRQ\n",
+								gpio);
+		return ret;
+	}
+
+	if ((gpio >= XGENE_GPIO8_IDX) &&
+			(hwirq < XGENE_MAX_GPIO_DS_IRQ)) {
+		xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_SEL_LO,
+				gpio * 2, 1);
+		xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_INT_LVL,
+				hwirq, lvl_type);
+	}
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		irq_set_handler_locked(d, handle_edge_irq);
+	else
+		irq_set_handler_locked(d, handle_level_irq);
+
+	return 0;
+}
+
+static void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
+{
+	int gpio = XGENE_HWIRQ_TO_GPIO(d->hwirq);
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(&priv->bgc.gc, gpio);
+}
+
+static void xgene_gpio_sb_irq_handler(struct irq_desc *desc)
+{
+	unsigned int cascade_irq;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct xgene_gpio_sb *priv = irq_desc_get_handler_data(desc);
+
+	chained_irq_enter(chip, desc);
+
+	cascade_irq = irq_find_mapping(priv->irq_domain,
+		GIC_IRQ_TO_GPIO_IRQ(irq_desc_get_irq_data(desc)->hwirq));
+
+	if (cascade_irq)
+		generic_handle_irq(cascade_irq);
+
+	chained_irq_exit(chip, desc);
+}
+
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+	.name           = "sbgpio",
+	.irq_ack        = xgene_gpio_sb_nop,
+	.irq_mask       = xgene_gpio_sb_irq_mask,
+	.irq_unmask     = xgene_gpio_sb_irq_unmask,
+	.irq_set_type   = xgene_gpio_sb_irq_set_type,
+	.irq_shutdown   = xgene_gpio_sb_irq_shutdown,
+};
+
+static inline struct xgene_gpio_sb *to_xgene_gpio_sb(struct gpio_chip *gc)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	return container_of(bgc, struct xgene_gpio_sb, bgc);
+}
+
+static int xgene_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 {
 	struct xgene_gpio_sb *priv = to_xgene_gpio_sb(gc);
 
-	if (priv->irq[gpio])
-		return priv->irq[gpio];
+	if ((gpio < XGENE_GPIO8_IDX) ||
+	    (gpio > XGENE_MAX_GPIO_DS_IRQ + XGENE_GPIO8_IDX))
+		return -ENXIO;
 
-	return -ENXIO;
+	return irq_find_mapping(priv->irq_domain, XGENE_GPIO_TO_HWIRQ(gpio));
+}
+
+static int xgene_irq_to_line(u32 irq)
+{
+	u32 offset = GIC_IRQ_TO_GPIO_IRQ(irq_get_irq_data(irq)->hwirq);
+
+	return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
+			(offset + XGENE_GPIO8_IDX) : -EINVAL;
 }
 
 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};
+	int virq, line;
 	struct resource *res;
 	void __iomem *regs;
 
@@ -99,38 +237,63 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
+	priv->regs = regs;
+
 	ret = bgpio_init(&priv->bgc, &pdev->dev, 4,
 			regs + MPA_GPIO_IN_ADDR,
 			regs + MPA_GPIO_OUT_ADDR, NULL,
 			regs + MPA_GPIO_OE_ADDR, NULL, 0);
-        if (ret)
-                return ret;
+	if (ret)
+		return ret;
 
-	priv->bgc.gc.to_irq = apm_gpio_sb_to_irq;
+	priv->bgc.gc.to_irq = xgene_gpio_sb_to_irq;
 	priv->bgc.gc.ngpio = XGENE_MAX_GPIO_DS;
 
-	priv->nirq = XGENE_MAX_GPIO_DS_IRQ;
-
-	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
-				   GFP_KERNEL);
-	if (!priv->irq)
-		return -ENOMEM;
+	/* Mapping and handling GIC irqs*/
+	for (i = 0; i < XGENE_MAX_GPIO_DS_IRQ; i++) {
+		virq = platform_get_irq(pdev, i);
+		if (virq < 0)
+			break;
+		line = xgene_irq_to_line(virq);
+		if (line < XGENE_GPIO8_IDX)
+			break;
+
+		irq_set_chained_handler_and_data(virq,
+					&xgene_gpio_sb_irq_handler, priv);
+	}
 
-	for (i = 0; i < priv->nirq; i++) {
-		priv->irq[default_lines[i]] = platform_get_irq(pdev, i);
-		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_SEL_LO,
-                                   default_lines[i] * 2, 1);
-		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_INT_LVL, i, 1);
+	priv->nirq = i;
+	if (priv->nirq > 0) {
+		virq = platform_get_irq(pdev, 0);
+		priv->gic_domain = irq_get_irq_data(virq)->domain;
 	}
 
 	platform_set_drvdata(pdev, priv);
 
+	priv->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+			priv->nirq,
+			&irq_domain_simple_ops, priv);
+	if (!priv->irq_domain)
+		return -ENODEV;
+
 	ret = gpiochip_add(&priv->bgc.gc);
-	if (ret)
-		dev_err(&pdev->dev, "failed to register X-Gene GPIO Standby driver\n");
-	else
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register X-Gene GPIO Standby driver\n");
+		irq_domain_remove(priv->irq_domain);
+	} else
 		dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
 
+	priv->bgc.gc.irqdomain = priv->irq_domain;
+
+	/* Init for new mapped irqs*/
+	for (i = 0; i < priv->nirq; i++) {
+		int irq = irq_create_mapping(priv->irq_domain, i);
+
+		irq_set_chip_data(irq, priv);
+		irq_set_chip(irq, &xgene_gpio_sb_irq_chip);
+	}
+
 	if (priv->nirq > 0) {
 		/* Register interrupt handlers for gpio signaled acpi events */
 		acpi_gpiochip_request_interrupts(&priv->bgc.gc);
@@ -143,9 +306,10 @@ static int xgene_gpio_sb_remove(struct platform_device *pdev)
 {
 	struct xgene_gpio_sb *priv = platform_get_drvdata(pdev);
 
-	if (priv->nirq > 0) {
+	if (priv->nirq > 0)
 		acpi_gpiochip_free_interrupts(&priv->bgc.gc);
-	}
+
+	irq_domain_remove(priv->irq_domain);
 
 	return bgpio_remove(&priv->bgc);
 }
-- 
1.7.1

Re: [PATCH v2 1/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Linus Walleij]

On Mon, Oct 26, 2015 at 7:49 AM, Y Vo <yvo@apm.com> wrote:

> Add support to configure GPIO line as input, output or external IRQ pin.
>
> Signed-off-by: Y Vo <yvo@apm.com>

As I will say again, this description is too terse, add lots of information
on how IRQs work on this controller please. I get confused.

(...)

> +#define XGENE_GPIO8_HWIRQ              0x48
> +#define XGENE_GPIO8_IDX                        8
(...)
> +#define XGENE_HWIRQ_TO_GPIO(hwirq)     ((hwirq) + XGENE_GPIO8_IDX)
> +#define XGENE_GPIO_TO_HWIRQ(gpio)      ((gpio) - XGENE_GPIO8_IDX)
> +#define GIC_IRQ_TO_GPIO_IRQ(hwirq)     ((hwirq) - XGENE_GPIO8_HWIRQ)
> +#define GPIO_IRQ_TO_GIC_IRQ(hwirq)     ((hwirq) + XGENE_GPIO8_HWIRQ)

I'm a bit uneasy about this, maybe I just don't get it.

What is this indexing into "GIC IRQ" that is starting to happen
here?

The GIC (if that is drivers/irqchip/irq-gic.c) has a totally dynamic IRQ
translation and the Linux IRQs it is using may change. I am worried
that there is some reliance here on Linux IRQ having certain numbers
because there is certainly no ABI like that.

Is this 0x48 really an index into the *hardware* offset in the GIC?

And if it is: why are we not getting this hardware information from the
device tree like all other interrupt numbers and offsets?

I'm worried about this.

> -       u32 *irq;
> +       void __iomem *regs;
> +       struct irq_domain *gic_domain;
> +       struct irq_domain *irq_domain;

And there is some secondary gic_domain here, which is confusing
since the GIC does have an IRQ domain too.

I think I want a clear explanation to how this GPIO controller interacts
with the GIC, and I want it to be part of the patch, as comments in the
code and in the commit message (which is way too small for a complex
patch like this).

Otherwise I have no chance to understand what is really going on here.

> +static int xgene_gpio_sb_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       int hwirq = d->hwirq;
> +       int gpio = XGENE_HWIRQ_TO_GPIO(hwirq);
> +       struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
> +       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_LVL_LEVEL_HIGH;
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +       case IRQ_TYPE_LEVEL_LOW:
> +               lvl_type = GPIO_INT_LVL_LEVEL_LOW;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       ret = gpiochip_lock_as_irq(&priv->bgc.gc, gpio);
> +       if (ret) {
> +               dev_err(priv->bgc.gc.dev,
> +               "Unable to configure XGene GPIO standby pin %d as IRQ\n",
> +                                                               gpio);
> +               return ret;
> +       }
> +
> +       if ((gpio >= XGENE_GPIO8_IDX) &&
> +                       (hwirq < XGENE_MAX_GPIO_DS_IRQ)) {
> +               xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_SEL_LO,
> +                               gpio * 2, 1);
> +               xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_INT_LVL,
> +                               hwirq, lvl_type);
> +       }
> +       if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +               irq_set_handler_locked(d, handle_edge_irq);
> +       else
> +               irq_set_handler_locked(d, handle_level_irq);
> +
> +       return 0;
> +}

If you are assigning hadle_edge_irq() your irqchip *must* have an
.irq_ack() callback that acknowledges the IRQs as they come in.

This makes me suspect that you haven't really tested edge
interrupts, because if you did, the kernel would crash as it
is unconditionally calling the .irq_ack() from handle_level_irq().

Yours,
Linus Walleij

Re: [PATCH v2 1/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Quan Nguyen]

Forgive me for not turn on plain text mode my last email.

Hi Linus,

My name is Quan Nguyen, I'm working with Y Vo on this patch.

Allow me to explain as below:

In current implementation, gic irq resources were used in both sbgpio
and gpios-key nodes, and this causes confusion.
To avoid this, we introduce sbgpio driver as interrupt controller, it
now provides 6 external irq pins mapped from gpio line 8-13. The
gpio-keys node now uses sbgpio irq resources instead.

-- Quan


On Thu, Oct 29, 2015 at 8:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 26, 2015 at 7:49 AM, Y Vo <yvo@apm.com> wrote:
>
>> Add support to configure GPIO line as input, output or external IRQ pin.
>>
>> Signed-off-by: Y Vo <yvo@apm.com>
>
> As I will say again, this description is too terse, add lots of information
> on how IRQs work on this controller please. I get confused.

How about:
+++++++++++++++++++++++++++++++++
Enable X-Gene standby GPIO driver as interrupt controller, it provides
6 external irq pins via gpio line 8-13.
+++++++++++++++++++++++++++++++++
>
> (...)
>
>> +#define XGENE_GPIO8_HWIRQ              0x48
>> +#define XGENE_GPIO8_IDX                        8
> (...)
>> +#define XGENE_HWIRQ_TO_GPIO(hwirq)     ((hwirq) + XGENE_GPIO8_IDX)
>> +#define XGENE_GPIO_TO_HWIRQ(gpio)      ((gpio) - XGENE_GPIO8_IDX)
>> +#define GIC_IRQ_TO_GPIO_IRQ(hwirq)     ((hwirq) - XGENE_GPIO8_HWIRQ)
>> +#define GPIO_IRQ_TO_GIC_IRQ(hwirq)     ((hwirq) + XGENE_GPIO8_HWIRQ)
>
> I'm a bit uneasy about this, maybe I just don't get it.
>
> What is this indexing into "GIC IRQ" that is starting to happen
> here?
>
> The GIC (if that is drivers/irqchip/irq-gic.c) has a totally dynamic IRQ
> translation and the Linux IRQs it is using may change. I am worried
> that there is some reliance here on Linux IRQ having certain numbers
> because there is certainly no ABI like that.
>
> Is this 0x48 really an index into the *hardware* offset in the GIC?
>
> And if it is: why are we not getting this hardware information from the
> device tree like all other interrupt numbers and offsets?
>
> I'm worried about this.

The SPI40(0x48) through SPI45(0x4D) from GIC are mapped as external
IRQ0 - IRQ5 in this GPIO block, so it is hardware offset not mapped
irq number.

Below is detail:

+ GIC: SPI40 (hwirq=0x48)  <=> SB-GPIO: (hwirq=0) (gpio line 8)
+ GIC: SPI41 (hwirq=0x49)  <=> SB-GPIO: (hwirq=1) (gpio line 9)
...
+ GIC: SPI45 (hwirq=0x4D)  <=> SB-GPIO: (hwirq=5) (gpio line 13)

These defines are to help translating between Gic hardware irq and
SBGPIO hardware irq number.

>
>> -       u32 *irq;
>> +       void __iomem *regs;
>> +       struct irq_domain *gic_domain;
>> +       struct irq_domain *irq_domain;
>
> And there is some secondary gic_domain here, which is confusing
> since the GIC does have an IRQ domain too.
>
> I think I want a clear explanation to how this GPIO controller interacts
> with the GIC, and I want it to be part of the patch, as comments in the
> code and in the commit message (which is way too small for a complex
> patch like this).
>
> Otherwise I have no chance to understand what is really going on here.

SBGPIO currently is not capable to mask/unmask/... irqs, so these will
rely on the parent (GIC) instead. To do so, we need keep a parent
domain reference here "struct irq_domain *gic_domain" so we can find
correspond parent irq in runtime.
>
>> +static int xgene_gpio_sb_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +       int hwirq = d->hwirq;
>> +       int gpio = XGENE_HWIRQ_TO_GPIO(hwirq);
>> +       struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
>> +       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_LVL_LEVEL_HIGH;
>> +               break;
>> +       case IRQ_TYPE_EDGE_FALLING:
>> +       case IRQ_TYPE_LEVEL_LOW:
>> +               lvl_type = GPIO_INT_LVL_LEVEL_LOW;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = gpiochip_lock_as_irq(&priv->bgc.gc, gpio);
>> +       if (ret) {
>> +               dev_err(priv->bgc.gc.dev,
>> +               "Unable to configure XGene GPIO standby pin %d as IRQ\n",
>> +                                                               gpio);
>> +               return ret;
>> +       }
>> +
>> +       if ((gpio >= XGENE_GPIO8_IDX) &&
>> +                       (hwirq < XGENE_MAX_GPIO_DS_IRQ)) {
>> +               xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_SEL_LO,
>> +                               gpio * 2, 1);
>> +               xgene_gpio_set_bit(&priv->bgc, priv->regs + MPA_GPIO_INT_LVL,
>> +                               hwirq, lvl_type);
>> +       }
>> +       if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
>> +               irq_set_handler_locked(d, handle_edge_irq);
>> +       else
>> +               irq_set_handler_locked(d, handle_level_irq);
>> +
>> +       return 0;
>> +}
>
> If you are assigning hadle_edge_irq() your irqchip *must* have an
> .irq_ack() callback that acknowledges the IRQs as they come in.
>
> This makes me suspect that you haven't really tested edge
> interrupts, because if you did, the kernel would crash as it
> is unconditionally calling the .irq_ack() from handle_level_irq().

irq_ack() callback is no-op function in this patch.
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+       .name           = "sbgpio",
+       .irq_ack        = xgene_gpio_sb_nop,

>
> Yours,
> Linus Walleij

Re: [PATCH v2 1/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Arnd Bergmann]

On Friday 30 October 2015 12:41:03 Quan Nguyen wrote:
> On Thu, Oct 29, 2015 at 8:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Oct 26, 2015 at 7:49 AM, Y Vo <yvo@apm.com> wrote:
> > (...)
> >> +#define XGENE_HWIRQ_TO_GPIO(hwirq)     ((hwirq) + XGENE_GPIO8_IDX)
> >> +#define XGENE_GPIO_TO_HWIRQ(gpio)      ((gpio) - XGENE_GPIO8_IDX)
> >> +#define GIC_IRQ_TO_GPIO_IRQ(hwirq)     ((hwirq) - XGENE_GPIO8_HWIRQ)
> >> +#define GPIO_IRQ_TO_GIC_IRQ(hwirq)     ((hwirq) + XGENE_GPIO8_HWIRQ)
> >
> > I'm a bit uneasy about this, maybe I just don't get it.
> >
> > What is this indexing into "GIC IRQ" that is starting to happen
> > here?
> >
> > The GIC (if that is drivers/irqchip/irq-gic.c) has a totally dynamic IRQ
> > translation and the Linux IRQs it is using may change. I am worried
> > that there is some reliance here on Linux IRQ having certain numbers
> > because there is certainly no ABI like that.
> >
> > Is this 0x48 really an index into the *hardware* offset in the GIC?
> >
> > And if it is: why are we not getting this hardware information from the
> > device tree like all other interrupt numbers and offsets?
> >
> > I'm worried about this.
> 
> The SPI40(0x48) through SPI45(0x4D) from GIC are mapped as external
> IRQ0 - IRQ5 in this GPIO block, so it is hardware offset not mapped
> irq number.
> 
> Below is detail:
> 
> + GIC: SPI40 (hwirq=0x48)  <=> SB-GPIO: (hwirq=0) (gpio line 8)
> + GIC: SPI41 (hwirq=0x49)  <=> SB-GPIO: (hwirq=1) (gpio line 9)
> ...
> + GIC: SPI45 (hwirq=0x4D)  <=> SB-GPIO: (hwirq=5) (gpio line 13)
> 
> These defines are to help translating between Gic hardware irq and
> SBGPIO hardware irq number.

But the numbers are already in DT, so don't duplicate them in the
source code but just use irq_of_parse_and_map() to get the parent
irq number.

> >> -       u32 *irq;
> >> +       void __iomem *regs;
> >> +       struct irq_domain *gic_domain;
> >> +       struct irq_domain *irq_domain;
> >
> > And there is some secondary gic_domain here, which is confusing
> > since the GIC does have an IRQ domain too.
> >
> > I think I want a clear explanation to how this GPIO controller interacts
> > with the GIC, and I want it to be part of the patch, as comments in the
> > code and in the commit message (which is way too small for a complex
> > patch like this).
> >
> > Otherwise I have no chance to understand what is really going on here.
> 
> SBGPIO currently is not capable to mask/unmask/... irqs, so these will
> rely on the parent (GIC) instead. To do so, we need keep a parent
> domain reference here "struct irq_domain *gic_domain" so we can find
> correspond parent irq in runtime.

Maybe rename the domain to 'parent_domain' or something like that
to avoid hardcoding any knowledge of the parent IRQ controller being
a GIC. If the same GPIO block gets reused on a PowerPC machine, we
want the driver to just work and not have any ARM specifics in it.

	Arnd

Re: [PATCH v2 1/3] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin

[Marc Zyngier]

On Fri, 30 Oct 2015 12:41:03 +0700
Quan Nguyen <qnguyen@apm.com> wrote:

> Forgive me for not turn on plain text mode my last email.
> 
> Hi Linus,
> 
> My name is Quan Nguyen, I'm working with Y Vo on this patch.
> 
> Allow me to explain as below:
> 
> In current implementation, gic irq resources were used in both sbgpio
> and gpios-key nodes, and this causes confusion.
> To avoid this, we introduce sbgpio driver as interrupt controller, it
> now provides 6 external irq pins mapped from gpio line 8-13. The
> gpio-keys node now uses sbgpio irq resources instead.
> 
> -- Quan
> 
> 
> On Thu, Oct 29, 2015 at 8:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Oct 26, 2015 at 7:49 AM, Y Vo <yvo@apm.com> wrote:
> >
> >> Add support to configure GPIO line as input, output or external IRQ pin.
> >>
> >> Signed-off-by: Y Vo <yvo@apm.com>
> >
> > As I will say again, this description is too terse, add lots of information
> > on how IRQs work on this controller please. I get confused.
> 
> How about:
> +++++++++++++++++++++++++++++++++
> Enable X-Gene standby GPIO driver as interrupt controller, it provides
> 6 external irq pins via gpio line 8-13.
> +++++++++++++++++++++++++++++++++
> >
> > (...)
> >
> >> +#define XGENE_GPIO8_HWIRQ              0x48
> >> +#define XGENE_GPIO8_IDX                        8
> > (...)
> >> +#define XGENE_HWIRQ_TO_GPIO(hwirq)     ((hwirq) + XGENE_GPIO8_IDX)
> >> +#define XGENE_GPIO_TO_HWIRQ(gpio)      ((gpio) - XGENE_GPIO8_IDX)
> >> +#define GIC_IRQ_TO_GPIO_IRQ(hwirq)     ((hwirq) - XGENE_GPIO8_HWIRQ)
> >> +#define GPIO_IRQ_TO_GIC_IRQ(hwirq)     ((hwirq) + XGENE_GPIO8_HWIRQ)
> >
> > I'm a bit uneasy about this, maybe I just don't get it.
> >
> > What is this indexing into "GIC IRQ" that is starting to happen
> > here?
> >
> > The GIC (if that is drivers/irqchip/irq-gic.c) has a totally dynamic IRQ
> > translation and the Linux IRQs it is using may change. I am worried
> > that there is some reliance here on Linux IRQ having certain numbers
> > because there is certainly no ABI like that.
> >
> > Is this 0x48 really an index into the *hardware* offset in the GIC?
> >
> > And if it is: why are we not getting this hardware information from the
> > device tree like all other interrupt numbers and offsets?
> >
> > I'm worried about this.
> 
> The SPI40(0x48) through SPI45(0x4D) from GIC are mapped as external
> IRQ0 - IRQ5 in this GPIO block, so it is hardware offset not mapped
> irq number.
> 
> Below is detail:
> 
> + GIC: SPI40 (hwirq=0x48)  <=> SB-GPIO: (hwirq=0) (gpio line 8)
> + GIC: SPI41 (hwirq=0x49)  <=> SB-GPIO: (hwirq=1) (gpio line 9)
> ...
> + GIC: SPI45 (hwirq=0x4D)  <=> SB-GPIO: (hwirq=5) (gpio line 13)
> 
> These defines are to help translating between Gic hardware irq and
> SBGPIO hardware irq number.
> 
> >
> >> -       u32 *irq;
> >> +       void __iomem *regs;
> >> +       struct irq_domain *gic_domain;
> >> +       struct irq_domain *irq_domain;
> >
> > And there is some secondary gic_domain here, which is confusing
> > since the GIC does have an IRQ domain too.
> >
> > I think I want a clear explanation to how this GPIO controller interacts
> > with the GIC, and I want it to be part of the patch, as comments in the
> > code and in the commit message (which is way too small for a complex
> > patch like this).
> >
> > Otherwise I have no chance to understand what is really going on here.
> 
> SBGPIO currently is not capable to mask/unmask/... irqs, so these will
> rely on the parent (GIC) instead. To do so, we need keep a parent
> domain reference here "struct irq_domain *gic_domain" so we can find
> correspond parent irq in runtime.

All this only means one thing: this should be implemented as a
hierarchical domain, just like any other "random widget stacked on top
of the GIC". Digging into the internals of the GIC driver is not
acceptable anymore, and this patch is just horrible.

There is plenty of examples in the tree for you to look at and rewrite
that code using the abstractions that are already in place.

In the meantime, I am NAKing this patch. Please include the irqchip
maintainers in the next iteration of this series.

Thanks,

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

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

[Y Vo]

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>
---
 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |   23 +++++++++++++-------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
index dae1300..7c0e7f4 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
@@ -3,8 +3,7 @@ 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.
+only GPIO_DS8..GPIO_DS13 support interrupts.
 
 Required properties:
 - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
@@ -16,6 +15,11 @@ 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: Shoule be two.
+	- first cell is 0-5 coresponding for GPIO pin 8..13.
+	- second cell is used to specify flags.
+- interrupt-controller: Marks the device node as an interrupt controller.
 
 Example:
 	sbgpio: sbgpio@17001000 {
@@ -23,10 +27,13 @@ Example:
 		reg = <0x0 0x17001000 0x0 0x400>;
 		#gpio-cells = <2>;
 		gpio-controller;
-		interrupts = 	<0x0 0x28 0x1>,
-				<0x0 0x29 0x1>,
-				<0x0 0x2a 0x1>,
-				<0x0 0x2b 0x1>,
-				<0x0 0x2c 0x1>,
-				<0x0 0x2d 0x1>;
+		interrupt-parent = <&gic>;
+		interrupts = 	<0x0 0x28 0x4>,
+				<0x0 0x29 0x4>,
+				<0x0 0x2a 0x4>,
+				<0x0 0x2b 0x4>,
+				<0x0 0x2c 0x4>,
+				<0x0 0x2d 0x4>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
 	};
-- 
1.7.1

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

[Y Vo]

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

Signed-off-by: Y Vo <yvo@apm.com>
---
 arch/arm64/boot/dts/apm/apm-storm.dtsi |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index 6c5ed11..40a79fb 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -765,12 +765,15 @@
 			reg = <0x0 0x17001000 0x0 0x400>;
 			#gpio-cells = <2>;
 			gpio-controller;
-			interrupts = 	<0x0 0x28 0x1>,
-					<0x0 0x29 0x1>,
-					<0x0 0x2a 0x1>,
-					<0x0 0x2b 0x1>,
-					<0x0 0x2c 0x1>,
-					<0x0 0x2d 0x1>;
+			interrupt-parent = <&gic>;
+			interrupts = 	<0x0 0x28 0x4>,
+					<0x0 0x29 0x4>,
+					<0x0 0x2a 0x4>,
+					<0x0 0x2b 0x4>,
+					<0x0 0x2c 0x4>,
+					<0x0 0x2d 0x4>;
+			#interrupt-cells = <2>;
+			interrupt-controller;
 		};
 
 		rtc: rtc@10510000 {
-- 
1.7.1