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

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

[Y Vo]

This patch supports to configure GPIO line as input, output or external IRQ pin.

Y Vo (2):
  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

 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |   26 +++++++++++----
 drivers/gpio/gpio-xgene-sb.c                       |   33 +++++++++++++++----
 2 files changed, 45 insertions(+), 14 deletions(-)

[PATCH 1/2] 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 |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index d57068b..76b15e0 100644
--- a/drivers/gpio/gpio-xgene-sb.c
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -32,6 +32,8 @@
 
 #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))
 
@@ -82,11 +84,19 @@ static int apm_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 	return -ENXIO;
 }
 
+static int xgene_irq_to_line(u32 irq)
+{
+	u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_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;
 
@@ -109,20 +119,29 @@ static int xgene_gpio_sb_probe(struct platform_device *pdev)
 	priv->bgc.gc.to_irq = apm_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;
 
-	for (i = 0; i < priv->nirq; i++) {
-		priv->irq[default_lines[i]] = platform_get_irq(pdev, i);
+	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;
+		priv->irq[line] = virq;
 		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);
+						line * 2, 1);
+		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_INT_LVL,
+						(line - XGENE_GPIO8_IDX), 1);
+		dev_dbg(&pdev->dev,
+			"gpio line %d mapped as external irq pin\n", line);
 	}
 
+	priv->nirq = i;
+
 	platform_set_drvdata(pdev, priv);
 
 	ret = gpiochip_add(&priv->bgc.gc);
-- 
1.7.1

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

[Linus Walleij]

On Fri, Sep 11, 2015 at 2:22 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>

Mostly OK but...

>  #define XGENE_MAX_GPIO_DS              22
>  #define XGENE_MAX_GPIO_DS_IRQ          6
> +#define XGENE_GPIO8_HWIRQ              0x48
> +#define XGENE_GPIO8_IDX                        8
(...)
> +static int xgene_irq_to_line(u32 irq)
> +{
> +       u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_HWIRQ;
> +
> +       return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
> +                       (offset + XGENE_GPIO8_IDX) : -EINVAL;
> +}

What is this hardcoded IRQ 0x48 business?

This patch needs kerneldoc for this xgene_irq_to_line() to explain
what exactly is going on, or noone will understand it enough to
debug or refactor the code.

I hope this 0x48 is not some way of compensating for Linux internal
IRQ offsets, that is what irqdomain is for.

Yours,
Linus Walleij

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

[Y Vo]

On Fri, Oct 2, 2015 at 4:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 11, 2015 at 2:22 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>
>
> Mostly OK but...
>
>>  #define XGENE_MAX_GPIO_DS              22
>>  #define XGENE_MAX_GPIO_DS_IRQ          6
>> +#define XGENE_GPIO8_HWIRQ              0x48
>> +#define XGENE_GPIO8_IDX                        8
> (...)
>> +static int xgene_irq_to_line(u32 irq)
>> +{
>> +       u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_HWIRQ;
>> +
>> +       return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
>> +                       (offset + XGENE_GPIO8_IDX) : -EINVAL;
>> +}
>
> What is this hardcoded IRQ 0x48 business?

This is hardware irq number ~ GPIO_DS8. So we use it to get the gpio
number from hwirq. But we are preparing another version which is
better to configure the GPIO lines as interrupt or direction(in/out).

>
> This patch needs kerneldoc for this xgene_irq_to_line() to explain
> what exactly is going on, or noone will understand it enough to
> debug or refactor the code.
>
> I hope this 0x48 is not some way of compensating for Linux internal
> IRQ offsets, that is what irqdomain is for.
>
> Yours,
> Linus Walleij

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

[Y Vo]

Signed-off-by: Y Vo <yvo@apm.com>
---
 .../devicetree/bindings/gpio/gpio-xgene-sb.txt     |   26 ++++++++++++++-----
 1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
index dae1300..bee1347 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt
@@ -3,8 +3,11 @@ 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
+only GPIO_DS8..GPIO_DS13 support external interrupts. The IRQ mapping
 is currently 1-to-1 on interrupts 0x28 thru 0x2d.
+GPIO pins from GPIO_DS8 to GPIO_DS13 are configured as external interrupt
+by default. If one wish not to use any of these pins as external interrupts,
+the corresponding interrupt resource for those pins should be removed.
 
 Required properties:
 - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller
@@ -23,10 +26,19 @@ 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>;
+		interrupts = 	<0x0 0x28 0x1>,		/* GPIO_DS8 */
+				<0x0 0x29 0x1>,		/* GPIO_DS9 */
+				<0x0 0x2a 0x1>,		/* GPIO_DS10 */
+				<0x0 0x2b 0x1>,		/* GPIO_DS11 */
+				<0x0 0x2c 0x1>,		/* GPIO_DS12 */
+				<0x0 0x2d 0x1>;		/* GPIO_DS13 */
 	};
+
+/* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
+&sbgpio {
+	interrupts =	<0x0 0x29 0x1>,		/* GPIO_DS9 */
+			<0x0 0x2a 0x1>,		/* GPIO_DS10 */
+			<0x0 0x2b 0x1>,		/* GPIO_DS11 */
+			<0x0 0x2c 0x1>,		/* GPIO_DS12 */
+			<0x0 0x2d 0x1>;		/* GPIO_DS13 */
+}
-- 
1.7.1

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

[Arnd Bergmann]

On Friday 11 September 2015 16:22:12 Y Vo wrote:
>  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
> +only GPIO_DS8..GPIO_DS13 support external interrupts. The IRQ mapping
>  is currently 1-to-1 on interrupts 0x28 thru 0x2d.
> +GPIO pins from GPIO_DS8 to GPIO_DS13 are configured as external interrupt
> +by default. If one wish not to use any of these pins as external interrupts,
> +the corresponding interrupt resource for those pins should be removed.

According to this description, I fail to see how one would specify
a missing interrupt for one of the pins that is not the last one in
the list.

	Arnd

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

[Y Vo]

On Fri, Sep 11, 2015 at 4:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 September 2015 16:22:12 Y Vo wrote:
>>  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
>> +only GPIO_DS8..GPIO_DS13 support external interrupts. The IRQ mapping
>>  is currently 1-to-1 on interrupts 0x28 thru 0x2d.
>> +GPIO pins from GPIO_DS8 to GPIO_DS13 are configured as external interrupt
>> +by default. If one wish not to use any of these pins as external interrupts,
>> +the corresponding interrupt resource for those pins should be removed.
>
> According to this description, I fail to see how one would specify
> a missing interrupt for one of the pins that is not the last one in
> the list.
>
All GPIO_DS8..GPIO_DS13 are configured as external IRQ by default. So
if anyone want to use one of them as gpio mode, they can drop add the
sbgpio node to their DTB board-specific, and remove that GPIO from
interrupts property. As example below I want to leave GPIO_DS8 as gpio
mode, so I remove it as below:

/* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
&sbgpio {
       interrupts =    <0x0 0x29 0x1>,         /* GPIO_DS9 */
                       <0x0 0x2a 0x1>,         /* GPIO_DS10 */
                       <0x0 0x2b 0x1>,         /* GPIO_DS11 */
                       <0x0 0x2c 0x1>,         /* GPIO_DS12 */
                       <0x0 0x2d 0x1>;         /* GPIO_DS13 */
}

>         Arnd

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

[Arnd Bergmann]

On Friday 11 September 2015 18:24:47 Y Vo wrote:
> On Fri, Sep 11, 2015 at 4:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 11 September 2015 16:22:12 Y Vo wrote:
> >>  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
> >> +only GPIO_DS8..GPIO_DS13 support external interrupts. The IRQ mapping
> >>  is currently 1-to-1 on interrupts 0x28 thru 0x2d.
> >> +GPIO pins from GPIO_DS8 to GPIO_DS13 are configured as external interrupt
> >> +by default. If one wish not to use any of these pins as external interrupts,
> >> +the corresponding interrupt resource for those pins should be removed.
> >
> > According to this description, I fail to see how one would specify
> > a missing interrupt for one of the pins that is not the last one in
> > the list.
> >
> All GPIO_DS8..GPIO_DS13 are configured as external IRQ by default. So
> if anyone want to use one of them as gpio mode, they can drop add the
> sbgpio node to their DTB board-specific, and remove that GPIO from
> interrupts property. As example below I want to leave GPIO_DS8 as gpio
> mode, so I remove it as below:
> 
> /* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
> &sbgpio {
>        interrupts =    <0x0 0x29 0x1>,         /* GPIO_DS9 */
>                        <0x0 0x2a 0x1>,         /* GPIO_DS10 */
>                        <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>                        <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>                        <0x0 0x2d 0x1>;         /* GPIO_DS13 */
> }

This would be a very unusual binding, because that requires the driver
to know the contents of the properties in advance and the driver
cannot derive the function of an irq from the position in the array.

Please come up with another approach that is more like other drivers
do this and that makes it possible to reuse the driver if the
interrupt numbers change.

	Arnd

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

[Y Vo]

On Fri, Sep 11, 2015 at 7:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 September 2015 18:24:47 Y Vo wrote:
>> On Fri, Sep 11, 2015 at 4:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 11 September 2015 16:22:12 Y Vo wrote:
>> >>  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
>> >> +only GPIO_DS8..GPIO_DS13 support external interrupts. The IRQ mapping
>> >>  is currently 1-to-1 on interrupts 0x28 thru 0x2d.
>> >> +GPIO pins from GPIO_DS8 to GPIO_DS13 are configured as external interrupt
>> >> +by default. If one wish not to use any of these pins as external interrupts,
>> >> +the corresponding interrupt resource for those pins should be removed.
>> >
>> > According to this description, I fail to see how one would specify
>> > a missing interrupt for one of the pins that is not the last one in
>> > the list.
>> >
>> All GPIO_DS8..GPIO_DS13 are configured as external IRQ by default. So
>> if anyone want to use one of them as gpio mode, they can drop add the
>> sbgpio node to their DTB board-specific, and remove that GPIO from
>> interrupts property. As example below I want to leave GPIO_DS8 as gpio
>> mode, so I remove it as below:
>>
>> /* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
>> &sbgpio {
>>        interrupts =    <0x0 0x29 0x1>,         /* GPIO_DS9 */
>>                        <0x0 0x2a 0x1>,         /* GPIO_DS10 */
>>                        <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>>                        <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>>                        <0x0 0x2d 0x1>;         /* GPIO_DS13 */
>> }
>
> This would be a very unusual binding, because that requires the driver
> to know the contents of the properties in advance and the driver
> cannot derive the function of an irq from the position in the array.
>
> Please come up with another approach that is more like other drivers
> do this and that makes it possible to reuse the driver if the
> interrupt numbers change.
The GPIO standby doesn't support interrupt controller itself, it only
wires the GPIO line to the external IRQ of GIC. So it is not similar
to other driver. Each GPIO is fixed to GIC IRQ index, and never
change.

Is that possible if we change to this approach:
- Keep all GPIO as GPIO mode by default.
- If anyone want to use them as interrupt, they must add the interrupt
node to DT specific-board below:

Example to configure GPIO_DS11, 12, 13 as external irq:

&sbgpio {
        interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
                        <0x0 0x2c 0x1>,         /* GPIO_DS12 */
                        <0x0 0x2d 0x1>;         /* GPIO_DS13 */


>         Arnd

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

[Arnd Bergmann]

On Friday 11 September 2015 21:23:31 Y Vo wrote:
> >> /* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
> >> &sbgpio {
> >>        interrupts =    <0x0 0x29 0x1>,         /* GPIO_DS9 */
> >>                        <0x0 0x2a 0x1>,         /* GPIO_DS10 */
> >>                        <0x0 0x2b 0x1>,         /* GPIO_DS11 */
> >>                        <0x0 0x2c 0x1>,         /* GPIO_DS12 */
> >>                        <0x0 0x2d 0x1>;         /* GPIO_DS13 */
> >> }
> >
> > This would be a very unusual binding, because that requires the driver
> > to know the contents of the properties in advance and the driver
> > cannot derive the function of an irq from the position in the array.
> >
> > Please come up with another approach that is more like other drivers
> > do this and that makes it possible to reuse the driver if the
> > interrupt numbers change.
> The GPIO standby doesn't support interrupt controller itself, it only
> wires the GPIO line to the external IRQ of GIC. So it is not similar
> to other driver. Each GPIO is fixed to GIC IRQ index, and never
> change.

That is not relevant here, what matters is just that each IRQ still
has a very specific function, which is basically what every driver
does.

> Is that possible if we change to this approach:
> - Keep all GPIO as GPIO mode by default.
> - If anyone want to use them as interrupt, they must add the interrupt
> node to DT specific-board below:
> 
> Example to configure GPIO_DS11, 12, 13 as external irq:
> 
> &sbgpio {
>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */

That is exactly the same as you have in your patch. You should
always list all interrupts that the gpio controller is wired
to, whether they are used or not.

Ideally, the driver just decides at runtime how to wire that
mux, based on how the gpio is configured by the client driver.
If the client asks for an interrupt (with gpio_to_irq) and is
it is configured as an input, then you connect it to the GIC,
otherwise you leave it to be handled by the gpio chip itself.
Any reason you can't do that?

	Arnd

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

[Y Vo]

On Fri, Sep 11, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 September 2015 21:23:31 Y Vo wrote:
>> >> /* Board-specific: exclude GPIO_DS8 to leave it as GPIO */
>> >> &sbgpio {
>> >>        interrupts =    <0x0 0x29 0x1>,         /* GPIO_DS9 */
>> >>                        <0x0 0x2a 0x1>,         /* GPIO_DS10 */
>> >>                        <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>> >>                        <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>> >>                        <0x0 0x2d 0x1>;         /* GPIO_DS13 */
>> >> }
>> >
>> > This would be a very unusual binding, because that requires the driver
>> > to know the contents of the properties in advance and the driver
>> > cannot derive the function of an irq from the position in the array.
>> >
>> > Please come up with another approach that is more like other drivers
>> > do this and that makes it possible to reuse the driver if the
>> > interrupt numbers change.
>> The GPIO standby doesn't support interrupt controller itself, it only
>> wires the GPIO line to the external IRQ of GIC. So it is not similar
>> to other driver. Each GPIO is fixed to GIC IRQ index, and never
>> change.
>
> That is not relevant here, what matters is just that each IRQ still
> has a very specific function, which is basically what every driver
> does.
>
>> Is that possible if we change to this approach:
>> - Keep all GPIO as GPIO mode by default.
>> - If anyone want to use them as interrupt, they must add the interrupt
>> node to DT specific-board below:
>>
>> Example to configure GPIO_DS11, 12, 13 as external irq:
>>
>> &sbgpio {
>>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */
>
> That is exactly the same as you have in your patch. You should
> always list all interrupts that the gpio controller is wired
> to, whether they are used or not.
>
> Ideally, the driver just decides at runtime how to wire that
> mux, based on how the gpio is configured by the client driver.
> If the client asks for an interrupt (with gpio_to_irq) and is
> it is configured as an input, then you connect it to the GIC,
> otherwise you leave it to be handled by the gpio chip itself.
> Any reason you can't do that?
>
As I just explained, the GPIO Standby doesn't support the interrupt.
Both interrupts are came from GIC, so if the client asks for an
interrupt, it will go to the GIC driver (not GPIO Standby driver), so
that's my trouble. We can not implement the code to configured the
GPIO standby at GIC driver.

>         Arnd

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

[Arnd Bergmann]

On Friday 11 September 2015 22:06:58 Y Vo wrote:
> On Fri, Sep 11, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 11 September 2015 21:23:31 Y Vo wrote:
> >> Is that possible if we change to this approach:
> >> - Keep all GPIO as GPIO mode by default.
> >> - If anyone want to use them as interrupt, they must add the interrupt
> >> node to DT specific-board below:
> >>
> >> Example to configure GPIO_DS11, 12, 13 as external irq:
> >>
> >> &sbgpio {
> >>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
> >>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
> >>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */
> >
> > That is exactly the same as you have in your patch. You should
> > always list all interrupts that the gpio controller is wired
> > to, whether they are used or not.
> >
> > Ideally, the driver just decides at runtime how to wire that
> > mux, based on how the gpio is configured by the client driver.
> > If the client asks for an interrupt (with gpio_to_irq) and is
> > it is configured as an input, then you connect it to the GIC,
> > otherwise you leave it to be handled by the gpio chip itself.
> > Any reason you can't do that?
> >
> As I just explained, the GPIO Standby doesn't support the interrupt.
> Both interrupts are came from GIC, so if the client asks for an
> interrupt, it will go to the GIC driver (not GPIO Standby driver), so
> that's my trouble. We can not implement the code to configured the
> GPIO standby at GIC driver.

So you are saying we merged a binding that is incapable of describing
the hardware? If that is the case, we may have to go back to
the drawing board and come up with a working binding.

One part that I don't understand yet is how the driver handles
irqs to start with, as it does not register a nested irq domain
or irqchip, so I'm guessing that client drivers cannot actually
use the IRQ functionality. Is that right?

It also seems to me that the binding cannot distinguish between a
line configured as an input and one that is configured as an
interrupt, which are for other gpio chips the same thing, but
not on this one.  Could this be rectified by using another bit
of the second gpio cell? The low bit is used for active-high/active-low,
so you could use the second bit for irq/input.

	Arnd

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

[Y Vo]

On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 September 2015 22:06:58 Y Vo wrote:
>> On Fri, Sep 11, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 11 September 2015 21:23:31 Y Vo wrote:
>> >> Is that possible if we change to this approach:
>> >> - Keep all GPIO as GPIO mode by default.
>> >> - If anyone want to use them as interrupt, they must add the interrupt
>> >> node to DT specific-board below:
>> >>
>> >> Example to configure GPIO_DS11, 12, 13 as external irq:
>> >>
>> >> &sbgpio {
>> >>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>> >>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>> >>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */
>> >
>> > That is exactly the same as you have in your patch. You should
>> > always list all interrupts that the gpio controller is wired
>> > to, whether they are used or not.
>> >
>> > Ideally, the driver just decides at runtime how to wire that
>> > mux, based on how the gpio is configured by the client driver.
>> > If the client asks for an interrupt (with gpio_to_irq) and is
>> > it is configured as an input, then you connect it to the GIC,
>> > otherwise you leave it to be handled by the gpio chip itself.
>> > Any reason you can't do that?
>> >
>> As I just explained, the GPIO Standby doesn't support the interrupt.
>> Both interrupts are came from GIC, so if the client asks for an
>> interrupt, it will go to the GIC driver (not GPIO Standby driver), so
>> that's my trouble. We can not implement the code to configured the
>> GPIO standby at GIC driver.
>
> So you are saying we merged a binding that is incapable of describing
> the hardware? If that is the case, we may have to go back to
> the drawing board and come up with a working binding.
>
> One part that I don't understand yet is how the driver handles
> irqs to start with, as it does not register a nested irq domain
> or irqchip, so I'm guessing that client drivers cannot actually
> use the IRQ functionality. Is that right?
For our case the irq_domain is in irq-gic driver, the GPIO driver only
is responsible to wire 2 the external irq of GIC to GPIO pin.

Example for configure GPIO_DS13 as interrupt and use as button with
the current gpio driver:
        gpio-keys {
                compatible = "gpio-keys";
                button@1 {
                        label = "POWER";
                        linux,code = <116>;
                        linux,input-type = <0x1>;
                        interrupts = <0x0 0x2d 0x1>;
                };
        };

>
> It also seems to me that the binding cannot distinguish between a
> line configured as an input and one that is configured as an
> interrupt, which are for other gpio chips the same thing, but
> not on this one.  Could this be rectified by using another bit
> of the second gpio cell? The low bit is used for active-high/active-low,
> so you could use the second bit for irq/input.
>
Do you mean #gpio-cells property and using the high bit of the second
bit for irq/input  ?

>         Arnd

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

[Arnd Bergmann]

On Saturday 12 September 2015 12:55:55 Y Vo wrote:
> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 11 September 2015 22:06:58 Y Vo wrote:
> >> On Fri, Sep 11, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Friday 11 September 2015 21:23:31 Y Vo wrote:
> >> >> Is that possible if we change to this approach:
> >> >> - Keep all GPIO as GPIO mode by default.
> >> >> - If anyone want to use them as interrupt, they must add the interrupt
> >> >> node to DT specific-board below:
> >> >>
> >> >> Example to configure GPIO_DS11, 12, 13 as external irq:
> >> >>
> >> >> &sbgpio {
> >> >>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
> >> >>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
> >> >>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */
> >> >
> >> > That is exactly the same as you have in your patch. You should
> >> > always list all interrupts that the gpio controller is wired
> >> > to, whether they are used or not.
> >> >
> >> > Ideally, the driver just decides at runtime how to wire that
> >> > mux, based on how the gpio is configured by the client driver.
> >> > If the client asks for an interrupt (with gpio_to_irq) and is
> >> > it is configured as an input, then you connect it to the GIC,
> >> > otherwise you leave it to be handled by the gpio chip itself.
> >> > Any reason you can't do that?
> >> >
> >> As I just explained, the GPIO Standby doesn't support the interrupt.
> >> Both interrupts are came from GIC, so if the client asks for an
> >> interrupt, it will go to the GIC driver (not GPIO Standby driver), so
> >> that's my trouble. We can not implement the code to configured the
> >> GPIO standby at GIC driver.
> >
> > So you are saying we merged a binding that is incapable of describing
> > the hardware? If that is the case, we may have to go back to
> > the drawing board and come up with a working binding.
> >
> > One part that I don't understand yet is how the driver handles
> > irqs to start with, as it does not register a nested irq domain
> > or irqchip, so I'm guessing that client drivers cannot actually
> > use the IRQ functionality. Is that right?
> For our case the irq_domain is in irq-gic driver, the GPIO driver only
> is responsible to wire 2 the external irq of GIC to GPIO pin.
> 
> Example for configure GPIO_DS13 as interrupt and use as button with
> the current gpio driver:
>         gpio-keys {
>                 compatible = "gpio-keys";
>                 button@1 {
>                         label = "POWER";
>                         linux,code = <116>;
>                         linux,input-type = <0x1>;
>                         interrupts = <0x0 0x2d 0x1>;
>                 };
>         };

Wait, this looks wrong: the gpio driver doesn't actually see
the connection here and won't be able to configure the interrupt
correctly. The interrupt is already owned by the gpio driver, so
you cannot use it in the button node.

> >
> > It also seems to me that the binding cannot distinguish between a
> > line configured as an input and one that is configured as an
> > interrupt, which are for other gpio chips the same thing, but
> > not on this one.  Could this be rectified by using another bit
> > of the second gpio cell? The low bit is used for active-high/active-low,
> > so you could use the second bit for irq/input.
> >
> Do you mean #gpio-cells property and using the high bit of the second
> bit for irq/input  ?

Yes, that would be an option.

	Arnd

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

[Y Vo]

On Mon, Sep 14, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 12 September 2015 12:55:55 Y Vo wrote:
>> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 11 September 2015 22:06:58 Y Vo wrote:
>> >> On Fri, Sep 11, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Friday 11 September 2015 21:23:31 Y Vo wrote:
>> >> >> Is that possible if we change to this approach:
>> >> >> - Keep all GPIO as GPIO mode by default.
>> >> >> - If anyone want to use them as interrupt, they must add the interrupt
>> >> >> node to DT specific-board below:
>> >> >>
>> >> >> Example to configure GPIO_DS11, 12, 13 as external irq:
>> >> >>
>> >> >> &sbgpio {
>> >> >>         interrupts = <0x0 0x2b 0x1>,         /* GPIO_DS11 */
>> >> >>                         <0x0 0x2c 0x1>,         /* GPIO_DS12 */
>> >> >>                         <0x0 0x2d 0x1>;         /* GPIO_DS13 */
>> >> >
>> >> > That is exactly the same as you have in your patch. You should
>> >> > always list all interrupts that the gpio controller is wired
>> >> > to, whether they are used or not.
>> >> >
>> >> > Ideally, the driver just decides at runtime how to wire that
>> >> > mux, based on how the gpio is configured by the client driver.
>> >> > If the client asks for an interrupt (with gpio_to_irq) and is
>> >> > it is configured as an input, then you connect it to the GIC,
>> >> > otherwise you leave it to be handled by the gpio chip itself.
>> >> > Any reason you can't do that?
>> >> >
>> >> As I just explained, the GPIO Standby doesn't support the interrupt.
>> >> Both interrupts are came from GIC, so if the client asks for an
>> >> interrupt, it will go to the GIC driver (not GPIO Standby driver), so
>> >> that's my trouble. We can not implement the code to configured the
>> >> GPIO standby at GIC driver.
>> >
>> > So you are saying we merged a binding that is incapable of describing
>> > the hardware? If that is the case, we may have to go back to
>> > the drawing board and come up with a working binding.
>> >
>> > One part that I don't understand yet is how the driver handles
>> > irqs to start with, as it does not register a nested irq domain
>> > or irqchip, so I'm guessing that client drivers cannot actually
>> > use the IRQ functionality. Is that right?
>> For our case the irq_domain is in irq-gic driver, the GPIO driver only
>> is responsible to wire 2 the external irq of GIC to GPIO pin.
>>
>> Example for configure GPIO_DS13 as interrupt and use as button with
>> the current gpio driver:
>>         gpio-keys {
>>                 compatible = "gpio-keys";
>>                 button@1 {
>>                         label = "POWER";
>>                         linux,code = <116>;
>>                         linux,input-type = <0x1>;
>>                         interrupts = <0x0 0x2d 0x1>;
>>                 };
>>         };
>
> Wait, this looks wrong: the gpio driver doesn't actually see
> the connection here and won't be able to configure the interrupt
> correctly. The interrupt is already owned by the gpio driver, so
> you cannot use it in the button node.

In summary:
- Our GPIO doesn't support interrupt controller.
- There are 6 pins which used the external interrupt from GIC, so all
setup for those irqs are from gic driver. The GPIO driver only
configure to wire those lines.

For your concern:
- That's correct: if we use that defined, the gpio driver never saw
the connection here (That's why it already is configued at the
beginning).
- At the first time, we tried to use the define: <&sbgpio 13 1>, it
means using the GPIO_DS13, it will go into the GPIO driver to setup,
but there is another problem which I have sent out to all of you:
+ It will go into gpio_keys_setup_key (gpio_keys.c driver) function,
then set the irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
but the gic only support IRQ_TYPE_LEVEL_HIGH && IRQF_TRIGGER_RISING,
so it always returns failed at gpio_keys_setup_key function. Please
see the gic_set_type at gic driver.

static int gic_set_type(struct irq_data *d, unsigned int type)
{
        void __iomem *base = gic_dist_base(d);
        unsigned int gicirq = gic_irq(d);

        /* Interrupt configuration for SGIs can't be changed */
        if (gicirq < 16)
                return -EINVAL;

        /* SPIs have restrictions on the supported types */
        if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
                            type != IRQ_TYPE_EDGE_RISING)
                return -EINVAL;

        return gic_configure_irq(gicirq, type, base, NULL);
}
+ Another issue: in order gpio_key works it needs the status of GPIO.
For our chip, when the GPIO is configued as interrupt, we need to
access to GIC register to read the real status, it is not acceptable
to implement accessing GIC registers at gpio driver. The function
irq_get_irqchip_state(..) also doesn't work in our chip too. Because
it needs to access different offset.

>
>> >
>> > It also seems to me that the binding cannot distinguish between a
>> > line configured as an input and one that is configured as an
>> > interrupt, which are for other gpio chips the same thing, but
>> > not on this one.  Could this be rectified by using another bit
>> > of the second gpio cell? The low bit is used for active-high/active-low,
>> > so you could use the second bit for irq/input.
>> >
>> Do you mean #gpio-cells property and using the high bit of the second
>> bit for irq/input  ?
>
> Yes, that would be an option.
I will look into it.

Is there possible if:
- Keep GPIO8..GPIO as interrupt by default.
- Anyone want to use these GPIO pins as GPIO, we will re-configure
them to GPIO mode ?


>
>         Arnd

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

[Arnd Bergmann]

On Monday 14 September 2015 16:39:43 Y Vo wrote:
> On Mon, Sep 14, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Saturday 12 September 2015 12:55:55 Y Vo wrote:
> >> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Friday 11 September 2015 22:06:58 Y Vo wrote:
> >>
> >> Example for configure GPIO_DS13 as interrupt and use as button with
> >> the current gpio driver:
> >>         gpio-keys {
> >>                 compatible = "gpio-keys";
> >>                 button@1 {
> >>                         label = "POWER";
> >>                         linux,code = <116>;
> >>                         linux,input-type = <0x1>;
> >>                         interrupts = <0x0 0x2d 0x1>;
> >>                 };
> >>         };
> >
> > Wait, this looks wrong: the gpio driver doesn't actually see
> > the connection here and won't be able to configure the interrupt
> > correctly. The interrupt is already owned by the gpio driver, so
> > you cannot use it in the button node.
> 
> In summary:
> - Our GPIO doesn't support interrupt controller.
> - There are 6 pins which used the external interrupt from GIC, so all
> setup for those irqs are from gic driver. The GPIO driver only
> configure to wire those lines.
> 
> For your concern:
> - That's correct: if we use that defined, the gpio driver never saw
> the connection here (That's why it already is configued at the
> beginning).
> - At the first time, we tried to use the define: <&sbgpio 13 1>, it
> means using the GPIO_DS13, it will go into the GPIO driver to setup,
> but there is another problem which I have sent out to all of you:
> + It will go into gpio_keys_setup_key (gpio_keys.c driver) function,
> then set the irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> but the gic only support IRQ_TYPE_LEVEL_HIGH && IRQF_TRIGGER_RISING,
> so it always returns failed at gpio_keys_setup_key function. Please
> see the gic_set_type at gic driver.

Hmm, I see now how the event handling in the gpio-keys driver differs
between irq mode and gpio mode, where gpio mode relies on getting
a separate event for the release. This is certainly something that
could be changed in the gpio-keys driver as an extension, but that
seems to be what Laxman Dewangan did when he introduced the irq-mode.

> static int gic_set_type(struct irq_data *d, unsigned int type)
> {
>         void __iomem *base = gic_dist_base(d);
>         unsigned int gicirq = gic_irq(d);
> 
>         /* Interrupt configuration for SGIs can't be changed */
>         if (gicirq < 16)
>                 return -EINVAL;
> 
>         /* SPIs have restrictions on the supported types */
>         if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
>                             type != IRQ_TYPE_EDGE_RISING)
>                 return -EINVAL;
> 
>         return gic_configure_irq(gicirq, type, base, NULL);
> }
> + Another issue: in order gpio_key works it needs the status of GPIO.
> For our chip, when the GPIO is configued as interrupt, we need to
> access to GIC register to read the real status, it is not acceptable
> to implement accessing GIC registers at gpio driver. The function
> irq_get_irqchip_state(..) also doesn't work in our chip too. Because
> it needs to access different offset.

I thought we had solved that problem long ago when you first
submitted the driver.

Did 1b7047edfcfb25 ("genirq: Allow the irqchip state of an IRQ to be
save/restored") not address the problem for you? You were on
Cc to that patch and should have spoken up when the code that was
merged was not sufficient.

> >> > It also seems to me that the binding cannot distinguish between a
> >> > line configured as an input and one that is configured as an
> >> > interrupt, which are for other gpio chips the same thing, but
> >> > not on this one.  Could this be rectified by using another bit
> >> > of the second gpio cell? The low bit is used for active-high/active-low,
> >> > so you could use the second bit for irq/input.
> >> >
> >> Do you mean #gpio-cells property and using the high bit of the second
> >> bit for irq/input  ?
> >
> > Yes, that would be an option.
> I will look into it.
> 
> Is there possible if:
> - Keep GPIO8..GPIO as interrupt by default.
> - Anyone want to use these GPIO pins as GPIO, we will re-configure
> them to GPIO mode ?

That's not perfect but better than the patch you sent here.
The main disadvantage is that you end up with two references
to the same IRQ. It can still work, but only as long as nothing
tries to walk the entire DT to parse all the interrupts properties.

It would be ok for gpio-keys, as that does not need both the state
and the event together, but for other gpio users, you still need a
working driver that supports reading the state and getting an
interrupt.

	Arnd

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

[Y Vo]

On Mon, Sep 14, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 14 September 2015 16:39:43 Y Vo wrote:
>> On Mon, Sep 14, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Saturday 12 September 2015 12:55:55 Y Vo wrote:
>> >> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Friday 11 September 2015 22:06:58 Y Vo wrote:
>> >>
>> >> Example for configure GPIO_DS13 as interrupt and use as button with
>> >> the current gpio driver:
>> >>         gpio-keys {
>> >>                 compatible = "gpio-keys";
>> >>                 button@1 {
>> >>                         label = "POWER";
>> >>                         linux,code = <116>;
>> >>                         linux,input-type = <0x1>;
>> >>                         interrupts = <0x0 0x2d 0x1>;
>> >>                 };
>> >>         };
>> >
>> > Wait, this looks wrong: the gpio driver doesn't actually see
>> > the connection here and won't be able to configure the interrupt
>> > correctly. The interrupt is already owned by the gpio driver, so
>> > you cannot use it in the button node.
>>
>> In summary:
>> - Our GPIO doesn't support interrupt controller.
>> - There are 6 pins which used the external interrupt from GIC, so all
>> setup for those irqs are from gic driver. The GPIO driver only
>> configure to wire those lines.
>>
>> For your concern:
>> - That's correct: if we use that defined, the gpio driver never saw
>> the connection here (That's why it already is configued at the
>> beginning).
>> - At the first time, we tried to use the define: <&sbgpio 13 1>, it
>> means using the GPIO_DS13, it will go into the GPIO driver to setup,
>> but there is another problem which I have sent out to all of you:
>> + It will go into gpio_keys_setup_key (gpio_keys.c driver) function,
>> then set the irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> but the gic only support IRQ_TYPE_LEVEL_HIGH && IRQF_TRIGGER_RISING,
>> so it always returns failed at gpio_keys_setup_key function. Please
>> see the gic_set_type at gic driver.
>
> Hmm, I see now how the event handling in the gpio-keys driver differs
> between irq mode and gpio mode, where gpio mode relies on getting
> a separate event for the release. This is certainly something that
> could be changed in the gpio-keys driver as an extension, but that
> seems to be what Laxman Dewangan did when he introduced the irq-mode.
>
>> static int gic_set_type(struct irq_data *d, unsigned int type)
>> {
>>         void __iomem *base = gic_dist_base(d);
>>         unsigned int gicirq = gic_irq(d);
>>
>>         /* Interrupt configuration for SGIs can't be changed */
>>         if (gicirq < 16)
>>                 return -EINVAL;
>>
>>         /* SPIs have restrictions on the supported types */
>>         if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
>>                             type != IRQ_TYPE_EDGE_RISING)
>>                 return -EINVAL;
>>
>>         return gic_configure_irq(gicirq, type, base, NULL);
>> }
>> + Another issue: in order gpio_key works it needs the status of GPIO.
>> For our chip, when the GPIO is configued as interrupt, we need to
>> access to GIC register to read the real status, it is not acceptable
>> to implement accessing GIC registers at gpio driver. The function
>> irq_get_irqchip_state(..) also doesn't work in our chip too. Because
>> it needs to access different offset.
>
> I thought we had solved that problem long ago when you first
> submitted the driver.
>
> Did 1b7047edfcfb25 ("genirq: Allow the irqchip state of an IRQ to be
> save/restored") not address the problem for you? You were on
> Cc to that patch and should have spoken up when the code that was
> merged was not sufficient.

Yes, I am in this mail-list too, but I also had a issue on this, I
think you are still in my submitted for this.
Currently, irq_get|set_irqchip_state(..) supports access to
GIC_DIST_ENABLE_SET, GIC_DIST_ACTIVE_SET, GIC_DIST_PENDING_SET. But
our hw only has the valid value at SPISR register ("[PATCH v4 2/3]
irqchip: GIC: Add support for irq_{get,set}_irqchip_state"), so I
still can not use it.

>
>> >> > It also seems to me that the binding cannot distinguish between a
>> >> > line configured as an input and one that is configured as an
>> >> > interrupt, which are for other gpio chips the same thing, but
>> >> > not on this one.  Could this be rectified by using another bit
>> >> > of the second gpio cell? The low bit is used for active-high/active-low,
>> >> > so you could use the second bit for irq/input.
>> >> >
>> >> Do you mean #gpio-cells property and using the high bit of the second
>> >> bit for irq/input  ?
>> >
>> > Yes, that would be an option.
>> I will look into it.
>>
>> Is there possible if:
>> - Keep GPIO8..GPIO as interrupt by default.
>> - Anyone want to use these GPIO pins as GPIO, we will re-configure
>> them to GPIO mode ?
>
> That's not perfect but better than the patch you sent here.
> The main disadvantage is that you end up with two references
> to the same IRQ. It can still work, but only as long as nothing
> tries to walk the entire DT to parse all the interrupts properties.
>
Let me think how.

> It would be ok for gpio-keys, as that does not need both the state
> and the event together, but for other gpio users, you still need a
> working driver that supports reading the state and getting an
> interrupt.
>
In irq mode, if I reconfigured that gpio pin to gpio mode, then
reading -> the value is valid.
Could I do that way badly ? It means switch to gpio mode to read
value, then switch back to  irq mode.


>         Arnd

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

[Arnd Bergmann]

On Monday 14 September 2015 22:06:15 Y Vo wrote:
> >
> > Did 1b7047edfcfb25 ("genirq: Allow the irqchip state of an IRQ to be
> > save/restored") not address the problem for you? You were on
> > Cc to that patch and should have spoken up when the code that was
> > merged was not sufficient.
> 
> Yes, I am in this mail-list too, but I also had a issue on this, I
> think you are still in my submitted for this.
> Currently, irq_get|set_irqchip_state(..) supports access to
> GIC_DIST_ENABLE_SET, GIC_DIST_ACTIVE_SET, GIC_DIST_PENDING_SET. But
> our hw only has the valid value at SPISR register ("[PATCH v4 2/3]
> irqchip: GIC: Add support for irq_{get,set}_irqchip_state"), so I
> still can not use it.

Ok.

> >> >> > It also seems to me that the binding cannot distinguish between a
> >> >> > line configured as an input and one that is configured as an
> >> >> > interrupt, which are for other gpio chips the same thing, but
> >> >> > not on this one.  Could this be rectified by using another bit
> >> >> > of the second gpio cell? The low bit is used for active-high/active-low,
> >> >> > so you could use the second bit for irq/input.
> >> >> >
> >> >> Do you mean #gpio-cells property and using the high bit of the second
> >> >> bit for irq/input  ?
> >> >
> >> > Yes, that would be an option.
> >> I will look into it.
> >>
> >> Is there possible if:
> >> - Keep GPIO8..GPIO as interrupt by default.
> >> - Anyone want to use these GPIO pins as GPIO, we will re-configure
> >> them to GPIO mode ?
> >
> > That's not perfect but better than the patch you sent here.
> > The main disadvantage is that you end up with two references
> > to the same IRQ. It can still work, but only as long as nothing
> > tries to walk the entire DT to parse all the interrupts properties.
> >
> Let me think how.
> 
> > It would be ok for gpio-keys, as that does not need both the state
> > and the event together, but for other gpio users, you still need a
> > working driver that supports reading the state and getting an
> > interrupt.
> >
> In irq mode, if I reconfigured that gpio pin to gpio mode, then
> reading -> the value is valid.
> Could I do that way badly ? It means switch to gpio mode to read
> value, then switch back to  irq mode.

I don't see any downsides of this at the moment, other than it being
a bit slow. As long as we don't try to do any high-speed communication
over this gpio line, that seems like the best workaround given the
various constraints of the hardware.

	Arnd

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

[Marc Zyngier]

On 14/09/15 16:06, Y Vo wrote:
> On Mon, Sep 14, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 14 September 2015 16:39:43 Y Vo wrote:
>>> On Mon, Sep 14, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Saturday 12 September 2015 12:55:55 Y Vo wrote:
>>>>> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>> On Friday 11 September 2015 22:06:58 Y Vo wrote:
>>>>>
>>>>> Example for configure GPIO_DS13 as interrupt and use as button with
>>>>> the current gpio driver:
>>>>>         gpio-keys {
>>>>>                 compatible = "gpio-keys";
>>>>>                 button@1 {
>>>>>                         label = "POWER";
>>>>>                         linux,code = <116>;
>>>>>                         linux,input-type = <0x1>;
>>>>>                         interrupts = <0x0 0x2d 0x1>;
>>>>>                 };
>>>>>         };
>>>>
>>>> Wait, this looks wrong: the gpio driver doesn't actually see
>>>> the connection here and won't be able to configure the interrupt
>>>> correctly. The interrupt is already owned by the gpio driver, so
>>>> you cannot use it in the button node.
>>>
>>> In summary:
>>> - Our GPIO doesn't support interrupt controller.
>>> - There are 6 pins which used the external interrupt from GIC, so all
>>> setup for those irqs are from gic driver. The GPIO driver only
>>> configure to wire those lines.
>>>
>>> For your concern:
>>> - That's correct: if we use that defined, the gpio driver never saw
>>> the connection here (That's why it already is configued at the
>>> beginning).
>>> - At the first time, we tried to use the define: <&sbgpio 13 1>, it
>>> means using the GPIO_DS13, it will go into the GPIO driver to setup,
>>> but there is another problem which I have sent out to all of you:
>>> + It will go into gpio_keys_setup_key (gpio_keys.c driver) function,
>>> then set the irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>> but the gic only support IRQ_TYPE_LEVEL_HIGH && IRQF_TRIGGER_RISING,
>>> so it always returns failed at gpio_keys_setup_key function. Please
>>> see the gic_set_type at gic driver.
>>
>> Hmm, I see now how the event handling in the gpio-keys driver differs
>> between irq mode and gpio mode, where gpio mode relies on getting
>> a separate event for the release. This is certainly something that
>> could be changed in the gpio-keys driver as an extension, but that
>> seems to be what Laxman Dewangan did when he introduced the irq-mode.
>>
>>> static int gic_set_type(struct irq_data *d, unsigned int type)
>>> {
>>>         void __iomem *base = gic_dist_base(d);
>>>         unsigned int gicirq = gic_irq(d);
>>>
>>>         /* Interrupt configuration for SGIs can't be changed */
>>>         if (gicirq < 16)
>>>                 return -EINVAL;
>>>
>>>         /* SPIs have restrictions on the supported types */
>>>         if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
>>>                             type != IRQ_TYPE_EDGE_RISING)
>>>                 return -EINVAL;
>>>
>>>         return gic_configure_irq(gicirq, type, base, NULL);
>>> }
>>> + Another issue: in order gpio_key works it needs the status of GPIO.
>>> For our chip, when the GPIO is configued as interrupt, we need to
>>> access to GIC register to read the real status, it is not acceptable
>>> to implement accessing GIC registers at gpio driver. The function
>>> irq_get_irqchip_state(..) also doesn't work in our chip too. Because
>>> it needs to access different offset.
>>
>> I thought we had solved that problem long ago when you first
>> submitted the driver.
>>
>> Did 1b7047edfcfb25 ("genirq: Allow the irqchip state of an IRQ to be
>> save/restored") not address the problem for you? You were on
>> Cc to that patch and should have spoken up when the code that was
>> merged was not sufficient.
> 
> Yes, I am in this mail-list too, but I also had a issue on this, I
> think you are still in my submitted for this.
> Currently, irq_get|set_irqchip_state(..) supports access to
> GIC_DIST_ENABLE_SET, GIC_DIST_ACTIVE_SET, GIC_DIST_PENDING_SET. But
> our hw only has the valid value at SPISR register ("[PATCH v4 2/3]
> irqchip: GIC: Add support for irq_{get,set}_irqchip_state"), so I
> still can not use it.

And I asked Feng Kan to explain *why* it doesn't work, but nobody ever
bothered giving me a straight answer on that:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg892972.html

The GIC400 TRM explicitly states that reading GICD_SPISRn is the same as
reading GICD_ICPENDRn. if that doesn't work for you please explain why.

If I can get a reasonable explanation on *why* it doesn't work, then we
can look at having an X-Gene specific workaround.

Thanks,

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

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

[Y Vo]

On Mon, Sep 14, 2015 at 10:23 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 14/09/15 16:06, Y Vo wrote:
>> On Mon, Sep 14, 2015 at 9:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Monday 14 September 2015 16:39:43 Y Vo wrote:
>>>> On Mon, Sep 14, 2015 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Saturday 12 September 2015 12:55:55 Y Vo wrote:
>>>>>> On Fri, Sep 11, 2015 at 11:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>>> On Friday 11 September 2015 22:06:58 Y Vo wrote:
>>>>>>
>>>>>> Example for configure GPIO_DS13 as interrupt and use as button with
>>>>>> the current gpio driver:
>>>>>>         gpio-keys {
>>>>>>                 compatible = "gpio-keys";
>>>>>>                 button@1 {
>>>>>>                         label = "POWER";
>>>>>>                         linux,code = <116>;
>>>>>>                         linux,input-type = <0x1>;
>>>>>>                         interrupts = <0x0 0x2d 0x1>;
>>>>>>                 };
>>>>>>         };
>>>>>
>>>>> Wait, this looks wrong: the gpio driver doesn't actually see
>>>>> the connection here and won't be able to configure the interrupt
>>>>> correctly. The interrupt is already owned by the gpio driver, so
>>>>> you cannot use it in the button node.
>>>>
>>>> In summary:
>>>> - Our GPIO doesn't support interrupt controller.
>>>> - There are 6 pins which used the external interrupt from GIC, so all
>>>> setup for those irqs are from gic driver. The GPIO driver only
>>>> configure to wire those lines.
>>>>
>>>> For your concern:
>>>> - That's correct: if we use that defined, the gpio driver never saw
>>>> the connection here (That's why it already is configued at the
>>>> beginning).
>>>> - At the first time, we tried to use the define: <&sbgpio 13 1>, it
>>>> means using the GPIO_DS13, it will go into the GPIO driver to setup,
>>>> but there is another problem which I have sent out to all of you:
>>>> + It will go into gpio_keys_setup_key (gpio_keys.c driver) function,
>>>> then set the irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>> but the gic only support IRQ_TYPE_LEVEL_HIGH && IRQF_TRIGGER_RISING,
>>>> so it always returns failed at gpio_keys_setup_key function. Please
>>>> see the gic_set_type at gic driver.
>>>
>>> Hmm, I see now how the event handling in the gpio-keys driver differs
>>> between irq mode and gpio mode, where gpio mode relies on getting
>>> a separate event for the release. This is certainly something that
>>> could be changed in the gpio-keys driver as an extension, but that
>>> seems to be what Laxman Dewangan did when he introduced the irq-mode.
>>>
>>>> static int gic_set_type(struct irq_data *d, unsigned int type)
>>>> {
>>>>         void __iomem *base = gic_dist_base(d);
>>>>         unsigned int gicirq = gic_irq(d);
>>>>
>>>>         /* Interrupt configuration for SGIs can't be changed */
>>>>         if (gicirq < 16)
>>>>                 return -EINVAL;
>>>>
>>>>         /* SPIs have restrictions on the supported types */
>>>>         if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH &&
>>>>                             type != IRQ_TYPE_EDGE_RISING)
>>>>                 return -EINVAL;
>>>>
>>>>         return gic_configure_irq(gicirq, type, base, NULL);
>>>> }
>>>> + Another issue: in order gpio_key works it needs the status of GPIO.
>>>> For our chip, when the GPIO is configued as interrupt, we need to
>>>> access to GIC register to read the real status, it is not acceptable
>>>> to implement accessing GIC registers at gpio driver. The function
>>>> irq_get_irqchip_state(..) also doesn't work in our chip too. Because
>>>> it needs to access different offset.
>>>
>>> I thought we had solved that problem long ago when you first
>>> submitted the driver.
>>>
>>> Did 1b7047edfcfb25 ("genirq: Allow the irqchip state of an IRQ to be
>>> save/restored") not address the problem for you? You were on
>>> Cc to that patch and should have spoken up when the code that was
>>> merged was not sufficient.
>>
>> Yes, I am in this mail-list too, but I also had a issue on this, I
>> think you are still in my submitted for this.
>> Currently, irq_get|set_irqchip_state(..) supports access to
>> GIC_DIST_ENABLE_SET, GIC_DIST_ACTIVE_SET, GIC_DIST_PENDING_SET. But
>> our hw only has the valid value at SPISR register ("[PATCH v4 2/3]
>> irqchip: GIC: Add support for irq_{get,set}_irqchip_state"), so I
>> still can not use it.
>
> And I asked Feng Kan to explain *why* it doesn't work, but nobody ever
> bothered giving me a straight answer on that:
>
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg892972.html
>
> The GIC400 TRM explicitly states that reading GICD_SPISRn is the same as
> reading GICD_ICPENDRn. if that doesn't work for you please explain why.
>
> If I can get a reasonable explanation on *why* it doesn't work, then we
> can look at having an X-Gene specific workaround.
>
I am checking and let you know soon.

Thanks

> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...