[PATCH 0/2] Fix for GPIO interrupts

[PATCH 0/2] Fix for GPIO interrupts

[Biju Das]

The following issues observed while adding IRQ support for RTC.
 * The driver is not following reverse order of enable() for disable().
 * The driver is not following as per hardware manual for setting
   GPIO interrupts. As per HW manual we need to set noise filter
   for GPIO interrupts.

This patch series fixes those issues.

Biju Das (2):
  pinctrl: renesas: rzg2l: Make reverse order of enable() for disable()
  pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] pinctrl: renesas: rzg2l: Make reverse order of enable() for disable()

[Biju Das]

We usually do reverse order of enable() for disable(). Currently, the
ordering of irq_chip_disable_parent() is not correct in
rzg2l_gpio_irq_disable(). Fix the incorrect order.

Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 37cdfe4b04f9..2ea6ef99cc70 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -1175,6 +1175,8 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
 	u32 port;
 	u8 bit;
 
+	irq_chip_disable_parent(d);
+
 	port = RZG2L_PIN_ID_TO_PORT(hwirq);
 	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
 
@@ -1189,7 +1191,6 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
 	spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	gpiochip_disable_irq(gc, hwirq);
-	irq_chip_disable_parent(d);
 }
 
 static void rzg2l_gpio_irq_enable(struct irq_data *d)
-- 
2.25.1

Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Make reverse order of enable() for disable()

[Geert Uytterhoeven]

On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> We usually do reverse order of enable() for disable(). Currently, the
> ordering of irq_chip_disable_parent() is not correct in
> rzg2l_gpio_irq_disable(). Fix the incorrect order.
>
> Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-pinctrl-for-v6.7.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt (TINT)
and 41.4.1 Operation for GPIO function, we need to set digital noise
filter for GPIO interrupt.

This patch enables noise filter for GPIO interrupt in
rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().

Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 2ea6ef99cc70..6d3fa962ac97 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -96,6 +96,7 @@
 #define PIN(n)			(0x0800 + 0x10 + (n))
 #define IOLH(n)			(0x1000 + (n) * 8)
 #define IEN(n)			(0x1800 + (n) * 8)
+#define FILONOFF(n)		(0x2080 + (n) * 8)
 #define ISEL(n)			(0x2c80 + (n) * 8)
 #define PWPR			(0x3014)
 #define SD_CH(n)		(0x3000 + (n) * 4)
@@ -1169,9 +1170,9 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
+	void __iomem *addr, *noise_filter_addr;
 	unsigned int hwirq = irqd_to_hwirq(d);
 	unsigned long flags;
-	void __iomem *addr;
 	u32 port;
 	u8 bit;
 
@@ -1181,12 +1182,15 @@ static void rzg2l_gpio_irq_disable(struct irq_data *d)
 	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
 
 	addr = pctrl->base + ISEL(port);
+	noise_filter_addr = pctrl->base + FILONOFF(port);
 	if (bit >= 4) {
 		bit -= 4;
 		addr += 4;
+		noise_filter_addr += 4;
 	}
 
 	spin_lock_irqsave(&pctrl->lock, flags);
+	writel(readl(noise_filter_addr) & ~BIT(bit * 8), noise_filter_addr);
 	writel(readl(addr) & ~BIT(bit * 8), addr);
 	spin_unlock_irqrestore(&pctrl->lock, flags);
 
@@ -1197,9 +1201,9 @@ static void rzg2l_gpio_irq_enable(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
+	void __iomem *addr, *noise_filter_addr;
 	unsigned int hwirq = irqd_to_hwirq(d);
 	unsigned long flags;
-	void __iomem *addr;
 	u32 port;
 	u8 bit;
 
@@ -1209,13 +1213,16 @@ static void rzg2l_gpio_irq_enable(struct irq_data *d)
 	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
 
 	addr = pctrl->base + ISEL(port);
+	noise_filter_addr = pctrl->base + FILONOFF(port);
 	if (bit >= 4) {
 		bit -= 4;
 		addr += 4;
+		noise_filter_addr += 4;
 	}
 
 	spin_lock_irqsave(&pctrl->lock, flags);
 	writel(readl(addr) | BIT(bit * 8), addr);
+	writel(readl(noise_filter_addr) | BIT(bit * 8), noise_filter_addr);
 	spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	irq_chip_enable_parent(d);
-- 
2.25.1

Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Geert Uytterhoeven]

Hi Biju,

On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt (TINT)
> and 41.4.1 Operation for GPIO function, we need to set digital noise
> filter for GPIO interrupt.
>
> This patch enables noise filter for GPIO interrupt in
> rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
>
> Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -96,6 +96,7 @@
>  #define PIN(n)                 (0x0800 + 0x10 + (n))
>  #define IOLH(n)                        (0x1000 + (n) * 8)
>  #define IEN(n)                 (0x1800 + (n) * 8)
> +#define FILONOFF(n)            (0x2080 + (n) * 8)
>  #define ISEL(n)                        (0x2c80 + (n) * 8)
>  #define PWPR                   (0x3014)
>  #define SD_CH(n)               (0x3000 + (n) * 4)

LGTM, but shouldn't you configure the Digital Noise Filter Number
(FILNUM) and Clock Selection (FILCLKSEL) registers, too?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> Hi Biju,
> 
> On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt
> > (TINT) and 41.4.1 Operation for GPIO function, we need to set digital
> > noise filter for GPIO interrupt.
> >
> > This patch enables noise filter for GPIO interrupt in
> > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> >
> > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > to handle GPIO interrupt")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -96,6 +96,7 @@
> >  #define PIN(n)                 (0x0800 + 0x10 + (n))
> >  #define IOLH(n)                        (0x1000 + (n) * 8)
> >  #define IEN(n)                 (0x1800 + (n) * 8)
> > +#define FILONOFF(n)            (0x2080 + (n) * 8)
> >  #define ISEL(n)                        (0x2c80 + (n) * 8)
> >  #define PWPR                   (0x3014)
> >  #define SD_CH(n)               (0x3000 + (n) * 4)
> 
> LGTM, but shouldn't you configure the Digital Noise Filter Number
> (FILNUM) and Clock Selection (FILCLKSEL) registers, too?

Currently it uses reset values.

00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value) for FILNUM and

00b: Not divided (initial value) for FILCLKSEL

Do you mean we should provide these settings to DT, so that
it is customised based on the PCB design and the environment
the board is used in? I guess this will make it easier for
customers to make the required changes for their application.

Cheers,
Biju

Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Geert Uytterhoeven]

Hi Biju,

On Mon, Sep 18, 2023 at 3:18 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> > GPIO interrupt input
> >
> > On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt
> > > (TINT) and 41.4.1 Operation for GPIO function, we need to set digital
> > > noise filter for GPIO interrupt.
> > >
> > > This patch enables noise filter for GPIO interrupt in
> > > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> > >
> > > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > > to handle GPIO interrupt")
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > @@ -96,6 +96,7 @@
> > >  #define PIN(n)                 (0x0800 + 0x10 + (n))
> > >  #define IOLH(n)                        (0x1000 + (n) * 8)
> > >  #define IEN(n)                 (0x1800 + (n) * 8)
> > > +#define FILONOFF(n)            (0x2080 + (n) * 8)
> > >  #define ISEL(n)                        (0x2c80 + (n) * 8)
> > >  #define PWPR                   (0x3014)
> > >  #define SD_CH(n)               (0x3000 + (n) * 4)
> >
> > LGTM, but shouldn't you configure the Digital Noise Filter Number
> > (FILNUM) and Clock Selection (FILCLKSEL) registers, too?
>
> Currently it uses reset values.
>
> 00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value) for FILNUM and
>
> 00b: Not divided (initial value) for FILCLKSEL
>
> Do you mean we should provide these settings to DT, so that
> it is customised based on the PCB design and the environment
> the board is used in? I guess this will make it easier for
> customers to make the required changes for their application.

If the optimal values are board-dependent, you should indeed add
a way to configure this from DT.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Geert,

> Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> Hi Biju,
> 
> On Mon, Sep 18, 2023 at 3:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise
> > > filter for GPIO interrupt input
> > >
> > > On Mon, Sep 18, 2023 at 2:34 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO
> > > > Interrupt
> > > > (TINT) and 41.4.1 Operation for GPIO function, we need to set
> > > > digital noise filter for GPIO interrupt.
> > > >
> > > > This patch enables noise filter for GPIO interrupt in
> > > > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> > > >
> > > > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > domain to handle GPIO interrupt")
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > @@ -96,6 +96,7 @@
> > > >  #define PIN(n)                 (0x0800 + 0x10 + (n))
> > > >  #define IOLH(n)                        (0x1000 + (n) * 8)
> > > >  #define IEN(n)                 (0x1800 + (n) * 8)
> > > > +#define FILONOFF(n)            (0x2080 + (n) * 8)
> > > >  #define ISEL(n)                        (0x2c80 + (n) * 8)
> > > >  #define PWPR                   (0x3014)
> > > >  #define SD_CH(n)               (0x3000 + (n) * 4)
> > >
> > > LGTM, but shouldn't you configure the Digital Noise Filter Number
> > > (FILNUM) and Clock Selection (FILCLKSEL) registers, too?
> >
> > Currently it uses reset values.
> >
> > 00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value) for
> > FILNUM and
> >
> > 00b: Not divided (initial value) for FILCLKSEL
> >
> > Do you mean we should provide these settings to DT, so that it is
> > customised based on the PCB design and the environment the board is
> > used in? I guess this will make it easier for customers to make the
> > required changes for their application.
> 
> If the optimal values are board-dependent, you should indeed add a way to
> configure this from DT.

Looks like the above values are signal based. I am checking with HW person about this. I will confirm once I have update on this.

Cheers,
Biju

RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Geert Uytterhoeven,

> Subject: RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> Hi Geert,
> 
> > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter
> > for GPIO interrupt input
> >
> > Hi Biju,
> >
> > On Mon, Sep 18, 2023 at 3:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise
> > > > filter for GPIO interrupt input
> > > >
> > > > On Mon, Sep 18, 2023 at 2:34 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO
> > > > > Interrupt
> > > > > (TINT) and 41.4.1 Operation for GPIO function, we need to set
> > > > > digital noise filter for GPIO interrupt.
> > > > >
> > > > > This patch enables noise filter for GPIO interrupt in
> > > > > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> > > > >
> > > > > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ
> > > > > domain to handle GPIO interrupt")
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > > @@ -96,6 +96,7 @@
> > > > >  #define PIN(n)                 (0x0800 + 0x10 + (n))
> > > > >  #define IOLH(n)                        (0x1000 + (n) * 8)
> > > > >  #define IEN(n)                 (0x1800 + (n) * 8)
> > > > > +#define FILONOFF(n)            (0x2080 + (n) * 8)
> > > > >  #define ISEL(n)                        (0x2c80 + (n) * 8)
> > > > >  #define PWPR                   (0x3014)
> > > > >  #define SD_CH(n)               (0x3000 + (n) * 4)
> > > >
> > > > LGTM, but shouldn't you configure the Digital Noise Filter Number
> > > > (FILNUM) and Clock Selection (FILCLKSEL) registers, too?
> > >
> > > Currently it uses reset values.
> > >
> > > 00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value) for
> > > FILNUM and
> > >
> > > 00b: Not divided (initial value) for FILCLKSEL
> > >
> > > Do you mean we should provide these settings to DT, so that it is
> > > customised based on the PCB design and the environment the board is
> > > used in? I guess this will make it easier for customers to make the
> > > required changes for their application.
> >
> > If the optimal values are board-dependent, you should indeed add a way
> > to configure this from DT.
> 
> Looks like the above values are signal based. I am checking with HW person
> about this. I will confirm once I have update on this.

I rechecked this,

As per section 8.7.3 GPIO Interrupt (TINT), It is not a must. But we should have the flexibility to configure through DT.

 "The noise filter function of GPIO pins assigned to TINT can be enabled by GPIO register setting."

The confusion came because of the section "41.4.1 Operation for GPIO Function"

● Set the FILONOFF register when using GPIO to set a digital noise filter for interrupt input and GPIO input.

So I would like to drop this patch and will send separate patches
for configuring it through DT, if anyone plans to use digital noise
filters.

Cheers,
Biju

Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Linus Walleij]

On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt (TINT)
> and 41.4.1 Operation for GPIO function, we need to set digital noise
> filter for GPIO interrupt.
>
> This patch enables noise filter for GPIO interrupt in
> rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
>
> Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Why is this called "digital noise filter" when the rest of the world calls
it "debounce"? It is a bit confusing to have differing terminology in the
kernel.

(There might be a good reason, such that the noise in question is very
different from mechanical button bounce noise.)

Is this only applicable to interrupt lines or also to GPIO lines in
general? Because I just want to ascertain that in that case
the existing binding for @PIN_CONFIG_INPUT_DEBOUNCE
and the corresponding backend call from gpio_chip() maps
to the same feature as well.

It seems hard-coded for some pins in the Renesas driver right
now but the day you make this configurable from DT please
call it debounce, if the usecase is similar.

Yours,
Linus Walleij

RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Linus Walleij,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> On Mon, Sep 18, 2023 at 2:34 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> 
> > As per RZ/G2L hardware manual Rev.1.30 section 8.7.3 GPIO Interrupt
> > (TINT) and 41.4.1 Operation for GPIO function, we need to set digital
> > noise filter for GPIO interrupt.
> >
> > This patch enables noise filter for GPIO interrupt in
> > rzg2l_gpio_irq_enable() and disable it in rzg2l_gpio_irq_disable().
> >
> > Fixes: db2e5f21a48e ("pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain
> > to handle GPIO interrupt")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Why is this called "digital noise filter" when the rest of the world calls
> it "debounce"? It is a bit confusing to have differing terminology in the
> kernel.
> 
> (There might be a good reason, such that the noise in question is very
> different from mechanical button bounce noise.)

It is almost same.

> 
> Is this only applicable to interrupt lines or also to GPIO lines in
> general? Because I just want to ascertain that in that case the existing
> binding for @PIN_CONFIG_INPUT_DEBOUNCE and the corresponding backend call
> from gpio_chip() maps to the same feature as well.
> 
> It seems hard-coded for some pins in the Renesas driver right now but the
> day you make this configurable from DT please call it debounce, if the
> usecase is similar.

Yes, the usecase is similar and we can use PIN_CONFIG_INPUT_DEBOUNCE
for enabling digital noise filter.

But apart from this, I need to add 2 more configurable properties
associated with DT for PIN_CONFIG_INPUT_DEBOUNCE.

1) Digital Noise Filter Number Register (FILNUM) 

This register sets the number of FF stages of digital noise filter for GPIO pin and NMI pin.

00b: 4-stage filter (41.666 ns x 4 = 166.666 ns) (initial value)
01b: 8-stage filter (41.666 ns x 8 = 333.333 ns)
10b: 12-stage filter (41.666 ns x 12 = 500 ns)
11b: 16-stage filter (41.666 ns x 16 = 666.666 ns)
Note: This value is the value when the external clock is 24MHz.


2) Digital Noise Filter Clock Selection Register (FILCLKSEL):

The FILCLKSEL register selects the divided clock to be input to digital noise filters.

00b: Not divided (initial value)
01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns)
10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns)
11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns)
Note: This value is the value when the external clock is 24MHz.

Q1) What is the recommended way to associate the above values with
    PIN_CONFIG_INPUT_DEBOUNCE?

Eg: I need to configure filter on, 8 stage filter , a divisor of 18000 for a mechanical button bounce noise.

Cheers,
Biju

Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Linus Walleij]

On Wed, Sep 20, 2023 at 2:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> 2) Digital Noise Filter Clock Selection Register (FILCLKSEL):
>
> The FILCLKSEL register selects the divided clock to be input to digital noise filters.
>
> 00b: Not divided (initial value)
> 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns)
> 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns)
> 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns)
> Note: This value is the value when the external clock is 24MHz.
>
> Q1) What is the recommended way to associate the above values with
>     PIN_CONFIG_INPUT_DEBOUNCE?
>
> Eg: I need to configure filter on, 8 stage filter , a divisor of 18000 for a mechanical button bounce noise.

As per the generic pin config and DT bindings:

 * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
 *      which means it will wait for signals to settle when reading inputs. The
 *      argument gives the debounce time in usecs. Setting the
 *      argument to zero turns debouncing off.

  input-debounce:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: Takes the debounce time in usec as argument or 0 to disable
      debouncing

The recommended way is to pass the desired clock cycle in microseconds as the
argument to the pin config.

Yours,
Linus Walleij

RE: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Biju Das]

Hi Linus Walleij,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for
> GPIO interrupt input
> 
> On Wed, Sep 20, 2023 at 2:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> 
> > 2) Digital Noise Filter Clock Selection Register (FILCLKSEL):
> >
> > The FILCLKSEL register selects the divided clock to be input to digital
> noise filters.
> >
> > 00b: Not divided (initial value)
> > 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns)
> > 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns)
> > 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns)
> > Note: This value is the value when the external clock is 24MHz.
> >
> > Q1) What is the recommended way to associate the above values with
> >     PIN_CONFIG_INPUT_DEBOUNCE?
> >
> > Eg: I need to configure filter on, 8 stage filter , a divisor of 18000
> for a mechanical button bounce noise.
> 
> As per the generic pin config and DT bindings:
> 
>  * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce
> mode,
>  *      which means it will wait for signals to settle when reading inputs.
> The
>  *      argument gives the debounce time in usecs. Setting the
>  *      argument to zero turns debouncing off.
> 
>   input-debounce:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     description: Takes the debounce time in usec as argument or 0 to
> disable
>       debouncing
> 
> The recommended way is to pass the desired clock cycle in microseconds as
> the argument to the pin config.

How to add number of FF stages info on top of this clock cycle?

Use some encoding in DT and decode it in driver??

Cheers,
Biju

Re: [PATCH 2/2] pinctrl: renesas: rzg2l: Enable noise filter for GPIO interrupt input

[Linus Walleij]

On Wed, Sep 20, 2023 at 4:06 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> > > 2) Digital Noise Filter Clock Selection Register (FILCLKSEL):
> > >
> > > The FILCLKSEL register selects the divided clock to be input to digital
> > noise filters.
> > >
> > > 00b: Not divided (initial value)
> > > 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns)
> > > 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns)
> > > 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns)
> > > Note: This value is the value when the external clock is 24MHz.
> > >
> > > Q1) What is the recommended way to associate the above values with
> > >     PIN_CONFIG_INPUT_DEBOUNCE?
> > >
> > > Eg: I need to configure filter on, 8 stage filter , a divisor of 18000
> > for a mechanical button bounce noise.
> >
> > As per the generic pin config and DT bindings:
> >
> >  * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce
> > mode,
> >  *      which means it will wait for signals to settle when reading inputs.
> > The
> >  *      argument gives the debounce time in usecs. Setting the
> >  *      argument to zero turns debouncing off.
> >
> >   input-debounce:
> >     $ref: /schemas/types.yaml#/definitions/uint32
> >     description: Takes the debounce time in usec as argument or 0 to
> > disable
> >       debouncing
> >
> > The recommended way is to pass the desired clock cycle in microseconds as
> > the argument to the pin config.
>
> How to add number of FF stages info on top of this clock cycle?

You don't, look below.

> Use some encoding in DT and decode it in driver??

No you put in the desired delay in microseconds, the  you use
the table that you already provided to look up the appropriate
divisor:

> 00b: Not divided (initial value)
> 01b: Divided by 9000 (41.666 ns x 9000 = 375,000 ns)
> 10b: Divided by 18000 (41.666 ns x 18000 = 750,000 ns)
> 11b: Divided by 36000 (41.666 ns x 36000 = 1,500,000 ns)

For 0, 375, 750 and 1500 us...

> Note: This value is the value when the external clock is 24MHz.

And this can be provided from the clock framework, and then
you need some more elaborate math to calculate the right
divisor from the parameter in the device tree.

Yours,
Linus Walleij