All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] gpio/rockchip: Convert to generic_handle_domain_irq()
@ 2022-08-20  9:59 ` Jeffy Chen
  0 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Linus Walleij, Bartosz Golaszewski,
	linux-kernel, linux-gpio, linux-arm-kernel

Follow commit dbd1c54fc820 ("gpio: Bulk conversion to
generic_handle_domain_irq()").

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index e342a6dc4c6c..a98351cd6821 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -333,16 +333,10 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 	pend = readl_relaxed(bank->reg_base + bank->gpio_regs->int_status);
 
 	while (pend) {
-		unsigned int irq, virq;
+		unsigned int irq;
 
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
-		virq = irq_find_mapping(bank->domain, irq);
-
-		if (!virq) {
-			dev_err(bank->dev, "unmapped irq %d\n", irq);
-			continue;
-		}
 
 		dev_dbg(bank->dev, "handling irq %d\n", irq);
 
@@ -377,7 +371,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
 
-		generic_handle_irq(virq);
+		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 1/2] gpio/rockchip: Convert to generic_handle_domain_irq()
@ 2022-08-20  9:59 ` Jeffy Chen
  0 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Linus Walleij, Bartosz Golaszewski,
	linux-kernel, linux-gpio, linux-arm-kernel

Follow commit dbd1c54fc820 ("gpio: Bulk conversion to
generic_handle_domain_irq()").

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index e342a6dc4c6c..a98351cd6821 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -333,16 +333,10 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 	pend = readl_relaxed(bank->reg_base + bank->gpio_regs->int_status);
 
 	while (pend) {
-		unsigned int irq, virq;
+		unsigned int irq;
 
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
-		virq = irq_find_mapping(bank->domain, irq);
-
-		if (!virq) {
-			dev_err(bank->dev, "unmapped irq %d\n", irq);
-			continue;
-		}
 
 		dev_dbg(bank->dev, "handling irq %d\n", irq);
 
@@ -377,7 +371,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
 
-		generic_handle_irq(virq);
+		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 1/2] gpio/rockchip: Convert to generic_handle_domain_irq()
@ 2022-08-20  9:59 ` Jeffy Chen
  0 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Linus Walleij, Bartosz Golaszewski,
	linux-kernel, linux-gpio, linux-arm-kernel

Follow commit dbd1c54fc820 ("gpio: Bulk conversion to
generic_handle_domain_irq()").

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index e342a6dc4c6c..a98351cd6821 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -333,16 +333,10 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 	pend = readl_relaxed(bank->reg_base + bank->gpio_regs->int_status);
 
 	while (pend) {
-		unsigned int irq, virq;
+		unsigned int irq;
 
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
-		virq = irq_find_mapping(bank->domain, irq);
-
-		if (!virq) {
-			dev_err(bank->dev, "unmapped irq %d\n", irq);
-			continue;
-		}
 
 		dev_dbg(bank->dev, "handling irq %d\n", irq);
 
@@ -377,7 +371,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
 
-		generic_handle_irq(virq);
+		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-20  9:59 ` Jeffy Chen
  (?)
@ 2022-08-20  9:59   ` Jeffy Chen
  -1 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Bartosz Golaszewski, Linus Walleij,
	linux-kernel, linux-gpio, linux-arm-kernel

Otherwise the trigger mode might be out-of-sync when a level change
occurred in between.

Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index a98351cd6821..736b4d90f1ca 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
 
-		dev_dbg(bank->dev, "handling irq %d\n", irq);
+		generic_handle_domain_irq(bank->domain, irq);
 
 		/*
 		 * Triggering IRQ on both rising and falling edge
@@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 						     bank->gpio_regs->ext_port);
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
-
-		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-20  9:59   ` Jeffy Chen
  0 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Bartosz Golaszewski, Linus Walleij,
	linux-kernel, linux-gpio, linux-arm-kernel

Otherwise the trigger mode might be out-of-sync when a level change
occurred in between.

Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index a98351cd6821..736b4d90f1ca 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
 
-		dev_dbg(bank->dev, "handling irq %d\n", irq);
+		generic_handle_domain_irq(bank->domain, irq);
 
 		/*
 		 * Triggering IRQ on both rising and falling edge
@@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 						     bank->gpio_regs->ext_port);
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
-
-		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-20  9:59   ` Jeffy Chen
  0 siblings, 0 replies; 21+ messages in thread
From: Jeffy Chen @ 2022-08-20  9:59 UTC (permalink / raw)
  To: Heiko Stuebner, Doug Anderson
  Cc: Jeffy Chen, linux-rockchip, Bartosz Golaszewski, Linus Walleij,
	linux-kernel, linux-gpio, linux-arm-kernel

Otherwise the trigger mode might be out-of-sync when a level change
occurred in between.

Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/gpio/gpio-rockchip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index a98351cd6821..736b4d90f1ca 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 		irq = __ffs(pend);
 		pend &= ~BIT(irq);
 
-		dev_dbg(bank->dev, "handling irq %d\n", irq);
+		generic_handle_domain_irq(bank->domain, irq);
 
 		/*
 		 * Triggering IRQ on both rising and falling edge
@@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
 						     bank->gpio_regs->ext_port);
 			} while ((data & BIT(irq)) != (data_old & BIT(irq)));
 		}
-
-		generic_handle_domain_irq(bank->domain, irq);
 	}
 
 	chained_irq_exit(chip, desc);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-20  9:59   ` Jeffy Chen
  (?)
@ 2022-08-22 17:08     ` Doug Anderson
  -1 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2022-08-22 17:08 UTC (permalink / raw)
  To: Jeffy Chen
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi,

On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>
> Otherwise the trigger mode might be out-of-sync when a level change
> occurred in between.
>
> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  drivers/gpio/gpio-rockchip.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index a98351cd6821..736b4d90f1ca 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                 irq = __ffs(pend);
>                 pend &= ~BIT(irq);
>
> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
> +               generic_handle_domain_irq(bank->domain, irq);
>
>                 /*
>                  * Triggering IRQ on both rising and falling edge
> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                                                      bank->gpio_regs->ext_port);
>                         } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>                 }
> -
> -               generic_handle_domain_irq(bank->domain, irq);

I'm happy to let others say for sure, but from my point of view I'm
not convinced. It feels like with your new code you could lose edges.

The abstraction I always assume for edge triggered interrupts is that
multiple edges are coalesced into one IRQ but that if an edge comes in
after the first line of the IRQ handler starts executing then the IRQ
handler will run again. In other words:

- edge A
- edge B
- IRQ handler starts running (once for A/B)
- IRQ handler finishes running
- <idle>
- edge C
- IRQ handler starts running (for C)
- edge D
- edge E
- IRQ handler finishes running
- IRQ handler starts running (for D/E)
- IRQ handler finishes running
- <idle>

For your new code I don't think that will necessarily be the case. I
think this can happen with your new code:

- rising edge
- IRQ handler starts running for rising edge
- IRQ handler finishes running for rising edge
- falling edge (not latched since we're looking for rising edges)
- notice that level is low
- keep it configured for rising edge

...in other words an edge happened _after_ the IRQ handler ran but we
didn't call the IRQ handler again. I don't think this is right.


What problem are you trying to solve?

-Doug

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-22 17:08     ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2022-08-22 17:08 UTC (permalink / raw)
  To: Jeffy Chen
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi,

On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>
> Otherwise the trigger mode might be out-of-sync when a level change
> occurred in between.
>
> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  drivers/gpio/gpio-rockchip.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index a98351cd6821..736b4d90f1ca 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                 irq = __ffs(pend);
>                 pend &= ~BIT(irq);
>
> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
> +               generic_handle_domain_irq(bank->domain, irq);
>
>                 /*
>                  * Triggering IRQ on both rising and falling edge
> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                                                      bank->gpio_regs->ext_port);
>                         } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>                 }
> -
> -               generic_handle_domain_irq(bank->domain, irq);

I'm happy to let others say for sure, but from my point of view I'm
not convinced. It feels like with your new code you could lose edges.

The abstraction I always assume for edge triggered interrupts is that
multiple edges are coalesced into one IRQ but that if an edge comes in
after the first line of the IRQ handler starts executing then the IRQ
handler will run again. In other words:

- edge A
- edge B
- IRQ handler starts running (once for A/B)
- IRQ handler finishes running
- <idle>
- edge C
- IRQ handler starts running (for C)
- edge D
- edge E
- IRQ handler finishes running
- IRQ handler starts running (for D/E)
- IRQ handler finishes running
- <idle>

For your new code I don't think that will necessarily be the case. I
think this can happen with your new code:

- rising edge
- IRQ handler starts running for rising edge
- IRQ handler finishes running for rising edge
- falling edge (not latched since we're looking for rising edges)
- notice that level is low
- keep it configured for rising edge

...in other words an edge happened _after_ the IRQ handler ran but we
didn't call the IRQ handler again. I don't think this is right.


What problem are you trying to solve?

-Doug

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-22 17:08     ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2022-08-22 17:08 UTC (permalink / raw)
  To: Jeffy Chen
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi,

On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>
> Otherwise the trigger mode might be out-of-sync when a level change
> occurred in between.
>
> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  drivers/gpio/gpio-rockchip.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index a98351cd6821..736b4d90f1ca 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                 irq = __ffs(pend);
>                 pend &= ~BIT(irq);
>
> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
> +               generic_handle_domain_irq(bank->domain, irq);
>
>                 /*
>                  * Triggering IRQ on both rising and falling edge
> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                                                      bank->gpio_regs->ext_port);
>                         } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>                 }
> -
> -               generic_handle_domain_irq(bank->domain, irq);

I'm happy to let others say for sure, but from my point of view I'm
not convinced. It feels like with your new code you could lose edges.

The abstraction I always assume for edge triggered interrupts is that
multiple edges are coalesced into one IRQ but that if an edge comes in
after the first line of the IRQ handler starts executing then the IRQ
handler will run again. In other words:

- edge A
- edge B
- IRQ handler starts running (once for A/B)
- IRQ handler finishes running
- <idle>
- edge C
- IRQ handler starts running (for C)
- edge D
- edge E
- IRQ handler finishes running
- IRQ handler starts running (for D/E)
- IRQ handler finishes running
- <idle>

For your new code I don't think that will necessarily be the case. I
think this can happen with your new code:

- rising edge
- IRQ handler starts running for rising edge
- IRQ handler finishes running for rising edge
- falling edge (not latched since we're looking for rising edges)
- notice that level is low
- keep it configured for rising edge

...in other words an edge happened _after_ the IRQ handler ran but we
didn't call the IRQ handler again. I don't think this is right.


What problem are you trying to solve?

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-22 17:08     ` Doug Anderson
  (?)
@ 2022-08-23  2:50       ` Chen Jeffy
  -1 siblings, 0 replies; 21+ messages in thread
From: Chen Jeffy @ 2022-08-23  2:50 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi Doug,

It's been a long time, hope you're doing well :)

On 8/23 星期二 1:08, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>
>> Otherwise the trigger mode might be out-of-sync when a level change
>> occurred in between.
>>
>> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/gpio/gpio-rockchip.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
>> index a98351cd6821..736b4d90f1ca 100644
>> --- a/drivers/gpio/gpio-rockchip.c
>> +++ b/drivers/gpio/gpio-rockchip.c
>> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                  irq = __ffs(pend);
>>                  pend &= ~BIT(irq);
>>
>> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
>> +               generic_handle_domain_irq(bank->domain, irq);
>>
>>                  /*
>>                   * Triggering IRQ on both rising and falling edge
>> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                                                       bank->gpio_regs->ext_port);
>>                          } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>>                  }
>> -
>> -               generic_handle_domain_irq(bank->domain, irq);
> 
> I'm happy to let others say for sure, but from my point of view I'm
> not convinced. It feels like with your new code you could lose edges.
> 
> The abstraction I always assume for edge triggered interrupts is that
> multiple edges are coalesced into one IRQ but that if an edge comes in
> after the first line of the IRQ handler starts executing then the IRQ
> handler will run again. In other words:
> 
> - edge A
> - edge B
> - IRQ handler starts running (once for A/B)
> - IRQ handler finishes running
> - <idle>
> - edge C
> - IRQ handler starts running (for C)
> - edge D
> - edge E
> - IRQ handler finishes running
> - IRQ handler starts running (for D/E)
> - IRQ handler finishes running
> - <idle>

The thing is, we are currently toggling the trigger mode to make sure it 
matches the current GPIO level (e.g. level low -> rising edge mode), 
than ack it in gpio IRQ handler.

So if an edge come in between, that new IRQ status would be 
acked(cleared) in the following GPIO irq handler as well as the old one, 
without triggering another IRQ demux() to toggle the trigger mode.

- rising edge
- toggle to falling edge mode
- GPIO high with falling edge mode <-- correct
- falling edge
- IRQ handler acked that IRQ
- IRQ handler finished
- GPIO low with falling edge mode <-- oops
- rising edge <-- missed

> 
> For your new code I don't think that will necessarily be the case. I
> think this can happen with your new code:
> 
> - rising edge
> - IRQ handler starts running for rising edge
> - IRQ handler finishes running for rising edge
> - falling edge (not latched since we're looking for rising edges)
> - notice that level is low
> - keep it configured for rising edge
> 
> ...in other words an edge happened _after_ the IRQ handler ran but we
> didn't call the IRQ handler again. I don't think this is right.
> 

Right, so guessing we could somehow move the IRQ ack into the toggling 
flow to make sure that it would not clear the new IRQ?

And it looks like there are quite a few drivers having this kind of 
need, would it make sense to handle it in the framework?

> 
> What problem are you trying to solve?
> 
> -Doug
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-23  2:50       ` Chen Jeffy
  0 siblings, 0 replies; 21+ messages in thread
From: Chen Jeffy @ 2022-08-23  2:50 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi Doug,

It's been a long time, hope you're doing well :)

On 8/23 星期二 1:08, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>
>> Otherwise the trigger mode might be out-of-sync when a level change
>> occurred in between.
>>
>> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/gpio/gpio-rockchip.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
>> index a98351cd6821..736b4d90f1ca 100644
>> --- a/drivers/gpio/gpio-rockchip.c
>> +++ b/drivers/gpio/gpio-rockchip.c
>> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                  irq = __ffs(pend);
>>                  pend &= ~BIT(irq);
>>
>> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
>> +               generic_handle_domain_irq(bank->domain, irq);
>>
>>                  /*
>>                   * Triggering IRQ on both rising and falling edge
>> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                                                       bank->gpio_regs->ext_port);
>>                          } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>>                  }
>> -
>> -               generic_handle_domain_irq(bank->domain, irq);
> 
> I'm happy to let others say for sure, but from my point of view I'm
> not convinced. It feels like with your new code you could lose edges.
> 
> The abstraction I always assume for edge triggered interrupts is that
> multiple edges are coalesced into one IRQ but that if an edge comes in
> after the first line of the IRQ handler starts executing then the IRQ
> handler will run again. In other words:
> 
> - edge A
> - edge B
> - IRQ handler starts running (once for A/B)
> - IRQ handler finishes running
> - <idle>
> - edge C
> - IRQ handler starts running (for C)
> - edge D
> - edge E
> - IRQ handler finishes running
> - IRQ handler starts running (for D/E)
> - IRQ handler finishes running
> - <idle>

The thing is, we are currently toggling the trigger mode to make sure it 
matches the current GPIO level (e.g. level low -> rising edge mode), 
than ack it in gpio IRQ handler.

So if an edge come in between, that new IRQ status would be 
acked(cleared) in the following GPIO irq handler as well as the old one, 
without triggering another IRQ demux() to toggle the trigger mode.

- rising edge
- toggle to falling edge mode
- GPIO high with falling edge mode <-- correct
- falling edge
- IRQ handler acked that IRQ
- IRQ handler finished
- GPIO low with falling edge mode <-- oops
- rising edge <-- missed

> 
> For your new code I don't think that will necessarily be the case. I
> think this can happen with your new code:
> 
> - rising edge
> - IRQ handler starts running for rising edge
> - IRQ handler finishes running for rising edge
> - falling edge (not latched since we're looking for rising edges)
> - notice that level is low
> - keep it configured for rising edge
> 
> ...in other words an edge happened _after_ the IRQ handler ran but we
> didn't call the IRQ handler again. I don't think this is right.
> 

Right, so guessing we could somehow move the IRQ ack into the toggling 
flow to make sure that it would not clear the new IRQ?

And it looks like there are quite a few drivers having this kind of 
need, would it make sense to handle it in the framework?

> 
> What problem are you trying to solve?
> 
> -Doug
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-23  2:50       ` Chen Jeffy
  0 siblings, 0 replies; 21+ messages in thread
From: Chen Jeffy @ 2022-08-23  2:50 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM, Brian Norris

Hi Doug,

It's been a long time, hope you're doing well :)

On 8/23 星期二 1:08, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>>
>> Otherwise the trigger mode might be out-of-sync when a level change
>> occurred in between.
>>
>> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/gpio/gpio-rockchip.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
>> index a98351cd6821..736b4d90f1ca 100644
>> --- a/drivers/gpio/gpio-rockchip.c
>> +++ b/drivers/gpio/gpio-rockchip.c
>> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                  irq = __ffs(pend);
>>                  pend &= ~BIT(irq);
>>
>> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
>> +               generic_handle_domain_irq(bank->domain, irq);
>>
>>                  /*
>>                   * Triggering IRQ on both rising and falling edge
>> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>>                                                       bank->gpio_regs->ext_port);
>>                          } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>>                  }
>> -
>> -               generic_handle_domain_irq(bank->domain, irq);
> 
> I'm happy to let others say for sure, but from my point of view I'm
> not convinced. It feels like with your new code you could lose edges.
> 
> The abstraction I always assume for edge triggered interrupts is that
> multiple edges are coalesced into one IRQ but that if an edge comes in
> after the first line of the IRQ handler starts executing then the IRQ
> handler will run again. In other words:
> 
> - edge A
> - edge B
> - IRQ handler starts running (once for A/B)
> - IRQ handler finishes running
> - <idle>
> - edge C
> - IRQ handler starts running (for C)
> - edge D
> - edge E
> - IRQ handler finishes running
> - IRQ handler starts running (for D/E)
> - IRQ handler finishes running
> - <idle>

The thing is, we are currently toggling the trigger mode to make sure it 
matches the current GPIO level (e.g. level low -> rising edge mode), 
than ack it in gpio IRQ handler.

So if an edge come in between, that new IRQ status would be 
acked(cleared) in the following GPIO irq handler as well as the old one, 
without triggering another IRQ demux() to toggle the trigger mode.

- rising edge
- toggle to falling edge mode
- GPIO high with falling edge mode <-- correct
- falling edge
- IRQ handler acked that IRQ
- IRQ handler finished
- GPIO low with falling edge mode <-- oops
- rising edge <-- missed

> 
> For your new code I don't think that will necessarily be the case. I
> think this can happen with your new code:
> 
> - rising edge
> - IRQ handler starts running for rising edge
> - IRQ handler finishes running for rising edge
> - falling edge (not latched since we're looking for rising edges)
> - notice that level is low
> - keep it configured for rising edge
> 
> ...in other words an edge happened _after_ the IRQ handler ran but we
> didn't call the IRQ handler again. I don't think this is right.
> 

Right, so guessing we could somehow move the IRQ ack into the toggling 
flow to make sure that it would not clear the new IRQ?

And it looks like there are quite a few drivers having this kind of 
need, would it make sense to handle it in the framework?

> 
> What problem are you trying to solve?
> 
> -Doug
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-23  2:50       ` Chen Jeffy
  (?)
@ 2022-08-23 18:50         ` Brian Norris
  -1 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-23 18:50 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM

Hi,

On Tue, Aug 23, 2022 at 10:50:16AM +0800, Jeffy Chen wrote:
> On 8/23 星期二 1:08, Doug Anderson wrote:
> > I'm happy to let others say for sure, but from my point of view I'm
> > not convinced. It feels like with your new code you could lose edges.

I won't claim to know for sure either, but I'm trying to follow along,
because we've dealt with various sorts of nasty logical errors in the
gpio and/or irqchip areas here :)

> > ...in other words an edge happened _after_ the IRQ handler ran but we
> > didn't call the IRQ handler again. I don't think this is right.

I believe Doug has identified one problem correctly -- that we can't
handle the both-edge toggling entirely after the ack+dispatch.

(out-of-order quote:)
> So if an edge come in between, that new IRQ status would be acked(cleared)
> in the following GPIO irq handler as well as the old one, without triggering
> another IRQ demux() to toggle the trigger mode.

I believe Jeffy has identified another -- that we can't handle the
both-edge toggling entirely before the ack+dispatch.

> Right, so guessing we could somehow move the IRQ ack into the toggling flow
> to make sure that it would not clear the new IRQ?

This sounds like we need the toggling to happen within the .irq_ack()
callback, in between the ACK and the chain dispatch.

I'm not sure I've fully walked through this yet, but would it suffice to
move the 'toggle_edge_mode' loop into irq_ack(), right after writing the
EOI register? That way we're in one of these cases within irq_ack():

(1) no further edges occurred after the ACK/EIO write: the toggle-loop
will toggle correctly, and then we dispatch the chained handler.

(2) one or more additional edges occurred after the ACK/EIO write
(possibly during the toggle-loop): the loop only exits when we're sure
things have quiesced, so future edges will toggle correctly (i.e.,
polarity is correct). We continue to dispatch the chained handler (which
"handles" all these edge(s)). We may still have leave an IRQ pending (if
many edges happen, racing with the toggle loop) and fire again shortly,
but that's OK.

Does this make sense to anyone else?

> And it looks like there are quite a few drivers having this kind of need,
> would it make sense to handle it in the framework?

I suppose it would be possible to implement the above as a general
irq_gc_ack*() helper, instead of open-coding it, although it might need
the irqchip framework to gain a better understanding of a chip's
polarity register.

Brian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-23 18:50         ` Brian Norris
  0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-23 18:50 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM

Hi,

On Tue, Aug 23, 2022 at 10:50:16AM +0800, Jeffy Chen wrote:
> On 8/23 星期二 1:08, Doug Anderson wrote:
> > I'm happy to let others say for sure, but from my point of view I'm
> > not convinced. It feels like with your new code you could lose edges.

I won't claim to know for sure either, but I'm trying to follow along,
because we've dealt with various sorts of nasty logical errors in the
gpio and/or irqchip areas here :)

> > ...in other words an edge happened _after_ the IRQ handler ran but we
> > didn't call the IRQ handler again. I don't think this is right.

I believe Doug has identified one problem correctly -- that we can't
handle the both-edge toggling entirely after the ack+dispatch.

(out-of-order quote:)
> So if an edge come in between, that new IRQ status would be acked(cleared)
> in the following GPIO irq handler as well as the old one, without triggering
> another IRQ demux() to toggle the trigger mode.

I believe Jeffy has identified another -- that we can't handle the
both-edge toggling entirely before the ack+dispatch.

> Right, so guessing we could somehow move the IRQ ack into the toggling flow
> to make sure that it would not clear the new IRQ?

This sounds like we need the toggling to happen within the .irq_ack()
callback, in between the ACK and the chain dispatch.

I'm not sure I've fully walked through this yet, but would it suffice to
move the 'toggle_edge_mode' loop into irq_ack(), right after writing the
EOI register? That way we're in one of these cases within irq_ack():

(1) no further edges occurred after the ACK/EIO write: the toggle-loop
will toggle correctly, and then we dispatch the chained handler.

(2) one or more additional edges occurred after the ACK/EIO write
(possibly during the toggle-loop): the loop only exits when we're sure
things have quiesced, so future edges will toggle correctly (i.e.,
polarity is correct). We continue to dispatch the chained handler (which
"handles" all these edge(s)). We may still have leave an IRQ pending (if
many edges happen, racing with the toggle loop) and fire again shortly,
but that's OK.

Does this make sense to anyone else?

> And it looks like there are quite a few drivers having this kind of need,
> would it make sense to handle it in the framework?

I suppose it would be possible to implement the above as a general
irq_gc_ack*() helper, instead of open-coding it, although it might need
the irqchip framework to gain a better understanding of a chip's
polarity register.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-23 18:50         ` Brian Norris
  0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-23 18:50 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, Linus Walleij, LKML,
	open list:GPIO SUBSYSTEM, Linux ARM

Hi,

On Tue, Aug 23, 2022 at 10:50:16AM +0800, Jeffy Chen wrote:
> On 8/23 星期二 1:08, Doug Anderson wrote:
> > I'm happy to let others say for sure, but from my point of view I'm
> > not convinced. It feels like with your new code you could lose edges.

I won't claim to know for sure either, but I'm trying to follow along,
because we've dealt with various sorts of nasty logical errors in the
gpio and/or irqchip areas here :)

> > ...in other words an edge happened _after_ the IRQ handler ran but we
> > didn't call the IRQ handler again. I don't think this is right.

I believe Doug has identified one problem correctly -- that we can't
handle the both-edge toggling entirely after the ack+dispatch.

(out-of-order quote:)
> So if an edge come in between, that new IRQ status would be acked(cleared)
> in the following GPIO irq handler as well as the old one, without triggering
> another IRQ demux() to toggle the trigger mode.

I believe Jeffy has identified another -- that we can't handle the
both-edge toggling entirely before the ack+dispatch.

> Right, so guessing we could somehow move the IRQ ack into the toggling flow
> to make sure that it would not clear the new IRQ?

This sounds like we need the toggling to happen within the .irq_ack()
callback, in between the ACK and the chain dispatch.

I'm not sure I've fully walked through this yet, but would it suffice to
move the 'toggle_edge_mode' loop into irq_ack(), right after writing the
EOI register? That way we're in one of these cases within irq_ack():

(1) no further edges occurred after the ACK/EIO write: the toggle-loop
will toggle correctly, and then we dispatch the chained handler.

(2) one or more additional edges occurred after the ACK/EIO write
(possibly during the toggle-loop): the loop only exits when we're sure
things have quiesced, so future edges will toggle correctly (i.e.,
polarity is correct). We continue to dispatch the chained handler (which
"handles" all these edge(s)). We may still have leave an IRQ pending (if
many edges happen, racing with the toggle loop) and fire again shortly,
but that's OK.

Does this make sense to anyone else?

> And it looks like there are quite a few drivers having this kind of need,
> would it make sense to handle it in the framework?

I suppose it would be possible to implement the above as a general
irq_gc_ack*() helper, instead of open-coding it, although it might need
the irqchip framework to gain a better understanding of a chip's
polarity register.

Brian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-23  2:50       ` Chen Jeffy
  (?)
@ 2022-08-26  8:54         ` Linus Walleij
  -1 siblings, 0 replies; 21+ messages in thread
From: Linus Walleij @ 2022-08-26  8:54 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM,
	Brian Norris

On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:

> The thing is, we are currently toggling the trigger mode to make sure it
> matches the current GPIO level (e.g. level low -> rising edge mode),
> than ack it in gpio IRQ handler.

Yes this is an old trick, I don't know if I invented it again for Linux in
commit cc890cd78acd7ab03442907d354b6af34e973cb3
in 2011, surely the trick must be well known.

Back then I did it like this:

+       val = readl(U300_PIN_REG(offset, icr));
+       /* Set mode depending on state */
+       if (u300_gpio_get(&gpio->chip, offset)) {
+               /* High now, let's trigger on falling edge next then */
+               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
+                       offset);
+       } else {
+               /* Low now, let's trigger on rising edge next then */
+               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
+                       offset);
+       }

Notice that I read the current level of the raw input to decide what the next
trigger should be. The Rockchip driver does not do this, maybe that works
better?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-26  8:54         ` Linus Walleij
  0 siblings, 0 replies; 21+ messages in thread
From: Linus Walleij @ 2022-08-26  8:54 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM,
	Brian Norris

On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:

> The thing is, we are currently toggling the trigger mode to make sure it
> matches the current GPIO level (e.g. level low -> rising edge mode),
> than ack it in gpio IRQ handler.

Yes this is an old trick, I don't know if I invented it again for Linux in
commit cc890cd78acd7ab03442907d354b6af34e973cb3
in 2011, surely the trick must be well known.

Back then I did it like this:

+       val = readl(U300_PIN_REG(offset, icr));
+       /* Set mode depending on state */
+       if (u300_gpio_get(&gpio->chip, offset)) {
+               /* High now, let's trigger on falling edge next then */
+               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
+                       offset);
+       } else {
+               /* Low now, let's trigger on rising edge next then */
+               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
+                       offset);
+       }

Notice that I read the current level of the raw input to decide what the next
trigger should be. The Rockchip driver does not do this, maybe that works
better?

Yours,
Linus Walleij

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-26  8:54         ` Linus Walleij
  0 siblings, 0 replies; 21+ messages in thread
From: Linus Walleij @ 2022-08-26  8:54 UTC (permalink / raw)
  To: Chen Jeffy
  Cc: Doug Anderson, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM,
	Brian Norris

On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:

> The thing is, we are currently toggling the trigger mode to make sure it
> matches the current GPIO level (e.g. level low -> rising edge mode),
> than ack it in gpio IRQ handler.

Yes this is an old trick, I don't know if I invented it again for Linux in
commit cc890cd78acd7ab03442907d354b6af34e973cb3
in 2011, surely the trick must be well known.

Back then I did it like this:

+       val = readl(U300_PIN_REG(offset, icr));
+       /* Set mode depending on state */
+       if (u300_gpio_get(&gpio->chip, offset)) {
+               /* High now, let's trigger on falling edge next then */
+               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
+                       offset);
+       } else {
+               /* Low now, let's trigger on rising edge next then */
+               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
+               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
+                       offset);
+       }

Notice that I read the current level of the raw input to decide what the next
trigger should be. The Rockchip driver does not do this, maybe that works
better?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
  2022-08-26  8:54         ` Linus Walleij
  (?)
@ 2022-08-29 23:26           ` Brian Norris
  -1 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-29 23:26 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Chen Jeffy, Doug Anderson, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM

Hi Linus,

On Fri, Aug 26, 2022 at 10:54:08AM +0200, Linus Walleij wrote:
> On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:
> 
> > The thing is, we are currently toggling the trigger mode to make sure it
> > matches the current GPIO level (e.g. level low -> rising edge mode),
> > than ack it in gpio IRQ handler.
> 
> Yes this is an old trick, I don't know if I invented it again for Linux in
> commit cc890cd78acd7ab03442907d354b6af34e973cb3
> in 2011, surely the trick must be well known.
> 
> Back then I did it like this:
> 
> +       val = readl(U300_PIN_REG(offset, icr));
> +       /* Set mode depending on state */
> +       if (u300_gpio_get(&gpio->chip, offset)) {
> +               /* High now, let's trigger on falling edge next then */
> +               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));

This looks pretty close to the right solution, but you still have a
race: a falling edge may happen right between the get() and the
writel(), and you'll miss it. You're in a little better shape than any
of the rockchip proposals so far, because at least you do the ACK before
this step (Rockchip doesn't). But you do still have a narrow race
window.

I think you need to iterate on this, and check the status again
afterward, looping until you are sure you either got it right, or are
leaving an edge still latched.

And at that point, I think you have the solution I recommended in my
other email :)

> +               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
> +                       offset);
> +       } else {
> +               /* Low now, let's trigger on rising edge next then */
> +               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
> +               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
> +                       offset);
> +       }
> 
> Notice that I read the current level of the raw input to decide what the next
> trigger should be. The Rockchip driver does not do this, maybe that works
> better?

Actually, it does; that's what the 'ext_port' register is doing. The
question is when exactly to do this toggling -- before or after the ACK,
and with what sort of ordering to ensure you don't end up at the wrong
polarity when exiting.

Brian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-29 23:26           ` Brian Norris
  0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-29 23:26 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Chen Jeffy, Doug Anderson, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM

Hi Linus,

On Fri, Aug 26, 2022 at 10:54:08AM +0200, Linus Walleij wrote:
> On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:
> 
> > The thing is, we are currently toggling the trigger mode to make sure it
> > matches the current GPIO level (e.g. level low -> rising edge mode),
> > than ack it in gpio IRQ handler.
> 
> Yes this is an old trick, I don't know if I invented it again for Linux in
> commit cc890cd78acd7ab03442907d354b6af34e973cb3
> in 2011, surely the trick must be well known.
> 
> Back then I did it like this:
> 
> +       val = readl(U300_PIN_REG(offset, icr));
> +       /* Set mode depending on state */
> +       if (u300_gpio_get(&gpio->chip, offset)) {
> +               /* High now, let's trigger on falling edge next then */
> +               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));

This looks pretty close to the right solution, but you still have a
race: a falling edge may happen right between the get() and the
writel(), and you'll miss it. You're in a little better shape than any
of the rockchip proposals so far, because at least you do the ACK before
this step (Rockchip doesn't). But you do still have a narrow race
window.

I think you need to iterate on this, and check the status again
afterward, looping until you are sure you either got it right, or are
leaving an edge still latched.

And at that point, I think you have the solution I recommended in my
other email :)

> +               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
> +                       offset);
> +       } else {
> +               /* Low now, let's trigger on rising edge next then */
> +               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
> +               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
> +                       offset);
> +       }
> 
> Notice that I read the current level of the raw input to decide what the next
> trigger should be. The Rockchip driver does not do this, maybe that works
> better?

Actually, it does; that's what the 'ext_port' register is doing. The
question is when exactly to do this toggling -- before or after the ACK,
and with what sort of ordering to ensure you don't end up at the wrong
polarity when exiting.

Brian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
@ 2022-08-29 23:26           ` Brian Norris
  0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2022-08-29 23:26 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Chen Jeffy, Doug Anderson, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Bartosz Golaszewski, LKML, open list:GPIO SUBSYSTEM, Linux ARM

Hi Linus,

On Fri, Aug 26, 2022 at 10:54:08AM +0200, Linus Walleij wrote:
> On Tue, Aug 23, 2022 at 4:50 AM Chen Jeffy <jeffy.chen@rock-chips.com> wrote:
> 
> > The thing is, we are currently toggling the trigger mode to make sure it
> > matches the current GPIO level (e.g. level low -> rising edge mode),
> > than ack it in gpio IRQ handler.
> 
> Yes this is an old trick, I don't know if I invented it again for Linux in
> commit cc890cd78acd7ab03442907d354b6af34e973cb3
> in 2011, surely the trick must be well known.
> 
> Back then I did it like this:
> 
> +       val = readl(U300_PIN_REG(offset, icr));
> +       /* Set mode depending on state */
> +       if (u300_gpio_get(&gpio->chip, offset)) {
> +               /* High now, let's trigger on falling edge next then */
> +               writel(val & ~U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));

This looks pretty close to the right solution, but you still have a
race: a falling edge may happen right between the get() and the
writel(), and you'll miss it. You're in a little better shape than any
of the rockchip proposals so far, because at least you do the ACK before
this step (Rockchip doesn't). But you do still have a narrow race
window.

I think you need to iterate on this, and check the status again
afterward, looping until you are sure you either got it right, or are
leaving an edge still latched.

And at that point, I think you have the solution I recommended in my
other email :)

> +               dev_dbg(gpio->dev, "next IRQ on falling edge on pin %d\n",
> +                       offset);
> +       } else {
> +               /* Low now, let's trigger on rising edge next then */
> +               writel(val | U300_PIN_BIT(offset), U300_PIN_REG(offset, icr));
> +               dev_dbg(gpio->dev, "next IRQ on rising edge on pin %d\n",
> +                       offset);
> +       }
> 
> Notice that I read the current level of the raw input to decide what the next
> trigger should be. The Rockchip driver does not do this, maybe that works
> better?

Actually, it does; that's what the 'ext_port' register is doing. The
question is when exactly to do this toggling -- before or after the ACK,
and with what sort of ordering to ensure you don't end up at the wrong
polarity when exiting.

Brian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-08-29 23:27 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-20  9:59 [PATCH 1/2] gpio/rockchip: Convert to generic_handle_domain_irq() Jeffy Chen
2022-08-20  9:59 ` Jeffy Chen
2022-08-20  9:59 ` Jeffy Chen
2022-08-20  9:59 ` [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking Jeffy Chen
2022-08-20  9:59   ` Jeffy Chen
2022-08-20  9:59   ` Jeffy Chen
2022-08-22 17:08   ` Doug Anderson
2022-08-22 17:08     ` Doug Anderson
2022-08-22 17:08     ` Doug Anderson
2022-08-23  2:50     ` Chen Jeffy
2022-08-23  2:50       ` Chen Jeffy
2022-08-23  2:50       ` Chen Jeffy
2022-08-23 18:50       ` Brian Norris
2022-08-23 18:50         ` Brian Norris
2022-08-23 18:50         ` Brian Norris
2022-08-26  8:54       ` Linus Walleij
2022-08-26  8:54         ` Linus Walleij
2022-08-26  8:54         ` Linus Walleij
2022-08-29 23:26         ` Brian Norris
2022-08-29 23:26           ` Brian Norris
2022-08-29 23:26           ` Brian Norris

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.