[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Gregory CLEMENT]

From: Ken Ma <make@marvell.com>

Current edge both type gpio irqs which need to swap polarity in each
interrupt are not supported, this patch adds edge both type gpio irq
support.

Signed-off-by: Ken Ma <make@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 71b944748304..4e8d836a8c6f 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -576,6 +576,19 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
 	case IRQ_TYPE_EDGE_FALLING:
 		val |= (BIT(d->hwirq % GPIO_PER_REG));
 		break;
+	case IRQ_TYPE_EDGE_BOTH: {
+		u32 in_val, in_reg = INPUT_VAL;
+
+		armada_37xx_irq_update_reg(&in_reg, d);
+		regmap_read(info->regmap, in_reg, &in_val);
+
+		/* Set initial polarity based on current input level. */
+		if (in_val & d->mask)
+			val |= d->mask;		/* falling */
+		else
+			val &= ~d->mask;	/* rising */
+		break;
+	}
 	default:
 		spin_unlock_irqrestore(&info->irq_lock, flags);
 		return -EINVAL;
@@ -586,6 +599,40 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static int armada_37xx_edge_both_irq_swap_pol(struct armada_37xx_pinctrl *info,
+					     u32 pin_idx)
+{
+	u32 reg_idx = pin_idx / GPIO_PER_REG;
+	u32 bit_num = pin_idx % GPIO_PER_REG;
+	u32 p, l, ret;
+	unsigned long flags;
+
+	regmap_read(info->regmap, INPUT_VAL + 4*reg_idx, &l);
+
+	spin_lock_irqsave(&info->irq_lock, flags);
+	p = readl(info->base + IRQ_POL + 4 * reg_idx);
+	if ((p ^ l) & (1 << bit_num)) {
+		/*
+		 * For the gpios which are used for both-edge irqs, when their
+		 * interrupts happen, their input levels are changed,
+		 * yet their interrupt polarities are kept in old values, we
+		 * should synchronize their interrupt polarities; for example,
+		 * at first a gpio's input level is low and its interrupt
+		 * polarity control is "Detect rising edge", then the gpio has
+		 * a interrupt , its level turns to high, we should change its
+		 * polarity control to "Detect falling edge" correspondingly.
+		 */
+		p ^= 1 << bit_num;
+		writel(p, info->base + IRQ_POL + 4 * reg_idx);
+		ret = 0;
+	} else {
+		/* Spurious irq */
+		ret = -1;
+	}
+
+	spin_unlock_irqrestore(&info->irq_lock, flags);
+	return ret;
+}
 
 static void armada_37xx_irq_handler(struct irq_desc *desc)
 {
@@ -609,6 +656,23 @@ static void armada_37xx_irq_handler(struct irq_desc *desc)
 			u32 hwirq = ffs(status) - 1;
 			u32 virq = irq_find_mapping(d, hwirq +
 						     i * GPIO_PER_REG);
+			u32 t = irq_get_trigger_type(virq);
+
+			if ((t & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) {
+				/* Swap polarity (race with GPIO line) */
+				if (armada_37xx_edge_both_irq_swap_pol(info,
+					hwirq + i * GPIO_PER_REG)) {
+					/*
+					 * For spurious irq, which gpio level
+					 * is not as expected after incoming
+					 * edge, just ack the gpio irq.
+					 */
+					writel(1 << hwirq,
+					       info->base +
+					       IRQ_STATUS + 4 * i);
+					continue;
+				}
+			}
 
 			generic_handle_irq(virq);
 
-- 
2.14.2

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Gregory CLEMENT]

From: Ken Ma <make@marvell.com>

Current edge both type gpio irqs which need to swap polarity in each
interrupt are not supported, this patch adds edge both type gpio irq
support.

Signed-off-by: Ken Ma <make@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 71b944748304..4e8d836a8c6f 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -576,6 +576,19 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
 	case IRQ_TYPE_EDGE_FALLING:
 		val |= (BIT(d->hwirq % GPIO_PER_REG));
 		break;
+	case IRQ_TYPE_EDGE_BOTH: {
+		u32 in_val, in_reg = INPUT_VAL;
+
+		armada_37xx_irq_update_reg(&in_reg, d);
+		regmap_read(info->regmap, in_reg, &in_val);
+
+		/* Set initial polarity based on current input level. */
+		if (in_val & d->mask)
+			val |= d->mask;		/* falling */
+		else
+			val &= ~d->mask;	/* rising */
+		break;
+	}
 	default:
 		spin_unlock_irqrestore(&info->irq_lock, flags);
 		return -EINVAL;
@@ -586,6 +599,40 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static int armada_37xx_edge_both_irq_swap_pol(struct armada_37xx_pinctrl *info,
+					     u32 pin_idx)
+{
+	u32 reg_idx = pin_idx / GPIO_PER_REG;
+	u32 bit_num = pin_idx % GPIO_PER_REG;
+	u32 p, l, ret;
+	unsigned long flags;
+
+	regmap_read(info->regmap, INPUT_VAL + 4*reg_idx, &l);
+
+	spin_lock_irqsave(&info->irq_lock, flags);
+	p = readl(info->base + IRQ_POL + 4 * reg_idx);
+	if ((p ^ l) & (1 << bit_num)) {
+		/*
+		 * For the gpios which are used for both-edge irqs, when their
+		 * interrupts happen, their input levels are changed,
+		 * yet their interrupt polarities are kept in old values, we
+		 * should synchronize their interrupt polarities; for example,
+		 *@first a gpio's input level is low and its interrupt
+		 * polarity control is "Detect rising edge", then the gpio has
+		 * a interrupt , its level turns to high, we should change its
+		 * polarity control to "Detect falling edge" correspondingly.
+		 */
+		p ^= 1 << bit_num;
+		writel(p, info->base + IRQ_POL + 4 * reg_idx);
+		ret = 0;
+	} else {
+		/* Spurious irq */
+		ret = -1;
+	}
+
+	spin_unlock_irqrestore(&info->irq_lock, flags);
+	return ret;
+}
 
 static void armada_37xx_irq_handler(struct irq_desc *desc)
 {
@@ -609,6 +656,23 @@ static void armada_37xx_irq_handler(struct irq_desc *desc)
 			u32 hwirq = ffs(status) - 1;
 			u32 virq = irq_find_mapping(d, hwirq +
 						     i * GPIO_PER_REG);
+			u32 t = irq_get_trigger_type(virq);
+
+			if ((t & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) {
+				/* Swap polarity (race with GPIO line) */
+				if (armada_37xx_edge_both_irq_swap_pol(info,
+					hwirq + i * GPIO_PER_REG)) {
+					/*
+					 * For spurious irq, which gpio level
+					 * is not as expected after incoming
+					 * edge, just ack the gpio irq.
+					 */
+					writel(1 << hwirq,
+					       info->base +
+					       IRQ_STATUS + 4 * i);
+					continue;
+				}
+			}
 
 			generic_handle_irq(virq);
 
-- 
2.14.2

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote:
> From: Ken Ma <make@marvell.com>
> 
> Current edge both type gpio irqs which need to swap polarity in each
> interrupt are not supported, this patch adds edge both type gpio irq
> support.

So is the assumption here that you can handle the interrupt and flip
the edge, faster than it takes the signal to change?

If the software is too slow, you loose the following edge? And you
might loose the edge after that as well, since the software will at
some point handle the interrupt and reconfigure the edge, potentially
for the wrong edge?

Or am i missing something which makes this race free?

   Andrew

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote:
> From: Ken Ma <make@marvell.com>
> 
> Current edge both type gpio irqs which need to swap polarity in each
> interrupt are not supported, this patch adds edge both type gpio irq
> support.

So is the assumption here that you can handle the interrupt and flip
the edge, faster than it takes the signal to change?

If the software is too slow, you loose the following edge? And you
might loose the edge after that as well, since the software will at
some point handle the interrupt and reconfigure the edge, potentially
for the wrong edge?

Or am i missing something which makes this race free?

   Andrew

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., oct. 19 2017, Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote:
>> From: Ken Ma <make@marvell.com>
>> 
>> Current edge both type gpio irqs which need to swap polarity in each
>> interrupt are not supported, this patch adds edge both type gpio irq
>> support.
>
> So is the assumption here that you can handle the interrupt and flip
> the edge, faster than it takes the signal to change?
>
> If the software is too slow, you loose the following edge? And you
> might loose the edge after that as well, since the software will at
> some point handle the interrupt and reconfigure the edge, potentially
> for the wrong edge?
>
> Or am i missing something which makes this race free?

To put more context, this patch is just the port of what is currently
done in gpio-mvebu.c.

I needed this support for the SD detect pin and didn't have any issue
because it was not an intensive use at all.

Going back to your concern, yes if the interrupt frequency is too high
we might loose 2 events. But the same things could happen with a single
edge, we can still miss an event.  But I agree that with the "both edge"
mode then we will loose 2 events instead of one.

It is the problem for the edge interrupt in general. So as soon as you
use edge interrupt you must be ready to loose some interrupt.

In the end I don't think it is a problem.

Gregory


>
>    Andrew

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Gregory CLEMENT]

Hi Andrew,
 
 On jeu., oct. 19 2017, Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Oct 19, 2017 at 03:10:03PM +0200, Gregory CLEMENT wrote:
>> From: Ken Ma <make@marvell.com>
>> 
>> Current edge both type gpio irqs which need to swap polarity in each
>> interrupt are not supported, this patch adds edge both type gpio irq
>> support.
>
> So is the assumption here that you can handle the interrupt and flip
> the edge, faster than it takes the signal to change?
>
> If the software is too slow, you loose the following edge? And you
> might loose the edge after that as well, since the software will at
> some point handle the interrupt and reconfigure the edge, potentially
> for the wrong edge?
>
> Or am i missing something which makes this race free?

To put more context, this patch is just the port of what is currently
done in gpio-mvebu.c.

I needed this support for the SD detect pin and didn't have any issue
because it was not an intensive use at all.

Going back to your concern, yes if the interrupt frequency is too high
we might loose 2 events. But the same things could happen with a single
edge, we can still miss an event.  But I agree that with the "both edge"
mode then we will loose 2 events instead of one.

It is the problem for the edge interrupt in general. So as soon as you
use edge interrupt you must be ready to loose some interrupt.

In the end I don't think it is a problem.

Gregory


>
>    Andrew

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

Hi Gregory

> In the end I don't think it is a problem.

I think it should be clearly documented somewhere. Hardware which can
do both edges in hardware won't have this problem. If i'm using this
generic feature, i want an idea if it is mostly going to work, or
always going to work.

   Andrew

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

Hi Gregory

> In the end I don't think it is a problem.

I think it should be clearly documented somewhere. Hardware which can
do both edges in hardware won't have this problem. If i'm using this
generic feature, i want an idea if it is mostly going to work, or
always going to work.

   Andrew

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Thu, Oct 19, 2017 at 3:10 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> From: Ken Ma <make@marvell.com>
>
> Current edge both type gpio irqs which need to swap polarity in each
> interrupt are not supported, this patch adds edge both type gpio irq
> support.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Patch applied.

The discussion here is interesting, it is customary for GPIO drivers
to implement double-edge detection emulation by swapping the
edge detector around like this.

It might be possible to collect some generic information about
this in the Documentation/gpio/driver.txt document.

Yours,
Linus Walleij

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Thu, Oct 19, 2017 at 3:10 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:

> From: Ken Ma <make@marvell.com>
>
> Current edge both type gpio irqs which need to swap polarity in each
> interrupt are not supported, this patch adds edge both type gpio irq
> support.
>
> Signed-off-by: Ken Ma <make@marvell.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Patch applied.

The discussion here is interesting, it is customary for GPIO drivers
to implement double-edge detection emulation by swapping the
edge detector around like this.

It might be possible to collect some generic information about
this in the Documentation/gpio/driver.txt document.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

> Patch applied.
> 
> The discussion here is interesting, it is customary for GPIO drivers
> to implement double-edge detection emulation by swapping the
> edge detector around like this.

Hi Linus

I was not aware this was customary.

> It might be possible to collect some generic information about
> this in the Documentation/gpio/driver.txt document.

Yes, i think it should be documented somewhere. Even in the use case
here, detecting an SD card being inserted/removed, you could get some
bounce on the microswitch, miss an edge, and be in the wrong state.

       Andrew

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Andrew Lunn]

> Patch applied.
> 
> The discussion here is interesting, it is customary for GPIO drivers
> to implement double-edge detection emulation by swapping the
> edge detector around like this.

Hi Linus

I was not aware this was customary.

> It might be possible to collect some generic information about
> this in the Documentation/gpio/driver.txt document.

Yes, i think it should be documented somewhere. Even in the use case
here, detecting an SD card being inserted/removed, you could get some
bounce on the microswitch, miss an edge, and be in the wrong state.

       Andrew

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Tue, Oct 31, 2017 at 2:16 PM, Andrew Lunn <andrew@lunn.ch> wrote:

>> The discussion here is interesting, it is customary for GPIO drivers
>> to implement double-edge detection emulation by swapping the
>> edge detector around like this.
>
> Hi Linus
>
> I was not aware this was customary.
>
>> It might be possible to collect some generic information about
>> this in the Documentation/gpio/driver.txt document.
>
> Yes, i think it should be documented somewhere. Even in the use case
> here, detecting an SD card being inserted/removed, you could get some
> bounce on the microswitch, miss an edge, and be in the wrong state.

Indeed :/

This is I guess why it is a good idea to combine this with the
debounce feature if the hardware supports it.

I'll think about some writeup.

Yours,
Linus Walleij

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Tue, Oct 31, 2017 at 2:16 PM, Andrew Lunn <andrew@lunn.ch> wrote:

>> The discussion here is interesting, it is customary for GPIO drivers
>> to implement double-edge detection emulation by swapping the
>> edge detector around like this.
>
> Hi Linus
>
> I was not aware this was customary.
>
>> It might be possible to collect some generic information about
>> this in the Documentation/gpio/driver.txt document.
>
> Yes, i think it should be documented somewhere. Even in the use case
> here, detecting an SD card being inserted/removed, you could get some
> bounce on the microswitch, miss an edge, and be in the wrong state.

Indeed :/

This is I guess why it is a good idea to combine this with the
debounce feature if the hardware supports it.

I'll think about some writeup.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Uwe Kleine-König]

On Tue, Oct 31, 2017 at 02:16:17PM +0100, Andrew Lunn wrote:
> > Patch applied.
> > 
> > The discussion here is interesting, it is customary for GPIO drivers
> > to implement double-edge detection emulation by swapping the
> > edge detector around like this.
> 
> Hi Linus
> 
> I was not aware this was customary.
> 
> > It might be possible to collect some generic information about
> > this in the Documentation/gpio/driver.txt document.
> 
> Yes, i think it should be documented somewhere. Even in the use case
> here, detecting an SD card being inserted/removed, you could get some
> bounce on the microswitch, miss an edge, and be in the wrong state.

Maybe I'm wrong, but I wonder if there could be a set of helper
functions provided by the gpio core that helps implementing this
software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
possible in software) to prevent common mistakes.

First draft:

		disable_irq_nosync(...);
		level = gpio_get(...);
	retry:
		if (level)
			configure_for_falling_edge();
		else
			configure_for_raising_edge();
		postlevel = gpio_get(...);

		if (level != postlevel) {
			mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
			level = postlevel;
			goto retry;
		}

		enable_irq(...); /* this resends the irq */

I think this only looses an event if there is an edge between gpio_get
and the configure_for_${some}_edge and another before postlevel = ...
that make the two events invisible. But I think this is okish, as a
short spike might also be missed by a hw-edge-detector. And compared to
the current code there should be no way to end in a state where we
configured for raising edge and the level is already high.

When the gpio toggles quickly this might keep the cpu busy in an endless
loop, but such a sequence would also block a controller that can trigger
on both edges in hardware. Not sure if breaking the loop at some point
is sensible anyhow. Also calling the irq handlers would be beneficial,
but I don't know if/how this works without (more) racing.

A similar approach would be great to have to "simulate" level sensitive
irqs if the hardware only implements edge logic (which affects
armada-37xx, too, which annoys me).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Uwe Kleine-König]

On Tue, Oct 31, 2017 at 02:16:17PM +0100, Andrew Lunn wrote:
> > Patch applied.
> > 
> > The discussion here is interesting, it is customary for GPIO drivers
> > to implement double-edge detection emulation by swapping the
> > edge detector around like this.
> 
> Hi Linus
> 
> I was not aware this was customary.
> 
> > It might be possible to collect some generic information about
> > this in the Documentation/gpio/driver.txt document.
> 
> Yes, i think it should be documented somewhere. Even in the use case
> here, detecting an SD card being inserted/removed, you could get some
> bounce on the microswitch, miss an edge, and be in the wrong state.

Maybe I'm wrong, but I wonder if there could be a set of helper
functions provided by the gpio core that helps implementing this
software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
possible in software) to prevent common mistakes.

First draft:

		disable_irq_nosync(...);
		level = gpio_get(...);
	retry:
		if (level)
			configure_for_falling_edge();
		else
			configure_for_raising_edge();
		postlevel = gpio_get(...);

		if (level != postlevel) {
			mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
			level = postlevel;
			goto retry;
		}

		enable_irq(...); /* this resends the irq */

I think this only looses an event if there is an edge between gpio_get
and the configure_for_${some}_edge and another before postlevel = ...
that make the two events invisible. But I think this is okish, as a
short spike might also be missed by a hw-edge-detector. And compared to
the current code there should be no way to end in a state where we
configured for raising edge and the level is already high.

When the gpio toggles quickly this might keep the cpu busy in an endless
loop, but such a sequence would also block a controller that can trigger
on both edges in hardware. Not sure if breaking the loop at some point
is sensible anyhow. Also calling the irq handlers would be beneficial,
but I don't know if/how this works without (more) racing.

A similar approach would be great to have to "simulate" level sensitive
irqs if the hardware only implements edge logic (which affects
armada-37xx, too, which annoys me).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Uwe Kleine-König]

On Tue, Oct 31, 2017 at 02:16:17PM +0100, Andrew Lunn wrote:
> > Patch applied.
> > 
> > The discussion here is interesting, it is customary for GPIO drivers
> > to implement double-edge detection emulation by swapping the
> > edge detector around like this.
> 
> Hi Linus
> 
> I was not aware this was customary.
> 
> > It might be possible to collect some generic information about
> > this in the Documentation/gpio/driver.txt document.
> 
> Yes, i think it should be documented somewhere. Even in the use case
> here, detecting an SD card being inserted/removed, you could get some
> bounce on the microswitch, miss an edge, and be in the wrong state.

Maybe I'm wrong, but I wonder if there could be a set of helper
functions provided by the gpio core that helps implementing this
software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
possible in software) to prevent common mistakes.

First draft:

		disable_irq_nosync(...);
		level = gpio_get(...);
	retry:
		if (level)
			configure_for_falling_edge();
		else
			configure_for_raising_edge();
		postlevel = gpio_get(...);

		if (level != postlevel) {
			mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
			level = postlevel;
			goto retry;
		}

		enable_irq(...); /* this resends the irq */

I think this only looses an event if there is an edge between gpio_get
and the configure_for_${some}_edge and another before postlevel = ...
that make the two events invisible. But I think this is okish, as a
short spike might also be missed by a hw-edge-detector. And compared to
the current code there should be no way to end in a state where we
configured for raising edge and the level is already high.

When the gpio toggles quickly this might keep the cpu busy in an endless
loop, but such a sequence would also block a controller that can trigger
on both edges in hardware. Not sure if breaking the loop at some point
is sensible anyhow. Also calling the irq handlers would be beneficial,
but I don't know if/how this works without (more) racing.

A similar approach would be great to have to "simulate" level sensitive
irqs if the hardware only implements edge logic (which affects
armada-37xx, too, which annoys me).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Tue, Mar 20, 2018 at 10:56 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> Maybe I'm wrong, but I wonder if there could be a set of helper
> functions provided by the gpio core that helps implementing this
> software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
> possible in software) to prevent common mistakes.
>
> First draft:
>
>                 disable_irq_nosync(...);
>                 level = gpio_get(...);
>         retry:
>                 if (level)
>                         configure_for_falling_edge();
>                 else
>                         configure_for_raising_edge();
>                 postlevel = gpio_get(...);
>
>                 if (level != postlevel) {
>                         mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
>                         level = postlevel;
>                         goto retry;
>                 }
>
>                 enable_irq(...); /* this resends the irq */
>
> I think this only looses an event if there is an edge between gpio_get
> and the configure_for_${some}_edge and another before postlevel = ...
> that make the two events invisible. But I think this is okish, as a
> short spike might also be missed by a hw-edge-detector. And compared to
> the current code there should be no way to end in a state where we
> configured for raising edge and the level is already high.

This is looking good compared to the solutions people have hacked up.

> When the gpio toggles quickly this might keep the cpu busy in an endless
> loop, but such a sequence would also block a controller that can trigger
> on both edges in hardware. Not sure if breaking the loop at some point
> is sensible anyhow. Also calling the irq handlers would be beneficial,
> but I don't know if/how this works without (more) racing.

What would make sense (if you want a perfect solution) is to enforce
some reasonable debouncing on double edges.

That may seem hard to do since not all HW has debounce.

In the past I had the idea to implement also generic debounce with a timer
in gpiolib, so that gpiod_set_debounce() would never fail, so in effect
to factor the code from drivers/input/keyboard/gpio_keys.c
over to gpiolib so they don't need a fallback at all, and then with
double edges, enforce some debouncing based on HZ.

At one point I tried to bring the debounce code over from the
input driver, but I hit some snag, I don't remember what though.
An optional per-gpiod timer can be created in struct gpio_desc
when needed.

> A similar approach would be great to have to "simulate" level sensitive
> irqs if the hardware only implements edge logic (which affects
> armada-37xx, too, which annoys me).

Yes that would be neat too...

Yours,
Linus Walleij

[PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Tue, Mar 20, 2018 at 10:56 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:

> Maybe I'm wrong, but I wonder if there could be a set of helper
> functions provided by the gpio core that helps implementing this
> software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
> possible in software) to prevent common mistakes.
>
> First draft:
>
>                 disable_irq_nosync(...);
>                 level = gpio_get(...);
>         retry:
>                 if (level)
>                         configure_for_falling_edge();
>                 else
>                         configure_for_raising_edge();
>                 postlevel = gpio_get(...);
>
>                 if (level != postlevel) {
>                         mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
>                         level = postlevel;
>                         goto retry;
>                 }
>
>                 enable_irq(...); /* this resends the irq */
>
> I think this only looses an event if there is an edge between gpio_get
> and the configure_for_${some}_edge and another before postlevel = ...
> that make the two events invisible. But I think this is okish, as a
> short spike might also be missed by a hw-edge-detector. And compared to
> the current code there should be no way to end in a state where we
> configured for raising edge and the level is already high.

This is looking good compared to the solutions people have hacked up.

> When the gpio toggles quickly this might keep the cpu busy in an endless
> loop, but such a sequence would also block a controller that can trigger
> on both edges in hardware. Not sure if breaking the loop at some point
> is sensible anyhow. Also calling the irq handlers would be beneficial,
> but I don't know if/how this works without (more) racing.

What would make sense (if you want a perfect solution) is to enforce
some reasonable debouncing on double edges.

That may seem hard to do since not all HW has debounce.

In the past I had the idea to implement also generic debounce with a timer
in gpiolib, so that gpiod_set_debounce() would never fail, so in effect
to factor the code from drivers/input/keyboard/gpio_keys.c
over to gpiolib so they don't need a fallback at all, and then with
double edges, enforce some debouncing based on HZ.

At one point I tried to bring the debounce code over from the
input driver, but I hit some snag, I don't remember what though.
An optional per-gpiod timer can be created in struct gpio_desc
when needed.

> A similar approach would be great to have to "simulate" level sensitive
> irqs if the hardware only implements edge logic (which affects
> armada-37xx, too, which annoys me).

Yes that would be neat too...

Yours,
Linus Walleij

Re: [PATCH] pinctrl: armada-37xx: Add edge both type gpio irq support

[Linus Walleij]

On Tue, Mar 20, 2018 at 10:56 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> Maybe I'm wrong, but I wonder if there could be a set of helper
> functions provided by the gpio core that helps implementing this
> software simulation of IRQ_TYPE_EDGE_BOTH reliably (i.e. as good as
> possible in software) to prevent common mistakes.
>
> First draft:
>
>                 disable_irq_nosync(...);
>                 level = gpio_get(...);
>         retry:
>                 if (level)
>                         configure_for_falling_edge();
>                 else
>                         configure_for_raising_edge();
>                 postlevel = gpio_get(...);
>
>                 if (level != postlevel) {
>                         mark_irq_pending(); /* something like desc->istate |= IRQS_PENDING */
>                         level = postlevel;
>                         goto retry;
>                 }
>
>                 enable_irq(...); /* this resends the irq */
>
> I think this only looses an event if there is an edge between gpio_get
> and the configure_for_${some}_edge and another before postlevel = ...
> that make the two events invisible. But I think this is okish, as a
> short spike might also be missed by a hw-edge-detector. And compared to
> the current code there should be no way to end in a state where we
> configured for raising edge and the level is already high.

This is looking good compared to the solutions people have hacked up.

> When the gpio toggles quickly this might keep the cpu busy in an endless
> loop, but such a sequence would also block a controller that can trigger
> on both edges in hardware. Not sure if breaking the loop at some point
> is sensible anyhow. Also calling the irq handlers would be beneficial,
> but I don't know if/how this works without (more) racing.

What would make sense (if you want a perfect solution) is to enforce
some reasonable debouncing on double edges.

That may seem hard to do since not all HW has debounce.

In the past I had the idea to implement also generic debounce with a timer
in gpiolib, so that gpiod_set_debounce() would never fail, so in effect
to factor the code from drivers/input/keyboard/gpio_keys.c
over to gpiolib so they don't need a fallback at all, and then with
double edges, enforce some debouncing based on HZ.

At one point I tried to bring the debounce code over from the
input driver, but I hit some snag, I don't remember what though.
An optional per-gpiod timer can be created in struct gpio_desc
when needed.

> A similar approach would be great to have to "simulate" level sensitive
> irqs if the hardware only implements edge logic (which affects
> armada-37xx, too, which annoys me).

Yes that would be neat too...

Yours,
Linus Walleij